diff options
author | Janis Danisevskis <jdanis@google.com> | 2021-07-02 16:25:36 +0000 |
---|---|---|
committer | Android (Google) Code Review <android-gerrit@google.com> | 2021-07-02 16:25:36 +0000 |
commit | d76e8e1c98708a79a25026819a8e0fe24ab8d93e (patch) | |
tree | 05b4bbc3c453d2df5dd2237bc0103c890c0767ca | |
parent | 691d4d4dfb8fb174c336761b66c6b79fb08dbac4 (diff) | |
parent | cbc2860dd42f0b7bc0b1af2323696524183f4866 (diff) | |
download | security-d76e8e1c98708a79a25026819a8e0fe24ab8d93e.tar.gz |
Merge "Keystore 2.0 legacy Keystore: Cleanup when app/user removed." into sc-dev
-rw-r--r-- | keystore2/legacykeystore/Android.bp | 2 | ||||
-rw-r--r-- | keystore2/legacykeystore/TEST_MAPPING | 7 | ||||
-rw-r--r-- | keystore2/legacykeystore/lib.rs | 186 | ||||
-rw-r--r-- | keystore2/src/keystore2_main.rs | 9 | ||||
-rw-r--r-- | keystore2/src/legacy_blob.rs | 57 | ||||
-rw-r--r-- | keystore2/src/maintenance.rs | 49 |
6 files changed, 264 insertions, 46 deletions
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<LegacyKeystore>, +} + +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<dyn ILegacyKeystore> { + pub fn new_native_binder( + path: &Path, + ) -> (Box<dyn DeleteListener + Send + Sync + 'static>, Strong<dyn ILegacyKeystore>) { 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<DB> { @@ -242,18 +287,17 @@ impl LegacyKeystore { fn get_effective_uid(uid: i32) -> Result<u32> { 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<Vec<String>> { 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<LegacyKeystore>, +} -impl ILegacyKeystore for LegacyKeystore { +impl binder::Interface for LegacyKeystoreService {} + +impl ILegacyKeystore for LegacyKeystoreService { fn get(&self, alias: &str, uid: i32) -> BinderResult<Vec<u8>> { 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<Vec<String>> { 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) } } @@ -466,6 +576,52 @@ mod db_test { } #[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::<String>::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::<String>::new(), + db.list(2 + 2 * cutils_bindgen::AID_USER_OFFSET).expect("Failed to list entries.") + ); + + assert_eq!( + Vec::<String>::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( TempDir::new("concurrent_legacy_keystore_entry_test_") diff --git a/keystore2/src/keystore2_main.rs b/keystore2/src/keystore2_main.rs index 9535ecb8..2c7d4a00 100644 --- a/keystore2/src/keystore2_main.rs +++ b/keystore2/src/keystore2_main.rs @@ -97,7 +97,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( @@ -128,9 +132,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<Vec<String>> { + pub fn list_legacy_keystore_entries_for_uid(&self, uid: u32) -> Result<Vec<String>> { 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<String> = 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<String> { + 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<HashMap<u32, HashSet<String>>> { + 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::<u32, HashSet<String>>::new(), |mut acc, v| { + if let Some(sep_pos) = v.find('_') { + if let Ok(uid) = v[0..sep_pos].parse::<u32>() { + 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_<android user id>/<uid>_<alias>. 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<String> { + fn extract_keystore_alias(encoded_alias: &str) -> Option<String> { // 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::<u32, HashSet<String>>::new(), |mut acc, v| { if let Some(sep_pos) = v.find('_') { if let Ok(uid) = v[0..sep_pos].parse::<u32>() { - 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<dyn DeleteListener + Send + Sync + 'static>, +} impl Maintenance { - /// Create a new instance of Keystore User Manager service. - pub fn new_native_binder() -> Result<Strong<dyn IKeystoreMaintenance>> { + /// Create a new instance of Keystore Maintenance service. + pub fn new_native_binder( + delete_listener: Box<dyn DeleteListener + Send + Sync + 'static>, + ) -> Result<Strong<dyn IKeystoreMaintenance>> { 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<AidlUserState> { @@ -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<AidlUserState> { |