Closed Bug 1718084 Opened 1 year ago Closed 1 year ago

Provide a way to blocklist certain subdirectories from an allowed dir.

Categories

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

x86_64
Linux
enhancement

Tracking

()

RESOLVED FIXED
92 Branch
Tracking Status
firefox-esr91 --- fixed
firefox92 --- fixed

People

(Reporter: gcp, Assigned: gerard-majax)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 1 obsolete file)

See bug 259356.

We'd like to allow .config and everything below, but block access to .config/mozilla, which would contain the profile data.

For added fun, we need to figure out whether we need to support
https://bugzilla.mozilla.org/show_bug.cgi?id=1393805
which is ~/.mozilla/systemextensionsdev

But, but, for extra extra fun, note that this is XRE_USER_SYS_EXTENSION_DEV_DIR, and we also have XRE_USER_SYS_EXTENSION_DIR, which is whitelisted on Windows (only) https://searchfox.org/mozilla-central/rev/b355f4e36eb45ef84ce9d3dd6af7f1a417ca8bfe/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp#165 if ENABLE_SYSTEM_EXTENSION_DIRS is set.

Blocks: 259356
Severity: -- → S4
Priority: -- → P5

From discussion, apparently bug 1524327 means the last 2 parts can go.

(In reply to Gian-Carlo Pascutto [:gcp] from comment #0)

We'd like to allow .config and everything below, but block access to .config/mozilla, which would contain the profile data.

From https://searchfox.org/mozilla-central/rev/5b3444ad300e244b5af4214212e22bd9e4b7088a/security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp#391-418 it seems like we already allow ~/.config/

I think you're going to run into some problems with how recursive permissions work: any rules for paths underneath them have the inherited permissions automatically OR'ed into them. It would be possible to add a new flag (OVERRIDE?) for rules like ~./config/mozilla where that shouldn't happen, but we also assume elsewhere that all rules will have MAY_ACCESS set, which we don't want here. So… maybe a DENY_ALL flag which overrides the other bits, similar to CRASH_INSTEAD, would be the simplest fix.

It would be nicer if we could allowlist the .config subdirectories we actually need; is there a reason that won't work?

(In reply to Jed Davis [:jld] ⟨⏰|UTC-6⟩ ⟦he/him⟧ from comment #5)

I think you're going to run into some problems with how recursive permissions work: any rules for paths underneath them have the inherited permissions automatically OR'ed into them. It would be possible to add a new flag (OVERRIDE?) for rules like ~./config/mozilla where that shouldn't happen, but we also assume elsewhere that all rules will have MAY_ACCESS set, which we don't want here. So… maybe a DENY_ALL flag which overrides the other bits, similar to CRASH_INSTEAD, would be the simplest fix.

Yeah, I just shared early versions but it's far from being finished, and this is exactly what I was running into and what I was expecting. So far I'm going with a MAY_DENY similar to what you suggest, and I'm poking where we need to change for the inheritance, whilst not breaking anything.

It would be nicer if we could allowlist the .config subdirectories we actually need; is there a reason that won't work?

Assignee: nobody → lissyx+mozillians
Attachment #9229866 - Attachment description: WIP: Bug 1718084 - Test allow $HOME/.config and block $HOME/.config/mozilla/ → Bug 1718084 - Test allow $HOME/.config and block $HOME/.config/mozilla/ r?gcp
Attachment #9229867 - Attachment description: WIP: Bug 1718084 - Block access to $HOME/.config/mozilla/ → Bug 1718084 - Block access to $HOME/.config/mozilla/ r?gcp
Attachment #9230225 - Attachment description: WIP: Bug 1718084 - Reorganize test for lower complexity → Bug 1718084 - Reorganize test for lower complexity r?gcp

(In reply to Gian-Carlo Pascutto [:gcp] from comment #0)

For added fun, we need to figure out whether we need to support
https://bugzilla.mozilla.org/show_bug.cgi?id=1393805
which is ~/.mozilla/systemextensionsdev

There is test code that ensures we keep this readable, so this looks fine: https://searchfox.org/mozilla-central/rev/79a3ae0b0fa6abeca2cb7cf220df293c8dec9207/security/sandbox/test/browser_content_sandbox_fs.js#350-358

Depends on D119179

Attachment #9232425 - Attachment description: Bug 1718084 - Split XDG-specific tests r?gcp → Bug 1718084 - Rework XDG-specific policy and tests r?gcp
Attachment #9232425 - Attachment is obsolete: true
Blocks: 1722272
Pushed by alissy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e17f3c2886ed
Reorganize test for lower complexity r=gcp
https://hg.mozilla.org/integration/autoland/rev/75c7384df123
Block access to $HOME/.config/mozilla/ r=gcp
https://hg.mozilla.org/integration/autoland/rev/28f0d0747745
Test allow $HOME/.config and block $HOME/.config/mozilla/ r=gcp

For added fun, we need to figure out whether we need to support
https://bugzilla.mozilla.org/show_bug.cgi?id=1393805
which is ~/.mozilla/systemextensionsdev

https://treeherder.mozilla.org/logviewer?job_id=346741721&repo=try&lineNumber=14197

INFO - TEST-UNEXPECTED-FAIL | security/sandbox/test/browser_content_sandbox_fs.js | reading system extensions dev dir from a web process is allowed (/builds/worker/.config/mozilla/systemextensionsdev) -

Blocks: 1723753

Bug 1732580 depends on this work, which we're trying to land on ESR91. Please nominate for approval.

Flags: needinfo?(lissyx+mozillians)

Comment on attachment 9229867 [details]
Bug 1718084 - Block access to $HOME/.config/mozilla/ r?gcp

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: easier to uplift bug 1732580
  • User impact if declined: broken webgl on snap
  • Fix Landed on Version: 92
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): has tests, no known regression, landed three months ago
  • String or UUID changes made by this patch:
Flags: needinfo?(lissyx+mozillians)
Attachment #9229867 - Flags: approval-mozilla-esr91?
Attachment #9229866 - Flags: approval-mozilla-esr91?
Attachment #9230225 - Flags: approval-mozilla-esr91?

Comment on attachment 9229866 [details]
Bug 1718084 - Test allow $HOME/.config and block $HOME/.config/mozilla/ r?gcp

Approved for 91.3esr, thanks.

Attachment #9229866 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+
Attachment #9229867 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+
Attachment #9230225 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+
You need to log in before you can comment on or make changes to this bug.