New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Dec 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug-Security

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment

Domui process can be ptraced from a compromised renderer leading to sandbox escape, take 2

Project Member Reported by jln@chromium.org, Apr 26 2012

Issue description

It turns out that "https://code.google.com/p/chromium/issues/detail?id=74763" is not fixed anymore.

We made the check more tight:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=da48524eb20662618854bb3df2db01fc65f3070c

But it was then relaxed:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=243b422af9ea9af4ead07a8ad54c90d4f9b6081a

Unfortunately, breakpad trusts SI_USER:

http://code.google.com/searchframe#I3UcDWKuRYs/trunk/src/client/linux/handler/exception_handler.cc&l=338

I didn't notice at the time the check was relaxed that kill() uses SI_USER, not SI_TKILL and that it would be an issue.

There are two ways to fix this:

1. a new kernel patch to prevent SI_USER spoofing
2. a breakpad patch where we only trust TKILL signals (i.e. signals sent by either tkill or tgkill)

Since tgkill doesn't allow to send a signal to a whole thread group, I don't think that (2) can work.

I'll look into (1)

https://lkml.org/lkml/2011/3/28/482
 
What's the process relationship between domui and renderer? Both Chrome OS and Ubuntu run with Yama enabled, so ptrace between cousins should not be possible.

Comment 2 by jln@chromium.org, Apr 26 2012

Yeah Yama should mitigate this indeed:
http://code.google.com/searchframe#I3UcDWKuRYs/trunk/src/client/linux/handler/exception_handler.cc&l=411

So it appears only the child we just created can ptrace us.

I'll send a patch upstream anyway (at the very least the comment that kill/tgkill are not spoofable is not valid any more), but it looks like it's not a big deal on Ubuntu / ChromeOS thanks to your patch :)
Yama was in 10.04 LTS right?
If so, we're in reasonable shape. Ubuntu dominates our Linux install base from what I'm aware.
On Sun, Apr 29, 2012 at 12:21 AM,  <scarybeasts@gmail.com> wrote:

Unfortunately, no, it was introduced in 10.10:
https://wiki.ubuntu.com/Security/Features#ptrace

-- 
Kees Cook
Chrome OS Security
Owner: jln@chromium.org
Ah well. At least we closed a hole so that a compromised renderer can't easily cause the launch of a WebUI process.

Comment 6 by cdn@chromium.org, May 2 2012

Labels: -Area-Undefined Area-Internals SecSeverity-High Mstone-19 SecImpacts-Stable SecImpacts-Beta
Status: Assigned

Comment 7 by cdn@chromium.org, May 4 2012

Labels: -SecSeverity-High SecSeverity-Medium
Per offline discussion
What's the status here, Julien?

Comment 9 by jln@chromium.org, May 30 2012

Not done.

Yama should mitigates this, but I wanted to ask Kees about

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=bf06189e4d14641c0148bea16e9dd24943862215

PR_SET_PTRACER_ANY is a new flag to completely disable Yama and it was apparently added for Chrome.

Kees, I didn't see PR_SET_PTRACER_ANY used, is it anywhere ?

I still want to send an upstream patch to fix the spoofing issue, it's on my TODO list.

Julien
Is there anything useful we could do in breakpad?

Comment 11 by jln@chromium.org, May 30 2012

As I mention above, maybe we could limit ourselves to trusting TKILL only, but I'm not sure it can work.

Comment 13 by jln@chromium.org, May 31 2012

Cc: thestig@chromium.org
How unfortunate that PR_SET_PTRACER_ANY wasn't #define-ed, grep failed me.

It looks that this completely disables Yama protection between renderers then, which is precisely where we need it.

I'll need to look at this in details, but it seems likely that you can indeed SIGSEGV another renderer to get its privileges. Which would be a full bypass of the sandbox on Linux.

Thestig: shouldn't sys_prctl(PR_SET_PTRACER, -1); be done in HandleSignal() in exception_handler.cc. At least we would be protected by the signal origin check (which is currently broken, cf. this bug).
SIGH!

I don't think the code used to do PR_SET_PTRACER_ANY. AFAIR, it used to be more selective. Can you locate the CL that regressed this?

Comment 15 by jln@chromium.org, May 31 2012

http://codereview.chromium.org/9212033 it was landed around the same time when Kees landed the Linux patch.
What a headache. I was thinking of the other usage of PR_SET_PTRACER (in src/breakpad/src/client/linux/handler/exception_handler.cc -- presumably that one is for handling dumps for non-sandboxed processes such as the browser?)
Ok, now I'm confused. http://codereview.chromium.org/9212033 is relatively recent (4 months) but ptrace restrictions have been in Ubuntu since 10.10. I'm pretty sure crash dumps for renderers were working before that time frame in Ubuntu 10.x and 11.x though?

The associated bug (https://code.google.com/p/chromium/issues/detail?id=46368) doesn't seem to note any definitive breakage that was fixed by the CL, either -- other than the ability to strace things as non-root. We shouldn't lower security in production builds for developer use cases if that's the only reason.

The usual process relationship is browser -> zygote -> renderer

Kees, can you state whether by default, a child-of-a-child can be traced? ( Bug 46368  says "direct children", I'm interested about indirect).

Also, my understanding was that when a renderer crashes, the browser process itself attaches. The browser would only attach to a crashed process from a clone() when the browser itself crashes.

I could of course be very wrong.

One interesting option to fix this for Ubuntu 12.04 is to land a renderer seccomp filter that specifically denies ptrace (and newer variant process_vm_writev etc) but allows everything else. Safe in terms of breakage.
Cc: mkrebs@chromium.org
Ok, https://code.google.com/p/chromium-os/issues/detail?id=25087#c9 _does_ note definite renderer crash reporting breakage.

Kees, any idea why the browser was unable to ptrace() the renderer in that case? The process tree would seem to allow it.
(Also adding mkrebs@ from that bug)

I thought it used to work when I last tested it.
I guess I don't understand what the process tree for Chrome looks like.

When I originally wrote Yama, and there were breakpad problems with
Chrome, something was changed to use PR_SET_PTRACER during a crash (I
don't know what crashes this was now catching).

Once I enabled Yama in Chrome OS, it was noticed that renderer crashes
were no longer getting reported. The reason for this is that it was
running inside the setuid sandbox, with a pid namespace, so it could
no longer declare who was going to ptrace it during a crash. At this
point, PTRACER_ANY was used to allow things outside the sandbox to
ptrace it.

The default relationships for ptrace restriction is that of ancestor
to descendant, i.e. "An ancestor can ptrace any of its descendants."
In the declared relationship (PR_SET_PTRACER), the declared pid and
any of its descendants can ptrace the target. i.e. calling
PR_SET_PTRACER with pid 1 means everyone in your pid namespace can now
ptrace you. Or, in the case of things like Wine, calling
PR_SET_PTRACER on the wine server pid means that all processes started
by wine can ptrace you. Or, in the case of KDE, the gdb that gets
started by the declared crash handler can ptrace the target, etc.

So, questions:

- are pid namespaces used on Ubuntu? If so, this
lack-of-crash-reported bug has existed there since the introduction of
Yama.
- are renderers in separate namespaces? If so, they can't ptrace each
other because they can't describe the other renderer's pid.

-Kees

Comment 20 by jln@chromium.org, May 31 2012

Yes, they are used. It's how the setuid sandbox works. We have browser
| zygote-init - zygote (| denotes PID namespace change). Before that
we had browser | setuid-sandbox-helper-init - zygote. And before that
browser | zygote

In any case there was always a direct relationship from the browser to
the zygote, so the browser could always ptrace any renderer forked by
the zygote.

No, that's what makes everything so hard. We rely on the dumpable flag
to protect every renderer from each other (and it's very important
because many renderers allow a full sandbox escape!).

Unfortunately they can still send signals to each others. So renderer
A can SIGSEGV renderer B, who will set its dumpable flag to get
ptrace-ed. To prevent that we try to authenticate where the signals
come from (c.f. the kernel patch mentioned above).

But that's band-aid, and currently doesn't even work anymore (the
reason why this bug was started originally) and we rely on Yama for
true inner peace.

And currently we don't understand why PR_SET_PTRACER was needed at all
(outside of the case where you clone a child to ptrace you, but that's
not cross PID namespaces).

My suspicion is that we don't really ptrace() from the browser anymore
and that we fork() some helper process to do it.

Julien

Comment 22 by jln@chromium.org, May 31 2012

Jorge: that code is for the self ptracing, which shouldn't happen in that case.

That code already had PTRACE_SET_TRACER for a long time with "child" as an argument.

What we don't understand is why the browser could not ptrace() the renderer without the need to disable Yama since there should still be a relationship.
Does the browser directly ptrace the renderer, or does it spawn
another process (a cousin) to do it? If the latter, then there is no
descendant relationship between tracer and renderer.

-Kees

Comment 24 by jln@chromium.org, May 31 2012

Yes, that's my suspicion as stated above, that it changed at some point and broke it.

I didn't have time to look into it yet, and I presume that thestig@ already knows.

Comment 25 Deleted

On my machines, in pstree, I see two separate branches of chrome processes:

1) init -> ... -> bash -> browser -> plugins
2) init -> chrome-sandbox -> zygote -> renderers

I think that might be what's breaking the ancestor relationship between the browser and the renderers.

Comment 27 by jln@chromium.org, May 31 2012

Cc: markus@chromium.org
You're right, how did I miss that. I've just checked it was not introduced by recent changes in this area, such as:
- https://chromiumcodereview.appspot.com/10389214
- https://chromiumcodereview.appspot.com/10417019

But it wasn't.

I'm working around that code anyway, so I'll look into where the source of the double fork() is. If we can fix that issue, hopefully we can revert the PTRACE_SET_TRACER change.
jln: re: "shouldn't sys_prctl(PR_SET_PTRACER, -1); be done in HandleSignal() in exception_handler.cc. At least we would be protected by the signal origin check (which is currently broken, cf. this bug)."

I believe NonBrowserCrashHandler() which contains the sys_prctl(PR_SET_PTRACER, -1) gets called after HandleSignal() gets called. Thus it's protected by whatever checks HandleSignal() does.
So crash reporting broke when chrome-sandbox got reparented to init instead of the browser? When did that happen?

Sounds like if we can repair the process tree to what it used to be, we can revert the CL that introduced the promiscuous ptracer call.
Not sure, we might have been reparenting for a long time. But with 10.04 and before, there was no ptrace protection, so it didn't matter whether chrome-sandbox was reparented or not. I think starting with 10.10, renderer crashes were (sometimes?) broken, e.g  bug 56730 . However, that particular bug did not get any traction, and for all I know, it might have been broken for a long time. All my machines were on 10.04 LTS, so did not get the itch to fix it.

OTOH, CrOS folks actually test crash reporting and when they noticed it was broken, we ended up putting in the PR_SET_PTRACER call.
For completeness sake, re: c#21/22/23, when we call the crash handler in:

http://code.google.com/searchframe#OAMlx_jo-ck/src/breakpad/src/client/linux/handler/exception_handler.cc&exact_package=chromium&q=src/breakpad/src/client/linux/handler/exception_handler.cc&type=cs&l=356

If the call to the handler succeeds, we don't call the GenerateDump() function that calls clone(). Fixing the reparenting should be enough.

Comment 32 by jln@chromium.org, May 31 2012

So, the re-parenting happens because when we launch zygote through the setuid sandbox, we try to reap the zombie of the setuid sandbox (the setuid sandbox needs to clone(), because we cannot unshare() the PID namespace) [1]

I'll try to fix it and try to figure out since when it happened. It's very possible that it happened a long time ago. For some reason people don't like seeing zombies :)

[1] FYI it also clone() a another time for the chroot() magic, but I made sure that the helper is our child.

Comment 33 by jln@chromium.org, Jun 1 2012

So, it turns out I was wrong in a small way: keeping the Zombie of the setuid sandbox around won't help, because a zombie cannot have children (they get re-parented).

So we need to keep the setuid sandbox around, which this CL does: https://chromiumcodereview.appspot.com/10447135

Handling the kZygoteIdFd properly is a major pain though.
I want something simple and mergeable for M20, so I used syscall filtering to deny ptrace:
https://chromiumcodereview.appspot.com/10454110/

That may even be a "get out of jail free" card if it turns out the correct solution (patching up the process tree structure) is too hairy to merge to M20.
Project Member

Comment 35 by bugdroid1@chromium.org, Jun 1 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=140080

------------------------------------------------------------------------
r140080 | cevans@chromium.org | Fri Jun 01 12:34:56 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/renderer_main_platform_delegate_linux.cc?r1=140080&r2=140079&pathrev=140080
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/common/sandbox_init_linux.cc?r1=140080&r2=140079&pathrev=140080
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/worker/worker_main.cc?r1=140080&r2=140079&pathrev=140080

Block ptrace (and ptrace-like) syscalls from the renderer and worker processs.

BUG= 125225 
Review URL: https://chromiumcodereview.appspot.com/10454110
------------------------------------------------------------------------
Project Member

Comment 36 by bugdroid1@chromium.org, Jun 1 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=140104

------------------------------------------------------------------------
r140104 | jln@chromium.org | Fri Jun 01 14:41:43 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/sandbox/linux/suid/sandbox.c?r1=140104&r2=140103&pathrev=140104
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/common/zygote_commands_linux.h?r1=140104&r2=140103&pathrev=140104

Don't fork Zygote as a background process

On Linux, with the setuid sandbox, the Zygote would become a background
process of sort because the setuid sandbox would exit.

The problem is that the Chrome process tree would be broken because the
Zygote would be reparented to init.

In turn, this could create issues with the browser not being able to ptrace()
the Zygote if certain kernel restrictions are in place (e.g. Yama).

BUG= 125225 
TEST=
NOTRY=true


Review URL: https://chromiumcodereview.appspot.com/10447135
------------------------------------------------------------------------
Labels: -Mstone-19 Mstone-20 Merge-Approved
Marking Merge-Approved, Mstone-20 so we know to merge at least one of the pieces.
Project Member

Comment 38 by bugdroid1@chromium.org, Jun 2 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=140137

------------------------------------------------------------------------
r140137 | thestig@chromium.org | Fri Jun 01 17:31:05 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/app/breakpad_linuxish.cc?r1=140137&r2=140136&pathrev=140137

Remove ptracer call now that the process tree is no longer broken.

BUG= 125225 
TEST=none

Review URL: https://chromiumcodereview.appspot.com/10488002
------------------------------------------------------------------------
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Status: FixUnreleased
Looks like we're in better shape on trunk; I'll take the TODO to merge the ptrace-deny patch to M20.
Once you merge these, you also need to merge the CL  bug 130786 .
Project Member

Comment 41 by bugdroid1@chromium.org, Jun 12 2012

Labels: merge-merged-1132
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=141714

------------------------------------------------------------------------
r141714 | cevans@chromium.org | Tue Jun 12 12:49:44 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/1132/src/content/worker/worker_main.cc?r1=141714&r2=141713&pathrev=141714
 M http://src.chromium.org/viewvc/chrome/branches/1132/src/content/renderer/renderer_main_platform_delegate_linux.cc?r1=141714&r2=141713&pathrev=141714
 M http://src.chromium.org/viewvc/chrome/branches/1132/src/content/common/sandbox_init_linux.cc?r1=141714&r2=141713&pathrev=141714

Merge 140080 - Block ptrace (and ptrace-like) syscalls from the renderer and worker processs.

BUG= 125225 
Review URL: https://chromiumcodereview.appspot.com/10454110

TBR=cevans@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10546130
------------------------------------------------------------------------
Labels: -Mstone-20 -Merge-Approved Mstone-21
Ok, confirmed not a single SIGSYS_Handler hit in the renderer with the deny-ptrace patch applied, so merged it to M20.

In terms of the rest of the fixes, there are a bit scary so they can roll into M21 and we'll announce the fix there.
Labels: CVE-2012-2846
Project Member

Comment 44 by bugdroid1@chromium.org, Oct 13 2012

Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.
Status: Fixed
Project Member

Comment 46 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Type-Security -Area-Internals -SecSeverity-Medium -Mstone-21 -SecImpacts-Stable -SecImpacts-Beta Security-Impact-Beta Security-Severity-Medium M-21 Cr-Internals Security-Impact-Stable Type-Bug-Security
Project Member

Comment 47 by bugdroid1@chromium.org, Mar 13 2013

Labels: Restrict-View-EditIssue
Project Member

Comment 48 by bugdroid1@chromium.org, Mar 14 2013

Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue

Comment 49 by jln@chromium.org, Mar 17 2013

Labels: -Restrict-View-SecurityNotify -Restrict-View-EditIssue
Project Member

Comment 50 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Impact-Stable Security_Impact-Stable
Project Member

Comment 51 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Severity-Medium Security_Severity-Medium
Project Member

Comment 52 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Impact-Beta Security_Impact-Beta
Project Member

Comment 53 by sheriffbot@chromium.org, Jun 14 2016

Labels: -security_impact-beta
Project Member

Comment 54 by sheriffbot@chromium.org, Oct 1 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 55 by sheriffbot@chromium.org, Oct 2 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: allpublic
Labels: CVE_description-submitted

Sign in to add a comment