Closed
Bug 928042
Opened 12 years ago
Closed 12 years ago
Add an environment variable to disable content processes sandboxing even when MOZ_CONTENT_SANDBOX is defined
Categories
(Core :: Security, defect)
Core
Security
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: bbondy, Assigned: bbondy)
References
Details
(Whiteboard: [qa-])
Attachments
(1 file, 3 obsolete files)
8.40 KB,
patch
|
bbondy
:
review+
|
Details | Diff | Splinter Review |
Bug 925571 adds code to start sandboxed processes instead of normal processes. It would be nice to bypass that code though via a preference. We'd land this and then post another bug to enable it by default (in case the enabling by default needs to be backed out).
Updated•12 years ago
|
Product: Calendar → Core
Updated•12 years ago
|
Assignee | ||
Comment 1•12 years ago
|
||
Klang if you could review the Linux part of the changes I'd appreciate it.
Aklotz, sorry for all the reviews, things will slow down a lot soon as I enable things that break the browser in various ways and I need to find out why.
Attachment #821365 -
Flags: review?(gdestuynder)
Attachment #821365 -
Flags: review?(aklotz)
Assignee | ||
Comment 2•12 years ago
|
||
*kang
Comment 3•12 years ago
|
||
Comment on attachment 821365 [details] [diff] [review]
Patch v1.
Review of attachment 821365 [details] [diff] [review]:
-----------------------------------------------------------------
One thing to fix, then r=me.
::: ipc/glue/GeckoChildProcessHost.h
@@ +131,5 @@
> GeckoProcessType mProcessType;
> ChildPrivileges mPrivileges;
> Monitor mMonitor;
> FilePath mProcessPath;
> + bool mSandboxEnabled;
An initializer for mSandboxEnabled is missing in the constructor.
Attachment #821365 -
Flags: review?(aklotz) → review+
Assignee | ||
Comment 4•12 years ago
|
||
Comment on attachment 821365 [details] [diff] [review]
Patch v1.
Review of attachment 821365 [details] [diff] [review]:
-----------------------------------------------------------------
::: ipc/glue/GeckoChildProcessHost.h
@@ +131,5 @@
> GeckoProcessType mProcessType;
> ChildPrivileges mPrivileges;
> Monitor mMonitor;
> FilePath mProcessPath;
> + bool mSandboxEnabled;
Good catch, SetSadnboxEnabled is always being called so there's no bug, but I agree we should initialize it.
Comment on attachment 821365 [details] [diff] [review]
Patch v1.
Review of attachment 821365 [details] [diff] [review]:
-----------------------------------------------------------------
Since the new flag/pref is not respected under all conditions on B2G I'm setting r-. It would otherwise be ok.
::: dom/ipc/ContentParent.cpp
@@ +1510,5 @@
> // uid/gid/etc immediately after forking. Thus, we send this message,
> // which is otherwise a no-op, to sandbox it at an appropriate point
> // during startup.
> + if (ShouldSandboxContentProcesses() &&
> + aOSPrivileges != base::PRIVILEGES_INHERIT) {
in B2G, we also SendSetProcessPrivileges() in ContentParent::TransformPreallocatedIntoApp.
If you prefer to patch a single point of entry instead, I would use ShouldSandboxContentProcess() in dom/ipc/ContentChild.cpp at ContentChild::RecvSetProcessPrivileges (which is was is triggered by SendSetProcessPrivileges()) just before SetCurrentProcessSandbox()
Or even, in security/sandbox/Sandbox.cpp's SetCurrentProcessSandbox().
Maybe we could also use similar or same names/entries points instead.
Attachment #821365 -
Flags: review?(gdestuynder) → review-
Assignee | ||
Comment 6•12 years ago
|
||
I think having ContentParent control it makes sense and is clean, even with 2 checks.
Attachment #821365 -
Attachment is obsolete: true
Attachment #822757 -
Flags: review?(gdestuynder)
Comment on attachment 821365 [details] [diff] [review]
Patch v1.
Review of attachment 821365 [details] [diff] [review]:
-----------------------------------------------------------------
I'll have to blame the farewell party for mcoates to save the face (I'm still face palming myself right now), but I forgot to mention that, controlling the sandbox being enabled/disabled at the SendSetProcessPrivileges call means that when sandbox is disabled on B2G, we're also disabling setuid - which means making everything run as root, basically. I don't think that it's the intended functionality (?)
I do however think that we eventually would want to remove the setuid behind that function (for consistency between desktop and B2G), this depends on bug 845191 and probably also need an overall agreement/approval from more people. In fact, if bug 880808 was also fixed (in addition to the aforementioned bug 845191), we could remove SendSetProcessPrivileges altogether.
Sorry about that :(
This also means that the only control I can think of is indeed inside SetCurrentProcessSandbox or around it when it's called. Yeah :/
Note that, there's a little more to it. On Linux, using the same sandbox as B2G (which makes perfect sense), we also need a similar place for the SetCurrentProcessSandbox call anyway, which might end up being different from the B2G's SendSetProcessPrivileges.
Currently, to my knowledge, that call is not sent because desktop doesn't use the preallocated processes. Since we want to keep the whitelist small, and we need all threads initialized, there might be a better place to be found for the actual sandbox call (again, that'd be bug 880808)
Adding jld and dkeeler, specially with a needinfo on dkeeler: have you found a place to start the sandbox on desktop?
Flags: needinfo?(dkeeler)
Assignee | ||
Comment 9•12 years ago
|
||
The pref is enabled by default even if not specified, as long as the build flag is enabled. Which it is on b2g.
The pref is only intended to be used for troubleshooting problems.
That being said I think it's fine that it also disables setuid.
I can make it windows only as well if you prefer, I just thought a global pref to control all platforms would be nice.
I would generally advise against a pref that makes b2g run in a way that isn't expected, but I really don't have a long experience with what gets in or not, and how it's generally done.
If you think it's ok to have the pref that way, then it's fine for me. (in this case i can r+, it looks fine)
Else, we can just ask someone from the B2G team I guess.
Assignee | ||
Comment 11•12 years ago
|
||
Ya let's wait for jld and dkeeler to weigh in
Flags: needinfo?(jld)
Comment 12•12 years ago
|
||
The persistence makes me uneasy — the pref gets set once, and then things silently run with lowered security forever until and unless it's deliberately reset. But I'll defer to people who've dealt with this kind of thing before; I can't imagine this is the first time something like this has been considered. (And I do understand that we need to provide a reasonable opt-out for desktop Nightly users.)
From the b2g point of view: Is it safe to assume that anything that could change the pref would be able to make persistent changes to any other form of configuration (e.g., edit scripts to set an env var on boot)?
Flags: needinfo?(jld)
![]() |
||
Comment 13•12 years ago
|
||
(In reply to Guillaume Destuynder [:kang] (use NEEDINFO!) from comment #8)
> dkeeler: have you found
> a place to start the sandbox on desktop?
No - I was hoping desktop would be sharing the pre-allocated process mechanism b2g uses. If that's not going to happen, I can look into finding a place for it.
About having a preference to disable the sandbox: a) maybe it could be an environment variable? b) we're landing with a completely open sandbox anyway, right? So it doesn't matter if someone forgets to turn the sandbox back on until it actually does something. In the future, I think we're unfortunately going to need a "run Firefox in terrible, insecure mode" option. Whenever someone uses this, we should do something special with the UI like turn it red and have blinking marquees everywhere (mostly joking).
Flags: needinfo?(dkeeler)
Assignee | ||
Comment 14•12 years ago
|
||
An environment variable is an option, so is a command line option when you start Firefox. I just went with preference since that's the same way we control whether or not to use e10s in the first place right now.
Let me know which way you want me to go ahead with.
(In reply to David Keeler (:keeler) from comment #13)
> b) we're landing with a completely open sandbox
> anyway, right? So it doesn't matter if someone forgets to turn the sandbox
> back on until it actually does something. In the future, I think we're
> unfortunately going to need a "run Firefox in terrible, insecure mode"
> option. Whenever someone uses this, we should do something special with the
> UI like turn it red and have blinking marquees everywhere (mostly joking).
The B2G sandbox is getting closer to decent https://wiki.mozilla.org/FoxInABox#Status
I would also like if desktop was using the preallocated process, but I don't know if that's on their roadmap.
I like the environment variable option as well.
Assignee | ||
Updated•12 years ago
|
Attachment #822757 -
Attachment description: Patch v1. rev2. → Patch 1. rev2.
Assignee | ||
Comment 16•12 years ago
|
||
Same thing but with an environment variable to disable the functionality.
Attachment #822757 -
Attachment is obsolete: true
Attachment #822757 -
Flags: review?(gdestuynder)
Attachment #824811 -
Flags: review?(gdestuynder)
Attachment #824811 -
Flags: review?(gdestuynder) → review+
Assignee | ||
Updated•12 years ago
|
Summary: Add a preference to selectively decide if content processes should be sandboxed → Add an environment variable to disable content processes sandboxing even when MOZ_CONTENT_SANDBOX is defined
I had a chat with :briansmith earlier, as I have some concerns still regarding making b2g child processes run as root when this is enabled and I wasn't sure how we generally handle those things with prefs/env variables/etc in Firefox.
Since it *is* a regression when enabled for B2G, maybe it'd be better to do this:
- either implement the check at the SetCurrentProcessSandbox() level so that we don't have any regression or unexpected behavior (= be root)
- either skip b2g implementation for now.
Flags: needinfo?(brian)
Assignee | ||
Comment 18•12 years ago
|
||
I'll just make this windows only and if we want this preference on Linux Desktop Firefox and OSX we'll selectively include it for those platforms when needed.
Flags: needinfo?(brian)
Assignee | ||
Comment 19•12 years ago
|
||
Carrying forward aklotz's r+.
We'll file follow up bugs for osx and linux when they are using content sandboxing.
Attachment #824811 -
Attachment is obsolete: true
Attachment #826160 -
Flags: review+
Assignee | ||
Comment 20•12 years ago
|
||
Target Milestone: --- → mozilla28
Comment 21•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•