Create warn only sandbox prototype.

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bobowen, Assigned: bobowen)

Tracking

unspecified
mozilla35
All
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 18 obsolete attachments)

30.21 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
28.42 KB, patch
bobowen
: review+
benjamin
: review+
Details | Diff | Splinter Review
This bug is just to keep track of progress of a prototype version of a warn only windows content sandbox.

This will just add code into the various sandbox interception classes to write messages to stderr, when access to a resource is denied and also if it is subsequently created / opened by the sandbox broker.

This will demonstrate how feasible this approach for a warn only sandbox is and we can then look at more sophisticated logging and prefs.
Try push for basic mochitests on Windows 7 with e10s enabled:
https://tbpl.mozilla.org/?tree=Try&rev=49d41ed5ee03

This gives us a baseline of what passes without the sandbox enabled.

Same try push, but with the sandbox enabled:
https://tbpl.mozilla.org/?tree=Try&rev=8fe18442846d
This try push was a first stab at adding rules that allow all access for the sandboxed process through the sandboxed broker and adding basic logging when so doing:
https://tbpl.mozilla.org/?tree=Try&rev=b018e0312c1b

This is a fairly encouraging start as most of the tests seem to work as well as the ones with just e10s apart from mochitest-3.
These appear to have an issue with the basic audio tests.

It is also encouraging that a lot of the things that are being blocked are the creation of temporary files, which should be fixed when we address bug 1018988.
This just pulls the Chromium StackTrace code into the sandbox version.
I have fairly clearly commented out some of the code as it was causing an avalanche of other things that I would need to pull in.
This adds another constructor to the StackTrace class, so that you can specify a number of frames to skip and the depth of frames required.
Finally got this building without the circular dependency in the linking, thanks to the help from rbarker.

Tim - would you mind having a look at this for me, particularly the fairly nasty hack to which I resorted, to get the function pointers into the interception code.
Feel free to comment on the other two patches. :-)

This adds and uses some simple namespaced helper functions to put the Windows process sandbox into warn only mode.

It sets up all the allowable rules, so that most blocked resources are made available through the sandbox broker.
It also adds logging to all of the interception classes, which will log to the Browser Console and to stderr (if compiled with DEBUG).
It logs on the initial block and again if it is then allowed by the broker.
The BLOCKED logging message can have a stack trace logged as well to help with any debugging.

The warn only sandbox would only be available on nightly and is controlled through two prefs:

* browser.tabs.remote.sandbox.warnOnly (true/false(default)) - enable/disable the warn only sandbox (requires a restart)

* browser.tabs.remote.sandbox.warnOnlyStackTraceDepth (int (0 default)) - the depth of stack trace to print with the BLOCKED messages.

You still need to build with --enable-content-sandbox at the moment, but once Bug 985252 has landed, and all the sandbox code is compiled in anyway, I intend to put the sandbox behind a nightly pref as well.

Here's a try push with this enabled (along with e10s) for windows:
https://tbpl.mozilla.org/?tree=Try&rev=e9a24f32e680

This is pretty similar to an e10s only try push:
https://tbpl.mozilla.org/?tree=Try&rev=a1450693444d

The main difference is for the mochitest-3 tests, where some audio tests are failing.

I need to do some testing to make sure that all the different interception logging is working.
The process and file ones certainly are.
Attachment #8446075 - Flags: feedback?(tabraldes)
Comment on attachment 8446075 [details] [diff] [review]
Part 3: Add warn only sandbox functionality for Windows processes.

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

Seems pretty good to me except for the hack that you mention. Please fix that ;)

::: security/sandbox/win/src/warnonlysandbox/warnOnlySandbox.cpp
@@ +39,5 @@
> +    sGetIntPrefCallback = aGetIntPrefCallback;
> +
> +    // Unfortunately when the interception code is triggered it appears to get
> +    // its own version of the static variables above that we are using to store
> +    // the state required to operate the warn only sandbox.

This is happening because of the `static` keyword used when declaring those variables. I believe the correct way to go about this is:
  1) Declare them in a header without the `static` keyword (properly namespaced under `mozilla::warnonlysandbox`)
  2) Include the header in the consumers and in this cpp file
  3) Remove the `static` keyword from the definitions above

Each compilation unit that includes the header will have external dependencies on those symbols. The linker will find the correct definitions. At least that's how I think it should work :)
Attachment #8446075 - Flags: feedback?(tabraldes) → feedback+
(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #6)

> > +    // Unfortunately when the interception code is triggered it appears to get
> > +    // its own version of the static variables above that we are using to store
> > +    // the state required to operate the warn only sandbox.
> 
> This is happening because of the `static` keyword used when declaring those
> variables. I believe the correct way to go about this is:
>   1) Declare them in a header without the `static` keyword (properly
> namespaced under `mozilla::warnonlysandbox`)
>   2) Include the header in the consumers and in this cpp file
>   3) Remove the `static` keyword from the definitions above
> 
> Each compilation unit that includes the header will have external
> dependencies on those symbols. The linker will find the correct definitions.
> At least that's how I think it should work :)

Thanks, Tim.
I've tried various combinations, although there is a chance that is not one of them.
I even tried using shared data segments, but I'll give that a go tomorrow and see how I get on.
In this attempt I just moved variable declarations into the warnOnlySandbox.h file and commented out the hack.

Unless I've done something really stupid (quite possible) this still didn't work.
The variables hold between the enable in GeckoChildProcessHost.cpp and the call in sandboxBroker.cpp (this is the same as before) which is in the sandboxbroker.dll. (This is in the main process.)
However, they don't hold between the call in nsEmbedFunctions.cpp and the subsequent calls in the interception files. (This is in the content process.)

I've also tried moving the declarations to a totally separate file, which I'll upload next, but still no joy.

I can only assume that it is something to do with the way the interceptions are wired in.
Flags: needinfo?(tabraldes)
Posted patch wosSeparateFileVars (obsolete) — Splinter Review
This has the variables and types in separate files.
This patch includes the variables file in all the others, but I also tried it with it just in the warnOnlySandbox.cpp.
Attachment #8446447 - Attachment is patch: true
OK, I think I've worked out what's going on here and it's not to do with the way the interceptions are wired in.

I'm fairly new to this, so I might not get all the terms right, but hopefully it will make sense.

Currently, the sandboxbroker.dll links against the sandbox_s library and xul.dll links against sandboxbroker.dll.

However, plugin-container.exe links against xul.dll and the sandbox_s library as well.

So, in the main process when GeckoChildProcessHost::PrepareLaunch() in xul.dll calls EnableWarnOnlySandbox() this is in sandboxbroker.dll, so when subsequently SandboxBroker::LaunchApp calls mozilla::warnonlysandbox::SetUpIfEnabled(*mPolicy) we are also in sandboxbroker.dll, so it works.

In the content process when XRE_InitChildProcess in xul.dll calls EnableWarnOnlySandbox(), again this is in sandboxbroker.dll, but when we hit the interception code we are in plugin-container.exe and so we have a different version of the variables.

Now that I've worked this out hopefully I can find a way round this.

It does raise the question as to whether this linking is correct in the first place.
I've got this working now along a similar line to the StartSandboxCallback in sandboxTarget.h.

This explains why that's been done with a similar strange callback.
Here's my updated patch with the nasty hack removed and a slightly more pleasant hack added to work around the fact that sandbox_s is linked into plugin-container.exe twice.

There are a few other changes, like stopping it from breaking the build when you don't have --enable-content-sandbox :-)

Here's a try push with it enabled and a stack trace depth of 20:
https://tbpl.mozilla.org/?tree=Try&rev=c69d64543154

If you search for "Process Sandbox" in the debug logs you can see the messages.

And here's a try push without --enable-content-sandbox:
https://tbpl.mozilla.org/?tree=Try&rev=7030f938a3ac

Tim, would you mind taking another look at this.
What do you think over the fact that sandbox_s gets linked twice?
Attachment #8446075 - Attachment is obsolete: true
Attachment #8446446 - Attachment is obsolete: true
Attachment #8446447 - Attachment is obsolete: true
Attachment #8448687 - Flags: feedback?(tabraldes)
Flags: needinfo?(tabraldes)
Comment on attachment 8448687 [details] [diff] [review]
Part 3: Add warn only sandbox functionality for Windows processes. v2

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

(In reply to Bob Owen (:bobowen) from comment #12)
> Created attachment 8448687 [details] [diff] [review]
> Part 3: Add warn only sandbox functionality for Windows processes. v2
> 
> Here's my updated patch with the nasty hack removed and a slightly more
> pleasant hack added to work around the fact that sandbox_s is linked into
> plugin-container.exe twice.
> 
> There are a few other changes, like stopping it from breaking the build when
> you don't have --enable-content-sandbox :-)
> 
> Here's a try push with it enabled and a stack trace depth of 20:
> https://tbpl.mozilla.org/?tree=Try&rev=c69d64543154
> 
> If you search for "Process Sandbox" in the debug logs you can see the
> messages.
> 
> And here's a try push without --enable-content-sandbox:
> https://tbpl.mozilla.org/?tree=Try&rev=7030f938a3ac
> 
> Tim, would you mind taking another look at this.
> What do you think over the fact that sandbox_s gets linked twice?

Seems good to me! Especially since we already seem to be doing the same thing with `SetStartSandboxCallback`.

Great work on this, Bob!
Attachment #8448687 - Flags: feedback?(tabraldes) → feedback+
Attachment #8446056 - Flags: review?(aklotz)
Attachment #8446059 - Flags: review?(aklotz)
Attachment #8448687 - Flags: review?(aklotz)
Comment on attachment 8446056 [details] [diff] [review]
Part 1: Import Chromium StackTrace code. v1

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

Other than moz.build, this is just a direct import, right? If so r=me on moz.build and rs=me on the chromium bits.
Attachment #8446056 - Flags: review?(aklotz) → review+
Comment on attachment 8446059 [details] [diff] [review]
Part 2: Add the ability to specify frames to skip and frame depth v1

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

AFAIK we've been trying very, very hard to leave the chromium sandboxing bits unchanged from upstream so that we can easily import newer revisions as necessary. It seems to me that these changes would impair that. Can this be done in a less invasive way?
Attachment #8446059 - Flags: review?(aklotz) → review-
Comment on attachment 8448687 [details] [diff] [review]
Part 3: Add warn only sandbox functionality for Windows processes. v2

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

Looks good overall, but:

1) See my comments about logging UNICODE_STRING;
2) As with the previous patch I also have concerns in this patch about modifying the imported chromium code. What is the plan to ensure that our ability to import will not be impacted?

::: security/sandbox/win/src/registry_interception.cc
@@ +29,5 @@
>      return status;
>  
> +#ifdef NIGHTLY_BUILD
> +  mozilla::warnonlysandbox::LogBlocked("NtCreateKey",
> +                                       object_attributes->ObjectName->Buffer);

You need to be careful with object_attributes->ObjectName->Buffer. UNICODE_STRING is not guaranteed to be null-terminated. Same thing for other references in this patch.

::: security/sandbox/win/src/warnonlysandbox/warnOnlySandbox.cpp
@@ +12,5 @@
> +#include "base/debug/stack_trace.h"
> +#include "base/strings/utf_string_conversions.h"
> +#include "sandbox/win/src/sandbox_policy.h"
> +
> +using namespace std;

Can you move this inside the warnonlysandbox namespace to avoid spillage, or else remove the using statement and reference the std types by their fully-qualified name?

::: security/sandbox/win/src/warnonlysandbox/warnOnlySandbox.h
@@ +39,5 @@
> +// Log a "BLOCKED" msg to the browser console and, if DEBUG build, stderr.
> +void LogBlocked(const char* aFunctionName, const char* aContext = nullptr);
> +
> +// Log a "BLOCKED" msg to the browser console and, if DEBUG build, stderr.
> +void LogBlocked(const char* aFunctionName, const wchar_t* aContext);

Should we also have a default nullptr param on aContext?

@@ +45,5 @@
> +// Log a "ALLOWED" msg to the browser console and, if DEBUG build, stderr.
> +void LogAllowed(const char* aFunctionName, const char* aContext = nullptr);
> +
> +// Log a "ALLOWED" msg to the browser console and, if DEBUG build, stderr.
> +void LogAllowed(const char* aFunctionName, const wchar_t* aContext);

Ditto?
Attachment #8448687 - Flags: review?(aklotz) → review-
(In reply to Aaron Klotz [:aklotz] from comment #14)
> Comment on attachment 8446056 [details] [diff] [review]
> Part 1: Import Chromium StackTrace code. v1

Thanks for all the reviews.

> Review of attachment 8446056 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Other than moz.build, this is just a direct import, right? If so r=me on
> moz.build and rs=me on the chromium bits.

Unfortunately, not.
The files I was having to import was snowballing, so I resorted to commenting out small parts of the code to get around this.

I'm not sure how the upstream import would work, I've no experience of such things.
I thought that the way I had commented things would help ease the pain.

I'll get onto the other things tomorrow.
Flags: needinfo?(aklotz)
Comment on attachment 8446056 [details] [diff] [review]
Part 1: Import Chromium StackTrace code. v1

Okay. In light of that, I'm afraid I'm going to have to r- this until a decision is made about what do to with respect to imported chromium code.

The consensus back when I was more heavily involved with sandboxing was that we would try very hard to avoid directly modifying imported chromium source. Instead we would do things like subclassing, preprocessor tricks etc.

The last thing we want is yet another chromium import that has diverged so far from upstream that it's effectively now our code (see ipc/chromium for an example).

FWIW I'm probably not the person to be making the call on how best to handle this, as I haven't been directly involved with sandboxing other than reviews for several months now. Perhaps the attendees at the sandboxing standing meeting would want to discuss this?
Attachment #8446056 - Flags: review+ → review-
Flags: needinfo?(aklotz)
(In reply to Aaron Klotz [:aklotz] from comment #18)
> Comment on attachment 8446056 [details] [diff] [review]
> Part 1: Import Chromium StackTrace code. v1
> 
> Okay. In light of that, I'm afraid I'm going to have to r- this until a
> decision is made about what do to with respect to imported chromium code.
> 
> The consensus back when I was more heavily involved with sandboxing was that
> we would try very hard to avoid directly modifying imported chromium source.
> Instead we would do things like subclassing, preprocessor tricks etc.
> 
> The last thing we want is yet another chromium import that has diverged so
> far from upstream that it's effectively now our code (see ipc/chromium for
> an example).
> 
> FWIW I'm probably not the person to be making the call on how best to handle
> this, as I haven't been directly involved with sandboxing other than reviews
> for several months now. Perhaps the attendees at the sandboxing standing
> meeting would want to discuss this?

OK, I thought that might be your view, so I thought I'd get back over that straight away.

I completely take your point over the other chromium code, it would be really good to be able to merge these together, but as it currently stands that would be a massive undertaking.
I did look at using the StackTrace code from there, but couldn't get it to work.

Either way it won't solve all my issues.

I can see how subclassing might be made to work in some cases.
What sort of preprocessor tricks might I employ?
(I'm relatively inexperienced in some areas of C++)

Thanks for your help.
Flags: needinfo?(aklotz)
With a bit of dancing with shims I've got the Chromium StackTrace import working without changes.
I think I've addressed the other issues, apart from changes to the chromium interception code.

I can't think of a way around those, but I've broken them out into a separate patch, so they would be easy to back out.

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=669b8a56b5a0
(In reply to Bob Owen (:bobowen) from comment #19)
> I can see how subclassing might be made to work in some cases.
> What sort of preprocessor tricks might I employ?
> (I'm relatively inexperienced in some areas of C++)
> 
> Thanks for your help.

I did some preprocessor magic to replace fopen in our spellchecker (which comes from upstream): https://bug857830.bugzilla.mozilla.org/attachment.cgi?id=746247

Also bbondy did some shim stuff when he first landed the chromium sandbox:
https://bug922756.bugzilla.mozilla.org/attachment.cgi?id=822751
Flags: needinfo?(aklotz)
Here's the patch, I mentioned in comment 20, importing the Chromium code using new shim files.

I notice that we already have a version of path_service.cc in the tree, but it's not being compiled.
Should I remove this?
Attachment #8446056 - Attachment is obsolete: true
Attachment #8451451 - Flags: review?(aklotz)
(In reply to Bob Owen (:bobowen) from comment #22)
> I notice that we already have a version of path_service.cc in the tree, but
> it's not being compiled.
> Should I remove this?

Sure, might as well minimize the profile of the imported code.
Comment on attachment 8451451 [details] [diff] [review]
Part 1: Import Chromium StackTrace code. v2

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

r+ on the shims, rs=me on the imported bits.

Thanks for doing this, I know it's kind of annoying but it will be beneficial in the long run!

::: security/sandbox/chromium/base/shim/base/process/launch.h
@@ +14,5 @@
> +#include "base/files/file_path.h"
> +
> +namespace base {
> +
> +static inline void RouteStdioToConsole() {};

Nit: Semicolon should not be necessary here since you've defined the function with {}
Attachment #8451451 - Flags: review?(aklotz) → review+
(In reply to Aaron Klotz [:aklotz] (PTO, back on July 21) from comment #24)
> Comment on attachment 8451451 [details] [diff] [review]
> Part 1: Import Chromium StackTrace code. v2
> 
> Review of attachment 8451451 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ on the shims, rs=me on the imported bits.
> 
> Thanks for doing this, I know it's kind of annoying but it will be
> beneficial in the long run!

Thanks Aaron
I'm fine with doing this, I've worked around skipping and limiting frames in the stack trace with the existing Chromium code as well.

I might struggle with adding the logging into the interception code, but I'll have a read of your patch and see if I can come up with anything.

Sorry, didn't see the PTO notification in your profile name ... have a great break.
From comment 24:
r=aklotz - for shim code and moz.build
rs=aklotz - for imported Chromium StackTrace code

Removed semicolon as per nit in comment - thanks.

Over path_service.cc, I've done a quick audit and there appears to be quite a few files that are not compiled.
So, I'll pick these up in a separate bug.
Attachment #8451451 - Attachment is obsolete: true
Attachment #8451491 - Flags: review+
This replaces old parts 2 and 3.

(In reply to Aaron Klotz [:aklotz] (PTO, back on July 21) from comment #15)
> Comment on attachment 8446059 [details] [diff] [review]
> Part 2: Add the ability to specify frames to skip and frame depth v1
> 
> Review of attachment 8446059 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> AFAIK we've been trying very, very hard to leave the chromium sandboxing
> bits unchanged from upstream so that we can easily import newer revisions as
> necessary. It seems to me that these changes would impair that. Can this be
> done in a less invasive way?

I've found that I can use the existing StackTrace class to achieve what I want.
It's a little more verbose and inefficient, but neither of those matter in this circumstance.


(In reply to Aaron Klotz [:aklotz] (PTO, back on July 21) from comment #16)
> Comment on attachment 8448687 [details] [diff] [review]
> Part 3: Add warn only sandbox functionality for Windows processes. v2
> 
> Review of attachment 8448687 [details] [diff] [review]:
> -----------------------------------------------------------------

> 2) As with the previous patch I also have concerns in this patch about
> modifying the imported chromium code. What is the plan to ensure that our
> ability to import will not be impacted?

I've looked into using the preprocessor for this.
The only thing I can think of is wrapping each of the interception functions with a function that repeats the start of each function (checking the orig_* function call) and logs if blocked.
Then calls the original Target* function (which will call the blocked function again and then the broker) and logs if it returns a success.

This seems like a pretty clumsy work around for something that we will want to remove at some point anyway.

So, I've split the changes that affect the Chromium code into their own patch and I'll upload a backout patch as well.
This could be used if we ever come to merge from upstream.
I've added a comment to the first change in each file to this effect

Hopefully this is OK, otherwise, I'll give the wrapping solution a go, unless someone can spot a better solution.
 
> ::: security/sandbox/win/src/registry_interception.cc
> @@ +29,5 @@
> >      return status;
> >  
> > +#ifdef NIGHTLY_BUILD
> > +  mozilla::warnonlysandbox::LogBlocked("NtCreateKey",
> > +                                       object_attributes->ObjectName->Buffer);
> 
> You need to be careful with object_attributes->ObjectName->Buffer.
> UNICODE_STRING is not guaranteed to be null-terminated. Same thing for other
> references in this patch.

Thanks ... added Log* functions that also take a length, to be used when we have UNICODE_STRING context.

> ::: security/sandbox/win/src/warnonlysandbox/warnOnlySandbox.cpp
> @@ +12,5 @@
> > +#include "base/debug/stack_trace.h"
> > +#include "base/strings/utf_string_conversions.h"
> > +#include "sandbox/win/src/sandbox_policy.h"
> > +
> > +using namespace std;
> 
> Can you move this inside the warnonlysandbox namespace to avoid spillage, or
> else remove the using statement and reference the std types by their
> fully-qualified name?

Thanks, removed and added to each use.

> ::: security/sandbox/win/src/warnonlysandbox/warnOnlySandbox.h
> @@ +39,5 @@
> > +// Log a "BLOCKED" msg to the browser console and, if DEBUG build, stderr.
> > +void LogBlocked(const char* aFunctionName, const char* aContext = nullptr);
> > +
> > +// Log a "BLOCKED" msg to the browser console and, if DEBUG build, stderr.
> > +void LogBlocked(const char* aFunctionName, const wchar_t* aContext);
> 
> Should we also have a default nullptr param on aContext?

I don't think so, as this would mean that someone calling LogBlocked("wibble"), would get a compile error complaining about ambiguous overloads.
Attachment #8446059 - Attachment is obsolete: true
Attachment #8448687 - Attachment is obsolete: true
This adds the logging lines into the Chromium interception code, as mentioned in comment 27.
This can be used to backout Part 3, if we need to merge from upstream.
Attachment #8453130 - Flags: review?(tabraldes)
Attachment #8453131 - Flags: review?(tabraldes)
Comment on attachment 8453130 [details] [diff] [review]
Part 2: Add warn only sandbox functionality for Windows processes. v3

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

Sorry this review took so long! I was finishing up bug 985252, but also I was trying to wrap my head around the library/callback stuff going on in this patch. My unedited notes follow at [1].

Generally this patch looks good. My only concern is whether things will still work correctly after bug 985252 lands since that patch causes us to build sandbox code regardless of whether --enable-content-sandbox/MOZ_CONTENT_SANDBOX is specified. I plan to land bug 985252 momentarily so this may be as simple as rebasing and verifying that everything builds/runs as expected.


[1]
SANDBOX_EXPORTS - defined when building sandboxbroker.dll
TARGET_SANDBOX_EXPORTS
 - defined in ContentChild.cpp if MOZ_CONTENT_SANDBOX before including sandboxtarget.h
 - defined in toolkit/xre/nsEmbedFunctions.cpp before including warnonlysandbox/wosEnableCallback.h

if SANDBOX_EXPORTS, warnonlysandbox/warnOnlySandbox.h exports `Enable`
else, warnonlysandbox/warnOnlySandbox.h imports `Enable`

if TARGET_SANDBOX_EXPORTS, sandboxtarget.h exports `SandboxTarget`
else, sandboxtarget.h imports `SandboxTarget`

ipc/app/MozillaRuntimeMain.cpp includes sandboxtarget.h without defining TARGET_SANDBOX_EXPORTS

if TARGET_SANDBOX_EXPORTS, warnonlysandbox/wosEnableCallback.h exports `SetEnableCallback`
else, warnonlysandbox/wosEnableCallback.h imports `SetEnableCallback`

toolkit/xre/nsEmbedFunctions.cpp defines TARGET_SANDBOX_EXPORTS before including warnonlysandbox/wosEnableCallback.h
In `XRE_InitChildProcess`:
  We call `sEnableCallback`, passing in our logging function and our `GetIntPrefOnMainThread` function

ipc/app/MozillaRuntimeMain.cpp includes warnonlysandbox/wosEnableCallback.h without defining TARGET_SANDBOX_EXPORTS
In `main`:
  We have imported `Enable` from sandboxbroker.dll
  We have imported `SetEnableCallback` from xul.dll
  We call `SetEnableCallback(Enable)` so that our xul.dll functions will use the `Enable` from sandboxbroker.dll

::: security/sandbox/win/src/warnonlysandbox/warnOnlySandbox.cpp
@@ +27,5 @@
> +{
> +  sWarnOnlySandboxEnabled = true;
> +  if (aLogToConsoleCallback) {
> +    sLogToConsoleCallback = aLogToConsoleCallback;
> +    sGetIntPrefCallback = aGetIntPrefCallback;

It seems like this will produce unexpected results if we call `Enable(nullptr, someFunctionPointer)` or `Enable(someFunctionPointer, nullptr)`. Maybe we should do separate checks for `aLogToConsoleCallback` and `aGetIntPrefCallback` or remove the check altogether?

::: toolkit/xre/nsEmbedFunctions.cpp
@@ +73,5 @@
>  #include "GMPProcessChild.h"
>  
>  #include "GeckoProfiler.h"
>  
> +#if defined(NIGHTLY_BUILD) && defined(XP_WIN) && defined(MOZ_CONTENT_SANDBOX)

It seems like requiring `defined(MOZ_CONTENT_SANDBOX)` here will mean that things will break if we don't specify --enable-content-sandbox. The patch in bug 985252 is decoupling --enable-content-sandbox from general sandboxing, so we may want to remove this check

@@ +275,5 @@
>          ::FreeLibrary(hDLL);
>  }
>  #endif
>  
> +#if defined(NIGHTLY_BUILD) && defined(XP_WIN) && defined(MOZ_CONTENT_SANDBOX)

Same comment here RE MOZ_CONTENT_SANDBOX

@@ +279,5 @@
> +#if defined(NIGHTLY_BUILD) && defined(XP_WIN) && defined(MOZ_CONTENT_SANDBOX)
> +// Simple wrapper to return an int pref when on the main thread and -1 when
> +// not.  This is to be used as a callback by the Window's warn only sandbox.
> +static int32_t
> +GetIntPrefOnMainThread(const char* aPref)

Do we use the "return [...] -1 when not [on the main thread]" functionality of this function? Should it instead be an error to try to call this function off the main thread?

@@ +547,5 @@
>          NS_LogTerm();
>          return NS_ERROR_FAILURE;
>        }
>  
> +#if defined(NIGHTLY_BUILD) && defined(XP_WIN) && defined(MOZ_CONTENT_SANDBOX)

Same comment here RE MOZ_CONTENT_SANDBOX
Attachment #8453130 - Flags: review?(tabraldes) → review+
Comment on attachment 8453131 [details] [diff] [review]
Part 3: Make warn only sandbox changes to the Chromium code

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

r+ but let's get input from someone who was involved in the initial sandboxing effort to see how comfortable we feel about having added some extra effort requirements for pulling in new upstream changes.
Attachment #8453131 - Flags: review?(tabraldes) → review+
(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #30)
> Comment on attachment 8453130 [details] [diff] [review]
> Part 2: Add warn only sandbox functionality for Windows processes. v3
> 
> Review of attachment 8453130 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry this review took so long! I was finishing up bug 985252, but also I
> was trying to wrap my head around the library/callback stuff going on in
> this patch. My unedited notes follow at [1].

No problem, that was definitely more urgent.
Yeah, the callback stuff is down to the double linking (Bug 1035125).
It led me a merry dance until I worked this out. :)
I think the last part of the notes aren't quite right, I'll comment below.

> Generally this patch looks good. My only concern is whether things will
> still work correctly after bug 985252 lands since that patch causes us to
> build sandbox code regardless of whether
> --enable-content-sandbox/MOZ_CONTENT_SANDBOX is specified. 

I knew I might have to fix some stuff if that landed first, I might just remove all the MOZ_CONTENT_SANDBOX bits. (I'll wait until your patch hits m-c)

That will just leave the one around starting the content sandbox, I think.
I plan to replace this with a nightly only pref very soon, so that it is easier for people to try out the content sandbox.
I think this makes sense now that we're compiling the sandbox anyway.
Once we start shipping it for real, then we'll remove the pref as we wouldn't want people to be able to turn it off with just a pref.

> [1]

> ipc/app/MozillaRuntimeMain.cpp includes warnonlysandbox/wosEnableCallback.h
> without defining TARGET_SANDBOX_EXPORTS
> In `main`:
>   We have imported `Enable` from sandboxbroker.dll
>   We have imported `SetEnableCallback` from xul.dll
>   We call `SetEnableCallback(Enable)` so that our xul.dll functions will use
> the `Enable` from sandboxbroker.dll

All correct up to this point, I think this bit works like this ...

In `main`:
  `Enable` comes from the sandbox_s library that plugin-container.exe directly links against
  We have imported `SetEnableCallback` from xul.dll
  We call `SetEnableCallback(Enable)` so that our xul.dll functions can use the `Enable` from plugin-container.exe (if we try to call Enable directly from xull.dll, we call the one linked in from sandboxbroker.dll, because this is also links against the sandbox_s library)


> > +  if (aLogToConsoleCallback) {
> > +    sLogToConsoleCallback = aLogToConsoleCallback;
> > +    sGetIntPrefCallback = aGetIntPrefCallback;
> 
> It seems like this will produce unexpected results if we call
> `Enable(nullptr, someFunctionPointer)` or `Enable(someFunctionPointer,
> nullptr)`. Maybe we should do separate checks for `aLogToConsoleCallback`
> and `aGetIntPrefCallback` or remove the check altogether?

Hmm, I think I only added the check for when I was using the really nasty hack.
I think it can go, thanks.
 
> 
> @@ +279,5 @@
> > +#if defined(NIGHTLY_BUILD) && defined(XP_WIN) && defined(MOZ_CONTENT_SANDBOX)
> > +// Simple wrapper to return an int pref when on the main thread and -1 when
> > +// not.  This is to be used as a callback by the Window's warn only sandbox.
> > +static int32_t
> > +GetIntPrefOnMainThread(const char* aPref)
> 
> Do we use the "return [...] -1 when not [on the main thread]" functionality
> of this function? Should it instead be an error to try to call this function
> off the main thread?

Yes, if the interception code is triggered off main thread, then when the warn only sandbox calls this and gets back -1, it just uses its cached value from the last time it was triggered on main thread.

I'll update the comment here, so people don't have to go to the warn only sandbox code to discover this.
Comment on attachment 8453131 [details] [diff] [review]
Part 3: Make warn only sandbox changes to the Chromium code

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #31)
> Comment on attachment 8453131 [details] [diff] [review]
> Part 3: Make warn only sandbox changes to the Chromium code
> 
> Review of attachment 8453131 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ but let's get input from someone who was involved in the initial
> sandboxing effort to see how comfortable we feel about having added some
> extra effort requirements for pulling in new upstream changes.

Thanks for the reviews Tim.

Brian ... as we discussed on IRC, would you mind taking a look at this.
Aaron originally raised this back in comments 15, 16 and 18.

I managed to work around the changes from comments 15 and 18, which was great.
However, I couldn't think of a relatively straight forward way of doing it for these changes.

I think I could possibly create macro wrappers for each of the functions and repeat the first part of each function, but I think this would be very brittle and would probably lead to more problems than just adding the lines.

We are almost certainly going to want to remove this warn only code anyway, once we start shipping with the content sandbox on as default.

Oh, I have just noticed that Aaron is back on Monday (I thought he was away for another week), so we could just wait until he's back.
Attachment #8453131 - Flags: review?(netzen)
Comment on attachment 8453131 [details] [diff] [review]
Part 3: Make warn only sandbox changes to the Chromium code

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

Thanks for the patch!

A lot of effort was done initially to make sure 0 changes were done to the core Chromium code, so we could just copy on top when new updates were made.

This is likely to get lost once we update from Chromium trunk again next time. 

We should maintain a list of these types of changes or something so that we can re-apply them after doing a new copy over from Chromium. I think in GFX we've actually kept the patches to apply in tree, but I think instead just a text file with a link to the changeset to apply is good enough.
Attachment #8453131 - Flags: review?(netzen) → review+
Got back to looking at why this is failing for Windows XP since bug 985282 landed.
Finally ended up spinning up a Windows XP VM to see why it wasn't starting with the Chromium StackTrace code and it seems that it can't find SymGetSearchPathW in dbghelp.dll.

The only things I can find in the Chromium code about this are that it delays the load of that dll, so I'm trying that.
That appears to have done the trick:
https://tbpl.mozilla.org/?tree=Try&rev=ee67699cda67

Here's a fuller try push again without --enable-content-sandbox or --e10s:
https://tbpl.mozilla.org/?tree=Try&rev=e24c93c09db0

I'm going to do some manual testing on Windows XP as well.

I also think I should probably get some further reviews for the build changes and non security/sandbox things in Part 2.
From comment 24:
r=aklotz - for shim code and moz.build
rs=aklotz - for imported Chromium StackTrace code

Now that the sandboxing code is being compiled by default, this was crashing on Windows XP, so I've delayed the load of dbghelp.dll, which seems to solve the problem.
Attachment #8451491 - Attachment is obsolete: true
Attachment #8468470 - Flags: review+
r=tabraldes - from comment 30 with comments addressed below

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

> Generally this patch looks good. My only concern is whether things will
> still work correctly after bug 985252 lands since that patch causes us to
> build sandbox code regardless of whether
> --enable-content-sandbox/MOZ_CONTENT_SANDBOX is specified. I plan to land
> bug 985252 momentarily so this may be as simple as rebasing and verifying
> that everything builds/runs as expected.

Checked through the uses of this after bug 985252.
Here's a try run without --enable-content-sandbox:
https://tbpl.mozilla.org/?tree=Try&rev=e24c93c09db0

> > +{
> > +  sWarnOnlySandboxEnabled = true;
> > +  if (aLogToConsoleCallback) {
> > +    sLogToConsoleCallback = aLogToConsoleCallback;
> > +    sGetIntPrefCallback = aGetIntPrefCallback;
> 
> It seems like this will produce unexpected results if we call
> `Enable(nullptr, someFunctionPointer)` or `Enable(someFunctionPointer,
> nullptr)`. Maybe we should do separate checks for `aLogToConsoleCallback`
> and `aGetIntPrefCallback` or remove the check altogether?

Check removed.

> > +#if defined(NIGHTLY_BUILD) && defined(XP_WIN) && defined(MOZ_CONTENT_SANDBOX)
> > +// Simple wrapper to return an int pref when on the main thread and -1 when
> > +// not.  This is to be used as a callback by the Window's warn only sandbox.
> > +static int32_t
> > +GetIntPrefOnMainThread(const char* aPref)
> 
> Do we use the "return [...] -1 when not [on the main thread]" functionality
> of this function? Should it instead be an error to try to call this function
> off the main thread?

Added comment as to why we need this.
Attachment #8453130 - Attachment is obsolete: true
Attachment #8468482 - Flags: review+
Comment on attachment 8468470 [details] [diff] [review]
Part 1: Import Chromium StackTrace code. v4

As I mentioned yesterday on IRC, would you mind having a look at the moz.build changes in this patch?
Attachment #8468470 - Flags: review?(benjamin)
Comment on attachment 8468482 [details] [diff] [review]
Part 2: Add warn only sandbox functionality for Windows processes. v4

In this one there are ipc and toolkit changes as well as moz.build changes.
Thanks.
Attachment #8468482 - Flags: review?(benjamin)
Comment on attachment 8468470 [details] [diff] [review]
Part 1: Import Chromium StackTrace code. v4

Why are we using this code instead of the existing NS_StackWalk code?
Flags: needinfo?(bobowencode)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #41)
> Comment on attachment 8468470 [details] [diff] [review]
> Part 1: Import Chromium StackTrace code. v4
> 
> Why are we using this code instead of the existing NS_StackWalk code?

It was a while ago, but I'm pretty sure I attempted to use that code, but ran into a problem.
I can't remember if it was some sort of linking issue or just that I couldn't work out how to use it properly. :-)

If you think it could work in this circumstance and have an example I could follow, I'd be happy to go with that instead.
Flags: needinfo?(bobowencode)
Comment on attachment 8468482 [details] [diff] [review]
Part 2: Add warn only sandbox functionality for Windows processes. v4

What are the threading requirements of these functions? It's not clear from the patch whether LogToConsoleCallback will be main-thread only, but it uses prefs which are certainly main-thread-only.

This at least needs better documentation.

I also think the naming of some of this is strange: +pref("browser.tabs.remote.sandbox.warnOnly", false);

If this pref is false, does that mean there is no sandbox at all, or that there is a strict sandbox?

Similar question for the mozilla::warnonlysandbox::Enable() function. It appears that you're calling this function in GeckoChildProcessHost::PrepareLaunch, no matter what kind of process is being launched. And then in XRE_InitChildProcess, it looks like we're turning on the warn-only sandbox for all process types, when presumably this should only affect content processes?
Attachment #8468482 - Flags: review?(benjamin) → review-
Comment on attachment 8468470 [details] [diff] [review]
Part 1: Import Chromium StackTrace code. v4

I'm not the best person to deal with stackwalking nowadays: I'm going to delegate this to froydnj. I'm just allergic to having duplicate functions for performing this sort of thing.

I'm also a little worried that dbghelp.dll isn't always available on WinXP and the dependency doesn't appear to be debug-only.
Attachment #8468470 - Flags: review?(benjamin) → review?(nfroyd)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #44)
> Comment on attachment 8468470 [details] [diff] [review]
> Part 1: Import Chromium StackTrace code. v4
> 
> I'm not the best person to deal with stackwalking nowadays: I'm going to
> delegate this to froydnj. I'm just allergic to having duplicate functions
> for performing this sort of thing.
> 
> I'm also a little worried that dbghelp.dll isn't always available on WinXP
> and the dependency doesn't appear to be debug-only.

I agree, if I can do this with existing code, I'm all for it.
I'm also slightly worried about non-debug WinXP builds.
I used it for browsing for a while on a VM and didn't hit the problem, but that's only fairly limited testing and I suspect for WinXP we often don't catch things until release.
Does all this sandboxing code wind up in libxul, or does it wind up in a separate executable?  If it winds up in libxul, we ought to be able to use all our stack unwinding functions.  If it winds up in a separate blob, I care less about the duplication of effort.

Can you try using NS_StackWalk again and report back what the issues are?
Flags: needinfo?(bobowencode)
(In reply to Nathan Froyd (:froydnj) from comment #46)
> Does all this sandboxing code wind up in libxul, or does it wind up in a
> separate executable?  If it winds up in libxul, we ought to be able to use
> all our stack unwinding functions.  If it winds up in a separate blob, I
> care less about the duplication of effort.

It ends up in the sandbox_s library, which is linked into sandboxbroker.dll and plugin_container.exe
sandboxbroker.dll is linked into xul.dll
 
> Can you try using NS_StackWalk again and report back what the issues are?

I certainly can (probably be next Monday before I get a chance, PTO until then), do you have a good example for me to follow?
Flags: needinfo?(bobowencode) → needinfo?(nfroyd)
Comment on attachment 8468470 [details] [diff] [review]
Part 1: Import Chromium StackTrace code. v4

Canceling this review while while we figure out if NS_StackWalk can be used.

(In reply to Bob Owen (:bobowen) from comment #47)
> > Can you try using NS_StackWalk again and report back what the issues are?
> 
> I certainly can (probably be next Monday before I get a chance, PTO until
> then), do you have a good example for me to follow?

I'm not sure what sort of example you mean other than e.g.:

http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsTraceRefcnt.cpp#970

Ah, looking at that, I guess the problem might have been that we don't have stackwalking facilities available with certain compilation options (I think we forgo the necessary bits, like frame pointers, entirely in release builds)?  Does that sound plausible?

But it sounds like CaptureStackBackTrace is subject to basically the same limitations, so I'm not sure what we're gaining from using that.  Where is this warn-only sandbox stuff intended to be deployed?  Is it for local development, or are we intending on distributing release builds with the sandboxing stuff hidden behind a pref?
Attachment #8468470 - Flags: review?(nfroyd)
Flags: needinfo?(nfroyd) → needinfo?(bobowencode)
Thanks for looking at this, sorry for the delay, but I've been on PTO.

A couple of questions over your comments below.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #43)
> Comment on attachment 8468482 [details] [diff] [review]
> Part 2: Add warn only sandbox functionality for Windows processes. v4
> 
> What are the threading requirements of these functions? It's not clear from
> the patch whether LogToConsoleCallback will be main-thread only, but it uses
> prefs which are certainly main-thread-only.

The logging needs to work on any thread.
LogToConsoleCallback gets set to nsContentUtils::LogMessageToConsole, I can't spot where this is using prefs?

The other callback which is specifically designed to retrieve an int pref, returns -1 off main thread and I then use a cached value.

> I also think the naming of some of this is strange:
> +pref("browser.tabs.remote.sandbox.warnOnly", false);
> 
> If this pref is false, does that mean there is no sandbox at all, or that
> there is a strict sandbox?
> 
> Similar question for the mozilla::warnonlysandbox::Enable() function. It
> appears that you're calling this function in
> GeckoChildProcessHost::PrepareLaunch, no matter what kind of process is
> being launched. And then in XRE_InitChildProcess, it looks like we're
> turning on the warn-only sandbox for all process types, when presumably this
> should only affect content processes?

If false then the normal strict sandbox would be in place, maybe warnOnlyMode would be better.
This was originally being developed for the content sandbox, but might be useful for any, so I should look at moving it somewhere else, maybe security.sandbox.warnOnlyMode?
Flags: needinfo?(benjamin)
(In reply to Nathan Froyd (:froydnj) from comment #48)

> I'm not sure what sort of example you mean other than e.g.:
> 
> http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsTraceRefcnt.
> cpp#970

Thanks, I'll give that a try.

> Ah, looking at that, I guess the problem might have been that we don't have
> stackwalking facilities available with certain compilation options (I think
> we forgo the necessary bits, like frame pointers, entirely in release
> builds)?  Does that sound plausible?
> 
> But it sounds like CaptureStackBackTrace is subject to basically the same
> limitations, so I'm not sure what we're gaining from using that.  Where is
> this warn-only sandbox stuff intended to be deployed?  Is it for local
> development, or are we intending on distributing release builds with the
> sandboxing stuff hidden behind a pref?

I originally thought this was required for addon authors, but have since found out that it is more to do with checking that we are not adding new violations while we try and get to the point where we can turn sandboxing on by default.
So, the first of these may not be debug builds, although I'm not sure what addon authors normally use.
However addon authors would hopefully be able to work out, which part of their code might be causing the violation.
The second will need to be some sort of log parsing on automated tests, so this could be done on debug builds.

Anyway, clearly I need to test either stack trace method on an opt build to see if it is even worth having it at all for those ... thanks for spotting this.
Flags: needinfo?(bobowencode)
> > What are the threading requirements of these functions? It's not clear from
> > the patch whether LogToConsoleCallback will be main-thread only, but it uses
> > prefs which are certainly main-thread-only.
> 
> The logging needs to work on any thread.
> LogToConsoleCallback gets set to nsContentUtils::LogMessageToConsole, I
> can't spot where this is using prefs?

The Log function itself is using sGetIntPrefCallback, which I assumed was main-thread-only. I see below...

> The other callback which is specifically designed to retrieve an int pref,
> returns -1 off main thread and I then use a cached value.

that it returns -1 from other threads, but I don't see the caching (in the Log method or in GetIntPrefOnMainThread). I expect that what this should really be doing is caching the pref value in an atomic and reading that from any thread.

> This was originally being developed for the content sandbox, but might be
> useful for any, so I should look at moving it somewhere else, maybe
> security.sandbox.warnOnlyMode?

I think the following requirements apply:

* The preference should be separate for different process types, or should only apply to content processes: we want to control the sandbox settings for GMP plugin processes separately from the sandbox settings for content processes.
* The name should make it clear that we're turning *off* a security protection, instead of perhaps turning *on* some enhanced security feature.

If we already have a pref for enabling content-process sandboxes at all, perhaps we should just make that a tri-state: off/warn/enable?
Flags: needinfo?(benjamin)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #51)
 
> The Log function itself is using sGetIntPrefCallback, which I assumed was
> main-thread-only. I see below...
> 
> > The other callback which is specifically designed to retrieve an int pref,
> > returns -1 off main thread and I then use a cached value.
> 
> that it returns -1 from other threads, but I don't see the caching (in the
> Log method or in GetIntPrefOnMainThread). I expect that what this should
> really be doing is caching the pref value in an atomic and reading that from
> any thread.

The caching part is:

if (stackTraceDepth > -1) {
  sStackTraceDepth = stackTraceDepth;
}
if (sStackTraceDepth) {

Where sStackTraceDepth is static and it only gets updated when on main thread.
I could add a comment here explaining this as well as above GetIntPrefOnMainThread.

If I cached in an atomic as you describe, wouldn't any change to the pref require a restart?

> > This was originally being developed for the content sandbox, but might be
> > useful for any, so I should look at moving it somewhere else, maybe
> > security.sandbox.warnOnlyMode?
> 
> I think the following requirements apply:
> 
> * The preference should be separate for different process types, or should
> only apply to content processes: we want to control the sandbox settings for
> GMP plugin processes separately from the sandbox settings for content
> processes.

OK, I'll try to come up with something, might discuss it at the sandboxing meeting on Thursday.

> * The name should make it clear that we're turning *off* a security
> protection, instead of perhaps turning *on* some enhanced security feature.

Right, I see what you mean, I'll take this into account.
 
> If we already have a pref for enabling content-process sandboxes at all,
> perhaps we should just make that a tri-state: off/warn/enable?

No, it is a configure flag at the moment (--enable-content-sandbox), but I want to change it to a nightly only pref (bug 1040016).
So, maybe I should do it as part of this bug, I like the tri-state idea.
The main problem is that I think people were against using a pref as it could be fairly easily turned off.
That's why I think we ended up with the configure flag.
However, I think that making it a nightly only pref that we will eventually remove, should be OK.
Having it as a pref will make it much easier for people to try out the sandbox.
Flags: needinfo?(benjamin)
Oh, I was looking for the cache in the getintpref code. If we need it, I think it should be there, and not in Log().

The better cache would be to use AddUintVarCache (if atomics aren't necessary) or use an equivalent callback with an atomic. That way even if there are only violations from non-main threads, the pref will still be honored.
Flags: needinfo?(benjamin)
Re-written a fair bit of part 2 following the review from bsmedberg and advice from froydnj.

This now uses NS_StackWalk to print the stack trace, so no need for new chromium code.
Once I'd got that working and with the feedback about preferences, it seemed to make sense to move all the logging code back into libxul and just pass one callback to the warn only sandbox code.

This is working locally, but the try run here doesn't seem to be logging anything for debug:
https://tbpl.mozilla.org/?tree=Try&rev=b2e955376c4d
New part 1 that replaces the old parts 1 and 2 with the following changes:

* Changed configure.in, so that MOZ_CONTENT_SANDBOX is set on Windows NIGHTLY_BUILD.
* Added a tri-state pref (off(or anything else)/warn/on) to control the Windows content sandbox.
* Added code that is #if included by MOZ_CONTENT_SANDBOX and XP_WIN to allow the Windows content sandbox to be turned on using the pref.
* As before, warn only mode changes are #if included by NIGHTLY_BUILD, to make sure that these changes never go past Nightly by mistake.
* Uses AddUintVarCache to cache the warn stack trace depth in a static variable, so that it can be used off main thread - this is much better thanks. :-)
* Uses NS_StackWalk instead of Chromium stack trace code.
* Now AddUintVarCache and particularly NS_StackWalk are being used it seemed to make sense to move the log function into libxul (included from wosCallbacks.h in nsEmbedFunctions.cpp).  This means that only this log function callback needs to be passed into the warn only sandbox code.
Attachment #8468470 - Attachment is obsolete: true
Attachment #8468482 - Attachment is obsolete: true
Attachment #8481206 - Flags: review?(benjamin)
Replaces old part 3.

r=tabraldes - from comment 31
r=netzen - from comment 34

As suggested in comment 34, added new file to track changes to Chromium code that would need to be reapplied after taking a new version of the code from Chromium.
Attachment #8453131 - Attachment is obsolete: true
Attachment #8453132 - Attachment is obsolete: true
Attachment #8481211 - Flags: review+
Try push (only change from current patches is moving the text file tracking Chromium code changes to Part 2):
https://tbpl.mozilla.org/?tree=Try&rev=e2c449e0342f

Windows mochitest Try push with the warn only sandbox enabled, but no stack trace:
https://tbpl.mozilla.org/?tree=Try&rev=ff5c13b2f09e
Comment on attachment 8481206 [details] [diff] [review]
Part 1: Add warn only sandbox functionality for Windows processes. v5

Is there a particular reason you don't want this to leak past nightly builds? I don't think that's necessary, as long as this is all within the MOZ_CONTENT_SANDBOX #ifdef. And it would make the combination of #ifdefs and possible flags a lot smaller and easier to reason about.

I don't understand why sStartInWarnOnlyMode is a static variable in warnOnlySandbox.cpp. This still seems like a policy decision that we should be making using member variables in GeckoChildProcessHost, not global statics. Is the static variable really necessary?

How does this code affect SandboxBroken::LaunchApp when called for non-content processes (GMP processes)? None of this code will affect the sandbox for GMP processes, correct?
Attachment #8481206 - Flags: review?(benjamin)
Flags: needinfo?(bobowencode)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #58)
> Comment on attachment 8481206 [details] [diff] [review]
> Part 1: Add warn only sandbox functionality for Windows processes. v5
> 
> Is there a particular reason you don't want this to leak past nightly
> builds? I don't think that's necessary, as long as this is all within the
> MOZ_CONTENT_SANDBOX #ifdef. And it would make the combination of #ifdefs and
> possible flags a lot smaller and easier to reason about.

It was just because this is only going to be a temporary measure, because we wouldn't want this code or the pref in place when we start to ship the sandbox proper.
Otherwise it might be too easy for attackers to persuade people to disable the sandbox or do it through an addon.

I was probably just being over cautious.
So, I've changed all of the NIGHTLY_BUILDs to MOZ_CONTENT_SANDBOX, consolidating where possible.
This does make the pre-processor logic simpler.

The only other thing was that in Part 2, I've had to add logging statements to the interception code, which will affect all sandboxing (albeit just with an extra function call for non-content processes).

These are also included with #ifdef NIGHTY_BUILD, do you think I should change these to MOZ_CONTENT_SANDBOX as well for consistency?
 
> I don't understand why sStartInWarnOnlyMode is a static variable in
> warnOnlySandbox.cpp. This still seems like a policy decision that we should
> be making using member variables in GeckoChildProcessHost, not global
> statics. Is the static variable really necessary?

Ah, I had it this way before I'd added in the pref to turn the sandbox proper on.
You're right it definitely makes sense to move this to a member variable to match, thanks.

> How does this code affect SandboxBroken::LaunchApp when called for
> non-content processes (GMP processes)? None of this code will affect the
> sandbox for GMP processes, correct?

Freudian slip in that namespace there. :D
As I mentioned above, the only affect on non-content processes will be an extra one or two function calls in the interception code. These will return immediately because the log function pointer won't be set.

Try is closed, so I've only tested this locally at the moment.
Had to do a bit of work re-basing this as well, because of other changes, so I'll do a limited Try push when it's open to make sure I haven't broken things during this re-work.
Attachment #8481206 - Attachment is obsolete: true
Attachment #8482300 - Flags: review?(benjamin)
Flags: needinfo?(bobowencode)
Try push building on various platforms and running tests on Windows XP and 7:
https://tbpl.mozilla.org/?tree=Try&rev=2f7f74f08027

Windows mochitest Try push with the warn only sandbox enabled, but no stack trace:
https://tbpl.mozilla.org/?tree=Try&rev=bc1f97703a79
Comment on attachment 8482300 [details] [diff] [review]
Part 1: Add warn only sandbox functionality for Windows processes. v6

>diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp

>-    mozilla::SandboxTarget::Instance()->StartSandbox();
>+    nsAdoptingString contentSandboxPref =
>+        Preferences::GetString("browser.tabs.remote.sandbox");

I'm surprised that this is an adoptingstring, but it probably doesn't hurt. I'm also a little surprised that we're using prefs in the child for this, but communicating that state in some other way probably isn't worth the hassle.

>+    if (contentSandboxPref == NS_LITERAL_STRING("on")
>+        || contentSandboxPref == NS_LITERAL_STRING("warn")) {

But this should definitely be using .EqualsLiteral instead of == NS_LITERAL_STRING.

>diff --git a/ipc/glue/GeckoChildProcessHost.cpp b/ipc/glue/GeckoChildProcessHost.cpp

>+#if defined(MOZ_CONTENT_SANDBOX)
>+  // We need to get the pref here as the process is launched off main thread.
>+  if (mProcessType == GeckoProcessType_Content) {
>+    nsAdoptingString contentSandboxPref =
>+      Preferences::GetString("browser.tabs.remote.sandbox");
>+    if (contentSandboxPref == NS_LITERAL_STRING("on")) {

EqualsLiteral again ;-)


>+++ b/security/sandbox/win/src/warnonlysandbox/warnOnlySandbox.h

>+#ifndef __SECURITY_SANDBOX_WARNONLYSANDBOX_H__
>+#define __SECURITY_SANDBOX_WARNONLYSANDBOX_H__

nit, please use security_sandbox_warnOnlySandbox_h__ for this define name and for wostypes below.
Attachment #8482300 - Flags: review?(benjamin) → review+
Thanks Ben, I'll sort out those last few things tomorrow.

Sorry to bother you again, but do you think I should change Part 2 to use MOZ_CONTENT_SANDBOX instead of NIGHTLY_BUILD as well?
Flags: needinfo?(benjamin)
Yes.
Flags: needinfo?(benjamin)
r=bsmedberg - from comment 61

Now uses .EqualsLiteral and changed the header guards define names as requested.
Attachment #8482300 - Attachment is obsolete: true
Attachment #8483547 - Flags: review+
r=tabraldes - from comment 31
r=netzen - from comment 34

Changed to use MOZ_CONTENT_SANDBOX instead of NIGHTLY_BUILD as per comment 62 and 63.
Attachment #8481211 - Attachment is obsolete: true
Attachment #8483548 - Flags: review+
r=bsmedberg - from comment 61

Just did a release build and caught a flaw in my #ifs, now fixed.

Windows only try push:
https://tbpl.mozilla.org/?tree=Try&rev=3ab69dc3a51a
Attachment #8483547 - Attachment is obsolete: true
Attachment #8483568 - Flags: review+
Comment on attachment 8483568 [details] [diff] [review]
Part 1: Add warn only sandbox functionality for Windows processes. v8

The sandboxing side of things has changed a fair bit, because of the reviews for the other bits.
So, I don't feel comfortable carrying your previous r+. :)
Attachment #8483568 - Flags: review?(tabraldes)
Comment on attachment 8483568 [details] [diff] [review]
Part 1: Add warn only sandbox functionality for Windows processes. v8

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

> Is there a particular reason you don't want this to leak past nightly
> builds? I don't think that's necessary, as long as this is all within the
> MOZ_CONTENT_SANDBOX #ifdef. And it would make the combination of #ifdefs and
> possible flags a lot smaller and easier to reason about.

See below.

::: browser/app/profile/firefox.js
@@ +1190,5 @@
> +// on = full sandbox enabled
> +// warn = warn only sandbox enabled
> +// anything else = sandbox disabled
> +// This will probably require a restart.
> +pref("browser.tabs.remote.sandbox", "off");

This pref was going to be nightly-only. Now it seems that we'll be including a pref in our release versions of Firefox that can disable the content sandbox. Since users frequently encounter malware that changes prefs, I'm pretty nervous about this pref existing in release versions. We already have an environment variable MOZ_DISABLE_CONTENT_SANDBOX that can disable the sandbox; could we enhance that to control whether we use a warn-only sandbox also? It's comparatively difficult for malware (or social engineering) to set an environment variable compared to flipping a pref.
My understanding of this is that we're going to start out *not* using a sandbox, and progressively opt in to warning and then enabling the sandbox.

At the point where we turn the sandbox on by default, I suggest that we completely remove all of this code (or force-disable it). Until then, I don't see why it shouldn't just ride the normal trains.
Comment on attachment 8483568 [details] [diff] [review]
Part 1: Add warn only sandbox functionality for Windows processes. v8

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

OK that makes sense. As long as we're going to remove the pref eventually I'm happy with this patch.
Attachment #8483568 - Flags: review?(tabraldes) → review+
I think this is resolved, but it's clear that I haven't explained this well, so I'll add my thoughts.

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

> > +pref("browser.tabs.remote.sandbox", "off");
> 
> This pref was going to be nightly-only. Now it seems that we'll be including
> a pref in our release versions of Firefox that can disable the content
> sandbox. Since users frequently encounter malware that changes prefs, I'm
> pretty nervous about this pref existing in release versions. We already have
> an environment variable MOZ_DISABLE_CONTENT_SANDBOX that can disable the
> sandbox; could we enhance that to control whether we use a warn-only sandbox
> also? It's comparatively difficult for malware (or social engineering) to
> set an environment variable compared to flipping a pref.

Currently, because of the change in configure.in, this is still effectively a Nightly-only pref.
MOZ_CONTENT_SANDBOX will be defined as 1 (assuming no --disable-sandbox) for Windows on Nightly, but not afterwards.
So, at the moment this will only be riding the train to the first stop. :)
As we've just gone through a release cycle, we've got time to change that if we want.

This does mean that when we use MOZ_CONTENT_SANDBOX we have to be careful it will work without it, by doing non-Nightly builds (I don't think this currently happens on m-i).

When I was originally asked to look at this, I thought that the main use was for addon developers, so they could test and get useful debug information if things didn't work with the sandbox.
Hopefully it will prove useful to Firefox devs as well.
While I'm sure most addon devs could handle an env var, prefs make it even easier.

Brad has since told me that the original intention was so that we can set up some sort of violation tracking on tbpl.
This will probably take the form of a simple count of violations with an allowable limit, to make sure we don't get new ones while we're trying to get rid of existing ones.
I imagine an env var would have been just as easy for this use-case.

I think the other advantage of having it as a pref, particularly one close to the e10s prefs, is that people might be more inclined to try it out at the same time as trying out e10s.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #69)
> My understanding of this is that we're going to start out *not* using a
> sandbox, and progressively opt in to warning and then enabling the sandbox.
> 
> At the point where we turn the sandbox on by default, I suggest that we
> completely remove all of this code (or force-disable it). Until then, I
> don't see why it shouldn't just ride the normal trains.

Yeah, it was always my intention that this would be a temporary pref that we would remove when we started shipping for real. That's why I've left the MOZ_DISABLE_CONTENT_SANDBOX env var, as at least to start with we will want a fairly simple but relatively safe way to disable the sandbox.

Unfortunately, this is not a perfect warn only sandbox.
It still puts the full sandbox in place, but then adds all of the policy rules that send requests to the Broker.
It adds logging to the interception code that sends these requests, so that we know when this happens. 
This means that some things that we can't allow through these rules may still break and we won't get anything in the logs.

I suppose we could ship with warn as the default, just before we turn it on for real, as a sort of trial run.
If we do that it would probably make sense to change the logging to telemetry.

Anyway, there are likely to be a lot of things we need to sort out before we get to that point. :-)
Thanks to all the various reviewers for their help on this.

Please land in Part number order, thanks.

I reduced the scope of the try pushes as the review changes got smaller.
Fairly full try push of what was pretty much v5 of Part 1:
https://tbpl.mozilla.org/?tree=Try&rev=e2c449e0342f

Try push of v6 building on various platforms and running tests on Windows XP and 7:
https://tbpl.mozilla.org/?tree=Try&rev=2f7f74f08027

Windows 7 only try push of final patches:
https://tbpl.mozilla.org/?tree=Try&rev=3ab69dc3a51a
Keywords: checkin-needed
r=tabraldes - for the sandboxing changes from comment 70
Attachment #8483568 - Attachment is obsolete: true
Attachment #8484302 - Flags: review+
Comment on attachment 8484302 [details] [diff] [review]
Part 1: Add warn only sandbox functionality for Windows processes. v9

NS_StackWalk isn't included in optimized Windows builds that don't have DEBUG or MOZ_PROFILING.

Most of the m-i (and try) builds have MOZ_PROFILING, but fortunately B2G Desktop Windows doesn't, so this got caught.

Here's a new patch with some #ifdefs around the stack walking parts.
Try push with a test for Windows 7 and build on B2G Desktop Windows Opt:
https://tbpl.mozilla.org/?tree=Try&rev=2a162e0f0c16

I'm thinking of filing a follow-up bug for a new define (MOZ_STACKWALKING) in configure.in based on the logic in xpcom/base/moz.build for whether nsStackWalk.cpp gets built.
This could then be used there and in at least three other places.
What do you think?
Attachment #8484302 - Flags: review?(benjamin)
#if MOZ_PROFILING || MOZ_DEBUG sounds good for now. I don't have a strong opinion about the MOZ_STACKWALKING define.

Updated

5 years ago
Attachment #8484302 - Flags: review?(benjamin) → review+
Take two. :)

Added some #ifdefs to isolate the Stack Walking code that isn't always available.

On top of the previous Try pushes in comment 72 here's one with the build that failed last time:
https://tbpl.mozilla.org/?tree=Try&rev=2a162e0f0c16

Please land in Part number order, thanks.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7be34c88c9ac
https://hg.mozilla.org/mozilla-central/rev/e7eef85c1b0a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.