1339105 - Implement Windows Level 3 content process sandbox

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement)

Description

[Bob Owen (:bobowen)]

Implement Level 3 content process sandbox, which should be the same as level 2, but with access token level USER_LIMITED and job level JOB_RESTRICTED.

This will mean adding rules to allow read access to chrome and extensions profile directories.
Also, write access to content temp directory.

Comment 1

[Bob Owen (:bobowen)]

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

xpcshell failure there is because it registers a directory provider after we've already called for the file in question and so it's already cached.

I have a fix that undefines and sets the property on the directory service instead, not sure if there will be other instances of this:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=248f8fe7f003554d265ab9998063d4e14c45d898

Comment 2

[Bob Owen (:bobowen)]

Attachment: Part 1: Implement Windows Level 3 content process sandbox policy

MozReview-Commit-ID: L8wcVhdLvFe

Comment 3

[Bob Owen (:bobowen)]

Attachment: Part 2: Fix registerFakePath in head_migration.js to work when the key has already been accessed

This also fixes the cleanup to fully remove the fake path and put back original if it existed.

Comment 4

[Bob Owen (:bobowen)]

Attachment: Part 3: Move NPAPI windows process sandbox file rules into SandboxBroker

This also removes a rule that was added for sandboxing the Java plugin,
which we never did and we now only allow Flash anyway.

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8869430 [details] [diff] [review]
Part 2: Fix registerFakePath in head_migration.js to work when the key has already been accessed

Review of attachment 8869430 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/migration/tests/unit/head_migration.js
@@ +68,5 @@
> +    // To clean up just undefine.
> +    do_register_cleanup(() => {
> +      dirsvc.undefine(key);
> +    });
> +  }

nit: I think it would be a little bit more readable and compact as:
let originalFile;
try {
  // If a file is already cached undefine to set ours.
  originalFile = dirsvc.get(key, Ci.nsIFile);
  dirsvc.undefine(key);
} catch (e) { /* file doesn't exist * / }

// Set our file.
dirsvc.set(key, file);

do_register_cleanup(() => {
  dirsvc.undefine(key);
  if (originalFile)
    dirsvc.set(key, originalFile);
});

Comment 6

[Bob Owen (:bobowen)]

Thanks both for the reviews.

(In reply to Marco Bonardo [::mak] from comment #5)
> Comment on attachment 8869430 [details] [diff] [review]
> Part 2: Fix registerFakePath in head_migration.js to work when the key has
> already been accessed
...
> nit: I think it would be a little bit more readable and compact as:
...

Yeah, I toyed with doing it that way, so happy to change it.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50bf4c923818
Part 1: Implement Windows Level 3 content process sandbox policy. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/964b6ee8ec32
Part 2: Fix registerFakePath in head_migration.js to work when the key has already been accessed. r=mak
https://hg.mozilla.org/integration/mozilla-inbound/rev/367734cc9370
Part 3: Move NPAPI windows process sandbox file rules into SandboxBroker. r=jimm

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for Windows bustage: calling protected constructor of class 'nsAString' at sandboxBroker.cpp(208,11):

https://hg.mozilla.org/integration/mozilla-inbound/rev/3df269387c8cf2b969b928f20c1c5a9d542073ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f389e643ba52e15c1c0a6c5deaa75bd764e4b9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5145aab4731280a9713896c08ea94c2e5dace30

Push with failures (only look at the Windows ones): https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=367734cc9370f2528dc564921e3d678cb352f514&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=100929062&repo=mozilla-inbound

13:47:13     INFO -  z:/task_1495457102/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp(208,11):  error: calling a protected constructor of class 'nsAString'
13:47:13     INFO -            aRelativePath, aAccess);
13:47:13     INFO -            ^
13:47:13     INFO -  z:/task_1495457102/build/src/obj-firefox/dist/include/nsTSubstring.h(1107,3):  note: declared protected here
13:47:13     INFO -    nsTSubstring_CharT(const self_type& aStr)
13:47:13     INFO -    ^
13:47:13     INFO -  z:/task_1495457102/build/src/obj-firefox/dist/include/string-template-def-unichar.h(15,45):  note: expanded from macro 'nsTSubstring_CharT'
13:47:13     INFO -  #define nsTSubstring_CharT                  nsAString
13:47:13     INFO -                                              ^
13:47:13     INFO -  z:/task_1495457102/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp(208,11):  error: cannot pass object of non-trivial type 'const nsAString' through variadic function; call will abort at runtime [-Wnon-pod-varargs]
13:47:13     INFO -            aRelativePath, aAccess);
13:47:13     INFO -            ^
13:47:13     INFO -  z:/task_1495457102/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp(221,28):  error: cannot pass object of non-trivial type 'nsAutoString' through variadic function; call will abort at runtime [-Wnon-pod-varargs]
13:47:13     INFO -            result, aAccess, rulePath);
13:47:13     INFO -                             ^
13:47:13     INFO -  3 errors generated.
13:47:13     INFO -  z:/task_1495457102/build/src/config/rules.mk:1060: recipe for target 'sandboxBroker.obj' failed
13:47:13     INFO -  mozmake.EXE[5]: *** [sandboxBroker.obj] Error 1
13:47:13     INFO -  mozmake.EXE[5]: Leaving directory 'z:/task_1495457102/build/src/obj-firefox/security/sandbox/win/src/sandboxbroker'
13:47:13     INFO -  z:/task_1495457102/build/src/config/recurse.mk:73: recipe for target 'security/sandbox/win/src/sandboxbroker/target' failed
13:47:13     INFO -  mozmake.EXE[4]: *** [security/sandbox/win/src/sandboxbroker/target] Error 2

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3fb60fbc326
Part 1: Implement Windows Level 3 content process sandbox policy. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/445875fbf13b
Part 2: Fix registerFakePath in head_migration.js to work when the key has already been accessed. r=mak
https://hg.mozilla.org/integration/mozilla-inbound/rev/431267ab28de
Part 3: Move NPAPI windows process sandbox file rules into SandboxBroker. r=jimm

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=101032225&repo=mozilla-inbound on Win64 debug browser-chrome tests

https://hg.mozilla.org/integration/mozilla-inbound/rev/f38ab8b5fba5

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76376445ed04
Part 1: Implement Windows Level 3 content process sandbox policy. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dbcfc90a5c2
Part 2: Fix registerFakePath in head_migration.js to work when the key has already been accessed. r=mak
https://hg.mozilla.org/integration/mozilla-inbound/rev/31e7b206282d
Part 3: Move NPAPI windows process sandbox file rules into SandboxBroker. r=jimm

Comment 12

[Phil Ringnalda (:philor)]

Not your leak.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/76376445ed04
https://hg.mozilla.org/mozilla-central/rev/4dbcfc90a5c2
https://hg.mozilla.org/mozilla-central/rev/31e7b206282d