977859 - Drop uid 0 in all content processes immediately after fork

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

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

Once camera access no longer depends on Unix group membership, we can set a content process's uid immediately after it forks.  The main process and Nuwa still need to be root, because each content process runs as its own uid, but preallocated processes don't need to wait until RecvSetProcessPrivileges.

We do however still need an IPC call at that point to turn on sandboxing, until/unless bug 880808 can move it earlier, but we might want to change the name to reflect that purpose.

Comment 1

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

Attachment: bug977859-depriv-on-fork-hg0.diff

Based on the patches for bug 976398 and bug 968002.  Inherently depends on having some fix for bug 976398, obviously.

I'm still not entirely sure who reviews which parts of the systems-y side of IPC these days; redirect if needed.

Comment 2

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

(In reply to Jed Davis [:jld] from comment #1)
> Based on the patches for

To clarify, that's “based on” as in indicating the base revision that the patch is taken against (and without which it might not apply cleanly), not “based on” as in “derived from” in any other way.

Comment 3

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

Comment on attachment 8385124 [details] [diff] [review]
bug977859-depriv-on-fork-hg0.diff

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

bug 942698 seems to move setprocesssandbox to another ipc-received-msg, so having it at a similar place as where we drop uid looks better. even thus linux desktop doesn't seem to have nuwa yet (and afaik nobody's working on getting it there)

other than that its fine - as in its better than what we previously had

Comment 4

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

(In reply to Guillaume Destuynder [:kang] (use NEEDINFO!) from comment #3)
> bug 942698 seems to move setprocesssandbox to another ipc-received-msg, so
> having it at a similar place as where we drop uid looks better. even thus
> linux desktop doesn't seem to have nuwa yet (and afaik nobody's working on
> getting it there)

I think I'll look into that as a followup — if we can start the sandbox immediately after fork in a Nuwa-templated child, then we might be able to start it at the same time we'd call 
OnFinishNuwaPreparation (but don't) in a plain child.  Or maybe not, because threads, but it's worth trying.

Comment 5

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

Comment on attachment 8385124 [details] [diff] [review]
bug977859-depriv-on-fork-hg0.diff

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

This looks ok to me as long as these are addressed:

::: dom/ipc/ContentParent.h
@@ +294,5 @@
>  
>      // Transform a pre-allocated app process into a "real" app
>      // process, for the specified manifest URL.  If this returns false, the
>      // child process has died.
> +    bool TransformPreallocatedIntoApp(const nsAString& aAppManifestURL);

This can no longer fail so it should return void, and the comment needs to be updated.

::: dom/ipc/PContent.ipdl
@@ +256,5 @@
>       * Update OS process privileges to |privs|.  Can usually only be
>       * performed zero or one times.  The child will abnormally exit if
>       * the privilege update fails.
>       */
> +    async SetProcessSandbox();

Nit: The comment needs to be updated too.

::: mozglue/build/Nuwa.cpp
@@ +1540,5 @@
> +{
> +  void (*AfterNuwaFork)();
> +
> +  AfterNuwaFork = (void (*)())
> +    dlsym(RTLD_DEFAULT, "AfterNuwaFork");

Nit: I'd comment that this lives in ContentChild.

Comment 6

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

Attachment: bug977859-depriv-on-fork-hg2.diff

Fixed and rebased.  Carrying over r+(bent,kang).

Trying: https://tbpl.mozilla.org/?tree=Try&rev=4fd1b468c3e4

Comment 7

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

https://hg.mozilla.org/integration/b2g-inbound/rev/602a61ed0448

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/602a61ed0448