1309394 - Introduce automated tests to validate content process sandboxing works as intended

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Haik Aftandilian [:haik]]

It's important that we have automated tests that validate content sandboxing works as expected and doesn't regress. A bug in a fix to the Mac rulesets, Linux broker, or Windows access token code could inadvertently disable sandboxing and tests that help catch those bugs would be beneficial.

There's value in Javascript tests because they allow us to test release binaries with no instrumentation or debug test code, but it would also be valuable to have native/CPP test code because ultimately it would be possible for a content process implementation to be broken in a way such that it rejects Javascript file I/O but doesn't prevent native file I/O.

With this bug I'd like to start with some minimal Javascript sandbox regression tests.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

JavaScript tests can also use js-ctypes to call native code directly: https://developer.mozilla.org/en-US/docs/Mozilla/js-ctypes

For example:

  Cu.import("resource://gre/modules/ctypes.jsm")
  libc = ctypes.open("libc.so.6")
  prctl1 = libc.declare("prctl", ctypes.default_abi, ctypes.long, ctypes.long, ctypes.long)
  prctl1(22, 1)

Comment 2

[Haik Aftandilian [:haik]]

Attachment: A WIP browser chrome test for file I/O from a sandboxed content process

Here's a work in progress test that does some file I/O from the content process. It tries to create a new file in the home directory and fails if that is allowed. And it tries to create a file in the content temp directory and fails if that is _not_ allowed. More tests can be added if the approach seems reasonable. Only runs tests on e10s and if pref(security.sandbox.content.level) is set high enough for the platform. (On Linux that means it depends on seccomp being supported.)

It sends the process script to all message managers, but it should just send it to content processes.

Using js-ctypes, per Jed's comment, might be a better approach. I'll look into that.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ae6896f063e5c16b709a289bebf7be18db572d8

Comment 3

[Haik Aftandilian [:haik]]

Updated the patch with some improvements. Now, the process script is just sent to the content process.

Comment 4

[Haik Aftandilian [:haik]]

Attachment: Browser chrome test for file I/O from a sandboxed content process

Added uses of js-ctypes to test the open, exec, and fork syscalls.

The exec test isn't run on Linux because the content process gets killed. A good enhancement would be to instead do the exec and validate that content is killed.

Not yet tested on Win, just Mac and Debian running 3.16 kernel.

Comment 5

[Haik Aftandilian [:haik]]

Attachment: Bug 1309394 - automated tests to validate content process sandboxing works as intended;

Adds dom/base/test/browser_content_sandbox_fs.js for validating content
sandbox file I/O restrictions.

Adds dom/base/test/browser_content_sandbox_syscalls.js for validating
OS-level calls are sandboxed as intended. Uses js-ctypes to invoke native
library routines. Windows tests yet to be added here.

Review commit: https://reviewboard.mozilla.org/r/96594/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96594/

Comment 6

[Haik Aftandilian [:haik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37e4d71e5ee32182378fd08c0e4ce03e25c821ee

Comment 7

[Bob Owen (:bobowen)]

Comment on attachment 8815817 [details]
Bug 1309394 - automated tests to validate content process sandboxing works as intended;

https://reviewboard.mozilla.org/r/96594/#review97070

Thanks for working on these, it'll be great to have some tests for sandboxing and give us something to build on.

I'm going to remove the review request until the question over using ContentTask.spawn is resolved.

https://wiki.mozilla.org/Electrolysis/e10s_test_tips#ContentTask

::: dom/base/test/browser.ini:32
(Diff revision 1)
>  [browser_state_notifications.js]
>  skip-if = true # Bug 1271028
>  [browser_use_counters.js]
>  [browser_bug1307747.js]
> +[browser_content_sandbox_fs.js]
> +[browser_content_sandbox_syscalls.js]

I wonder if these shouldn't live somewhere else.
Under security/sandbox or browser maybe, we could discuss at meeting.

::: dom/base/test/browser_content_sandbox_fs.js:10
(Diff revision 1)
> +            .getService(Ci.nsIPrefBranch);
> +
> +const uuidGenerator = Cc["@mozilla.org/uuid-generator;1"]
> +                      .getService(Ci.nsIUUIDGenerator);
> +
> +ppmm.QueryInterface(Ci.nsIProcessScriptLoader);

Not sure what this does and why we need ppmm at all.
Perhaps something below not work without this, please add comment if this is the case.

::: dom/base/test/browser_content_sandbox_fs.js:24
(Diff revision 1)
> +function ns(msgName) { return("ContentSandboxFileIO:" + msgName); }
> +
> +// Installs handlers to respond to file I/O messages sent from the chrome
> +// process in order to validate content process file system sandboxing is
> +// in effect.
> +function processScript() {

Could we use ContentTask.spawn instead of all this?

::: dom/base/test/browser_content_sandbox_fs.js:162
(Diff revision 1)
> +  // Set fileIOSandboxMinLevel to the lowest level that has
> +  // content filesystem sandboxing enabled. For now, this
> +  // varies across Windows, Mac, Linux, other.
> +  switch (Services.appinfo.OS) {
> +    case "WINNT":
> +      fileIOSandboxMinLevel = 2;

This should be 1 for write blocking.

Comment 8

[Haik Aftandilian [:haik]]

Comment on attachment 8815817 [details]
Bug 1309394 - automated tests to validate content process sandboxing works as intended;

https://reviewboard.mozilla.org/r/96594/#review97070

Thanks for the feedback. I've switched to using ContentTask.spawn which simplifies all the messaging. Will post updated code shortly.

> I wonder if these shouldn't live somewhere else.
> Under security/sandbox or browser maybe, we could discuss at meeting.

I've moved them to security/sandbox/test, but open to other suggestions.

> Not sure what this does and why we need ppmm at all.
> Perhaps something below not work without this, please add comment if this is the case.

That was leftover code from an earlier version where I used ppmm to broadcast the process script.

> Could we use ContentTask.spawn instead of all this?

Yep.

Comment 9

[Haik Aftandilian [:haik]]

Comment on attachment 8815817 [details]
Bug 1309394 - automated tests to validate content process sandboxing works as intended;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96594/diff/1-2/

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8815817 [details]
Bug 1309394 - automated tests to validate content process sandboxing works as intended;

https://reviewboard.mozilla.org/r/96594/#review97518

::: security/sandbox/test/browser_content_sandbox_syscalls.js:59
(Diff revision 2)
> +function uuid() {
> +  return uuidGenerator.generateUUID().toString();
> +}
> +
> +// Returns a file object for a new file in the home dir ($HOME/<UUID>).
> +function fileInHomeDir() {

Duplicate code? Should be possible to avoid this.

Comment 11

[Haik Aftandilian [:haik]]

Comment on attachment 8815817 [details]
Bug 1309394 - automated tests to validate content process sandboxing works as intended;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96594/diff/2-3/

Comment 12

[Haik Aftandilian [:haik]]

Comment on attachment 8815817 [details]
Bug 1309394 - automated tests to validate content process sandboxing works as intended;

https://reviewboard.mozilla.org/r/96594/#review97518

> Duplicate code? Should be possible to avoid this.

Thanks. Moved the shared functions to security/sandbox/test/browser_content_sandbox_utils.js.

Comment 13

[Haik Aftandilian [:haik]]

Comment on attachment 8815817 [details]
Bug 1309394 - automated tests to validate content process sandboxing works as intended;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96594/diff/3-4/

Comment 14

[Bob Owen (:bobowen)]

Are we still going ahead with these, I forget what we decided?
Let me know and I'll review if we still want to land these.

Comment 15

[Haik Aftandilian [:haik]]

(In reply to Bob Owen (:bobowen) from comment #14)
> Are we still going ahead with these, I forget what we decided?
> Let me know and I'll review if we still want to land these.

There was a concern about using c-types for these tests and I talked to bsmedberg about that. There isn't a concrete plan to deprecate or remove c-typ

Comment 16

[Haik Aftandilian [:haik]]

(Hit save to early on previous comment)

There was a concern about using c-types for these tests and I talked to bsmedberg about that. There isn't a concrete plan to deprecate or remove c-types at the moment, but bsmedberg recommended looking into using gtests with compiled code for these types of tests. That could be done, but it would take some work to get the gtests running in the content process and a downside is that it wouldn't be testing shipping code due to the custom lib XUL used.

I think it makes sense to get this (current approach) integrated and then extend the tests here to load a custom library for each platform which can run compiled code in the content process. That would avoid the excessive boilerplate needed for more complicated native calls and would be relatively easy to convert to gtests later.

Comment 17

[Bob Owen (:bobowen)]

Comment on attachment 8815817 [details]
Bug 1309394 - automated tests to validate content process sandboxing works as intended;

https://reviewboard.mozilla.org/r/96594/#review99398

Sorry, I'd mostly finished this on Friday, but then got distracted by another printing issue.

Didn't really look through browser_content_sandbox_syscalls.js as I don't think it affects Windows, but some of the comment might apply there as well.

::: security/sandbox/test/browser_content_sandbox_fs.js:26
(Diff revision 4)
> +  let encoder = new TextEncoder();
> +  let array = encoder.encode("WRITING FROM CONTENT PROCESS");
> +  return OS.File.writeAtomic(path, array).then(function(value) {
> +    return true;
> +  }, function(reason) {
> +    dump("Failed writing to " + path + "\n");

nit: I don't think we should have these dumps, given that sometimes this is the correct action.
If you think we need extra logging in the child when something goes wrong, we should pass in an extra boolean and log when it does the wrong thing.

::: security/sandbox/test/browser_content_sandbox_fs.js:40
(Diff revision 4)
> +function deleteFile(path) {
> +  Components.utils.import("resource://gre/modules/osfile.jsm");
> +  return OS.File.remove(path, {ignoreAbsent: false}).then(function(value) {
> +    return true;
> +  }).catch(function(err) {
> +    dump(`Failed deleting ${path}\n`);

nit: perhaps we should use Template literals everywhere, looks a bit odd just here.

::: security/sandbox/test/browser_content_sandbox_fs.js:80
(Diff revision 4)
> +// content temp dir key.
> +//
> +add_task(function*() {
> +  // This test is only relevant in e10s
> +  if (!gMultiProcessBrowser) {
> +    ok(true, "e10s is not enabled, nothing to test");

nit: I think this should probably fail and we should skip these tests if e10s is not enabled. Rather than making it look like they test something and pass.

::: security/sandbox/test/browser_content_sandbox_fs.js:85
(Diff revision 4)
> +    ok(true, "e10s is not enabled, nothing to test");
> +    return;
> +  }
> +
> +  let level = prefs.getIntPref("security.sandbox.content.level");
> +  ok(true, "got security.sandbox.content.level pref, value=" + level);

nit: I think this should just be an info.

::: security/sandbox/test/browser_content_sandbox_fs.js:90
(Diff revision 4)
> +  ok(true, "got security.sandbox.content.level pref, value=" + level);
> +
> +  // Fail and quit if the content sandbox is disabled;
> +  // it should be enabled in automation.
> +  if (level == 0) {
> +    ok(false, "Content process sandbox is disabled.");

If Linux sandboxing doesn't manage to ride the trains, won't this start failing on Aurora?

::: security/sandbox/test/moz.build:7
(Diff revision 4)
> +# vim: set filetype=python:
> +# Any copyright is dedicated to the Public Domain.
> +# http://creativecommons.org/publicdomain/zero/1.0/
> +
> +BROWSER_CHROME_MANIFESTS += [
> +    'browser.ini',

I seem to remember that at one point it was generally preferred to not add new moz.build files just for tests and add from the parent directory. Something to do with parsing performance.
We should get a build peer to review to check this.

Comment 18

[Haik Aftandilian [:haik]]

Comment on attachment 8815817 [details]
Bug 1309394 - automated tests to validate content process sandboxing works as intended;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96594/diff/4-5/

Comment 19

[Haik Aftandilian [:haik]]

Comment on attachment 8815817 [details]
Bug 1309394 - automated tests to validate content process sandboxing works as intended;

https://reviewboard.mozilla.org/r/96594/#review99398

> nit: I don't think we should have these dumps, given that sometimes this is the correct action.
> If you think we need extra logging in the child when something goes wrong, we should pass in an extra boolean and log when it does the wrong thing.

OK. Removed those dumps and the code that runs in content now just returns the result.

> nit: perhaps we should use Template literals everywhere, looks a bit odd just here.

Fixed.

> nit: I think this should probably fail and we should skip these tests if e10s is not enabled. Rather than making it look like they test something and pass.

Changed the .ini file to skip the test if e10s is not enabled. And made the tests fail if they're run without e10s.

> nit: I think this should just be an info.

Fixed

> If Linux sandboxing doesn't manage to ride the trains, won't this start failing on Aurora?

Fixed this for now by not failing if we're on Linux and the sandbox is disabled, but I'm going to include an isNightly() check so that this will only apply to !nightly.

> I seem to remember that at one point it was generally preferred to not add new moz.build files just for tests and add from the parent directory. Something to do with parsing performance.
> We should get a build peer to review to check this.

Haven't resolved this yet.

Comment 20

[Haik Aftandilian [:haik]]

Comment on attachment 8815817 [details]
Bug 1309394 - automated tests to validate content process sandboxing works as intended;

>> I seem to remember that at one point it was generally preferred
>> to not add new moz.build files just for tests and add from the
>> parent directory. Something to do with parsing performance.
>> We should get a build peer to review to check this.

glandium, any recommendations regarding adding a new moz.build file? This change adds the directory security/sandbox/test/ and security/sandbox/test/moz.build as well as some new tests in the same dir.

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 8815817 [details]
Bug 1309394 - automated tests to validate content process sandboxing works as intended;

https://reviewboard.mozilla.org/r/96594/#review104392

Yeah, it would be better to avoid adding the new file. You can use relative paths for BROWSER_CHROME_MANIFESTS.

Comment 22

[Haik Aftandilian [:haik]]

Attachment: mychanges

Review commit: https://reviewboard.mozilla.org/r/103914/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103914/

Comment 23

[Haik Aftandilian [:haik]]

Comment on attachment 8815817 [details]
Bug 1309394 - automated tests to validate content process sandboxing works as intended;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96594/diff/5-6/

Comment 24

[Haik Aftandilian [:haik]]

Comment on attachment 8815817 [details]
Bug 1309394 - automated tests to validate content process sandboxing works as intended;

https://reviewboard.mozilla.org/r/96594/#review99398

> Haven't resolved this yet.

Fixed in latest version. Removed the new test/moz.build and modified security/sandbox/moz.build instead per glandium's tip.

Comment 25

[Haik Aftandilian [:haik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8404e1cd9d3f3136a4f00fda3ef8df6845c60a8c

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7104fa22611
automated tests to validate content process sandboxing works as intended; r=bobowen,gcp

Comment 27

[Haik Aftandilian [:haik]]

Aurora/52 test results below. Lots of orange, but all of the new tests passed.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=34c8d88bdfac25a0fe7e58ee106b2bcd1ec8e637

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Uplift note: the patch as pushed to Try is what needs to land on Aurora (it needed some rebasing)
https://hg.mozilla.org/try/raw-rev/177111a700a569b1ee5d4e031dc92b667314a2e9

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d7104fa22611

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/99845265381f