Closed
Bug 1365257
Opened 8 years ago
Closed 7 years ago
Consolidate MOZ_DISABLE_CONTENT_SANDBOX logic into GetEffectiveContentSandboxLevel
Categories
(Core :: Security: Process Sandboxing, enhancement)
Core
Security: Process Sandboxing
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: Alex_Gaynor, Assigned: Alex_Gaynor)
Details
(Whiteboard: sb+)
Attachments
(1 file)
Follow up to bug 1358223 - that moves reading the pref. Next step will be to hoist the MOZ_DISABLE_CONTENT_SANDBOX env var into that function as well.
Assignee | ||
Comment 1•8 years ago
|
||
Note to self: after reviewing the code, the primary thing this appears to do is disable setting up the brokers for Linux and Windows. On Linux it also causes `SetContentProcessSandbox` to bail out. I haven't traced all the call paths, but it looks like this assumes you're also setting `security.sandbox.content.level` to 0, or all it does is mess up your telemetry (on linux it looks like it'll mark `SANDBOX_CONTENT_ENABLED` if you have the env var set, even if the pref is set to a non-zero level). Consolidating all this logic into EffectiveContentSandboxLevel will also clean up this discrepancy.
Additional note to self: When we do this, we'll be up to multiple callers which really only need `effective-level > 0`, so we should also expose `IsContentSandboxEnabled` probably.
Comment 2•7 years ago
|
||
It might be worth taking a look at MOZ_DISABLE_GMP_SANDBOX handling as well. In particular, on Linux we're using that bit in SandboxEarlyInit, and we'll have the same requirement for content once content sandboxing is farther along. (I'd actually like to get rid of SandboxEarlyInit — bug 1322526 — but it's nontrivial.)
Updated•7 years ago
|
Whiteboard: sb+
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → agaynor
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
So, a challenge here is the |sandbox/common/| is currently a part of libxul, the |sandbox/linux/| code where I tried to use it is a part of libmozsandbox, so you get a link error.
macOS and Windows aren't affected, so I'd love some feedback from the Linux folks on what they think makes the most sense. Options are:
a) Ignore that callsite for now
b) Maybe |sandbox/common/| shouldn't be in libxul, and should be in it's own thing (that both libxul and libmozsandbox link)
c) Directly include the source files into libmozsandbox as well.
Flags: needinfo?(gpascutto)
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8873430 [details]
Bug 1365257 - Further consolidate the configuration of the content sandbox;
https://reviewboard.mozilla.org/r/144838/#review149064
::: security/sandbox/linux/SandboxInfo.cpp:230
(Diff revision 2)
> }
> }
> }
>
> #ifdef MOZ_CONTENT_SANDBOX
> - if (!getenv("MOZ_DISABLE_CONTENT_SANDBOX")) {
> + if (IsContentSandboxEnabled()) {
This gets called at static intializer time, so the preference service almost certainly won't work yet (on top of the problem of going backwards in the library dependency graph from libmozsandbox to libxul).
The singleton could be change to be lazily constructed (now that Primetime is gone), but that first use will wind up happening in `SandboxEarlyInit`, which might still be too early.
But here's an idea: you could just bypass the pref system and set `MOZ_DISABLE_CONTENT_SANDBOX` in the child's environment in `GeckoChildProcessHost` if the pref is 0. It's simple and it avoids all of the timing and linkage problems. That still leaves the parent process, but you could do something like `SandboxInfo::ThreadingCheck` that changes flags and call it from a pref observer.
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8873430 [details]
Bug 1365257 - Further consolidate the configuration of the content sandbox;
https://reviewboard.mozilla.org/r/144838/#review150312
See jld's remark.
Attachment #8873430 -
Flags: review-
Updated•7 years ago
|
Flags: needinfo?(gpascutto)
Comment 8•7 years ago
|
||
It seems worth revisiting this based on https://bugzilla.mozilla.org/show_bug.cgi?id=1412090#c44.
Even if we can't replace all checks by IsContentSandboxEnabled(), we can still move in the env var check and remove the duplication (and error-prone-ness) from the callers where that call is available.
Flags: needinfo?(agaynor)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(agaynor)
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8873430 [details]
Bug 1365257 - Further consolidate the configuration of the content sandbox;
https://reviewboard.mozilla.org/r/144838/#review203660
Attachment #8873430 -
Flags: review?(gpascutto) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 11•7 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/00edc1ac58f9
Further consolidate the configuration of the content sandbox; r=gcp
Keywords: checkin-needed
Comment 12•7 years ago
|
||
Backed out 1 changesets (bug 1365257) for failing gl in \build\build\src\obj-firefox\dist\include\mozilla/ServoStyleSet.h:97 r=backout on a CLOSED TREE
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=00edc1ac58f9e9eb7c2773013b95a6a87d1fcc3e&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=143777025
https://hg.mozilla.org/integration/autoland/rev/1be6d94e801583f8c38964a90d6130741decab17
Flags: needinfo?(agaynor)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(agaynor)
Keywords: checkin-needed
Comment 14•7 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/993f57169829
Further consolidate the configuration of the content sandbox; r=gcp
Keywords: checkin-needed
Comment 15•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•