mandoline sandbox: prewarm libraries before we raise the sandbox.

mandoline sandbox: prewarm libraries before we raise the sandbox.

That was quick.

This moves back to a world where we don't perform dlopen() in the
sandbox. As is, we actually load other shared objects from our build
directories (html_viewer.mojo depends on libmedia_library.so, for
example), and the useage of things like base::SysInfo is too pervasive
throughout chrome to reasonably not so this.

BUG=492524
Committed: https://crrev.com/25d8872d321ede78e50868d362eb2f278ebe0bf0
Cr-Commit-Position: refs/heads/master@{#340989}

[Elliot Glaysher, 2015-07-29 17:26:21]

erg@chromium.org changed reviewers:
+ jam@chromium.org, jln@chromium.org

[Elliot Glaysher, 2015-07-29 17:36:38]

Things I found after landing the non-warming patch yesterday:

- Our build system makes shared object dependencies...meaning html_viewer.mojo
doesn't (couldn't?) load correctly since it depends on other shared objects
which are in a variable path.

- ICU wants access to a whole bunch of /usr/share, which I assume is for
timezone info given the function names in the stack.

- base::SysInfo is used throughout our codebase. I started writing a mojo
service which just proxied the information about the machine. This works, but
getting usage to all call sites is scary.

- The network service wants access to ~/.pki/nssdb/; Hopefully when BoringSSL
gets X509 support, it'll have a file proxying interface instead.

https://codereview.chromium.org/1264463005/diff/1/mandoline/app/desktop/main.cc
File mandoline/app/desktop/main.cc (right):

https://codereview.chromium.org/1264463005/diff/1/mandoline/app/desktop/main....
mandoline/app/desktop/main.cc:40: cleanup);
Fun fact: app cleanup was also broken in the previous patch. When we were trying
to run LoadNativeApplication() inside the sandbox and had read only access to
the path where the .mojo lives, we couldn't delete the shared object afterwards.

Not that anyone cares because we don't support ephemeral mojo apps yet (and
won't for a very long time), but I thought this was interesting.

[jam, 2015-07-29 20:16:08]

lgtm

[jln (very slow on Chromium), 2015-07-29 20:28:04]

Quick lgtm, but some suggestion for refactor (but can happen in another CL).

https://codereview.chromium.org/1264463005/diff/120001/mandoline/services/cor...
File mandoline/services/core_services/main.cc (right):

https://codereview.chromium.org/1264463005/diff/120001/mandoline/services/cor...
mandoline/services/core_services/main.cc:18: base::RandUint64();
Do we still want to do that individually even though you already do it globally?

https://codereview.chromium.org/1264463005/diff/120001/mojo/runner/child_proc...
File mojo/runner/child_process.cc (right):

https://codereview.chromium.org/1264463005/diff/120001/mojo/runner/child_proc...
mojo/runner/child_process.cc:314: 
The contract for "MojoSanboxWarm" should be:

1. Don't start threads
2. Don't have any directory file descriptor open

Could you do:

ThreadHelpers::AssertSingleThreaded(sandbox-proc_fd());
CHECK(!ProcUtil::HasOpenDirectory(sandbox-proc_fd());

after running sandbox_warm?

Also it seems to be like at least part of l297-l321 should be in the sandbox
class, no? That way, you wouldn't even need to add a proc_fd() accessor.

One simple way to refactor: put l302 to l314 in its own function that returns a
callback. l315 to l320 can move to the sandbox class. It would run that clalback
first and then call the sanity checks I mentioned above.

[Elliot Glaysher, 2015-07-29 21:08:32]

https://codereview.chromium.org/1264463005/diff/120001/mandoline/services/cor...
File mandoline/services/core_services/main.cc (right):

https://codereview.chromium.org/1264463005/diff/120001/mandoline/services/cor...
mandoline/services/core_services/main.cc:18: base::RandUint64();
On 2015/07/29 20:28:04, jln wrote:
> Do we still want to do that individually even though you already do it
globally?

We don't do it globally. We do it in the copy of base that is in the shell,
which is different from the copy of base that's linked into the shared object
this function is in.

https://codereview.chromium.org/1264463005/diff/120001/mojo/runner/child_proc...
File mojo/runner/child_process.cc (right):

https://codereview.chromium.org/1264463005/diff/120001/mojo/runner/child_proc...
mojo/runner/child_process.cc:314: 
On 2015/07/29 20:28:04, jln wrote:
> The contract for "MojoSanboxWarm" should be:
> 
> 1. Don't start threads
> 2. Don't have any directory file descriptor open
> 
> Could you do:
> 
> ThreadHelpers::AssertSingleThreaded(sandbox-proc_fd());
> CHECK(!ProcUtil::HasOpenDirectory(sandbox-proc_fd());
> 
> after running sandbox_warm?
> 
> Also it seems to be like at least part of l297-l321 should be in the sandbox
> class, no? That way, you wouldn't even need to add a proc_fd() accessor.
> 
> One simple way to refactor: put l302 to l314 in its own function that returns
a
> callback. l315 to l320 can move to the sandbox class. It would run that
clalback
> first and then call the sanity checks I mentioned above.

I have added the checks to LinuxSandbox::Warmup(). Those checks seem like a good
idea anyway. I'm hesitant to add callbacks when a simpler linear piece of code
will do.

[Elliot Glaysher, 2015-07-29 21:08:36]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2015-07-29 21:08:36]

The patchset sent to the CQ was uploaded after l-g-t-m from jln@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1264463005/#ps140001
(title: "Add security checks to LinuxSandbox::Warmup()")

[commit-bot: I haz the power, 2015-07-29 21:08:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1264463005/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1264463005/140001

[commit-bot: I haz the power, 2015-07-29 21:24:23]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-07-29 21:25:07]

Patchset 8 (id:??) landed as
https://crrev.com/25d8872d321ede78e50868d362eb2f278ebe0bf0
Cr-Commit-Position: refs/heads/master@{#340989}