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
diff --git a/compos/compos_key_helper/compos_key_main.cpp b/compos/compos_key_helper/compos_key_main.cpp
index 77a9cf9..caf646e 100644
--- a/compos/compos_key_helper/compos_key_main.cpp
+++ b/compos/compos_key_helper/compos_key_main.cpp
@@ -31,16 +31,16 @@
using compos_key::Ed25519KeyPair;
namespace {
-Result<Ed25519KeyPair> 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<Ed25519KeyPair> 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 6dba760..4b77b43 100644
--- a/microdroid/vm_payload/include/vm_payload.h
+++ b/microdroid/vm_payload/include/vm_payload.h
@@ -16,6 +16,10 @@
#pragma once
+#include <stdbool.h>
+#include <stddef.h>
+#include <stdint.h>
+
#ifdef __cplusplus
extern "C" {
#endif
@@ -27,6 +31,20 @@
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.
* TODO: don't expose the contained privacy breaking identifiers to the payload
@@ -41,13 +59,6 @@
*/
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 e3da227..74dd8f4 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 18d8222..cfc3884 100644
--- a/microdroid/vm_payload/src/vm_service.rs
+++ b/microdroid/vm_payload/src/vm_service.rs
@@ -42,6 +42,40 @@
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<Vec<u8>> {
+ 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 @@
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<Vec<u8>> {
- get_vm_payload_service()?.getDiceSealingCdi().context("Cannot get sealing CDI")
-}
-
fn get_vm_payload_service() -> Result<Strong<dyn IVmPayloadService>> {
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 9f56957..d3ebe5c 100644
--- a/microdroid_manager/aidl/android/system/virtualization/payload/IVmPayloadService.aidl
+++ b/microdroid_manager/aidl/android/system/virtualization/payload/IVmPayloadService.aidl
@@ -28,6 +28,15 @@
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.
*
* STOPSHIP:
@@ -44,13 +53,4 @@
* 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 70117c0..4b47ad9 100644
--- a/microdroid_manager/src/vm_payload_service.rs
+++ b/microdroid_manager/src/vm_payload_service.rs
@@ -19,7 +19,10 @@
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 @@
self.virtual_machine_service.notifyPayloadReady()
}
+ fn getVmInstanceSecret(&self, identifier: &[u8], size: i32) -> binder::Result<Vec<u8>> {
+ 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<Vec<u8>> {
Ok(self.dice.bcc.clone())
}
@@ -39,10 +60,6 @@
fn getDiceAttestationCdi(&self) -> binder::Result<Vec<u8>> {
Ok(self.dice.cdi_attest.to_vec())
}
-
- fn getDiceSealingCdi(&self) -> binder::Result<Vec<u8>> {
- 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 0913fe3..ebb2bcf 100644
--- a/tests/aidl/com/android/microdroid/testservice/ITestService.aidl
+++ b/tests/aidl/com/android/microdroid/testservice/ITestService.aidl
@@ -25,8 +25,8 @@
/* 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 4b40293..54a2587 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 @@
private class VmCdis {
public byte[] cdiAttest;
- public byte[] cdiSeal;
+ public byte[] instanceSecret;
}
private VmCdis launchVmAndGetCdis(String instanceName)
@@ -247,7 +247,7 @@
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 @@
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 @@
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 d1cfc56..85cbd97 100644
--- a/tests/testapk/src/native/testbinary.cpp
+++ b/tests/testapk/src/native/testbinary.cpp
@@ -73,14 +73,14 @@
return ndk::ScopedAStatus::ok();
}
- ndk::ScopedAStatus insecurelyExposeSealingCdi(std::vector<uint8_t>* 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<uint8_t>* 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();
}