Closed Bug 1358223 Opened 7 years ago Closed 7 years ago

Hard code the lowest allowable content process sandbox at level 1 for Windows/OSX

Categories

(Core :: Security: Process Sandboxing, enhancement)

Product:
Core
Component:
Security: Process Sandboxing
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jimm, Assigned: Alex_Gaynor)

References

(Blocks 1 open bug)

Details

(Whiteboard: sbwc2, sbmc2, sblc3)

Attachments

(2 files)

Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1.
59 bytes, text/x-review-board-request
jimm
: review+
haik
: review+
bobowen
: review+
Details
Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value
59 bytes, text/x-review-board-request
Dexter
: review+
Details 
We're going to level 3 in our next rollout, so we'll be setting a minimum level of 1 to prevent users from accidentally disabling (or getting tricked into) turning the sandbox off through prefs.

We will keep one down level available (the newest) for testing and debugging issues that show up on release channels.
I'll pick this up at the same time as adding level 3, which I already partly started, before these other file content process regressions emerged.
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
(In reply to Bob Owen (:bobowen) from comment #1)
> I'll pick this up at the same time as adding level 3, which I already partly
> started, before these other file content process regressions emerged.

Thanks!
Assignee: bobowencode → jmathies
Summary: Hard code the content process sandbox at level 1 for Windows/OSX → Hard code the lowest allowable content process sandbox at level 1 for Windows/OSX
Assignee: jmathies → bobowencode
(Reasonably certain that :jimm assigning to himself was an accident)
Let's discuss this before adding the limit for Mac. For Mac:

We shipped level=1 in build 52 which blocks most filesystem write access.
We are running level=2 on Nightly at the moment which includes no read access to ~/Library and $PROFILE (with some exceptions).
We are planning to turn on level=3 in Build 55 which will remove read access to $HOME (at a minimum).

Level 2 and above depend on the content file process. Level 2 is acceptable for nightly, but can't be shipped without file content process but it prevents users from opening web files locally in some cases.

We expect content file process to be ready to ship in 55. The plan is to skip shipping of level 3 rather than to change the definition of level 2 to avoid confusion (internally) about levels. Of course, that is open for discussion.
> The plan is to skip shipping of level 3

Skip _to_ shipping level 3, right?
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #5)
> > The plan is to skip shipping of level 3
> 
> Skip _to_ shipping level 3, right?

Yes, thanks. I meant skip shipping level 2.
I thought I'd be touching this code, but I won't be, so someone else might get to this before I do.
Should be pretty straight forward case of checking when we get the pref security.sandbox.content.level and forcing it to be at least 1.

(In reply to Haik Aftandilian [:haik] from comment #6)
> (In reply to Alex Gaynor [:Alex_Gaynor] from comment #5)
> > > The plan is to skip shipping of level 3
> > 
> > Skip _to_ shipping level 3, right?
> 
> Yes, thanks. I meant skip shipping level 2.

If I understand correctly, it doesn't matter if we ship 2 or 3, as we'll be hard-coding the minimum to 1 either way.
Assignee: bobowencode → nobody
Status: ASSIGNED → NEW
Assignee: nobody → agaynor
I'll take a swing at this.
Attachment #8861446 - Flags: review?(bobowencode)
Blocks: 1359559
Comment on attachment 8861446 [details]
Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1.

https://reviewboard.mozilla.org/r/133440/#review136274

Thanks Alex, just a few nits.

::: browser/app/profile/firefox.js
(Diff revision 3)
>  #endif
>  
>  #if defined(XP_LINUX) && defined(MOZ_SANDBOX) && defined(MOZ_CONTENT_SANDBOX)
>  // This pref is introduced as part of bug 742434, the naming is inspired from
>  // its Windows/Mac counterpart, but on Linux it's an integer which means:
> -// 0 -> "no sandbox"

This is the linux one, which I don't think we should be changing.

::: dom/ipc/ContentChild.cpp:1416
(Diff revision 3)
>    }
>  #elif defined(XP_WIN)
>    mozilla::SandboxTarget::Instance()->StartSandbox();
>  #elif defined(XP_MACOSX)
> -  sandboxEnabled = StartMacOSContentSandbox();
> +  StartMacOSContentSandbox();
> +  sandboxEnabled = true;

nit: this is already initialised to true

::: ipc/glue/GeckoChildProcessHost.cpp:345
(Diff revision 3)
>  #if defined(MOZ_CONTENT_SANDBOX)
>    // We need to get the pref here as the process is launched off main thread.
>    if (mProcessType == GeckoProcessType_Content) {
>      mSandboxLevel = Preferences::GetInt("security.sandbox.content.level");
> +    if (mSandboxLevel < 1) {
> +        mSandboxLevel = 1;

nit: should only be indented by 2. This occurs in some other places as well.

::: security/sandbox/test/browser_content_sandbox_fs.js:171
(Diff revision 3)
>    ok(prefExists, "pref security.sandbox.content.level exists");
>    if (!prefExists) {
>      return;
>    }
>  
> +  if (level < 1) {

I don't think we should change any of these tests, they would catch it if someone mistakenly changed the pref file.

::: toolkit/xre/nsEmbedFunctions.cpp:316
(Diff revision 3)
>  AddContentSandboxLevelAnnotation()
>  {
>    if (XRE_GetProcessType() == GeckoProcessType_Content) {
>      int level = Preferences::GetInt("security.sandbox.content.level");
> +    if (level < 1) {
> +        level = 1;

All of this makes me think we should have some sort of central settings/configuration class to keep this logic, instead of it being spread throughout the code.

Might take a bit to get the design of that right, particularly cross platform. So it should be a follow-up, I've added it for discussion on the roundtable.

::: toolkit/xre/nsXREDirProvider.cpp:771
(Diff revision 3)
>  IsContentSandboxDisabled()
>  {
>    if (!BrowserTabsRemoteAutostart()) {
>      return false;
>    }
>  #if defined(XP_WIN) || defined(XP_MACOSX)

Existing issue ... but I'm assuming that we're already in a #if here or this wouldn't compile on Linux.

Also, that logic above looks wrong ... if e10s is off (!BrowserTabsRemoteAutostart()) then we should return true, as the sandbox is effectively disabled.
I missed that in the original review. :)

Anyway, looks like we can just return !BrowserTabsRemoteAutostart() for now.

I think it's worth keeping the separate method for now.
Attachment #8861446 - Flags: review?(bobowencode)
Attachment #8861446 - Flags: review?(haftandilian)
Comment on attachment 8861446 [details]
Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1.

https://reviewboard.mozilla.org/r/133440/#review137410

::: toolkit/xre/nsAppRunner.cpp:4174
(Diff revision 5)
>  #if defined(MOZ_CONTENT_SANDBOX) && !defined(MOZ_WIDGET_GONK)
>  void AddSandboxAnnotations()
>  {
>    // Include the sandbox content level, regardless of platform
>    int level = Preferences::GetInt("security.sandbox.content.level");
> +  if (level < 1) {

I was just about to r+ this, when I realised that we're going to get a bit of an inconsistency with what we're reporting in telemetry and in about:support.

We're not changing toolkit/modules/Troubleshoot.jsm (for about:support).
We're also not changing toolkit/components/telemetry/TelemetryEnvironment.jsm, which looks like it can't be done or at least not in a trivial way.

We are affecting the Telemetry annotation here and in toolkit/xre/nsEmbedFunctions.cpp.

Now in some ways that might be useful, because we'd know the effective sandbox level in telemetry as well as the actual pref, but I think it's worth discussing this at the next meeting before landing.
Just to make sure everybody is happy with it and knows about it, because it could be a cause of confusion when debugging future problems.
On IRC yesterday we discussed letting people still set it to 0 on Nightly, so that's also an actionitem for me. Since that'll introduce yet more code at every callsite, I want to introduce a |GetEffectiveSandboxLevel| function. Does anyone have a suggestion on where the right place in the codebase for that is, since it'll be called from all over.
Comment on attachment 8861446 [details]
Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1.

https://reviewboard.mozilla.org/r/133440/#review137410

> I was just about to r+ this, when I realised that we're going to get a bit of an inconsistency with what we're reporting in telemetry and in about:support.
> 
> We're not changing toolkit/modules/Troubleshoot.jsm (for about:support).
> We're also not changing toolkit/components/telemetry/TelemetryEnvironment.jsm, which looks like it can't be done or at least not in a trivial way.
> 
> We are affecting the Telemetry annotation here and in toolkit/xre/nsEmbedFunctions.cpp.
> 
> Now in some ways that might be useful, because we'd know the effective sandbox level in telemetry as well as the actual pref, but I think it's worth discussing this at the next meeting before landing.
> Just to make sure everybody is happy with it and knows about it, because it could be a cause of confusion when debugging future problems.

At a minimum, is there a good place for me to document this so that whoever's debugging next has a good chance at setting the docs?
Comment on attachment 8861446 [details]
Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1.

https://reviewboard.mozilla.org/r/133440/#review137852

Looks good. Waiting for discussion on the telemetry issue Bob commented on.

::: dom/ipc/ContentChild.cpp:242
(Diff revision 7)
>  using namespace mozilla::widget;
>  
>  namespace mozilla {
>  
> +int GetEffectiveSandboxLevel() {
> +  int level = Preferences::GetInt("security.sandbox.content.level");

Low priority corner case question:

Could we cache the level here and avoid re-reading the pref and also the possibility of getting into an undefined state after the pref is changed on the fly?

There are comments below in ContentParent.cpp (in original code) that imply our Linux sandbox could tolerate security.sandbox.content.level changing during the lifetime of a content process and that's it could be useful. On Mac, we wouldn't want to try to support that. I don't think that would work for Windows. If it's useful on Linux, then I'd vote to leave the code as is.

::: dom/ipc/ContentParent.cpp:2374
(Diff revision 7)
>    // XXX: Checking the pref here makes it possible to enable/disable sandboxing
>    // during an active session. Currently the pref is only used for testing
>    // purpose. If the decision is made to permanently rely on the pref, this
>    // should be changed so that it is required to restart firefox for the change
>    // of value to take effect.
> -  shouldSandbox = (Preferences::GetInt("security.sandbox.content.level") > 0) &&
> +  shouldSandbox = (GetEffectiveSandboxLevel() > 0) &&

You could add an IsSandboxed() method to hide the numerical level from callers that don't need to know about it.
Comment on attachment 8861446 [details]
Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1.

https://reviewboard.mozilla.org/r/133440/#review138502

::: dom/ipc/ContentParent.cpp:2374
(Diff revision 7)
>    // XXX: Checking the pref here makes it possible to enable/disable sandboxing
>    // during an active session. Currently the pref is only used for testing
>    // purpose. If the decision is made to permanently rely on the pref, this
>    // should be changed so that it is required to restart firefox for the change
>    // of value to take effect.
> -  shouldSandbox = (Preferences::GetInt("security.sandbox.content.level") > 0) &&
> +  shouldSandbox = (GetEffectiveSandboxLevel() > 0) &&

Two of these callers care; I'm not sure it's worth it since one of them is `IsContentSandboxDisabled`, I think it'd just make it more confusing.

I did realize that I should put `Content` in the function's name, so I'm going to go do that.
Comment on attachment 8861446 [details]
Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1.

https://reviewboard.mozilla.org/r/133440/#review137410

> At a minimum, is there a good place for me to document this so that whoever's debugging next has a good chance at setting the docs?

This now implements the logic we talked about on thursday: telemetry gets the effecitve level, `about:support` gets both.
Comment on attachment 8861446 [details]
Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1.

https://reviewboard.mozilla.org/r/133440/#review141292

::: dom/ipc/mozISandboxStatus.idl:8
(Diff revision 11)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsISupports.idl"
> +

Could use a comment describing the purpose of this interface and why its necessary.

::: dom/ipc/mozISandboxStatus.idl:10
(Diff revision 11)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsISupports.idl"
> +
> +[scriptable, builtinclass, uuid(5516303d-9007-45a0-94b9-940ef134a6e2)]
> +interface mozISandboxStatus : nsISupports

Just a question: is there a rule you followed to use "moz" as the name prefix?

::: dom/ipc/mozISandboxStatus.idl:12
(Diff revision 11)
> +#include "nsISupports.idl"
> +
> +[scriptable, builtinclass, uuid(5516303d-9007-45a0-94b9-940ef134a6e2)]
> +interface mozISandboxStatus : nsISupports
> +{
> +  long getEffectiveContentSandboxLevel();

Should this be a readonly attribute where the C++ implementation is a Set and Get method? I've seen that pattern a lot.
Comment on attachment 8861446 [details]
Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1.

https://reviewboard.mozilla.org/r/133440/#review141292

> Just a question: is there a rule you followed to use "moz" as the name prefix?

I _assumed_ (maybe incorrectly!) that the `ns` prefix was mostly for older ones, and `moz` was the new way of doing things. I don't think I saw any docs on naming conventions.
Comment on attachment 8861446 [details]
Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1.

https://reviewboard.mozilla.org/r/133440/#review141562

Sorry that (as is often the way with software development) this has turned into a more complicated change than I originally thought. :-)
(This is doubly true on Gecko.)

Thanks for starting to clean this stuff up, still a few more things, I'll clear the review so I know when it's ready again.
Mozreview seems to cause a lot of updates, so it's difficult to know otherwise.

::: dom/ipc/ContentChild.h:42
(Diff revision 11)
>  class RemoteSpellcheckEngineChild;
>  
> +// Return the current sandbox level. This is the
> +// "security.sandbox.content.level" preference, but rounded up to the current
> +// minimum allowed level.
> +int GetEffectiveContentSandboxLevel();

I'm not sure ContentChild is the right place for this.
On Windows at least we also read this in the parent, so seems a little odd.

Why not just have this code in the new class you have added?
It could have a normal header file as well with the same or a static member for C++ access.

::: dom/ipc/ContentChild.cpp:241
(Diff revision 11)
>  #endif
>  using namespace mozilla::widget;
>  
>  namespace mozilla {
>  
> +int GetEffectiveContentSandboxLevel() {

I thought we also agreed to include the effect of the environment variable MOZ_DISABLE_CONTENT_SANDBOX here as well. Maybe that could be a follow-up though.

Out of interest, does the env var work for Mac?

::: dom/ipc/mozISandboxStatus.idl:10
(Diff revision 11)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsISupports.idl"
> +
> +[scriptable, builtinclass, uuid(5516303d-9007-45a0-94b9-940ef134a6e2)]
> +interface mozISandboxStatus : nsISupports

I think this should go in security/sandbox.

I'm not keen on SandboxStatus.
Looking through the code SandboxConfig or SandboxSettings, would seem to fit in more.

Also I don't know enough about XPCOM to be confident in reviewing a new implementation. Maybe someone else on the team could check it over.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:979
(Diff revision 11)
>        if (policy.what == TelemetryEnvironment.RECORD_PREF_STATE) {
>          prefValue = "<user-set>";
>        } else {
>          prefValue = Preferences.get(pref, null);
>        }
> +      if (policy.postProcess) {

I'm not convinced this is the best solution.

Feels like it should probably, go in _currentEnvironment.settings updated in _updateSettings, now that this isn't simply a pref.

Either way this should probably have a Telemetry peer review, so maybe break this out into a separate patch.

::: xpcom/base/nsSystemInfo.cpp
(Diff revision 11)
>    return rv;
>  }
>  
>  NS_IMPL_ISUPPORTS_INHERITED(nsSystemInfo, nsHashPropertyBag, nsIObserver)
>  #endif // defined(XP_WIN)
> -

Is this a real change or some mozreview artifact?
Comment on attachment 8861446 [details]
Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1.

https://reviewboard.mozilla.org/r/133440/#review141562

> I thought we also agreed to include the effect of the environment variable MOZ_DISABLE_CONTENT_SANDBOX here as well. Maybe that could be a follow-up though.
> 
> Out of interest, does the env var work for Mac?

I thought we'd agreed not to change that behavior -- I didn't realize we wanted it in this patch. Happy to do it, but let's move to a follow up.

> I'm not convinced this is the best solution.
> 
> Feels like it should probably, go in _currentEnvironment.settings updated in _updateSettings, now that this isn't simply a pref.
> 
> Either way this should probably have a Telemetry peer review, so maybe break this out into a separate patch.

Agreed that it feels messy. My concern about just moving the value out of the prefs is that it then means telemetry's meaning has changed between before-and-after this patch, which might make analysis harder.
Attachment #8861446 - Flags: review?(jmathies)
Attachment #8861446 - Flags: review?(haftandilian)
Attachment #8861446 - Flags: review?(bobowencode)
Comment on attachment 8861446 [details]
Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1.

https://reviewboard.mozilla.org/r/133440/#review142254
Attachment #8861446 - Flags: review?(haftandilian) → review+
Comment on attachment 8861446 [details]
Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1.

https://reviewboard.mozilla.org/r/133440/#review142610
Attachment #8861446 - Flags: review?(jmathies) → review+
Attachment #8861446 - Flags: review?(bobowencode)
Attachment #8867358 - Flags: review?(alessio.placitelli)
:dexter, would love some feedback on the telemetry portion of this, specifically the changes I made adding `postProcess`, so we can get the effective value for the pref, instead of the raw value. Thanks.
Comment on attachment 8861446 [details]
Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1.

https://reviewboard.mozilla.org/r/133440/#review142946

Thanks for all your work on this.

::: security/sandbox/common/SandboxSettings.cpp:14
(Diff revision 17)
> +#include "mozilla/ModuleUtils.h"
> +#include "mozilla/Preferences.h"
> +
> +namespace mozilla {
> +
> +int GetEffectiveContentSandboxLevel() {

Please, file the follow-up on moving the MOZ_DISABLE_CONTENT_SANDBOX env var logic into here as well (if you haven't already :-) ).
Having that overriding it to 0, should work I think.

It'll be great to have it all in one place and mean we get consistent reporting for when that is used as well.

It will also mean we properly take account of it in some areas where we don't currently, that will be good but we'll have to be mindful of potential issues.
Attachment #8861446 - Flags: review+
Comment on attachment 8867358 [details]
Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value

https://reviewboard.mozilla.org/r/138882/#review142950

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:160
(Diff revision 1)
> +// Swap out the pref for the effective level to make our telemetry reports more
> +// accurate.
> +function processSandboxLevel(level) {
> +  let sandboxSettings = Cc["@mozilla.org/sandbox/sandbox-settings;1"].
> +                        getService(Ci.mozISandboxSettings);
> +  return sandboxSettings.effectiveContentSandboxLevel;

Can this ever throw? If so, it could disrupt gathering Telemetry for all the prefs :(

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:236
(Diff revision 1)
>    ["places.history.enabled", {what: RECORD_PREF_VALUE}],
>    ["privacy.trackingprotection.enabled", {what: RECORD_PREF_VALUE}],
>    ["privacy.donottrackheader.enabled", {what: RECORD_PREF_VALUE}],
>    ["security.mixed_content.block_active_content", {what: RECORD_PREF_VALUE}],
>    ["security.mixed_content.block_display_content", {what: RECORD_PREF_VALUE}],
> -  ["security.sandbox.content.level", {what: RECORD_PREF_VALUE}],
> +  ["security.sandbox.content.level", {what: RECORD_PREF_VALUE, postProcess: processSandboxLevel}],

Could we please add test coverage for the "postProcess" step in [test_TelemetryEnvironment.js](http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js)?

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:979
(Diff revision 1)
>        if (policy.what == TelemetryEnvironment.RECORD_PREF_STATE) {
>          prefValue = "<user-set>";
>        } else {
>          prefValue = Preferences.get(pref, null);
>        }
> +      if (policy.postProcess) {

This is only really meaningful for RECORD_PREF_VALUEs, so move it in the previous block.
Comment on attachment 8867358 [details]
Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value

https://reviewboard.mozilla.org/r/138882/#review142956

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:237
(Diff revision 1)
>    ["privacy.trackingprotection.enabled", {what: RECORD_PREF_VALUE}],
>    ["privacy.donottrackheader.enabled", {what: RECORD_PREF_VALUE}],
>    ["security.mixed_content.block_active_content", {what: RECORD_PREF_VALUE}],
>    ["security.mixed_content.block_display_content", {what: RECORD_PREF_VALUE}],
> -  ["security.sandbox.content.level", {what: RECORD_PREF_VALUE}],
> +  ["security.sandbox.content.level", {what: RECORD_PREF_VALUE, postProcess: processSandboxLevel}],
>    ["xpinstall.signatures.required", {what: RECORD_PREF_VALUE}],

Can we also document this change [here](https://dxr.mozilla.org/mozilla-central/rev/0f4df67c5f162e00d6f52825badf468aefbfba19/toolkit/components/telemetry/docs/data/environment.rst#336)?
Comment on attachment 8867358 [details]
Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value

https://reviewboard.mozilla.org/r/138882/#review142950

This looks good overall, thanks!
Comment on attachment 8867358 [details]
Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value

https://reviewboard.mozilla.org/r/138882/#review142972

On a second thought, this change is no longer reporting a 'pref' so the preferences section might not be the best place for this change afterall. Can we move it somewhere else, e.g. settings?

Sorry for the noise.
Attachment #8867358 - Flags: review?(alessio.placitelli) → review-
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #33)
> > I'm not convinced this is the best solution.
> > 
> > Feels like it should probably, go in _currentEnvironment.settings updated in _updateSettings, now that this isn't simply a pref.
> > 
> > Either way this should probably have a Telemetry peer review, so maybe break this out into a separate patch.
> 
> Agreed that it feels messy. My concern about just moving the value out of
> the prefs is that it then means telemetry's meaning has changed between
> before-and-after this patch, which might make analysis harder.

Alex, what's your concern about the analysis?
Flags: needinfo?(agaynor)
My concern is that if someone is tracking some metric, or correlation, involving the sandbox level, the data is no longer consistent between before-and-after this patch. For example, if we were watching "what sandbox level do people" use to see how many people this auto-upgrades, now we have to watch that across two metrics. Does that make sense?
Flags: needinfo?(agaynor)
Comment on attachment 8861446 [details]
Bug 1358223 - Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1.

https://reviewboard.mozilla.org/r/133440/#review142946

> Please, file the follow-up on moving the MOZ_DISABLE_CONTENT_SANDBOX env var logic into here as well (if you haven't already :-) ).
> Having that overriding it to 0, should work I think.
> 
> It'll be great to have it all in one place and mean we get consistent reporting for when that is used as well.
> 
> It will also mean we properly take account of it in some areas where we don't currently, that will be good but we'll have to be mindful of potential issues.

Bug 1365257
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #48)
> My concern is that if someone is tracking some metric, or correlation,
> involving the sandbox level, the data is no longer consistent between
> before-and-after this patch. For example, if we were watching "what sandbox
> level do people" use to see how many people this auto-upgrades, now we have
> to watch that across two metrics. Does that make sense?

It does, but the meaning of the "pref" would be changing on the Telemetry side anyway, so one would need to still pay attention to this.
This might be less subtle and prone to error by properly moving this information outside the "userPrefs" section.
That's not true -- the concept of an effective value doesn't exist until part 1 of this patch, so both pre-and-post this patch, the value in telemetry is the "currently running value".
I went ahead and implemented your original review comments, on the hope that maybe you found my position compelling :-) If not, reverting is easy enough.
Flags: needinfo?(alessio.placitelli)
Comment on attachment 8867358 [details]
Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value

https://reviewboard.mozilla.org/r/138882/#review143100

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:979
(Diff revision 2)
>        if (policy.what == TelemetryEnvironment.RECORD_PREF_STATE) {
>          prefValue = "<user-set>";
>        } else {
>          prefValue = Preferences.get(pref, null);
> +        if (policy.postProcess) {
> +          prefValue = policy.postProcess(prefValue);

In the light of one of the previous comments, should this have a try/catch?
Comment on attachment 8867358 [details]
Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value

https://reviewboard.mozilla.org/r/138882/#review143100

> In the light of one of the previous comments, should this have a try/catch?

The current `postProcess` function can never raise; I'll defer to :Dexter on what the right style for this code long term is.
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #51)
> That's not true -- the concept of an effective value doesn't exist until
> part 1 of this patch, so both pre-and-post this patch, the value in
> telemetry is the "currently running value".

Good point :-) The issue that remains is that this is not a pref any more, so it seems weird to keep it around like that to make analyses not consider two data points, at least at the beginning. Georg, do you have any opinion on this?
Flags: needinfo?(alessio.placitelli) → needinfo?(gfritzsche)
This part of the environment lists user prefs, not effective values.
I think it's confusing to do this. We have established patterns of adding these things as `settings.foo` properties, you should follow that.
If you don't need the original `userPref` entry, just remove it.
Flags: needinfo?(gfritzsche)
Comment on attachment 8867358 [details]
Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value

Cancelling the review as per comment 58.
Attachment #8867358 - Flags: review?(alessio.placitelli)
Comment on attachment 8867358 [details]
Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value

https://reviewboard.mozilla.org/r/138882/#review147940

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1282
(Diff revision 3)
> +    let sandboxSettings = Cc["@mozilla.org/sandbox/sandbox-settings;1"].
> +                          getService(Ci.mozISandboxSettings);
> +    return {
> +      effectiveContentProcessLevel: sandboxSettings.effectiveContentSandboxLevel
> +    };

Can you wrap these in a try/catch and return something meaningful if we throw? e.g. setting effectiveContentProcessLevel to null?

Let's also document this in environment.rst
Comment on attachment 8867358 [details]
Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value

https://reviewboard.mozilla.org/r/138882/#review147968

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1288
(Diff revision 4)
> +  _getSandboxData() {
> +    let effectiveContentProcessLevel = null;
> +    try {
> +      let sandboxSettings = Cc["@mozilla.org/sandbox/sandbox-settings;1"].
> +                            getService(Ci.mozISandboxSettings);
> +      effectiveContentProcessLevel = (

nit: why are the parentheses needed? Can't this simply be effectiveContentProcessLevel = sandboxSettings.effectiveContent; ?

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1292
(Diff revision 4)
> +                            getService(Ci.mozISandboxSettings);
> +      effectiveContentProcessLevel = (
> +        sandboxSettings.effectiveContentSandboxLevel);
> +    } catch (e) {}
> +    return {
> +      effectiveContentProcessLevel,

Shouldn't this be (maybe renaming the effectiveContentProcessLevel variable to something like effectiveLevel for clarity :) ):

```
{
 effectiveContentProcessLevel = effectiveLevel,
}
``` 

?

Otherwise the environment will contain "sandbox: <integer>" instead of "sandbox: { effectiveContentProcessLevel: <integer> }", which is different than what described in the docs
Comment on attachment 8867358 [details]
Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value

https://reviewboard.mozilla.org/r/138882/#review147968

> nit: why are the parentheses needed? Can't this simply be effectiveContentProcessLevel = sandboxSettings.effectiveContent; ?

To avoid wrapping beyond 80 columns. If that's not the prefered style, I can make it a long line.

> Shouldn't this be (maybe renaming the effectiveContentProcessLevel variable to something like effectiveLevel for clarity :) ):
> 
> ```
> {
>  effectiveContentProcessLevel = effectiveLevel,
> }
> ``` 
> 
> ?
> 
> Otherwise the environment will contain "sandbox: <integer>" instead of "sandbox: { effectiveContentProcessLevel: <integer> }", which is different than what described in the docs

`{name}` is equivilant to `{name: name}` and `eslint` told me to use the short version.
Comment on attachment 8867358 [details]
Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value

https://reviewboard.mozilla.org/r/138882/#review147982

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1288
(Diff revision 4)
> +  _getSandboxData() {
> +    let effectiveContentProcessLevel = null;
> +    try {
> +      let sandboxSettings = Cc["@mozilla.org/sandbox/sandbox-settings;1"].
> +                            getService(Ci.mozISandboxSettings);
> +      effectiveContentProcessLevel = (

Good point. Given that we do something similar at line 1276, do we need parentheses here?

I'm fine with breaking the line without them to keep it within the 80/90 chars.
Comment on attachment 8867358 [details]
Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value

https://reviewboard.mozilla.org/r/138882/#review147968

> `{name}` is equivilant to `{name: name}` and `eslint` told me to use the short version.

Wow, I didn't know that! Thanks, looks good then!
Comment on attachment 8867358 [details]
Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value

https://reviewboard.mozilla.org/r/138882/#review147982

> Good point. Given that we do something similar at line 1276, do we need parentheses here?
> 
> I'm fine with breaking the line without them to keep it within the 80/90 chars.

You're right, no parens needed. I forgot JS is happy with this, too much python :-)
Comment on attachment 8867358 [details]
Bug 1358223 - Part 2 - In telemetry send the effective sandbox level instead of the raw value

https://reviewboard.mozilla.org/r/138882/#review148214

This looks good now, thanks for the patience! Please note that we need to update the schema for all the pings using the 'environment' [here](https://github.com/mozilla-services/mozilla-pipeline-schemas/tree/master/schemas/telemetry) too. We don't need to block this bug on this, but can you please file a follow-up bug?
Attachment #8867358 - Flags: review?(alessio.placitelli) → review+
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Keywords: checkin-needed
Keywords: checkin-needed
Thanks for the heads up -- made sure all the issues in MozReview were resolved.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4e283b54baa6
Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1. r=bobowen,haik,jimm
https://hg.mozilla.org/integration/autoland/rev/852ffb5b89bf
Part 2 - In telemetry send the effective sandbox level instead of the raw value r=Dexter
Keywords: checkin-needed
Flags: needinfo?(agaynor)
Keywords: checkin-needed
Should be all fixed up now! Confirmed in try that everything looks good on Android once again.
hg error in cmd: hg pull gecko -r 39b29adcf17531edace6af46ff5c09980ec18e96: pulling from https://reviewboard-hg.mozilla.org/gecko
abort: HTTP Error 500: Internal Server Error
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/39941ecd6096
Part 1 - On Windows and macOS hardcode the minimum content sandbox level at 1. r=bobowen,haik,jimm
https://hg.mozilla.org/integration/autoland/rev/533d0f200498
Part 2 - In telemetry send the effective sandbox level instead of the raw value r=Dexter
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/39941ecd6096
https://hg.mozilla.org/mozilla-central/rev/533d0f200498
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1370438
> +effectiveContentSandboxLevel = Effective Content Process Sandbox Level

Out of l10n interest: Is this about
- "the level of the/an effective content process sandbox" i.e. [[Effective Content Process] Sandbox] Level"
- "the sandbox level of the/an effective content process" i.e. [[Effective Content Process] Sandbox Level]"
- "the effective level of the content process sandbox" i.e. [Effective [Content Process Sandbox] Level]"
- or something else?

The "ease" of English allows multiple nouns to simply follow up in compound nouns, where a word like "A B C D" usually means "D x C x B x A", where "x" refers to "of/from/for" and D is the actual noun. Longer compounds nouns like these however can be confusing for l10n since miles may vary (i.e. it’s not always like this), so I’m not surporised to see different localizations per locale for this one currently. A l10n note would therefor be welcome, and the existing "Content Process Sandbox Level" string should be clear(er) when this one is.

https://transvision.mozfr.org/string/?entity=toolkit/chrome/global/aboutSupport.properties:effectiveContentSandboxLevel&repo=central

Also, "effective" may lead up to different interpretations - is this something like "actual/active/working" here?

Please don’t reply in a too technical way, not all localizers have developer skills. ;)
It is "the effective level of the content process sandbox". Cheers!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: