TeamWin / Team-Win-Recovery-Project

Core recovery files for the Team Win Recovery Project (T.W.R.P) - this is not up to date, please see https://github.com/TeamWin/android_bootable_recovery/
http://twrp.me
1.97k stars 740 forks source link

Warn when CRYPT_DATA_CORRUPT cryptfs metadata flag is set #1509

Open digitalcircuit opened 5 years ago

digitalcircuit commented 5 years ago

Device codename: shamu TWRP version: 3.3.1-0

WHAT STEPS WILL REPRODUCE THE PROBLEM?

Have a full-disk encrypted device that has erroneously failed to mount userdata, setting CRYPT_DATA_CORRUPT on the cryptfs metadata.

I cannot reliably reproduce this, but it's happened at least twice on my shamu after a spurious reboot, and potentially has happened to others. For testing purposes, one can manually modify the cryptfs metadata to set this.

TWRP did not set this flag, the Android operating system (LineageOS 15.1 in my case) did.

WHAT IS THE EXPECTED RESULT?

TWRP should check the cryptfs metadata and warn if the CRYPT_DATA_CORRUPT flag has been set, either before or after attempting full disk decryption.

For example, in cryptfs_verify_passwd from cryptfs.cpp, it might be like this:

     if (get_crypt_ftr_and_key(&crypt_ftr)) {
         SLOGE("Error getting crypt footer and key\n");
         return -1;
+    }
+
+    if (crypt_ftr.flags & CRYPT_DATA_CORRUPT) {
+        /* The cryptfs metadata claims the device is corrupt, log a warning, but try anyways in case the metadata is mistaken */
+        SLOGW("Warning: crypto footer claims data is corrupt (CRYPT_DATA_CORRUPT set), trying decryption anyways...\n");
     }

     if (crypt_ftr.flags & CRYPT_MNT_KEY_UNENCRYPTED) {

NOTE: This flag can be set when the underlying data is actually fine! As TWRP is a recovery, it makes sense to try anyways in case data can be.. recovered. I've had this flag wrongly set, and TWRP was able to decrypt just fine. See my comment on an older issue for details.

I haven't yet found the contribution guidelines for TWRP, whether Gerrit or GitHub pull requests are used, but I'd be happy to submit this as a change request.

WHAT HAPPENS INSTEAD?

TWRP mounts the userdata partition after decrypting with no indication of what's going on. Technically, this behavior is fine, but it would be useful to signal to the user somehow that Android had marked the userdata partition as corrupt.

ADDITIONAL INFORMATION

For the Nexus 6, based on fstab.shamu's encryptable=/dev/block/platform/msm_sdcc.1/by-name/metadata lines, you can modify the cryptfs metadata flags for testing like this:

  1. ADB in TWRP recovery, pull all /data files you care about, making a backup on your computer.
  2. ADB: dd if=/dev/block/platform/msm_sdcc.1/by-name/metadata of=/sdcard/metadata.img
  3. ADB pull metadata.img
  4. Hex-edit metadata.img, checking bytes 12-16. There should only be a 0 or an 8. If so, set the 12th-16th bytes to zero to signal no flags, or to 0x080000 to signal CRYPT_DATA_CORRUPT. Save the file.
  5. ADB push the modified metadata.img back,
  6. ADB: dd if=/sdcard/metadata.img of=/dev/block/platform/msm_sdcc.1/by-name/metadata bs=1 count=16
  7. Reboot to recovery.

The additional logs did not seem to be applicable to this case.

CaptainThrowback commented 5 years ago

Can you push this change to Gerrit so it can be reviewed?

digitalcircuit commented 5 years ago

@CaptainThrowback I've submitted the change to Gerrit at https://gerrit.omnirom.org/#/c/android_bootable_recovery/+/35126/

I tried adding you as a reviewer, but Gerrit claimed your account was not registered. Regardless, thank you for the pointer!

bigbiff commented 4 years ago

Hello, can you resubmit this to https://gerrit.twrp.me with the android_bootable_recovery project?

digitalcircuit commented 4 years ago

@bigbiff Sure thing!

I apologize for my delay in replying. Since September I've done a complete operating system reinstall and also traveled out of town, so I'm still catching up and rebuilding my development environment. I'll try to re-submit this within a month, and if there's any other deadline, I can try to meet that instead.

digitalcircuit commented 4 years ago

@bigbiff Change has been resubmitted to gerrit.twrp.me as https://gerrit.twrp.me/c/android_bootable_recovery/+/1889