Revert "pVM to use Secretkeeper protected secrets"
Revert submission 2705357-sk_vm
Reason for revert: DroidMonitor-triggered revert due to breakage https://android-build.corp.google.com/builds/quarterdeck?branch=git_aosp-main-with-phones&target=aosp_oriole-trunk_staging-userdebug&lkgb=11221468&lkbb=11221626&fkbb=11221480
Reverted changes: /q/submissionid:2705357-sk_vm
Bug: 316391577
Change-Id: I8ba23154f91edd3bd239b8eb3a1240adbcb452ff
diff --git a/microdroid_manager/Android.bp b/microdroid_manager/Android.bp
index cb3b2aa..8481edf 100644
--- a/microdroid_manager/Android.bp
+++ b/microdroid_manager/Android.bp
@@ -5,10 +5,7 @@
rust_defaults {
name: "microdroid_manager_defaults",
crate_name: "microdroid_manager",
- defaults: [
- "avf_build_flags_rust",
- "secretkeeper_use_latest_hal_aidl_rust",
- ],
+ defaults: ["avf_build_flags_rust"],
srcs: ["src/main.rs"],
edition: "2021",
prefer_rlib: true,
@@ -46,8 +43,6 @@
"libprotobuf",
"librpcbinder_rs",
"librustutils",
- "libsecretkeeper_client",
- "libsecretkeeper_comm_nostd",
"libscopeguard",
"libserde",
"libserde_cbor",
@@ -56,7 +51,6 @@
"libuuid",
"libvsock",
"librand",
- "libzeroize",
],
init_rc: ["microdroid_manager.rc"],
multilib: {
diff --git a/microdroid_manager/src/main.rs b/microdroid_manager/src/main.rs
index c94a937..9e167a4 100644
--- a/microdroid_manager/src/main.rs
+++ b/microdroid_manager/src/main.rs
@@ -105,6 +105,7 @@
MicrodroidError::PayloadInvalidConfig(msg) => {
(ErrorCode::PAYLOAD_INVALID_CONFIG, msg.to_string())
}
+
// Connection failure won't be reported to VS; return the default value
MicrodroidError::FailedToConnectToVirtualizationService(msg) => {
(ErrorCode::UNKNOWN, msg.to_string())
@@ -281,8 +282,7 @@
// To minimize the exposure to untrusted data, derive dice profile as soon as possible.
info!("DICE derivation for payload");
let dice_artifacts = dice_derivation(dice, &instance_data, &payload_metadata)?;
- let vm_secret =
- VmSecret::new(dice_artifacts, service).context("Failed to create VM secrets")?;
+ let vm_secret = VmSecret::new(dice_artifacts).context("Failed to create VM secrets")?;
if cfg!(dice_changes) {
// Now that the DICE derivation is done, it's ok to allow payload code to run.
diff --git a/microdroid_manager/src/vm_secret.rs b/microdroid_manager/src/vm_secret.rs
index 5d234dd..d84c2e2 100644
--- a/microdroid_manager/src/vm_secret.rs
+++ b/microdroid_manager/src/vm_secret.rs
@@ -14,28 +14,18 @@
//! Class for encapsulating & managing represent VM secrets.
-use anyhow::{anyhow, ensure, Result};
-use android_system_virtualmachineservice::aidl::android::system::virtualmachineservice::IVirtualMachineService::IVirtualMachineService;
-use android_hardware_security_secretkeeper::aidl::android::hardware::security::secretkeeper::ISecretkeeper::ISecretkeeper;
-use secretkeeper_comm::data_types::request::Request;
-use binder::{Strong};
-use coset::CborSerializable;
+use anyhow::Result;
use diced_open_dice::{DiceArtifacts, OwnedDiceArtifacts};
use keystore2_crypto::ZVec;
use openssl::hkdf::hkdf;
use openssl::md::Md;
use openssl::sha;
-use secretkeeper_client::SkSession;
-use secretkeeper_comm::data_types::{Id, ID_SIZE, Secret, SECRET_SIZE};
-use secretkeeper_comm::data_types::response::Response;
-use secretkeeper_comm::data_types::packet::{ResponsePacket, ResponseType};
-use secretkeeper_comm::data_types::request_response_impl::{
- StoreSecretRequest, GetSecretResponse, GetSecretRequest};
-use secretkeeper_comm::data_types::error::SecretkeeperError;
-use zeroize::Zeroizing;
const ENCRYPTEDSTORE_KEY_IDENTIFIER: &str = "encryptedstore_key";
+// Size of the secret stored in Secretkeeper.
+const SK_SECRET_SIZE: usize = 64;
+
// Generated using hexdump -vn32 -e'14/1 "0x%02X, " 1 "\n"' /dev/urandom
const SALT_ENCRYPTED_STORE: &[u8] = &[
0xFC, 0x1D, 0x35, 0x7B, 0x96, 0xF3, 0xEF, 0x17, 0x78, 0x7D, 0x70, 0xED, 0xEA, 0xFE, 0x1D, 0x6F,
@@ -46,24 +36,6 @@
0x55, 0xF8, 0x08, 0x23, 0x81, 0x5F, 0xF5, 0x16, 0x20, 0x3E, 0xBE, 0xBA, 0xB7, 0xA8, 0x43, 0x92,
];
-// TODO(b/291213394): Remove this once policy is generated from dice_chain
-const HYPOTHETICAL_DICE_POLICY: [u8; 43] = [
- 0x83, 0x01, 0x81, 0x83, 0x01, 0x80, 0xA1, 0x01, 0x00, 0x82, 0x83, 0x01, 0x81, 0x01, 0x73, 0x74,
- 0x65, 0x73, 0x74, 0x69, 0x6E, 0x67, 0x5F, 0x64, 0x69, 0x63, 0x65, 0x5F, 0x70, 0x6F, 0x6C, 0x69,
- 0x63, 0x79, 0x83, 0x02, 0x82, 0x03, 0x18, 0x64, 0x19, 0xE9, 0x75,
-];
-// TODO(b/291213394): Differentiate the Id of nPVM based on 'salt'
-const ID_NP_VM: [u8; ID_SIZE] = [
- 0xF1, 0xB2, 0xED, 0x3B, 0xD1, 0xBD, 0xF0, 0x7D, 0xE1, 0xF0, 0x01, 0xFC, 0x61, 0x71, 0xD3, 0x42,
- 0xE5, 0x8A, 0xAF, 0x33, 0x6C, 0x11, 0xDC, 0xC8, 0x6F, 0xAE, 0x12, 0x5C, 0x26, 0x44, 0x6B, 0x86,
- 0xCC, 0x24, 0xFD, 0xBF, 0x91, 0x4A, 0x54, 0x84, 0xF9, 0x01, 0x59, 0x25, 0x70, 0x89, 0x38, 0x8D,
- 0x5E, 0xE6, 0x91, 0xDF, 0x68, 0x60, 0x69, 0x26, 0xBE, 0xFE, 0x79, 0x58, 0xF7, 0xEA, 0x81, 0x7D,
-];
-const SKP_SECRET_NP_VM: [u8; SECRET_SIZE] = [
- 0xA9, 0x89, 0x97, 0xFE, 0xAE, 0x97, 0x55, 0x4B, 0x32, 0x35, 0xF0, 0xE8, 0x93, 0xDA, 0xEA, 0x24,
- 0x06, 0xAC, 0x36, 0x8B, 0x3C, 0x95, 0x50, 0x16, 0x67, 0x71, 0x65, 0x26, 0xEB, 0xD0, 0xC3, 0x98,
-];
-
pub enum VmSecret {
// V2 secrets are derived from 2 independently secured secrets:
// 1. Secretkeeper protected secrets (skp secret).
@@ -82,47 +54,15 @@
V1 { dice: OwnedDiceArtifacts },
}
-fn get_id() -> [u8; ID_SIZE] {
- if super::is_strict_boot() {
- todo!("Id for protected VM is not implemented");
- } else {
- ID_NP_VM
- }
-}
-
impl VmSecret {
- pub fn new(
- dice_artifacts: OwnedDiceArtifacts,
- vm_service: &Strong<dyn IVirtualMachineService>,
- ) -> Result<VmSecret> {
- ensure!(dice_artifacts.bcc().is_some(), "Dice chain missing");
-
- if let Some(sk_service) = is_sk_supported(vm_service)? {
- let id = get_id();
- let mut skp_secret = Zeroizing::new([0u8; SECRET_SIZE]);
- if super::is_strict_boot() {
- if super::is_new_instance() {
- *skp_secret = rand::random();
- store_secret(sk_service.clone(), id, skp_secret.clone(), &dice_artifacts)?;
- } else {
- // Subsequent run of the pVM -> get the secret stored in Secretkeeper.
- *skp_secret = get_secret(sk_service.clone(), id, &dice_artifacts)?;
- }
- } else {
- // TODO(b/291213394): Non protected VM don't need to use Secretkeeper, remove this
- // once we have sufficient testing on protected VM.
- store_secret(sk_service.clone(), id, SKP_SECRET_NP_VM.into(), &dice_artifacts)?;
- *skp_secret = get_secret(sk_service.clone(), id, &dice_artifacts)?;
- }
- return Ok(Self::V2 {
- dice: dice_artifacts,
- skp_secret: ZVec::try_from(skp_secret.to_vec())?,
- });
+ pub fn new(dice_artifacts: OwnedDiceArtifacts) -> Result<VmSecret> {
+ if is_sk_supported() {
+ // TODO(b/291213394): Change this to real Sk protected secret.
+ let fake_skp_secret = ZVec::new(SK_SECRET_SIZE)?;
+ return Ok(Self::V2 { dice: dice_artifacts, skp_secret: fake_skp_secret });
}
- // Use V1 secrets if Secretkeeper is not supported.
Ok(Self::V1 { dice: dice_artifacts })
}
-
pub fn dice(&self) -> &OwnedDiceArtifacts {
match self {
Self::V2 { dice, .. } => dice,
@@ -154,86 +94,13 @@
}
}
-fn store_secret(
- secretkeeper: binder::Strong<dyn ISecretkeeper>,
- id: [u8; ID_SIZE],
- secret: Zeroizing<[u8; SECRET_SIZE]>,
- _dice_chain: &OwnedDiceArtifacts,
-) -> Result<()> {
- // Start a new secretkeeper session!
- let session = SkSession::new(secretkeeper).map_err(anyhow_err)?;
- let store_request = StoreSecretRequest {
- id: Id(id),
- secret: Secret(*secret),
- // TODO(b/291233371): Construct policy out of dice_chain.
- sealing_policy: HYPOTHETICAL_DICE_POLICY.to_vec(),
+// Does the hardware support Secretkeeper.
+fn is_sk_supported() -> bool {
+ if cfg!(llpvm_changes) {
+ return false;
};
- log::info!("Secretkeeper operation: {:?}", store_request);
-
- let store_request = store_request.serialize_to_packet().to_vec().map_err(anyhow_err)?;
- let store_response = session.secret_management_request(&store_request).map_err(anyhow_err)?;
- let store_response = ResponsePacket::from_slice(&store_response).map_err(anyhow_err)?;
- let response_type = store_response.response_type().map_err(anyhow_err)?;
- ensure!(
- response_type == ResponseType::Success,
- "Secretkeeper store failed with error: {:?}",
- *SecretkeeperError::deserialize_from_packet(store_response).map_err(anyhow_err)?
- );
- Ok(())
-}
-
-fn get_secret(
- secretkeeper: binder::Strong<dyn ISecretkeeper>,
- id: [u8; ID_SIZE],
- _dice_chain: &OwnedDiceArtifacts,
-) -> Result<[u8; SECRET_SIZE]> {
- // Start a new secretkeeper session!
- let session = SkSession::new(secretkeeper).map_err(anyhow_err)?;
- let get_request = GetSecretRequest {
- id: Id(id),
- // TODO(b/291233371): Construct policy out of dice_chain.
- updated_sealing_policy: None,
- };
- log::info!("Secretkeeper operation: {:?}", get_request);
-
- let get_request = get_request.serialize_to_packet().to_vec().map_err(anyhow_err)?;
- let get_response = session.secret_management_request(&get_request).map_err(anyhow_err)?;
- let get_response = ResponsePacket::from_slice(&get_response).map_err(anyhow_err)?;
- let response_type = get_response.response_type().map_err(anyhow_err)?;
- ensure!(
- response_type == ResponseType::Success,
- "Secretkeeper get failed with error: {:?}",
- *SecretkeeperError::deserialize_from_packet(get_response).map_err(anyhow_err)?
- );
- let get_response = *GetSecretResponse::deserialize_from_packet(get_response).unwrap();
- Ok(get_response.secret.0)
-}
-
-#[inline]
-fn anyhow_err<E: core::fmt::Debug>(err: E) -> anyhow::Error {
- anyhow!("{:?}", err)
-}
-
-// Get the secretkeeper connection if supported. Host can be consulted whether the device supports
-// secretkeeper but that should be used with caution for protected VM.
-fn is_sk_supported(
- host: &Strong<dyn IVirtualMachineService>,
-) -> Result<Option<Strong<dyn ISecretkeeper>>> {
- let sk = if cfg!(llpvm_changes) {
- if super::is_strict_boot() {
- // TODO: For protected VM check for Secretkeeper authentication data in device tree.
- None
- } else {
- // For non-protected VM, believe what host claims.
- host.getSecretkeeper()
- // TODO rename this error!
- .map_err(|e| {
- super::MicrodroidError::FailedToConnectToVirtualizationService(e.to_string())
- })?
- }
- } else {
- // LLPVM flag is disabled
- None
- };
- Ok(sk)
+ // TODO(b/292209416): This value should be extracted from device tree.
+ // Note: this does not affect the security of pVM. pvmfw & microdroid_manager continue to block
+ // upgraded images. Setting this true is equivalent to including constant salt in vm secrets.
+ true
}
diff --git a/virtualizationmanager/Android.bp b/virtualizationmanager/Android.bp
index 88e9c70..33897b2 100644
--- a/virtualizationmanager/Android.bp
+++ b/virtualizationmanager/Android.bp
@@ -5,11 +5,7 @@
rust_defaults {
name: "virtualizationmanager_defaults",
crate_name: "virtualizationmanager",
- defaults: [
- "avf_build_flags_rust",
- "secretkeeper_use_latest_hal_aidl_rust",
- "authgraph_use_latest_hal_aidl_rust",
- ],
+ defaults: ["avf_build_flags_rust"],
edition: "2021",
// Only build on targets which crosvm builds on.
enabled: false,
diff --git a/virtualizationmanager/src/aidl.rs b/virtualizationmanager/src/aidl.rs
index f7ea21f..7f98fe8 100644
--- a/virtualizationmanager/src/aidl.rs
+++ b/virtualizationmanager/src/aidl.rs
@@ -52,13 +52,6 @@
use android_system_virtualmachineservice::aidl::android::system::virtualmachineservice::IVirtualMachineService::{
BnVirtualMachineService, IVirtualMachineService,
};
-use android_hardware_security_secretkeeper::aidl::android::hardware::security::secretkeeper::ISecretkeeper::{BnSecretkeeper, ISecretkeeper};
-use android_hardware_security_authgraph::aidl::android::hardware::security::authgraph::{
- Arc::Arc as AuthgraphArc, IAuthGraphKeyExchange::IAuthGraphKeyExchange,
- IAuthGraphKeyExchange::BnAuthGraphKeyExchange, Identity::Identity, KeInitResult::KeInitResult,
- Key::Key, PubKey::PubKey, SessionIdSignature::SessionIdSignature, SessionInfo::SessionInfo,
- SessionInitiationInfo::SessionInitiationInfo,
-};
use anyhow::{anyhow, bail, Context, Result};
use apkverify::{HashAlgorithm, V4Signature};
use avflog::LogResult;
@@ -109,10 +102,6 @@
const MICRODROID_OS_NAME: &str = "microdroid";
-// TODO(b/291213394): Use 'default' instance for secretkeeper instead of 'nonsecure'
-const SECRETKEEPER_IDENTIFIER: &str =
- "android.hardware.security.secretkeeper.ISecretkeeper/nonsecure";
-
const UNFORMATTED_STORAGE_MAGIC: &str = "UNFORMATTED-STORAGE";
/// Roughly estimated sufficient size for storing vendor public key into DTBO.
@@ -1381,20 +1370,6 @@
}
}
- fn getSecretkeeper(&self) -> binder::Result<Option<Strong<dyn ISecretkeeper>>> {
- let sk = match binder::get_interface(SECRETKEEPER_IDENTIFIER) {
- Ok(sk) => {
- Some(BnSecretkeeper::new_binder(SecretkeeperProxy(sk), BinderFeatures::default()))
- }
- Err(StatusCode::NAME_NOT_FOUND) => None,
- Err(e) => {
- error!("unexpected error while fetching connection to Secretkeeper {:?}", e);
- return Err(e.into());
- }
- };
- Ok(sk)
- }
-
fn requestAttestation(&self, csr: &[u8]) -> binder::Result<Vec<Certificate>> {
GLOBAL_SERVICE.requestAttestation(csr, get_calling_uid() as i32)
}
@@ -1579,59 +1554,3 @@
Ok(())
}
}
-
-struct SecretkeeperProxy(Strong<dyn ISecretkeeper>);
-
-impl Interface for SecretkeeperProxy {}
-
-impl ISecretkeeper for SecretkeeperProxy {
- fn processSecretManagementRequest(&self, req: &[u8]) -> binder::Result<Vec<u8>> {
- // Pass the request to the channel, and read the response.
- self.0.processSecretManagementRequest(req)
- }
-
- fn getAuthGraphKe(&self) -> binder::Result<Strong<dyn IAuthGraphKeyExchange>> {
- let ag = AuthGraphKeyExchangeProxy(self.0.getAuthGraphKe()?);
- Ok(BnAuthGraphKeyExchange::new_binder(ag, BinderFeatures::default()))
- }
-}
-
-struct AuthGraphKeyExchangeProxy(Strong<dyn IAuthGraphKeyExchange>);
-
-impl Interface for AuthGraphKeyExchangeProxy {}
-
-impl IAuthGraphKeyExchange for AuthGraphKeyExchangeProxy {
- fn create(&self) -> binder::Result<SessionInitiationInfo> {
- self.0.create()
- }
-
- fn init(
- &self,
- peer_pub_key: &PubKey,
- peer_id: &Identity,
- peer_nonce: &[u8],
- peer_version: i32,
- ) -> binder::Result<KeInitResult> {
- self.0.init(peer_pub_key, peer_id, peer_nonce, peer_version)
- }
-
- fn finish(
- &self,
- peer_pub_key: &PubKey,
- peer_id: &Identity,
- peer_signature: &SessionIdSignature,
- peer_nonce: &[u8],
- peer_version: i32,
- own_key: &Key,
- ) -> binder::Result<SessionInfo> {
- self.0.finish(peer_pub_key, peer_id, peer_signature, peer_nonce, peer_version, own_key)
- }
-
- fn authenticationComplete(
- &self,
- peer_signature: &SessionIdSignature,
- shared_keys: &[AuthgraphArc; 2],
- ) -> binder::Result<[AuthgraphArc; 2]> {
- self.0.authenticationComplete(peer_signature, shared_keys)
- }
-}
diff --git a/virtualizationservice/aidl/Android.bp b/virtualizationservice/aidl/Android.bp
index 8ca375a..c69fe8f 100644
--- a/virtualizationservice/aidl/Android.bp
+++ b/virtualizationservice/aidl/Android.bp
@@ -57,10 +57,7 @@
aidl_interface {
name: "android.system.virtualmachineservice",
srcs: ["android/system/virtualmachineservice/**/*.aidl"],
- imports: [
- "android.hardware.security.secretkeeper-V1",
- "android.system.virtualizationcommon",
- ],
+ imports: ["android.system.virtualizationcommon"],
unstable: true,
backend: {
java: {
diff --git a/virtualizationservice/aidl/android/system/virtualmachineservice/IVirtualMachineService.aidl b/virtualizationservice/aidl/android/system/virtualmachineservice/IVirtualMachineService.aidl
index cf91302..3c60478 100644
--- a/virtualizationservice/aidl/android/system/virtualmachineservice/IVirtualMachineService.aidl
+++ b/virtualizationservice/aidl/android/system/virtualmachineservice/IVirtualMachineService.aidl
@@ -15,7 +15,6 @@
*/
package android.system.virtualmachineservice;
-import android.hardware.security.secretkeeper.ISecretkeeper;
import android.system.virtualizationcommon.Certificate;
import android.system.virtualizationcommon.ErrorCode;
@@ -55,11 +54,4 @@
* key's certificate chain. The attestation key is provided in the CSR.
*/
Certificate[] requestAttestation(in byte[] csr);
-
- /**
- * Request connection to Secretkeeper. This is used by pVM to store Anti-Rollback protected
- * secrets. Note that the return value is nullable to reflect that Secretkeeper HAL may not be
- * present.
- */
- @nullable ISecretkeeper getSecretkeeper();
}