MM: Check if Secretkeeper is supported from DT
Virtmgr sets a prop in DT to indicate that Secretkeeper HAL is
supported. Use that as the single source of information from host. This
eliminates the vulnerability that arises when host gives different
answers if asked multiple times.
Test: atest MicrodroidTests#encryptedStorageIsPersistent
Bug: 291213394
Change-Id: I0bb71df64462c90dbf197b9630e7c57a94216388
diff --git a/microdroid_manager/src/main.rs b/microdroid_manager/src/main.rs
index 9a9a1f8..2386bd4 100644
--- a/microdroid_manager/src/main.rs
+++ b/microdroid_manager/src/main.rs
@@ -72,6 +72,7 @@
"/proc/device-tree/virtualization/guest/debug-microdroid,no-verified-boot";
const SECRETKEEPER_KEY: &str = "/proc/device-tree/avf/secretkeeper_public_key";
const INSTANCE_ID_PATH: &str = "/proc/device-tree/avf/untrusted/instance-id";
+const DEFER_ROLLBACK_PROTECTION: &str = "/proc/device-tree/avf/untrusted/defer-rollback-protection";
const ENCRYPTEDSTORE_BIN: &str = "/system/bin/encryptedstore";
const ZIPFUSE_BIN: &str = "/system/bin/zipfuse";
@@ -161,6 +162,10 @@
Ok(instance_id)
}
+fn should_defer_rollback_protection() -> bool {
+ Path::new(DEFER_ROLLBACK_PROTECTION).exists()
+}
+
fn main() -> Result<()> {
// If debuggable, print full backtrace to console log with stdio_to_kmsg
if is_debuggable()? {
@@ -299,10 +304,10 @@
let dice = DiceDriver::new(Path::new("/dev/open-dice0"), is_strict_boot())
.context("Failed to load DICE")?;
- // TODO(b/291306122): Checking with host about Secretkeeper support multiple times introduces
- // a whole range of security vulnerability since host can give different answers. Guest should
- // check only once and the same answer should be known to pVM Firmware and Microdroid.
- let instance_data = if let Some(_sk) = vm_secret::is_sk_supported(service)? {
+ // Microdroid skips checking payload against instance image iff the device supports
+ // secretkeeper. In that case Microdroid use VmSecret::V2, which provide protection against
+ // rollback of boot images and packages.
+ let instance_data = if should_defer_rollback_protection() {
verify_payload(&metadata, None)?
} else {
verify_payload_with_instance_img(&metadata, &dice)?
diff --git a/microdroid_manager/src/vm_secret.rs b/microdroid_manager/src/vm_secret.rs
index 7b65491..ed8ab1d 100644
--- a/microdroid_manager/src/vm_secret.rs
+++ b/microdroid_manager/src/vm_secret.rs
@@ -91,28 +91,19 @@
vm_service: &Strong<dyn IVirtualMachineService>,
) -> Result<Self> {
ensure!(dice_artifacts.bcc().is_some(), "Dice chain missing");
-
- let Some(sk_service) =
- is_sk_supported(vm_service).context("Failed to check if Secretkeeper is supported")?
- else {
- // Use V1 secrets if Secretkeeper is not supported.
+ if !crate::should_defer_rollback_protection() {
return Ok(Self::V1 { dice_artifacts });
- };
+ }
let explicit_dice = OwnedDiceArtifactsWithExplicitKey::from_owned_artifacts(dice_artifacts)
.context("Failed to get Dice artifacts in explicit key format")?;
// For pVM, skp_secret are stored in Secretkeeper. For non-protected it is all 0s.
let mut skp_secret = Zeroizing::new([0u8; SECRET_SIZE]);
if super::is_strict_boot() {
- let mut session = SkSession::new(
- sk_service,
- &explicit_dice,
- Some(get_secretkeeper_identity().context("Failed to get secretkeeper identity")?),
- )
- .context("Failed to setup a Secretkeeper session")?;
- let id = super::get_instance_id()
- .context("Failed to get instance-id")?
- .ok_or(anyhow!("Missing instance-id"))?;
+ let sk_service = get_secretkeeper_service(vm_service)?;
+ let mut session =
+ SkSession::new(sk_service, &explicit_dice, Some(get_secretkeeper_identity()?))?;
+ let id = super::get_instance_id()?.ok_or(anyhow!("Missing instance_id"))?;
let explicit_dice_chain = explicit_dice
.explicit_key_dice_chain()
.ok_or(anyhow!("Missing explicit dice chain, this is unusual"))?;
@@ -279,22 +270,15 @@
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.
-pub fn is_sk_supported(
+fn get_secretkeeper_service(
host: &Strong<dyn IVirtualMachineService>,
-) -> Result<Option<Strong<dyn ISecretkeeper>>> {
- let sk = if cfg!(llpvm_changes) {
- host.getSecretkeeper()
- // TODO rename this error!
- .map_err(|e| {
- super::MicrodroidError::FailedToConnectToVirtualizationService(format!(
- "Failed to get Secretkeeper: {e:?}"
- ))
- })?
- } else {
- // LLPVM flag is disabled
- None
- };
- Ok(sk)
+) -> Result<Strong<dyn ISecretkeeper>> {
+ Ok(host
+ .getSecretkeeper()
+ // TODO rename this error!
+ .map_err(|e| {
+ super::MicrodroidError::FailedToConnectToVirtualizationService(format!(
+ "Failed to get Secretkeeper: {e:?}"
+ ))
+ })?)
}
diff --git a/virtualizationmanager/src/aidl.rs b/virtualizationmanager/src/aidl.rs
index 22bea58..6be219e 100644
--- a/virtualizationmanager/src/aidl.rs
+++ b/virtualizationmanager/src/aidl.rs
@@ -49,7 +49,7 @@
use android_system_virtualmachineservice::aidl::android::system::virtualmachineservice::IVirtualMachineService::{
BnVirtualMachineService, IVirtualMachineService,
};
-use android_hardware_security_secretkeeper::aidl::android::hardware::security::secretkeeper::ISecretkeeper::ISecretkeeper;
+use android_hardware_security_secretkeeper::aidl::android::hardware::security::secretkeeper::ISecretkeeper::{BnSecretkeeper, ISecretkeeper};
use android_hardware_security_secretkeeper::aidl::android::hardware::security::secretkeeper::SecretId::SecretId;
use android_hardware_security_authgraph::aidl::android::hardware::security::authgraph::{
Arc::Arc as AuthgraphArc, IAuthGraphKeyExchange::IAuthGraphKeyExchange,
@@ -1501,11 +1501,12 @@
}
}
- fn getSecretkeeper(&self) -> binder::Result<Option<Strong<dyn ISecretkeeper>>> {
- // TODO(b/327526008): Session establishment wth secretkeeper is failing.
- // Re-enable this when fixed.
- let _sk_supported = is_secretkeeper_supported();
- Ok(None)
+ fn getSecretkeeper(&self) -> binder::Result<Strong<dyn ISecretkeeper>> {
+ if !is_secretkeeper_supported() {
+ return Err(StatusCode::NAME_NOT_FOUND)?;
+ }
+ let sk = binder::wait_for_interface(SECRETKEEPER_IDENTIFIER)?;
+ Ok(BnSecretkeeper::new_binder(SecretkeeperProxy(sk), BinderFeatures::default()))
}
fn requestAttestation(&self, csr: &[u8], test_mode: bool) -> binder::Result<Vec<Certificate>> {
@@ -1514,8 +1515,11 @@
}
fn is_secretkeeper_supported() -> bool {
- binder::is_declared(SECRETKEEPER_IDENTIFIER)
- .expect("Could not check for declared Secretkeeper interface")
+ // TODO(b/327526008): Session establishment wth secretkeeper is failing.
+ // Re-enable this when fixed.
+ let _sk_supported = binder::is_declared(SECRETKEEPER_IDENTIFIER)
+ .expect("Could not check for declared Secretkeeper interface");
+ false
}
impl VirtualMachineService {
diff --git a/virtualizationservice/aidl/android/system/virtualmachineservice/IVirtualMachineService.aidl b/virtualizationservice/aidl/android/system/virtualmachineservice/IVirtualMachineService.aidl
index 6806a5c..662c8f1 100644
--- a/virtualizationservice/aidl/android/system/virtualmachineservice/IVirtualMachineService.aidl
+++ b/virtualizationservice/aidl/android/system/virtualmachineservice/IVirtualMachineService.aidl
@@ -58,9 +58,9 @@
Certificate[] requestAttestation(in byte[] csr, in boolean testMode);
/**
- * 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.
+ * Request connection to Secretkeeper. This is used by pVM to store rollback protected secrets.
+ * Note that this returns error if Secretkeeper is not supported on device. Guest should check
+ * that Secretkeeper is supported from Linux device tree before calling this.
*/
- @nullable ISecretkeeper getSecretkeeper();
+ ISecretkeeper getSecretkeeper();
}