Drop to no-rights user after fork()

RESOLVED FIXED in mozilla17

Status

()

Core
IPC
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cjones, Assigned: kang)

Tracking

(Blocks: 1 bug)

Trunk
mozilla17
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(3 attachments)

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.
Created attachment 645071 [details] [diff] [review]
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.
Attachment #645071 - Flags: review?(jones.chris.g)
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.
Attachment #645071 - Flags: review?(jones.chris.g)
Created attachment 645149 [details] [diff] [review]
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)
Blocks: 776840
D'oh!  I didn't see the updated patch.  Please re-request review from me if I don't r+ the first time :).
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! :)
Attachment #645149 - Flags: review+
Created attachment 646464 [details] [diff] [review]
Version for landing
https://hg.mozilla.org/integration/mozilla-inbound/rev/201612a0b133
Guillaume, it looks like this is your first gecko patch, so congratulations! :)

Let's get some more landed after this ;).
thanks for the tips and changes!
yeah i wasn't sure because of the r removed instead of r- :)
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.
http://hg.mozilla.org/integration/mozilla-inbound/rev/a9e8a62d7a71
https://hg.mozilla.org/mozilla-central/rev/a9e8a62d7a71
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
blocking-basecamp: --- → +
You need to log in before you can comment on or make changes to this bug.