Closed Bug 928062 Opened 6 years ago Closed 6 years ago

Set low integrity on content processes for Windows sandboxing policy

Categories

(Core :: Security, defect)

x86_64
Windows NT
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: bbondy, Assigned: bobowen)

References

Details

Attachments

(1 file)

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.
Product: Calendar → Core
Summary: Set integrity on content processes for Windows sandboxing policy → Set low integrity on content processes for Windows sandboxing policy
Assignee: netzen → nobody
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.
Flags: needinfo?(netzen)
(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
Flags: needinfo?(netzen)
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.
(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\
Assignee: nobody → bobowencode
Attachment #8403346 - Flags: review?(aklotz)
Attachment #8403346 - Flags: review?(aklotz) → review+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/af5b63ae25d6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
See Also: → 724330
You need to log in before you can comment on or make changes to this bug.