summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorEric Biggers <ebiggers@google.com>2024-03-13 19:31:42 +0000
committerEric Biggers <ebiggers@google.com>2024-03-14 17:43:49 +0000
commitb5613dae228ac193a6acfce63db81c5c4c61db9d (patch)
tree8e01c3878819544db60fa9026c607400d958eb01
parente2ce4fd6428cf719a1b28d649d0c739b98492684 (diff)
downloadsecurity-b5613dae228ac193a6acfce63db81c5c4c61db9d.tar.gz
Remove broken and unused support for expiring keys when off-body
Remove IKeystoreMaintenance#onDeviceOffBody(), as it's no longer called. In addition, remove the code that tried to enforce the AllowWhileOnBody key parameter. This code was broken during the rewrite of Keystore in Android 12, and as a result, AllowWhileOnBody has no user-visible effect. AllowWhileOnBody is *supposed* to cause the key's authentication timeout, if it has one, to automatically expire when the device is removed from the user's body. (A better name for it might have been something like UserAuthenticationExpiresWhenRemovedFromBody.) Android 11 Keystore implemented this behavior; see https://android.googlesource.com/platform/system/security/+/refs/heads/android11-release/keystore/auth_token_table.cpp#165 Android 12 Keystore changed AllowWhileOnBody to have no effect. Apparently due to a misunderstanding, the (incorrect) behavior that was attempted to be implemented was "The key may be used after authentication timeout if device is still on-body". But what was actually implemented was that the Keystore daemon stopped enforcing authentication timeouts for AllowWhileOnBody keys entirely, except after a wearable device was removed from the body in which case the timeout is enforced for any earlier authentications. Yet, this has no user-visible effect because KeyMint still enforces the authentication timeout as usual. So, AllowWhileOnBody has really been a no-op since Android 12. We can always bring this code back, fixed and with tests, if this feature comes back. But for now there is no reason to keep it around. Bug: 289849354 Test: atest -p --include-subdirs system/security/keystore2 Test: atest CtsKeystoreTestCases Change-Id: I4a7b3a90b56dacbb5316e30a30bf3fabc0debe48
-rw-r--r--keystore2/aidl/android/security/maintenance/IKeystoreMaintenance.aidl10
-rw-r--r--keystore2/src/database.rs38
-rw-r--r--keystore2/src/database/perboot.rs19
-rw-r--r--keystore2/src/enforcements.rs50
-rw-r--r--keystore2/src/globals.rs3
-rw-r--r--keystore2/src/key_parameter.rs3
-rw-r--r--keystore2/src/maintenance.rs16
-rw-r--r--keystore2/src/permission.rs5
-rw-r--r--keystore2/src/super_key.rs2
9 files changed, 30 insertions, 116 deletions
diff --git a/keystore2/aidl/android/security/maintenance/IKeystoreMaintenance.aidl b/keystore2/aidl/android/security/maintenance/IKeystoreMaintenance.aidl
index abea9582..50e98286 100644
--- a/keystore2/aidl/android/security/maintenance/IKeystoreMaintenance.aidl
+++ b/keystore2/aidl/android/security/maintenance/IKeystoreMaintenance.aidl
@@ -112,16 +112,6 @@ interface IKeystoreMaintenance {
void earlyBootEnded();
/**
- * Informs Keystore 2.0 that the an off body event was detected.
- *
- * ## Error conditions:
- * `ResponseCode::PERMISSION_DENIED` - if the caller does not have the `ReportOffBody`
- * permission.
- * `ResponseCode::SYSTEM_ERROR` - if an unexpected error occurred.
- */
- void onDeviceOffBody();
-
- /**
* Migrate a key from one namespace to another. The caller must have use, grant, and delete
* permissions on the source namespace and rebind permissions on the destination namespace.
* The source may be specified by Domain::APP, Domain::SELINUX, or Domain::KEY_ID. The target
diff --git a/keystore2/src/database.rs b/keystore2/src/database.rs
index 2757313f..f343cb33 100644
--- a/keystore2/src/database.rs
+++ b/keystore2/src/database.rs
@@ -2864,26 +2864,11 @@ impl KeystoreDB {
}
/// Find the newest auth token matching the given predicate.
- pub fn find_auth_token_entry<F>(&self, p: F) -> Option<(AuthTokenEntry, BootTime)>
+ pub fn find_auth_token_entry<F>(&self, p: F) -> Option<AuthTokenEntry>
where
F: Fn(&AuthTokenEntry) -> bool,
{
- self.perboot.find_auth_token_entry(p).map(|entry| (entry, self.get_last_off_body()))
- }
-
- /// Insert last_off_body into the metadata table at the initialization of auth token table
- pub fn insert_last_off_body(&self, last_off_body: BootTime) {
- self.perboot.set_last_off_body(last_off_body)
- }
-
- /// Update last_off_body when on_device_off_body is called
- pub fn update_last_off_body(&self, last_off_body: BootTime) {
- self.perboot.set_last_off_body(last_off_body)
- }
-
- /// Get last_off_body time when finding auth tokens
- fn get_last_off_body(&self) -> BootTime {
- self.perboot.get_last_off_body()
+ self.perboot.find_auth_token_entry(p)
}
/// Load descriptor of a key by key id
@@ -5051,23 +5036,6 @@ pub mod tests {
}
#[test]
- fn test_last_off_body() -> Result<()> {
- let mut db = new_test_db()?;
- db.insert_last_off_body(BootTime::now());
- let tx = db.conn.transaction_with_behavior(TransactionBehavior::Immediate)?;
- tx.commit()?;
- let last_off_body_1 = db.get_last_off_body();
- let one_second = Duration::from_secs(1);
- thread::sleep(one_second);
- db.update_last_off_body(BootTime::now());
- let tx2 = db.conn.transaction_with_behavior(TransactionBehavior::Immediate)?;
- tx2.commit()?;
- let last_off_body_2 = db.get_last_off_body();
- assert!(last_off_body_1 < last_off_body_2);
- Ok(())
- }
-
- #[test]
fn test_unbind_keys_for_user() -> Result<()> {
let mut db = new_test_db()?;
db.unbind_keys_for_user(1, false)?;
@@ -5491,7 +5459,7 @@ pub mod tests {
// All three entries are in the database
assert_eq!(db.perboot.auth_tokens_len(), 3);
// It selected the most recent timestamp
- assert_eq!(db.find_auth_token_entry(|_| true).unwrap().0.auth_token.mac, b"mac2".to_vec());
+ assert_eq!(db.find_auth_token_entry(|_| true).unwrap().auth_token.mac, b"mac2".to_vec());
Ok(())
}
diff --git a/keystore2/src/database/perboot.rs b/keystore2/src/database/perboot.rs
index 1b7c80d6..4727015f 100644
--- a/keystore2/src/database/perboot.rs
+++ b/keystore2/src/database/perboot.rs
@@ -13,15 +13,14 @@
// limitations under the License.
//! This module implements a per-boot, shared, in-memory storage of auth tokens
-//! and last-time-on-body for the main Keystore 2.0 database module.
+//! for the main Keystore 2.0 database module.
-use super::{AuthTokenEntry, BootTime};
+use super::AuthTokenEntry;
use android_hardware_security_keymint::aidl::android::hardware::security::keymint::{
HardwareAuthToken::HardwareAuthToken, HardwareAuthenticatorType::HardwareAuthenticatorType,
};
use lazy_static::lazy_static;
use std::collections::HashSet;
-use std::sync::atomic::{AtomicI64, Ordering};
use std::sync::Arc;
use std::sync::RwLock;
@@ -62,17 +61,13 @@ impl PartialEq<AuthTokenEntryWrap> for AuthTokenEntryWrap {
impl Eq for AuthTokenEntryWrap {}
-/// Per-boot state structure. Currently only used to track auth tokens and
-/// last-off-body.
+/// Per-boot state structure. Currently only used to track auth tokens.
#[derive(Default)]
pub struct PerbootDB {
// We can use a .unwrap() discipline on this lock, because only panicking
// while holding a .write() lock will poison it. The only write usage is
// an insert call which inserts a pre-constructed pair.
auth_tokens: RwLock<HashSet<AuthTokenEntryWrap>>,
- // Ordering::Relaxed is appropriate for accessing this atomic, since it
- // does not currently need to be synchronized with anything else.
- last_off_body: AtomicI64,
}
lazy_static! {
@@ -102,14 +97,6 @@ impl PerbootDB {
matches.sort_by_key(|x| x.0.time_received);
matches.last().map(|x| x.0.clone())
}
- /// Get the last time the device was off the user's body
- pub fn get_last_off_body(&self) -> BootTime {
- BootTime(self.last_off_body.load(Ordering::Relaxed))
- }
- /// Set the last time the device was off the user's body
- pub fn set_last_off_body(&self, last_off_body: BootTime) {
- self.last_off_body.store(last_off_body.0, Ordering::Relaxed)
- }
/// Return how many auth tokens are currently tracked.
pub fn auth_tokens_len(&self) -> usize {
self.auth_tokens.read().unwrap().len()
diff --git a/keystore2/src/enforcements.rs b/keystore2/src/enforcements.rs
index 55c9591f..95dd026d 100644
--- a/keystore2/src/enforcements.rs
+++ b/keystore2/src/enforcements.rs
@@ -476,7 +476,6 @@ impl Enforcements {
let mut user_id: i32 = -1;
let mut user_secure_ids = Vec::<i64>::new();
let mut key_time_out: Option<i64> = None;
- let mut allow_while_on_body = false;
let mut unlocked_device_required = false;
let mut key_usage_limited: Option<i64> = None;
let mut confirmation_token_receiver: Option<Arc<Mutex<Option<Receiver<Vec<u8>>>>>> = None;
@@ -533,9 +532,6 @@ impl Enforcements {
KeyParameterValue::UnlockedDeviceRequired => {
unlocked_device_required = true;
}
- KeyParameterValue::AllowWhileOnBody => {
- allow_while_on_body = true;
- }
KeyParameterValue::UsageCountLimit(_) => {
// We don't examine the limit here because this is enforced on finish.
// Instead, we store the key_id so that finish can look up the key
@@ -607,13 +603,12 @@ impl Enforcements {
let (hat, state) = if user_secure_ids.is_empty() {
(None, DeferredAuthState::NoAuthRequired)
} else if let Some(key_time_out) = key_time_out {
- let (hat, last_off_body) =
- Self::find_auth_token(|hat: &AuthTokenEntry| match user_auth_type {
- Some(auth_type) => hat.satisfies(&user_secure_ids, auth_type),
- None => false, // not reachable due to earlier check
- })
- .ok_or(Error::Km(Ec::KEY_USER_NOT_AUTHENTICATED))
- .context(ks_err!("No suitable auth token found."))?;
+ let hat = Self::find_auth_token(|hat: &AuthTokenEntry| match user_auth_type {
+ Some(auth_type) => hat.satisfies(&user_secure_ids, auth_type),
+ None => false, // not reachable due to earlier check
+ })
+ .ok_or(Error::Km(Ec::KEY_USER_NOT_AUTHENTICATED))
+ .context(ks_err!("No suitable auth token found."))?;
let now = BootTime::now();
let token_age = now
.checked_sub(&hat.time_received())
@@ -623,9 +618,7 @@ impl Enforcements {
Validity cannot be established."
))?;
- let on_body_extended = allow_while_on_body && last_off_body < hat.time_received();
-
- if token_age.seconds() > key_time_out && !on_body_extended {
+ if token_age.seconds() > key_time_out {
return Err(Error::Km(Ec::KEY_USER_NOT_AUTHENTICATED))
.context(ks_err!("matching auth token is expired."));
}
@@ -660,8 +653,8 @@ impl Enforcements {
let need_auth_token = timeout_bound || unlocked_device_required;
- let hat_and_last_off_body = if need_auth_token {
- let hat_and_last_off_body = Self::find_auth_token(|hat: &AuthTokenEntry| {
+ let hat = if need_auth_token {
+ let hat = Self::find_auth_token(|hat: &AuthTokenEntry| {
if let (Some(auth_type), true) = (user_auth_type, timeout_bound) {
hat.satisfies(&user_secure_ids, auth_type)
} else {
@@ -669,8 +662,7 @@ impl Enforcements {
}
});
Some(
- hat_and_last_off_body
- .ok_or(Error::Km(Ec::KEY_USER_NOT_AUTHENTICATED))
+ hat.ok_or(Error::Km(Ec::KEY_USER_NOT_AUTHENTICATED))
.context(ks_err!("No suitable auth token found."))?,
)
} else {
@@ -678,8 +670,8 @@ impl Enforcements {
};
// Now check the validity of the auth token if the key is timeout bound.
- let hat = match (hat_and_last_off_body, key_time_out) {
- (Some((hat, last_off_body)), Some(key_time_out)) => {
+ let hat = match (hat, key_time_out) {
+ (Some(hat), Some(key_time_out)) => {
let now = BootTime::now();
let token_age = now
.checked_sub(&hat.time_received())
@@ -689,15 +681,13 @@ impl Enforcements {
Validity cannot be established."
))?;
- let on_body_extended = allow_while_on_body && last_off_body < hat.time_received();
-
- if token_age.seconds() > key_time_out && !on_body_extended {
+ if token_age.seconds() > key_time_out {
return Err(Error::Km(Ec::KEY_USER_NOT_AUTHENTICATED))
.context(ks_err!("matching auth token is expired."));
}
Some(hat)
}
- (Some((hat, _)), None) => Some(hat),
+ (Some(hat), None) => Some(hat),
// If timeout_bound is true, above code must have retrieved a HAT or returned with
// KEY_USER_NOT_AUTHENTICATED. This arm should not be reachable.
(None, Some(_)) => panic!("Logical error."),
@@ -728,7 +718,7 @@ impl Enforcements {
})
}
- fn find_auth_token<F>(p: F) -> Option<(AuthTokenEntry, BootTime)>
+ fn find_auth_token<F>(p: F) -> Option<AuthTokenEntry>
where
F: Fn(&AuthTokenEntry) -> bool,
{
@@ -848,7 +838,7 @@ impl Enforcements {
(challenge == hat.challenge()) && hat.satisfies(&sids, auth_type)
});
- let auth_token = if let Some((auth_token_entry, _)) = result {
+ let auth_token = if let Some(auth_token_entry) = result {
auth_token_entry.take_auth_token()
} else {
// Filter the matching auth tokens by age.
@@ -863,7 +853,7 @@ impl Enforcements {
token_valid && auth_token_entry.satisfies(&sids, auth_type)
});
- if let Some((auth_token_entry, _)) = result {
+ if let Some(auth_token_entry) = result {
auth_token_entry.take_auth_token()
} else {
return Err(AuthzError::Rc(AuthzResponseCode::NO_AUTH_TOKEN_FOUND))
@@ -895,11 +885,7 @@ impl Enforcements {
let result =
Self::find_auth_token(|entry: &AuthTokenEntry| entry.satisfies(&sids, auth_type));
- if let Some((auth_token_entry, _)) = result {
- Some(auth_token_entry.time_received())
- } else {
- None
- }
+ result.map(|auth_token_entry| auth_token_entry.time_received())
}
}
diff --git a/keystore2/src/globals.rs b/keystore2/src/globals.rs
index 8535df1b..7ac1038d 100644
--- a/keystore2/src/globals.rs
+++ b/keystore2/src/globals.rs
@@ -16,6 +16,7 @@
//! database connections and connections to services that Keystore needs
//! to talk to.
+use crate::async_task::AsyncTask;
use crate::gc::Gc;
use crate::km_compat::{BacklevelKeyMintWrapper, KeyMintV1};
use crate::ks_err;
@@ -23,7 +24,6 @@ use crate::legacy_blob::LegacyBlobLoader;
use crate::legacy_importer::LegacyImporter;
use crate::super_key::SuperKeyManager;
use crate::utils::watchdog as wd;
-use crate::{async_task::AsyncTask, database::BootTime};
use crate::{
database::KeystoreDB,
database::Uuid,
@@ -68,7 +68,6 @@ pub fn create_thread_local_db() -> KeystoreDB {
DB_INIT.call_once(|| {
log::info!("Touching Keystore 2.0 database for this first time since boot.");
- db.insert_last_off_body(BootTime::now());
log::info!("Calling cleanup leftovers.");
let n = db.cleanup_leftovers().expect("Failed to cleanup database on startup.");
if n != 0 {
diff --git a/keystore2/src/key_parameter.rs b/keystore2/src/key_parameter.rs
index 02a1f16c..bd452073 100644
--- a/keystore2/src/key_parameter.rs
+++ b/keystore2/src/key_parameter.rs
@@ -912,7 +912,8 @@ pub enum KeyParameterValue {
/// The time in seconds for which the key is authorized for use, after user authentication
#[key_param(tag = AUTH_TIMEOUT, field = Integer)]
AuthTimeout(i32),
- /// The key may be used after authentication timeout if device is still on-body
+ /// The key's authentication timeout, if it has one, is automatically expired when the device is
+ /// removed from the user's body. No longer implemented; this tag is no longer enforced.
#[key_param(tag = ALLOW_WHILE_ON_BODY, field = BoolValue)]
AllowWhileOnBody,
/// The key must be unusable except when the user has provided proof of physical presence
diff --git a/keystore2/src/maintenance.rs b/keystore2/src/maintenance.rs
index 3e34cff3..8780e9e9 100644
--- a/keystore2/src/maintenance.rs
+++ b/keystore2/src/maintenance.rs
@@ -14,7 +14,7 @@
//! This module implements IKeystoreMaintenance AIDL interface.
-use crate::database::{BootTime, KeyEntryLoadBits, KeyType};
+use crate::database::{KeyEntryLoadBits, KeyType};
use crate::error::map_km_error;
use crate::error::map_or_log_err;
use crate::error::Error;
@@ -224,14 +224,6 @@ impl Maintenance {
Maintenance::call_on_all_security_levels("earlyBootEnded", |dev| dev.earlyBootEnded())
}
- fn on_device_off_body() -> Result<()> {
- // Security critical permission check. This statement must return on fail.
- check_keystore_permission(KeystorePerm::ReportOffBody).context(ks_err!())?;
-
- DB.with(|db| db.borrow_mut().update_last_off_body(BootTime::now()));
- Ok(())
- }
-
fn migrate_key_namespace(source: &KeyDescriptor, destination: &KeyDescriptor) -> Result<()> {
let calling_uid = ThreadState::get_calling_uid();
@@ -355,12 +347,6 @@ impl IKeystoreMaintenance for Maintenance {
map_or_log_err(Self::early_boot_ended(), Ok)
}
- fn onDeviceOffBody(&self) -> BinderResult<()> {
- log::info!("onDeviceOffBody()");
- let _wp = wd::watch_millis("IKeystoreMaintenance::onDeviceOffBody", 500);
- map_or_log_err(Self::on_device_off_body(), Ok)
- }
-
fn migrateKeyNamespace(
&self,
source: &KeyDescriptor,
diff --git a/keystore2/src/permission.rs b/keystore2/src/permission.rs
index bc73744f..982bc821 100644
--- a/keystore2/src/permission.rs
+++ b/keystore2/src/permission.rs
@@ -137,10 +137,7 @@ implement_class!(
/// Checked when earlyBootEnded() is called.
#[selinux(name = early_boot_ended)]
EarlyBootEnded,
- /// Checked when IKeystoreMaintenance::onDeviceOffBody is called.
- #[selinux(name = report_off_body)]
- ReportOffBody,
- /// Checked when IkeystoreMetrics::pullMetrics is called.
+ /// Checked when IKeystoreMetrics::pullMetrics is called.
#[selinux(name = pull_metrics)]
PullMetrics,
/// Checked when IKeystoreMaintenance::deleteAllKeys is called.
diff --git a/keystore2/src/super_key.rs b/keystore2/src/super_key.rs
index 44ce9abf..11ab734f 100644
--- a/keystore2/src/super_key.rs
+++ b/keystore2/src/super_key.rs
@@ -1011,7 +1011,7 @@ impl SuperKeyManager {
let mut errs = vec![];
for sid in &biometric.sids {
let sid = *sid;
- if let Some((auth_token_entry, _)) = db.find_auth_token_entry(|entry| {
+ if let Some(auth_token_entry) = db.find_auth_token_entry(|entry| {
entry.auth_token().userId == sid || entry.auth_token().authenticatorId == sid
}) {
let res: Result<(Arc<SuperKey>, Arc<SuperKey>)> = (|| {