Closed Bug 1309394 Opened 5 years ago Closed 5 years ago

Introduce automated tests to validate content process sandboxing works as intended

Categories

(Core :: Security: Process Sandboxing, defect)

51 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: haik, Assigned: haik)

References

(Blocks 1 open bug)

Details

(Whiteboard: sbmc2 sbwc2 sblc2)

Attachments

(1 file, 3 obsolete files)

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.
Whiteboard: sbmc2 sbwc2 sblc2
Assignee: nobody → haftandilian
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)
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
Whiteboard: sbmc2 sbwc2 sblc2 → sb? sbmc2 sbwc2 sblc2
Whiteboard: sb? sbmc2 sbwc2 sblc2 → sbmc2 sbwc2 sblc2
Updated the patch with some improvements. Now, the process script is just sent to the 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.
Attachment #8800298 - Attachment is obsolete: true
Attachment #8813869 - Attachment is obsolete: true
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.
Attachment #8815817 - Flags: review?(bobowencode)
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 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.
Attachment #8815817 - Flags: review?(gpascutto) → review+
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.
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.
Flags: needinfo?(haftandilian)
(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
Flags: needinfo?(haftandilian)
(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 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.
Attachment #8815817 - Flags: review?(bobowencode) → review+
See Also: → 1329294
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 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.
Attachment #8815817 - Flags: review?(mh+mozilla)
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.
Attachment #8815817 - Flags: review?(mh+mozilla)
Attachment #8825839 - Attachment is obsolete: true
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.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d7104fa22611
automated tests to validate content process sandboxing works as intended; r=bobowen,gcp
Keywords: checkin-needed
Aurora/52 test results below. Lots of orange, but all of the new tests passed.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=34c8d88bdfac25a0fe7e58ee106b2bcd1ec8e637
Whiteboard: sbmc2 sbwc2 sblc2 → sbmc2 sbwc2 sblc2 [checkin-needed-aurora]
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
https://hg.mozilla.org/mozilla-central/rev/d7104fa22611
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
https://hg.mozilla.org/releases/mozilla-aurora/rev/99845265381f
Flags: in-testsuite+
Whiteboard: sbmc2 sbwc2 sblc2 [checkin-needed-aurora] → sbmc2 sbwc2 sblc2
Blocks: 1330785
You need to log in before you can comment on or make changes to this bug.