[not yet triaged, but some tactical comments based on the description]
- Whitelist reading in chrome to just have it make sure the policy is a regular file only.
- The decrypt_stateful copy should copy symlinks not follow them.

Do I understand correctly that /run/modprobe.d is a file, not a directory? If not, I probably haven't understood the CopyDirectory logic properly (I do see that it behaves more like rsync when pieces of the dest already exist - like it looks like the mkdir failure handling would have allowed a symlink as well).

Suggestion: Blow away the destination directory right before we create it in platform2/cryptohome/stateful_recovery.cc. If doing so fails, don't attempt to copy at all. If appropriate, I guess this could be done straight in CopyDirectory as well.

Would also be cool there were a better signal than "presence of flag file" to gate this kind of stuff on :-/

/run/modprobe.d/ should be a dir if it exists, but normally i don't think it does

but i don't think that particular path is relevant to the underlying bug -- we pretty much never should be derefing symlinks when copying.

i think this is at least the 2nd time we've been bitten by leveraging /run/modprobe.d/ loads.  maybe we should be creating & locking down that path during chromeos_startup ?  or maybe patching out support in kmod in the first place if nothing ever cares about it ?

A simple way to close this is to just make sure we have a clean root-owned directory we copy to. Deleting the existing /mnt/stateful_partition/decrypted and recreating it before starting the copy operation should do the trick as there are no intermediate directories on the path an attacker can mess with (e.g. placing symlinks).

I'll make a CL for that.

+1 to nuking modprobe.d for good, but that'll require more diligence to make sure we don't break anything.

FWIW, here's a cryptohome CL that nukes the existing /mnt/stateful_partition/decrypted before recreating it.

I have an M60 tree anyways, so I'll see whether I can verify this cuts off the exploit.

I have a VM running the wolf image in which the exploit reproduces. I've tested the CL against M60, and it looks like it did help, however at the expense of the reboot after the initial attack failing. I see the Chrome splash screen, but then the device reboots. Time and again. The likely cause is session_manager's inability to start due to hanging on the attempt to read the policy file, which has been replaced by a fifo. Another thing to fix.

OK, so with the session_manager hang resolved, I do see the UI come up successfully, but the device still reboots. That is probably due to the reboot into recovery mode triggered at the end of stateful recovery (which looks like a normal reboot in my VM). Anyhow, sufficient validation that the CL does the right thing.

#12: Indeed, cryptohomed does a recovery.PerformReboot() in service.cc:576. The exploit avoids that by making cryptohomed block on a then/then/block fifo with the last copy.

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

commit 496f2827e7513f0ab5847e0ec3b8931c1962a3b0
Author: Mattias Nissler <mnissler@chromium.org>
Date: Thu Sep 21 08:08:03 2017

cryptohome: Create fresh destination directory for stateful recovery.

Contents of pre-existing directories can't be trusted because the
stateful file system is susceptible to manipulation by attackers
before boot.

BUG= chromium:766276 
TEST=new unit test

Change-Id: Ie5b39c006b2d080ae7e981e7000fca4ebcebb14e
Reviewed-on: https://chromium-review.googlesource.com/674697
Commit-Ready: Mattias Nissler <mnissler@chromium.org>
Tested-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>

[modify] https://crrev.com/496f2827e7513f0ab5847e0ec3b8931c1962a3b0/cryptohome/stateful_recovery.cc
[modify] https://crrev.com/496f2827e7513f0ab5847e0ec3b8931c1962a3b0/cryptohome/stateful_recovery_unittest.cc

Requesting merge to M61 and M62 for https://chromium-review.googlesource.com/674697

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

Approved for 62. 

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

IMHO, safe to go into 61 - the patch is very low risk. Even if we were to break stateful recovery, I don't think any actual users rely on it, so it wouldn't be the end of the world if we break it.

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

commit f4798d07e33a444710ec92222906525c5a1d0718
Author: Mattias Nissler <mnissler@chromium.org>
Date: Fri Sep 22 07:19:47 2017

cryptohome: Create fresh destination directory for stateful recovery.

Contents of pre-existing directories can't be trusted because the
stateful file system is susceptible to manipulation by attackers
before boot.

BUG= chromium:766276 
TEST=new unit test

Change-Id: Ie5b39c006b2d080ae7e981e7000fca4ebcebb14e
Reviewed-on: https://chromium-review.googlesource.com/674697
Commit-Ready: Mattias Nissler <mnissler@chromium.org>
Tested-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>
(cherry picked from commit 496f2827e7513f0ab5847e0ec3b8931c1962a3b0)
Reviewed-on: https://chromium-review.googlesource.com/678614

[modify] https://crrev.com/f4798d07e33a444710ec92222906525c5a1d0718/cryptohome/stateful_recovery.cc
[modify] https://crrev.com/f4798d07e33a444710ec92222906525c5a1d0718/cryptohome/stateful_recovery_unittest.cc

ketakid@, bhthompson@ - can we get a decision regarding M61 merge here?

Based on mnissler@'s comments in #19 approving merge to M61.

kerrnel: 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

Mattias, is this fixed?

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/+/9c2df8427b3cccf0fe0e8a593736684422706fa0

commit 9c2df8427b3cccf0fe0e8a593736684422706fa0
Author: Mattias Nissler <mnissler@chromium.org>
Date: Fri Oct 06 15:12:51 2017

cryptohome: Create fresh destination directory for stateful recovery.

Contents of pre-existing directories can't be trusted because the
stateful file system is susceptible to manipulation by attackers
before boot.

BUG= chromium:766276 
TEST=new unit test

Change-Id: Ie5b39c006b2d080ae7e981e7000fca4ebcebb14e
Reviewed-on: https://chromium-review.googlesource.com/674697
Commit-Ready: Mattias Nissler <mnissler@chromium.org>
Tested-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>
(cherry picked from commit 496f2827e7513f0ab5847e0ec3b8931c1962a3b0)
Reviewed-on: https://chromium-review.googlesource.com/704825

[modify] https://crrev.com/9c2df8427b3cccf0fe0e8a593736684422706fa0/cryptohome/stateful_recovery.cc
[modify] https://crrev.com/9c2df8427b3cccf0fe0e8a593736684422706fa0/cryptohome/stateful_recovery_unittest.cc

This is fixed indeed.