Kees probably has thoughts on the protected_symlinks issue, if he's cc'd.

Good point, adding Kees.

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.

Here's an (untested) CL: https://chromium-review.googlesource.com/#/c/chromiumos/platform2/+/672943

#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.

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

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

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

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

Approved for 62. 

We are really late for 61, is this safe to put directly into stable?

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

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

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

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.

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

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

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/c3baf93fcd4230cff520cc4d3e32cc940d297f37

commit c3baf93fcd4230cff520cc4d3e32cc940d297f37
Author: Mike Frysinger <vapier@chromium.org>
Date: Sat Oct 14 23:54:37 2017

crash: move spool dir init into C++

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

Change-Id: Ib54584cf16891fc2eada4d3a53b4f2253a1578b4
Reviewed-on: https://chromium-review.googlesource.com/717116
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/c3baf93fcd4230cff520cc4d3e32cc940d297f37/crash-reporter/crash_reporter.cc
[modify] https://crrev.com/c3baf93fcd4230cff520cc4d3e32cc940d297f37/crash-reporter/init/crash-reporter.conf
[modify] https://crrev.com/c3baf93fcd4230cff520cc4d3e32cc940d297f37/crash-reporter/kernel_collector_test.cc
[modify] https://crrev.com/c3baf93fcd4230cff520cc4d3e32cc940d297f37/crash-reporter/crash_collector.cc
[modify] https://crrev.com/c3baf93fcd4230cff520cc4d3e32cc940d297f37/crash-reporter/crash_collector.h

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

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

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

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

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

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