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

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

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

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.

Comment 1

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

Attachment: bug921817-seccomp-nonprealloc-hg0.diff

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.

Comment 2

[Guillaume Destuynder [:kang] [this account is no longer active]]

Comment on attachment 811651 [details] [diff] [review]
bug921817-seccomp-nonprealloc-hg0.diff

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.

thanks!

::: dom/ipc/ContentParent.cpp
@@ +1316,5 @@
>  
> +#ifdef MOZ_CONTENT_SANDBOX
> +    // 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

Comment 3

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

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.

Comment 4

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

Attachment: bug921817-seccomp-nonprealloc-hg1.diff

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.

Comment 5

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

Comment on attachment 813913 [details] [diff] [review]
bug921817-seccomp-nonprealloc-hg1.diff

This still needs review by a DOM and/or IPC peer, I think?

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 813913 [details] [diff] [review]
bug921817-seccomp-nonprealloc-hg1.diff

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.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(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.

Comment 8

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

Attachment: bug921817-seccomp-nonprealloc-hg2.diff

Now with more curly braces.  Carrying over r=kang r=bent.

Comment 9

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

Checkin note: b2g-inbound.

And I think the followup bug in question already exists as bug 880808.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/597eac1e1988

Comment 11

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

Comment on attachment 816794 [details] [diff] [review]
bug921817-seccomp-nonprealloc-hg2.diff

[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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/597eac1e1988

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ae0bba31d967