Merge "Remove direct access to the sealing CDI from the payload"
diff --git a/compos/compos_key_helper/compos_key_main.cpp b/compos/compos_key_helper/compos_key_main.cpp
index bb2bac8..6c4fc1f 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 d903a47..51c145e 100644
--- a/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java
+++ b/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java
@@ -188,7 +188,7 @@
private static class VmCdis {
public byte[] cdiAttest;
- public byte[] cdiSeal;
+ public byte[] instanceSecret;
}
private VmCdis launchVmAndGetCdis(String instanceName) throws Exception {
@@ -203,7 +203,7 @@
ITestService testService = ITestService.Stub.asInterface(
vm.connectToVsockServer(ITestService.SERVICE_PORT));
vmCdis.cdiAttest = testService.insecurelyExposeAttestationCdi();
- vmCdis.cdiSeal = testService.insecurelyExposeSealingCdi();
+ vmCdis.instanceSecret = testService.insecurelyExposeVmInstanceSecret();
forceStop(vm);
} catch (Exception e) {
exception.complete(e);
@@ -234,10 +234,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
@@ -257,9 +256,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 64f0839..5d6e901 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();
}