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.
Ignore-AOSP-First: No mergepath from AOSP.
Bug: 192575371
Test: legacykeystore_test
Change-Id: I06e8a4927af66092140ec84e7f5d83621cbb0b62
diff --git a/keystore2/legacykeystore/Android.bp b/keystore2/legacykeystore/Android.bp
index c2890cc..fb6f60f 100644
--- a/keystore2/legacykeystore/Android.bp
+++ b/keystore2/legacykeystore/Android.bp
@@ -31,6 +31,7 @@
"android.security.legacykeystore-rust",
"libanyhow",
"libbinder_rs",
+ "libcutils_bindgen",
"libkeystore2",
"liblog_rust",
"librusqlite",
@@ -48,6 +49,7 @@
"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 0000000..37d1439
--- /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 8a4d7d8..efa0870 100644
--- a/keystore2/legacykeystore/lib.rs
+++ b/keystore2/legacykeystore/lib.rs
@@ -24,10 +24,14 @@
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 @@
})?;
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 @@
)
}
+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 @@
/// 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 @@
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 @@
}
}
+ 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 @@
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 @@
.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 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 @@
}
#[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 9535ecb..2c7d4a0 100644
--- a/keystore2/src/keystore2_main.rs
+++ b/keystore2/src/keystore2_main.rs
@@ -97,7 +97,11 @@
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 @@
});
}
- 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 e0d2133..6b16d2e 100644
--- a/keystore2/src/legacy_blob.rs
+++ b/keystore2/src/legacy_blob.rs
@@ -678,7 +678,7 @@
}
/// 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 @@
_ => {
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 @@
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 @@
.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 @@
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 @@
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 @@
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 0633bc1..637fb61 100644
--- a/keystore2/src/maintenance.rs
+++ b/keystore2/src/maintenance.rs
@@ -32,22 +32,35 @@
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 @@
}
}
- 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 @@
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 @@
.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 @@
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> {
