New issue
Advanced search Search tips
Starred by 1 user
Status: Fixed
Owner:
Closed: Sep 21
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug-Security

Blocking:
issue 766253



Sign in to add a comment
Security: chronos to root with crash reporter and /tmp symlink
Project Member Reported by meacer@chromium.org, Sep 18 Back to list
Split from  bug 766253 :

"""
The crash handler for non-chrome processes copies files to /tmp/crash_reporter/<crashed pid>/ as root. user_collector.cc:130:

  static const char* const kProcFiles[] = {
    "auxv",
    "cmdline",
    "environ",
    "maps",
    "status"
  };
  for (std::string proc_file : kProcFiles) {
    if (!base::CopyFile(process_path.Append(proc_file),
                        container_dir.Append(proc_file))) {

Symlink /tmp/crash_reporter/<getpid()>/environ to /proc/sys/kernel/core_pattern, then crash. And then crash again to launch the command in core_pattern. Actually, this won't work because of protected_symlinks. Even root gets permission denied for non-root symlinks in sticky directories. But surprisingly, this check only seems to apply for a symlink in the last component of a path. So symlink the pid directory to outside the sticky /tmp. And from there, symlink environ to core_pattern. Exploit in crasher.c. There is also a noexec bypass, using bash and dd. See drop/yexec and tools/yesexec.cc.

"""

vapier: Sorry, another one. Can you please take a look? Thanks.
 
crasher.c
4.2 KB View Download
Blocking: 766253
Comment 2 Deleted
Comment 3 Deleted
Kees probably has thoughts on the protected_symlinks issue, if he's cc'd.
Cc: keescook@chromium.org
Good point, adding Kees.
Cc: semenzato@chromium.org
Some observations:

1. Symlinks strike a again. This time, not on stateful though. We should look into whether we can add a per-process flag that prevents symlink traversal entirely.

2. crash_reporter shouldn't be running with full privileges in general. Probably worthwhile to wrap it in minijail0.

3. crash_reporter should process crashes and other untrusted input in a sandboxed environment that doesn't have access to anything but its inputs and the output.

4. Nice noexec bypass. We should look into whether we can just kill /proc/self/mem access.

5. Regarding noexec, in a previous discussion with vapier@ we touched upon the fundamental issue that shell scripts are not subject to noexec and potential ways to fix that. Time to look at that again?

All these are a longer-term mitigations. I'll now look into crash_reporter some more to see what short-term fixes we can make.
More thoughts:

1. reducing crash_reporter privileges when handling a crash isn't trivial because it needs to pull information from the crashed process' /proc/$pid/ and thus generally requires root privilege.

2. Access to /proc/sys/kernel/core_pattern surprisingly only requires root privileges, but no capabilities.

3. One thing we can do is to minijail with /proc mounted as read-only which will cut off the core_pattern overwrite.

4. There's a TOCTOU in the exploit where /tmp/crash_reporter/$pid gets replaced by a symlink. crash_reporter attempts to recreate a clean directory, but allows an attacking process to swap the newly created one for a symlink. A simple fix for this would be to mount a fresh /tmp in a mount namespace, again via minijail.
Oh, one more thing I don't understand yet: /tmp/crash_reporter is root-accessible only in my tests, but the exploit seems to access it as chronos to stage the symlink? I must be missing something...
Regarding the /tmp/crash_reporter permissions: The explanation is probably that the directory gets created by crash_reporter with root-only permissions on the first crash, but if nothing crashed before the exploit runs, then a chronos-writable directory can be created before crash_reporter touches the directory.
#9 That is correct.
i can't think of any reason we need /tmp in the first place.  i'll switch it to /run, and have the init scripts initialize that at boot.

in the meantime i'll update crash-reporter to sanity check its /tmp dir as that's something with low impact.
Yeah, /tmp should not be used for this. (Or if it is, yes, it should sanity-check the paths, as you say.)

One of weaknesses of the protected symlink is the need for the _parent_ to be in +wt directory. So, yes, this is the remaining class of /tmp races: tools that build a tree in /tmp and work within that tree. (Which, again, is not the right way to do things...)
this CL should cut off the core bug:
  https://chromium-review.googlesource.com/673485

i've got a few follow up CLs after that, but will wait for this to land/cherry pick back before doing more.
Labels: Security_Severity-High M-62 Pri-1
Does it make sense to break this out into additional sub-bugs, given the comments in #6, #7, et c.?

Giving this PRi and Severity, just for completeness, but again, the main bug is the one that matters.
i'll take care of splitting out ideas into more bugs
Project Member Comment 17 by bugdroid1@chromium.org, Sep 21
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/9f95fe6adaf8b536d2857b22a0cefec69b9bba12

commit 9f95fe6adaf8b536d2857b22a0cefec69b9bba12
Author: Mike Frysinger <vapier@chromium.org>
Date: Thu Sep 21 01:43:01 2017

crash: start moving state dir initialization into C++ logic

We have various init code spread around (like the init script, and lazy
setup by various sub-collectors).  During early boot, or if no crashes
have yet been created, it can be confusing when these paths don't yet
exist, and autotests can mess them up.  So have the early --init logic
start handling all of this for us.

BUG= chromium:766275 
TEST=deleted paths and ran --init on system and checked result
TEST=precq passes

Change-Id: I67ab2370f9d2a94cfa4d18461bafd06896d10260
Reviewed-on: https://chromium-review.googlesource.com/673485
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Kees Cook <keescook@chromium.org>

[modify] https://crrev.com/9f95fe6adaf8b536d2857b22a0cefec69b9bba12/crash-reporter/user_collector_base.h
[modify] https://crrev.com/9f95fe6adaf8b536d2857b22a0cefec69b9bba12/crash-reporter/user_collector_base.cc
[modify] https://crrev.com/9f95fe6adaf8b536d2857b22a0cefec69b9bba12/crash-reporter/user_collector.cc

Labels: Merge-Request-62 Merge-Request-61
Project Member Comment 19 by sheriffbot@chromium.org, Sep 21
Labels: -Merge-Request-62 Merge-Review-62 Hotlist-Merge-Review
This bug requires manual review: M62 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member Comment 20 by sheriffbot@chromium.org, Sep 21
Status: Fixed
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-62 Merge-Approved-62
Approved for 62. 

We are really late for 61, is this safe to put directly into stable?
Project Member Comment 22 by sheriffbot@chromium.org, Sep 22
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member Comment 23 by bugdroid1@chromium.org, Sep 23
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/6941a97c1926bbc75aefaa6883baf4e5a8637b1d

commit 6941a97c1926bbc75aefaa6883baf4e5a8637b1d
Author: Mike Frysinger <vapier@chromium.org>
Date: Sat Sep 23 05:25:39 2017

crash-reporter: depend on minijail

BUG= chromium:766275 
TEST=crash_reporter still reports crashes successfully.

Change-Id: I5fa8d6b0496b7b75be83fa51187e5c04ed6978ad
Reviewed-on: https://chromium-review.googlesource.com/678474
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>

[modify] https://crrev.com/6941a97c1926bbc75aefaa6883baf4e5a8637b1d/chromeos-base/crash-reporter/crash-reporter-9999.ebuild

Project Member Comment 24 by sheriffbot@chromium.org, Sep 25
Cc: bhthompson@google.com
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member Comment 25 by bugdroid1@chromium.org, Sep 26
Labels: merge-merged-release-R62-9901.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/2faab961327f2e8cfc228324b5d1939653ba5361

commit 2faab961327f2e8cfc228324b5d1939653ba5361
Author: Mike Frysinger <vapier@chromium.org>
Date: Tue Sep 26 07:04:22 2017

crash: start moving state dir initialization into C++ logic

We have various init code spread around (like the init script, and lazy
setup by various sub-collectors).  During early boot, or if no crashes
have yet been created, it can be confusing when these paths don't yet
exist, and autotests can mess them up.  So have the early --init logic
start handling all of this for us.

BUG= chromium:766275 
TEST=deleted paths and ran --init on system and checked result
TEST=precq passes

Change-Id: I67ab2370f9d2a94cfa4d18461bafd06896d10260
(cherry picked from commit 9f95fe6adaf8b536d2857b22a0cefec69b9bba12)
Reviewed-on: https://chromium-review.googlesource.com/677025
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Commit-Queue: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/2faab961327f2e8cfc228324b5d1939653ba5361/crash-reporter/user_collector_base.h
[modify] https://crrev.com/2faab961327f2e8cfc228324b5d1939653ba5361/crash-reporter/user_collector_base.cc
[modify] https://crrev.com/2faab961327f2e8cfc228324b5d1939653ba5361/crash-reporter/user_collector.cc

Labels: -Merge-Request-61
Labels: Merge-Request-61
Bernie, please reconsider https://chromium-review.googlesource.com/c/chromiumos/platform2/+/673485 for M61. The CL is small and should be safe, and the underlying issue is a trivial escalation to root from almost any process, so IMHO pretty important to fix. If you feel this is too risky, maybe we can put this into the first M61 respin?

FWIW, I'll not open up the main bug for the exploit chain before we have this fix on stable.
Labels: -Hotlist-Merge-Review -Merge-Request-61 Merge-Approved-61
Project Member Comment 29 by bugdroid1@chromium.org, Sep 27
Labels: merge-merged-release-R61-9765.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/a6a1d0616082d66894ac9a46785b015d53890bec

commit a6a1d0616082d66894ac9a46785b015d53890bec
Author: Mike Frysinger <vapier@chromium.org>
Date: Wed Sep 27 23:42:44 2017

crash: start moving state dir initialization into C++ logic

We have various init code spread around (like the init script, and lazy
setup by various sub-collectors).  During early boot, or if no crashes
have yet been created, it can be confusing when these paths don't yet
exist, and autotests can mess them up.  So have the early --init logic
start handling all of this for us.

BUG= chromium:766275 
TEST=deleted paths and ran --init on system and checked result
TEST=precq passes

Change-Id: I67ab2370f9d2a94cfa4d18461bafd06896d10260
(cherry picked from commit 9f95fe6adaf8b536d2857b22a0cefec69b9bba12)
Reviewed-on: https://chromium-review.googlesource.com/687859
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/a6a1d0616082d66894ac9a46785b015d53890bec/crash-reporter/user_collector_base.h
[modify] https://crrev.com/a6a1d0616082d66894ac9a46785b015d53890bec/crash-reporter/user_collector_base.cc
[modify] https://crrev.com/a6a1d0616082d66894ac9a46785b015d53890bec/crash-reporter/user_collector.cc

Labels: -Merge-Approved-61 -Merge-Approved-62
Project Member Comment 31 by bugdroid1@chromium.org, Oct 15
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/5bfbdc6461cb1e03cab428e628766e8061fda123

commit 5bfbdc6461cb1e03cab428e628766e8061fda123
Author: Mike Frysinger <vapier@chromium.org>
Date: Fri Oct 13 21:45:17 2017

crash: sandbox the sender script a bit

We're a bit limited in what we can pull off here since it's a shell
script and we need to read reports saved under the root account, but
it's better than nothing.

BUG= chromium:766275 
TEST=ran by hand and it still was able to process crash reports
TEST=precq passes

Change-Id: I0d3e0c13787c075ede8e2e8ffee8fd66108a0687
Reviewed-on: https://chromium-review.googlesource.com/678294
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/5bfbdc6461cb1e03cab428e628766e8061fda123/crash-reporter/crash_sender

Project Member Comment 33 by bugdroid1@chromium.org, Oct 17
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/da54c4bb6fd8b089dbfa7862b6bcc87dfd249663

commit da54c4bb6fd8b089dbfa7862b6bcc87dfd249663
Author: Mike Frysinger <vapier@chromium.org>
Date: Tue Oct 17 09:37:32 2017

crash: move test knobs from /tmp to /run

We're updating crash_reporter to use a path under /run, so update the
autotests to flag the new locations.

BUG= chromium:766275 
TEST=precq passes (which runs autotests that use these flags)
CQ-DEPEND=CL:721564

Change-Id: Id9ffd097571e33a188c4f3be66a342cd74796bc6
Reviewed-on: https://chromium-review.googlesource.com/716878
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>

[modify] https://crrev.com/da54c4bb6fd8b089dbfa7862b6bcc87dfd249663/client/cros/crash/crash_test.py
[modify] https://crrev.com/da54c4bb6fd8b089dbfa7862b6bcc87dfd249663/client/site_tests/logging_CrashSender/logging_CrashSender.py

Project Member Comment 34 by bugdroid1@chromium.org, Oct 17
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/bde9e80e0be18a6e3023cb7959931801e8b59f84

commit bde9e80e0be18a6e3023cb7959931801e8b59f84
Author: Mike Frysinger <vapier@chromium.org>
Date: Tue Oct 17 09:37:31 2017

crash: move runtime flags out of /tmp and into /run

Use a path we fully control all the time so random processes can't
wedge us (albeit for a single boot since /tmp is cleared).

BUG= chromium:766275 
TEST=precq passes (which runs autotests that use these flags)
CQ-DEPEND=CL:716878

Change-Id: I88cf97cf96eefc054c040be5c19a27d4e93331b0
Reviewed-on: https://chromium-review.googlesource.com/721564
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>

[modify] https://crrev.com/bde9e80e0be18a6e3023cb7959931801e8b59f84/crash-reporter/crash_sender
[modify] https://crrev.com/bde9e80e0be18a6e3023cb7959931801e8b59f84/crash-reporter/crash_collector.cc

Project Member Comment 35 by bugdroid1@chromium.org, Oct 18
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/4a231529c57075b111d546f7a2f33733c5adcffb

commit 4a231529c57075b111d546f7a2f33733c5adcffb
Author: Mike Frysinger <vapier@chromium.org>
Date: Wed Oct 18 13:15:05 2017

crash: change sender to use unique /tmp dir

Now that the runtime/test knobs are in /run and not /tmp, we can
set up a unique /tmp path for this script everytime we run it.

BUG= chromium:766275 
TEST=precq passes

Change-Id: I6ae974b2f3c82162fe943168b5b63d725bba5660
Reviewed-on: https://chromium-review.googlesource.com/723869
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>

[modify] https://crrev.com/4a231529c57075b111d546f7a2f33733c5adcffb/crash-reporter/crash_sender

Project Member Comment 36 by bugdroid1@chromium.org, Oct 18
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/8581b7ad159c29dffd16836cf83c90ae00ed9509

commit 8581b7ad159c29dffd16836cf83c90ae00ed9509
Author: Mike Frysinger <vapier@chromium.org>
Date: Wed Oct 18 17:30:40 2017

crash_test: drop /bin/sh redirection

There's no need to run this through /bin/sh as it's a standalone program.

BUG= chromium:766275 
TEST=precq passes

Change-Id: I56b2324f03b1e260024646a8a97671f88a9d1d1b
Reviewed-on: https://chromium-review.googlesource.com/724733
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>

[modify] https://crrev.com/8581b7ad159c29dffd16836cf83c90ae00ed9509/client/cros/crash/crash_test.py

Project Member Comment 37 by bugdroid1@chromium.org, Oct 19
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/ec108678e26eba449c657e89f5ca30c98f73d312

commit ec108678e26eba449c657e89f5ca30c98f73d312
Author: Mike Frysinger <vapier@chromium.org>
Date: Thu Oct 19 09:51:45 2017

crash: exit non-zero when writing /proc files fail

If we don't update /proc files successfully, we log a warning, but still
exit 0.  This causes crash related autotests to fail in confusing ways.
Instead, exit 1 when the steps fail so we get a clear failing signal.

BUG= chromium:766275 
TEST=crash autotests still pass in precq

Change-Id: I392d63ec3d3b339f0fa06173ad9833ba782f2d92
Reviewed-on: https://chromium-review.googlesource.com/724940
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>

[modify] https://crrev.com/ec108678e26eba449c657e89f5ca30c98f73d312/crash-reporter/crash_reporter.cc

Project Member Comment 38 by bugdroid1@chromium.org, Oct 19
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/123f48fd2823f3aee135930f587705a0165cab91

commit 123f48fd2823f3aee135930f587705a0165cab91
Author: Mike Frysinger <vapier@chromium.org>
Date: Thu Oct 19 19:56:47 2017

crash: sandbox crash collector

BUG= chromium:766275 
TEST=crash_reporter still reports crashes successfully.

Change-Id: Ic50e1d5d7689950273a98c9ddf67dcf74a71387a
Reviewed-on: https://chromium-review.googlesource.com/672943
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>

[modify] https://crrev.com/123f48fd2823f3aee135930f587705a0165cab91/crash-reporter/crash_reporter.cc
[modify] https://crrev.com/123f48fd2823f3aee135930f587705a0165cab91/crash-reporter/crash-reporter.gyp

i was thinking about moving crash_sender's temp file handling out of /tmp and into /run, but i'm now revising that position.  the temp output could be quite large (in case the server dumps a lot of junk on us), and in theory we could fill up /run which would have bad consequences for other daemons using /run.

crash_sender itself is now sandboxed and sets up a unique /tmp mount every time it runs, so shouldn't be an attack route anymore.

for reference: https://chromium-review.googlesource.com/736757
Labels: CVE-2017-15404
Labels: -Restrict-View-SecurityNotify
Labels: allpublic
Sign in to add a comment