I'm filing this as private because it hints at a bug we may not be able to fix without a kernel patch. We are currently vulnerable to ptrace attacks. The reason is - we don't have a sandboxing mechanism in place to block the ptrace syscall - app processes run with the same uid - prctl(DUMPABLE) defaults to "true" in linux This shouldn't be an issue; we just prctl(SET_DUMPABLE, 0) in the parent or after forking and move on, right? Nope! Linux happily resets dumpable state across exec(). We have two approaches to fix this - patch the kernel to prevent ptrace() except for tasks with CAP_SYS_PTRACE. This is an almost trivial patch that we should be able to get included in the "b2g patch series" pretty soon. - fix in gecko. We need to inherited privileges past fork+exec, prctl(SET_DUMPABLE, 0), then drop rights.

There's a third fix too - patch the kernel to either inherit the "dumpable" setting across exec() or to default it to false All three of these fixes are complementary.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #0) > - fix in gecko. We need to inherited privileges past fork+exec, > prctl(SET_DUMPABLE, 0), then drop rights. Ironically, this is orthogonally required for a startup optimization we've been considering.

Forgot to mention, STR (1) Apply patch, rebuild (2) $ adb shell # stop b2g # /system/bin/b2g.sh (3) Open dialer app (4) Open browser app If this is printed error: No such file or directory (2): PWNED!!! bad news bears.

Guillaume, let me know if you're comfortable reviewing this. Sadly I can't test it because I don't have a kernel tree around that I can build for device :/. May need to ask for help on QA.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #5) > Created attachment 691136 [details] [diff] [review] > Only allow ptrace for CAP_SYS_PTRACE tasks > > Guillaume, let me know if you're comfortable reviewing this. > > Sadly I can't test it because I don't have a kernel tree around that I can > build for device :/. May need to ask for help on QA. I've seen the patch... and I don't like it. Nevermind this meaning we have to have a 'special' kernel instead of the basic kernel (or do you intend to push this upstream to the mainstream Linux kernel?), you're basically completely removing the ability for same user processes to be profiled/modified

Ugh, sorry, saved before I wanted to. As I was saying, you're completely removing the same user process profiling, unless the user has the capability (which allow to modify/profile ANY process not just his own). The problem is that there are legit uses for that... and I don't know if any of the system processes that we're running on the phone use actually use that for something. Of all the possible changes, modifying the Linux kernel should be the last one IMO. My personal list would go: * Fix Gecko so it uses a different uid per process. This is *our* problem after all, no need to put it on Linux's roof. * Fix Gecko so it disables dumpable on startup (I don't know why you say we need to inherit privileges... AFAIK prctl cannot fail with EPERM for SET_DUMPABLE (at least according to the documentation, and to the code I've seen). This might open the window to a race condition though (try to open a new process AND inject it *before* it sets the SET_DUMPABLE). Hmm maybe this was why you wanted to keep it privileged until set_dumpable is set? But I don't know if resetting the uid will reset that flag also. * Whatever we think without touching the kernel * Still don't want to touch the kernel *... * ok, so we didn't think of anything else and all the previous were unfeasible, let's touch the kernel.

Sorry, I was travelling to France. Haven't slept yet :( I agree with Antonio here, and we can actually use different UIDs per content-process. However, it means we have to add the logic to maintain a list of uids that are in use (with proper locking), so that we can reuse uids when the apps die/exit (else we'll run out eventually). That's probably the right way to go. Also, does this patch means we have access to the kernel sources? In case that case we could also have seccomp/selinux/whatever that can enforce this and other system calls access in the future.

Guys, we need to have two discussions, one looking at the long term and one at the short term (releasing a v1 product). In the long term, we're trying to build a security model for gecko that applies across OSes as much as possible. "Applications" in gecko and b2g are abstracted above the underlying OS; we already have an application-level sandbox in place. For an OS-level sandbox for defense in depth, all we need is the lowest common denominator. It's possible to build a no-rights, unprivileged sandbox (mostly) on all platforms we care about. Not all platforms we care about have the concept of uids or other POSIX-y things. By decoupling the gecko and OS sandboxing, we're able to focus each on its strengths with minimal wasted code. In the short term, we're months after feature freeze and need to ship a product. This bug is a critical blocker and we need the quickest correct fix. Even if it made strategic technical sense to build on the POSIX model for the needed check here, it's just not feasible. I'll reply to individual comments separately.

(In reply to Antonio Manuel Amaya Calvo from comment #8) > As I was saying, you're completely removing the same user process profiling, > unless the user has the capability (which allow to modify/profile ANY > process not just his own). The problem is that there are legit uses for > that... and I don't know if any of the system processes that we're running > on the phone use actually use that for something. There are no use cases for that in b2g. > Of all the possible changes, modifying the Linux kernel should be the last > one IMO. I agree, but we're already patching kernel bugs that bite b2g. Sometimes there's no workaround in userspace. > * Fix Gecko so it disables dumpable on startup This doesn't work, as I partially explained in comment 0. Since linux doesn't carry dumpable state across exec(), we can't correctly disable dumping. After the exec(), a compromised process could attach to the new app before it has a chance to prctl. > * Whatever we think without touching the kernel > * Still don't want to touch the kernel I understand your motivation and I agree. However, the point of owning the entire stack is that we *can* make these changes when needed.

(In reply to Guillaume Destuynder [:kang] from comment #9) > Also, does this patch means we have access to the kernel sources? In case > that case we could also have seccomp/selinux/whatever that can enforce this > and other system calls access in the future. We can apply patches, yes. As we discussed elsewhere, seccomp2 is absolutely what we want to use post v1. Unfortunately, for v1 our downstream partners judged it to be too risky because it affects system libraries we have to use.

Comment on attachment 691136 [details] [diff] [review] Only allow ptrace for CAP_SYS_PTRACE tasks Re-requesting review based on above discussion.

Comment on attachment 691136 [details] [diff] [review] Only allow ptrace for CAP_SYS_PTRACE tasks Review of attachment 691136 [details] [diff] [review]: ----------------------------------------------------------------- not all OSes may have ptrace() either, so unfortunately part of the implementation of the sandbox is OS specific. We can however use (in the future, not now) a pluggable sandbox and have adequate code depending on the platform (just like OpenSSH does, in fact) For a quick hack, the patch should work fine. (user can't ptrace own uid, unless he has ptrace cap)

(In reply to Guillaume Destuynder [:kang] from comment #14) > Comment on attachment 691136 [details] [diff] [review] > Only allow ptrace for CAP_SYS_PTRACE tasks > > Review of attachment 691136 [details] [diff] [review]: > ----------------------------------------------------------------- > > not all OSes may have ptrace() either, so unfortunately part of the > implementation of the sandbox is OS specific. > We can however use (in the future, not now) a pluggable sandbox and have > adequate code depending on the platform (just like OpenSSH does, in fact) > > For a quick hack, the patch should work fine. (user can't ptrace own uid, > unless he has ptrace cap) That doesn't mean only that user can't ptrace his own uid unless he has ptrace cap. It means he either can ptrace *all* process or he can't ptrace any. And, once again, this change affects everything on the OS, not just Gecko. Do you know if the proprietary applications/drivers/libraries use or need this? It is after all a part of the standard Linux behavior that we're modifying. This is a quick hack... but it's also the most invasive one.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #10) > Guys, we need to have two discussions, one looking at the long term and one > at the short term (releasing a v1 product). > > In the long term, we're trying to build a security model for gecko that > applies across OSes as much as possible. "Applications" in gecko and b2g > are abstracted above the underlying OS; we already have an application-level > sandbox in place. For an OS-level sandbox for defense in depth, all we need > is the lowest common denominator. It's possible to build a no-rights, > unprivileged sandbox (mostly) on all platforms we care about. Not all > platforms we care about have the concept of uids or other POSIX-y things. I don't get this, sorry. The sandbox/uid code is platform specific, isn't it? And you don't actually have to touch the application model to add application specific uids. > In the short term, we're months after feature freeze and need to ship a > product. This bug is a critical blocker and we need the quickest correct > fix. Even if it made strategic technical sense to build on the POSIX model > for the needed check here, it's just not feasible. Fair enough.

(In reply to Guillaume Destuynder [:kang] from comment #14) > Comment on attachment 691136 [details] [diff] [review] > Only allow ptrace for CAP_SYS_PTRACE tasks > > Review of attachment 691136 [details] [diff] [review]: > ----------------------------------------------------------------- > > not all OSes may have ptrace() either, so unfortunately part of the > implementation of the sandbox is OS specific. The OS-level sandbox is entirely OS specific. I'm not sure I follow this comment.

(In reply to Antonio Manuel Amaya Calvo from comment #15) > Do you know if the proprietary applications/drivers/libraries use or > need this? I asked, they're not affected.

(In reply to Antonio Manuel Amaya Calvo from comment #16) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #10) > > Guys, we need to have two discussions, one looking at the long term and one > > at the short term (releasing a v1 product). > > > > In the long term, we're trying to build a security model for gecko that > > applies across OSes as much as possible. "Applications" in gecko and b2g > > are abstracted above the underlying OS; we already have an application-level > > sandbox in place. For an OS-level sandbox for defense in depth, all we need > > is the lowest common denominator. It's possible to build a no-rights, > > unprivileged sandbox (mostly) on all platforms we care about. Not all > > platforms we care about have the concept of uids or other POSIX-y things. > > I don't get this, sorry. The sandbox/uid code is platform specific, isn't > it? And you don't actually have to touch the application model to add > application specific uids. It's platform specific, but that platform specific code would have to live in *gecko*. My point is, that's what I'm trying to avoid. The OS code should live in the OS as much as possible. Tying uids to web apps would require major gecko changes. A seccomp2 sandbox, for example, would almost entirely live in the OS. Some gecko changes are needed to be compatible with that, but the changes are mostly cross-platform.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #11) > (In reply to Antonio Manuel Amaya Calvo from comment #8) > > As I was saying, you're completely removing the same user process profiling, > > unless the user has the capability (which allow to modify/profile ANY > > process not just his own). The problem is that there are legit uses for > > that... and I don't know if any of the system processes that we're running > > on the phone use actually use that for something. > > There are no use cases for that in b2g. Are you sure about that? Even taking into account the proprietary applications/libraries/drivers we have to include, some of them which we might haven't even seen yet (since the hardware partners are still under discussion)? > > > Of all the possible changes, modifying the Linux kernel should be the last > > one IMO. > > I agree, but we're already patching kernel bugs that bite b2g. Sometimes > there's no workaround in userspace. > > > * Fix Gecko so it disables dumpable on startup > > This doesn't work, as I partially explained in comment 0. Since linux > doesn't carry dumpable state across exec(), we can't correctly disable > dumping. After the exec(), a compromised process could attach to the new > app before it has a chance to prctl. Yeah, that was the race condition I was referring to. But... I think that something like: child_process: main() { ... // I'm running as root here... so I'm not vulnerable. prctl(SET_DUMPABLE, 0); setuid(app_0); //Do whatever we were doing already } Should work unless setuid resets the SET_DUMPABLE flag which if it does isn't documented (haven't had time to check the code, can do if needed). This is also a quick hack and one that *doesn't* impact the rest of the system, just the child processes. > > I understand your motivation and I agree. However, the point of owning the > entire stack is that we *can* make these changes when needed. Yeah, only I thought we were taking the kernel from Android/Linux as is. Touching it means opening the 'what happens when the next Linux/Android kernel patch is released' worm can.

(In reply to Antonio Manuel Amaya Calvo from comment #20) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #11) > > (In reply to Antonio Manuel Amaya Calvo from comment #8) > > > As I was saying, you're completely removing the same user process profiling, > > > unless the user has the capability (which allow to modify/profile ANY > > > process not just his own). The problem is that there are legit uses for > > > that... and I don't know if any of the system processes that we're running > > > on the phone use actually use that for something. > > > > There are no use cases for that in b2g. > > Are you sure about that? Even taking into account the proprietary > applications/libraries/drivers we have to include, some of them which we > might haven't even seen yet (since the hardware partners are still under > discussion)? > Yes, they've been asked. > > > > > Of all the possible changes, modifying the Linux kernel should be the last > > > one IMO. > > > > I agree, but we're already patching kernel bugs that bite b2g. Sometimes > > there's no workaround in userspace. > > > > > * Fix Gecko so it disables dumpable on startup > > > > This doesn't work, as I partially explained in comment 0. Since linux > > doesn't carry dumpable state across exec(), we can't correctly disable > > dumping. After the exec(), a compromised process could attach to the new > > app before it has a chance to prctl. > > Yeah, that was the race condition I was referring to. But... I think that > something like: > > child_process: > main() { > ... > // I'm running as root here... so I'm not vulnerable. > prctl(SET_DUMPABLE, 0); > setuid(app_0); > //Do whatever we were doing already > } This is what I described in comment 0. That's rather unpleasant security wise and is another invasive gecko change.

Hey m1, can we get this patch added the chainsaw queue?

(The second one, not the first one ;) .)

(In reply to Chris Jones [:cjones] [:warhammer] from comment #21) > (In reply to Antonio Manuel Amaya Calvo from comment #20) > > Yeah, that was the race condition I was referring to. But... I think that > > something like: > > > > child_process: > > main() { > > ... > > // I'm running as root here... so I'm not vulnerable. > > prctl(SET_DUMPABLE, 0); > > setuid(app_0); > > //Do whatever we were doing already > > } > > This is what I described in comment 0. That's rather unpleasant security > wise and is another invasive gecko change. Well, so long as the SET_DUMPABLE/setuid is the first thing you do upon process startup I don't really see what the security problem is... and it's two code lines on an #ifdef PTRACE_EXISTS (or whatever). As invasive goes... this doesn't even make a blip on my radar compared to changing the Linux kernel ;)

(In reply to Antonio Manuel Amaya Calvo from comment #24) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #21) > > (In reply to Antonio Manuel Amaya Calvo from comment #20) > > > Yeah, that was the race condition I was referring to. But... I think that > > > something like: > > > > > > child_process: > > > main() { > > > ... > > > // I'm running as root here... so I'm not vulnerable. > > > prctl(SET_DUMPABLE, 0); > > > setuid(app_0); > > > //Do whatever we were doing already > > > } > > > > This is what I described in comment 0. That's rather unpleasant security > > wise and is another invasive gecko change. > > Well, so long as the SET_DUMPABLE/setuid is the first thing you do upon > process startup I don't really see what the security problem is... and it's > two code lines on an #ifdef PTRACE_EXISTS (or whatever). We don't know what uid we're going to use at this point in startup. > As invasive goes... this doesn't even make a blip on my radar compared to > changing the Linux kernel ;) I don't quite follow, sorry. I thought we already had this discussion.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #25) > (In reply to Antonio Manuel Amaya Calvo from comment #24) > > (In reply to Chris Jones [:cjones] [:warhammer] from comment #21) > > > (In reply to Antonio Manuel Amaya Calvo from comment #20) > > > > Yeah, that was the race condition I was referring to. But... I think that > > > > something like: > > > > > > > > child_process: > > > > main() { > > > > ... > > > > // I'm running as root here... so I'm not vulnerable. > > > > prctl(SET_DUMPABLE, 0); > > > > setuid(app_0); > > > > //Do whatever we were doing already > > > > } > > > > > > This is what I described in comment 0. That's rather unpleasant security > > > wise and is another invasive gecko change. > > > > Well, so long as the SET_DUMPABLE/setuid is the first thing you do upon > > process startup I don't really see what the security problem is... and it's > > two code lines on an #ifdef PTRACE_EXISTS (or whatever). > > We don't know what uid we're going to use at this point in startup. But that has a pretty easy solution. On the parent, change if (setuid(CHILD_UNPRIVILEGED_UID) != 0) { with if (seteuid(CHILD_UNPRIVILEGED_UID) != 0) { on [1]. And on the child process (haven't found where the main for that is, sorry) do: main() { #ifdef WHATEVER_WE_USE prctl(SET_DUMPABLE, 0); setuid(geteuid()); #endif ... We're still protected against race conditions (since the real uid is root), there's no added security risk that I can see, and it's only two lines. We don't need any further modification of the child process, as far as I can see. BTW... I think I'm missing something here. How can we not know at runtime something that's a define? [2] > > > As invasive goes... this doesn't even make a blip on my radar compared to > > changing the Linux kernel ;) > > I don't quite follow, sorry. I thought we already had this discussion. Ah that was a joke, which I guess only I got, sorry. I just found it funny when you said *that* was invasive after modifying the kernel :). As per the discussion thing... I was still trying to find something that was more amenable than changing the underlying OS kernel to fix an app problem. I'm done now :). [1] https://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/process_util_linux.cc#239 [2] https://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/process_util_linux.cc#29

(In reply to Chris Jones [:cjones] [:warhammer] from comment #0) > - app processes run with the same uid What is the LoE/risk of changing the |setuid(AID_APP);| to |setuid(AID_APP+i);| where i is some small >= 0 number that is unique from all other currently running apps?

BTW. ptrace.c patch seems fine w/ a minor warning fix. Wondering if we can avoid it though still with a minor? Gecko change like comment 27.

(In reply to Antonio Manuel Amaya Calvo from comment #26) > But that has a pretty easy solution. On the parent, change > > if (setuid(CHILD_UNPRIVILEGED_UID) != 0) { > > with > > if (seteuid(CHILD_UNPRIVILEGED_UID) != 0) { > > on [1]. > > And on the child process (haven't found where the main for that is, sorry) > do: > > main() { > #ifdef WHATEVER_WE_USE > prctl(SET_DUMPABLE, 0); > setuid(geteuid()); > #endif > ... So my mind has been stuck on what we've proposed in bug 786631 (comment 2). To implement that, we would need to launch all subprocesses with inherited privileges and then choose which uid to switch to when loading the app. But you're proposing something much simpler than that. This is relatively easy to implement and I agree is correct.

(In reply to Michael Vines [:m1] from comment #27) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #0) > > - app processes run with the same uid > > What is the LoE/risk of changing the |setuid(AID_APP);| to > |setuid(AID_APP+i);| where i is some small >= 0 number that is unique from > all other currently running apps? This is a fair amount of effort in relatively sensitive code. The problem is that our process management and uid-management logic live in different levels. In gecko it would be a bit delicate to make that approach bulletproof. There's also a bit of unknown risk at the gonk layer, but I'm not too concerned about that.

(In reply to Michael Vines [:m1] from comment #28) > BTW. ptrace.c patch seems fine w/ a minor warning fix. Wondering if we can > avoid it though still with a minor? Gecko change like comment 27. Comment 26 is about as minimal of a fix as we can do. That would fix this bug. But TBH, like I said in comment 1 these changes are complementary. I would really still prefer to take the kernel patch, TBH.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #31) > But TBH, like I said in comment 1 these changes are complementary. I would > really still prefer to take the kernel patch, TBH. I meant, in addition to a gecko fix.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #31) > But TBH, like I said in comment 1 these changes are complementary. I would > really still prefer to take the kernel patch, TBH. I would definitely strongly prefer to not deviate from standard *core* Linux kernel behavior to compensate for a Gecko bug. But I think that view is largely shared here so no need to ramble on. > There's also a bit of unknown risk at the gonk layer, but I'm not too > concerned about that. Android does this so we should have pretty low risk here. I can absolutely land the kernel patch in our patches tree if we determine that a Gecko-only solution is incompatible with our v1 schedule. Not sure we are quite there yet though.

Nope, a gecko-only fix is eminently doable for v1. If there are no concerns on the gonk level, then we can synthesize comment 26 and comment 27 by doing ~main() { prctl(SET_DUMPABLE, 0); uid_t uid = geteuid(); if (uid == CHILD_UNPRIVILEGED_UID) { uid += getpid(); } setuid(uid); and can disable ptracing child processes in two ways, and avoid the hairy uid management code referred to in comment 30.

error: Operation not permitted (1): Couldn't attach to Communications

Comment on attachment 692173 Whups!

Anyone object to opening this bug?

(In reply to Chris Jones [:cjones] [:warhammer] from comment #38) > Anyone object to opening this bug? Do you mean Opening to the public? If that's what you're asking then I for one don't understand why is it closed in the first place, considering there's no still a stable/commercial/for general use release of B2G. On an unrelated note, I think you forgot an 'e' on the seteuid calls (they're setuid call on your patch still) ;). I would copy the code here but my phone is temperamental this morning and doesn't want to copy.

Comment on attachment 692174 [details] [diff] [review] Fix uid += getpid(); I'm not too sure about that one. In theory we're ok because pid_max defaults to 32768 and AID_APP or even the other default (65335) + 32768 is way below int max. If we ever reach int max for some reason tho, setuid(int max +1) is the same as setuid(0). I'd probably go with a if (uid < CHILD_UNPRIVILEGED_UID) _exit(127) just to be safe. (even thus it may look paranoid) Also, less important stuff: Error messages mention seteuid/setegid but its calling setuid/setgid I like it otherwise, since we don't have to really manage uids that way, and we don't rely on the child process to properly setuid. Also, that unprivileged can still work with AID_SYSTEM (somewhat privileged in android) is nice, instead of uid 0.

Ugh, gmail seems not to be sending me notifications about new comments here, or delaying them a long time. Sorry for lag.

(In reply to Antonio Manuel Amaya Calvo from comment #39) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #38) > > Anyone object to opening this bug? > > Do you mean Opening to the public? If that's what you're asking then I for > one don't understand why is it closed in the first place, considering > there's no still a stable/commercial/for general use release of B2G. Yes. Quoting comment 0 > I'm filing this as private because it hints at a bug we may not be able to > fix without a kernel patch. We reviewed that theoretical bug and it turns out we're OK. > On an unrelated note, I think you forgot an 'e' on the seteuid calls > (they're setuid call on your patch still) ;). I would copy the code here but > my phone is temperamental this morning and doesn't want to copy. This patch ends up just implementing the uid-per-running-app-process approach fix. I ran into bizarro errors trying to implement the additional disable-dumpable part.

(In reply to Guillaume Destuynder [:kang] from comment #40) > Comment on attachment 692174 [details] [diff] [review] > Fix > > uid += getpid(); > > I'm not too sure about that one. In theory we're ok because pid_max defaults > to 32768 and AID_APP or even the other default (65335) + 32768 is way below > int max. > If we ever reach int max for some reason tho, setuid(int max +1) is the same > as setuid(0). Good catch. I'll an aborting check there. > I'd probably go with a if (uid < CHILD_UNPRIVILEGED_UID) _exit(127) just to > be safe. (even thus it may look paranoid) That's not quite enough because |uid| could end up being the uid of an already running app process. > Also, less important stuff: > Error messages mention seteuid/setegid but its calling setuid/setgid Will fix.

Will deal with INHERIT stragglers in a followup.

Comment on attachment 692225 [details] [diff] [review] Fix, v2 Looks ok. Note that on non-MOZ_WIDGET_GONK we'd still get the same uid for all content processes, if we're not running anything that doesn't get this define set, it's fine. If it's a problem, the alternatives to find an uid, if its a problem would be: - get a unique uid as app install time and use it every time (like android) - check for the next unused uid in the process list (need to make sure there's no race conditions from the b2g process) - keep a list of uids - just use uid 10000+ (like aid_app) and do the same thing

Comment on attachment 692225 [details] [diff] [review] Fix, v2 (In reply to Guillaume Destuynder [:kang] from comment #45) > Comment on attachment 692225 [details] [diff] [review] > Fix, v2 > > Looks ok. > Note that on non-MOZ_WIDGET_GONK we'd still get the same uid for all content > processes, if we're not running anything that doesn't get this define set, > it's fine. That's intentional --- we don't use that code anywhere else yet, and it's not clear to me how it should work in general. That is, we can't provide the guarantee we need anywhere but gonk, so I think we should punt elsewhere.

https://hg.mozilla.org/releases/mozilla-aurora/rev/638ba4108a3b https://hg.mozilla.org/releases/mozilla-b2g18/rev/71b4409fbf12