Twrp 16.0 decrypt fixes - #1
Merged
Merged
Conversation
Restore the AOSP guard that returns early when KeyMint did not upgrade the blob; without it *opHandle.getUpgradedBlob() dereferenced an empty std::optional on every retrieveKey() call. Also drop the write to /tmp/keymaster_key_blob/, which nothing reads. A blob rebound to a newer OS version or patch level can no longer be opened by the installed system, so keep the upgrade in memory only.
AOSP calls deleteAllKeys() here because a missing metadata key means a factory reset is in progress. Recovery has no such guarantee: the key also looks missing when /metadata failed to mount, and wiping Keystore destroys every key on the device, not just the FBE ones. read_key() is reached with neverGen() from TWRP, so a genuinely missing key still fails safely in retrieveOrGenerateKey().
fscrypt_initialize_systemwide_keys() and fscrypt_init_user0() are AOSP first-boot paths: they generate the device key and user 0's DE/CE keys whenever those are missing. In recovery a key reads as missing whenever we simply failed to open it, and generating a replacement makes the existing /data unrecoverable. Pass neverGen() for the device key and fail outright when user 0's keys are not found.
These files exist so that init can pick up the device DE policy on the next boot, and the installed system rewrites them itself. Recovery only needs s_device_policy in memory, so writing them here just puts three more writes on /data during decryption; per_boot_ref in particular was overwritten with a key that is discarded on reboot.
AOSP fixates on a successful read: every sibling key directory is passed to destroyKey(), which secdiscards the files and deletes the KeyMint key from Keystore. Normally only "current" exists, but a user midway through a credential change also has a cx* binding, and losing it in recovery is not recoverable. Rename to read_user_ce_key() since it no longer fixates.
The GateKeeper callback only assigns auth_token and auth_token_len on success, but the caller used them unconditionally: hwRet.isOk() is still true when GateKeeper rejects the password, so a rejected unlock sized a stack VLA from an uninitialized length. The hex buffer it built was never read, and the token itself was leaked, so drop the buffer, free the token and check ret before continuing.
The step-failure path returned without finalizing the statement or closing the database, and sqlite3_open() can allocate a handle even when it reports failure. Either one leaves /data/system/locksettings.db open and /data busy on unmount.
uint2hex() renders the sp-handle unpadded, which is what the Keystore alias needs, but the spblob files are named after the handle padded to 16 digits. The fallback only tried one and two leading zeros, so a handle with more than two high zero digits failed to open.
AServiceManager_waitForService() never returns if the service does not register, so a keystore2 that fails to start leaves the decrypt screen hung with no error. Poll checkService() instead; the callers already handle a null binder. Allow thirty seconds rather than a few. Devices whose KeyMint HAL is started from a script after the version properties have been fixed up keep keystore2 blocked until that happens, and it is on the far side of mounting a partition and reading its build.prop.
sehandle.h declares sehandle at global scope and AOSP has every executable that links libvold define and initialize it: main.cpp for vold, VoldFuzzer.cpp for the fuzzer. libvold itself only reads it. A second definition was later added inside namespace android::vold in Utils.cpp so that recovery, which links libvold without vold's main(), would still link. That definition shadows the global one for every unqualified use in the namespace, so PrepareDir() and CreateDeviceNode() read a pointer nothing ever assigns. It was harmless while AOSP still guarded the lookup with if (sehandle), but once that guard went away selabel_lookup() started faulting on it, and the lookup was commented out of PrepareDir() in response. Every directory created under /data since then inherits its parent's label. Keep a single definition at global scope and open the handle on first use, so main() and the fuzzer keep their eager initialization while recovery gets one too, then restore the AOSP labeling.
Setting selinux.restorecon_recursive only works because AOSP's init.rc has a trigger for it. Recovery's init does not, and WaitForProperty() returns immediately since we just set the value ourselves, so the three calls at the end of fscrypt_prepare_user_storage() did nothing at all.
copySqliteDb() streamed the file over the database keystore2 already had open, and left the stale -wal and -shm next to it. Stop keystore2 first, clear those, copy through the SQLite backup API so pending WAL content comes across, then start keystore2 again and wait for it to register. Do it from Decrypt_User_Synth_Pass() as well, since the synthetic password key is looked up by alias and needs the database in place whatever the credential type is.
Decrypt_User_Synth_Pass() already tries the AIDL service before falling back to HIDL, but the gatekeeper.password.key path still called IGatekeeper::getService() only, so it returned null and failed outright on a device that declares just the AIDL GateKeeper.
createOperation() returns upgradedBlob whenever KeyMint rebinds the key because our OS version or patch levels do not match the installed system, and we were dropping it on the floor. It is the only direct evidence that the environment is wrong, so log it. The upgrade itself is contained: it only reaches the tmpfs Keystore database, and keystore2 gates its garbage collector, which is what would delete the superseded blob from KeyMint, on sys.boot_completed. Call that out too, since a device tree that sets the property to start its own services also removes that protection.
There was a problem hiding this comment.
Pull request overview
This PR updates vold/libvold to improve TWRP/recovery decryption reliability by making SELinux labeling work without a dedicated main(), avoiding destructive key/Keystore behaviors in recovery-like environments, and hardening keystore2 / GateKeeper interactions to reduce UI hangs and mismatches.
Changes:
- Centralize SELinux
sehandleownership inUtils.cppand exposeandroid::vold::GetSehandle()for consistent initialization across binaries (including fuzzers andvold). - Adjust recovery-oriented encryption/decryption flows (avoid Keystore wipe on missing metadata key; avoid persisting upgraded KeyMint blobs; stop generating/creating certain keys; relabel via
selinux_android_restorecon()instead of init property triggers). - Improve decrypt plumbing: sync keystore2 DB into tmpfs using SQLite backup API; add AIDL GateKeeper path; add bounded keystore2 service polling in
Keystore.cpp.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| Utils.cpp | Defines global sehandle, adds GetSehandle(), updates labeling + restorecon behavior. |
| sehandle.h | Declares android::vold::GetSehandle() alongside global sehandle. |
| main.cpp | Uses GetSehandle() instead of per-binary sehandle init. |
| tests/VoldFuzzer.cpp | Switches fuzzer init to GetSehandle(). |
| MetadataCrypt.cpp | Avoids wiping Keystore when metadata key is missing (recovery-motivated). |
| KeystoreInfo.hpp | Adds backupDatabase() API. |
| KeystoreInfo.cpp | Adds SQLite cleanup in error paths + implements backupDatabase() via SQLite backup API. |
| Keystore.cpp | Replaces infinite AServiceManager_waitForService() usage with bounded polling helper. |
| KeyStorage.cpp | Stops persisting upgraded blobs; logs upgrade as operation-only. |
| FsCrypt.cpp | Avoids generating missing device/user keys; adjusts recovery-vs-installed-system assumptions. |
| Decrypt.h | Renames DB helper to syncKeystoreDb() returning bool. |
| Decrypt.cpp | Adds keystore DB sync logic, improves spblob lookup, adds AIDL GateKeeper path, and removes some older DB-copy flow. |
Suppressed comments (1)
FsCrypt.cpp:662
- fscrypt_init_user0() now fails when the CE key for user 0 is missing instead of creating it. As with the DE key check above, this is recovery-safe but can break first boot / provisioning flows. Consider limiting this behavior to recovery-only mode.
if (!ce_key_exists(0)) {
LOG(ERROR) << "CE key for user 0 not found, refusing to create one";
return false;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| struct selabel_handle* sehandle; | ||
|
|
||
| extern "C" int LLVMFuzzerInitialize(int argc, char argv) { |
Comment on lines
+97
to
+98
| sqlite3_close(dst_db); | ||
| sqlite3_close(src_db); |
| // Devices that only declare the AIDL GateKeeper have no HIDL service. | ||
| constexpr const char gatekeeperServiceName[] = "android.hardware.gatekeeper.IGatekeeper/default"; | ||
| if (AServiceManager_isDeclared(gatekeeperServiceName)) { | ||
| ::ndk::SpAIBinder gkBinder(AServiceManager_waitForService(gatekeeperServiceName)); |
Comment on lines
+554
to
558
| // Never generate the device key here: a new one makes the existing /data | ||
| // permanently unreadable. | ||
| if (!retrieveOrGenerateKey(device_key_path, device_key_temp, kEmptyAuthentication, | ||
| makeGen(s_data_options), &device_key)) | ||
| android::vold::neverGen(), &device_key)) | ||
| return false; |
Comment on lines
+655
to
+658
| if (!de_key_exists(0)) { | ||
| LOG(ERROR) << "DE key for user 0 not found, refusing to create one"; | ||
| return false; | ||
| } |
Comment on lines
132
to
136
| if (!pathExists(dir) && !in_dsu && first_key) { | ||
| auto delete_all = android::base::GetBoolProperty( | ||
| "ro.crypto.metadata_init_delete_all_keys.enabled", false); | ||
| if (delete_all) { | ||
| LOG(INFO) << "Metadata key does not exist, calling deleteAllKeys"; | ||
| Keystore::deleteAllKeys(); | ||
| } else { | ||
| LOG(INFO) << "Metadata key does not exist but " | ||
| "ro.crypto.metadata_init_delete_all_keys.enabled is false"; | ||
| } | ||
| // AOSP wipes Keystore here, assuming a missing key means a factory | ||
| // reset. In recovery it usually means /metadata failed to mount. | ||
| LOG(WARNING) << "Metadata key does not exist at " << dir << ", not wiping Keystore"; | ||
| } |
Comment on lines
+520
to
+524
| // keystore2 holds the destination open, so stop it for a clean copy. | ||
| printf("stopping keystore2 to sync '%s'\n", src.c_str()); | ||
| property_set("ctl.stop", "keystore2"); | ||
| waitForServiceState("keystore2", "stopped"); | ||
|
|
Comment on lines
1174
to
+1178
| } | ||
| } | ||
| ); | ||
| if (!hwRet.isOk()) { | ||
| delete[] auth_token; | ||
| if (!hwRet.isOk() || ret != 0) { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.