New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user
Status: Fixed
Owner:
Closed: Oct 6
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug-Security

Blocked on:
issue 780039

Blocking:
issue 766253



Sign in to add a comment
Security: persistence with cryptohomed stateful recovery
Project Member Reported by meacer@chromium.org, Sep 18 Back to list
Split from  bug 766253 :

"""
Cryptohomed has a feature called a stateful recovery. The file /mnt/stateful_partition/decrypt_stateful indicates a recovery request during boot. Cryptohomed takes a username and password hash from decrypt_stateful, decrypts the corresponding cryptohome and copies it to /mnt/stateful_partition/decrypted. And then it reboots. There is probably some sort of recovery USB stick, which asks the user for the password, writes it to decrypt_stateful, boots and later passes the decrypted files to the user. I don't know much about that.

In any case, the copying follows symlinks, so the exploit symlinks modprobe.d source file to /run/modprobe.d and runs a command as root with the uinput module.

There is a race between cryptohomed and uinput. uinput runs after login prompt is visible. With trickery, it's possible to reliably win the race. Chrome depends on the session manager, which reads /var/lib/whitelist/policy during initialization. Turn the policy file into a fifo. Reading of the fifo blocks until something writes to the fifo. Now, symlink then/unblock_session_manager to the fifo. The copying is done breadth first, so unblock_session_manager is written after modprobe.d.

Finally, cryptohomed would reboot, so make it block indefinitely on a then/then/block fifo. Once exploit gets root, it removes decrypt_stateful and restarts cryptohomed. Exploit in drop/persist.

"""

Greg: Do you mind taking a look at this, assign a severity and reassign if necessary? Thanks.
 
persist
1.6 KB View Download
Blocking: 766253
Comment 2 Deleted
Cc: keescook@chromium.org
[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.
Comment 4 Deleted
Comment 5 Deleted
Comment 6 Deleted
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.
Project Member Comment 14 by bugdroid1@chromium.org, Sep 21
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

Labels: Merge-Request-62 Merge-Request-61
Requesting merge to M61 and M62 for https://chromium-review.googlesource.com/674697
Project Member Comment 16 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
Labels: -Merge-Review-62 Merge-Approved-62
Approved for 62. 

We are really late in 61, is this safe to put directly into stable?
Labels: Security_Severity-High M-62
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.
Project Member Comment 20 by bugdroid1@chromium.org, Sep 22
Labels: merge-merged-release-R62-9901.B
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

Labels: -Merge-Approved-62
Project Member Comment 22 by sheriffbot@chromium.org, Sep 22
Labels: Pri-1
Cc: keta...@chromium.org
ketakid@, bhthompson@ - can we get a decision regarding M61 merge here?
Labels: -Merge-Request-61 Merge-Approved-61
Based on mnissler@'s comments in #19 approving merge to M61.
Project Member Comment 25 by sheriffbot@chromium.org, Oct 3
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
Owner: mnissler@chromium.org
Mattias, is this fixed?
Project Member Comment 27 by sheriffbot@chromium.org, Oct 6
Cc: bhthompson@google.com ketakid@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 28 by bugdroid1@chromium.org, Oct 6
Labels: merge-merged-release-R61-9765.B
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

Labels: -Merge-Approved-61
Status: Fixed
This is fixed indeed.
Project Member Comment 30 by sheriffbot@chromium.org, Oct 7
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Blockedon: 780039
Labels: CVE-2017-15405
Labels: -Restrict-View-SecurityNotify
Labels: allpublic
Sign in to add a comment