1126402 - Add a pref to enable a more strict version of the Windows NPAPI process sandbox.

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Bob Owen (:bobowen)]

The pref should enable the strongest policy that appears to work for the latest Flash, Silverlight and Java plugins.
This will enable the policy to be tested more thoroughly.

Comment 1

[Bob Owen (:bobowen)]

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

Comment 2

[Bob Owen (:bobowen)]

Attachment: Add a pref to enable a more strict version of the Windows NPAPI process sandbox.

This seems to be about as strong as I can get the sandbox settings before definitely breaking things.

These settings will not work with flash protected mode enabled.

The main things I see this blocking in the logs are:
* access to HKEY_LOCAL_MACHINE registry keys, probably because of setting "Authenticated Users" to deny only in the access token.

* NetBIOS over TCP/IP devices, which looks like some sort of scan of local machines.


I think it is much more likely that these settings could cause issues, hence the need to have them behind a pref for testing.

Comment 3

[Benjamin Smedberg]

Comment on attachment 8555891 [details] [diff] [review]
Add a pref to enable a more strict version of the Windows NPAPI process sandbox.

The pref-handling code here looks sane. I'm not sure I'm the right person to look at the actual rules... do you have somebody else who might be a good candidate for that, maybe TimA or aklotz?

I think we should try to get a build of this so that we can run Flash automated tests against the various sandbox settings and see what breaks.

Comment 4

[Bob Owen (:bobowen)]

Comment on attachment 8555891 [details] [diff] [review]
Add a pref to enable a more strict version of the Windows NPAPI process sandbox.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)

> The pref-handling code here looks sane. I'm not sure I'm the right person to
> look at the actual rules... do you have somebody else who might be a good
> candidate for that, maybe TimA or aklotz?
> 
> I think we should try to get a build of this so that we can run Flash
> automated tests against the various sandbox settings and see what breaks.

Thanks Ben.

I asked Brian if he wouldn't mind looking at this when he reviewed something else for me yesterday.

Comment 5

[Brian R. Bondy [:bbondy]]

Comment on attachment 8555891 [details] [diff] [review]
Add a pref to enable a more strict version of the Windows NPAPI process sandbox.

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

There's a possibility there are ways to lock down even further, but this patch makes it better (when the pref is explicitly enabled) than what it used to be.  So r+ing.

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
@@ +124,5 @@
>    }
>  
> +  sandbox::ResultCode result;
> +  bool ret;
> +  if (aMoreStrict) {

Gating this against a pref means any user mode process can change it, but I assume that's ok and it makes it no worse if changed from what it used to be.

@@ +132,3 @@
>  
> +    result = mPolicy->SetTokenLevel(sandbox::USER_RESTRICTED_SAME_ACCESS,
> +                                    sandbox::USER_INTERACTIVE);

Can this be lowered any more for the lowered token? For example USER_LIMITED.

@@ +168,2 @@
>  
>    result = mPolicy->SetDelayedIntegrityLevel(sandbox::INTEGRITY_LEVEL_MEDIUM);

Should we be changing the delayed integrity level as well differently?

Comment 6

[Bob Owen (:bobowen)]

(In reply to Brian R. Bondy [:bbondy] from comment #5)

Thanks Brian.

> @@ +132,3 @@
> >  
> > +    result = mPolicy->SetTokenLevel(sandbox::USER_RESTRICTED_SAME_ACCESS,
> > +                                    sandbox::USER_INTERACTIVE);
> 
> Can this be lowered any more for the lowered token? For example USER_LIMITED.

This removes the current user from the access token.

If I try this I have to give access to ...\AppData\Local\Temp\* as well to get it to work at all (this is probably so our disable protected mode hook can work).

I'd also probably have to give access to: 
...\AppData\Roaming\Macromedia\Flash Player\*
...\AppData\Roaming\Adobe\Flash Player\*

for some of the caching etc. to work.

My other concern is this may well render upload / download functionality useless.

It would be really good if we had some sort of test suite to try these things out.
 
> @@ +168,2 @@
> >  
> >    result = mPolicy->SetDelayedIntegrityLevel(sandbox::INTEGRITY_LEVEL_MEDIUM);
> 
> Should we be changing the delayed integrity level as well differently?

Changing to low integrity definitely breaks audio and we have the same problem for the content process.

Comment 7

[Chris Peterson [:cpeterson]]

Comment on attachment 8555891 [details] [diff] [review]
Add a pref to enable a more strict version of the Windows NPAPI process sandbox.

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

::: browser/app/profile/firefox.js
@@ +1189,5 @@
>  pref("dom.ipc.plugins.sandbox.flash", true);
>  
> +// This controls whether the Windows NPAPI process sandbox is using a more
> +// strict sandboxing policy.  This will require a restart.
> +pref("dom.ipc.plugins.moreStrictSandbox", false);

Instead of adding additional boolean prefs, maybe the "dom.ipc.plugins.sandbox.*" prefs can be truthy integers: 0 is no sandboxing and nonnegative values specify the successive layers of strict sandboxing you add over time. Boolean parameters are a code smell; once you have two states, you will probably need a third state for a new special case. :)

That would simplify some of the C++ checks below and make testing easier. For example, you could ask people to run some test comparing (say) sandbox levels 2 and 3.

Comment 8

[Bob Owen (:bobowen)]

(In reply to Chris Peterson [:cpeterson] from comment #7)
> Comment on attachment 8555891 [details] [diff] [review]
> Add a pref to enable a more strict version of the Windows NPAPI process
> sandbox.
> 
> Review of attachment 8555891 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/app/profile/firefox.js
> @@ +1189,5 @@
> >  pref("dom.ipc.plugins.sandbox.flash", true);
> >  
> > +// This controls whether the Windows NPAPI process sandbox is using a more
> > +// strict sandboxing policy.  This will require a restart.
> > +pref("dom.ipc.plugins.moreStrictSandbox", false);
> 
> Instead of adding additional boolean prefs, maybe the
> "dom.ipc.plugins.sandbox.*" prefs can be truthy integers: 0 is no sandboxing
> and nonnegative values specify the successive layers of strict sandboxing
> you add over time. Boolean parameters are a code smell; once you have two
> states, you will probably need a third state for a new special case. :)
> 
> That would simplify some of the C++ checks below and make testing easier.
> For example, you could ask people to run some test comparing (say) sandbox
> levels 2 and 3.

I was just in the process of copying you in on this bug. :-)

Yeah, that's definitely where I would like to take this, same for the content process sandbox.

The reason I haven't is because the way that the policies are set currently is a little inflexible.
So, I didn't want to add a switch statement to each of the policy setup functions, which are already called based on a switch statement.

Ideally I would like to pass some sort of policy object to the broker (or GeckoChildProcessHost), which is more controlled by the non-sandboxing code.
Then we could have these held in some sort of static array(s), to which the pref could map.
A null policy would mean no sandboxing.

But thinking about it, even having 0, 1, 2 is better than what I've just landed, as it would mean we could control the level per plugin.
I'll file a follow-up.

Comment 9

[Bob Owen (:bobowen)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ea243bbbb45c

Comment 10

[Carsten Book [:Tomcat]]

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