Thanks for your report. Passing this over to Chrome OS Security folks.

i'm not sure about this one.  once  crbug.com/677817  is closed (fix is already cherry picked into M57), this is no longer an issue.  this fundamentally relies on having a shell and running arbitrary commands which is not allowed in verified mode.

while we can harden crash_sender (and arguably, we should be rewriting that in C++ anyways), this attack route can be triggered multiple ways.  debugd also cats this file w/out doing a symlink check.  neither of which can defend against a symlink because of TOCTOU vectors.  we could add sanity checks like only extracting the first ~20 bytes (however many we store for the guid), but that wouldn't really fix this bug because it'd still be "local file read of first ~20 bytes".

also, this specific attack against crash_sender doesn't allow you to fully read a file.  bash will silently delete NUL bytes, and the code in question deletes all dashes via `tr`.

The use of  crbug.com/677817  was only to prove that it was not due to the disk permissions change caused by the switch to developer mode, and because I had it available to me.

With that said, I agree with your points and this issue would have to be part of a longer attack chain.

Would it not be possible to move the "Consent To Send Stats" to a location where the chronos user does not have write access to create a symlink? Verified IO could be done to the file via dbus/debugd which should mitigate any TOCTOU vectors.

I noticed debugd catting the file, but have not reported it because I cannot see any security impact, there were no files on my device owned by, or in the group of debugd (/proc/self/status shows the specific cat command is run as this user).

currently the consent file is managed by chromium (the browser), so changing it to read/write the file indirectly (say via dbus) would take a good amount of work.  so it's possible, but a bit of a yak shave.

wrt debugd, iirc, you can go to chrome://net-internals and get debug logs, and that'll make the dbus calls to debugd to run the logging functions.  then you can unpack that tarball and get the full file directly.

I just gave the debug logs functionality in chrome://net-internals a go, whilst the dbus endpoint (org.chromium.debugd.GetLogs I think) does read the file, it does not end up in the final .tgz.

In that case, perhaps more restrictive filtering may be appropriate, rather than just tr. For example, a sed for only guids:

cut -c -36 | sed -rn "/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}/p"

This would not 'fix' the issue, but would mitigate it to the point that it is not usefully exploitable, unless there are other critical GUIDS stored alone in files on the system.

specifically for this one file, i posted this CL:
  http://chromium-review.googlesource.com/422851

It looks like that will effectively mitigate this issue.

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

commit c3e835a0b87ea608be5c4edf02be8ae115496277
Author: Mike Frysinger <vapier@chromium.org>
Date: Fri Jan 06 09:09:34 2017

metrics: centralize client id look up

We provide limited sanity checking on the client id file, and we do so
in different places (differently).  Centralize the logic in the metrics
client so we don't duplicate the logic and we only have to robustify a
single code path.

I'm not a fan of exposing this ID more, but forcing people to duplicate
the logic is clearly worse in practice.

BUG= chromium:678365 
TEST=precq passes

Change-Id: Ib8c16fa0b3dc7d5e42cfb3f81df29d99336613d6
Reviewed-on: https://chromium-review.googlesource.com/422851
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Luigi Semenzato <semenzato@chromium.org>

[modify] https://crrev.com/c3e835a0b87ea608be5c4edf02be8ae115496277/metrics/metrics_library.cc
[modify] https://crrev.com/c3e835a0b87ea608be5c4edf02be8ae115496277/debugd/src/log_tool.cc
[modify] https://crrev.com/c3e835a0b87ea608be5c4edf02be8ae115496277/metrics/metrics_client.cc
[modify] https://crrev.com/c3e835a0b87ea608be5c4edf02be8ae115496277/metrics/metrics_library_test.cc
[modify] https://crrev.com/c3e835a0b87ea608be5c4edf02be8ae115496277/metrics/metrics_library.h
[modify] https://crrev.com/c3e835a0b87ea608be5c4edf02be8ae115496277/crash-reporter/crash_sender

vapier: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

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

i think we'll consider this issue resolved for M57+.  the impact is low as i don't believe there's any content on the device that strictly needs to be kept secret while a user is logged in.  we don't allow multiple users to be logged in simultaneously, so you can't escalate to reading other people's data.  we don't store secret keys that can be abused (as far as i'm aware).

unless mnissler@ thinks there's anything else to do here, i'll close it out.

Thanks for the report!  The panel decided to award $500 for this bug, noting the limited impact per comment 12.  A member of our finance team will be in touch shortly.

Thanks!

Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

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

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