diff --git a/dstack/dstack-attest/src/attestation.rs b/dstack/dstack-attest/src/attestation.rs index e55d63827..02cd8de52 100644 --- a/dstack/dstack-attest/src/attestation.rs +++ b/dstack/dstack-attest/src/attestation.rs @@ -1657,7 +1657,7 @@ fn decode_app_info_sev_snp( embedded_config: &str, external_vm_config: &str, ) -> Result { - let parsed = crate::amd_sev_snp::parse_amd_snp_report(report)?; + let parsed = crate::amd_sev_snp::parse_unverified_amd_snp_report(report)?; let mr_config_document = if let Some(mr_config) = mr_config { Cow::Borrowed(mr_config) } else if let Some(mr_config) = mr_config_document_from_config(external_vm_config)? { diff --git a/dstack/dstack-attest/src/v1.rs b/dstack/dstack-attest/src/v1.rs index f895a94cf..168fd58a8 100644 --- a/dstack/dstack-attest/src/v1.rs +++ b/dstack/dstack-attest/src/v1.rs @@ -7,7 +7,6 @@ use cc_eventlog::{ tdx::{self, TDX_ACPI_DATA_EVENT_PAYLOAD}, RuntimeEvent, TdxEvent, }; -use dstack_types::mr_config::MrConfigV3; use serde::{Deserialize, Serialize}; use tpm_types::TpmQuote; @@ -155,11 +154,6 @@ impl PlatformEvidence { } } - pub fn sev_snp_mr_config(&self) -> Option { - self.sev_snp_mr_config_document() - .and_then(|document| MrConfigV3::from_document(document).ok()) - } - pub fn tdx_event_log_mut(&mut self) -> Option<&mut Vec> { match self { Self::Tdx { event_log, .. } | Self::GcpTdx { event_log, .. } => Some(event_log), diff --git a/dstack/dstack-mr/src/main.rs b/dstack/dstack-mr/src/main.rs index 7b5e52d53..0b71c00dd 100644 --- a/dstack/dstack-mr/src/main.rs +++ b/dstack/dstack-mr/src/main.rs @@ -134,11 +134,8 @@ fn inspect_measurement(kind: &str, path: &Path) -> Result { .map_err(anyhow::Error::msg), "gcp" => dstack_types::GcpOsImageMeasurement::cbor_json_value_from_slice(&cbor) .map_err(anyhow::Error::msg), - "aws" => { - let measurement = dstack_types::AwsOsImageMeasurement::from_cbor_slice(&cbor) - .map_err(anyhow::Error::msg)?; - serde_json::to_value(measurement).context("failed to convert AWS measurement to JSON") - } + "aws" => dstack_types::AwsOsImageMeasurement::cbor_json_value_from_slice(&cbor) + .map_err(anyhow::Error::msg), other => bail!("unknown measurement kind {other:?}; expected tdx, snp, gcp, or aws"), } } diff --git a/dstack/dstack-mr/src/sev.rs b/dstack/dstack-mr/src/sev.rs index b1d419a49..172fcc1a4 100644 --- a/dstack/dstack-mr/src/sev.rs +++ b/dstack/dstack-mr/src/sev.rs @@ -34,6 +34,37 @@ pub const MAX_OVMF_METADATA_PAGES: u64 = 16_777_216; // VMSA page GPA: (u64)(-1) page-aligned, bits >51 cleared. const VMSA_GPA: u64 = 0x0000_FFFF_FFFF_F000; +/// `SNPActive`, bit 0 of `SEV_FEATURES` (AMD64 APM vol. 2, VMCB SEV_FEATURES; +/// the same bit layout the SEV-SNP ABI's `GUEST_FEATURES` field in +/// `SNP_LAUNCH_START` carries). Set on every SNP guest. +const SEV_FEATURE_SNP_ACTIVE: u64 = 1 << 0; + +/// `SEV_FEATURES` bits a dstack launch may carry. +/// +/// `guest_features` is the only launch parameter that is neither pinned by +/// `SevOsImageMeasurement` nor otherwise constrained: it rides in the +/// host-written `SnpMeasurementDocument` alongside `vcpus`/`vcpu_type`, outside +/// the CBOR that `os_image_hash` commits to, and lands verbatim in both VMSA +/// pages at offset 0x3B0. Because the expected launch digest is *recomputed* +/// from the declared value, a host that really booted the guest with extra +/// feature bits and declares them gets a matching digest and a key release — +/// the bits themselves were never policy-checked. The dangerous direction is +/// DebugSwap (bit 5), which swaps the guest's debug registers on VMEXIT and so +/// exposes guest state to the hypervisor; clearing RestrictedInjection (bit 3), +/// SecureTSC (bit 9) or VmsaRegProt (bit 14) likewise removes a guest-side +/// protection the operator may believe is on. +/// +/// dstack has exactly one launch path, and it sets no feature property: +/// `sev-snp-guest,id=sev0,policy=0x30000,...` in `dstack/vmm/src/app/qemu.rs`, +/// which leaves KVM to start the guest with `SNPActive` alone, and the VMM +/// writes the matching `guest_features: 1` into the measurement document +/// (`dstack/vmm/src/app.rs`). `1` is therefore the only value dstack has ever +/// produced — the captured real-hardware vector in this module's tests carries +/// it too — so it is the only value accepted. Widening this is a one-line +/// change once a launch path actually sets another bit; accepting bits nothing +/// emits would only ever admit a guest dstack did not configure. +const ALLOWED_GUEST_FEATURES: u64 = SEV_FEATURE_SNP_ACTIVE; + #[derive(Debug, Clone, PartialEq, Eq, serde::Deserialize, serde::Serialize)] #[serde(deny_unknown_fields)] pub struct OvmfSectionParam { @@ -111,8 +142,16 @@ where /// Validate a `MeasurementInput` for shape/bounds before recomputation. pub fn validate_measurement_input(input: &MeasurementInput) -> Result<()> { - if input.guest_features == 0 { - bail!("guest_features must be non-zero"); + if input.guest_features & SEV_FEATURE_SNP_ACTIVE == 0 { + bail!("guest_features must set SNPActive (bit 0)"); + } + let unexpected_features = input.guest_features & !ALLOWED_GUEST_FEATURES; + if unexpected_features != 0 { + bail!( + "guest_features {:#x} carries unsupported SEV_FEATURES bits {unexpected_features:#x}; \ + dstack launches amd sev-snp guests with SNPActive only ({ALLOWED_GUEST_FEATURES:#x})", + input.guest_features + ); } rootfs_hash_from_cmdline(Some(&input.base_cmdline))?; @@ -836,16 +875,33 @@ fn file_sha256(path: &Path) -> Result> { Ok(Sha256::digest(data).to_vec()) } +/// Read the rootfs identity the measured kernel command line commits to. +/// +/// A duplicated `dstack.rootfs_hash=` is rejected rather than resolved. The +/// Linux command line has no "first wins" rule -- the dstack initramfs that +/// mounts the rootfs reads the last occurrence -- so returning the first, as +/// this did, would have answered with an identity the guest did not use. Taking +/// the last would agree with the initramfs but would still accept a measured +/// command line asserting two different rootfs identities, which is not a shape +/// any dstack image produces and not one a reader can disambiguate. Callers use +/// this as a validation gate on a command line an untrusted host supplies, so +/// it fails closed. pub fn rootfs_hash_from_cmdline(cmdline: Option<&str>) -> Result { - let rootfs_hash = cmdline - .unwrap_or_default() - .split_whitespace() - .find_map(|param| param.strip_prefix("dstack.rootfs_hash=")) - .map(ToString::to_string) - .context("dstack.rootfs_hash is required in amd sev-snp measured cmdline")?; + let mut rootfs_hash = None; + for param in cmdline.unwrap_or_default().split_whitespace() { + let Some(value) = param.strip_prefix("dstack.rootfs_hash=") else { + continue; + }; + if rootfs_hash.is_some() { + bail!("dstack.rootfs_hash appears more than once in the measured cmdline"); + } + rootfs_hash = Some(value); + } + let rootfs_hash = + rootfs_hash.context("dstack.rootfs_hash is required in amd sev-snp measured cmdline")?; Ok(hex::encode(decode_required_hex( "dstack.rootfs_hash", - &rootfs_hash, + rootfs_hash, 32, )?)) } @@ -1527,6 +1583,103 @@ mod tests { assert_eq!(binding.mr_config.app_id, mr_config.app_id); } + /// A host that really boots the guest with DebugSwap and declares it is not + /// tampering: every field is internally consistent, the recomputed launch + /// digest matches the hardware `MEASUREMENT`, and `host_data` matches the + /// mr_config. The only thing standing between that guest and a key release + /// is a policy check on `guest_features` itself. + /// Bit positions from the AMD64 APM `SEV_FEATURES` table, which the SEV-SNP + /// ABI `GUEST_FEATURES` field in `SNP_LAUNCH_START` mirrors. + const SNP_ACTIVE: u64 = 1 << 0; + const DEBUG_SWAP: u64 = 1 << 5; + + /// `find_map` took the first `dstack.rootfs_hash=`, while the initramfs + /// that actually mounts the rootfs honours the last one. The function is + /// `pub` and reads authoritative, so a caller that trusted it would have + /// been told a different rootfs identity than the guest used. + #[test] + fn a_duplicated_rootfs_hash_is_not_silently_resolved() { + let first = hex_of(0x11, 32); + let last = hex_of(0x22, 32); + let cmdline = format!( + "console=ttyS0 dstack.rootfs_hash={first} init=/init dstack.rootfs_hash={last}" + ); + let err = match rootfs_hash_from_cmdline(Some(&cmdline)) { + Ok(hash) => panic!("a duplicated rootfs hash resolved to {hash}"), + Err(err) => err.to_string(), + }; + assert!( + err.contains("dstack.rootfs_hash appears more than once"), + "unexpected error: {err}" + ); + + // A single occurrence is unchanged. + let cmdline = format!("console=ttyS0 dstack.rootfs_hash={first}"); + assert_eq!( + rootfs_hash_from_cmdline(Some(&cmdline)).expect("single occurrence"), + first + ); + } + + #[test] + fn verify_sev_launch_rejects_a_consistent_debugswap_guest() { + let mut input = valid_input(); + input.guest_features = SNP_ACTIVE | DEBUG_SWAP; + let mr_config = synthetic_mr_config(); + let host_data = MrConfigV3::snp_host_data_from_document(&mr_config.to_canonical_json()); + let measurement = compute_expected_measurement(&input).expect("measurement"); + let vm_config = synthetic_vm_config(&input, &mr_config); + + let err = match verify_sev_launch(&measurement, &host_data, &vm_config) { + Ok(binding) => panic!( + "a DebugSwap launch verified; bound os_image_hash {}", + hex::encode(binding.os_image_hash) + ), + Err(err) => err.to_string(), + }; + assert!( + err.contains("unsupported SEV_FEATURES bits 0x20"), + "unexpected error: {err}" + ); + } + + #[test] + fn rejects_guest_features_outside_the_launch_allowlist() { + let cases: [(&str, u64); 4] = [ + ("RestrictedInjection", SNP_ACTIVE | (1 << 3)), + ("DebugSwap", SNP_ACTIVE | (1 << 5)), + ("SecureTSC", SNP_ACTIVE | (1 << 9)), + ("reserved bit 63", SNP_ACTIVE | (1 << 63)), + ]; + for (name, features) in cases { + let mut input = valid_input(); + input.guest_features = features; + let err = validate_measurement_input(&input) + .expect_err("feature bit outside the allowlist must not be accepted") + .to_string(); + assert!( + err.contains("unsupported SEV_FEATURES bits"), + "{name}: unexpected error: {err}" + ); + } + + // SNPActive missing is still rejected, now by name. + let mut input = valid_input(); + input.guest_features = 0; + let err = validate_measurement_input(&input) + .expect_err("guest_features 0 must not be accepted") + .to_string(); + assert!( + err.contains("must set SNPActive"), + "unexpected error: {err}" + ); + + // The one value dstack's launch path produces stays accepted. + let mut input = valid_input(); + input.guest_features = SNP_ACTIVE; + validate_measurement_input(&input).expect("SNPActive-only launch is accepted"); + } + #[test] fn verify_sev_launch_rejects_forged_measurement() { let (_input, _mr, measurement, host_data, vm_config) = honest_case(); @@ -1579,7 +1732,12 @@ mod tests { ("vcpu_type", |i| { i.vcpu_type = Some("epyc-milan".to_string()) }), - ("guest_features", |i| i.guest_features = 3), + // guest_features is not in this list: the SEV_FEATURES allowlist + // now rejects every value but SNPActive before the measurement is + // ever recomputed, so there is no tampered value left to reach the + // measurement comparison. See + // `rejects_guest_features_outside_the_launch_allowlist` and + // `verify_sev_launch_rejects_a_consistent_debugswap_guest`. ]; for (name, mutate) in cases { let mut tampered = input.clone(); diff --git a/dstack/dstack-types/src/lib.rs b/dstack/dstack-types/src/lib.rs index 944bf12e1..5317c9bcb 100644 --- a/dstack/dstack-types/src/lib.rs +++ b/dstack/dstack-types/src/lib.rs @@ -1524,12 +1524,28 @@ fn cbor_to_vec(value: &T, context: &str) -> Vec { out } +/// Decode one CBOR document, and only one. +/// +/// Every caller is decoding a fixed-size measurement document whose bytes are +/// also hashed into `os_image_hash`, so a decoder that stops at the end of the +/// first item and ignores the rest lets one byte string decode to a value it +/// does not hash as. Nothing exploits that today because the verification path +/// hashes the raw bytes (`verify_measurement_material`), but the ambiguity is +/// cheap to remove here and expensive to notice later. fn cbor_from_slice( bytes: &[u8], context: &str, ) -> Result { - ciborium::de::from_reader(Cursor::new(bytes)) - .map_err(|e| format!("{context}: failed to decode CBOR: {e}")) + let mut cursor = Cursor::new(bytes); + let value = ciborium::de::from_reader(&mut cursor) + .map_err(|e| format!("{context}: failed to decode CBOR: {e}"))?; + let trailing = bytes.len() as u64 - cursor.position(); + if trailing != 0 { + return Err(format!( + "{context}: {trailing} trailing byte(s) after the CBOR document" + )); + } + Ok(value) } fn sha256(bytes: &[u8]) -> [u8; 32] { @@ -1540,6 +1556,7 @@ fn sha256(bytes: &[u8]) -> [u8; 32] { pub const TDX_MEASUREMENT_FILENAME: &str = "measurement.tdx.cbor"; pub const SNP_MEASUREMENT_FILENAME: &str = "measurement.snp.cbor"; pub const GCP_MEASUREMENT_FILENAME: &str = "measurement.gcp.cbor"; +pub const AWS_MEASUREMENT_FILENAME: &str = "measurement.aws.cbor"; pub fn image_hash_from_sha256sum(checksum_file: &[u8]) -> [u8; 32] { sha256(checksum_file) @@ -1747,7 +1764,32 @@ pub struct AwsOsImageMeasurement { pub boot_pcr_digest: Vec, } +/// On-wire mirror of [`AwsOsImageMeasurement`], matching +/// `CborGcpOsImageMeasurement`: the document names its schema version so a +/// future shape fails closed on an old verifier instead of decoding as this +/// one. Like its three siblings it does not `deny_unknown_fields`: the version +/// gate below is the fail-closed mechanism, and rejecting on the unknown field +/// first would replace its diagnostic with `unknown field ...`. The bytes are +/// bound to `os_image_hash` by the `measurement.aws.cbor` entry in +/// `sha256sum.txt`, so an ignored field cannot change what was measured. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +struct CborAwsOsImageMeasurement { + version: u32, + #[serde(with = "hex_bytes")] + boot_pcr_digest: Vec, +} + +impl From<&AwsOsImageMeasurement> for CborAwsOsImageMeasurement { + fn from(measurement: &AwsOsImageMeasurement) -> Self { + Self { + version: AwsOsImageMeasurement::VERSION, + boot_pcr_digest: measurement.boot_pcr_digest.clone(), + } + } +} + impl AwsOsImageMeasurement { + pub const VERSION: u32 = 1; pub const BOOT_PCR_DIGEST_LEN: usize = 32; pub const PCR_SHA384_LEN: usize = 48; @@ -1782,13 +1824,31 @@ impl AwsOsImageMeasurement { } pub fn to_cbor_vec(&self) -> Vec { - cbor_to_vec(self, "AwsOsImageMeasurement") + cbor_to_vec( + &CborAwsOsImageMeasurement::from(self), + "AwsOsImageMeasurement", + ) } pub fn from_cbor_slice(bytes: &[u8]) -> Result { - let measurement: Self = cbor_from_slice(bytes, "AwsOsImageMeasurement")?; + let measurement: CborAwsOsImageMeasurement = + cbor_from_slice(bytes, "AwsOsImageMeasurement")?; + if measurement.version != Self::VERSION { + return Err(format!( + "AwsOsImageMeasurement: unsupported version {}, expected {}", + measurement.version, + Self::VERSION + )); + } Self::new(measurement.boot_pcr_digest) } + + pub fn cbor_json_value_from_slice(bytes: &[u8]) -> Result { + let measurement: CborAwsOsImageMeasurement = + cbor_from_slice(bytes, "AwsOsImageMeasurement")?; + serde_json::to_value(measurement) + .map_err(|e| format!("AwsOsImageMeasurement: failed to convert to JSON: {e}")) + } } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -1819,7 +1879,7 @@ impl AwsOsImageMeasurementDocument { os_image_hash, &self.checksum_file, &self.measurement, - "measurement.aws.cbor", + AWS_MEASUREMENT_FILENAME, ) } } @@ -2876,3 +2936,88 @@ mod tdx_measurement_cbor_tests { assert!(err.contains("unsupported version 2"), "unexpected: {err}"); } } + +#[cfg(test)] +mod cbor_canonicalization_tests { + use super::*; + + fn sev_measurement() -> SevOsImageMeasurement { + SevOsImageMeasurement { + base_cmdline: "console=ttyS0 dstack.rootfs_hash=11".to_string(), + ovmf_hash: vec![0x44; 48], + kernel_hash: vec![0x55; 32], + initrd_hash: vec![0x66; 32], + sev_hashes_table_gpa: 0x80_1000, + sev_es_reset_eip: 0xffff_fff0, + ovmf_sections: vec![OvmfSection { + gpa: 0x100000, + size: 0x1000, + section_type: 1, + }], + } + } + + /// Every `*_MEASUREMENT_FILENAME` document is bound to `os_image_hash` by a + /// hash over its raw bytes, so today the trailing bytes ride along in that + /// hash and no live check is bypassed. That property is an accident of the + /// call sites, not of the decoder: the moment anything compares a + /// re-encoded `measurement_hash()` against a `sha256sum.txt` entry, a + /// document with trailing bytes decodes to one value and hashes as another. + /// Decoding is where the ambiguity belongs. + #[test] + fn cbor_decoders_reject_trailing_bytes() { + let mut cbor = sev_measurement().to_cbor_vec(); + let clean = SevOsImageMeasurement::from_cbor_slice(&cbor).expect("clean document decodes"); + cbor.push(0x00); + + let err = SevOsImageMeasurement::from_cbor_slice(&cbor) + .expect_err("a document with trailing bytes must not decode"); + assert!(err.contains("trailing"), "unexpected error: {err}"); + + // The re-encoded hash of what it decodes to is not the hash of the + // bytes it was decoded from -- the ambiguity the check removes. + assert_ne!(clean.measurement_hash().to_vec(), sha256(&cbor).to_vec()); + + // Same decoder, so every other measurement document is covered too. + let mut gcp = GcpOsImageMeasurement::new(vec![0x77; 32]) + .expect("gcp measurement") + .to_cbor_vec(); + gcp.extend_from_slice(b"junk"); + let err = GcpOsImageMeasurement::from_cbor_slice(&gcp) + .expect_err("a GCP document with trailing bytes must not decode"); + assert!(err.contains("trailing"), "unexpected error: {err}"); + } + + /// Each of `measurement.{tdx,snp,gcp}.cbor` encodes a mirror struct whose + /// first field is `version`, checked on decode, so a future schema change + /// fails closed on old verifiers. `measurement.aws.cbor` encoded its public + /// struct directly and named no version at all, which would have left a v2 + /// document decoding as a v1 one. + #[test] + fn the_aws_measurement_document_names_and_checks_its_version() { + let cbor = AwsOsImageMeasurement::new(vec![0x77; 32]) + .expect("aws measurement") + .to_cbor_vec(); + + let value: ciborium::value::Value = + ciborium::de::from_reader(Cursor::new(&cbor[..])).expect("document decodes as CBOR"); + let entries = value.as_map().expect("document is a CBOR map"); + let version = entries + .iter() + .find(|(key, _)| key.as_text() == Some("version")) + .map(|(_, value)| value.clone()) + .expect("measurement.aws.cbor names a version, like its three siblings"); + assert_eq!(version.as_integer(), Some(1u32.into())); + + let forged = cbor_to_vec( + &CborAwsOsImageMeasurement { + version: 2, + boot_pcr_digest: vec![0x77; 32], + }, + "AwsOsImageMeasurement", + ); + let err = AwsOsImageMeasurement::from_cbor_slice(&forged) + .expect_err("a v2 document must not decode as v1"); + assert!(err.contains("unsupported version 2"), "unexpected: {err}"); + } +} diff --git a/dstack/dstack-types/src/mr_config.rs b/dstack/dstack-types/src/mr_config.rs index ce38f4e7c..d11c3a5aa 100644 --- a/dstack/dstack-types/src/mr_config.rs +++ b/dstack/dstack-types/src/mr_config.rs @@ -105,7 +105,9 @@ impl From for MrConfigDocumentError { #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(deny_unknown_fields)] pub struct MrConfigV3 { - #[serde(default = "mr_config_v3_version")] + /// Document schema version, always 3. No serde default: `validate_mr_config` + /// rejects anything but 3, and a default would have made an absent version + /// mean 3 rather than fail that gate. pub version: u8, /// Optional application identity pin. #[serde(default, with = "hex_bytes")] @@ -251,7 +253,7 @@ mod tests { #[test] fn mr_config_v3_defaults_missing_app_id_to_empty() -> Result<(), Box> { let config = MrConfigV3::from_document( - r#"{"compose_hash":"2222222222222222222222222222222222222222222222222222222222222222","key_provider":"none"}"#, + r#"{"version":3,"compose_hash":"2222222222222222222222222222222222222222222222222222222222222222","key_provider":"none"}"#, )?; assert!(config.app_id.is_none()); @@ -316,4 +318,21 @@ mod tests { ); Ok(()) } + + /// `validate_mr_config` gates on `version != 3`, which only means anything + /// if an absent `version` is absent. A serde default made every document + /// that never mentions a version pass that gate as a v3 document. + #[test] + fn a_document_without_a_version_is_not_a_v3_document() { + let err = MrConfigV3::from_document(concat!( + "{\"compose_hash\":", + "\"2222222222222222222222222222222222222222222222222222222222222222\",", + "\"key_provider\":\"none\"}" + )) + .expect_err("a document without a version must not parse as v3"); + assert!( + err.to_string().contains("version"), + "unexpected error: {err}" + ); + } } diff --git a/dstack/dstack-util/src/system_setup/config_id_verifier.rs b/dstack/dstack-util/src/system_setup/config_id_verifier.rs index 3e71cd76c..fdb00c034 100644 --- a/dstack/dstack-util/src/system_setup/config_id_verifier.rs +++ b/dstack/dstack-util/src/system_setup/config_id_verifier.rs @@ -48,7 +48,7 @@ fn read_snp_host_data() -> Result<[u8; 32]> { let AttestationQuote::DstackAmdSevSnp(quote) = attestation.quote else { bail!("attestation mode is not AMD SEV-SNP"); }; - let parsed = dstack_attest::amd_sev_snp::parse_amd_snp_report("e.report) + let parsed = dstack_attest::amd_sev_snp::parse_unverified_amd_snp_report("e.report) .context("Failed to parse SNP report")?; Ok(parsed.host_data) } diff --git a/dstack/kms/src/main_service/amd_attest.rs b/dstack/kms/src/main_service/amd_attest.rs index a9a943131..f34c26da3 100644 --- a/dstack/kms/src/main_service/amd_attest.rs +++ b/dstack/kms/src/main_service/amd_attest.rs @@ -786,7 +786,13 @@ mod tests { fn rejects_unsafe_machine_config() { let mut input = valid_input(); input.guest_features = 0; - assert_rejects(input, "guest_features must be non-zero"); + assert_rejects(input, "guest_features must set SNPActive"); + + // DebugSwap hands guest debug state to the host; dstack never launches + // with it, so the KMS must not recompute a matching digest for it. + let mut input = valid_input(); + input.guest_features = 1 | (1 << 5); + assert_rejects(input, "unsupported SEV_FEATURES bits"); let mut input = valid_input(); input.ovmf_sections[0].size = 0; diff --git a/dstack/sev-snp-qvl/src/lib.rs b/dstack/sev-snp-qvl/src/lib.rs index b370b14bf..68883363b 100644 --- a/dstack/sev-snp-qvl/src/lib.rs +++ b/dstack/sev-snp-qvl/src/lib.rs @@ -443,7 +443,13 @@ fn decode_amd_snp_report(report_bytes: &[u8]) -> Result { .map_err(|err| anyhow!("failed to parse amd sev-snp report: {err}")) } -pub fn parse_amd_snp_report(report_bytes: &[u8]) -> Result { +/// Decode an attestation report's fields without checking its authenticity. +/// +/// This does not verify the report signature or the AMD certificate chain. +/// Use [`verify_amd_snp_attestation`] when authenticity is in question; use +/// this only where the signature is established elsewhere, or for the guest's +/// own report. +pub fn parse_unverified_amd_snp_report(report_bytes: &[u8]) -> Result { let report = decode_amd_snp_report(report_bytes)?; parsed_amd_snp_report_from_report(&report) } diff --git a/dstack/tpm-qvl/src/verify.rs b/dstack/tpm-qvl/src/verify.rs index 21c31167a..04bda0329 100644 --- a/dstack/tpm-qvl/src/verify.rs +++ b/dstack/tpm-qvl/src/verify.rs @@ -115,6 +115,39 @@ pub fn verify_quote_with_ca( }); } + // The attested bank fixes both the digest length of every quoted PCR and + // the name `PcrValue` must carry, and neither was checked. Length matters + // because compute_pcr_digest() concatenates the values with nothing marking + // where one ends: a caller can re-split the preimage of a genuine, + // AK-signed pcr_digest into differently sized values, match the digest, and + // have VerifiedReport::get_pcr() hand out a PCR the TPM never held. The + // name matters because it is the only thing a consumer of VerifiedReport + // can read to learn which bank it is looking at. + const SHA256_BANK: &str = "sha256"; + const SHA256_DIGEST_LEN: usize = 32; + for pcr in "e.pcr_values { + if pcr.algorithm != SHA256_BANK { + return Err(VerificationError { + status: status.clone(), + error: anyhow!( + "PCR {} is labelled {:?}, but the quote attests the {SHA256_BANK} bank", + pcr.index, + pcr.algorithm + ), + }); + } + if pcr.value.len() != SHA256_DIGEST_LEN { + return Err(VerificationError { + status: status.clone(), + error: anyhow!( + "PCR {} value is {} bytes, expected {SHA256_DIGEST_LEN} for the {SHA256_BANK} bank", + pcr.index, + pcr.value.len() + ), + }); + } + } + let computed_pcr_digest = compute_pcr_digest("e.pcr_values).map_err(|e| VerificationError { status: status.clone(), @@ -687,3 +720,127 @@ fn verify_ak_chain_with_collateral( } } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::{QuoteCollateral, GCP_ROOT_CA}; + + /// Everything a quote must satisfy to reach `compute_pcr_digest` is + /// reproducible without a TPM: the PCR selection list lives in the + /// still-unsigned TPMS_ATTEST, and `pcr_digest` is a plain SHA-256 over the + /// supplied PCR values. Both gates run before the AK signature is checked. + fn attest_message(pcr_indices: &[u8], hash_alg: u16, pcr_digest: &[u8]) -> Vec { + let mut msg = Vec::new(); + msg.extend_from_slice(&0xff544347u32.to_be_bytes()); // magic + msg.extend_from_slice(&0x8018u16.to_be_bytes()); // TPM_ST_ATTEST_QUOTE + msg.extend_from_slice(&0u16.to_be_bytes()); // qualified_signer + msg.extend_from_slice(&0u16.to_be_bytes()); // qualified_data + msg.extend_from_slice(&0u64.to_be_bytes()); // clock + msg.extend_from_slice(&0u32.to_be_bytes()); // reset_count + msg.extend_from_slice(&0u32.to_be_bytes()); // restart_count + msg.push(1); // safe + msg.extend_from_slice(&0u64.to_be_bytes()); // firmware_version + msg.extend_from_slice(&1u32.to_be_bytes()); // one selection + msg.extend_from_slice(&hash_alg.to_be_bytes()); + msg.push(2); // sizeof_select + let mut bitmap = [0u8; 2]; + for index in pcr_indices { + bitmap[(index / 8) as usize] |= 1 << (index % 8); + } + msg.extend_from_slice(&bitmap); + msg.extend_from_slice(&(pcr_digest.len() as u16).to_be_bytes()); + msg.extend_from_slice(pcr_digest); + msg + } + + fn quote_of(pcr_values: Vec, message: Vec) -> TpmQuote { + TpmQuote { + message, + signature: Vec::new(), + pcr_values, + ak_cert: Vec::new(), + platform: Platform::Gcp, + event_log: Vec::new(), + } + } + + fn empty_collateral() -> QuoteCollateral { + QuoteCollateral { + cert_chain_pem: String::new(), + crls: Vec::new(), + root_ca_crl: None, + } + } + + /// `compute_pcr_digest` concatenates the PCR values with nothing marking + /// where one ends and the next begins, so a caller can move the boundary. + /// The signed `pcr_digest` still matches, the indices still match the signed + /// selection, and `VerifiedReport::get_pcr` then hands out a PCR 4 value + /// that no TPM ever held. + #[test] + fn rejects_pcr_values_whose_lengths_do_not_match_the_attested_bank() { + let pcr4 = vec![0xaa; 32]; + let pcr7 = vec![0xbb; 32]; + let mut concatenated = pcr4.clone(); + concatenated.extend_from_slice(&pcr7); + let genuine_digest = Sha256::digest(&concatenated).to_vec(); + + // Same bytes, boundary moved one byte left. + let shifted = vec![ + PcrValue { + index: 4, + algorithm: "sha256".into(), + value: concatenated[..31].to_vec(), + }, + PcrValue { + index: 7, + algorithm: "sha256".into(), + value: concatenated[31..].to_vec(), + }, + ]; + let message = attest_message(&[4, 7], 0x000b, &genuine_digest); + let err = match verify_quote_with_ca( + "e_of(shifted, message), + &empty_collateral(), + GCP_ROOT_CA, + ) { + Ok(report) => panic!( + "a re-split PCR list verified; get_pcr(4) = {}", + hex::encode(report.get_pcr(4).expect("PCR 4")) + ), + Err(err) => err.error.to_string(), + }; + assert!( + err.contains("PCR 4") && err.contains("31 bytes"), + "unexpected error: {err}" + ); + } + + /// `PcrValue::algorithm` is the only place a consumer of `VerifiedReport` + /// can read which bank the values came from, and nothing compared it against + /// the bank the signed TPMS_ATTEST names. + #[test] + fn rejects_pcr_values_that_misname_the_attested_bank() { + let pcr4 = vec![0xaa; 32]; + let digest = Sha256::digest(&pcr4).to_vec(); + let values = vec![PcrValue { + index: 4, + algorithm: "sha384".into(), + value: pcr4, + }]; + let message = attest_message(&[4], 0x000b, &digest); + let err = match verify_quote_with_ca( + "e_of(values, message), + &empty_collateral(), + GCP_ROOT_CA, + ) { + Ok(_) => panic!("a mislabelled PCR bank verified"), + Err(err) => err.error.to_string(), + }; + assert!( + err.contains("PCR 4") && err.contains("sha384") && err.contains("sha256"), + "unexpected error: {err}" + ); + } +} diff --git a/dstack/vmm/src/app/image.rs b/dstack/vmm/src/app/image.rs index fcb5600f6..ae941fb74 100644 --- a/dstack/vmm/src/app/image.rs +++ b/dstack/vmm/src/app/image.rs @@ -10,11 +10,11 @@ use anyhow::{bail, Context, Result}; use dstack_types::{ version::Version, AwsOsImageMeasurementDocument, AwsPcrReplay, GcpOsImageMeasurementDocument, GcpTpmReplay, SevOsImageMeasurementDocument, TdxOsImageMeasurementDocument, - GCP_MEASUREMENT_FILENAME, SNP_MEASUREMENT_FILENAME, TDX_MEASUREMENT_FILENAME, + AWS_MEASUREMENT_FILENAME, GCP_MEASUREMENT_FILENAME, SNP_MEASUREMENT_FILENAME, + TDX_MEASUREMENT_FILENAME, }; use serde::{Deserialize, Serialize}; -const AWS_MEASUREMENT_FILENAME: &str = "measurement.aws.cbor"; const AWS_PCR_REPLAY_FILENAME: &str = "measurement.aws.replay.json"; const GCP_TPM_EVENT_LOG_FILENAME: &str = "measurement.gcp.eventlog.bin";