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.

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

Thanks Tim. I'm going to hold off landing this for a bit, while I investigate bug 1217185.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ec207bba72780009baa5638dfff046bc2a8b640 Bug 1229829 - make sameBuffer more discriminating. r=waldo

(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.

(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.

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

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.

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.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af741aaec660219308862ccdb0e4834bc702ffab&group_state=expanded passing try run

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!

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

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

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

https://hg.mozilla.org/mozilla-central/rev/d342ab7ef7c3 https://hg.mozilla.org/mozilla-central/rev/6d9980e17a8c

This is a likely culprit for bug 1356365, which is a 57 blocker.