From 3d33a135ad54c2b0ed23ea51d2dd2232f0efb66d Mon Sep 17 00:00:00 2001 From: Shikha Panwar Date: Wed, 20 Dec 2023 01:01:04 +0000 Subject: [PATCH] Secretkeeper/VTS: Per-connection replay protection Add test coverage for replay protection in Secretkeeper. Test that: 1. Sk implementation encrypts/decrypts messages using correct sequence_numbers. 2. Out of order messages are not accepted. 3. The sequence numbers are per-connection ie, new SeqNum is used for a fresh connection. Also, refactor code. SeqNumbers are maintained by libsecretkeeper_client. Have sk_client use a handle to SkSession for SecretManagement requests. Replay protection tests however require more fine grained control of SeqNums. For these we have introduced `secret_management_request_custom_aad()` method. Bug: 316126411 Test: atest VtsSecretkeeperTargetTest Change-Id: I385856c04e185d2b300d59a1b54cb8f09cbf836f --- security/secretkeeper/aidl/vts/Android.bp | 1 + .../aidl/vts/secretkeeper_test_client.rs | 209 +++++++++++++++--- 2 files changed, 181 insertions(+), 29 deletions(-) diff --git a/security/secretkeeper/aidl/vts/Android.bp b/security/secretkeeper/aidl/vts/Android.bp index 7fc7a70f78..7de9d6a379 100644 --- a/security/secretkeeper/aidl/vts/Android.bp +++ b/security/secretkeeper/aidl/vts/Android.bp @@ -27,6 +27,7 @@ rust_test { ], test_config: "AndroidTest.xml", rustlibs: [ + "libsecretkeeper_client", "libsecretkeeper_comm_nostd", "libsecretkeeper_core_nostd", "android.hardware.security.secretkeeper-V1-rust", diff --git a/security/secretkeeper/aidl/vts/secretkeeper_test_client.rs b/security/secretkeeper/aidl/vts/secretkeeper_test_client.rs index 118a7b2e19..5d1306afaf 100644 --- a/security/secretkeeper/aidl/vts/secretkeeper_test_client.rs +++ b/security/secretkeeper/aidl/vts/secretkeeper_test_client.rs @@ -24,13 +24,14 @@ use authgraph_core::key; use binder::StatusCode; use coset::{CborSerializable, CoseEncrypt0}; use log::{info, warn}; +use secretkeeper_client::SkSession; use secretkeeper_core::cipher; use secretkeeper_comm::data_types::error::SecretkeeperError; use secretkeeper_comm::data_types::request::Request; use secretkeeper_comm::data_types::request_response_impl::{ GetVersionRequest, GetVersionResponse, GetSecretRequest, GetSecretResponse, StoreSecretRequest, StoreSecretResponse }; -use secretkeeper_comm::data_types::{Id, Secret}; +use secretkeeper_comm::data_types::{Id, Secret, SeqNum}; use secretkeeper_comm::data_types::response::Response; use secretkeeper_comm::data_types::packet::{ResponsePacket, ResponseType}; @@ -95,7 +96,10 @@ fn get_connection() -> Option<(binder::Strong, String)> { info!("No /{instance} instance of ISecretkeeper present"); } Err(e) => { - panic!("unexpected error while fetching connection to Secretkeeper {:?}", e); + panic!( + "unexpected error while fetching connection to Secretkeeper {:?}", + e + ); } } } @@ -120,8 +124,7 @@ macro_rules! setup_client { struct SkClient { sk: binder::Strong, name: String, - aes_keys: [key::AesKey; 2], - session_id: Vec, + session: SkSession, } impl Drop for SkClient { @@ -134,27 +137,58 @@ impl Drop for SkClient { impl SkClient { fn new() -> Option { let (sk, name) = get_connection()?; - let (aes_keys, session_id) = authgraph_key_exchange(sk.clone()); - Some(Self { sk, name, aes_keys, session_id }) + Some(Self { + sk: sk.clone(), + name, + session: SkSession::new(sk).unwrap(), + }) } - /// Wrapper around `ISecretkeeper::processSecretManagementRequest` that handles + /// This method is wrapper that use `SkSession::secret_management_request` which handles /// encryption and decryption. - fn secret_management_request(&self, req_data: &[u8]) -> Vec { + fn secret_management_request(&mut self, req_data: &[u8]) -> Vec { + self.session.secret_management_request(req_data).unwrap() + } + + /// Unlike the method [`secret_management_request`], this method directly uses + /// `cipher::encrypt_message` & `cipher::decrypt_message`, allowing finer control of request + /// & response aad. + fn secret_management_request_custom_aad( + &self, + req_data: &[u8], + req_aad: &[u8], + expected_res_aad: &[u8], + ) -> Vec { let aes_gcm = boring::BoringAes; let rng = boring::BoringRng; - let request_bytes = - cipher::encrypt_message(&aes_gcm, &rng, &self.aes_keys[0], &self.session_id, &req_data) - .unwrap(); + let request_bytes = cipher::encrypt_message( + &aes_gcm, + &rng, + self.session.encryption_key(), + self.session.session_id(), + &req_data, + req_aad, + ) + .unwrap(); - let response_bytes = self.sk.processSecretManagementRequest(&request_bytes).unwrap(); + // Binder call! + let response_bytes = self + .sk + .processSecretManagementRequest(&request_bytes) + .unwrap(); let response_encrypt0 = CoseEncrypt0::from_slice(&response_bytes).unwrap(); - cipher::decrypt_message(&aes_gcm, &self.aes_keys[1], &response_encrypt0).unwrap() + cipher::decrypt_message( + &aes_gcm, + self.session.decryption_key(), + &response_encrypt0, + expected_res_aad, + ) + .unwrap() } /// Helper method to store a secret. - fn store(&self, id: &Id, secret: &Secret) { + fn store(&mut self, id: &Id, secret: &Secret) { let store_request = StoreSecretRequest { id: id.clone(), secret: secret.clone(), @@ -165,14 +199,20 @@ impl SkClient { let store_response = self.secret_management_request(&store_request); let store_response = ResponsePacket::from_slice(&store_response).unwrap(); - assert_eq!(store_response.response_type().unwrap(), ResponseType::Success); + assert_eq!( + store_response.response_type().unwrap(), + ResponseType::Success + ); // Really just checking that the response is indeed StoreSecretResponse let _ = StoreSecretResponse::deserialize_from_packet(store_response).unwrap(); } /// Helper method to get a secret. - fn get(&self, id: &Id) -> Option { - let get_request = GetSecretRequest { id: id.clone(), updated_sealing_policy: None }; + fn get(&mut self, id: &Id) -> Option { + let get_request = GetSecretRequest { + id: id.clone(), + updated_sealing_policy: None, + }; let get_request = get_request.serialize_to_packet().to_vec().unwrap(); let get_response = self.secret_management_request(&get_request); @@ -191,7 +231,10 @@ impl SkClient { /// Helper method to delete secrets. fn delete(&self, ids: &[&Id]) { - let ids: Vec = ids.iter().map(|id| SecretId { id: id.0.to_vec() }).collect(); + let ids: Vec = ids + .iter() + .map(|id| SecretId { id: id.0.to_vec() }) + .collect(); self.sk.deleteIds(&ids).unwrap(); } @@ -259,7 +302,7 @@ fn authgraph_corrupt_keys() { #[test] fn secret_management_get_version() { - let sk_client = setup_client!(); + let mut sk_client = setup_client!(); let request = GetVersionRequest {}; let request_packet = request.serialize_to_packet(); @@ -268,7 +311,10 @@ fn secret_management_get_version() { let response_bytes = sk_client.secret_management_request(&request_bytes); let response_packet = ResponsePacket::from_slice(&response_bytes).unwrap(); - assert_eq!(response_packet.response_type().unwrap(), ResponseType::Success); + assert_eq!( + response_packet.response_type().unwrap(), + ResponseType::Success + ); let get_version_response = *GetVersionResponse::deserialize_from_packet(response_packet).unwrap(); assert_eq!(get_version_response.version, CURRENT_VERSION); @@ -276,7 +322,7 @@ fn secret_management_get_version() { #[test] fn secret_management_malformed_request() { - let sk_client = setup_client!(); + let mut sk_client = setup_client!(); let request = GetVersionRequest {}; let request_packet = request.serialize_to_packet(); @@ -288,14 +334,17 @@ fn secret_management_malformed_request() { let response_bytes = sk_client.secret_management_request(&request_bytes); let response_packet = ResponsePacket::from_slice(&response_bytes).unwrap(); - assert_eq!(response_packet.response_type().unwrap(), ResponseType::Error); + assert_eq!( + response_packet.response_type().unwrap(), + ResponseType::Error + ); let err = *SecretkeeperError::deserialize_from_packet(response_packet).unwrap(); assert_eq!(err, SecretkeeperError::RequestMalformed); } #[test] fn secret_management_store_get_secret_found() { - let sk_client = setup_client!(); + let mut sk_client = setup_client!(); sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE); @@ -305,7 +354,7 @@ fn secret_management_store_get_secret_found() { #[test] fn secret_management_store_get_secret_not_found() { - let sk_client = setup_client!(); + let mut sk_client = setup_client!(); // Store a secret (corresponding to an id). sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE); @@ -316,7 +365,7 @@ fn secret_management_store_get_secret_not_found() { #[test] fn secretkeeper_store_delete_ids() { - let sk_client = setup_client!(); + let mut sk_client = setup_client!(); sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE); sk_client.store(&ID_EXAMPLE_2, &SECRET_EXAMPLE); @@ -333,7 +382,7 @@ fn secretkeeper_store_delete_ids() { #[test] fn secretkeeper_store_delete_multiple_ids() { - let sk_client = setup_client!(); + let mut sk_client = setup_client!(); sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE); sk_client.store(&ID_EXAMPLE_2, &SECRET_EXAMPLE); @@ -345,7 +394,7 @@ fn secretkeeper_store_delete_multiple_ids() { #[test] fn secretkeeper_store_delete_duplicate_ids() { - let sk_client = setup_client!(); + let mut sk_client = setup_client!(); sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE); sk_client.store(&ID_EXAMPLE_2, &SECRET_EXAMPLE); @@ -358,7 +407,7 @@ fn secretkeeper_store_delete_duplicate_ids() { #[test] fn secretkeeper_store_delete_nonexistent() { - let sk_client = setup_client!(); + let mut sk_client = setup_client!(); sk_client.store(&ID_EXAMPLE, &SECRET_EXAMPLE); sk_client.store(&ID_EXAMPLE_2, &SECRET_EXAMPLE); @@ -371,7 +420,7 @@ fn secretkeeper_store_delete_nonexistent() { #[test] fn secretkeeper_store_delete_all() { - let sk_client = setup_client!(); + let mut sk_client = setup_client!(); if sk_client.name != "nonsecure" { // Don't run deleteAll() on a secure device, as it might affect @@ -397,3 +446,105 @@ fn secretkeeper_store_delete_all() { // (Try to) Get the secret that was never stored assert_eq!(sk_client.get(&ID_NOT_STORED), None); } + +// This test checks that Secretkeeper uses the expected [`RequestSeqNum`] as aad while +// decrypting requests and the responses are encrypted with correct [`ResponseSeqNum`] for the +// first few messages. +#[test] +fn secret_management_replay_protection_seq_num() { + let sk_client = setup_client!(); + // Construct encoded request packets for the test + let (req_1, req_2, req_3) = construct_secret_management_requests(); + + // Lets now construct the seq_numbers(in request & expected in response) + let mut seq_a = SeqNum::new(); + let [seq_0, seq_1, seq_2] = std::array::from_fn(|_| 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), + ) + .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), + ) + .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), + ) + .unwrap(); + assert_eq!(res.response_type().unwrap(), ResponseType::Success); +} + +// This test checks that Secretkeeper uses fresh [`RequestSeqNum`] & [`ResponseSeqNum`] +// for new sessions. +#[test] +fn secret_management_replay_protection_seq_num_per_session() { + let sk_client = setup_client!(); + + // Construct encoded request packets for the test + let (req_1, _, _) = construct_secret_management_requests(); + + // Lets now construct the seq_number (in request & expected in response) + let mut seq_a = SeqNum::new(); + 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), + ) + .unwrap(); + assert_eq!(res.response_type().unwrap(), ResponseType::Success); + + // Start another session + let sk_client_diff = setup_client!(); + // 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), + ) + .unwrap(); + assert_eq!(res.response_type().unwrap(), ResponseType::Success); +} + +// This test checks that Secretkeeper rejects requests with out of order [`RequestSeqNum`] +#[test] +#[should_panic] +fn secret_management_replay_protection_out_of_seq_req_not_accepted() { + let sk_client = setup_client!(); + + // Construct encoded request packets for the test + let (req_1, req_2, _) = construct_secret_management_requests(); + + // Lets now construct the seq_numbers(in request & expected in response) + let mut seq_a = SeqNum::new(); + 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); + + // 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); +} + +fn construct_secret_management_requests() -> (Vec, Vec, Vec) { + let version_request = GetVersionRequest {}; + let version_request = version_request.serialize_to_packet().to_vec().unwrap(); + let store_request = StoreSecretRequest { + id: ID_EXAMPLE, + secret: SECRET_EXAMPLE, + sealing_policy: HYPOTHETICAL_DICE_POLICY.to_vec(), + }; + let store_request = store_request.serialize_to_packet().to_vec().unwrap(); + let get_request = GetSecretRequest { + id: ID_EXAMPLE, + updated_sealing_policy: None, + }; + let get_request = get_request.serialize_to_packet().to_vec().unwrap(); + (version_request, store_request, get_request) +}