Open Bug 1557282 Opened 6 months ago Updated 3 days ago

Add SetLockdownDefaultDacl to the content process sandbox policy for Windows.

Categories

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

All
Windows
enhancement

Tracking

()

REOPENED
Tracking Status
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 + wontfix

People

(Reporter: bobowen, Assigned: bobowen)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

I noticed this additional setting that we haven't enabled yet, which shouldn't cause any issues.
r=jmathies via IRC.

Attachment #9070216 - Flags: review+
Pushed by bobowencode@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0039b4ffc66b
Add SetLockdownDefaultDacl to the content process sandbox policy. r=jmathies

Comment on attachment 9070216 [details] [diff] [review]
Add SetLockdownDefaultDacl to the content process sandbox policy.

Beta/Release Uplift Approval Request

  • User impact if declined: They won't get this small improvement to the sandbox policy.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Just enabling a minor feature in the sandbox policy.
  • String changes made/needed: None
Attachment #9070216 - Flags: approval-mozilla-beta?

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Very simple enhancement to content sandbox policy.
  • User impact if declined: They won't get this small improvement to the sandbox policy.
  • Fix Landed on Version: 68
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Just enabling a minor feature in the sandbox policy.
  • String or UUID changes made by this patch: None
Attachment #9070240 - Flags: approval-mozilla-esr60?
Flags: needinfo?(jmathies)
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/adfee51b222f
Backed out changeset 0039b4ffc66b for causing permafailures in z:/build/build/src/dom/media/GraphDriver.cpp:522 CLOSED TREE

I'll look into this.

Flags: needinfo?(jmathies) → needinfo?(bobowencode)
Comment on attachment 9070216 [details] [diff] [review]
Add SetLockdownDefaultDacl to the content process sandbox policy.

I'll clear the uplift flag for now until this has re-landed to get it off my list.
Attachment #9070216 - Flags: approval-mozilla-beta?
Comment on attachment 9070240 [details] [diff] [review]
ESR60 - Add SetLockdownDefaultDacl to the content process sandbox policy.

Clearing the ESR60 request for now too until this lands and sticks.
Attachment #9070240 - Flags: approval-mozilla-esr60?

I've burned a fair amount of time trying to see why this breaks audio on Windows before version 10 (I've tried win8.1 and it's broken there as well).
The failing call in our code is at [1], but I've been unable to pinpoint where it fails below that.
Sometimes you can see an obvious difference where a system call returns an access denied code, but here it looks like something must be failing further down, but you don't see the issue until it fails to try and pull something out of a list or possibly when it tries to QI. Even this is a bit unclear.

So, I think the best course of action is to get this turned on for windows 10 and block the turning on for other versions on audio remoting.
I'll also put this behind a bump of the sandbox level to 6, so we can turn it off easily if we hit problems in win10 as well.
It's probably also good to do this just as we've moved to 70, so we have plenty of time for any issues to surface.

[1] https://searchfox.org/mozilla-central/rev/40ef22080910c2e2c27d9e2120642376b1d8b8b2/media/libcubeb/src/cubeb_wasapi.cpp#1840

Flags: needinfo?(bobowencode)
Summary: Add SetLockdownDefaultDacl to the content process sandbox policy. → Add SetLockdownDefaultDacl to the content process sandbox policy for Windows 10 or later.
Attachment #9070216 - Attachment is obsolete: true
Attachment #9070240 - Attachment is obsolete: true
Pushed by bobowencode@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9559ef8f347d
SetLockdownDefaultDacl for content process sandbox policy for Windows 10 or later. r=jmathies
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

Is this something we need to consider for backport?

Flags: needinfo?(bobowencode)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)

Is this something we need to consider for backport?

Yes, although I'm looking at that possible performance regression at the moment.

Regressions: 1567662

Re-opening this because it's been disabled because of regressions, including audio on Windows 10, which we'd only seen on Windows 7 & 8 before.

Status: RESOLVED → REOPENED
Flags: needinfo?(bobowencode)
Resolution: FIXED → ---

Too late for this cycle.

Duplicate of this bug: 1564842

The audio problems that I thought were only win8.1 and earlier can occur on win10 as well.

Depends on: 1432303
Priority: P1 → P2
Summary: Add SetLockdownDefaultDacl to the content process sandbox policy for Windows 10 or later. → Add SetLockdownDefaultDacl to the content process sandbox policy for Windows.
Target Milestone: mozilla70 → ---
You need to log in before you can comment on or make changes to this bug.