Closed Bug 1375863 Opened 2 years ago Closed 6 months ago

Merge MOZ_SANDBOX and MOZ_CONTENT_SANDBOX

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: jimm, Assigned: Alex_Gaynor)

References

Details

(Whiteboard: sb+)

Attachments

(1 file)

<jimm> what's the difference between MOZ_CONTENT_SANDBOX and MOZ_SANDBOX?
<Alex_Gaynor> jimm: I think MOZ_SANDBOX is if we sandbox _anything_ (GMP, etc.) CONTENT_SANDBOX is for content processes specifically
<jimm> huh, thx
<jimm> I wonder if we can remove the content one
<jimm> bobowen: ^?
<Alex_Gaynor> Just fold it into MOZ_SANDBOX? Seems prima facie reasonable to me.
<Alex_Gaynor> haik: sounds good; thanks
<jimm> probably has something to do with the funky linking we have to do for content processes
<jimm> (funky linking of the sandbox lib)
<bobowen> I agree, now that we sandboxing content on all platforms we can merge them
Yes, MOZ_SANDBOX == MOZ_CONTENT_SANDBOX || MOZ_GMP_SANDBOX.

If we're going to make this change, is there any reason not to just merge all three flags?  I can see use cases for desktop-but-not-GMP sandboxing, but not really for the reverse.  (If GMP itself is disabled, the linker should be able to GC most of the GMP-only sandboxing code, and I don't think there's much of that to begin with.)

Also, if we're going to force --disable-gmp-sandbox on anyone with a reason to use --disable-content-sandbox, we should make a decision on whether that means running GMP unsandboxed (the current behavior) or disabling GMP (as proposed in bug 1141825).
See Also: → 1141825, 1357594
Whiteboard: sb+
Priority: -- → P3
Bug 1402133 is making me think we'd be better off merging these: the Linux build with --enable-content-sandbox --disable-gmp-sandbox has apparently been broken for as long as the code has existed.  Also, the configure logic has some weird corner cases that I don't enjoy explaining, like --enable-sandbox not actually enabling anything if it's an unsupported platform.

There's still the question of --disable-content-sandbox --enable-gmp-sandbox, but we're not enforcing a minimum value for security.sandbox.content.level on Linux (yet), so there's still the alternative of setting the pref.

Also, --{en,dis}able-gmp-sandbox don't exist and never have; MOZ_GMP_SANDBOX is defined iff --enable-sandbox and x86, unless you edit the configure script.  So that's another oddity we could get rid of by flattening everything into MOZ_SANDBOX.
This should be doable by:

1. ripping out the MOZ_(CONTENT|GMP)_SANDBOX parts of old-configure.in, except that the architecture restrictions for Linux should now apply to MOZ_SANDBOX instead

2. globally replacing MOZ_(CONTENT|GMP)_SANDBOX with MOZ_SANDBOX

3. going through security/sandbox to remove the redundant MOZ_SANDBOX ifdefs left by step 2 (but not security/sandbox/chromium, which has some deliberate use of MOZ_SANDBOX patched in and I assume it's like that for a reason); there are about 30 of them, so it'll be mildly annoying but not awful

(As a more ambitious alternative to step 1, replace the remaining autoconf with moz.configure scripting.)
Assignee: nobody → agaynor
Priority: P3 → P1
Attachment #9049984 - Attachment description: Bug 1375863 - fold MOZ_CONTEN_SANDBOX and MOZ_GMP_SANDBOX into MOZ_SANDBOX; r?jld → Bug 1375863 - fold MOZ_CONTENT_SANDBOX and MOZ_GMP_SANDBOX into MOZ_SANDBOX; r?jld
Keywords: checkin-needed

Pushed by dvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dc8935d7c0b1
fold MOZ_CONTENT_SANDBOX and MOZ_GMP_SANDBOX into MOZ_SANDBOX; r=jld,firefox-build-system-reviewers

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Regressions: 1579323
You need to log in before you can comment on or make changes to this bug.