928062 - Set low integrity on content processes for Windows sandboxing policy

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Brian R. Bondy [:bbondy]]

There are 2 tokens that you set when you start a sandboxed process. This bug is to set the 2nd token (after the content process calls LowerToken) to low integrity.

Comment 1

[Bob Owen (:bobowen)]

Since my last pull from m-c, running with the sandbox on windows seems pretty broken.

I've narrowed it down to this patch:
https://hg.mozilla.org/mozilla-central/rev/88b79c3c7d47

Looks like the when sandboxed we were relying on some of the deprecated code that has been removed.

I think the reason we are, is that this device creation was failing:
http://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxWindowsPlatform.cpp#466

and so we were falling back to something that kind of worked, but now doesn't.

It looks like the reason this creation is failing because the integrity level is set to untrusted (which is lower than what this bug is advocating).
This was done for bug 969559, I'm guessing that's because things seemed to work at untrusted, so it was left at that.

Brian - would you be OK if I raised the level to low, which was the original goal for this bug.
This allows the device to be created.
Does this still give us the file and pipe security benefits?

Note: the code gives a warning straight after creating the device as it tries to set a pref, but I'll follow that up in a separate bug.

Comment 2

[Brian R. Bondy [:bbondy]]

(In reply to Bob Owen (:bobowen) from comment #1)
> Since my last pull from m-c, running with the sandbox on windows seems
> pretty broken.
> 
> I've narrowed it down to this patch:
> https://hg.mozilla.org/mozilla-central/rev/88b79c3c7d47
> 
> Looks like the when sandboxed we were relying on some of the deprecated code
> that has been removed.
> 
> I think the reason we are, is that this device creation was failing:
> http://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxWindowsPlatform.
> cpp#466
> 
> and so we were falling back to something that kind of worked, but now
> doesn't.
> 
> It looks like the reason this creation is failing because the integrity
> level is set to untrusted (which is lower than what this bug is advocating).
> This was done for bug 969559, I'm guessing that's because things seemed to
> work at untrusted, so it was left at that.
> 
> Brian - would you be OK if I raised the level to low, which was the original
> goal for this bug.
> This allows the device to be created.
> Does this still give us the file and pipe security benefits?
> 
> Note: the code gives a warning straight after creating the device as it
> tries to set a pref, but I'll follow that up in a separate bug.

So this just stopped working with that changeset right?

Let's just allow setting it via the Sandbox Broker with our own enum in sandboxbroker.h.  Just make the enum values consistent but named differently than the ones in chromium. 
http://dxr.mozilla.org/mozilla-central/source/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp#33

Then feel free to adjust the policy as you want.  We'll have to decide eventually how locked down we want to be, but while we're providing *any* lock down in the meantime, it's an improvement over no lockdown :).

Maybe default the policy's integrity to what it is now but then change it here:
http://dxr.mozilla.org/mozilla-central/source/ipc/glue/GeckoChildProcessHost.cpp#819

Comment 3

[Brian R. Bondy [:bbondy]]

By the way this bug was specifically about this:
http://dxr.mozilla.org/mozilla-central/source/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp#54

So I'd do this work described in the previous comment in a new spin off bug.

Comment 4

[Bob Owen (:bobowen)]

Attachment: Bug 928062: Set Windows sandbox delayed integrity level to INTEGRITY_LEVEL_LOW.bug928062fix.patch

(In reply to Brian R. Bondy [:bbondy] from comment #2)
 
> So this just stopped working with that changeset right?

Yeah, the untrusted integrity level was causing it to fall back to some gfx thing that seems to have changed with that changeset.
It later causes an ASSERTION.
 
> Let's just allow setting it via the Sandbox Broker with our own enum in
> sandboxbroker.h.  Just make the enum values consistent but named differently
> than the ones in chromium. 
> http://dxr.mozilla.org/mozilla-central/source/security/sandbox/win/src/
> sandboxbroker/sandboxBroker.cpp#33

Well, I think we can just change the delayed integrity level.
This is what gets set as part of LowerToken().

The low integrity level allows me to write to:
C:\Users\<username>\AppData\Local\Temp\Low\

But not:
C:\Users\<username>\AppData\Local\Temp\
or
C:\Users\<username>\Desktop\

Comment 5

[Bob Owen (:bobowen)]

This change should only affect people who are compiling for and using the windows process sandbox, but here's a quick try push just to make sure nothing is broken:
https://tbpl.mozilla.org/?tree=Try&rev=84e2d207ece2

Comment 6

[Carsten Book [:Tomcat]]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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