1250125 - Make a 0 security.sandbox.content.level turn off the content process sandbox to allow Beta testing.

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement)

Description

[Bob Owen (:bobowen)]

In case we want to run experiments in Beta, it will be useful to be able to disable the content process sandbox completely using the pref.

Comment 1

[Bob Owen (:bobowen)]

Decided to change this, so that 0 turns off the sandbox entirely, the same as for NPAPI.
The USER_NON_ADMIN policy was useful at one point, but not so much now and using 0 for both is probably more intuitive.

Comment 2

[Bob Owen (:bobowen)]

Attachment: MozReview Request: Bug 1250125: Make a 0 security.sandbox.content.level turn off the content process sandbox. r?tabraldes

This also fixes a bug where we weren't setting parts of the policy correctly for levels 3 to 9.

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

Comment 3

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

https://reviewboard.mozilla.org/r/52165/#review49163

I'm not sure what "Open an Issue" means in the context of a review so we'll see what happens! In short, I'm nervous about the power of the pref in release versions of Firefox. I'm open to discussion about it - I don't clearly recall the past discussions that led to the current setup.

::: ipc/glue/GeckoChildProcessHost.cpp:990
(Diff revision 1)
>    // of reorganizing so I don't think this patch is the right time.
>    switch (mProcessType) {
>      case GeckoProcessType_Content:
>  #if defined(MOZ_CONTENT_SANDBOX)
> -      if (!PR_GetEnv("MOZ_DISABLE_CONTENT_SANDBOX")) {
> +      if (mSandboxLevel > 0 &&
> +          !PR_GetEnv("MOZ_DISABLE_CONTENT_SANDBOX")) {

It makes me nervous to have a pref (or specific pref value) in production versions of Firefox that is equivalent to MOZ_DISABLE_CONTENT_SANDBOX.

I don't recall all the details of past discussion about this - why does that pref exist in Firefox (as opposed to existing only in Nightly/Aurora/Beta or perhaps Debug versions of Firefox)?

If we're keeping the pref in release versions of Firefox, I'd like to see the meaning of a 0 value changed to "use default." My understanding is that we only want the ability to disable the sandbox via a pref in non-release versions of Firefox.

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp:138
(Diff revision 1)
>      initialIntegrityLevel = sandbox::INTEGRITY_LEVEL_LOW;
>      delayedIntegrityLevel = sandbox::INTEGRITY_LEVEL_LOW;
>    } else {
> -    jobLevel = sandbox::JOB_NONE;
> -    accessTokenLevel = sandbox::USER_NON_ADMIN;
> -    initialIntegrityLevel = sandbox::INTEGRITY_LEVEL_MEDIUM;
> +    // Shouldn't be called with aSandboxLevel < 1, we could MOZ_ASSERT here
> +    // after bug 1035125 lands.
> +    return false;

Hopefully consumers are checking their return values. It would be ideal if we did MOZ_ASSERT here like the comment says.

Since we can't yet do that, I'm inclined to have the first block in this 'if chain' be 'if (aSandboxLevel >= 20 || aSandboxLevel < 1)' so any malicious or misconfigured values result in maximum security. We could still return false just to indicate that the input was invalid.

Comment 4

[Bob Owen (:bobowen)]

https://reviewboard.mozilla.org/r/52165/#review49163

Thanks, "Open an Issue" is the right option I think.

> It makes me nervous to have a pref (or specific pref value) in production versions of Firefox that is equivalent to MOZ_DISABLE_CONTENT_SANDBOX.
> 
> I don't recall all the details of past discussion about this - why does that pref exist in Firefox (as opposed to existing only in Nightly/Aurora/Beta or perhaps Debug versions of Firefox)?
> 
> If we're keeping the pref in release versions of Firefox, I'd like to see the meaning of a 0 value changed to "use default." My understanding is that we only want the ability to disable the sandbox via a pref in non-release versions of Firefox.

This is just a tool to allow experiments, which are essentially addons as I understand it, to flip prefs for certain users, so we can test with the sandbox enabled and disabled.

Once we're ready to ship for real, we'd uplift a patch to Beta that would enforce a minimum, unless an env var was set.
This is what we did for NPAPI.
I don't want to have any channels where we allow the minimum policy to be reduced just by setting a pref. :-)

In reality even having the current 0 settings, would not provide a credible sandbox of any sort.

> Hopefully consumers are checking their return values. It would be ideal if we did MOZ_ASSERT here like the comment says.
> 
> Since we can't yet do that, I'm inclined to have the first block in this 'if chain' be 'if (aSandboxLevel >= 20 || aSandboxLevel < 1)' so any malicious or misconfigured values result in maximum security. We could still return false just to indicate that the input was invalid.

There's only one consumer and it doesn't check (but then it'll never pass < 1). :-)
Bug 1146873 is filed on that and I should get to that next week, now I'm getting through the other issues.

I should be able to add that MOZ_ASSERT really soon, as SandboxBroker is moving into xul as part of bug 1035125.

Comment 5

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

https://reviewboard.mozilla.org/r/52165/#review49163

> There's only one consumer and it doesn't check (but then it'll never pass < 1). :-)
> Bug 1146873 is filed on that and I should get to that next week, now I'm getting through the other issues.
> 
> I should be able to add that MOZ_ASSERT really soon, as SandboxBroker is moving into xul as part of bug 1035125.

OK I'll consider this fixed if we file a bug to follow up and add the assertion.

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 8751709 [details]
MozReview Request: Bug 1250125: Make a 0 security.sandbox.content.level turn off the content process sandbox. r?tabraldes

https://reviewboard.mozilla.org/r/52167/#review49185

Found the "Ship It" checkbox

Comment 7

[Bob Owen (:bobowen)]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #6)
> Comment on attachment 8751709 [details]
> MozReview Request: Bug 1250125: Make a 0 security.sandbox.content.level turn
> off the content process sandbox. r?tabraldes
> 
> https://reviewboard.mozilla.org/r/52167/#review49185
> 
> Found the "Ship It" checkbox

Thanks Tim, yes the MozReview UI is not very intuitive.

I was about to file the follow-up, but then I thought as this is not very urgent, I might as well wait until I get bug 1035125 landed and add the MOZ_ASSERT.

Comment 8

[Bob Owen (:bobowen)]

Comment on attachment 8751709 [details]
MozReview Request: Bug 1250125: Make a 0 security.sandbox.content.level turn off the content process sandbox. r?tabraldes

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

Comment 9

[Bob Owen (:bobowen)]

Added MOZ_ASSERT_UNREACHABLE for sandbox level < 1.

Also, noticed that I could remove the if from around the mitigations as this was for sandbox level >= 1.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/035a7be9628a

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/035a7be9628a