1365257 - Consolidate MOZ_DISABLE_CONTENT_SANDBOX logic into GetEffectiveContentSandboxLevel

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement)

Description

[Alex Gaynor [:Alex_Gaynor]]

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.

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

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

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.)

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1365257 - Further consolidate the configuration of the content sandbox;

This patch moves handling of the "MOZ_DISABLE_CONTENT_SANDBOX" environment
variable into GetEffectiveContentSandboxLevel. It also introduces
IsContentSandboxEnabled and ports many users of GetEffectiveContentSandboxLevel
to use it.

Review commit: https://reviewboard.mozilla.org/r/144838/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144838/

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8873430 [details]
Bug 1365257 - Further consolidate the configuration of the content sandbox;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144838/diff/1-2/

Comment 5

[Alex Gaynor [:Alex_Gaynor]]

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.

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 9

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8873430 [details]
Bug 1365257 - Further consolidate the configuration of the content sandbox;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144838/diff/2-3/

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8873430 [details]
Bug 1365257 - Further consolidate the configuration of the content sandbox;

https://reviewboard.mozilla.org/r/144838/#review203660

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/00edc1ac58f9
Further consolidate the configuration of the content sandbox; r=gcp

Comment 12

[Stefan Hindli [:stefan_hindli]]

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

Comment 13

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8873430 [details]
Bug 1365257 - Further consolidate the configuration of the content sandbox;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144838/diff/3-4/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/993f57169829
Further consolidate the configuration of the content sandbox; r=gcp

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/993f57169829