From 2c9d6d6675b145474c98131fb644e5615ab40534 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 29 Oct 2020 12:59:28 -0700 Subject: [PATCH] KeyUtil: don't use keepOld=true for system DE and volume keys Commit 77df7f207dce / http://aosp/1217657 ("Refactor to use EncryptionPolicy everywhere we used to use raw_ref") unintentionally made fscrypt_initialize_systemwide_keys() start specifying keepOld=true (via default parameter value) when retrieving the system DE key, and likewise for read_or_create_volkey() and volume keys. As a result, if the associated Keymaster key needs to be upgraded, the upgraded key blob gets written to "keymaster_key_blob_upgraded", but it doesn't replace the original "keymaster_key_blob", nor is the original key deleted from Keymaster. This happens at every boot, eventually resulting in the RPMB partition in Keymaster becoming full. Only the metadata encryption key ever needs keepOld=true, since it's the only key that isn't stored in /data, and the purpose of keepOld=true is to allow a key that isn't stored in /data to be committed or rolled back when a userdata checkpoint is committed or rolled back. So, fix this bug by removing the default value of keepOld, and specifying false everywhere except the metadata encryption key. Note that when an affected device gets this fix, it will finally upgrade its system DE key correctly. However, this fix doesn't free up space in Keymaster that was consumed by this bug. Test: On bramble: - Flashed rvc-d1-dev build, with wiping userdata - Flashed a newer build, without wiping userdata - Log expectedly shows key upgrades: $ adb logcat | grep 'Upgrading key' D vold : Upgrading key: /metadata/vold/metadata_encryption/key D vold : Upgrading key: /data/unencrypted/key D vold : Upgrading key: /data/misc/vold/user_keys/de/0 D vold : Upgrading key: /data/misc/vold/user_keys/ce/0/current - Rebooted - Log unexpectedly shows the system DE key being upgraded again: $ adb logcat | grep 'Upgrading key' D vold : Upgrading key: /data/unencrypted/key - "keymaster_key_blob_upgraded" unexpectedly still exists: $ adb shell find /data /metadata -name keymaster_key_blob_upgraded /data/unencrypted/key/keymaster_key_blob_upgraded - Applied this fix and flashed, without wiping userdata - Log shows system DE key being upgraded (expected because due to the bug, the upgraded key didn't replace the original one before) $ adb logcat | grep 'Upgrading key' D vold : Upgrading key: /data/unencrypted/key - "keymaster_key_blob_upgraded" expectedly no longer exists $ adb shell find /data /metadata -name keymaster_key_blob_upgraded - Rebooted - Log expectedly doesn't show any more key upgrades $ adb logcat | grep 'Upgrading key' Bug: 171944521 Bug: 172019387 (cherry picked from commit c493903732d0c17b33091cf722cbcc3262292801) Merged-In: I42d3f5fbe32cb2ec229f4b614cfb271412a3ed29 Change-Id: I42d3f5fbe32cb2ec229f4b614cfb271412a3ed29 --- FsCrypt.cpp | 12 ++++++------ KeyStorage.h | 14 +++++++++++++- KeyUtil.h | 4 +++- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/FsCrypt.cpp b/FsCrypt.cpp index e21524a..061ee7c 100644 --- a/FsCrypt.cpp +++ b/FsCrypt.cpp @@ -198,7 +198,7 @@ static bool read_and_fixate_user_ce_key(userid_t user_id, auto const paths = get_ce_key_paths(directory_path); for (auto const ce_key_path : paths) { LOG(DEBUG) << "Trying user CE key " << ce_key_path; - if (retrieveKey(ce_key_path, auth, ce_key)) { + if (retrieveKey(ce_key_path, auth, ce_key, false)) { LOG(DEBUG) << "Successfully retrieved key"; fixate_user_ce_key(directory_path, ce_key_path, paths); return true; @@ -399,7 +399,7 @@ static bool load_all_de_keys() { userid_t user_id = std::stoi(entry->d_name); auto key_path = de_dir + "/" + entry->d_name; KeyBuffer de_key; - if (!retrieveKey(key_path, kEmptyAuthentication, &de_key)) return false; + if (!retrieveKey(key_path, kEmptyAuthentication, &de_key, false)) return false; EncryptionPolicy de_policy; if (!install_storage_key(DATA_MNT_POINT, options, de_key, &de_policy)) return false; auto ret = s_de_policies.insert({user_id, de_policy}); @@ -433,7 +433,7 @@ bool fscrypt_initialize_systemwide_keys() { KeyBuffer device_key; if (!retrieveOrGenerateKey(device_key_path, device_key_temp, kEmptyAuthentication, - makeGen(options), &device_key)) + makeGen(options), &device_key, false)) return false; EncryptionPolicy device_policy; @@ -667,7 +667,7 @@ static bool read_or_create_volkey(const std::string& misc_path, const std::strin EncryptionOptions options; if (!get_volume_file_encryption_options(&options)) return false; KeyBuffer key; - if (!retrieveOrGenerateKey(key_path, key_path + "_tmp", auth, makeGen(options), &key)) + if (!retrieveOrGenerateKey(key_path, key_path + "_tmp", auth, makeGen(options), &key, false)) return false; if (!install_storage_key(BuildDataPath(volume_uuid), options, key, policy)) return false; return true; @@ -686,12 +686,12 @@ static bool fscrypt_rewrap_user_key(userid_t user_id, int serial, auto const directory_path = get_ce_key_directory_path(user_id); KeyBuffer ce_key; std::string ce_key_current_path = get_ce_key_current_path(directory_path); - if (retrieveKey(ce_key_current_path, retrieve_auth, &ce_key)) { + if (retrieveKey(ce_key_current_path, retrieve_auth, &ce_key, false)) { LOG(DEBUG) << "Successfully retrieved key"; // TODO(147732812): Remove this once Locksettingservice is fixed. // Currently it calls fscrypt_clear_user_key_auth with a secret when lockscreen is // changed from swipe to none or vice-versa - } else if (retrieveKey(ce_key_current_path, kEmptyAuthentication, &ce_key)) { + } else if (retrieveKey(ce_key_current_path, kEmptyAuthentication, &ce_key, false)) { LOG(DEBUG) << "Successfully retrieved key with empty auth"; } else { LOG(ERROR) << "Failed to retrieve key for user " << user_id; diff --git a/KeyStorage.h b/KeyStorage.h index f9d3ec6..5228f08 100644 --- a/KeyStorage.h +++ b/KeyStorage.h @@ -61,8 +61,20 @@ bool storeKeyAtomically(const std::string& key_path, const std::string& tmp_path const KeyAuthentication& auth, const KeyBuffer& key); // Retrieve the key from the named directory. +// +// If the key is wrapped by a Keymaster key that requires an upgrade, then that +// Keymaster key is upgraded. If |keepOld| is false, then the upgraded +// Keymaster key replaces the original one. As part of this, the original is +// deleted from Keymaster; however, if a user data checkpoint is active, this +// part is delayed until the checkpoint is committed. +// +// If instead |keepOld| is true, then the upgraded key doesn't actually replace +// the original one. This is needed *only* if |dir| isn't located in /data and +// a user data checkpoint is active. In this case the caller must handle +// replacing the original key if the checkpoint is committed, and deleting the +// upgraded key if the checkpoint is rolled back. bool retrieveKey(const std::string& dir, const KeyAuthentication& auth, KeyBuffer* key, - bool keepOld = false); + bool keepOld); // Securely destroy the key stored in the named directory and delete the directory. bool destroyKey(const std::string& dir); diff --git a/KeyUtil.h b/KeyUtil.h index 23278c1..0f5bc93 100644 --- a/KeyUtil.h +++ b/KeyUtil.h @@ -74,9 +74,11 @@ bool installKey(const std::string& mountpoint, const EncryptionOptions& options, // responsible for dropping caches. bool evictKey(const std::string& mountpoint, const EncryptionPolicy& policy); +// Retrieves the key from the named directory, or generates it if it doesn't +// exist. In most cases |keepOld| must be false; see retrieveKey() for details. bool retrieveOrGenerateKey(const std::string& key_path, const std::string& tmp_path, const KeyAuthentication& key_authentication, const KeyGeneration& gen, - KeyBuffer* key, bool keepOld = true); + KeyBuffer* key, bool keepOld); // Re-installs a file-based encryption key of fscrypt-provisioning type from the // global session keyring back into fs keyring of the mountpoint.