776647 - Drop to no-rights user after fork()

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Guillaume has the code to do this already, so assigning to him.

Unfortunately, we can only do this for Gonk, because all of our usages of content processes rely on normal privileges currently.

Comment 1

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

Attachment: Setuid support

Patch is also at https://github.com/gdestuynder/mozilla-central/commit/5340be48aacb764e105e17eb1455fdb8da73856b

Some explanations:

- as long as we don't use SHM/FS using a single UID should be fine (incl. IPDL via SHM I guess)
- I'm using app_0 on Android instead of nobody, as the UID needs to be hard-coded, and nobody may be used by other daemons
- app_0 cannot be renamed without modifying Bionic the libc, it's hard-coded as well
- we can use one UID per app in the future if wanted/needed as long as we track the number of child processes
- the 65534 UID fallback is arbitrary, albeit generally "nobody" on many distros (but not all)

I have tested this patch with the current OOP applications such as the browser and the calculator with success so far.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 645071 [details] [diff] [review]
Setuid support

>diff --git a/ipc/chromium/src/base/process_util_linux.cc b/ipc/chromium/src/base/process_util_linux.cc
>index 1b013fe..2457789 100644
>--- a/ipc/chromium/src/base/process_util_linux.cc
>+++ b/ipc/chromium/src/base/process_util_linux.cc
>@@ -22,6 +22,9 @@
> 
> #ifdef ANDROID
> #include <pthread.h>
>+#include <private/android_filesystem_config.h>
>+#define CHILD_UID AID_APP
>+#define CHILD_GID AID_APP

Please document these.  It probably makes sense to call these
CHILD_UNPRIVILEGED_GID/UID.

>+#define CHILD_UID 65534
>+#define CHILD_GID 65534

And here too.

> #endif
> 
> namespace {
>@@ -200,6 +206,18 @@ bool LaunchApp(const std::vector<std::string>& argv,
>       argv_cstr[i] = const_cast<char*>(argv[i].c_str());
>     argv_cstr[argv.size()] = NULL;
> 
>+    if (setgid(CHILD_GID) != 0) {

Let's add an argument to LaunchApp()

 enum ChildPrivileges {
   UNPRIVILEGED, SAME_PRIVILEGES_AS_PARENT
 };

and only do the setuid/setgid when ChildPrivileges ==
UNPRIVILEGED.  We want to pass UNPRIVILEGED in
GeckoChildProcessHost.cpp when we're on gonk (it won't work
anywhere else, and would break things).

>+    if (chdir("/") != 0)
>+        gProcessLog.print("==> could not chdir()\n");

What is this for?

Looking good.  Would like to see the next version of the patch.

Comment 3

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

Attachment: Same patch with requested changed

I added the changes that I believe were requested. Hopefully I understood them correctly, in particular the places where it could be using SAME_PRIVILEGES_AS_PARENT vs UNPRIVILEGED.

chdir() just make sure we're in a directory that won't fail to list, it's not really a security issue in that case. 

Also if we have seccomp later on, no operation related to that should occur (at least in theory).

I'm fine dropping it.

Note: for the seccomp patch, it would probably start in toolkit/xre/nsEmbedFunctions.cpp at XRE_InitChildProcess, as we may need a few more fds open and some syscalls to be run before seccomp filters are set.

While I like to setuid early on (before execve), it is also possible to setuid there *and* there is a distinction between process types at this stage as well.
(likewise it's possible to apply the seccomp filter before execve and those will be inherited via 'no new privs', but we probably wouldn't be able to make the filters as tight as this place)

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

D'oh!  I didn't see the updated patch.  Please re-request review from me if I don't r+ the first time :).

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 645149 [details] [diff] [review]
Same patch with requested changed

Looks good.  I'm about to attach a version for landing with a commit message (make sure you include those in the future), some whitespace fixes, and one minor addition: disable no-rights subprocesses by default, but enable them on gonk.  While it would warm my heart to enable them by default, we would break a slew of plugins and extensions on other platforms :/.

One day! :)

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Version for landing

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/201612a0b133

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Guillaume, it looks like this is your first gecko patch, so congratulations! :)

Let's get some more landed after this ;).

Comment 9

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

thanks for the tips and changes!
yeah i wasn't sure because of the r removed instead of r- :)

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oh, forgot to mention

https://hg.mozilla.org/integration/mozilla-inbound/rev/201612a0b133

Your first gecko backout, too! :)

I fixed up the patch locally, will try to reland tonight.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a9e8a62d7a71

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a9e8a62d7a71