Closed Bug 1229829 Opened 8 years ago Closed 7 years ago

Use an alternate desktop for the Windows content sandbox by default.

Categories

(Core :: Security: Process Sandboxing, defect, P2)

All
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: bobowen, Assigned: Alex_Gaynor)

References

Details

(Whiteboard: sb+)

Attachments

(2 files, 2 obsolete files)

The only issue that I think this causes is when we also specify an alternate winstation (see bug 1151941).

So, I'll look at landing this on Nightly.
This also fixes a bug where we weren't setting parts of the policy correctly for levels 3 to 9.
Attachment #8694833 - Flags: review?(tabraldes)
Comment on attachment 8694833 [details] [diff] [review]
Use an alternate desktop for the Windows content sandbox by default

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

Good catch
Attachment #8694833 - Flags: review?(tabraldes) → review+
Thanks Tim.

I'm going to hold off landing this for a bit, while I investigate bug 1217185.
(In reply to Lars T Hansen [:lth] from comment #5)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 4ec207bba72780009baa5638dfff046bc2a8b640
> Bug 1229829 - make sameBuffer more discriminating. r=waldo

Really belongs to bug 1229828.
https://hg.mozilla.org/mozilla-central/rev/4ec207bba727
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(In reply to Carsten Book [:Tomcat] from comment #7)
> https://hg.mozilla.org/mozilla-central/rev/4ec207bba727

Bug 1229828 landed with this bug number by mistake.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: sbwc2
Target Milestone: mozilla46 → ---
I discovered with the way the chromium sandbox code currently works, you can't have some child processes with an alternate winstation and some without [1].

So to land this we'd need to change the GMP policy.
Need to work out what that means security wise and whether we want to make the sandbox able to cope with both or work towards landing bug 1151941 instead.

[1] https://dxr.mozilla.org/mozilla-central/rev/3461f3cae78495f100a0f7d3d2e0b89292d3ec02/security/sandbox/chromium/sandbox/win/src/sandbox_policy_base.cc#231
Assignee: bobowencode → nobody
Whiteboard: sbwc2 → sbwc3
Priority: -- → P1
Priority: P1 → P2
Whiteboard: sbwc3 → sb+
I started working on a patch for the chromium sandbox lib to allow us to some children with alternate winstation and some without: https://chromium-review.googlesource.com/c/615145

I've got a try rum here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f46518e156370679b55a9bf511a0a4d0dfe025d5 but it looks like there's a fair number of tests that will need to be investigated.
All the tests failures here are EDE/CDM/GMP related -- it looks like when we try to spawn the GMP process, the process gets created, but never actually tells the parent "I'm up and running" (the connected callback).

Careful digging revealed that the problem was that the desktop's integrity level wasn't being lowered.
Assignee: nobody → agaynor
Comment on attachment 8902275 [details]
Bug 1229829 - Part 2 - Use an alternate desktop on the local winstation for content processes;

https://reviewboard.mozilla.org/r/173810/#review179428

Thanks for this Alex.

I think we should roll 1 and 2 together, to remove any chance of introducing bisection confusion.
I'll wait until part 2 is committed to chromium before r+ing that.

::: security/sandbox/win/SandboxInitialization.cpp:70
(Diff revision 1)
>    sandbox::TargetPolicy* policy = brokerServices->CreatePolicy();
> +  // GMP processes use alternate desktops on alternate winstations (true), while
> +  // content processes use alternate desktops on a local winstation (false). We
> +  // pre-create both here.
>    sandbox::ResultCode result = policy->CreateAlternateDesktop(true);
> +  result = policy->CreateAlternateDesktop(false);

Hmm, from reading the above comment, it looks like I was wrong about this, sorry about that.
I'd forgotten that it's the switching to an alternate window station that means this code needs to be run early.

We can probably leave the alternate desktop on the interactive (current) window station to be created lazily.

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp:339
(Diff revision 1)
>                       "SetIntegrityLevel should never fail, what happened?");
>    result = mPolicy->SetDelayedIntegrityLevel(delayedIntegrityLevel);
>    MOZ_RELEASE_ASSERT(sandbox::SBOX_ALL_OK == result,
>                       "SetDelayedIntegrityLevel should never fail, what happened?");
>  
> -  if (aSandboxLevel > 3) {
> +  result = mPolicy->SetAlternateDesktop(false);

I think we should just change the SetAlternatDesktop argument to false and then change the sandbox level to 4 on Nightly only.

Hopefully this won't cause any new regressions, but I don't think we should enable it across all levels.

I know we want to get rid of the levels, but while we have them we might as well still use them to control the roll out.
Comment on attachment 8902275 [details]
Bug 1229829 - Part 2 - Use an alternate desktop on the local winstation for content processes;

https://reviewboard.mozilla.org/r/173810/#review179428

Sounds good, I'll roll them together once part 2 is committed to chromium!
Blocks: 1394370
Attachment #8902274 - Attachment is obsolete: true
Attachment #8902274 - Flags: review?(bobowencode)
Comment on attachment 8902273 [details]
Bug 1229829 - Part 1 - Apply chromium sandbox patches from upstream which improves alternate desktop support;

https://reviewboard.mozilla.org/r/173806/#review180714
Attachment #8902273 - Flags: review?(bobowencode) → review+
Comment on attachment 8902275 [details]
Bug 1229829 - Part 2 - Use an alternate desktop on the local winstation for content processes;

https://reviewboard.mozilla.org/r/173810/#review180716
Attachment #8902275 - Flags: review?(bobowencode) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d342ab7ef7c3
Part 1 - Apply chromium sandbox patches from upstream which improves alternate desktop support; r=bobowen
https://hg.mozilla.org/integration/autoland/rev/6d9980e17a8c
Part 2 - Use an alternate desktop on the local winstation for content processes; r=bobowen
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d342ab7ef7c3
https://hg.mozilla.org/mozilla-central/rev/6d9980e17a8c
Status: REOPENED → RESOLVED
Closed: 8 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
This is a likely culprit for bug 1356365, which is a 57 blocker.
Depends on: 1402340
Depends on: 1400637, 1401721
Depends on: 1401095
Blocks: 1401095
No longer depends on: 1401095
Blocks: 1415250
You need to log in before you can comment on or make changes to this bug.