From 102067a6462747e2132421286f1bc065d833b766 Mon Sep 17 00:00:00 2001 From: Andrew Scull Date: Fri, 7 Oct 2022 00:34:40 +0000 Subject: [PATCH] Remove direct access to the sealing CDI from the payload Change the API from offering the raw sealing CDI to offering VM instance secrets that happend to be derived from the sealing CDI. This makes it harder for the payload to leak its sealing CDI and losing the ability to have secrets in the VM. Bug: 243514248 Test: atest MicrodroidTests Test: atest ComposHostTestCases Change-Id: I0e72dabe7daca4d72a35788412d2ee19a3b446a5 --- compos/compos_key_helper/compos_key_main.cpp | 18 +++--- microdroid/vm_payload/include/vm_payload.h | 25 +++++--- microdroid/vm_payload/src/lib.rs | 2 +- microdroid/vm_payload/src/vm_service.rs | 62 ++++++++++--------- .../payload/IVmPayloadService.aidl | 18 +++--- microdroid_manager/src/vm_payload_service.rs | 27 ++++++-- .../microdroid/testservice/ITestService.aidl | 4 +- .../microdroid/test/MicrodroidTests.java | 17 +++-- tests/testapk/src/native/testbinary.cpp | 12 ++-- 9 files changed, 109 insertions(+), 76 deletions(-) diff --git a/compos/compos_key_helper/compos_key_main.cpp b/compos/compos_key_helper/compos_key_main.cpp index 77a9cf90..caf646e5 100644 --- a/compos/compos_key_helper/compos_key_main.cpp +++ b/compos/compos_key_helper/compos_key_main.cpp @@ -31,16 +31,16 @@ using namespace std::literals; using compos_key::Ed25519KeyPair; namespace { -Result deriveKeyFromDice() { - uint8_t cdi_seal[64]; - size_t cdi_size = get_dice_sealing_cdi(cdi_seal, sizeof(cdi_seal)); - if (cdi_size == 0) { - return Error() << "Failed to get sealing CDI"; - } - // We use the sealing CDI because we want stability - the key needs to be the same - // for any instance of the "same" VM. - return compos_key::deriveKeyFromSecret(cdi_seal, cdi_size); +constexpr const char* kSigningKeySecretIdentifier = "CompOS signing key secret"; + +Result deriveKeyFromDice() { + uint8_t secret[32]; + if (!get_vm_instance_secret(kSigningKeySecretIdentifier, strlen(kSigningKeySecretIdentifier), + secret, sizeof(secret))) { + return Error() << "Failed to get signing key secret"; + } + return compos_key::deriveKeyFromSecret(secret, sizeof(secret)); } int write_public_key() { diff --git a/microdroid/vm_payload/include/vm_payload.h b/microdroid/vm_payload/include/vm_payload.h index 6dba7608..4b77b43d 100644 --- a/microdroid/vm_payload/include/vm_payload.h +++ b/microdroid/vm_payload/include/vm_payload.h @@ -16,6 +16,10 @@ #pragma once +#include +#include +#include + #ifdef __cplusplus extern "C" { #endif @@ -26,6 +30,20 @@ extern "C" { */ bool notify_payload_ready(); +/** + * Get a secret that is uniquely bound to this VM instance. The secrets are 32-byte values and the + * value associated with an identifier will not change over the lifetime of the VM instance. + * + * \param identifier identifier of the secret to return. + * \param identifier_size size of the secret identifier. + * \param secret pointer to size bytes where the secret is written. + * \param size number of bytes of the secret to get, up to the secret size. + * + * \return true on success and false on failure. + */ +bool get_vm_instance_secret(const void *identifier, size_t identifier_size, void *secret, + size_t size); + /** * Get the VM's attestation chain. * Returns the size of data or 0 on failure. @@ -41,13 +59,6 @@ size_t get_dice_attestation_chain(void *data, size_t size); */ size_t get_dice_attestation_cdi(void *data, size_t size); -/** - * Get the VM's sealing CDI. - * Returns the size of data or 0 on failure. - * TODO: don't expose the raw CDI, only derived values - */ -size_t get_dice_sealing_cdi(void *data, size_t size); - #ifdef __cplusplus } // extern "C" #endif diff --git a/microdroid/vm_payload/src/lib.rs b/microdroid/vm_payload/src/lib.rs index e3da227f..74dd8f49 100644 --- a/microdroid/vm_payload/src/lib.rs +++ b/microdroid/vm_payload/src/lib.rs @@ -17,6 +17,6 @@ mod vm_service; pub use vm_service::{ - get_dice_attestation_cdi, get_dice_attestation_chain, get_dice_sealing_cdi, + get_dice_attestation_cdi, get_dice_attestation_chain, get_vm_instance_secret, notify_payload_ready, }; diff --git a/microdroid/vm_payload/src/vm_service.rs b/microdroid/vm_payload/src/vm_service.rs index 18d82228..cfc3884b 100644 --- a/microdroid/vm_payload/src/vm_service.rs +++ b/microdroid/vm_payload/src/vm_service.rs @@ -42,6 +42,40 @@ fn try_notify_payload_ready() -> Result<()> { get_vm_payload_service()?.notifyPayloadReady().context("Cannot notify payload ready") } +/// Get a secret that is uniquely bound to this VM instance. +/// +/// # Safety +/// +/// The identifier must be identifier_size bytes and secret must be size bytes. +#[no_mangle] +pub unsafe extern "C" fn get_vm_instance_secret( + identifier: *const u8, + identifier_size: usize, + secret: *mut u8, + size: usize, +) -> bool { + let identifier = std::slice::from_raw_parts(identifier, identifier_size); + match try_get_vm_instance_secret(identifier, size) { + Err(e) => { + error!("{:?}", e); + false + } + Ok(vm_secret) => { + if vm_secret.len() != size { + return false; + } + std::ptr::copy_nonoverlapping(vm_secret.as_ptr(), secret, size); + true + } + } +} + +fn try_get_vm_instance_secret(identifier: &[u8], size: usize) -> Result> { + get_vm_payload_service()? + .getVmInstanceSecret(identifier, i32::try_from(size)?) + .context("Cannot get VM instance secret") +} + /// Get the VM's attestation chain. /// Returns the size of data or 0 on failure. /// @@ -98,34 +132,6 @@ fn try_get_dice_attestation_cdi() -> Result> { get_vm_payload_service()?.getDiceAttestationCdi().context("Cannot get attestation CDI") } -/// Get the VM's sealing CDI. -/// Returns the size of data or 0 on failure. -/// -/// # Safety -/// -/// The data must be size bytes big. -#[no_mangle] -pub unsafe extern "C" fn get_dice_sealing_cdi(data: *mut u8, size: usize) -> usize { - match try_get_dice_sealing_cdi() { - Err(e) => { - error!("{:?}", e); - 0 - } - Ok(cdi) => { - if size < cdi.len() { - 0 - } else { - std::ptr::copy_nonoverlapping(cdi.as_ptr(), data, cdi.len()); - cdi.len() - } - } - } -} - -fn try_get_dice_sealing_cdi() -> Result> { - get_vm_payload_service()?.getDiceSealingCdi().context("Cannot get sealing CDI") -} - fn get_vm_payload_service() -> Result> { wait_for_interface(VM_PAYLOAD_SERVICE_NAME) .context(format!("Failed to connect to service: {}", VM_PAYLOAD_SERVICE_NAME)) diff --git a/microdroid_manager/aidl/android/system/virtualization/payload/IVmPayloadService.aidl b/microdroid_manager/aidl/android/system/virtualization/payload/IVmPayloadService.aidl index 9f569574..d3ebe5cc 100644 --- a/microdroid_manager/aidl/android/system/virtualization/payload/IVmPayloadService.aidl +++ b/microdroid_manager/aidl/android/system/virtualization/payload/IVmPayloadService.aidl @@ -27,6 +27,15 @@ interface IVmPayloadService { /** Notifies that the payload is ready to serve. */ void notifyPayloadReady(); + /** + * Gets a secret that is uniquely bound to this VM instance. + * + * @param identifier the identifier of the secret to return. + * @param size the number of bytes of the secret to return. + * @return size bytes of the identified secret. + */ + byte[] getVmInstanceSecret(in byte[] identifier, int size); + /** * Gets the DICE attestation chain for the VM. * @@ -44,13 +53,4 @@ interface IVmPayloadService { * of it leaking. */ byte[] getDiceAttestationCdi(); - - /** - * Gets the DICE sealing CDI for the VM. - * - * TODO: A better API would handle key derivation on behalf of the payload so they can't forget - * to do it themselves. It also means the payload doesn't get the raw CDI so there's less chance - * of it leaking. - */ - byte[] getDiceSealingCdi(); } diff --git a/microdroid_manager/src/vm_payload_service.rs b/microdroid_manager/src/vm_payload_service.rs index 70117c08..4b47ad95 100644 --- a/microdroid_manager/src/vm_payload_service.rs +++ b/microdroid_manager/src/vm_payload_service.rs @@ -19,7 +19,10 @@ use android_system_virtualization_payload::aidl::android::system::virtualization BnVmPayloadService, IVmPayloadService, VM_PAYLOAD_SERVICE_NAME}; use android_system_virtualmachineservice::aidl::android::system::virtualmachineservice::IVirtualMachineService::IVirtualMachineService; use anyhow::{Context, Result}; -use binder::{Interface, BinderFeatures, Strong, add_service}; +use binder::{Interface, BinderFeatures, ExceptionCode, Status, Strong, add_service}; +use log::error; +use openssl::hkdf::hkdf; +use openssl::md::Md; /// Implementation of `IVmPayloadService`. struct VmPayloadService { @@ -32,6 +35,24 @@ impl IVmPayloadService for VmPayloadService { self.virtual_machine_service.notifyPayloadReady() } + fn getVmInstanceSecret(&self, identifier: &[u8], size: i32) -> binder::Result> { + if !(0..=32).contains(&size) { + return Err(Status::new_exception(ExceptionCode::ILLEGAL_ARGUMENT, None)); + } + // Use a fixed salt to scope the derivation to this API. It was randomly generated. + let salt = [ + 0x8B, 0x0F, 0xF0, 0xD3, 0xB1, 0x69, 0x2B, 0x95, 0x84, 0x2C, 0x9E, 0x3C, 0x99, 0x56, + 0x7A, 0x22, 0x55, 0xF8, 0x08, 0x23, 0x81, 0x5F, 0xF5, 0x16, 0x20, 0x3E, 0xBE, 0xBA, + 0xB7, 0xA8, 0x43, 0x92, + ]; + let mut secret = vec![0; size.try_into().unwrap()]; + hkdf(&mut secret, Md::sha256(), &self.dice.cdi_seal, &salt, identifier).map_err(|e| { + error!("Failed to derive VM instance secret: {:?}", e); + Status::new_service_specific_error(-1, None) + })?; + Ok(secret) + } + fn getDiceAttestationChain(&self) -> binder::Result> { Ok(self.dice.bcc.clone()) } @@ -39,10 +60,6 @@ impl IVmPayloadService for VmPayloadService { fn getDiceAttestationCdi(&self) -> binder::Result> { Ok(self.dice.cdi_attest.to_vec()) } - - fn getDiceSealingCdi(&self) -> binder::Result> { - Ok(self.dice.cdi_seal.to_vec()) - } } impl Interface for VmPayloadService {} diff --git a/tests/aidl/com/android/microdroid/testservice/ITestService.aidl b/tests/aidl/com/android/microdroid/testservice/ITestService.aidl index 0913fe33..ebb2bcfe 100644 --- a/tests/aidl/com/android/microdroid/testservice/ITestService.aidl +++ b/tests/aidl/com/android/microdroid/testservice/ITestService.aidl @@ -25,8 +25,8 @@ interface ITestService { /* read a system property. */ String readProperty(String prop); - /* get the VM's stable secret, this is _only_ done for testing. */ - byte[] insecurelyExposeSealingCdi(); + /* get a VM instance secret, this is _only_ done for testing. */ + byte[] insecurelyExposeVmInstanceSecret(); /* get the VM's attestation secret, this is _only_ done for testing. */ byte[] insecurelyExposeAttestationCdi(); diff --git a/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java b/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java index 4b402938..54a2587f 100644 --- a/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java +++ b/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java @@ -231,7 +231,7 @@ public class MicrodroidTests extends MicrodroidDeviceTestBase { private class VmCdis { public byte[] cdiAttest; - public byte[] cdiSeal; + public byte[] instanceSecret; } private VmCdis launchVmAndGetCdis(String instanceName) @@ -247,7 +247,7 @@ public class MicrodroidTests extends MicrodroidDeviceTestBase { ITestService testService = ITestService.Stub.asInterface( vm.connectToVsockServer(ITestService.SERVICE_PORT).get()); vmCdis.cdiAttest = testService.insecurelyExposeAttestationCdi(); - vmCdis.cdiSeal = testService.insecurelyExposeSealingCdi(); + vmCdis.instanceSecret = testService.insecurelyExposeVmInstanceSecret(); forceStop(vm); } catch (Exception e) { exception.complete(e); @@ -281,10 +281,9 @@ public class MicrodroidTests extends MicrodroidDeviceTestBase { assertThat(vm_a_cdis.cdiAttest).isNotNull(); assertThat(vm_b_cdis.cdiAttest).isNotNull(); assertThat(vm_a_cdis.cdiAttest).isNotEqualTo(vm_b_cdis.cdiAttest); - assertThat(vm_a_cdis.cdiSeal).isNotNull(); - assertThat(vm_b_cdis.cdiSeal).isNotNull(); - assertThat(vm_a_cdis.cdiSeal).isNotEqualTo(vm_b_cdis.cdiSeal); - assertThat(vm_a_cdis.cdiAttest).isNotEqualTo(vm_b_cdis.cdiSeal); + assertThat(vm_a_cdis.instanceSecret).isNotNull(); + assertThat(vm_b_cdis.instanceSecret).isNotNull(); + assertThat(vm_a_cdis.instanceSecret).isNotEqualTo(vm_b_cdis.instanceSecret); } @Test @@ -307,9 +306,9 @@ public class MicrodroidTests extends MicrodroidDeviceTestBase { VmCdis first_boot_cdis = launchVmAndGetCdis("test_vm"); VmCdis second_boot_cdis = launchVmAndGetCdis("test_vm"); // The attestation CDI isn't specified to be stable, though it might be - assertThat(first_boot_cdis.cdiSeal).isNotNull(); - assertThat(second_boot_cdis.cdiSeal).isNotNull(); - assertThat(first_boot_cdis.cdiSeal).isEqualTo(second_boot_cdis.cdiSeal); + assertThat(first_boot_cdis.instanceSecret).isNotNull(); + assertThat(second_boot_cdis.instanceSecret).isNotNull(); + assertThat(first_boot_cdis.instanceSecret).isEqualTo(second_boot_cdis.instanceSecret); } @Test diff --git a/tests/testapk/src/native/testbinary.cpp b/tests/testapk/src/native/testbinary.cpp index d1cfc56d..85cbd974 100644 --- a/tests/testapk/src/native/testbinary.cpp +++ b/tests/testapk/src/native/testbinary.cpp @@ -73,14 +73,14 @@ Result start_test_service() { return ndk::ScopedAStatus::ok(); } - ndk::ScopedAStatus insecurelyExposeSealingCdi(std::vector* out) override { - uint8_t cdi[64]; - size_t cdi_size = get_dice_sealing_cdi(cdi, sizeof(cdi)); - if (cdi_size == 0) { + ndk::ScopedAStatus insecurelyExposeVmInstanceSecret(std::vector* out) override { + const uint8_t identifier[] = {1, 2, 3, 4}; + uint8_t secret[32]; + if (!get_vm_instance_secret(identifier, sizeof(identifier), secret, sizeof(secret))) { return ndk::ScopedAStatus:: - fromServiceSpecificErrorWithMessage(0, "Failed to get sealing cdi"); + fromServiceSpecificErrorWithMessage(0, "Failed to VM instance secret"); } - *out = {cdi, cdi + cdi_size}; + *out = {secret, secret + sizeof(secret)}; return ndk::ScopedAStatus::ok(); }