Closed Bug 921817 Opened 6 years ago Closed 6 years ago

seccomp sandbox isn't enabled in non-preallocated child processes


(Core :: Security, defect)

Gonk (Firefox OS)
Not set



Tracking Status
firefox25 --- wontfix
firefox26 --- fixed
firefox27 --- fixed
b2g-v1.2 --- fixed


(Reporter: jld, Assigned: jld)



(Whiteboard: [c= p= s= u=])


(1 file, 2 obsolete files)

Child processes that are created directly rather than using an existing preallocated process don't activate the system call sandbox, because they never get a SetProcessPrivileges message.  This includes (on b2g) apps that are started less than 5s after the last app started, and (to my surprise) all browser tabs.

I have a small patch that fixes this (but doesn't fix bug 880808).  This may all change when Nuwa (bug 771765) lands, but I think we'll also need to fix this for Gecko 26 / FxOS 1.2.
I'm not sure if this is the right place in ContentParent construction to be doing this — or, if it should be higher up to sandbox more of child process startup, what it means that preallocated processes are doing those same things with full privileges.
Attachment #811651 - Flags: review?(gdestuynder)
Attachment #811651 - Flags: review?(bent.mozilla)
Comment on attachment 811651 [details] [diff] [review]

Review of attachment 811651 [details] [diff] [review]:

This looks fine to me, except for the little comment change I mentioned.
I'm not 100% certain all things are initialized at the same state than after the SetCurrentProcessPrivileges() call, but according to :jld it worked fine in his tests (ie no need to add more things to the whitelist).

Since we're better with this than without, until the templating lands, I'm all for it.

If this patch is added, please add a note to bug 880808 so that we remember to also move/remove this patch, when bug 880808 gets fixed.


::: dom/ipc/ContentParent.cpp
@@ +1316,5 @@
> +    // A deprivileged process will change its uid/gid/etc immediately after
> +    // forking, but must sandbox itself after it's finished exec()ing and
> +    // initializing; this message accomplishes that.

I think this should hint at this bug (#921817) or that we usually do that in setprivileges, except for the case you're handling here, for clarity
Attachment #811651 - Flags: review?(gdestuynder) → review+
So Nuwa has landed.  The existing patch still works on m-c, and it's still what we'd need for Gecko 26 / FxOS 1.2.

On m-c, we can now fix bug 880808 instead, because reconstituting the forked process should be a good time to start sandboxing (and would be consistent for all children).  But we could also land this (once I fix the comment), and then uplift it (assuming it's approved for that), and then clean up sandbox initialization later.
Improved comment.  Carrying over previous r+.

Also, if my previous comment was unclear: m-c still fails to sandbox non-preallocated processes without this patch; Nuwa doesn't change that.
Attachment #811651 - Attachment is obsolete: true
Attachment #811651 - Flags: review?(bent.mozilla)
Attachment #813913 - Flags: review+
Comment on attachment 813913 [details] [diff] [review]

This still needs review by a DOM and/or IPC peer, I think?
Attachment #813913 - Flags: review?(bent.mozilla)
Comment on attachment 813913 [details] [diff] [review]

Review of attachment 813913 [details] [diff] [review]:

I wonder if we can't do this sooner... What about right after the SetProcessPriority call?

::: dom/ipc/ContentParent.cpp
@@ +1511,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 (aOSPrivileges != base::PRIVILEGES_INHERIT)
> +        SendSetProcessPrivileges(base::PRIVILEGES_INHERIT);

Nit: Please use braces.
(In reply to ben turner [:bent] (needinfo? encouraged) from comment #6)
> I wonder if we can't do this sooner... What about right after the
> SetProcessPriority call?

jld and I chatted about this today, I think we can do this in a followup (or at least investigate whether we need to or not) since this patch is infinitely better than what we have today.
Attachment #813913 - Flags: review?(bent.mozilla) → review+
Now with more curly braces.  Carrying over r=kang r=bent.
Attachment #813913 - Attachment is obsolete: true
Attachment #816794 - Flags: review+
Checkin note: b2g-inbound.

And I think the followup bug in question already exists as bug 880808.
Blocks: b2g-seccomp
Keywords: checkin-needed
Comment on attachment 816794 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 790923
User impact if declined: Users on devices with the seccomp-bpf kernel patches (e.g., Geeksphone; but not the "official" 1.2 devices) won't actually get content process sandboxing for browser tabs and some apps.
Testing completed (on m-c, etc.): The emulator tests on b2g-inbound are unburnt, and I've done some minor dogfooding with it.
Risk to taking this patch (and alternatives if risky): Very little.  This won't actually change any permissions on the "official" 1.2 devices, because they don't have kernel support, and if somehow there were side-effects we'd see them in TBPL.
String or IDL/UUID changes made by this patch: None.
Attachment #816794 - Flags: approval-mozilla-aurora?
Whiteboard: [c= p= s= u=]
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Attachment #816794 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.