diff --git a/security/authgraph/aidl/Android.bp b/security/authgraph/aidl/Android.bp index d94f640570..f3d12816ca 100644 --- a/security/authgraph/aidl/Android.bp +++ b/security/authgraph/aidl/Android.bp @@ -34,7 +34,7 @@ aidl_interface { platform_apis: true, }, ndk: { - apps_enabled: false, + enabled: true, }, rust: { enabled: true, diff --git a/security/authgraph/aidl/vts/functional/lib.rs b/security/authgraph/aidl/vts/functional/lib.rs index da3fa1cec6..f4c1da923d 100644 --- a/security/authgraph/aidl/vts/functional/lib.rs +++ b/security/authgraph/aidl/vts/functional/lib.rs @@ -26,6 +26,7 @@ use android_hardware_security_authgraph::aidl::android::hardware::security::auth use authgraph_boringssl as boring; use authgraph_core::{error::Error as AgError, keyexchange as ke}; use coset::CborSerializable; +use std::{cell::RefCell, rc::Rc}; pub mod sink; pub mod source; @@ -34,7 +35,7 @@ pub mod source; pub fn test_ag_participant() -> Result { Ok(ke::AuthGraphParticipant::new( boring::crypto_trait_impls(), - Box::::default(), + Rc::new(RefCell::new(boring::test_device::AgDevice::default())), ke::MAX_OPENED_SESSIONS, )?) } diff --git a/security/authgraph/default/src/fuzzer.rs b/security/authgraph/default/src/fuzzer.rs index d4017776f8..387d72ffd1 100644 --- a/security/authgraph/default/src/fuzzer.rs +++ b/security/authgraph/default/src/fuzzer.rs @@ -22,10 +22,9 @@ use authgraph_hal::service::AuthGraphService; use authgraph_nonsecure::LocalTa; use binder_random_parcel_rs::fuzz_service; use libfuzzer_sys::fuzz_target; -use std::sync::{Arc, Mutex}; fuzz_target!(|data: &[u8]| { let local_ta = LocalTa::new().expect("Failed to create an AuthGraph local TA."); - let service = AuthGraphService::new_as_binder(Arc::new(Mutex::new(local_ta))); + let service = AuthGraphService::new_as_binder(local_ta); fuzz_service(&mut service.as_binder(), data); }); diff --git a/security/authgraph/default/src/lib.rs b/security/authgraph/default/src/lib.rs index 14741aafee..1f851b28e8 100644 --- a/security/authgraph/default/src/lib.rs +++ b/security/authgraph/default/src/lib.rs @@ -22,36 +22,62 @@ use authgraph_core::{ ta::{AuthGraphTa, Role}, }; use authgraph_hal::channel::SerializedChannel; -use std::sync::{Arc, Mutex}; +use std::cell::RefCell; +use std::rc::Rc; +use std::sync::{mpsc, Mutex}; /// Implementation of the AuthGraph TA that runs locally in-process (and which is therefore /// insecure). pub struct LocalTa { - ta: Arc>, + channels: Mutex, +} + +struct Channels { + in_tx: mpsc::Sender>, + out_rx: mpsc::Receiver>, } impl LocalTa { /// Create a new instance. pub fn new() -> Result { - Ok(Self { - ta: Arc::new(Mutex::new(AuthGraphTa::new( + // Create a pair of channels to communicate with the TA thread. + let (in_tx, in_rx) = mpsc::channel(); + let (out_tx, out_rx) = mpsc::channel(); + + // The TA code expects to run single threaded, so spawn a thread to run it in. + std::thread::spawn(move || { + let mut ta = AuthGraphTa::new( keyexchange::AuthGraphParticipant::new( boring::crypto_trait_impls(), - Box::::default(), + Rc::new(RefCell::new(boring::test_device::AgDevice::default())), keyexchange::MAX_OPENED_SESSIONS, - )?, + ) + .expect("failed to create AG participant"), Role::Both, - ))), + ); + // Loop forever processing request messages. + loop { + let req_data: Vec = in_rx.recv().expect("failed to receive next req"); + let rsp_data = ta.process(&req_data); + out_tx.send(rsp_data).expect("failed to send out rsp"); + } + }); + Ok(Self { + channels: Mutex::new(Channels { in_tx, out_rx }), }) } } -/// Pretend to be a serialized channel to the TA, but actually just directly invoke the TA with -/// incoming requests. impl SerializedChannel for LocalTa { const MAX_SIZE: usize = usize::MAX; - fn execute(&mut self, req_data: &[u8]) -> binder::Result> { - Ok(self.ta.lock().unwrap().process(req_data)) + fn execute(&self, req_data: &[u8]) -> binder::Result> { + // Serialize across both request and response. + let channels = self.channels.lock().unwrap(); + channels + .in_tx + .send(req_data.to_vec()) + .expect("failed to send in request"); + Ok(channels.out_rx.recv().expect("failed to receive response")) } } diff --git a/security/authgraph/default/src/main.rs b/security/authgraph/default/src/main.rs index 81f2dd6115..ced7567583 100644 --- a/security/authgraph/default/src/main.rs +++ b/security/authgraph/default/src/main.rs @@ -25,7 +25,6 @@ use authgraph_hal::service; use authgraph_nonsecure::LocalTa; use log::{error, info}; -use std::sync::{Arc, Mutex}; static SERVICE_NAME: &str = "android.hardware.security.authgraph.IAuthGraphKeyExchange"; static SERVICE_INSTANCE: &str = "nonsecure"; @@ -65,9 +64,8 @@ fn inner_main() -> Result<(), HalServiceError> { binder::ProcessState::start_thread_pool(); // Register the service - let local_ta = - LocalTa::new().map_err(|e| format!("Failed to create the TA because: {e:?}"))?; - let service = service::AuthGraphService::new_as_binder(Arc::new(Mutex::new(local_ta))); + let local_ta = LocalTa::new().map_err(|e| format!("Failed to create the TA because: {e:?}"))?; + let service = service::AuthGraphService::new_as_binder(local_ta); let service_name = format!("{}/{}", SERVICE_NAME, SERVICE_INSTANCE); binder::add_service(&service_name, service.as_binder()).map_err(|e| { format!( diff --git a/security/secretkeeper/aidl/Android.bp b/security/secretkeeper/aidl/Android.bp index c77d2995b7..ac923ca71e 100644 --- a/security/secretkeeper/aidl/Android.bp +++ b/security/secretkeeper/aidl/Android.bp @@ -20,8 +20,15 @@ aidl_interface { name: "android.hardware.security.secretkeeper", vendor_available: true, srcs: ["android/hardware/security/secretkeeper/*.aidl"], + imports: [ + "android.hardware.security.authgraph-V1", + ], stability: "vintf", + frozen: false, backend: { + java: { + enabled: false, + }, ndk: { enabled: true, }, @@ -34,3 +41,44 @@ aidl_interface { }, }, } + +// cc_defaults that includes the latest Secretkeeper AIDL library. +// Modules that depend on Secretkeeper directly can include this cc_defaults to avoid +// managing dependency versions explicitly. +cc_defaults { + name: "secretkeeper_use_latest_hal_aidl_ndk_static", + static_libs: [ + "android.hardware.security.secretkeeper-V1-ndk", + ], +} + +cc_defaults { + name: "secretkeeper_use_latest_hal_aidl_ndk_shared", + shared_libs: [ + "android.hardware.security.secretkeeper-V1-ndk", + ], +} + +cc_defaults { + name: "secretkeeper_use_latest_hal_aidl_cpp_static", + static_libs: [ + "android.hardware.security.secretkeeper-V1-cpp", + ], +} + +cc_defaults { + name: "secretkeeper_use_latest_hal_aidl_cpp_shared", + shared_libs: [ + "android.hardware.security.secretkeeper-V1-cpp", + ], +} + +// A rust_defaults that includes the latest Secretkeeper AIDL library. +// Modules that depend on Secretkeeper directly can include this rust_defaults to avoid +// managing dependency versions explicitly. +rust_defaults { + name: "secretkeeper_use_latest_hal_aidl_rust", + rustlibs: [ + "android.hardware.security.secretkeeper-V1-rust", + ], +} diff --git a/security/secretkeeper/aidl/aidl_api/android.hardware.security.secretkeeper/current/android/hardware/security/secretkeeper/ISecretkeeper.aidl b/security/secretkeeper/aidl/aidl_api/android.hardware.security.secretkeeper/current/android/hardware/security/secretkeeper/ISecretkeeper.aidl index 2eb33c56c9..023fc8f97f 100644 --- a/security/secretkeeper/aidl/aidl_api/android.hardware.security.secretkeeper/current/android/hardware/security/secretkeeper/ISecretkeeper.aidl +++ b/security/secretkeeper/aidl/aidl_api/android.hardware.security.secretkeeper/current/android/hardware/security/secretkeeper/ISecretkeeper.aidl @@ -34,5 +34,6 @@ package android.hardware.security.secretkeeper; @VintfStability interface ISecretkeeper { + android.hardware.security.authgraph.IAuthGraphKeyExchange getAuthGraphKe(); byte[] processSecretManagementRequest(in byte[] request); } diff --git a/security/secretkeeper/aidl/android/hardware/security/secretkeeper/ISecretkeeper.aidl b/security/secretkeeper/aidl/android/hardware/security/secretkeeper/ISecretkeeper.aidl index af715a90cd..1f4768a045 100644 --- a/security/secretkeeper/aidl/android/hardware/security/secretkeeper/ISecretkeeper.aidl +++ b/security/secretkeeper/aidl/android/hardware/security/secretkeeper/ISecretkeeper.aidl @@ -16,6 +16,8 @@ package android.hardware.security.secretkeeper; +import android.hardware.security.authgraph.IAuthGraphKeyExchange; + @VintfStability /** * Secretkeeper service definition. @@ -29,16 +31,21 @@ package android.hardware.security.secretkeeper; * - A completely separate, purpose-built and certified secure CPU. * * TODO(b/291224769): Extend the HAL interface to include: - * 1. Session setup api: This is used to perform cryptographic operations that allow shared keys to - * be exchanged between session participants, typically (but not necessarily) a pVM instance and - * Secretkeeper. This session setup is based on public key cryptography. - * 2. Dice policy operation - These allow sealing of the secrets with a class of Dice chains. + * 1. Dice policy operation - These allow sealing of the secrets with a class of Dice chains. * Typical operations are (securely) updating the dice policy sealing the Secrets above. These * operations are core to AntiRollback protected secrets - ie, ensuring secrets of a pVM are only * accessible to same or higher versions of the images. - * 3. Maintenance api: This is required for removing the Secretkeeper entries for obsolete pvMs. + * 2. Maintenance api: This is required for removing the Secretkeeper entries for obsolete pvMs. */ interface ISecretkeeper { + /** + * Retrieve the instance of the `IAuthGraphKeyExchange` HAL that should be used for shared + * session key establishment. These keys are used to perform encryption of messages as + * described in SecretManagement.cddl, allowing the client and Secretkeeper to have a + * cryptographically secure channel. + */ + IAuthGraphKeyExchange getAuthGraphKe(); + /** * processSecretManagementRequest method is used for interacting with the Secret Management API * diff --git a/security/secretkeeper/aidl/vts/Android.bp b/security/secretkeeper/aidl/vts/Android.bp index 6818298926..fac16f68ba 100644 --- a/security/secretkeeper/aidl/vts/Android.bp +++ b/security/secretkeeper/aidl/vts/Android.bp @@ -28,6 +28,8 @@ rust_test { rustlibs: [ "libsecretkeeper_comm_nostd", "android.hardware.security.secretkeeper-V1-rust", + "libauthgraph_core", + "libauthgraph_vts_test", "libbinder_rs", "liblog_rust", ], diff --git a/security/secretkeeper/aidl/vts/secretkeeper_test_client.rs b/security/secretkeeper/aidl/vts/secretkeeper_test_client.rs index 28923f7120..70f5da6d59 100644 --- a/security/secretkeeper/aidl/vts/secretkeeper_test_client.rs +++ b/security/secretkeeper/aidl/vts/secretkeeper_test_client.rs @@ -25,6 +25,8 @@ use secretkeeper_comm::data_types::request_response_impl::{ use secretkeeper_comm::data_types::response::Response; use secretkeeper_comm::data_types::packet::{ResponsePacket, ResponseType}; use android_hardware_security_secretkeeper::aidl::android::hardware::security::secretkeeper::ISecretkeeper::ISecretkeeper; +use authgraph_vts_test as ag_vts; +use authgraph_core::key; const SECRETKEEPER_IDENTIFIER: &str = "android.hardware.security.secretkeeper.ISecretkeeper/nonsecure"; @@ -42,6 +44,57 @@ fn get_connection() -> Option> { } } } +fn authgraph_key_exchange(sk: binder::Strong) -> [key::AesKey; 2] { + let sink = sk.getAuthGraphKe().expect("failed to get AuthGraph"); + let mut source = ag_vts::test_ag_participant().expect("failed to create a local source"); + ag_vts::sink::test_mainline(&mut source, sink) +} + +/// Test that the AuthGraph instance returned by SecretKeeper correctly performs +/// mainline key exchange against a local source implementation. +#[test] +fn authgraph_mainline() { + let sk = match get_connection() { + Some(sk) => sk, + None => { + warn!("Secretkeeper HAL is unavailable, skipping test"); + return; + } + }; + let _aes_keys = authgraph_key_exchange(sk); +} + +/// Test that the AuthGraph instance returned by SecretKeeper correctly rejects +/// a corrupted session ID signature. +#[test] +fn authgraph_corrupt_sig() { + let sk = match get_connection() { + Some(sk) => sk, + None => { + warn!("Secretkeeper HAL is unavailable, skipping test"); + return; + } + }; + let sink = sk.getAuthGraphKe().expect("failed to get AuthGraph"); + let mut source = ag_vts::test_ag_participant().expect("failed to create a local source"); + ag_vts::sink::test_corrupt_sig(&mut source, sink); +} + +/// Test that the AuthGraph instance returned by SecretKeeper correctly detects +/// when corrupted keys are returned to it. +#[test] +fn authgraph_corrupt_keys() { + let sk = match get_connection() { + Some(sk) => sk, + None => { + warn!("Secretkeeper HAL is unavailable, skipping test"); + return; + } + }; + let sink = sk.getAuthGraphKe().expect("failed to get AuthGraph"); + let mut source = ag_vts::test_ag_participant().expect("failed to create a local source"); + ag_vts::sink::test_corrupt_keys(&mut source, sink); +} // TODO(b/2797757): Add tests that match different HAL defined objects (like request/response) // with expected bytes. diff --git a/security/secretkeeper/default/Android.bp b/security/secretkeeper/default/Android.bp index 8240b89f80..a920c6ed6d 100644 --- a/security/secretkeeper/default/Android.bp +++ b/security/secretkeeper/default/Android.bp @@ -24,12 +24,19 @@ rust_binary { vendor: true, installable: false, // install APEX prefer_rlib: true, + defaults: [ + "authgraph_use_latest_hal_aidl_rust", + ], rustlibs: [ "android.hardware.security.secretkeeper-V1-rust", "libandroid_logger", + "libauthgraph_boringssl", + "libauthgraph_core", + "libauthgraph_hal", "libbinder_rs", "liblog_rust", "libsecretkeeper_comm_nostd", + "libsecretkeeper_hal", ], srcs: [ "src/main.rs", diff --git a/security/secretkeeper/default/src/main.rs b/security/secretkeeper/default/src/main.rs index 2d367c5184..4f9302d026 100644 --- a/security/secretkeeper/default/src/main.rs +++ b/security/secretkeeper/default/src/main.rs @@ -14,80 +14,106 @@ * limitations under the License. */ -use binder::{BinderFeatures, Interface}; +//! Non-secure implementation of the Secretkeeper HAL. + use log::{error, info, Level}; -use secretkeeper_comm::data_types::error::SecretkeeperError; -use secretkeeper_comm::data_types::packet::{RequestPacket, ResponsePacket}; -use secretkeeper_comm::data_types::request::Request; -use secretkeeper_comm::data_types::request_response_impl::{ - GetVersionRequest, GetVersionResponse, Opcode, -}; -use secretkeeper_comm::data_types::response::Response; - +use std::sync::{Arc, Mutex}; +use authgraph_boringssl as boring; +use authgraph_core::ta::{Role, AuthGraphTa}; +use authgraph_core::keyexchange::{MAX_OPENED_SESSIONS, AuthGraphParticipant}; +use secretkeeper_comm::ta::SecretkeeperTa; +use secretkeeper_hal::SecretkeeperService; +use authgraph_hal::channel::SerializedChannel; use android_hardware_security_secretkeeper::aidl::android::hardware::security::secretkeeper::ISecretkeeper::{ - BnSecretkeeper, BpSecretkeeper, ISecretkeeper, + ISecretkeeper, BpSecretkeeper, }; +use std::cell::RefCell; +use std::rc::Rc; +use std::sync::mpsc; -const CURRENT_VERSION: u64 = 1; +/// Implementation of the Secrekeeper TA that runs locally in-process (and which is therefore +/// insecure). +pub struct LocalTa { + in_tx: mpsc::Sender>, + out_rx: mpsc::Receiver>, +} -#[derive(Debug, Default)] -pub struct NonSecureSecretkeeper; +/// Prefix byte for messages intended for the AuthGraph TA. +const AG_MESSAGE_PREFIX: u8 = 0x00; +/// Prefix byte for messages intended for the Secretkeeper TA. +const SK_MESSAGE_PREFIX: u8 = 0x01; -impl Interface for NonSecureSecretkeeper {} +impl LocalTa { + /// Create a new instance. + pub fn new() -> Self { + // Create a pair of channels to communicate with the TA thread. + let (in_tx, in_rx) = mpsc::channel(); + let (out_tx, out_rx) = mpsc::channel(); -impl ISecretkeeper for NonSecureSecretkeeper { - fn processSecretManagementRequest(&self, request: &[u8]) -> binder::Result> { - Ok(self.process_opaque_request(request)) + // The TA code expects to run single threaded, so spawn a thread to run it in. + std::thread::spawn(move || { + let mut crypto_impls = boring::crypto_trait_impls(); + let sk_ta = Rc::new(RefCell::new( + SecretkeeperTa::new(&mut crypto_impls) + .expect("Failed to create local Secretkeeper TA"), + )); + let mut ag_ta = AuthGraphTa::new( + AuthGraphParticipant::new(crypto_impls, sk_ta.clone(), MAX_OPENED_SESSIONS) + .expect("Failed to create local AuthGraph TA"), + Role::Sink, + ); + + // Loop forever processing request messages. + loop { + let req_data: Vec = in_rx.recv().expect("failed to receive next req"); + let rsp_data = match req_data[0] { + AG_MESSAGE_PREFIX => ag_ta.process(&req_data[1..]), + SK_MESSAGE_PREFIX => { + // It's safe to `borrow_mut()` because this code is not a callback + // from AuthGraph (the only other holder of an `Rc`), and so there + // can be no live `borrow()`s in this (single) thread. + sk_ta.borrow_mut().process(&req_data[1..]) + } + prefix => panic!("unexpected messageprefix {prefix}!"), + }; + out_tx.send(rsp_data).expect("failed to send out rsp"); + } + }); + Self { in_tx, out_rx } + } + + fn execute_for(&mut self, prefix: u8, req_data: &[u8]) -> Vec { + let mut prefixed_req = Vec::with_capacity(req_data.len() + 1); + prefixed_req.push(prefix); + prefixed_req.extend_from_slice(req_data); + self.in_tx + .send(prefixed_req) + .expect("failed to send in request"); + self.out_rx.recv().expect("failed to receive response") } } -impl NonSecureSecretkeeper { - // A set of requests to Secretkeeper are 'opaque' - encrypted bytes with inner structure - // described by CDDL. They need to be decrypted, deserialized and processed accordingly. - fn process_opaque_request(&self, request: &[u8]) -> Vec { - // TODO(b/291224769) The request will need to be decrypted & response need to be encrypted - // with key & related artifacts pre-shared via Authgraph Key Exchange HAL. - self.process_opaque_request_unhandled_error(request) - .unwrap_or_else( - // SecretkeeperError is also a valid 'Response', serialize to a response packet. - |sk_err| { - Response::serialize_to_packet(&sk_err) - .into_bytes() - .expect("Panicking due to serialization failing") - }, - ) +pub struct AuthGraphChannel(Arc>); +impl SerializedChannel for AuthGraphChannel { + const MAX_SIZE: usize = usize::MAX; + fn execute(&self, req_data: &[u8]) -> binder::Result> { + Ok(self + .0 + .lock() + .unwrap() + .execute_for(AG_MESSAGE_PREFIX, req_data)) } +} - fn process_opaque_request_unhandled_error( - &self, - request: &[u8], - ) -> Result, SecretkeeperError> { - let request_packet = RequestPacket::from_bytes(request).map_err(|e| { - error!("Failed to get Request packet from bytes: {:?}", e); - SecretkeeperError::RequestMalformed - })?; - let response_packet = match request_packet - .opcode() - .map_err(|_| SecretkeeperError::RequestMalformed)? - { - Opcode::GetVersion => Self::process_get_version_request(request_packet)?, - _ => todo!("TODO(b/291224769): Unimplemented operations"), - }; - - response_packet - .into_bytes() - .map_err(|_| SecretkeeperError::UnexpectedServerError) - } - - fn process_get_version_request( - request: RequestPacket, - ) -> Result { - // Deserialization really just verifies the structural integrity of the request such - // as args being empty. - let _request = GetVersionRequest::deserialize_from_packet(request) - .map_err(|_| SecretkeeperError::RequestMalformed)?; - let response = GetVersionResponse::new(CURRENT_VERSION); - Ok(response.serialize_to_packet()) +pub struct SecretkeeperChannel(Arc>); +impl SerializedChannel for SecretkeeperChannel { + const MAX_SIZE: usize = usize::MAX; + fn execute(&self, req_data: &[u8]) -> binder::Result> { + Ok(self + .0 + .lock() + .unwrap() + .execute_for(SK_MESSAGE_PREFIX, req_data)) } } @@ -104,17 +130,17 @@ fn main() { error!("{}", panic_info); })); - let service = NonSecureSecretkeeper::default(); - let service_binder = BnSecretkeeper::new_binder(service, BinderFeatures::default()); + let ta = Arc::new(Mutex::new(LocalTa::new())); + let ag_channel = AuthGraphChannel(ta.clone()); + let sk_channel = SecretkeeperChannel(ta.clone()); + + let service = SecretkeeperService::new_as_binder(sk_channel, ag_channel); let service_name = format!( "{}/nonsecure", ::get_descriptor() ); - binder::add_service(&service_name, service_binder.as_binder()).unwrap_or_else(|e| { - panic!( - "Failed to register service {} because of {:?}.", - service_name, e - ); + binder::add_service(&service_name, service.as_binder()).unwrap_or_else(|e| { + panic!("Failed to register service {service_name} because of {e:?}.",); }); info!("Registered Binder service, joining threadpool."); binder::ProcessState::join_thread_pool();