928044 - Enable an open sandbox by default on Windows (when e10s enabled)

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Brian R. Bondy [:bbondy]]

Bug 928042 adds preference to enable content sandboxing. This bug is to enable that pref by default.

Comment 1

[Brian R. Bondy [:bbondy]]

bbondy
Does anyone have a problem with me doing bug 928044? Enabling building the Chromium sandbox by default on Windows and only using it when browser.tabs.remote is true? I could define MOZ_CONTENT_SANDBOX for windows inside confvars.sh so it is only Nightly.

bsmedberg
1) how much does the code weigh/is there a regression risk 2) can users control it 3) have you checked with billm and the e10s team? ;-)

bsmedberg
bbondy: the question of whether we build it and whether we turn it on (for desktop or for metro) should probably be separate

ehsan
bbondy: sounds like a dev-platform topic?

bbondy
k I'll get a bit more data then post on dev-platform

Comment 2

[Brian R. Bondy [:bbondy]]

I'll be posting about this on dev-platform next week but just wanted to give you guys a heads up and see if you had any initial feedback.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Worth posting on dev-sandbox too.  :)

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I think it would be best to turn it on at compile-time on nightly, but leave it behind a pref that's disabled by default. Then we can test it out for a week or so to get a feel for how stable it is. I don't think we should set a very high bar for it though. If a typical user can browse around for ~10 minutes and usually not crash, then I think it would be fine to enable it by default.

Comment 5

[Brian R. Bondy [:bbondy]]

There was some push back about adding a pref in bug 928042, but we have an environment variable that people could set if they run into trouble: MOZ_DISABLE_CONTENT_SANDBOX

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Whether it's a pref or an environment variable, I think my main concern is that it should be disabled by default at runtime. It sounds like the environment variable makes it enabled by default.

Comment 7

[Brian R. Bondy [:bbondy]]

Ya we'll have to swap to the opposite logic for the current implementation, np easy change.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Sounds good. Thanks.

Comment 9

[Bob Owen (:bobowen)]

I'm going to use this bug for the patch to turn on a weak Windows content sandbox by default when in e10s mode.

This changes the pref introduced in bug 1018966, so that the content sandbox is on by default and you can enable a more strict version of the sandbox, which is very similar to the current settings.

It will also change the "warn only logging" that was controlled by the same pref from bug 1018966, to a log mode that can be used with more than just the content process. This will resolve bug 1083850.
It should not make the sandbox weaker, but puts in dummy interceptions to allow us to hook in the logging.

I still need to check some things, but here's a Try push with the current version of the patch:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0f334640002c

The failures are either identical to or very similar to the ones we see with just e10s enabled.

Comment 10

[Chris Peterson [:cpeterson]]

Sandboxing does not block e10s MVP.

Comment 11

[Bob Owen (:bobowen)]

Attachment: Part 1: Remove Chromium interception logging changes.

This removes the warn only sandbox logging code from the Chromium code.

I'm doing it like this, just so that I can change things around in Part 2 without breaking things here.
I can then reapply the modified logging changes to the Chromium code in Part 3 and keep the Chromium changes in a separate patch.

Comment 12

[Bob Owen (:bobowen)]

Attachment: Part 2: Enable the content sandbox by default on Windows with a reduced policy.

Tim - sorry about dropping these reviews on you as your trying to start on something new, but you know these changes the best I think.
Ben - and sorry to drop another review on you, but there aren't many people that cover all the various areas I'm touching.

This patch turns the Windows content sandbox on by default, albeit with a very weak policy.

The browser.tabs.remote.sandbox pref has been removed and two new prefs have been added:

security.sandbox.windows.log - this now turns on logging in a way that does not effectively weaken the sandbox and so can be used in more process types.
As not all process types have access to prefs this can also be turned on by setting a MOZ_WIN_SANDBOX_LOGGING environment Variable.

security.sandbox.windows.content.moreStrict - this enables a sandbox with a more strict policy. This is so we can test a stronger policy as we try to lock things down.

The browser.tabs.remote.sandbox.warnOnlyStackTraceDepth pref has been moved to security.sandbox.windows.log.stackTraceDepth

Sorry about the size of the patch, it was difficult to split it out in any sensible way.

Comment 13

[Bob Owen (:bobowen)]

Attachment: Part 3: Add logging changes back into the Chromium interception code.

This adds the new versions of the logging code back into the Chromium code.

I've not added the logging back into NtOpenProcess as this just seemed to generate noise and it is something that gets enabled by default by the sandbox, so it isn't anything we could change anyway.

Comment 14

[Chris Peterson [:cpeterson]]

Bob: the e10s team plays to enable e10s on Nightly this week (as part of the #fxdecade announcements). How risky is enabling the Windows sandbox? To avoid e10s surprises, can we delay the Windows sandbox until next week?

Comment 15

[Bob Owen (:bobowen)]

(In reply to Chris Peterson (:cpeterson) from comment #14)
> Bob: the e10s team plays to enable e10s on Nightly this week (as part of the
> #fxdecade announcements). How risky is enabling the Windows sandbox? To
> avoid e10s surprises, can we delay the Windows sandbox until next week?

Sure, I was thinking of landing on Holly first anyway, because I didn't want to cause any problems with all the people running e10s on Nightly.

Comment 16

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

Comment on attachment 8516715 [details] [diff] [review]
Part 2: Enable the content sandbox by default on Windows with a reduced policy.

Review of attachment 8516715 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Exciting progress!

::: browser/app/profile/firefox.js
@@ +1186,5 @@
>  
> +#if defined(XP_WIN)
> +#if defined(MOZ_SANDBOX)
> +// When this pref is true the Windows process sandbox will set up dummy
> +// interecptions and log to the browser console when calls fail in the sandboxed

nit: interecptions->interceptions

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +98,5 @@
> +#if defined(XP_WIN)
> +#if defined(MOZ_CONTENT_SANDBOX)
> +    mMoreStrictContentSandbox(false),
> +#endif
> +#if defined(MOZ_SANDBOX)

Should this be set up nested (with the MOZ_CONTENT_SANDBOX block inside the MOZ_SANDBOX block)? Everywhere else seems to be set up that way

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
@@ +96,5 @@
> +    result = mPolicy->SetJobLevel(sandbox::JOB_NONE, 0);
> +    bool ret = (sandbox::SBOX_ALL_OK == result);
> +
> +    result = mPolicy->SetTokenLevel(sandbox::USER_RESTRICTED_SAME_ACCESS,
> +                                    sandbox::USER_NON_ADMIN);

Does this work better than USER_RESTRICTED_SAME_ACCESS? What's the reasoning for changing this?

Comment 17

[Bob Owen (:bobowen)]

Thanks for the reviews Tim.

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #16)

> > +// interecptions and log to the browser console when calls fail in the sandboxed
> 
> nit: interecptions->interceptions

Fixed locally, thanks.
 
> ::: ipc/glue/GeckoChildProcessHost.cpp
> @@ +98,5 @@
> > +#if defined(XP_WIN)
> > +#if defined(MOZ_CONTENT_SANDBOX)
> > +    mMoreStrictContentSandbox(false),
> > +#endif
> > +#if defined(MOZ_SANDBOX)
> 
> Should this be set up nested (with the MOZ_CONTENT_SANDBOX block inside the
> MOZ_SANDBOX block)? Everywhere else seems to be set up that way

Fixed.
Yeah, that makes sense as MOZ_CONTENT_SANDBOX should only be defined if MOZ_SANDBOX is.

> ::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
> @@ +96,5 @@
> > +    result = mPolicy->SetJobLevel(sandbox::JOB_NONE, 0);
> > +    bool ret = (sandbox::SBOX_ALL_OK == result);
> > +
> > +    result = mPolicy->SetTokenLevel(sandbox::USER_RESTRICTED_SAME_ACCESS,
> > +                                    sandbox::USER_NON_ADMIN);
> 
> Does this work better than USER_RESTRICTED_SAME_ACCESS? What's the reasoning
> for changing this?

Just because having the slightly more restrictive token doesn't seem to break anything.
The non admin token has all but a few of it's SIDs set to deny only and only has the bypass traverse checking privilege.

The only slight doubt I have is that USER_RESTRICTED_SAME_ACCESS has all existing SIDs in the token set up as restricting SIDs and the USER_NON_ADMIN has none.
As far as I can tell this amounts to the same thing, except that the USER_RESTRICTED_SAME_ACCESS access checks would be slower.
I think this is only done for the USER_RESTRICTED_SAME_ACCESS token as otherwise it wouldn't be treated as a restricted token at all and would be the same as USER_UNPROTECTED.

Does my reasoning make sense?

Comment 18

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

 > > ::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
> > @@ +96,5 @@
> > > +    result = mPolicy->SetJobLevel(sandbox::JOB_NONE, 0);
> > > +    bool ret = (sandbox::SBOX_ALL_OK == result);
> > > +
> > > +    result = mPolicy->SetTokenLevel(sandbox::USER_RESTRICTED_SAME_ACCESS,
> > > +                                    sandbox::USER_NON_ADMIN);
> > 
> > Does this work better than USER_RESTRICTED_SAME_ACCESS? What's the reasoning
> > for changing this?
> 
> Just because having the slightly more restrictive token doesn't seem to
> break anything.
> The non admin token has all but a few of it's SIDs set to deny only and only
> has the bypass traverse checking privilege.
> 
> The only slight doubt I have is that USER_RESTRICTED_SAME_ACCESS has all
> existing SIDs in the token set up as restricting SIDs and the USER_NON_ADMIN
> has none.
> As far as I can tell this amounts to the same thing, except that the
> USER_RESTRICTED_SAME_ACCESS access checks would be slower.
> I think this is only done for the USER_RESTRICTED_SAME_ACCESS token as
> otherwise it wouldn't be treated as a restricted token at all and would be
> the same as USER_UNPROTECTED.
> 
> Does my reasoning make sense?

I can't claim to be an expert in this area. My working understanding was that USER_RESTRICTED_SAME_ACCESS would allow access to anything that the user running Firefox could access, minus something (but I don't know what the something is) and that USER_NON_ADMIN would allow access to anything that the user running Firefox could access, minus the things only available to Administrators. It seems that I now need to expand my understanding :)

Some searching uncovered this from [1]:
  "[...] an access check compares the union of the user's identity and the list of groups the user is a member of against the ACL of interest. With a restricted token, the access check performs two passes. The first compares the union of the user's identity and the groups against the ACL; the second test compares only the SIDs in the token's "restricting SIDs" list against the ACL. The result of the access check is essentially the intersection of the results. If the first pass grants you "full control" and the second pass grants you "read only", then you get "read only". If either test fails to grant you any access, you get no access. [...] "restricting SIDs" and "deny-only SIDs" are completely different. "Restricting SIDs" are really misnamed, since the more SIDs you add to the list in a restricted token, the more you're granting access, not denying access."

The important quote for answering your question about the restricting SIDs is "the more SIDs you add to the list in a restricted token, the more you're granting access [...]."

That doesn't help me answer the second question (whether your reasoning makes sense) or whether there's a chance we're going to break users who otherwise would not be having issues. My advice would be to land this patch without the token level change and to lower the token level in a separate patch, but it's not critical that we do that.

[1] http://blogs.msdn.com/b/larryosterman/archive/2004/09/01/224051.aspx?PageIndex=2#comments

Comment 19

[Bob Owen (:bobowen)]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #18)

> That doesn't help me answer the second question (whether your reasoning
> makes sense) or whether there's a chance we're going to break users who
> otherwise would not be having issues. My advice would be to land this patch
> without the token level change and to lower the token level in a separate
> patch, but it's not critical that we do that.

I was actually asking about my reasoning about how the tokens work, but on reflection I think you're right about doing this in two patches.
Best to make sure that using the sandboxing code to start the process doesn't cause any issues on its own, before strengthening the policy.

That article confirms what I'd read in various other places that having all the SIDs in the restricting SIDs essentially means that USER_RESTRICTED_SAME_ACCESS gives the same access as USER_UNPROTECTED. I suppose the clue is in the name. :-)
It would be better if they had called the parameter RestrictToSids rather than SidsToRestrict. :)

I think I've possibly found the reason for the USER_RESTRICTED_SAME_ACCESS token in this comment in the Chromium code [1].
It seems that the impersonation token (initial) that is set on the new process's main thread (in order to allow extra permissions during start up), must be a restricted token if the process's primary token (lockdown) is restricted.
So, it does seem that adding all the SIDs is just a way of getting round this problem.

On the question of supplying no restricting SIDs, while it's not an documentation article, I found this patent useful [2].
If Fig. 7 is how the checks work in practice then having no restricting SIDs is the same as having all the original tokens SIDs as restricting SIDs.


[1] https://code.google.com/p/chromium/codesearch#chromium/src/content/common/sandbox_win.cc&l=368
[2] http://www.google.com/patents/US6279111

Comment 20

[Bob Owen (:bobowen)]

Attachment: Part 2: Enable the content sandbox by default on Windows with an open policy. v2

r=tabraldes from comment 16.

Fixed interceptions typo.
Fixed MOZ_SANDBOX / MOZ_CONTENT_SANDBOX nesting.
Changed to USER_RESTRICTED_SAME_ACCESS lockdown access token.

Comment 21

[Bob Owen (:bobowen)]

Comment on attachment 8518055 [details] [diff] [review]
Part 2: Enable the content sandbox by default on Windows with an open policy. v2

Re-requesting review on new patch, details in comment 12.

Comment 22

[Bob Owen (:bobowen)]

Comment on attachment 8518055 [details] [diff] [review]
Part 2: Enable the content sandbox by default on Windows with an open policy. v2

Flagging glandium for the build changes.

Comment 23

[Bob Owen (:bobowen)]

Comment on attachment 8518055 [details] [diff] [review]
Part 2: Enable the content sandbox by default on Windows with an open policy. v2

Thanks Jim.

Sorry about the brevity, I'm in a pub.

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 8518055 [details] [diff] [review]
Part 2: Enable the content sandbox by default on Windows with an open policy. v2

Review of attachment 8518055 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/app/profile/firefox.js
@@ +1187,5 @@
>  pref("browser.tabs.remote.autostart", false);
>  pref("browser.tabs.remote.desktopbehavior", true);
>  
> +#if defined(XP_WIN)
> +#if defined(MOZ_SANDBOX)

less nested #ifs is better for everyone :)
#if defined(XP_WIN) && defined(MOZ_SANDBOX)

::: configure.in
@@ +6607,5 @@
>  Darwin:1)
>      MOZ_CONTENT_SANDBOX=$MOZ_SANDBOX
>      ;;
>  *)
>      MOZ_ARG_ENABLE_BOOL(content-sandbox,

ouch. MOZ_ARG_ENABLE_BOOL and other --argument related things in configure.in are not entirely in the shell control flow, so what this effectively does is that there's a --enable-content-sandbox option, with its --disable-content-sandbox counterpart, on all platforms, except it does nothing on Windows and OSX. While, I guess, it should change the default. And with this change, --disable-content-sandbox should trigger an error.

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +96,5 @@
>      mProcessState(CREATING_CHANNEL),
>      mDelegate(nullptr),
> +#if defined(MOZ_SANDBOX) && defined(XP_WIN)
> +    mEnableSandboxLogging(false),
> +#if defined(MOZ_CONTENT_SANDBOX)

why keep this one when you're removing all the others under XP_WIN?

@@ +268,5 @@
>    if (mProcessType == GeckoProcessType_Plugin) {
>      InitWindowsGroupID();
>    }
>  
>  #if defined(MOZ_CONTENT_SANDBOX)

Why keep this one?

@@ +794,5 @@
>  #if defined(XP_WIN)
>    bool shouldSandboxCurrentProcess = false;
>    switch (mProcessType) {
>      case GeckoProcessType_Content:
>  #if defined(MOZ_CONTENT_SANDBOX)

and this one?

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
@@ +66,5 @@
>  
>    return true;
>  }
>  
>  #if defined(MOZ_CONTENT_SANDBOX)

same here

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.h
@@ +30,5 @@
>                   void **aProcessHandle);
>    virtual ~SandboxBroker();
>  
>    // Security levels for different types of processes
>  #if defined(MOZ_CONTENT_SANDBOX)

same here

Comment 25

[Bob Owen (:bobowen)]

Thanks for looking at this Mike.
Just a couple of questions below.

(In reply to Mike Hommey [:glandium] from comment #24)
> Comment on attachment 8518055 [details] [diff] [review]
> Part 2: Enable the content sandbox by default on Windows with an open
> policy. v2

> > +#if defined(XP_WIN)
> > +#if defined(MOZ_SANDBOX)
> 
> less nested #ifs is better for everyone :)
> #if defined(XP_WIN) && defined(MOZ_SANDBOX)

Ah, I'd moved these around a few times hadn't spotted I could drop that nesting, thanks.

> ::: configure.in
> @@ +6607,5 @@
> >  Darwin:1)
> >      MOZ_CONTENT_SANDBOX=$MOZ_SANDBOX
> >      ;;
> >  *)
> >      MOZ_ARG_ENABLE_BOOL(content-sandbox,
> 
> ouch. MOZ_ARG_ENABLE_BOOL and other --argument related things in
> configure.in are not entirely in the shell control flow, so what this
> effectively does is that there's a --enable-content-sandbox option, with its
> --disable-content-sandbox counterpart, on all platforms, except it does
> nothing on Windows and OSX. While, I guess, it should change the default.
> And with this change, --disable-content-sandbox should trigger an error.

Doesn't this mean that the option is effectively no longer there on Windows and OSX (Nightly)?
--disable-content-sandbox gives a warning the same as --disable-foo would.

Would you prefer this?

case "$OS_TARGET:$NIGHTLY_BUILD" in
WINNT:*|Darwin:1)
    MOZ_ARG_DISABLE_BOOL(content-sandbox,
    [  --disable-content-sandbox       Disable sandboxing support for content-processes],
        MOZ_CONTENT_SANDBOX=$MOZ_SANDBOX,
        MOZ_CONTENT_SANDBOX=)
    ;;
*)
    MOZ_ARG_ENABLE_BOOL(content-sandbox,
    [  --enable-content-sandbox        Enable sandboxing support for content-processes],
        MOZ_CONTENT_SANDBOX=1,
        MOZ_CONTENT_SANDBOX=)
    ;;
esac

> ::: ipc/glue/GeckoChildProcessHost.cpp
> @@ +96,5 @@
> >      mProcessState(CREATING_CHANNEL),
> >      mDelegate(nullptr),
> > +#if defined(MOZ_SANDBOX) && defined(XP_WIN)
> > +    mEnableSandboxLogging(false),
> > +#if defined(MOZ_CONTENT_SANDBOX)
> 
> why keep this one when you're removing all the others under XP_WIN?

Because that part is content process sandboxing specific.
Where I'm removing the others it should be logging related, which I am changing here from being content process specific to any sandboxed process.
Ideally I'd have done the turning on of the content sandbox by default and logging changes in separate patches, but as they were tied into the same pref before, it made it difficult to sensibly split the patches.

I suppose that if we don't change the above (over --disable-content-sandbox) then we are tying MOZ_CONTENT_SANDBOX to MOZ_SANDBOX on Windows, and we are unlikely to want to undo that.
So, getting rid of MOZ_CONTENT_SANDBOX under XP_WIN would make things easier to read.

I'm not sure how long we normally wait before deciding that a feature is sufficiently baked in and remove as much of the |#if|ing as possible.
We'd have to keep the MOZ_SANDBOX ones because of problems with other compilers (or arguably problems with MSVC :-) ).
Unless, the Chromium code changes and Bug 1042426 is no longer a problem.

Comment 26

[Mike Hommey [:glandium]]

(In reply to Bob Owen (:bobowen) from comment #25)
> Doesn't this mean that the option is effectively no longer there on Windows
> and OSX (Nightly)?
> --disable-content-sandbox gives a warning the same as --disable-foo would.

It really shouldn't.

> Would you prefer this?
> 
> case "$OS_TARGET:$NIGHTLY_BUILD" in
> WINNT:*|Darwin:1)
>     MOZ_ARG_DISABLE_BOOL(content-sandbox,
>     [  --disable-content-sandbox       Disable sandboxing support for
> content-processes],
>         MOZ_CONTENT_SANDBOX=$MOZ_SANDBOX,
>         MOZ_CONTENT_SANDBOX=)
>     ;;
> *)
>     MOZ_ARG_ENABLE_BOOL(content-sandbox,
>     [  --enable-content-sandbox        Enable sandboxing support for
> content-processes],
>         MOZ_CONTENT_SANDBOX=1,
>         MOZ_CONTENT_SANDBOX=)
>     ;;
> esac

Again, MOZ_ARG_ENABLE_BOOL/MOZ_ARG_DISABLE_BOOL are outside the shell control flow. You only need one of them, and it needs to be outside the case.

Comment 27

[Mike Hommey [:glandium]]

So, in practice, you need to put a MOZ_ARG_ENABLE_BOOL first, then the case, and make the case detect if the MOZ_ARG_ENABLE_BOOL disabled it, in which case, error out on windows where it's supposed to be enabled always.

Comment 28

[Bob Owen (:bobowen)]

Attachment: Part 1: Remove Chromium interception logging changes. v2

r=tabraldes - from history item 17

This is just a rebase because of the changes from bug 1041775.

Comment 29

[Bob Owen (:bobowen)]

Attachment: Part 2: Enable the content sandbox by default on Windows with an open policy. v3

(In reply to Mike Hommey [:glandium] from comment #24)
> Comment on attachment 8518055 [details] [diff] [review]

Thanks for your explanations on this Mike.

> >  *)
> >      MOZ_ARG_ENABLE_BOOL(content-sandbox,
> 
> ouch. MOZ_ARG_ENABLE_BOOL and other --argument related things in
> configure.in are not entirely in the shell control flow, so what this
> effectively does is that there's a --enable-content-sandbox option, with its
> --disable-content-sandbox counterpart, on all platforms, except it does
> nothing on Windows and OSX. While, I guess, it should change the default.

I've decided to go with what I think you were originally proposing here and have the configure option override the default on all platforms.
I think until we have this flag on by default on all platforms, this makes most sense.
As it is on or off by default depending on platform, I've added both options into the help text.  I did try and change this text depending on the default, but couldn't get it to work.

I also noticed that we had an existing situation where you could specify --disable-sandbox and --enable-content-sandbox, which doesn't make sense, so I added a test for that.

Personally, I think that once we have MOZ_CONTENT_SANDBOX on by default on Linux, we should look at getting rid of both MOZ_CONTENT_SANDBOX and MOZ_GMP_SANDBOX.
As we have MOZ_SANDBOX to remove sandboxing if there is an actual compilation issue, we should control whether the sandbox is turned on for a specific process by runtime methods.
This will simplify the #ifs in the code-base.
I'll speak to people at the sandboxing meeting about this.


Jim - it's the  ContentChild, plugin-container, GeckoChildProcessHost and nsEmbedFunctions side of things I'd like you to look at.  Although you might need to look at some of the other stuff to see how it hangs together, particularly loggingCallbacks.h


Also,
r=tabraldes from comment 16 for the security/sandbox/ parts.

Comment 30

[Bob Owen (:bobowen)]

Attachment: Part 3: Add logging changes back into the Chromium interception code. v2

r=tabraldes - from history item 21

This is just a rebase because of the changes from bug 1041775.

Comment 31

[Bob Owen (:bobowen)]

Try push with latest patches:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=debfea079562

Comment 32

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

(In reply to Bob Owen (:bobowen) from comment #29)
> Personally, I think that once we have MOZ_CONTENT_SANDBOX on by default on
> Linux, we should look at getting rid of both MOZ_CONTENT_SANDBOX and
> MOZ_GMP_SANDBOX.

There's also B2G, which has MOZ_CONTENT_SANDBOX but not MOZ_GMP_SANDBOX, and we won't have media plugins at all there until bug 1043403 and bug 1057908 are fixed.

Comment 33

[Mike Hommey [:glandium]]

Comment on attachment 8527506 [details] [diff] [review]
Part 2: Enable the content sandbox by default on Windows with an open policy. v3

Review of attachment 8527506 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +6605,5 @@
> +    MOZ_CONTENT_SANDBOX=)
> +
> +if test -n "$MOZ_CONTENT_SANDBOX" -a -z "$MOZ_SANDBOX"; then
> +    AC_MSG_ERROR([--enable-content-sandbox and --disable-sandbox are conflicting options])
> +fi

Do you want to allow --disable-content-sandbox on Windows?

Comment 34

[Bob Owen (:bobowen)]

(In reply to Mike Hommey [:glandium] from comment #33)
> Comment on attachment 8527506 [details] [diff] [review]

Thanks Mike.

> ::: configure.in
> @@ +6605,5 @@
> > +    MOZ_CONTENT_SANDBOX=)
> > +
> > +if test -n "$MOZ_CONTENT_SANDBOX" -a -z "$MOZ_SANDBOX"; then
> > +    AC_MSG_ERROR([--enable-content-sandbox and --disable-sandbox are conflicting options])
> > +fi
> 
> Do you want to allow --disable-content-sandbox on Windows?

Yeah, we may as well allow it given that the #if machinery for it exists in the code.

Comment 35

[Jim Mathies [:jimm]]

Comment on attachment 8527506 [details] [diff] [review]
Part 2: Enable the content sandbox by default on Windows with an open policy. v3

Review of attachment 8527506 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/sandbox/win/src/logging/loggingCallbacks.h
@@ +114,5 @@
> +    return;
> +  }
> +
> +  if (Preferences::GetBool("security.sandbox.windows.log")
> +      || PR_GetEnv("MOZ_WIN_SANDBOX_LOGGING")) {

nit - '||' on the end of the previous line

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
@@ +100,5 @@
> +                                    sandbox::USER_RESTRICTED_SAME_ACCESS);
> +    ret = ret && (sandbox::SBOX_ALL_OK == result);
> +
> +    result = mPolicy->SetDelayedIntegrityLevel(sandbox::INTEGRITY_LEVEL_MEDIUM);
> +    ret = ret && (sandbox::SBOX_ALL_OK == result);

This code seems a little odd, some questions:

1) do we care if one of these calls fails but the rest complete?
2) are failures here unexpected?
3) when we return false here what's the side effect?

I'm wondering if failures here should have harder consequences.

Comment 36

[Bob Owen (:bobowen)]

(In reply to Jim Mathies [:jimm] from comment #35)
> Comment on attachment 8527506 [details] [diff] [review]

Thanks Jim.

> > +  if (Preferences::GetBool("security.sandbox.windows.log")
> > +      || PR_GetEnv("MOZ_WIN_SANDBOX_LOGGING")) {
> 
> nit - '||' on the end of the previous line

Fixed locally.
 
> ::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
> @@ +100,5 @@
> > +                                    sandbox::USER_RESTRICTED_SAME_ACCESS);
> > +    ret = ret && (sandbox::SBOX_ALL_OK == result);
> > +
> > +    result = mPolicy->SetDelayedIntegrityLevel(sandbox::INTEGRITY_LEVEL_MEDIUM);
> > +    ret = ret && (sandbox::SBOX_ALL_OK == result);
> 
> This code seems a little odd, some questions:
> 
> 1) do we care if one of these calls fails but the rest complete?
> 2) are failures here unexpected?
> 3) when we return false here what's the side effect?
> 
> I'm wondering if failures here should have harder consequences.

Yeah, Tim and I discussed this when he first put them in.
1) yes, but we decided it was better to set up as much of the policy as possible, in case the caller ignores the return value (as happens now).
2) none of these calls should really fail.  You can get failures, if you call some of the functions with incorrect parameters, but they would always fail, not intermittently.  So it's likely things will get caught during development.
3) at the moment nothing and we probably should be hard failing, although I guess we need to consult a few different people on that.  I'm pretty sure there is an existing bug around this, but I don't think it's this specific issue, so I'll check and make sure a new one is filed.  Because of 2) we left it when I brought this up for GMP, but we should definitely get this sorted.

Comment 37

[Bob Owen (:bobowen)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c2c603abc7bb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/69c15302979a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0f763c186855

Comment 38

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c2c603abc7bb
https://hg.mozilla.org/mozilla-central/rev/69c15302979a
https://hg.mozilla.org/mozilla-central/rev/0f763c186855