pa-fazil
/
android_packages_modules_Virtualization
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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
This commit is contained in:
Andrew Scull 2022-10-07 00:34:40 +00:00
parent 3b80c9b14b
commit 102067a646
9 changed files with 109 additions and 76 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
compos/compos_key_helper/compos_key_main.cpp
View File

@ -31,16 +31,16 @@ using namespace std::literals;
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() {

25
microdroid/vm_payload/include/vm_payload.h
View File

@ -16,6 +16,10 @@
#pragma once
#include <stdbool.h>
#include <stddef.h>
#include <stdint.h>
#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

2
microdroid/vm_payload/src/lib.rs
View File

@ -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,
};

62
microdroid/vm_payload/src/vm_service.rs
View File

@ -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<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 @@ fn try_get_dice_attestation_cdi() -> Result<Vec<u8>> {
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))

18
microdroid_manager/aidl/android/system/virtualization/payload/IVmPayloadService.aidl
View File

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

27
microdroid_manager/src/vm_payload_service.rs
View File

@ -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<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 @@ impl IVmPayloadService for VmPayloadService {
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 {}

4
tests/aidl/com/android/microdroid/testservice/ITestService.aidl
View File

@ -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();

17
tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java
View File

@ -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

12
tests/testapk/src/native/testbinary.cpp
View File

@ -73,14 +73,14 @@ Result<void> start_test_service() {
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();
}