Closed Bug 977859 Opened 6 years ago Closed 6 years ago

Drop uid 0 in all content processes immediately after fork

Categories

(Core :: IPC, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jld, Assigned: jld)

References

Details

(Keywords: sec-want)

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → jld
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.
Attachment #8385124 - Flags: review?(bent.mozilla)
Attachment #8385124 - Flags: feedback?(gdestuynder)
Component: Widget: Gonk → IPC
(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 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
Attachment #8385124 - Flags: feedback?(gdestuynder) → feedback+
(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 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.
Attachment #8385124 - Flags: review?(bent.mozilla) → review+
Fixed and rebased.  Carrying over r+(bent,kang).

Trying: https://tbpl.mozilla.org/?tree=Try&rev=4fd1b468c3e4
Attachment #8385124 - Attachment is obsolete: true
Attachment #8389622 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/602a61ed0448
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.