Last Comment Bug 776647 - Drop to no-rights user after fork()
: Drop to no-rights user after fork()
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: IPC (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla17
Assigned To: Guillaume Destuynder [:kang]
:
Mentors:
Depends on:
Blocks: 746280 776840
  Show dependency treegraph
 
Reported: 2012-07-23 12:29 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-07-27 10:25 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Setuid support (1.71 KB, patch)
2012-07-23 14:23 PDT, Guillaume Destuynder [:kang]
no flags Details | Diff | Review
Same patch with requested changed (4.81 KB, patch)
2012-07-23 17:47 PDT, Guillaume Destuynder [:kang]
cjones.bugs: review+
Details | Diff | Review
Version for landing (8.02 KB, patch)
2012-07-26 21:41 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-23 12:29:04 PDT
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] 2012-07-23 14:23:53 PDT
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.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-23 14:59:11 PDT
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] 2012-07-23 17:47:24 PDT
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)
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-26 20:47:06 PDT
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 2012-07-26 21:41:10 PDT
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 2012-07-26 21:41:45 PDT
Created attachment 646464 [details] [diff] [review]
Version for landing
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-26 21:43:20 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/201612a0b133
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-26 21:46:22 PDT
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] 2012-07-26 22:47:39 PDT
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 2012-07-27 00:34:20 PDT
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 2012-07-27 01:34:44 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/a9e8a62d7a71
Comment 12 Ed Morley [:emorley] 2012-07-27 08:13:55 PDT
https://hg.mozilla.org/mozilla-central/rev/a9e8a62d7a71

Note You need to log in before you can comment on or make changes to this bug.