From fa4039b1620987d82f119576cbdfaf503cd4e2b5 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Mon, 3 Apr 2017 15:48:09 -0700 Subject: [PATCH] vold: unlink ext4 encryption keys rather than revoking them Unlinking keys rather than revoking them avoids bugs in certain kernel versions without having to hack around the problem with an arbitrary 20 second delay, which is not guaranteed to be sufficient and has caused full device hangs like in b/35988361. Furthermore, in the context of filesystem encryption, unlinking is not currently supposed to be any less secure than revoking. There was a case where revoking (but not unlinking) keys will cause the filesystem to deny access to files that were previously opened with that key. However, this was a means of _access control_, which encryption is not intended to be used for. Instead, file permissions and/or SELinux should be used to enforce access control, while filesystem encryption should be used to protect data at rest independently from access control. This misfeature has also been removed upstream (and backported to 4.4-stable and 4.9-stable) because it caused CVE-2017-7374. Eventually we'd really like to make the kernel support proper revocation of filesystem encryption keys, i.e. fully clearing all key material and plaintext and safely waiting for any affected filesystem operations or writeback to complete. But for now this functionality does not exist. ('sync && echo 3 > /proc/sys/vm/drop_caches' can be useful, but it's not good enough.) Bug: 35988361 Change-Id: Ib44effe5368cdce380ae129dc4e6c6fde6cb2719 (cherry picked from commit fd7ba5e4c61691d8a45bc729b7659940a984bab0) --- Ext4Crypt.cpp | 56 +++++++++++---------------------------------------- 1 file changed, 12 insertions(+), 44 deletions(-) diff --git a/Ext4Crypt.cpp b/Ext4Crypt.cpp index 59b76d6..9dfcad8 100644 --- a/Ext4Crypt.cpp +++ b/Ext4Crypt.cpp @@ -20,14 +20,11 @@ #include "Utils.h" #include -#include #include #include -#include #include #include #include -#include #include #include @@ -63,7 +60,6 @@ static constexpr int FLAG_STORAGE_DE = 1 << 0; static constexpr int FLAG_STORAGE_CE = 1 << 1; namespace { -const std::chrono::seconds s_key_eviction_sleep_time(20); const std::string device_key_dir = std::string() + DATA_MNT_POINT + e4crypt_unencrypted_folder; const std::string device_key_path = device_key_dir + "/key"; @@ -77,10 +73,6 @@ bool s_global_de_initialized = false; // Some users are ephemeral, don't try to wipe their keys from disk std::set s_ephemeral_users; -// Allow evictions to be cancelled. -std::map s_desired_eviction_threads; -std::mutex s_desired_eviction_threads_mutex; - // Map user ids to key references std::map s_de_key_raw_refs; std::map s_ce_key_raw_refs; @@ -167,9 +159,6 @@ static bool install_key(const std::string& key, std::string* raw_ref) { ext4_encryption_key ext4_key; if (!fill_key(key, &ext4_key)) return false; *raw_ref = generate_key_ref(ext4_key.raw, ext4_key.size); - // Ensure that no thread is waiting to evict this ref - std::lock_guard lock(s_desired_eviction_threads_mutex); - s_desired_eviction_threads.erase(*raw_ref); auto ref = keyname(*raw_ref); key_serial_t device_keyring; if (!e4crypt_keyring(&device_keyring)) return false; @@ -537,43 +526,22 @@ bool e4crypt_vold_create_user_key(userid_t user_id, int serial, bool ephemeral) return true; } -static void evict_key_after_delay(const std::string raw_ref) { - LOG(DEBUG) << "Waiting to evict key in thread " << std::this_thread::get_id(); - std::this_thread::sleep_for(s_key_eviction_sleep_time); - LOG(DEBUG) << "Done waiting to evict key in thread " << std::this_thread::get_id(); - - std::lock_guard lock(s_desired_eviction_threads_mutex); - // Check the key should be evicted by this thread - auto search = s_desired_eviction_threads.find(raw_ref); - if (search == s_desired_eviction_threads.end()) { - LOG(DEBUG) << "Not evicting renewed-desirability key"; - return; - } - if (search->second != std::this_thread::get_id()) { - LOG(DEBUG) << "We are not the thread to evict this key"; - return; - } - - // Remove the key from the keyring +static bool evict_key(const std::string &raw_ref) { auto ref = keyname(raw_ref); key_serial_t device_keyring; - if (!e4crypt_keyring(&device_keyring)) return; + 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; - return; - } - LOG(DEBUG) << "Revoked key with serial " << key_serial; -} -static bool evict_key(const std::string &raw_ref) { - // FIXME the use of a thread with delay is a work around for a kernel bug - std::lock_guard lock(s_desired_eviction_threads_mutex); - std::thread t(evict_key_after_delay, raw_ref); - s_desired_eviction_threads[raw_ref] = t.get_id(); - LOG(DEBUG) << "Scheduled key eviction in thread " << t.get_id(); - t.detach(); - return true; // Sadly no way to know if we were successful :( + // Unlink the key from the keyring. Prefer unlinking to revoking or + // invalidating, since unlinking is actually no less secure currently, and + // it avoids bugs in certain kernel versions where the keyring key is + // referenced from places it shouldn't be. + if (keyctl_unlink(key_serial, device_keyring) != 0) { + PLOG(ERROR) << "Failed to unlink key with serial " << key_serial << " ref " << ref; + return false; + } + LOG(DEBUG) << "Unlinked key with serial " << key_serial << " ref " << ref; + return true; } static bool evict_ce_key(userid_t user_id) {