Merge "Secretkeeper: add AuthGraph key exchange" into main am: 986e92e098 am: c074a562e3

Original change: https://android-review.googlesource.com/c/platform/hardware/interfaces/+/2824792

Change-Id: I3e0df63c27bb2cac1066811bdaca28932a58d276
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

security/authgraph/aidl/Android.bp

@@ -34,7 +34,7 @@ aidl_interface {
             platform_apis: true,
         },
         ndk: {
-            apps_enabled: false,
+            enabled: true,
         },
         rust: {
             enabled: true,

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<ke::AuthGraphParticipant, AgError> {
     Ok(ke::AuthGraphParticipant::new(
         boring::crypto_trait_impls(),
-        Box::<boring::test_device::AgDevice>::default(),
+        Rc::new(RefCell::new(boring::test_device::AgDevice::default())),
         ke::MAX_OPENED_SESSIONS,
     )?)
 }

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);
 });

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<Mutex<AuthGraphTa>>,
+    channels: Mutex<Channels>,
+}
+
+struct Channels {
+    in_tx: mpsc::Sender<Vec<u8>>,
+    out_rx: mpsc::Receiver<Vec<u8>>,
 }
 
 impl LocalTa {
     /// Create a new instance.
     pub fn new() -> Result<Self, error::Error> {
-        Ok(Self {
+        // Create a pair of channels to communicate with the TA thread.
-            ta: Arc::new(Mutex::new(AuthGraphTa::new(
+        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::<boring::test_device::AgDevice>::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<u8> = 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<Vec<u8>> {
+    fn execute(&self, req_data: &[u8]) -> binder::Result<Vec<u8>> {
-        Ok(self.ta.lock().unwrap().process(req_data))
+        // 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"))
     }
 }

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 =
+    let local_ta = LocalTa::new().map_err(|e| format!("Failed to create the TA because: {e:?}"))?;
-        LocalTa::new().map_err(|e| format!("Failed to create the TA because: {e:?}"))?;
+    let service = service::AuthGraphService::new_as_binder(local_ta);
-    let service = service::AuthGraphService::new_as_binder(Arc::new(Mutex::new(local_ta)));
     let service_name = format!("{}/{}", SERVICE_NAME, SERVICE_INSTANCE);
     binder::add_service(&service_name, service.as_binder()).map_err(|e| {
         format!(

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",
+    ],
+}

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);
 }

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
+ * 1. Dice policy operation - These allow sealing of the secrets with a class of Dice chains.
- * 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.
  * 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
      *

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",
     ],

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<binder::Strong<dyn ISecretkeeper>> {
         }
     }
 }
+fn authgraph_key_exchange(sk: binder::Strong<dyn ISecretkeeper>) -> [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.

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",

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 std::sync::{Arc, Mutex};
-use secretkeeper_comm::data_types::packet::{RequestPacket, ResponsePacket};
+use authgraph_boringssl as boring;
-use secretkeeper_comm::data_types::request::Request;
+use authgraph_core::ta::{Role, AuthGraphTa};
-use secretkeeper_comm::data_types::request_response_impl::{
+use authgraph_core::keyexchange::{MAX_OPENED_SESSIONS, AuthGraphParticipant};
-    GetVersionRequest, GetVersionResponse, Opcode,
+use secretkeeper_comm::ta::SecretkeeperTa;
-};
+use secretkeeper_hal::SecretkeeperService;
-use secretkeeper_comm::data_types::response::Response;
+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<Vec<u8>>,
+    out_rx: mpsc::Receiver<Vec<u8>>,
+}
 
-#[derive(Debug, Default)]
+/// Prefix byte for messages intended for the AuthGraph TA.
-pub struct NonSecureSecretkeeper;
+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 {
+        // The TA code expects to run single threaded, so spawn a thread to run it in.
-    fn processSecretManagementRequest(&self, request: &[u8]) -> binder::Result<Vec<u8>> {
+        std::thread::spawn(move || {
-        Ok(self.process_opaque_request(request))
+            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<u8> = 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<u8> {
+        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 {
+pub struct AuthGraphChannel(Arc<Mutex<LocalTa>>);
-    // A set of requests to Secretkeeper are 'opaque' - encrypted bytes with inner structure
+impl SerializedChannel for AuthGraphChannel {
-    // described by CDDL. They need to be decrypted, deserialized and processed accordingly.
+    const MAX_SIZE: usize = usize::MAX;
-    fn process_opaque_request(&self, request: &[u8]) -> Vec<u8> {
+    fn execute(&self, req_data: &[u8]) -> binder::Result<Vec<u8>> {
-        // TODO(b/291224769) The request will need to be decrypted & response need to be encrypted
+        Ok(self
-        // with key & related artifacts pre-shared via Authgraph Key Exchange HAL.
+            .0
-        self.process_opaque_request_unhandled_error(request)
+            .lock()
-            .unwrap_or_else(
+            .unwrap()
-                // SecretkeeperError is also a valid 'Response', serialize to a response packet.
+            .execute_for(AG_MESSAGE_PREFIX, req_data))
-                |sk_err| {
-                    Response::serialize_to_packet(&sk_err)
-                        .into_bytes()
-                        .expect("Panicking due to serialization failing")
-                },
-            )
     }
+}
 
-    fn process_opaque_request_unhandled_error(
+pub struct SecretkeeperChannel(Arc<Mutex<LocalTa>>);
-        &self,
+impl SerializedChannel for SecretkeeperChannel {
-        request: &[u8],
+    const MAX_SIZE: usize = usize::MAX;
-    ) -> Result<Vec<u8>, SecretkeeperError> {
+    fn execute(&self, req_data: &[u8]) -> binder::Result<Vec<u8>> {
-        let request_packet = RequestPacket::from_bytes(request).map_err(|e| {
+        Ok(self
-            error!("Failed to get Request packet from bytes: {:?}", e);
+            .0
-            SecretkeeperError::RequestMalformed
+            .lock()
-        })?;
+            .unwrap()
-        let response_packet = match request_packet
+            .execute_for(SK_MESSAGE_PREFIX, req_data))
-            .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<ResponsePacket, SecretkeeperError> {
-        // 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())
     }
 }
 
@@ -104,17 +130,17 @@ fn main() {
         error!("{}", panic_info);
     }));
 
-    let service = NonSecureSecretkeeper::default();
+    let ta = Arc::new(Mutex::new(LocalTa::new()));
-    let service_binder = BnSecretkeeper::new_binder(service, BinderFeatures::default());
+    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",
         <BpSecretkeeper as ISecretkeeper>::get_descriptor()
     );
-    binder::add_service(&service_name, service_binder.as_binder()).unwrap_or_else(|e| {
+    binder::add_service(&service_name, service.as_binder()).unwrap_or_else(|e| {
-        panic!(
+        panic!("Failed to register service {service_name} because of {e:?}.",);
-            "Failed to register service {} because of {:?}.",
-            service_name, e
-        );
     });
     info!("Registered Binder service, joining threadpool.");
     binder::ProcessState::join_thread_pool();