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)

enhancement
Not set
normal

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.
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.
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.)
Whiteboard: sb+
Assignee: nobody → agaynor
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 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 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-
Flags: needinfo?(gpascutto)
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)
Flags: needinfo?(agaynor)
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+
Keywords: checkin-needed
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
Flags: needinfo?(agaynor)
Keywords: checkin-needed
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
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: