From 5898d15dccc2547c16aad07c6ced9df1691ecd08 Mon Sep 17 00:00:00 2001 From: Janis Danisevskis Date: Tue, 15 Jun 2021 08:23:46 -0700 Subject: Keystore 2.0 legacy Keystore: Cleanup when app/user removed. Without this patch apps may leave the legacy keystore in an undefined state when uninstalled and when the UID is reused the new app would find stale entries in the legacy keystore. There is no public API to use legacy keystore, but malicious apps could use this to leave identifying information across installs. Bug: 192575371 Test: legacykeystore_test Merged-In: I06e8a4927af66092140ec84e7f5d83621cbb0b62 Change-Id: I06e8a4927af66092140ec84e7f5d83621cbb0b62 --- keystore2/legacykeystore/Android.bp | 2 + keystore2/legacykeystore/TEST_MAPPING | 7 ++ keystore2/legacykeystore/lib.rs | 186 +++++++++++++++++++++++++++++++--- keystore2/src/keystore2_main.rs | 9 +- keystore2/src/legacy_blob.rs | 57 ++++++++--- keystore2/src/maintenance.rs | 49 ++++++--- 6 files changed, 264 insertions(+), 46 deletions(-) create mode 100644 keystore2/legacykeystore/TEST_MAPPING (limited to 'keystore2') diff --git a/keystore2/legacykeystore/Android.bp b/keystore2/legacykeystore/Android.bp index c2890cc6..fb6f60f6 100644 --- a/keystore2/legacykeystore/Android.bp +++ b/keystore2/legacykeystore/Android.bp @@ -31,6 +31,7 @@ rust_library { "android.security.legacykeystore-rust", "libanyhow", "libbinder_rs", + "libcutils_bindgen", "libkeystore2", "liblog_rust", "librusqlite", @@ -48,6 +49,7 @@ rust_test { "android.security.legacykeystore-rust", "libanyhow", "libbinder_rs", + "libcutils_bindgen", "libkeystore2", "libkeystore2_test_utils", "liblog_rust", diff --git a/keystore2/legacykeystore/TEST_MAPPING b/keystore2/legacykeystore/TEST_MAPPING new file mode 100644 index 00000000..37d14398 --- /dev/null +++ b/keystore2/legacykeystore/TEST_MAPPING @@ -0,0 +1,7 @@ +{ + "presubmit": [ + { + "name": "legacykeystore_test" + } + ] +} diff --git a/keystore2/legacykeystore/lib.rs b/keystore2/legacykeystore/lib.rs index 8a4d7d82..efa0870f 100644 --- a/keystore2/legacykeystore/lib.rs +++ b/keystore2/legacykeystore/lib.rs @@ -24,10 +24,14 @@ use android_security_legacykeystore::binder::{ ThreadState, }; use anyhow::{Context, Result}; -use keystore2::{async_task::AsyncTask, legacy_blob::LegacyBlobLoader, utils::watchdog as wd}; +use keystore2::{ + async_task::AsyncTask, legacy_blob::LegacyBlobLoader, maintenance::DeleteListener, + maintenance::Domain, utils::watchdog as wd, +}; use rusqlite::{ params, Connection, OptionalExtension, Transaction, TransactionBehavior, NO_PARAMS, }; +use std::sync::Arc; use std::{ collections::HashSet, path::{Path, PathBuf}, @@ -144,6 +148,25 @@ impl DB { })?; Ok(removed == 1) } + + fn remove_uid(&mut self, uid: u32) -> Result<()> { + self.with_transaction(TransactionBehavior::Immediate, |tx| { + tx.execute("DELETE FROM profiles WHERE owner = ?;", params![uid]) + .context("In remove_uid: Failed to delete.") + })?; + Ok(()) + } + + fn remove_user(&mut self, user_id: u32) -> Result<()> { + self.with_transaction(TransactionBehavior::Immediate, |tx| { + tx.execute( + "DELETE FROM profiles WHERE cast ( ( owner/? ) as int) = ?;", + params![cutils_bindgen::AID_USER_OFFSET, user_id], + ) + .context("In remove_uid: Failed to delete.") + })?; + Ok(()) + } } /// This is the main LegacyKeystore error type, it wraps binder exceptions and the @@ -209,6 +232,19 @@ where ) } +struct LegacyKeystoreDeleteListener { + legacy_keystore: Arc, +} + +impl DeleteListener for LegacyKeystoreDeleteListener { + fn delete_namespace(&self, domain: Domain, namespace: i64) -> Result<()> { + self.legacy_keystore.delete_namespace(domain, namespace) + } + fn delete_user(&self, user_id: u32) -> Result<()> { + self.legacy_keystore.delete_user(user_id) + } +} + /// Implements ILegacyKeystore AIDL interface. pub struct LegacyKeystore { db_path: PathBuf, @@ -226,14 +262,23 @@ impl LegacyKeystore { /// It is kept for backward compatibility with early adopters. const LEGACY_KEYSTORE_FILE_NAME: &'static str = "vpnprofilestore.sqlite"; + const WIFI_NAMESPACE: i64 = 102; + const AID_WIFI: u32 = 1010; + /// Creates a new LegacyKeystore instance. - pub fn new_native_binder(path: &Path) -> Strong { + pub fn new_native_binder( + path: &Path, + ) -> (Box, Strong) { let mut db_path = path.to_path_buf(); db_path.push(Self::LEGACY_KEYSTORE_FILE_NAME); - let result = Self { db_path, async_task: Default::default() }; - result.init_shelf(path); - BnLegacyKeystore::new_binder(result, BinderFeatures::default()) + let legacy_keystore = Arc::new(Self { db_path, async_task: Default::default() }); + legacy_keystore.init_shelf(path); + let service = LegacyKeystoreService { legacy_keystore: legacy_keystore.clone() }; + ( + Box::new(LegacyKeystoreDeleteListener { legacy_keystore }), + BnLegacyKeystore::new_binder(service, BinderFeatures::default()), + ) } fn open_db(&self) -> Result { @@ -242,18 +287,17 @@ impl LegacyKeystore { fn get_effective_uid(uid: i32) -> Result { const AID_SYSTEM: u32 = 1000; - const AID_WIFI: u32 = 1010; let calling_uid = ThreadState::get_calling_uid(); let uid = uid as u32; if uid == UID_SELF as u32 || uid == calling_uid { Ok(calling_uid) - } else if calling_uid == AID_SYSTEM && uid == AID_WIFI { + } else if calling_uid == AID_SYSTEM && uid == Self::AID_WIFI { // The only exception for legacy reasons is allowing SYSTEM to access // the WIFI namespace. // IMPORTANT: If you attempt to add more exceptions, it means you are adding // more callers to this deprecated feature. DON'T! - Ok(AID_WIFI) + Ok(Self::AID_WIFI) } else { Err(Error::perm()).with_context(|| { format!("In get_effective_uid: caller: {}, requested uid: {}.", calling_uid, uid) @@ -303,6 +347,36 @@ impl LegacyKeystore { } } + fn delete_namespace(&self, domain: Domain, namespace: i64) -> Result<()> { + let uid = match domain { + Domain::APP => namespace as u32, + Domain::SELINUX => { + if namespace == Self::WIFI_NAMESPACE { + // Namespace WIFI gets mapped to AID_WIFI. + Self::AID_WIFI + } else { + // Nothing to do for any other namespace. + return Ok(()); + } + } + _ => return Ok(()), + }; + + if let Err(e) = self.bulk_delete_uid(uid) { + log::warn!("In LegacyKeystore::delete_namespace: {:?}", e); + } + let mut db = self.open_db().context("In LegacyKeystore::delete_namespace.")?; + db.remove_uid(uid).context("In LegacyKeystore::delete_namespace.") + } + + fn delete_user(&self, user_id: u32) -> Result<()> { + if let Err(e) = self.bulk_delete_user(user_id) { + log::warn!("In LegacyKeystore::delete_user: {:?}", e); + } + let mut db = self.open_db().context("In LegacyKeystore::delete_user.")?; + db.remove_user(user_id).context("In LegacyKeystore::delete_user.") + } + fn list(&self, prefix: &str, uid: i32) -> Result> { let mut db = self.open_db().context("In list.")?; let uid = Self::get_effective_uid(uid).context("In list.")?; @@ -340,7 +414,7 @@ impl LegacyKeystore { self.do_serialized(move |state| { state .legacy_loader - .list_legacy_keystore_entries(uid) + .list_legacy_keystore_entries_for_uid(uid) .context("Trying to list legacy keystore entries.") }) .context("In list_legacy.") @@ -364,6 +438,38 @@ impl LegacyKeystore { .context("In get_legacy.") } + fn bulk_delete_uid(&self, uid: u32) -> Result<()> { + self.do_serialized(move |state| { + let entries = state + .legacy_loader + .list_legacy_keystore_entries_for_uid(uid) + .context("In bulk_delete_uid: Trying to list entries.")?; + for alias in entries.iter() { + if let Err(e) = state.legacy_loader.remove_legacy_keystore_entry(uid, alias) { + log::warn!("In bulk_delete_uid: Failed to delete legacy entry. {:?}", e); + } + } + Ok(()) + }) + } + + fn bulk_delete_user(&self, user_id: u32) -> Result<()> { + self.do_serialized(move |state| { + let entries = state + .legacy_loader + .list_legacy_keystore_entries_for_user(user_id) + .context("In bulk_delete_user: Trying to list entries.")?; + for (uid, entries) in entries.iter() { + for alias in entries.iter() { + if let Err(e) = state.legacy_loader.remove_legacy_keystore_entry(*uid, alias) { + log::warn!("In bulk_delete_user: Failed to delete legacy entry. {:?}", e); + } + } + } + Ok(()) + }) + } + fn migrate_one_legacy_entry( uid: u32, alias: &str, @@ -386,24 +492,28 @@ impl LegacyKeystore { } } -impl binder::Interface for LegacyKeystore {} +struct LegacyKeystoreService { + legacy_keystore: Arc, +} -impl ILegacyKeystore for LegacyKeystore { +impl binder::Interface for LegacyKeystoreService {} + +impl ILegacyKeystore for LegacyKeystoreService { fn get(&self, alias: &str, uid: i32) -> BinderResult> { let _wp = wd::watch_millis("ILegacyKeystore::get", 500); - map_or_log_err(self.get(alias, uid), Ok) + map_or_log_err(self.legacy_keystore.get(alias, uid), Ok) } fn put(&self, alias: &str, uid: i32, entry: &[u8]) -> BinderResult<()> { let _wp = wd::watch_millis("ILegacyKeystore::put", 500); - map_or_log_err(self.put(alias, uid, entry), Ok) + map_or_log_err(self.legacy_keystore.put(alias, uid, entry), Ok) } fn remove(&self, alias: &str, uid: i32) -> BinderResult<()> { let _wp = wd::watch_millis("ILegacyKeystore::remove", 500); - map_or_log_err(self.remove(alias, uid), Ok) + map_or_log_err(self.legacy_keystore.remove(alias, uid), Ok) } fn list(&self, prefix: &str, uid: i32) -> BinderResult> { let _wp = wd::watch_millis("ILegacyKeystore::list", 500); - map_or_log_err(self.list(prefix, uid), Ok) + map_or_log_err(self.legacy_keystore.list(prefix, uid), Ok) } } @@ -465,6 +575,52 @@ mod db_test { assert_eq!(Some(TEST_BLOB4), db.get(2, "test1").expect("Failed to get entry.").as_deref()); } + #[test] + fn test_delete_uid() { + let test_dir = TempDir::new("test_delete_uid_").expect("Failed to create temp dir."); + let mut db = DB::new(&test_dir.build().push(LegacyKeystore::LEGACY_KEYSTORE_FILE_NAME)) + .expect("Failed to open database."); + + // Insert three entries for owner 2. + db.put(2, "test1", TEST_BLOB1).expect("Failed to insert test1."); + db.put(2, "test2", TEST_BLOB2).expect("Failed to insert test2."); + db.put(3, "test3", TEST_BLOB3).expect("Failed to insert test3."); + + db.remove_uid(2).expect("Failed to remove uid 2"); + + assert_eq!(Vec::::new(), db.list(2).expect("Failed to list entries.")); + + assert_eq!(vec!["test3".to_string(),], db.list(3).expect("Failed to list entries.")); + } + + #[test] + fn test_delete_user() { + let test_dir = TempDir::new("test_delete_user_").expect("Failed to create temp dir."); + let mut db = DB::new(&test_dir.build().push(LegacyKeystore::LEGACY_KEYSTORE_FILE_NAME)) + .expect("Failed to open database."); + + // Insert three entries for owner 2. + db.put(2 + 2 * cutils_bindgen::AID_USER_OFFSET, "test1", TEST_BLOB1) + .expect("Failed to insert test1."); + db.put(4 + 2 * cutils_bindgen::AID_USER_OFFSET, "test2", TEST_BLOB2) + .expect("Failed to insert test2."); + db.put(3, "test3", TEST_BLOB3).expect("Failed to insert test3."); + + db.remove_user(2).expect("Failed to remove user 2"); + + assert_eq!( + Vec::::new(), + db.list(2 + 2 * cutils_bindgen::AID_USER_OFFSET).expect("Failed to list entries.") + ); + + assert_eq!( + Vec::::new(), + db.list(4 + 2 * cutils_bindgen::AID_USER_OFFSET).expect("Failed to list entries.") + ); + + assert_eq!(vec!["test3".to_string(),], db.list(3).expect("Failed to list entries.")); + } + #[test] fn concurrent_legacy_keystore_entry_test() -> Result<()> { let temp_dir = Arc::new( diff --git a/keystore2/src/keystore2_main.rs b/keystore2/src/keystore2_main.rs index 45338c4a..dab68672 100644 --- a/keystore2/src/keystore2_main.rs +++ b/keystore2/src/keystore2_main.rs @@ -96,7 +96,11 @@ fn main() { panic!("Failed to register service {} because of {:?}.", AUTHORIZATION_SERVICE_NAME, e); }); - let maintenance_service = Maintenance::new_native_binder().unwrap_or_else(|e| { + let (delete_listener, legacykeystore) = LegacyKeystore::new_native_binder( + &keystore2::globals::DB_PATH.read().expect("Could not get DB_PATH."), + ); + + let maintenance_service = Maintenance::new_native_binder(delete_listener).unwrap_or_else(|e| { panic!("Failed to create service {} because of {:?}.", USER_MANAGER_SERVICE_NAME, e); }); binder::add_service(USER_MANAGER_SERVICE_NAME, maintenance_service.as_binder()).unwrap_or_else( @@ -120,9 +124,6 @@ fn main() { }); } - let legacykeystore = LegacyKeystore::new_native_binder( - &keystore2::globals::DB_PATH.read().expect("Could not get DB_PATH."), - ); binder::add_service(LEGACY_KEYSTORE_SERVICE_NAME, legacykeystore.as_binder()).unwrap_or_else( |e| { panic!( diff --git a/keystore2/src/legacy_blob.rs b/keystore2/src/legacy_blob.rs index e0d21334..6b16d2e0 100644 --- a/keystore2/src/legacy_blob.rs +++ b/keystore2/src/legacy_blob.rs @@ -678,7 +678,7 @@ impl LegacyBlobLoader { } /// List all entries belonging to the given uid. - pub fn list_legacy_keystore_entries(&self, uid: u32) -> Result> { + pub fn list_legacy_keystore_entries_for_uid(&self, uid: u32) -> Result> { let mut path = self.path.clone(); let user_id = uid_to_android_user(uid); path.push(format!("user_{}", user_id)); @@ -690,7 +690,7 @@ impl LegacyBlobLoader { _ => { return Err(e).context(format!( concat!( - "In list_legacy_keystore_entries: ,", + "In list_legacy_keystore_entries_for_uid: ,", "Failed to open legacy blob database: {:?}" ), path @@ -701,21 +701,54 @@ impl LegacyBlobLoader { let mut result: Vec = Vec::new(); for entry in dir { let file_name = entry - .context("In list_legacy_keystore_entries: Trying to access dir entry")? + .context("In list_legacy_keystore_entries_for_uid: Trying to access dir entry")? .file_name(); if let Some(f) = file_name.to_str() { let encoded_alias = &f[uid_str.len() + 1..]; if f.starts_with(&uid_str) && !Self::is_keystore_alias(encoded_alias) { - result.push( - Self::decode_alias(encoded_alias) - .context("In list_legacy_keystore_entries: Trying to decode alias.")?, - ) + result.push(Self::decode_alias(encoded_alias).context( + "In list_legacy_keystore_entries_for_uid: Trying to decode alias.", + )?) } } } Ok(result) } + fn extract_legacy_alias(encoded_alias: &str) -> Option { + if !Self::is_keystore_alias(encoded_alias) { + Self::decode_alias(encoded_alias).ok() + } else { + None + } + } + + /// Lists all keystore entries belonging to the given user. Returns a map of UIDs + /// to sets of decoded aliases. Only returns entries that do not begin with + /// KNOWN_KEYSTORE_PREFIXES. + pub fn list_legacy_keystore_entries_for_user( + &self, + user_id: u32, + ) -> Result>> { + let user_entries = self + .list_user(user_id) + .context("In list_legacy_keystore_entries_for_user: Trying to list user.")?; + + let result = + user_entries.into_iter().fold(HashMap::>::new(), |mut acc, v| { + if let Some(sep_pos) = v.find('_') { + if let Ok(uid) = v[0..sep_pos].parse::() { + if let Some(alias) = Self::extract_legacy_alias(&v[sep_pos + 1..]) { + let entry = acc.entry(uid).or_default(); + entry.insert(alias); + } + } + } + acc + }); + Ok(result) + } + /// This function constructs the legacy blob file name which has the form: /// user_/_. Legacy blob file names must not use /// known keystore prefixes. @@ -798,10 +831,10 @@ impl LegacyBlobLoader { .is_none()) } - fn extract_alias(encoded_alias: &str) -> Option { + fn extract_keystore_alias(encoded_alias: &str) -> Option { // We can check the encoded alias because the prefixes we are interested // in are all in the printable range that don't get mangled. - for prefix in &["USRPKEY_", "USRSKEY_", "USRCERT_", "CACERT_"] { + for prefix in Self::KNOWN_KEYSTORE_PREFIXES { if let Some(alias) = encoded_alias.strip_prefix(prefix) { return Self::decode_alias(&alias).ok(); } @@ -849,7 +882,7 @@ impl LegacyBlobLoader { user_entries.into_iter().fold(HashMap::>::new(), |mut acc, v| { if let Some(sep_pos) = v.find('_') { if let Ok(uid) = v[0..sep_pos].parse::() { - if let Some(alias) = Self::extract_alias(&v[sep_pos + 1..]) { + if let Some(alias) = Self::extract_keystore_alias(&v[sep_pos + 1..]) { let entry = acc.entry(uid).or_default(); entry.insert(alias); } @@ -877,7 +910,7 @@ impl LegacyBlobLoader { return None; } let encoded_alias = &v[uid_str.len()..]; - Self::extract_alias(encoded_alias) + Self::extract_keystore_alias(encoded_alias) }) .collect(); @@ -1388,7 +1421,7 @@ mod test { let temp_dir = TempDir::new("list_legacy_keystore_entries_on_non_existing_user")?; let legacy_blob_loader = LegacyBlobLoader::new(temp_dir.path()); - assert!(legacy_blob_loader.list_legacy_keystore_entries(20)?.is_empty()); + assert!(legacy_blob_loader.list_legacy_keystore_entries_for_user(20)?.is_empty()); Ok(()) } diff --git a/keystore2/src/maintenance.rs b/keystore2/src/maintenance.rs index 0633bc19..637fb612 100644 --- a/keystore2/src/maintenance.rs +++ b/keystore2/src/maintenance.rs @@ -32,22 +32,35 @@ use android_security_maintenance::aidl::android::security::maintenance::{ use android_security_maintenance::binder::{ BinderFeatures, Interface, Result as BinderResult, Strong, ThreadState, }; +use android_system_keystore2::aidl::android::system::keystore2::KeyDescriptor::KeyDescriptor; use android_system_keystore2::aidl::android::system::keystore2::ResponseCode::ResponseCode; -use android_system_keystore2::aidl::android::system::keystore2::{ - Domain::Domain, KeyDescriptor::KeyDescriptor, -}; use anyhow::{Context, Result}; use keystore2_crypto::Password; +/// Reexport Domain for the benefit of DeleteListener +pub use android_system_keystore2::aidl::android::system::keystore2::Domain::Domain; + +/// The Maintenance module takes a delete listener argument which observes user and namespace +/// deletion events. +pub trait DeleteListener { + /// Called by the maintenance module when an app/namespace is deleted. + fn delete_namespace(&self, domain: Domain, namespace: i64) -> Result<()>; + /// Called by the maintenance module when a user is deleted. + fn delete_user(&self, user_id: u32) -> Result<()>; +} + /// This struct is defined to implement the aforementioned AIDL interface. -/// As of now, it is an empty struct. -pub struct Maintenance; +pub struct Maintenance { + delete_listener: Box, +} impl Maintenance { - /// Create a new instance of Keystore User Manager service. - pub fn new_native_binder() -> Result> { + /// Create a new instance of Keystore Maintenance service. + pub fn new_native_binder( + delete_listener: Box, + ) -> Result> { Ok(BnKeystoreMaintenance::new_binder( - Self, + Self { delete_listener }, BinderFeatures { set_requesting_sid: true, ..BinderFeatures::default() }, )) } @@ -89,7 +102,7 @@ impl Maintenance { } } - fn add_or_remove_user(user_id: i32) -> Result<()> { + fn add_or_remove_user(&self, user_id: i32) -> Result<()> { // Check permission. Function should return if this failed. Therefore having '?' at the end // is very important. check_keystore_permission(KeystorePerm::change_user()).context("In add_or_remove_user.")?; @@ -102,10 +115,13 @@ impl Maintenance { false, ) }) - .context("In add_or_remove_user: Trying to delete keys from db.") + .context("In add_or_remove_user: Trying to delete keys from db.")?; + self.delete_listener + .delete_user(user_id as u32) + .context("In add_or_remove_user: While invoking the delete listener.") } - fn clear_namespace(domain: Domain, nspace: i64) -> Result<()> { + fn clear_namespace(&self, domain: Domain, nspace: i64) -> Result<()> { // Permission check. Must return on error. Do not touch the '?'. check_keystore_permission(KeystorePerm::clear_uid()).context("In clear_namespace.")?; @@ -113,7 +129,10 @@ impl Maintenance { .bulk_delete_uid(domain, nspace) .context("In clear_namespace: Trying to delete legacy keys.")?; DB.with(|db| db.borrow_mut().unbind_keys_for_namespace(domain, nspace)) - .context("In clear_namespace: Trying to delete keys from db.") + .context("In clear_namespace: Trying to delete keys from db.")?; + self.delete_listener + .delete_namespace(domain, nspace) + .context("In clear_namespace: While invoking the delete listener.") } fn get_state(user_id: i32) -> Result { @@ -231,17 +250,17 @@ impl IKeystoreMaintenance for Maintenance { fn onUserAdded(&self, user_id: i32) -> BinderResult<()> { let _wp = wd::watch_millis("IKeystoreMaintenance::onUserAdded", 500); - map_or_log_err(Self::add_or_remove_user(user_id), Ok) + map_or_log_err(self.add_or_remove_user(user_id), Ok) } fn onUserRemoved(&self, user_id: i32) -> BinderResult<()> { let _wp = wd::watch_millis("IKeystoreMaintenance::onUserRemoved", 500); - map_or_log_err(Self::add_or_remove_user(user_id), Ok) + map_or_log_err(self.add_or_remove_user(user_id), Ok) } fn clearNamespace(&self, domain: Domain, nspace: i64) -> BinderResult<()> { let _wp = wd::watch_millis("IKeystoreMaintenance::clearNamespace", 500); - map_or_log_err(Self::clear_namespace(domain, nspace), Ok) + map_or_log_err(self.clear_namespace(domain, nspace), Ok) } fn getState(&self, user_id: i32) -> BinderResult { -- cgit v1.2.3