Thanks for the report, confirmed.

Potentially allows privilege escalation -> high severity

Interestingly, the code does attempt to verify the path is not a symlink, but incorrectly: https://chromium.googlesource.com/chromiumos/platform2/+/master/crash-reporter/crash_collector.cc#114 Even if the check were correct, there'd still be a TOCTOU issue.

Best to change crash_reporter to not manipulate chronos-writable file system locations, but only read them (carefully!).

Another interesting note is that this wasn't unfortunately prevented by the symlink traversal blocking mortonm@ has enabled in non-chronos directories. Might want to consider expanding the policy to most of /home/chronos? We'd have to make sure the attacker can't manipulate the symlink traversal policy by deleting directories though.

IIUC, the only reason that directory is in /home/chronos is because it was intended to be within the cryptohome to not leak sensitive user data to a file system location that's not protected by user data encryption.

The best option might be to just move the directory to /home/root/<userhash>/crash, which is the location that system daemons are supposed to be using for storing sensitive user data. While we have this location for a long time, I think the crash reporter code is ancient enough to predate the availability of this.

Note that chrome crashes would still have to go to /home/chronos/<userhash>/crash, and have to be collected from their by crash_reporter.

Luigi does that sound reasonable? I'll try to cook up a CL proposal, but would appreciate help with this, in particular if there are more gotchas I'm not aware of yet :)

Investigated a bit more and found that at least for chrome and ARC++ crashes, crash_reporter is currently invoked from Chrome to generate the report data. So these cases can't easily be switched to a /home/root location. So maybe we should keep that for now, but still have other crashes go to /home/root?

we should be able to address the TOCTOU race by walking the dirs w/openat & O_NOFOLLOW.  that would prob be low risk enough for backporting.

looking forward, if we have dedicated dirs for daemons to store user-specific-encryption, then sounds good to use.  as you note, there are scenarios where we're invoked as non-root, so we'd want to use both: if we're invoked by the kernel (e.g. running as root), we'd use the secured dir under /home/root/, else we'd use the dir under /home/chronos/.  that'd further mitigate priv escalation.

let me know how much you want to pick up or if you want me to run with it from here.

i thought base::DirectoryExists DTRT wrt symlinks because so many of the other APIs in base:: do, but reading the source shows it def uses stat() and not lstat().  this introduced the chown logic:
  https://chromium-review.googlesource.com/717116
but even before that, we'd deref the symlink (since we only used PathExists) and write to wherever it pointed.  a diff bug, but maybe not as bad.

Agreed with all that Mike says, and his CL to make the code that creates the directory safe is here: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/1152078

Plan is to land that and merge it back, then switch to different directory eventually. As mentioned, I've started a CL in that direction, will share once it's in reasonable shape.

Here's the CL to switch to a root-owned directory for meta data generation: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/1155107

Untested, and likely needs more work, but sharing early since I have to run now.

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

commit 0f848cf9f3d557a1389cd0545f35a820269e779b
Author: Mike Frysinger <vapier@chromium.org>
Date: Thu Aug 02 20:53:18 2018

crash: treat attached files as relative to the metadata

There's no need for payloads to be in a sep location from the metadata,
and only crash reports are guaranteed to be stable across reboots and
OS upgrades.

For now we normalize the path by hand to support absolute and relative
paths as we might be uploading reports from older OS releases.  We'll
drop support for that after a few releases.

BUG= chromium:866895 
TEST=unittests pass

Change-Id: Ibda8c2c24377592d7356364c4acac915722748f8
Reviewed-on: https://chromium-review.googlesource.com/1156064
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>

[modify] https://crrev.com/0f848cf9f3d557a1389cd0545f35a820269e779b/crash-reporter/crash_sender.sh
[modify] https://crrev.com/0f848cf9f3d557a1389cd0545f35a820269e779b/crash-reporter/user_collector_base.cc
[modify] https://crrev.com/0f848cf9f3d557a1389cd0545f35a820269e779b/crash-reporter/crash_collector_test.cc
[modify] https://crrev.com/0f848cf9f3d557a1389cd0545f35a820269e779b/crash-reporter/crash_collector.cc
[modify] https://crrev.com/0f848cf9f3d557a1389cd0545f35a820269e779b/crash-reporter/crash_collector.h

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

commit 1f65431f85ae56e03303760e7e045cd483ba957b
Author: Mike Frysinger <vapier@chromium.org>
Date: Sat Aug 04 09:00:06 2018

crash: improve spool dir init

Use *at related functions to guarantee atomic walking of filepaths.
Then add unittests to make sure this all works as we expect.

BUG= chromium:866895 
TEST=unittests pass

Change-Id: If6e431d4013cfc5a68691ce341cd5933ffbff409
Reviewed-on: https://chromium-review.googlesource.com/1152078
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/1f65431f85ae56e03303760e7e045cd483ba957b/crash-reporter/user_collector_base.cc
[modify] https://crrev.com/1f65431f85ae56e03303760e7e045cd483ba957b/crash-reporter/crash_collector_test.cc
[modify] https://crrev.com/1f65431f85ae56e03303760e7e045cd483ba957b/crash-reporter/crash_collector.cc
[modify] https://crrev.com/1f65431f85ae56e03303760e7e045cd483ba957b/crash-reporter/crash_collector.h

not sure about merging back to R68

This bug requires manual review: M69 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), kariahda@(iOS), cindyb@(ChromeOS), govind@(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

I think we can live without the fix for M68. Mike, can you provide a list of changes to merge to M69? Thanks.

Adding cindyb@ for merge review.

only the two CLs listed above in comment #8/#9 are needed in R69

Merge approved, M69.

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

commit d14d5e88ff7d40f5f2d42751ed3c67224cbddc16
Author: Mike Frysinger <vapier@chromium.org>
Date: Fri Aug 10 04:38:44 2018

crash: treat attached files as relative to the metadata

There's no need for payloads to be in a sep location from the metadata,
and only crash reports are guaranteed to be stable across reboots and
OS upgrades.

For now we normalize the path by hand to support absolute and relative
paths as we might be uploading reports from older OS releases.  We'll
drop support for that after a few releases.

BUG= chromium:866895 
TEST=unittests pass

Change-Id: Ibda8c2c24377592d7356364c4acac915722748f8
Reviewed-on: https://chromium-review.googlesource.com/1156064
(cherry picked from commit 0f848cf9f3d557a1389cd0545f35a820269e779b)
Reviewed-on: https://chromium-review.googlesource.com/1170482
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/d14d5e88ff7d40f5f2d42751ed3c67224cbddc16/crash-reporter/crash_sender.sh
[modify] https://crrev.com/d14d5e88ff7d40f5f2d42751ed3c67224cbddc16/crash-reporter/user_collector_base.cc
[modify] https://crrev.com/d14d5e88ff7d40f5f2d42751ed3c67224cbddc16/crash-reporter/crash_collector_test.cc
[modify] https://crrev.com/d14d5e88ff7d40f5f2d42751ed3c67224cbddc16/crash-reporter/crash_collector.cc
[modify] https://crrev.com/d14d5e88ff7d40f5f2d42751ed3c67224cbddc16/crash-reporter/crash_collector.h

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

commit a4d88a90db5e68d5d0ff98b9081126ae9020d708
Author: Mike Frysinger <vapier@chromium.org>
Date: Fri Aug 10 04:38:47 2018

crash: improve spool dir init

Use *at related functions to guarantee atomic walking of filepaths.
Then add unittests to make sure this all works as we expect.

BUG= chromium:866895 
TEST=unittests pass

Change-Id: If6e431d4013cfc5a68691ce341cd5933ffbff409
Reviewed-on: https://chromium-review.googlesource.com/1152078
(cherry picked from commit 1f65431f85ae56e03303760e7e045cd483ba957b)
Reviewed-on: https://chromium-review.googlesource.com/1170483
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/a4d88a90db5e68d5d0ff98b9081126ae9020d708/crash-reporter/user_collector_base.cc
[modify] https://crrev.com/a4d88a90db5e68d5d0ff98b9081126ae9020d708/crash-reporter/crash_collector_test.cc
[modify] https://crrev.com/a4d88a90db5e68d5d0ff98b9081126ae9020d708/crash-reporter/crash_collector.cc
[modify] https://crrev.com/a4d88a90db5e68d5d0ff98b9081126ae9020d708/crash-reporter/crash_collector.h

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