From d9b9295b8c2f17448f4eb3ea2c6f7d4a5c207c3f Mon Sep 17 00:00:00 2001 From: Paul Crowley Date: Fri, 4 Mar 2016 13:45:00 -0800 Subject: [PATCH] Fix memory leak in generate_key wrapper. Other fixes. - catch errors in looking for the keyring - static_assert to prevent a buffer overrun - remove obsolete, misleading comment - dial down priority of some log messages - explain why we ignore some errors - idiomatic C++11 Bug: 27552432 Change-Id: Ic3ee05b41eae45e7c6b571a459b326a483663526 --- Ext4Crypt.cpp | 31 ++++++++++++++++++++----------- KeyStorage.cpp | 2 +- Keymaster.cpp | 1 + 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/Ext4Crypt.cpp b/Ext4Crypt.cpp index fdc0197..98cb05e 100644 --- a/Ext4Crypt.cpp +++ b/Ext4Crypt.cpp @@ -122,6 +122,8 @@ static std::string generate_key_ref(const char* key, int length) unsigned char key_ref2[SHA512_DIGEST_LENGTH]; SHA512_Final(key_ref2, &c); + static_assert(EXT4_KEY_DESCRIPTOR_SIZE <= SHA512_DIGEST_LENGTH, + "Hash too short for descriptor"); return std::string((char*)key_ref2, EXT4_KEY_DESCRIPTOR_SIZE); } @@ -144,16 +146,21 @@ static std::string keyname(const std::string &raw_ref) { std::ostringstream o; o << "ext4:"; - for (auto i = raw_ref.begin(); i != raw_ref.end(); ++i) { - o << std::hex << std::setw(2) << std::setfill('0') << (int)*i; + for (auto i: raw_ref) { + o << std::hex << std::setw(2) << std::setfill('0') << (int)i; } return o.str(); } // Get the keyring we store all keys in -static key_serial_t e4crypt_keyring() +static bool e4crypt_keyring(key_serial_t& device_keyring) { - return keyctl_search(KEY_SPEC_SESSION_KEYRING, "keyring", "e4crypt", 0); + device_keyring = keyctl_search(KEY_SPEC_SESSION_KEYRING, "keyring", "e4crypt", 0); + if (device_keyring == -1) { + PLOG(ERROR) << "Unable to find device keyring"; + return false; + } + return true; } // Install password into global keyring @@ -164,7 +171,8 @@ static bool install_key(const std::string &key, std::string &raw_ref) if (!fill_key(key, ext4_key)) return false; raw_ref = generate_key_ref(ext4_key.raw, ext4_key.size); auto ref = keyname(raw_ref); - key_serial_t device_keyring = e4crypt_keyring(); + key_serial_t device_keyring; + if (!e4crypt_keyring(device_keyring)) return false; key_serial_t key_id = add_key("logon", ref.c_str(), (void*)&ext4_key, sizeof(ext4_key), device_keyring); @@ -172,7 +180,7 @@ static bool install_key(const std::string &key, std::string &raw_ref) PLOG(ERROR) << "Failed to insert key into keyring " << device_keyring; return false; } - LOG(INFO) << "Added key " << key_id << " (" << ref << ") to keyring " + LOG(DEBUG) << "Added key " << key_id << " (" << ref << ") to keyring " << device_keyring << " in process " << getpid(); return true; } @@ -230,7 +238,7 @@ static bool store_key(const std::string &key_path, const std::string &tmp_path, return false; } if (path_exists(tmp_path)) { - android::vold::destroyKey(tmp_path); + android::vold::destroyKey(tmp_path); // May be partially created so ignore errors } if (!android::vold::storeKey(tmp_path, auth, key)) return false; if (rename(tmp_path.c_str(), key_path.c_str()) != 0) { @@ -377,10 +385,10 @@ bool e4crypt_init_user0() { auto ce_path = get_ce_key_path(0); if (!path_exists(de_path) || !path_exists(ce_path)) { if (path_exists(de_path)) { - android::vold::destroyKey(de_path); // Ignore failure + android::vold::destroyKey(de_path); // May be partially created so ignore errors } if (path_exists(ce_path)) { - android::vold::destroyKey(ce_path); // Ignore failure + android::vold::destroyKey(ce_path); // May be partially created so ignore errors } if (!create_and_install_user_keys(0, false)) return false; } @@ -420,13 +428,14 @@ bool e4crypt_vold_create_user_key(userid_t user_id, int serial, bool ephemeral) if (!create_and_install_user_keys(user_id, ephemeral)) { return false; } - // TODO: create second key for user_de data return true; } static bool evict_key(const std::string &raw_ref) { auto ref = keyname(raw_ref); - auto key_serial = keyctl_search(e4crypt_keyring(), "logon", ref.c_str(), 0); + key_serial_t device_keyring; + if (!e4crypt_keyring(device_keyring)) return false; + auto key_serial = keyctl_search(device_keyring, "logon", ref.c_str(), 0); if (keyctl_revoke(key_serial) != 0) { PLOG(ERROR) << "Failed to revoke key with serial " << key_serial << " ref " << ref; return false; diff --git a/KeyStorage.cpp b/KeyStorage.cpp index 2338f23..d51fc4f 100644 --- a/KeyStorage.cpp +++ b/KeyStorage.cpp @@ -194,7 +194,7 @@ static bool stretchSecret(const std::string &stretching, const std::string &secr const std::string &salt, std::string &stretched) { if (stretching == kStretch_nopassword) { if (!secret.empty()) { - LOG(ERROR) << "Password present but stretching is nopasswd"; + LOG(WARNING) << "Password present but stretching is nopassword"; // Continue anyway } stretched.clear(); diff --git a/Keymaster.cpp b/Keymaster.cpp index 0fde8fa..b3ece19 100644 --- a/Keymaster.cpp +++ b/Keymaster.cpp @@ -106,6 +106,7 @@ bool Keymaster::generateKey( return false; } key.assign(reinterpret_cast(keyBlob.key_material), keyBlob.key_material_size); + free(const_cast(keyBlob.key_material)); return true; }