Sk VTS: Policy gating & Out of Seq req rejection
1. Add a test to check Access control in Secretkeeper HAL: Construct
dice chains with different security version and check that the secret
is accessible with upgraded dice chain && DicePolicyError is thrown
when the access is requested using a dice chain with lower security
version.
2. Re-enable test #replay_protection_out_of_seq_req_not_accepted - This
was disabled because the method would not panic when run on
devices without Sk HAL, causing test failure. Refactor the test to
check the error instead of unwrapping.
Many methods of SkClient struct are refactored to return Error (also
introduced in this patch) instead of panicking on error.
Test: atest VtsSecretkeeperTargetTest
Bug: 291224769
Bug: 317416663
Change-Id: I646783e034064f81625a978a2bcacf143ff60c87
diff --git a/security/secretkeeper/aidl/vts/secretkeeper_test_client.rs b/security/secretkeeper/aidl/vts/secretkeeper_test_client.rs
index a62546d..4c0f659 100644
--- a/security/secretkeeper/aidl/vts/secretkeeper_test_client.rs
+++ b/security/secretkeeper/aidl/vts/secretkeeper_test_client.rs
@@ -113,8 +113,8 @@
/// This method is wrapper that use `SkSession::secret_management_request` which handles
/// encryption and decryption.
- fn secret_management_request(&mut self, req_data: &[u8]) -> Vec<u8> {
- self.session.secret_management_request(req_data).unwrap()
+ fn secret_management_request(&mut self, req_data: &[u8]) -> Result<Vec<u8>, Error> {
+ Ok(self.session.secret_management_request(req_data)?)
}
/// Unlike the method [`secret_management_request`], this method directly uses
@@ -125,7 +125,7 @@
req_data: &[u8],
req_aad: &[u8],
expected_res_aad: &[u8],
- ) -> Vec<u8> {
+ ) -> Result<Vec<u8>, Error> {
let aes_gcm = boring::BoringAes;
let rng = boring::BoringRng;
let request_bytes = cipher::encrypt_message(
@@ -136,53 +136,54 @@
&req_data,
req_aad,
)
- .unwrap();
+ .map_err(|e| secretkeeper_client::Error::CipherError(e))?;
// Binder call!
- let response_bytes = self.sk.processSecretManagementRequest(&request_bytes).unwrap();
+ let response_bytes = self.sk.processSecretManagementRequest(&request_bytes)?;
- let response_encrypt0 = CoseEncrypt0::from_slice(&response_bytes).unwrap();
- cipher::decrypt_message(
+ let response_encrypt0 = CoseEncrypt0::from_slice(&response_bytes)?;
+ Ok(cipher::decrypt_message(
&aes_gcm,
self.session.decryption_key(),
&response_encrypt0,
expected_res_aad,
)
- .unwrap()
+ .map_err(|e| secretkeeper_client::Error::CipherError(e))?)
}
/// Helper method to store a secret. This uses the default compatible sealing_policy on
/// dice_chain.
- fn store(&mut self, id: &Id, secret: &Secret) {
- let sealing_policy = sealing_policy(self.dice_artifacts.explicit_key_dice_chain().unwrap());
+ fn store(&mut self, id: &Id, secret: &Secret) -> Result<(), Error> {
+ let sealing_policy = sealing_policy(
+ self.dice_artifacts.explicit_key_dice_chain().ok_or(Error::UnexpectedError)?,
+ );
let store_request =
StoreSecretRequest { id: id.clone(), secret: secret.clone(), sealing_policy };
- let store_request = store_request.serialize_to_packet().to_vec().unwrap();
+ let store_request = store_request.serialize_to_packet().to_vec()?;
- let store_response = self.secret_management_request(&store_request);
- let store_response = ResponsePacket::from_slice(&store_response).unwrap();
+ let store_response = self.secret_management_request(&store_request)?;
+ let store_response = ResponsePacket::from_slice(&store_response)?;
- assert_eq!(store_response.response_type().unwrap(), ResponseType::Success);
+ assert_eq!(store_response.response_type()?, ResponseType::Success);
// Really just checking that the response is indeed StoreSecretResponse
- let _ = StoreSecretResponse::deserialize_from_packet(store_response).unwrap();
+ let _ = StoreSecretResponse::deserialize_from_packet(store_response)?;
+ Ok(())
}
/// Helper method to get a secret.
- fn get(&mut self, id: &Id) -> Option<Secret> {
+ fn get(&mut self, id: &Id) -> Result<Secret, Error> {
let get_request = GetSecretRequest { id: id.clone(), updated_sealing_policy: None };
- let get_request = get_request.serialize_to_packet().to_vec().unwrap();
+ let get_request = get_request.serialize_to_packet().to_vec()?;
- let get_response = self.secret_management_request(&get_request);
- let get_response = ResponsePacket::from_slice(&get_response).unwrap();
+ let get_response = self.secret_management_request(&get_request)?;
+ let get_response = ResponsePacket::from_slice(&get_response)?;
- if get_response.response_type().unwrap() == ResponseType::Success {
- let get_response = *GetSecretResponse::deserialize_from_packet(get_response).unwrap();
- Some(Secret(get_response.secret.0))
+ if get_response.response_type()? == ResponseType::Success {
+ let get_response = *GetSecretResponse::deserialize_from_packet(get_response)?;
+ Ok(Secret(get_response.secret.0))
} else {
- // Only expect a not-found failure.
- let err = *SecretkeeperError::deserialize_from_packet(get_response).unwrap();
- assert_eq!(err, SecretkeeperError::EntryNotFound);
- None
+ let err = *SecretkeeperError::deserialize_from_packet(get_response)?;
+ Err(Error::SecretkeeperError(err))
}
}
@@ -198,6 +199,50 @@
}
}
+#[derive(Debug)]
+enum Error {
+ // Errors from Secretkeeper API errors. These are thrown by core SecretManagement and
+ // not visible without decryption.
+ SecretkeeperError(SecretkeeperError),
+ InfraError(secretkeeper_client::Error),
+ UnexpectedError,
+}
+
+impl From<secretkeeper_client::Error> for Error {
+ fn from(e: secretkeeper_client::Error) -> Self {
+ Self::InfraError(e)
+ }
+}
+
+impl From<SecretkeeperError> for Error {
+ fn from(e: SecretkeeperError) -> Self {
+ Self::SecretkeeperError(e)
+ }
+}
+
+impl From<coset::CoseError> for Error {
+ fn from(e: coset::CoseError) -> Self {
+ Self::InfraError(secretkeeper_client::Error::from(e))
+ }
+}
+
+impl From<binder::Status> for Error {
+ fn from(s: binder::Status) -> Self {
+ Self::InfraError(secretkeeper_client::Error::from(s))
+ }
+}
+
+impl From<secretkeeper_comm::data_types::error::Error> for Error {
+ fn from(e: secretkeeper_comm::data_types::error::Error) -> Self {
+ Self::InfraError(secretkeeper_client::Error::from(e))
+ }
+}
+
+// Assert that the error is EntryNotFound
+fn assert_entry_not_found(res: Result<Secret, Error>) {
+ assert!(matches!(res.unwrap_err(), Error::SecretkeeperError(SecretkeeperError::EntryNotFound)))
+}
+
/// Construct a sealing policy on the dice chain. This method uses the following set of
/// constraints which are compatible with sample DICE chains used in VTS.
/// 1. ExactMatch on AUTHORITY_HASH (non-optional).
@@ -272,7 +317,7 @@
let request_packet = request.serialize_to_packet();
let request_bytes = request_packet.to_vec().unwrap();
- let response_bytes = sk_client.secret_management_request(&request_bytes);
+ let response_bytes = sk_client.secret_management_request(&request_bytes).unwrap();
let response_packet = ResponsePacket::from_slice(&response_bytes).unwrap();
assert_eq!(response_packet.response_type().unwrap(), ResponseType::Success);
@@ -292,7 +337,7 @@
// Deform the request
request_bytes[0] = !request_bytes[0];
- let response_bytes = sk_client.secret_management_request(&request_bytes);
+ let response_bytes = sk_client.secret_management_request(&request_bytes).unwrap();
let response_packet = ResponsePacket::from_slice(&response_bytes).unwrap();
assert_eq!(response_packet.response_type().unwrap(), ResponseType::Error);
@@ -304,10 +349,10 @@
fn secret_management_store_get_secret_found(instance: String) {
let mut sk_client = SkClient::new(&instance);
- sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE);
+ sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE).unwrap();
// Get the secret that was just stored
- assert_eq!(sk_client.get(&ID_EXAMPLE), Some(SECRET_EXAMPLE));
+ assert_eq!(sk_client.get(&ID_EXAMPLE).unwrap(), SECRET_EXAMPLE);
}
#[rdroidtest(get_instances())]
@@ -315,64 +360,63 @@
let mut sk_client = SkClient::new(&instance);
// Store a secret (corresponding to an id).
- sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE);
+ sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE).unwrap();
// Get the secret that was never stored
- assert_eq!(sk_client.get(&ID_NOT_STORED), None);
+ assert_entry_not_found(sk_client.get(&ID_NOT_STORED));
}
#[rdroidtest(get_instances())]
fn secretkeeper_store_delete_ids(instance: String) {
let mut sk_client = SkClient::new(&instance);
- sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE);
- sk_client.store(&ID_EXAMPLE_2, &SECRET_EXAMPLE);
+ sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE).unwrap();
+ sk_client.store(&ID_EXAMPLE_2, &SECRET_EXAMPLE).unwrap();
sk_client.delete(&[&ID_EXAMPLE]);
-
- assert_eq!(sk_client.get(&ID_EXAMPLE), None);
- assert_eq!(sk_client.get(&ID_EXAMPLE_2), Some(SECRET_EXAMPLE));
+ assert_entry_not_found(sk_client.get(&ID_EXAMPLE));
+ assert_eq!(sk_client.get(&ID_EXAMPLE_2).unwrap(), SECRET_EXAMPLE);
sk_client.delete(&[&ID_EXAMPLE_2]);
- assert_eq!(sk_client.get(&ID_EXAMPLE), None);
- assert_eq!(sk_client.get(&ID_EXAMPLE_2), None);
+ assert_entry_not_found(sk_client.get(&ID_EXAMPLE));
+ assert_entry_not_found(sk_client.get(&ID_EXAMPLE_2));
}
#[rdroidtest(get_instances())]
fn secretkeeper_store_delete_multiple_ids(instance: String) {
let mut sk_client = SkClient::new(&instance);
- sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE);
- sk_client.store(&ID_EXAMPLE_2, &SECRET_EXAMPLE);
+ sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE).unwrap();
+ sk_client.store(&ID_EXAMPLE_2, &SECRET_EXAMPLE).unwrap();
sk_client.delete(&[&ID_EXAMPLE, &ID_EXAMPLE_2]);
- assert_eq!(sk_client.get(&ID_EXAMPLE), None);
- assert_eq!(sk_client.get(&ID_EXAMPLE_2), None);
+ assert_entry_not_found(sk_client.get(&ID_EXAMPLE));
+ assert_entry_not_found(sk_client.get(&ID_EXAMPLE_2));
}
#[rdroidtest(get_instances())]
fn secretkeeper_store_delete_duplicate_ids(instance: String) {
let mut sk_client = SkClient::new(&instance);
- sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE);
- sk_client.store(&ID_EXAMPLE_2, &SECRET_EXAMPLE);
+ sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE).unwrap();
+ sk_client.store(&ID_EXAMPLE_2, &SECRET_EXAMPLE).unwrap();
// Delete the same secret twice.
sk_client.delete(&[&ID_EXAMPLE, &ID_EXAMPLE]);
- assert_eq!(sk_client.get(&ID_EXAMPLE), None);
- assert_eq!(sk_client.get(&ID_EXAMPLE_2), Some(SECRET_EXAMPLE));
+ assert_entry_not_found(sk_client.get(&ID_EXAMPLE));
+ assert_eq!(sk_client.get(&ID_EXAMPLE_2).unwrap(), SECRET_EXAMPLE);
}
#[rdroidtest(get_instances())]
fn secretkeeper_store_delete_nonexistent(instance: String) {
let mut sk_client = SkClient::new(&instance);
- sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE);
- sk_client.store(&ID_EXAMPLE_2, &SECRET_EXAMPLE);
+ sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE).unwrap();
+ sk_client.store(&ID_EXAMPLE_2, &SECRET_EXAMPLE).unwrap();
sk_client.delete(&[&ID_NOT_STORED]);
- assert_eq!(sk_client.get(&ID_EXAMPLE), Some(SECRET_EXAMPLE));
- assert_eq!(sk_client.get(&ID_EXAMPLE_2), Some(SECRET_EXAMPLE));
- assert_eq!(sk_client.get(&ID_NOT_STORED), None);
+ assert_eq!(sk_client.get(&ID_EXAMPLE).unwrap(), SECRET_EXAMPLE);
+ assert_eq!(sk_client.get(&ID_EXAMPLE_2).unwrap(), SECRET_EXAMPLE);
+ assert_entry_not_found(sk_client.get(&ID_NOT_STORED));
}
// Don't run deleteAll() on a secure device, as it might affect real secrets.
@@ -381,22 +425,22 @@
fn secretkeeper_store_delete_all(instance: String) {
let mut sk_client = SkClient::new(&instance);
- sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE);
- sk_client.store(&ID_EXAMPLE_2, &SECRET_EXAMPLE);
+ sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE).unwrap();
+ sk_client.store(&ID_EXAMPLE_2, &SECRET_EXAMPLE).unwrap();
sk_client.delete_all();
- assert_eq!(sk_client.get(&ID_EXAMPLE), None);
- assert_eq!(sk_client.get(&ID_EXAMPLE_2), None);
+ assert_entry_not_found(sk_client.get(&ID_EXAMPLE));
+ assert_entry_not_found(sk_client.get(&ID_EXAMPLE_2));
// Store a new secret (corresponding to an id).
- sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE);
+ sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE).unwrap();
// Get the restored secret.
- assert_eq!(sk_client.get(&ID_EXAMPLE), Some(SECRET_EXAMPLE));
+ assert_eq!(sk_client.get(&ID_EXAMPLE).unwrap(), SECRET_EXAMPLE);
// (Try to) Get the secret that was never stored
- assert_eq!(sk_client.get(&ID_NOT_STORED), None);
+ assert_entry_not_found(sk_client.get(&ID_NOT_STORED));
}
// This test checks that Secretkeeper uses the expected [`RequestSeqNum`] as aad while
@@ -416,21 +460,21 @@
// Check first request/response is successful
let res = ResponsePacket::from_slice(
- &sk_client.secret_management_request_custom_aad(&req_1, &seq_0, &seq_0),
+ &sk_client.secret_management_request_custom_aad(&req_1, &seq_0, &seq_0).unwrap(),
)
.unwrap();
assert_eq!(res.response_type().unwrap(), ResponseType::Success);
// Check 2nd request/response is successful
let res = ResponsePacket::from_slice(
- &sk_client.secret_management_request_custom_aad(&req_2, &seq_1, &seq_1),
+ &sk_client.secret_management_request_custom_aad(&req_2, &seq_1, &seq_1).unwrap(),
)
.unwrap();
assert_eq!(res.response_type().unwrap(), ResponseType::Success);
// Check 3rd request/response is successful
let res = ResponsePacket::from_slice(
- &sk_client.secret_management_request_custom_aad(&req_3, &seq_2, &seq_2),
+ &sk_client.secret_management_request_custom_aad(&req_3, &seq_2, &seq_2).unwrap(),
)
.unwrap();
assert_eq!(res.response_type().unwrap(), ResponseType::Success);
@@ -452,7 +496,7 @@
let seq_0 = seq_a.get_then_increment().unwrap();
// Check first request/response is successful
let res = ResponsePacket::from_slice(
- &sk_client.secret_management_request_custom_aad(&req_1, &seq_0, &seq_0),
+ &sk_client.secret_management_request_custom_aad(&req_1, &seq_0, &seq_0).unwrap(),
)
.unwrap();
assert_eq!(res.response_type().unwrap(), ResponseType::Success);
@@ -461,7 +505,7 @@
let sk_client_diff = SkClient::new(&instance);
// Check first request/response is with seq_0 is successful
let res = ResponsePacket::from_slice(
- &sk_client_diff.secret_management_request_custom_aad(&req_1, &seq_0, &seq_0),
+ &sk_client_diff.secret_management_request_custom_aad(&req_1, &seq_0, &seq_0).unwrap(),
)
.unwrap();
assert_eq!(res.response_type().unwrap(), ResponseType::Success);
@@ -470,7 +514,6 @@
// This test checks that Secretkeeper rejects requests with out of order [`RequestSeqNum`]
// TODO(b/317416663): This test fails, when HAL is not present in the device. Refactor to fix this.
#[rdroidtest(get_instances())]
-#[ignore]
fn secret_management_replay_protection_out_of_seq_req_not_accepted(instance: String) {
let dice_chain = make_explicit_owned_dice(/*Security version in a node */ 5);
let sealing_policy = sealing_policy(dice_chain.explicit_key_dice_chain().unwrap());
@@ -484,11 +527,36 @@
let [seq_0, seq_1, seq_2] = std::array::from_fn(|_| seq_a.get_then_increment().unwrap());
// Assume First request/response is successful
- sk_client.secret_management_request_custom_aad(&req_1, &seq_0, &seq_0);
+ sk_client.secret_management_request_custom_aad(&req_1, &seq_0, &seq_0).unwrap();
// Check 2nd request/response with skipped seq_num in request is a binder error
- // This should panic!
- sk_client.secret_management_request_custom_aad(&req_2, /*Skipping seq_1*/ &seq_2, &seq_1);
+ let res = sk_client
+ .secret_management_request_custom_aad(&req_2, /*Skipping seq_1*/ &seq_2, &seq_1);
+ let err = res.expect_err("Out of Seq messages accepted!");
+ // Incorrect sequence numbers lead to failed decryption. The resultant error should be
+ // thrown in clear text & wrapped in Binder errors.
+ assert!(matches!(err, Error::InfraError(secretkeeper_client::Error::BinderStatus(_e))));
+}
+
+// This test checks DICE policy based access control of Secretkeeper.
+#[rdroidtest(get_instances())]
+fn secret_management_policy_gate(instance: String) {
+ let dice_chain = make_explicit_owned_dice(/*Security version in a node */ 100);
+ let mut sk_client = SkClient::with_identity(&instance, dice_chain);
+ sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE).unwrap();
+
+ // Start a session with higher security_version & get the stored secret.
+ let dice_chain_upgraded = make_explicit_owned_dice(/*Security version in a node */ 101);
+ let mut sk_client_upgraded = SkClient::with_identity(&instance, dice_chain_upgraded);
+ assert_eq!(sk_client_upgraded.get(&ID_EXAMPLE).unwrap(), SECRET_EXAMPLE);
+
+ // Start a session with lower security_version (This should be denied access to the secret).
+ let dice_chain_downgraded = make_explicit_owned_dice(/*Security version in a node */ 99);
+ let mut sk_client_downgraded = SkClient::with_identity(&instance, dice_chain_downgraded);
+ assert!(matches!(
+ sk_client_downgraded.get(&ID_EXAMPLE).unwrap_err(),
+ Error::SecretkeeperError(SecretkeeperError::DicePolicyError)
+ ));
}
// Helper method that constructs 3 SecretManagement requests. Callers would usually not care about