Merge "Enhance Security Logs for Clarity" into main
diff --git a/keystore/Android.bp b/keystore/Android.bp
index c79d00b..2d78f33 100644
--- a/keystore/Android.bp
+++ b/keystore/Android.bp
@@ -20,14 +20,14 @@
sanitize: {
misc_undefined: [
- "signed-integer-overflow",
- "unsigned-integer-overflow",
- "shift",
- "integer-divide-by-zero",
- "implicit-unsigned-integer-truncation",
- // BUG: 123630767
- //"implicit-signed-integer-truncation",
- "implicit-integer-sign-change",
+ "signed-integer-overflow",
+ "unsigned-integer-overflow",
+ "shift",
+ "integer-divide-by-zero",
+ "implicit-unsigned-integer-truncation",
+ // BUG: 123630767
+ //"implicit-signed-integer-truncation",
+ "implicit-integer-sign-change",
],
},
@@ -66,7 +66,10 @@
// in Tag::ATTESTATION_APPLICATION_ID
cc_library {
name: "libkeystore-attestation-application-id",
- defaults: ["keystore_defaults"],
+ defaults: [
+ "keystore_defaults",
+ "keystore2_use_latest_aidl_ndk_shared",
+ ],
srcs: [
"keystore_attestation_id.cpp",
diff --git a/keystore/keystore_attestation_id.cpp b/keystore/keystore_attestation_id.cpp
index 1534be1..d607fbf 100644
--- a/keystore/keystore_attestation_id.cpp
+++ b/keystore/keystore_attestation_id.cpp
@@ -29,6 +29,7 @@
#include <binder/Parcelable.h>
#include <binder/PersistableBundle.h>
+#include <aidl/android/system/keystore2/ResponseCode.h>
#include <android/security/keystore/BpKeyAttestationApplicationIdProvider.h>
#include <android/security/keystore/IKeyAttestationApplicationIdProvider.h>
#include <android/security/keystore/KeyAttestationApplicationId.h>
@@ -49,6 +50,8 @@
constexpr const char* kAttestationSystemPackageName = "AndroidSystem";
constexpr const char* kUnknownPackageName = "UnknownPackage";
+constexpr const size_t kMaxAttempts = 3;
+constexpr const unsigned long kRetryIntervalUsecs = 500000; // sleep for 500 ms
std::vector<uint8_t> signature2SHA256(const security::keystore::Signature& sig) {
std::vector<uint8_t> digest_buffer(SHA256_DIGEST_LENGTH);
@@ -56,6 +59,7 @@
return digest_buffer;
}
+using ::aidl::android::system::keystore2::ResponseCode;
using ::android::security::keystore::BpKeyAttestationApplicationIdProvider;
class KeyAttestationApplicationIdProvider : public BpKeyAttestationApplicationIdProvider {
@@ -279,17 +283,31 @@
} else {
/* Get the attestation application ID from package manager */
auto& pm = KeyAttestationApplicationIdProvider::get();
- auto status = pm.getKeyAttestationApplicationId(uid, &key_attestation_id);
+ ::android::binder::Status status;
+
+ // Retry on failure if a service specific error code.
+ for (size_t attempt{0}; attempt < kMaxAttempts; ++attempt) {
+ status = pm.getKeyAttestationApplicationId(uid, &key_attestation_id);
+ if (status.isOk()) {
+ break;
+ } else if (status.exceptionCode() != binder::Status::EX_SERVICE_SPECIFIC) {
+ ALOGW("Retry: key attestation ID failed with service specific error: %s %d",
+ status.exceptionMessage().c_str(), status.serviceSpecificErrorCode());
+ usleep(kRetryIntervalUsecs);
+ } else {
+ ALOGW("Retry: key attestation ID failed with error: %s %d",
+ status.exceptionMessage().c_str(), status.exceptionCode());
+ usleep(kRetryIntervalUsecs);
+ }
+ }
+
// Package Manager call has failed, perform attestation but indicate that the
// caller is unknown.
if (!status.isOk()) {
ALOGW("package manager request for key attestation ID failed with: %s %d",
status.exceptionMessage().c_str(), status.exceptionCode());
- auto pinfo = KeyAttestationPackageInfo();
- pinfo.packageName = String16(kUnknownPackageName);
- pinfo.versionCode = 1;
- key_attestation_id.packageInfos.push_back(std::move(pinfo));
+ return int32_t(ResponseCode::GET_ATTESTATION_APPLICATION_ID_FAILED);
}
}
diff --git a/keystore2/aidl/android/security/maintenance/IKeystoreMaintenance.aidl b/keystore2/aidl/android/security/maintenance/IKeystoreMaintenance.aidl
index 50e9828..ecc1f4b 100644
--- a/keystore2/aidl/android/security/maintenance/IKeystoreMaintenance.aidl
+++ b/keystore2/aidl/android/security/maintenance/IKeystoreMaintenance.aidl
@@ -77,21 +77,6 @@
void onUserLskfRemoved(in int userId);
/**
- * Allows LockSettingsService to inform keystore about password change of a user.
- * Callers require 'ChangePassword' permission.
- *
- * ## Error conditions:
- * `ResponseCode::PERMISSION_DENIED` - if the callers does not have the 'ChangePassword'
- * permission.
- * `ResponseCode::SYSTEM_ERROR` - if failed to delete the super encrypted keys of the user.
- * `ResponseCode::Locked' - if the keystore is locked for the given user.
- *
- * @param userId - Android user id
- * @param password - a secret derived from the synthetic password of the user
- */
- void onUserPasswordChanged(in int userId, in @nullable byte[] password);
-
- /**
* This function deletes all keys within a namespace. It mainly gets called when an app gets
* removed and all resources of this app need to be cleaned up.
*
diff --git a/keystore2/src/authorization.rs b/keystore2/src/authorization.rs
index 5a3fdbc..c76f86b 100644
--- a/keystore2/src/authorization.rs
+++ b/keystore2/src/authorization.rs
@@ -150,7 +150,7 @@
&self,
user_id: i32,
unlocking_sids: &[i64],
- mut weak_unlock_enabled: bool,
+ weak_unlock_enabled: bool,
) -> Result<()> {
log::info!(
"on_device_locked(user_id={}, unlocking_sids={:?}, weak_unlock_enabled={})",
@@ -158,9 +158,6 @@
unlocking_sids,
weak_unlock_enabled
);
- if !android_security_flags::fix_unlocked_device_required_keys_v2() {
- weak_unlock_enabled = false;
- }
check_keystore_permission(KeystorePerm::Lock)
.context(ks_err!("caller missing Lock permission"))?;
ENFORCEMENTS.set_device_locked(user_id, true);
@@ -178,9 +175,6 @@
fn on_weak_unlock_methods_expired(&self, user_id: i32) -> Result<()> {
log::info!("on_weak_unlock_methods_expired(user_id={})", user_id);
- if !android_security_flags::fix_unlocked_device_required_keys_v2() {
- return Ok(());
- }
check_keystore_permission(KeystorePerm::Lock)
.context(ks_err!("caller missing Lock permission"))?;
SUPER_KEY.write().unwrap().wipe_plaintext_unlocked_device_required_keys(user_id as u32);
@@ -189,9 +183,6 @@
fn on_non_lskf_unlock_methods_expired(&self, user_id: i32) -> Result<()> {
log::info!("on_non_lskf_unlock_methods_expired(user_id={})", user_id);
- if !android_security_flags::fix_unlocked_device_required_keys_v2() {
- return Ok(());
- }
check_keystore_permission(KeystorePerm::Lock)
.context(ks_err!("caller missing Lock permission"))?;
SUPER_KEY.write().unwrap().wipe_all_unlocked_device_required_keys(user_id as u32);
diff --git a/keystore2/src/database.rs b/keystore2/src/database.rs
index a7f6a22..ffc80c9 100644
--- a/keystore2/src/database.rs
+++ b/keystore2/src/database.rs
@@ -2371,15 +2371,8 @@
.context(ks_err!())
}
- /// Delete the keys created on behalf of the user, denoted by the user id.
- /// Delete all the keys unless 'keep_non_super_encrypted_keys' set to true.
- /// Returned boolean is to hint the garbage collector to delete the unbound keys.
- /// The caller of this function should notify the gc if the returned value is true.
- pub fn unbind_keys_for_user(
- &mut self,
- user_id: u32,
- keep_non_super_encrypted_keys: bool,
- ) -> Result<()> {
+ /// Deletes all keys for the given user, including both client keys and super keys.
+ pub fn unbind_keys_for_user(&mut self, user_id: u32) -> Result<()> {
let _wp = wd::watch("KeystoreDB::unbind_keys_for_user");
self.with_transaction(Immediate("TX_unbind_keys_for_user"), |tx| {
@@ -2427,17 +2420,6 @@
let mut notify_gc = false;
for key_id in key_ids {
- if keep_non_super_encrypted_keys {
- // Load metadata and filter out non-super-encrypted keys.
- if let (_, Some((_, blob_metadata)), _, _) =
- Self::load_blob_components(key_id, KeyEntryLoadBits::KM, tx)
- .context(ks_err!("Trying to load blob info."))?
- {
- if blob_metadata.encrypted_by().is_none() {
- continue;
- }
- }
- }
notify_gc = Self::mark_unreferenced(tx, key_id)
.context("In unbind_keys_for_user.")?
|| notify_gc;
@@ -4946,16 +4928,16 @@
#[test]
fn test_unbind_keys_for_user() -> Result<()> {
let mut db = new_test_db()?;
- db.unbind_keys_for_user(1, false)?;
+ db.unbind_keys_for_user(1)?;
make_test_key_entry(&mut db, Domain::APP, 210000, TEST_ALIAS, None)?;
make_test_key_entry(&mut db, Domain::APP, 110000, TEST_ALIAS, None)?;
- db.unbind_keys_for_user(2, false)?;
+ db.unbind_keys_for_user(2)?;
assert_eq!(1, db.list_past_alias(Domain::APP, 110000, KeyType::Client, None)?.len());
assert_eq!(0, db.list_past_alias(Domain::APP, 210000, KeyType::Client, None)?.len());
- db.unbind_keys_for_user(1, true)?;
+ db.unbind_keys_for_user(1)?;
assert_eq!(0, db.list_past_alias(Domain::APP, 110000, KeyType::Client, None)?.len());
Ok(())
@@ -5009,28 +4991,14 @@
assert!(db.load_super_key(&key_name_enc, 2)?.is_some());
assert!(db.load_super_key(&key_name_nonenc, 2)?.is_some());
- // Delete only encrypted keys.
- db.unbind_keys_for_user(1, true)?;
+ // Delete all keys for user 1.
+ db.unbind_keys_for_user(1)?;
- // The encrypted superkey should be gone now.
- assert!(db.load_super_key(&key_name_enc, 1)?.is_none());
- assert!(db.load_super_key(&key_name_nonenc, 1)?.is_some());
-
- // Reinsert the encrypted key.
- db.store_super_key(1, &key_name_enc, &encrypted_super_key, &metadata, &KeyMetaData::new())?;
-
- // Check that both can be found in the database, again..
- assert!(db.load_super_key(&key_name_enc, 1)?.is_some());
- assert!(db.load_super_key(&key_name_nonenc, 1)?.is_some());
-
- // Delete all even unencrypted keys.
- db.unbind_keys_for_user(1, false)?;
-
- // Both should be gone now.
+ // All of user 1's keys should be gone.
assert!(db.load_super_key(&key_name_enc, 1)?.is_none());
assert!(db.load_super_key(&key_name_nonenc, 1)?.is_none());
- // Check that the second pair of keys was untouched.
+ // User 2's keys should not have been touched.
assert!(db.load_super_key(&key_name_enc, 2)?.is_some());
assert!(db.load_super_key(&key_name_nonenc, 2)?.is_some());
diff --git a/keystore2/src/enforcements.rs b/keystore2/src/enforcements.rs
index 95dd026..7038323 100644
--- a/keystore2/src/enforcements.rs
+++ b/keystore2/src/enforcements.rs
@@ -50,8 +50,6 @@
enum AuthRequestState {
/// An outstanding per operation authorization request.
OpAuth,
- /// An outstanding request for per operation authorization and secure timestamp.
- TimeStampedOpAuth(Mutex<Receiver<Result<TimeStampToken, Error>>>),
/// An outstanding request for a timestamp token.
TimeStamp(Mutex<Receiver<Result<TimeStampToken, Error>>>),
}
@@ -59,8 +57,7 @@
#[derive(Debug)]
struct AuthRequest {
state: AuthRequestState,
- /// This need to be set to Some to fulfill a AuthRequestState::OpAuth or
- /// AuthRequestState::TimeStampedOpAuth.
+ /// This need to be set to Some to fulfill an AuthRequestState::OpAuth.
hat: Mutex<Option<HardwareAuthToken>>,
}
@@ -69,13 +66,6 @@
Arc::new(Self { state: AuthRequestState::OpAuth, hat: Mutex::new(None) })
}
- fn timestamped_op_auth(receiver: Receiver<Result<TimeStampToken, Error>>) -> Arc<Self> {
- Arc::new(Self {
- state: AuthRequestState::TimeStampedOpAuth(Mutex::new(receiver)),
- hat: Mutex::new(None),
- })
- }
-
fn timestamp(
hat: HardwareAuthToken,
receiver: Receiver<Result<TimeStampToken, Error>>,
@@ -100,7 +90,7 @@
.context(ks_err!("No operation auth token received."))?;
let tst = match &self.state {
- AuthRequestState::TimeStampedOpAuth(recv) | AuthRequestState::TimeStamp(recv) => {
+ AuthRequestState::TimeStamp(recv) => {
let result = recv
.lock()
.unwrap()
@@ -132,9 +122,6 @@
/// loaded from the database, but it has to be accompanied by a time stamp token to inform
/// the target KM with a different clock about the time on the authenticators.
TimeStampRequired(HardwareAuthToken),
- /// Indicates that both an operation bound auth token and a verification token are
- /// before the operation can commence.
- TimeStampedOpAuthRequired,
/// In this state the auth info is waiting for the deferred authorizations to come in.
/// We block on timestamp tokens, because we can always make progress on these requests.
/// The per-op auth tokens might never come, which means we fail if the client calls
@@ -254,16 +241,6 @@
self.state = DeferredAuthState::Waiting(auth_request);
Some(OperationChallenge { challenge })
}
- DeferredAuthState::TimeStampedOpAuthRequired => {
- let (sender, receiver) = channel::<Result<TimeStampToken, Error>>();
- let auth_request = AuthRequest::timestamped_op_auth(receiver);
- let token_receiver = TokenReceiver(Arc::downgrade(&auth_request));
- ENFORCEMENTS.register_op_auth_receiver(challenge, token_receiver);
-
- ASYNC_TASK.queue_hi(move |_| timestamp_token_request(challenge, sender));
- self.state = DeferredAuthState::Waiting(auth_request);
- Some(OperationChallenge { challenge })
- }
DeferredAuthState::TimeStampRequired(hat) => {
let hat = (*hat).clone();
let (sender, receiver) = channel::<Result<TimeStampToken, Error>>();
@@ -349,9 +326,7 @@
match &self.state {
DeferredAuthState::NoAuthRequired => Ok((None, None)),
DeferredAuthState::Token(hat, tst) => Ok((Some((*hat).clone()), (*tst).clone())),
- DeferredAuthState::OpAuthRequired
- | DeferredAuthState::TimeStampedOpAuthRequired
- | DeferredAuthState::TimeStampRequired(_) => {
+ DeferredAuthState::OpAuthRequired | DeferredAuthState::TimeStampRequired(_) => {
Err(Error::Km(ErrorCode::KEY_USER_NOT_AUTHENTICATED)).context(ks_err!(
"No operation auth token requested??? \
This should not happen."
@@ -599,123 +574,36 @@
}
}
- if android_security_flags::fix_unlocked_device_required_keys_v2() {
- let (hat, state) = if user_secure_ids.is_empty() {
- (None, DeferredAuthState::NoAuthRequired)
- } else if let Some(key_time_out) = key_time_out {
- 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())
- .ok_or_else(Error::sys)
- .context(ks_err!(
- "Overflow while computing Auth token validity. \
- Validity cannot be established."
- ))?;
+ let (hat, state) = if user_secure_ids.is_empty() {
+ (None, DeferredAuthState::NoAuthRequired)
+ } else if let Some(key_time_out) = key_time_out {
+ 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()).ok_or_else(Error::sys).context(ks_err!(
+ "Overflow while computing Auth token validity. \
+ Validity cannot be established."
+ ))?;
- if token_age.seconds() > key_time_out {
- return Err(Error::Km(Ec::KEY_USER_NOT_AUTHENTICATED))
- .context(ks_err!("matching auth token is expired."));
- }
- let state = if requires_timestamp {
- DeferredAuthState::TimeStampRequired(hat.auth_token().clone())
- } else {
- DeferredAuthState::NoAuthRequired
- };
- (Some(hat.take_auth_token()), state)
+ if token_age.seconds() > key_time_out {
+ return Err(Error::Km(Ec::KEY_USER_NOT_AUTHENTICATED))
+ .context(ks_err!("matching auth token is expired."));
+ }
+ let state = if requires_timestamp {
+ DeferredAuthState::TimeStampRequired(hat.auth_token().clone())
} else {
- (None, DeferredAuthState::OpAuthRequired)
+ DeferredAuthState::NoAuthRequired
};
- return Ok((hat, AuthInfo { state, key_usage_limited, confirmation_token_receiver }));
- }
-
- if !unlocked_device_required && no_auth_required {
- return Ok((
- None,
- AuthInfo {
- state: DeferredAuthState::NoAuthRequired,
- key_usage_limited,
- confirmation_token_receiver,
- },
- ));
- }
-
- let has_sids = !user_secure_ids.is_empty();
-
- let timeout_bound = key_time_out.is_some() && has_sids;
-
- let per_op_bound = key_time_out.is_none() && has_sids;
-
- let need_auth_token = timeout_bound || unlocked_device_required;
-
- 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 {
- unlocked_device_required
- }
- });
- Some(
- hat.ok_or(Error::Km(Ec::KEY_USER_NOT_AUTHENTICATED))
- .context(ks_err!("No suitable auth token found."))?,
- )
+ (Some(hat.take_auth_token()), state)
} else {
- None
+ (None, DeferredAuthState::OpAuthRequired)
};
-
- // Now check the validity of the auth token if the key is timeout bound.
- 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())
- .ok_or_else(Error::sys)
- .context(ks_err!(
- "Overflow while computing Auth token validity. \
- Validity cannot be established."
- ))?;
-
- 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),
- // 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."),
- _ => None,
- };
-
- Ok(match (hat, requires_timestamp, per_op_bound) {
- // Per-op-bound and Some(hat) can only happen if we are both per-op bound and unlocked
- // device required. In addition, this KM instance needs a timestamp token.
- // So the HAT cannot be presented on create. So on update/finish we present both
- // an per-op-bound auth token and a timestamp token.
- (Some(_), true, true) => (None, DeferredAuthState::TimeStampedOpAuthRequired),
- (Some(hat), true, false) => (
- Some(hat.auth_token().clone()),
- DeferredAuthState::TimeStampRequired(hat.take_auth_token()),
- ),
- (Some(hat), false, true) => {
- (Some(hat.take_auth_token()), DeferredAuthState::OpAuthRequired)
- }
- (Some(hat), false, false) => {
- (Some(hat.take_auth_token()), DeferredAuthState::NoAuthRequired)
- }
- (None, _, true) => (None, DeferredAuthState::OpAuthRequired),
- (None, _, false) => (None, DeferredAuthState::NoAuthRequired),
- })
- .map(|(hat, state)| {
- (hat, AuthInfo { state, key_usage_limited, confirmation_token_receiver })
- })
+ Ok((hat, AuthInfo { state, key_usage_limited, confirmation_token_receiver }))
}
fn find_auth_token<F>(p: F) -> Option<AuthTokenEntry>
diff --git a/keystore2/src/maintenance.rs b/keystore2/src/maintenance.rs
index 6c07f0c..43d99d1 100644
--- a/keystore2/src/maintenance.rs
+++ b/keystore2/src/maintenance.rs
@@ -22,7 +22,7 @@
use crate::globals::{DB, LEGACY_IMPORTER, SUPER_KEY};
use crate::ks_err;
use crate::permission::{KeyPerm, KeystorePerm};
-use crate::super_key::{SuperKeyManager, UserState};
+use crate::super_key::SuperKeyManager;
use crate::utils::{
check_get_app_uids_affected_by_sid_permissions, check_key_permission,
check_keystore_permission, uid_to_android_user, watchdog as wd,
@@ -69,40 +69,6 @@
))
}
- fn on_user_password_changed(user_id: i32, password: Option<Password>) -> Result<()> {
- // Check permission. Function should return if this failed. Therefore having '?' at the end
- // is very important.
- check_keystore_permission(KeystorePerm::ChangePassword).context(ks_err!())?;
-
- let mut skm = SUPER_KEY.write().unwrap();
-
- if let Some(pw) = password.as_ref() {
- DB.with(|db| {
- skm.unlock_unlocked_device_required_keys(&mut db.borrow_mut(), user_id as u32, pw)
- })
- .context(ks_err!("unlock_unlocked_device_required_keys failed"))?;
- }
-
- if let UserState::BeforeFirstUnlock = DB
- .with(|db| skm.get_user_state(&mut db.borrow_mut(), &LEGACY_IMPORTER, user_id as u32))
- .context(ks_err!("Could not get user state while changing password!"))?
- {
- // Error - password can not be changed when the device is locked
- return Err(Error::Rc(ResponseCode::LOCKED)).context(ks_err!("Device is locked."));
- }
-
- DB.with(|db| match password {
- Some(pass) => {
- skm.init_user(&mut db.borrow_mut(), &LEGACY_IMPORTER, user_id as u32, &pass)
- }
- None => {
- // User transitioned to swipe.
- skm.reset_user(&mut db.borrow_mut(), &LEGACY_IMPORTER, user_id as u32)
- }
- })
- .context(ks_err!("Failed to change user password!"))
- }
-
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.
@@ -294,17 +260,6 @@
impl Interface for Maintenance {}
impl IKeystoreMaintenance for Maintenance {
- fn onUserPasswordChanged(&self, user_id: i32, password: Option<&[u8]>) -> BinderResult<()> {
- log::info!(
- "onUserPasswordChanged(user={}, password.is_some()={})",
- user_id,
- password.is_some()
- );
- let _wp = wd::watch("IKeystoreMaintenance::onUserPasswordChanged");
- Self::on_user_password_changed(user_id, password.map(|pw| pw.into()))
- .map_err(into_logged_binder)
- }
-
fn onUserAdded(&self, user_id: i32) -> BinderResult<()> {
log::info!("onUserAdded(user={user_id})");
let _wp = wd::watch("IKeystoreMaintenance::onUserAdded");
diff --git a/keystore2/src/security_level.rs b/keystore2/src/security_level.rs
index a53ccec..b4c5ac9 100644
--- a/keystore2/src/security_level.rs
+++ b/keystore2/src/security_level.rs
@@ -443,17 +443,19 @@
// If there is an attestation challenge we need to get an application id.
if params.iter().any(|kp| kp.tag == Tag::ATTESTATION_CHALLENGE) {
- let aaid = {
- let _wp = self
- .watch("In KeystoreSecurityLevel::add_required_parameters calling: get_aaid");
- keystore2_aaid::get_aaid(uid)
- .map_err(|e| anyhow!(ks_err!("get_aaid returned status {}.", e)))
- }?;
-
- result.push(KeyParameter {
- tag: Tag::ATTESTATION_APPLICATION_ID,
- value: KeyParameterValue::Blob(aaid),
- });
+ let _wp =
+ self.watch("In KeystoreSecurityLevel::add_required_parameters calling: get_aaid");
+ match keystore2_aaid::get_aaid(uid) {
+ Ok(aaid_ok) => {
+ result.push(KeyParameter {
+ tag: Tag::ATTESTATION_APPLICATION_ID,
+ value: KeyParameterValue::Blob(aaid_ok),
+ });
+ }
+ Err(e) => {
+ return Err(anyhow!(e)).context(ks_err!("Attestation ID retrieval error."))
+ }
+ }
}
if params.iter().any(|kp| kp.tag == Tag::INCLUDE_UNIQUE_ID) {
diff --git a/keystore2/src/super_key.rs b/keystore2/src/super_key.rs
index 1f9f5f8..706a255 100644
--- a/keystore2/src/super_key.rs
+++ b/keystore2/src/super_key.rs
@@ -576,13 +576,9 @@
pw: &Password,
) -> Result<(Vec<u8>, BlobMetaData)> {
let salt = generate_salt().context("In encrypt_with_password: Failed to generate salt.")?;
- let derived_key = if android_security_flags::fix_unlocked_device_required_keys_v2() {
- pw.derive_key_hkdf(&salt, AES_256_KEY_LENGTH)
- .context(ks_err!("Failed to derive key from password."))?
- } else {
- pw.derive_key_pbkdf2(&salt, AES_256_KEY_LENGTH)
- .context(ks_err!("Failed to derive password."))?
- };
+ let derived_key = pw
+ .derive_key_hkdf(&salt, AES_256_KEY_LENGTH)
+ .context(ks_err!("Failed to derive key from password."))?;
let mut metadata = BlobMetaData::new();
metadata.add(BlobMetaEntry::EncryptedBy(EncryptedBy::Password));
metadata.add(BlobMetaEntry::Salt(salt));
@@ -879,9 +875,7 @@
) {
let entry = self.data.user_keys.entry(user_id).or_default();
if unlocking_sids.is_empty() {
- if android_security_flags::fix_unlocked_device_required_keys_v2() {
- entry.biometric_unlock = None;
- }
+ entry.biometric_unlock = None;
} else if let (Some(aes), Some(ecdh)) = (
entry.unlocked_device_required_symmetric.as_ref().cloned(),
entry.unlocked_device_required_private.as_ref().cloned(),
@@ -984,8 +978,7 @@
user_id: UserId,
) -> Result<()> {
let entry = self.data.user_keys.entry(user_id).or_default();
- if android_security_flags::fix_unlocked_device_required_keys_v2()
- && entry.unlocked_device_required_symmetric.is_some()
+ if entry.unlocked_device_required_symmetric.is_some()
&& entry.unlocked_device_required_private.is_some()
{
// If the keys are already cached in plaintext, then there is no need to decrypt the
@@ -1096,92 +1089,13 @@
legacy_importer
.bulk_delete_user(user_id, false)
.context(ks_err!("Trying to delete legacy keys."))?;
- db.unbind_keys_for_user(user_id, false).context(ks_err!("Error in unbinding keys."))?;
+ db.unbind_keys_for_user(user_id).context(ks_err!("Error in unbinding keys."))?;
// Delete super key in cache, if exists.
self.forget_all_keys_for_user(user_id);
Ok(())
}
- /// Deletes all authentication bound keys and super keys for the given user. The user must be
- /// unlocked before this function is called. This function is used to transition a user to
- /// swipe.
- pub fn reset_user(
- &mut self,
- db: &mut KeystoreDB,
- legacy_importer: &LegacyImporter,
- user_id: UserId,
- ) -> Result<()> {
- log::info!("reset_user(user={user_id})");
- match self.get_user_state(db, legacy_importer, user_id)? {
- UserState::Uninitialized => {
- Err(Error::sys()).context(ks_err!("Tried to reset an uninitialized user!"))
- }
- UserState::BeforeFirstUnlock => {
- Err(Error::sys()).context(ks_err!("Tried to reset a locked user's password!"))
- }
- UserState::AfterFirstUnlock(_) => {
- // Mark keys created on behalf of the user as unreferenced.
- legacy_importer
- .bulk_delete_user(user_id, true)
- .context(ks_err!("Trying to delete legacy keys."))?;
- db.unbind_keys_for_user(user_id, true)
- .context(ks_err!("Error in unbinding keys."))?;
-
- // Delete super key in cache, if exists.
- self.forget_all_keys_for_user(user_id);
- Ok(())
- }
- }
- }
-
- /// If the user hasn't been initialized yet, then this function generates the user's
- /// AfterFirstUnlock super key and sets the user's state to AfterFirstUnlock. Otherwise this
- /// function returns an error.
- pub fn init_user(
- &mut self,
- db: &mut KeystoreDB,
- legacy_importer: &LegacyImporter,
- user_id: UserId,
- password: &Password,
- ) -> Result<()> {
- log::info!("init_user(user={user_id})");
- match self.get_user_state(db, legacy_importer, user_id)? {
- UserState::AfterFirstUnlock(_) | UserState::BeforeFirstUnlock => {
- Err(Error::sys()).context(ks_err!("Tried to re-init an initialized user!"))
- }
- UserState::Uninitialized => {
- // Generate a new super key.
- let super_key =
- generate_aes256_key().context(ks_err!("Failed to generate AES 256 key."))?;
- // Derive an AES256 key from the password and re-encrypt the super key
- // before we insert it in the database.
- let (encrypted_super_key, blob_metadata) =
- Self::encrypt_with_password(&super_key, password)
- .context(ks_err!("Failed to encrypt super key with password!"))?;
-
- let key_entry = db
- .store_super_key(
- user_id,
- &USER_AFTER_FIRST_UNLOCK_SUPER_KEY,
- &encrypted_super_key,
- &blob_metadata,
- &KeyMetaData::new(),
- )
- .context(ks_err!("Failed to store super key."))?;
-
- self.populate_cache_from_super_key_blob(
- user_id,
- USER_AFTER_FIRST_UNLOCK_SUPER_KEY.algorithm,
- key_entry,
- password,
- )
- .context(ks_err!("Failed to initialize user!"))?;
- Ok(())
- }
- }
- }
-
/// Initializes the given user by creating their super keys, both AfterFirstUnlock and
/// UnlockedDeviceRequired. If allow_existing is true, then the user already being initialized
/// is not considered an error.
@@ -1354,7 +1268,7 @@
assert!(skm
.write()
.unwrap()
- .init_user(&mut keystore_db, &legacy_importer, USER_ID, pw)
+ .initialize_user(&mut keystore_db, &legacy_importer, USER_ID, pw, false)
.is_ok());
(skm, keystore_db, legacy_importer)
}
@@ -1405,7 +1319,7 @@
}
#[test]
- fn test_init_user() {
+ fn test_initialize_user() {
let pw: Password = generate_password_blob();
let (skm, mut keystore_db, legacy_importer) = setup_test(&pw);
assert_unlocked(
@@ -1598,98 +1512,6 @@
.unwrap());
}
- fn test_user_reset(locked: bool) {
- let pw: Password = generate_password_blob();
- let (skm, mut keystore_db, legacy_importer) = setup_test(&pw);
- assert_unlocked(
- &skm,
- &mut keystore_db,
- &legacy_importer,
- USER_ID,
- "The user was not unlocked after initialization!",
- );
-
- assert!(make_test_key_entry(
- &mut keystore_db,
- Domain::APP,
- USER_ID.into(),
- TEST_KEY_ALIAS,
- None
- )
- .is_ok());
- assert!(make_bootlevel_key_entry(
- &mut keystore_db,
- Domain::APP,
- USER_ID.into(),
- TEST_BOOT_KEY_ALIAS,
- false
- )
- .is_ok());
- assert!(keystore_db
- .key_exists(Domain::APP, USER_ID.into(), TEST_KEY_ALIAS, KeyType::Client)
- .unwrap());
- assert!(keystore_db
- .key_exists(Domain::APP, USER_ID.into(), TEST_BOOT_KEY_ALIAS, KeyType::Client)
- .unwrap());
-
- if locked {
- skm.write().unwrap().data.user_keys.clear();
- assert_locked(
- &skm,
- &mut keystore_db,
- &legacy_importer,
- USER_ID,
- "Clearing the cache did not lock the user!",
- );
- assert!(skm
- .write()
- .unwrap()
- .reset_user(&mut keystore_db, &legacy_importer, USER_ID)
- .is_err());
- assert_locked(
- &skm,
- &mut keystore_db,
- &legacy_importer,
- USER_ID,
- "User state should not have changed!",
- );
-
- // Keys should still exist.
- assert!(keystore_db
- .key_exists(Domain::APP, USER_ID.into(), TEST_KEY_ALIAS, KeyType::Client)
- .unwrap());
- assert!(keystore_db
- .key_exists(Domain::APP, USER_ID.into(), TEST_BOOT_KEY_ALIAS, KeyType::Client)
- .unwrap());
- } else {
- assert!(skm
- .write()
- .unwrap()
- .reset_user(&mut keystore_db, &legacy_importer, USER_ID)
- .is_ok());
- assert_uninitialized(
- &skm,
- &mut keystore_db,
- &legacy_importer,
- USER_ID,
- "The user was not reset!",
- );
- assert!(!skm
- .write()
- .unwrap()
- .super_key_exists_in_db_for_user(&mut keystore_db, &legacy_importer, USER_ID)
- .unwrap());
-
- // Auth bound key should no longer exist.
- assert!(!keystore_db
- .key_exists(Domain::APP, USER_ID.into(), TEST_KEY_ALIAS, KeyType::Client)
- .unwrap());
- assert!(keystore_db
- .key_exists(Domain::APP, USER_ID.into(), TEST_BOOT_KEY_ALIAS, KeyType::Client)
- .unwrap());
- }
- }
-
#[test]
fn test_remove_unlocked_user() {
test_user_removal(false);
@@ -1699,14 +1521,4 @@
fn test_remove_locked_user() {
test_user_removal(true);
}
-
- #[test]
- fn test_reset_unlocked_user() {
- test_user_reset(false);
- }
-
- #[test]
- fn test_reset_locked_user() {
- test_user_reset(true);
- }
}
diff --git a/keystore2/test_utils/authorizations.rs b/keystore2/test_utils/authorizations.rs
index 2cb2aaf..a96d994 100644
--- a/keystore2/test_utils/authorizations.rs
+++ b/keystore2/test_utils/authorizations.rs
@@ -360,6 +360,15 @@
});
self
}
+
+ /// Set unlocked-device-required
+ pub fn unlocked_device_required(mut self) -> Self {
+ self.0.push(KeyParameter {
+ tag: Tag::UNLOCKED_DEVICE_REQUIRED,
+ value: KeyParameterValue::BoolValue(true),
+ });
+ self
+ }
}
impl Deref for AuthSetBuilder {
diff --git a/keystore2/tests/Android.bp b/keystore2/tests/Android.bp
index 01ea746..ca9f5e3 100644
--- a/keystore2/tests/Android.bp
+++ b/keystore2/tests/Android.bp
@@ -13,6 +13,7 @@
// limitations under the License.
package {
+ default_team: "trendy_team_android_hardware_backed_security",
// See: http://go/android-license-faq
// A large-scale-change added 'default_applicable_licenses' to import
// all of the 'license_kinds' from "system_security_license"
@@ -38,9 +39,13 @@
rustlibs: [
"android.hardware.security.secureclock-V1-rust",
"android.security.authorization-rust",
+ "android.security.maintenance-rust",
"libaconfig_android_hardware_biometrics_rust",
+ "libandroid_logger",
+ "libandroid_security_flags_rust",
"libbinder_rs",
"libkeystore2_test_utils",
+ "liblog_rust",
"libnix",
"libopenssl",
"librustutils",
diff --git a/keystore2/tests/keystore2_client_tests.rs b/keystore2/tests/keystore2_client_tests.rs
index a0c140a..34ba81f 100644
--- a/keystore2/tests/keystore2_client_tests.rs
+++ b/keystore2/tests/keystore2_client_tests.rs
@@ -12,6 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.
+// TODO: rename modules to strip text repeated from crate name ("keystore2_client_" and "_tests").
pub mod keystore2_client_3des_key_tests;
pub mod keystore2_client_aes_key_tests;
pub mod keystore2_client_attest_key_tests;
@@ -30,3 +31,5 @@
pub mod keystore2_client_rsa_key_tests;
pub mod keystore2_client_test_utils;
pub mod keystore2_client_update_subcomponent_tests;
+
+pub mod user_auth;
diff --git a/keystore2/tests/user_auth.rs b/keystore2/tests/user_auth.rs
new file mode 100644
index 0000000..263ad56
--- /dev/null
+++ b/keystore2/tests/user_auth.rs
@@ -0,0 +1,245 @@
+// Copyright 2024, The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//! Tests for user authentication interactions (via `IKeystoreAuthorization`).
+
+use crate::keystore2_client_test_utils::BarrierReached;
+use android_security_authorization::aidl::android::security::authorization::{
+ IKeystoreAuthorization::IKeystoreAuthorization
+};
+use android_security_maintenance::aidl::android::security::maintenance::IKeystoreMaintenance::{
+ IKeystoreMaintenance,
+};
+use android_hardware_security_keymint::aidl::android::hardware::security::keymint::{
+ Algorithm::Algorithm, Digest::Digest, EcCurve::EcCurve, HardwareAuthToken::HardwareAuthToken,
+ HardwareAuthenticatorType::HardwareAuthenticatorType, SecurityLevel::SecurityLevel,
+ KeyPurpose::KeyPurpose
+};
+use android_system_keystore2::aidl::android::system::keystore2::{
+ CreateOperationResponse::CreateOperationResponse, Domain::Domain, KeyDescriptor::KeyDescriptor,
+ KeyMetadata::KeyMetadata,
+};
+use android_hardware_security_secureclock::aidl::android::hardware::security::secureclock::{
+ Timestamp::Timestamp,
+};
+use keystore2_test_utils::{
+ get_keystore_service, run_as, authorizations::AuthSetBuilder,
+};
+use log::{warn, info};
+use nix::unistd::{Gid, Uid};
+use rustutils::users::AID_USER_OFFSET;
+
+/// Test user ID.
+const TEST_USER_ID: i32 = 100;
+/// Fake password blob.
+static PASSWORD: &[u8] = &[
+ 0x42, 0x39, 0x30, 0x37, 0x44, 0x37, 0x32, 0x37, 0x39, 0x39, 0x43, 0x42, 0x39, 0x41, 0x42, 0x30,
+ 0x34, 0x31, 0x30, 0x38, 0x46, 0x44, 0x33, 0x45, 0x39, 0x42, 0x32, 0x38, 0x36, 0x35, 0x41, 0x36,
+ 0x33, 0x44, 0x42, 0x42, 0x43, 0x36, 0x33, 0x42, 0x34, 0x39, 0x37, 0x33, 0x35, 0x45, 0x41, 0x41,
+ 0x32, 0x45, 0x31, 0x35, 0x43, 0x43, 0x46, 0x32, 0x39, 0x36, 0x33, 0x34, 0x31, 0x32, 0x41, 0x39,
+];
+/// Fake SID value corresponding to Gatekeeper.
+static GK_SID: i64 = 123456;
+/// Fake SID value corresponding to a biometric authenticator.
+static BIO_SID1: i64 = 345678;
+/// Fake SID value corresponding to a biometric authenticator.
+static BIO_SID2: i64 = 456789;
+
+const WEAK_UNLOCK_ENABLED: bool = true;
+const WEAK_UNLOCK_DISABLED: bool = false;
+const UNFORCED: bool = false;
+
+fn get_authorization() -> binder::Strong<dyn IKeystoreAuthorization> {
+ binder::get_interface("android.security.authorization").unwrap()
+}
+
+fn get_maintenance() -> binder::Strong<dyn IKeystoreMaintenance> {
+ binder::get_interface("android.security.maintenance").unwrap()
+}
+
+fn abort_op(result: binder::Result<CreateOperationResponse>) {
+ if let Ok(rsp) = result {
+ if let Some(op) = rsp.iOperation {
+ if let Err(e) = op.abort() {
+ warn!("abort op failed: {e:?}");
+ }
+ } else {
+ warn!("can't abort op with missing iOperation");
+ }
+ } else {
+ warn!("can't abort failed op: {result:?}");
+ }
+}
+
+/// RAII structure to ensure that test users are removed at the end of a test.
+struct TestUser {
+ id: i32,
+ maint: binder::Strong<dyn IKeystoreMaintenance>,
+}
+
+impl TestUser {
+ fn new() -> Self {
+ Self::new_user(TEST_USER_ID, PASSWORD)
+ }
+ fn new_user(user_id: i32, password: &[u8]) -> Self {
+ let maint = get_maintenance();
+ maint.onUserAdded(user_id).expect("failed to add test user");
+ maint
+ .initUserSuperKeys(user_id, password, /* allowExisting= */ false)
+ .expect("failed to init test user");
+ Self { id: user_id, maint }
+ }
+}
+
+impl Drop for TestUser {
+ fn drop(&mut self) {
+ let _ = self.maint.onUserRemoved(self.id);
+ }
+}
+
+#[test]
+fn keystore2_test_unlocked_device_required() {
+ android_logger::init_once(
+ android_logger::Config::default()
+ .with_tag("keystore2_client_tests")
+ .with_max_level(log::LevelFilter::Debug),
+ );
+ static CTX: &str = "u:r:untrusted_app:s0:c91,c256,c10,c20";
+ const UID: u32 = TEST_USER_ID as u32 * AID_USER_OFFSET + 1001;
+
+ // Safety: only one thread at this point, and nothing yet done with binder.
+ let mut child_handle = unsafe {
+ // Perform keystore actions while running as the test user.
+ run_as::run_as_child(
+ CTX,
+ Uid::from_raw(UID),
+ Gid::from_raw(UID),
+ move |reader, writer| -> Result<(), String> {
+ // Now we're in a new process, wait to be notified before starting.
+ reader.recv();
+
+ // Action A: create a new unlocked-device-required key (which thus requires
+ // super-encryption), while the device is unlocked.
+ let ks2 = get_keystore_service();
+ let sec_level = ks2.getSecurityLevel(SecurityLevel::TRUSTED_ENVIRONMENT).unwrap();
+ let params = AuthSetBuilder::new()
+ .unlocked_device_required()
+ .algorithm(Algorithm::EC)
+ .purpose(KeyPurpose::SIGN)
+ .purpose(KeyPurpose::VERIFY)
+ .digest(Digest::SHA_2_256)
+ .ec_curve(EcCurve::P_256);
+
+ let KeyMetadata { key, .. } = sec_level
+ .generateKey(
+ &KeyDescriptor {
+ domain: Domain::APP,
+ nspace: -1,
+ alias: Some("unlocked-device-required".to_string()),
+ blob: None,
+ },
+ None,
+ ¶ms,
+ 0,
+ b"entropy",
+ )
+ .expect("key generation failed");
+ info!("A: created unlocked-device-required key while unlocked {key:?}");
+ writer.send(&BarrierReached {}); // A done.
+
+ // Action B: fail to use the unlocked-device-required key while locked.
+ reader.recv();
+ let params =
+ AuthSetBuilder::new().purpose(KeyPurpose::SIGN).digest(Digest::SHA_2_256);
+ let result = sec_level.createOperation(&key, ¶ms, UNFORCED);
+ info!("B: use unlocked-device-required key while locked => {result:?}");
+ assert!(result.is_err());
+ writer.send(&BarrierReached {}); // B done.
+
+ // Action C: try to use the unlocked-device-required key while unlocked with a
+ // password.
+ reader.recv();
+ let result = sec_level.createOperation(&key, ¶ms, UNFORCED);
+ info!("C: use unlocked-device-required key while lskf-unlocked => {result:?}");
+ assert!(result.is_ok(), "failed with {result:?}");
+ abort_op(result);
+ writer.send(&BarrierReached {}); // C done.
+
+ // Action D: try to use the unlocked-device-required key while unlocked with a weak
+ // biometric.
+ reader.recv();
+ let result = sec_level.createOperation(&key, ¶ms, UNFORCED);
+ info!("D: use unlocked-device-required key while weak-locked => {result:?}");
+ assert!(result.is_ok(), "createOperation failed: {result:?}");
+ abort_op(result);
+ writer.send(&BarrierReached {}); // D done.
+
+ let _ = sec_level.deleteKey(&key);
+ Ok(())
+ },
+ )
+ }
+ .unwrap();
+
+ // Now that the separate process has been forked off, it's safe to use binder.
+ let user = TestUser::new();
+ let user_id = user.id;
+ let auth_service = get_authorization();
+
+ // Lock and unlock to ensure super keys are already created.
+ auth_service.onDeviceLocked(user_id, &[BIO_SID1, BIO_SID2], WEAK_UNLOCK_DISABLED).unwrap();
+ auth_service.onDeviceUnlocked(user_id, Some(PASSWORD)).unwrap();
+ auth_service.addAuthToken(&fake_lskf_token(GK_SID)).unwrap();
+
+ info!("trigger child process action A while unlocked and wait for completion");
+ child_handle.send(&BarrierReached {});
+ child_handle.recv();
+
+ // Move to locked and don't allow weak unlock, so super keys are wiped.
+ auth_service.onDeviceLocked(user_id, &[BIO_SID1, BIO_SID2], WEAK_UNLOCK_DISABLED).unwrap();
+
+ info!("trigger child process action B while locked and wait for completion");
+ child_handle.send(&BarrierReached {});
+ child_handle.recv();
+
+ // Unlock with password => loads super key from database.
+ auth_service.onDeviceUnlocked(user_id, Some(PASSWORD)).unwrap();
+ auth_service.addAuthToken(&fake_lskf_token(GK_SID)).unwrap();
+
+ info!("trigger child process action C while lskf-unlocked and wait for completion");
+ child_handle.send(&BarrierReached {});
+ child_handle.recv();
+
+ // Move to locked and allow weak unlock, then do a weak unlock.
+ auth_service.onDeviceLocked(user_id, &[BIO_SID1, BIO_SID2], WEAK_UNLOCK_ENABLED).unwrap();
+ auth_service.onDeviceUnlocked(user_id, None).unwrap();
+
+ info!("trigger child process action D while weak-unlocked and wait for completion");
+ child_handle.send(&BarrierReached {});
+ child_handle.recv();
+
+ assert_eq!(child_handle.get_result(), Ok(()), "child process failed");
+}
+
+/// Generate a fake [`HardwareAuthToken`] for the given sid.
+fn fake_lskf_token(gk_sid: i64) -> HardwareAuthToken {
+ HardwareAuthToken {
+ challenge: 0,
+ userId: gk_sid,
+ authenticatorId: 0,
+ authenticatorType: HardwareAuthenticatorType::PASSWORD,
+ timestamp: Timestamp { milliSeconds: 123 },
+ mac: vec![1, 2, 3],
+ }
+}