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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 3 obsolete files)

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).
Product: Calendar → Core
Blocks: 928044
No longer blocks: 925570
OS: Windows NT → All
Hardware: x86_64 → All
Blocks: 925570
Attached patch Patch v1. (obsolete) — Splinter Review
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)
*kang
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+
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-
Attached patch Patch 1. rev2. (obsolete) — Splinter Review
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)
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.
Ya let's wait for jld and dkeeler to weigh in
Flags: needinfo?(jld)
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)
(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)
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.
Attachment #822757 - Attachment description: Patch v1. rev2. → Patch 1. rev2.
Attached patch bug928042_pref.diff (obsolete) — Splinter Review
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+
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)
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)
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+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: