1366256 - Change 64-bit NPAPI process sandbox to run at level 3.

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement, P1)

Description

[Bob Owen (:bobowen)]

With the file permission functionality landed for bug 1284897, we might be able to now run the 64-bit NPAPI process sandbox at level 3 (i.e. using the USER_LIMITED access token level).

This will remove access to things where the access is provided by the user's own access token, for example their Users folder.

Comment 1

[Tracy Walker [:tracy]]

assume I change security.sandbox.content.level to 3.

What do you want me to spot check in testing?

Comment 2

[Bob Owen (:bobowen)]

Pref is dom.ipc.plugins.sandbox-level.flash (should be 2 at the moment on 64-bit).

The only thing that I know level 3 used to break is file uploading, but I think some general Flash testing as well would be good.

Comment 3

[Tracy Walker [:tracy]]

This looks good with latest Nightly 55.0a1, 20170531030204 64-bit on Window 10 with dom.ipc.plugins.sandbox-level.flash set to 3.

no problem uploading a file from Documents to google docs.  Also, spot tested some flash apps with ADP 26.0.0.115.  Seems to be good to go.

Comment 5

[David Parks [:handyman]]

Attachment: Set flash sandbox to level 3

I was going to submit this but I totally ignored the need for review.  The most controversial part of the patch:

> -// The lines in PluginModuleParent.cpp should be changed in line with this.

This was a reference to the MOZ_ALLOW_WEAKER_SANDBOX guard, which is no longer in the codebase.

For a partial list of plugins with interesting features, see the bottom of bug 1415160 comment 1.

Turns out this patch breaks the camera.  This plugin says that it can't find the camera : https://www.onlinemictest.com/webcam-test-in-adobe-flash/ .  Note that the microphone demo is still broken everywhere but I wouldn't be surprised if we also have microphone problems.  That might be enough to hold off on submission.

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

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 8952817 [details] [diff] [review]
Set flash sandbox to level 3

Need to sort out camera access issues before we ship.

Comment 7

[David Parks [:handyman]]

Note to self:

I somehow managed to miss that nsPluginTags still blocks sandbox-lowering... just not with MOZ_ALLOW_WEAKER_SANDBOX [1].  So I need to update that to use level 3 (and restore+clarify the comment).

>  // As level 2 is now the default NPAPI sandbox level for 64-bit flash, we
>  // don't want to allow a lower setting. This should be changed if the
>  // firefox.js pref file is changed.
>  if (mIsFlashPlugin && mSandboxLevel < 2) {
>    mSandboxLevel = 2;
>  }

[1] https://searchfox.org/mozilla-central/rev/14d933246211b02f5be21d2e730a57cf087c6606/dom/plugins/base/nsPluginTags.cpp#419

Comment 8

[David Parks [:handyman]]

Attachment: Part 1: Promote windows plugin sandbox level to 3

Requesting a quick re-review since I had missed the nsPluginTags part earlier.

Comment 9

[David Parks [:handyman]]

Attachment: Part 2: Add restricting user SID in plugin level 3

The camera initialization was failing a CreateMutex call using the user SID so this is definitely necessary.  I don't know how this changes security -- you might have a better idea of that.  But I don't think there is an alternative if we want USER_LIMITED.

Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f1772b7639423774cb49cfafd45ceb7882333d8

Comment 10

[Bob Owen (:bobowen)]

Comment on attachment 8955564 [details] [diff] [review]
Part 2: Add restricting user SID in plugin level 3

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

Ah, getting rid of the user's own SID is the main reason for going to USER_LIMITED I think.

Comment 11

[David Parks [:handyman]]

Attachment: WIP: Hook CreateMutexW to get the camera mutex via FunctionBroker

You were totally correct.  For the record, the (only) other difference between USER_INTERACTIVE and USER_LIMITED is the WinAuthenticatedUserSid.

So I had to get creative.  I decided to try brokering the CreateMutexW call when it is a request for this mutex.  I create the mutex in the chrome proc, DuplicateHandle it for use in the client proc, and return it.  This basically mimics what chromium does for e.g. the filesystem calls it brokers (but its much less painful).  Surprise: this was all it took to restore camera function.

Problem is (and this is admittedly minor) I'm getting an exception when plugin-container shuts down -- "Invalid Handle passed to CloseHandle".  This exception can be safely skipped -- it does not show up unless you are hooked into the debugger.  But it does suggest that I'm doing something wrong with the mutex.  With no brokering at the lower sandbox level, there is no exception.  Maybe something to do with the DuplicateHandle call?

NI to Bob to see if you can easily identify anything I'm doing wrong that would cause the handle copy not to be closable in the child (even tho it is used fine as a mutex up to that point so the handle must be valid).  FYI, I tried the DUPLICATE_CLOSE_SOURCE flag and it changed nothing -- I figure it just closes the source handle reference after duplicating it (ie it is uninteresting) but the docs are cryptic as usual so I tried it anyway.

(At least this is POC that we'll be at USER_LIMITED soon.  You'll also notice that I've hardcoded a GUID for my brand of camera as part of the mutex name... there is indeed still work to do.)

Comment 12

[Bob Owen (:bobowen)]

Not sure what might be going wrong with the CloseHandle.
The only thing I can think of is that because it is closing the last handle and because we are passing a null security descriptorm it is failing because it doesn't have the correct permissions to destroy the mutex.

Comment 13

[Bob Owen (:bobowen)]

Submitted that a bit early ...

I was about to say that that seems unlikely.

Other comments, the two TRUEs for inheritance (in the SECURITY_ATTRIBUTES and DuplicateHandle) should be FALSE, we don't need the handle to be inherited by new processes.
Also, I'd probable just use a HANDLE for serverMutex and use DUPLICATE_CLOSE_SOURCE, but I don't think it really matters.

I assume that flash is setting bInitialOwner to FALSE in its request, otherwise the releases would get out of kilter.

Comment 14

[David Parks [:handyman]]

initialOwner is FALSE in the call.  I hadn't put any thought into the InheritHandle field.  I've also got to switch permissions from DUP_ALL_ACCESS to just request SYNCHRONIZE.  It definitely needs cleanup.  I'm not even sure yet what other things I'm opening up here.

Comment 15

[David Parks [:handyman]]

Note to self:

The HANDLE issue was nothing.  I had forgotten that the FunctionBroker obfuscates HANDLEs -- it was returning essentially garbage for the mutex handle.  Things were working because the mutex locking was failing but it didn't matter -- the lock was just not in contention so it didn't matter that it wasnt really locked (and that is all that Windows wants here).  We do need the _real_ lock for thread safety so I return the unobfuscated HANDLE now by specializing ResponseHandler::Marshal/Unmarshal... but I'd like a better way of making this clear for the future.  The conflict is between the "safe" behavior of obfuscating the HANDLE (which is mildly beneficial) and the more "obvious" behavior of not doing that.  The Server EndpointHandler is the one who initiates this.  I'm tempted to make it a (template) parameter to the FunctionBroker so that it can be specialized -- and then I could have one that overloads the HANDLEs for routines that want that (or vice-versa).  This is much easier than it sounds (it basically boils down to some typedefs) but would still take some effort.

I've been focused on the other part of this -- the mutex name.  This may be annoying and a little fragile but the mutex name prefix ("eed3bd3a-a1ad-4e99-987b-d7cb3fcfa7f0") is a hard-coded UUID.  We can do the same but, if it changes in the future, we have to manually update.  And finding the value in Windows code requires the debugger.  On the plus side, we'd only need to do this until Flash is EOL.  I'm currently trying to find if the value is the consistent across versions of Windows.

Comment 17

[David Parks [:handyman]]

Attachment: 1. Promote Windows plugin sandbox to level 3

Requesting re-review to give you a heads up that I changed the min plugin sandbox level to 2 and the default to 3 -- jimm mentioned that we allow one level lower than default on the content proc and this allows a workaround for anyone who runs into trouble.

Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1daf2cf23041a1402eefd92e7586f6a0f9d088f5

Comment 18

[David Parks [:handyman]]

Attachment: 2. Refactor EndpointHandler to make special type handling opt-in

This looks more elaborate than it is.  There aren't any 'executable' code changes -- just a refactor to make the EndpointHandler a template parameter of the FunctionBroker.  It then moves some of the special-purpose EndpointHandler::Copy methods into separate classes (FileDlgEHContainer and SslEHContainer for file dialog and SSL communication APIs).  The base/default class, BaseEndpointHandler, defines operations for common types (e.g. marshaling ints).  The reason I do this here is that, in patch 3, CreateMutexW gets a HANDLE from the parent and the SSL behavior obfuscates its HANDLEs but that won't work for CreateMutexW.

The EHContainer classes simply hold EndpointHandler<SERVER> and EndpointHandler<CLIENT> instances, which has to be done for reasons of the C++ standard.  (mingw is very strict.)

Comment 19

[David Parks [:handyman]]

Attachment: 3. Broker camera CreateMutexW calls from plugin to parent proc

This is basically the same patch as before, only now it avoids the HANDLE obfuscation, which is moved to SslEndpointHandler (this FunctionBroker uses the default, BaseEndpointHandler, since it doesn't need any special type management).

Comment 20

[Bob Owen (:bobowen)]

Comment on attachment 8959316 [details] [diff] [review]
3. Broker camera CreateMutexW calls from plugin to parent proc

As you suggested on IRC, it's probably best if jimm looks at this.

Comment 21

[Jim Mathies [:jimm]]

Comment on attachment 8959314 [details] [diff] [review]
2. Refactor EndpointHandler to make special type handling opt-in

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

generally lgtm. one nit, that's it.

::: dom/plugins/ipc/FunctionBroker.cpp
@@ +162,5 @@
> +    }
> +    aDest = val;
> +  }
> +
> +  // HANDLEs and HINTERNETs unmarshal with obfuscation 

nit - ws

Comment 22

[Jim Mathies [:jimm]]

Comment on attachment 8959316 [details] [diff] [review]
3. Broker camera CreateMutexW calls from plugin to parent proc

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

::: dom/plugins/ipc/FunctionBroker.cpp
@@ +1151,5 @@
> +  void* buf = malloc(bufLen);
> +  success = ::GetTokenInformation(token, TokenUser, buf, bufLen, &bufLen);
> +  MOZ_ASSERT(success);
> +  if (success) {
> +  TOKEN_USER* tokenUser = static_cast<TOKEN_USER*>(buf);

nit - indentation

Comment 23

[David Parks [:handyman]]

Attachment: 1. Promote Windows plugin sandbox to level 3 (r=bobowen)

rebasing everything to work past a nasty merge conflict

Comment 24

[David Parks [:handyman]]

Attachment: 2. Refactor EndpointHandler to make special type handling opt-in (r=jimm)

Comment 25

[David Parks [:handyman]]

Attachment: 3. Broker camera CreateMutexW calls from plugin to parent proc (r=jimm)

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c08e6bfa67d0
Part 1 - Promote Windows plugin process sandbox to level 3 r=bobowen
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bee2ab0b174
Part 2 - Refactor EndpointHandler to make special type handling opt-in r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb9e8967c454
Part 3 - Broker camera CreateMutexW calls from plugin process r=jimm

Comment 27

[Stefan Hindli [:stefan_hindli]]

Backed out 3 changesets (bug 1366256) for failing in toolkit/components/url-classifier/tests/browser/browser_flashblock_off_with_always_activate.js | Plugin should have the correct 'plugin running' state on a CLOSED TREE

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/9c7cd4a36f061042e00296edfc922a8691aa0600
Problematic push: https://hg.mozilla.org/integration/mozilla-inbound/rev/c08e6bfa67d07357d94a4acc92b0c5ede674d816

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=169179062&repo=mozilla-inbound&lineNumber=5845

Relevant part of that log: 

12:16:49    ERROR -  1319 INFO TEST-UNEXPECTED-FAIL | toolkit/components/url-classifier/tests/browser/browser_flashblock_off_with_always_activate.js | Plugin should have the correct 'plugin running' state - Got false, expected true
12:16:49     INFO -  Stack trace:
12:16:49     INFO -  chrome://mochikit/content/browser-test.js:test_is:1283
12:16:49     INFO -  chrome://mochitests/content/browser/toolkit/components/url-classifier/tests/browser/classifierTester.js:checkPluginInfo:334
12:16:49     INFO -  chrome://mochitests/content/browser/toolkit/components/url-classifier/tests/browser/browser_flashblock_off_with_always_activate.js:checkFlashBlockLists:34
12:16:49     INFO -  1320 INFO TEST-PASS | toolkit/components/url-classifier/tests/browser/browser_flashblock_off_with_always_activate.js | Plugin's existance in navigator.plugins should match expected -
12:16:49     INFO -  1321 INFO RUNNING TEST: Nested unknown domains (Ask to Activate, Flashblock off)
12:16:49     INFO -  1322 INFO Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 170}]
12:16:50     INFO -  1323 INFO Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://example.com/browser/toolkit/components/url-classifier/tests/browser/flash_block_frame.html" line: 0}]
12:16:50     INFO -  1324 INFO TEST-PASS | toolkit/components/url-classifier/tests/browser/browser_flashblock_off_with_always_activate.js | Page's classification should match expected -
12:16:50     INFO -  1325 INFO TEST-PASS | toolkit/components/url-classifier/tests/browser/browser_flashblock_off_with_always_activate.js | Plugin should have the correct activation -
12:16:50     INFO -  Not taking screenshot here: see the one that was previously logged
12:16:50    ERROR -  1326 INFO TEST-UNEXPECTED-FAIL | toolkit/components/url-classifier/tests/browser/browser_flashblock_off_with_always_activate.js | Plugin should have the correct 'plugin running' state - Got false, expected true
12:16:50     INFO -  Stack trace:
12:16:50     INFO -  chrome://mochikit/content/browser-test.js:test_is:1283
12:16:50     INFO -  chrome://mochitests/content/browser/toolkit/components/url-classifier/tests/browser/classifierTester.js:checkPluginInfo:334
12:16:50     INFO -  chrome://mochitests/content/browser/toolkit/components/url-classifier/tests/browser/browser_flashblock_off_with_always_activate.js:checkFlashBlockLists:34
12:16:50     INFO -  1327 INFO TEST-PASS | toolkit/components/url-classifier/tests/browser/browser_flashblock_off_with_always_activate.js | Plugin's existance in navigator.plugins should match expected -
12:16:50     INFO -  1328 INFO RUNNING TEST: Allowed domain (Ask to Activate, Flashblock off)
12:16:50     INFO -  1329 INFO Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://flashallow.example.com/browser/toolkit/components/url-classifier/tests/browser/flash_block_frame.html" line: 0}]
12:16:50     INFO -  1330 INFO TEST-PASS | toolkit/components/url-classifier/tests/browser/browser_flashblock_off_with_always_activate.js | Page's classification should match expected -
12:16:50     INFO -  1331 INFO TEST-PASS | toolkit/components/url-classifier/tests/browser/browser_flashblock_off_with_always_activate.js | Plugin should have the correct activation -
12:16:50     INFO -  Not taking screenshot here: see the one that was previously logged
12:16:50    ERROR -  1332 INFO TEST-UNEXPECTED-FAIL | toolkit/components/url-classifier/tests/browser/browser_flashblock_off_with_always_activate.js | Plugin should have the correct 'plugin running' state - Got false, expected true
12:16:50     INFO -  Stack trace:
12:16:50     INFO -  chrome://mochikit/content/browser-test.js:test_is:1283
12:16:50     INFO -  chrome://mochitests/content/browser/toolkit/components/url-classifier/tests/browser/classifierTester.js:checkPluginInfo:334
12:16:50     INFO -  chrome://mochitests/content/browser/toolkit/components/url-classifier/tests/browser/browser_flashblock_off_with_always_activate.js:checkFlashBlockLists:34
12:16:50     INFO -  1333 INFO TEST-PASS | toolkit/components/url-classifier/tests/browser/browser_flashblock_off_with_always_activate.js | Plugin's existance in navigator.plugins should match expected -
12:16:50     INFO -  1334 INFO RUNNING TEST: Allowed nested domain (Ask to Activate, Flashblock off)
12:16:50     INFO -  1335 INFO Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://example.com/browser/toolkit/components/url-classifier/tests/browser/flash_block_frame.html" line: 0}]
12:16:51     INFO -  1336 INFO TEST-PASS | toolkit/components/url-classifier/tests/browser/browser_flashblock_off_with_always_activate.js | Page's classification should match expected -
12:16:51     INFO -  1337 INFO TEST-PASS | toolkit/components/url-classifier/tests/browser/browser_flashblock_off_with_always_activate.js | Plugin should have the correct activation -
12:16:51     INFO -  Not taking screenshot here: see the one that was previously logged
12:16:51    ERROR -  1338 INFO TEST-UNEXPECTED-FAIL | toolkit/components/url-classifier/tests/browser/browser_flashblock_off_with_always_activate.js | Plugin should have the correct 'plugin running' state - Got false, expected true

Comment 28

[David Parks [:handyman]]

ugh.. I'm not seeing this locally.  Currently trying to determine if its pgo-only.

Comment 29

[David Parks [:handyman]]

Hey Stefan,

Are you sure that the test failures you reported in comment 27 aren't from bug 1447275?  From the time stamps, it looks like that was filed nearly the same time as your comment and this may have just gotten caught up in it.

Comment 30

[David Parks [:handyman]]

Nevermind.  Time stamps weren't the same time zone.

Comment 31

[David Parks [:handyman]]

The test's plugin-container fails on CreateFileW when loading the plugin here [1].

It happens on any opt build made by automation but not builds made locally.  This is true even if the build is run on the automation machines (so the issue isn't the machine configuration).  CreateFileW is intercepted by us here [2] for setting the Adobe 32-bit protected mode, and NtCreateFile, which it calls, is brokered by Chromium here [3].  I'm just getting into it but I don't currently see anything from the brokered call in the parent process.

[1] https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/dom/plugins/base/nsPluginsDirWin.cpp#164
[2] https://searchfox.org/mozilla-central/rev/68fdb6cf4f40bea1a1f6c07531ebf58fb8ab060b/dom/plugins/ipc/FunctionHook.cpp#235
[3] https://searchfox.org/mozilla-central/rev/68fdb6cf4f40bea1a1f6c07531ebf58fb8ab060b/security/sandbox/chromium/sandbox/win/src/filesystem_interception.cc#25

Comment 32

[David Parks [:handyman]]

Attachment: 1. Promote Windows plugin sandbox to level 3 (r=bobowen)

refresh

Comment 33

[David Parks [:handyman]]

Attachment: 2. Refactor EndpointHandler to make special type handling opt-in (r=jimm)

Comment 34

[David Parks [:handyman]]

Attachment: 3. Broker camera CreateMutexW calls from plugin to parent proc (r=jimm)

Comment 35

[David Parks [:handyman]]

Attachment: 4. Add plugin DLL to plugin sandbox exceptions list

As advertised, I needed to add the DLL to the exceptions list.  This fixes the Win 10 opt bc3 mochitest failure.

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

Comment 36

[Bob Owen (:bobowen)]

Comment on attachment 8973270 [details] [diff] [review]
4. Add plugin DLL to plugin sandbox exceptions list

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

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
@@ +787,5 @@
> +    // automation builds.  This policy restores access.
> +    if (aSandboxLevel >= 3) {
> +      MOZ_ASSERT(aPluginFilePath);
> +      result = mPolicy->AddRule(sandbox::TargetPolicy::SUBSYS_FILES,
> +                                sandbox::TargetPolicy::FILES_ALLOW_ANY,

I'm pretty uncomfortable about giving all access to this file.
I don't think we should ever need write access to load it, so if our checks do for some reason I think they're doing something wrong.

If it's read only, then I think we can use the existing mechanism like here:
https://searchfox.org/mozilla-central/rev/f30847c12e5fb13791401ed4330ced3e1b9c8d81/dom/media/gmp/GMPProcessParent.cpp#70

(There used to be a similar mechanism for adding allowed write rules.)

Comment 37

[David Parks [:handyman]]

Attachment: 1. Promote Windows plugin sandbox to level 3 (r=bobowen)

refresh

Comment 38

[David Parks [:handyman]]

Attachment: 2. Refactor EndpointHandler to make special type handling opt-in (r=jimm)

Comment 39

[David Parks [:handyman]]

Attachment: 3. Broker camera CreateMutexW calls from plugin to parent proc (r=jimm)

Comment 40

[David Parks [:handyman]]

Attachment: 4. Add plugin DLL to plugin sandbox exceptions list

Changed FILES_ALLOW_ANY to FILES_ALLOW_READONLY

Comment 41

[David Parks [:handyman]]

Tests (prior failure was mochitest bc3 opt):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea24ac25a13ba06b30ec69685c667f9aa3b3342c

Comment 42

[David Parks [:handyman]]

Attachment: 1. Promote Windows plugin sandbox to level 3 (r=bobowen)

Comment 43

[David Parks [:handyman]]

Attachment: 2. Refactor EndpointHandler to make special type handling opt-in (r=jimm)

Comment 44

[David Parks [:handyman]]

Attachment: 3. Broker camera CreateMutexW calls from plugin to parent proc (r=jimm)

Comment 45

[David Parks [:handyman]]

Attachment: 4. Add plugin DLL to plugin sandbox exceptions list

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

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55e0a25f9535
Part 1: Promote Windows plugin process sandbox to level 3. r=bobowen
https://hg.mozilla.org/integration/mozilla-inbound/rev/4636210e0b56
Part 2: Refactor EndpointHandler to make special type handling opt-in. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/25e12b5fae7f
Part 3: Broker camera CreateMutexW calls from plugin process. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/a910482f4598
Part 4: Add plugin DLL to plugin sandbox exceptions list. r=bobowen

Comment 47

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/55e0a25f9535
https://hg.mozilla.org/mozilla-central/rev/4636210e0b56
https://hg.mozilla.org/mozilla-central/rev/25e12b5fae7f
https://hg.mozilla.org/mozilla-central/rev/a910482f4598