diff --git a/dstack/dstack-attest/tests/sev_snp_verify.rs b/dstack/dstack-attest/tests/sev_snp_verify.rs index 38a19145c..73455dc74 100644 --- a/dstack/dstack-attest/tests/sev_snp_verify.rs +++ b/dstack/dstack-attest/tests/sev_snp_verify.rs @@ -406,3 +406,44 @@ fn advertised_os_image_hash_must_match_sha256sum() { "unexpected error: {err:?}" ); } + +/// The whole `POST /verify` path, with the SNP report emptied. +/// +/// `dstack-verifier` decodes the request body with +/// `VersionedAttestation::from_bytes` and hands the result straight to +/// `verify`, which routes an SNP quote with no `cert_chain` to the AMD KDS +/// branch. That branch used to parse the report without checking its length, +/// and release builds are `panic = "abort"`, so this body took the verifier +/// process down rather than failing one request. +#[tokio::test] +async fn an_empty_snp_report_from_the_verify_body_is_rejected_rather_than_parsed() { + let VersionedAttestation::V0 { mut attestation } = + VersionedAttestation::from_scale(SEV_ATTESTATION_BIN).expect("decode VersionedAttestation") + else { + panic!("expected V0 attestation"); + }; + let AttestationQuote::DstackAmdSevSnp(quote) = &mut attestation.quote else { + panic!("expected an AMD SEV-SNP quote"); + }; + quote.report = Vec::new(); + quote.cert_chain = Vec::new(); + + let body = attestation + .into_versioned() + .to_bytes() + .expect("encode attestation"); + let verifier = dstack_attest::attestation::AttestationVerifier::new_prod(None) + .expect("build a production verifier"); + let Err(err) = VersionedAttestation::from_bytes(&body) + .expect("decode attestation") + .into_v1() + .verify(&verifier) + .await + else { + panic!("an empty SNP report must be rejected"); + }; + assert!( + format!("{err:#}").contains("invalid amd sev-snp report length"), + "unexpected error: {err:#}" + ); +} diff --git a/dstack/sev-snp-qvl/src/lib.rs b/dstack/sev-snp-qvl/src/lib.rs index 1fa4da6f5..b370b14bf 100644 --- a/dstack/sev-snp-qvl/src/lib.rs +++ b/dstack/sev-snp-qvl/src/lib.rs @@ -27,6 +27,9 @@ const VLEK_CERT_GUID: [u8; 16] = [ 0xa8, 0x07, 0x4b, 0xc2, 0xa2, 0x5a, 0x48, 0x3e, 0xaa, 0xe6, 0x39, 0xc0, 0x45, 0xa0, 0xb8, 0xa1, ]; const CERT_TABLE_ENTRY_SIZE: usize = 24; +/// Size of an AMD SEV-SNP attestation report, for every version the `sev` +/// crate decodes. +const AMD_SNP_REPORT_LEN: usize = 1184; pub const AMD_KDS_DEFAULT_BASE_URL: &str = "https://kdsintf.amd.com/vcek/v1"; const AMD_KDS_CONNECT_TIMEOUT: Duration = Duration::from_secs(30); const AMD_KDS_REQUEST_TIMEOUT: Duration = Duration::from_secs(120); @@ -142,8 +145,7 @@ impl QuoteVerifier { if !cert_chain.is_empty() { return self.verify(report, cert_chain, expected_report_data); } - let report_obj = AttestationReport::from_bytes(report) - .map_err(|err| anyhow!("failed to parse amd sev-snp report: {err}"))?; + let report_obj = decode_amd_snp_report(report)?; let mut errors = Vec::new(); for product in amd_snp_product_candidates_for_report(&report_obj)? { match kds_client @@ -421,15 +423,28 @@ pub fn verify_amd_snp_attestation( ) } -pub fn parse_amd_snp_report(report_bytes: &[u8]) -> Result { - if report_bytes.len() != 1184 { +/// Decode an attestation report, rejecting anything that is not report-sized +/// first. +/// +/// `AttestationReport::from_bytes` indexes `bytes[0..4]` and, for a version it +/// does not know, `bytes[392]`, without looking at the length: a report shorter +/// than that aborts the process, because release builds are `panic = "abort"` +/// (`dstack/Cargo.toml`). The report arrives in an unverified attestation blob, +/// so the length has to be checked on every path that reaches this parser, not +/// on the three that happened to. +fn decode_amd_snp_report(report_bytes: &[u8]) -> Result { + if report_bytes.len() != AMD_SNP_REPORT_LEN { bail!( - "invalid amd sev-snp report length: expected 1184 bytes, got {}", + "invalid amd sev-snp report length: expected {AMD_SNP_REPORT_LEN} bytes, got {}", report_bytes.len() ); } - let report = AttestationReport::from_bytes(report_bytes) - .map_err(|err| anyhow::anyhow!("failed to parse amd sev-snp report: {err}"))?; + AttestationReport::from_bytes(report_bytes) + .map_err(|err| anyhow!("failed to parse amd sev-snp report: {err}")) +} + +pub fn parse_amd_snp_report(report_bytes: &[u8]) -> Result { + let report = decode_amd_snp_report(report_bytes)?; parsed_amd_snp_report_from_report(&report) } @@ -491,14 +506,7 @@ fn verify_amd_snp_attestation_with_certs_and_arks( vcek_bytes: CertBytes, ark_for_product: impl Fn(AmdSnpProduct) -> (CertBytes, bool), ) -> Result { - if report_bytes.len() != 1184 { - bail!( - "invalid amd sev-snp report length: expected 1184 bytes, got {}", - report_bytes.len() - ); - } - let report = AttestationReport::from_bytes(report_bytes) - .map_err(|err| anyhow::anyhow!("failed to parse amd sev-snp report: {err}"))?; + let report = decode_amd_snp_report(report_bytes)?; let mut errors = Vec::new(); for product in amd_snp_product_candidates_for_report(&report)? { let (ark, external_root) = ark_for_product(product); @@ -526,14 +534,7 @@ fn verify_amd_snp_attestation_with_cert_chain( vcek_bytes: CertBytes, external_root: bool, ) -> Result { - if report_bytes.len() != 1184 { - bail!( - "invalid amd sev-snp report length: expected 1184 bytes, got {}", - report_bytes.len() - ); - } - let report = AttestationReport::from_bytes(report_bytes) - .map_err(|err| anyhow::anyhow!("failed to parse amd sev-snp report: {err}"))?; + let report = decode_amd_snp_report(report_bytes)?; let ark = parse_certificate(&ark_bytes, "ark")?; let ask = parse_certificate(&ask_bytes, "ask")?; @@ -988,6 +989,36 @@ mod tests { assert_eq!(stale_vcek_reported.tcb_status(), "OutOfDate"); } + /// `fetch_and_verify` takes the KDS branch whenever `cert_chain` is empty, + /// and that branch reached `AttestationReport::from_bytes` with whatever + /// the requester sent. `from_bytes` reads `bytes[0..4]` and, for an unknown + /// version, `bytes[392]`, unguarded — so an empty report is an abort, not a + /// rejected request. Both shapes come straight out of a `POST /verify` + /// body. + #[test] + fn a_short_report_without_a_cert_chain_is_rejected_rather_than_parsed() { + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("failed to build a test runtime"); + let verifier = QuoteVerifier::new_prod(); + let kds = AmdKdsClient::new().expect("failed to build a KDS client"); + + // Empty: `&bytes[0..4]` is out of range. + // Four bytes of version 3: `bytes[392]` is out of range. + for report in [vec![], vec![0x03, 0x00, 0x00, 0x00], vec![0u8; 1183]] { + let err = runtime + .block_on(verifier.fetch_and_verify(&kds, &report, &[], &[0u8; 64])) + .expect_err("a short report must be rejected"); + assert!( + err.to_string() + .contains("invalid amd sev-snp report length"), + "unexpected error for a {}-byte report: {err:#}", + report.len() + ); + } + } + #[test] fn missing_cert_chain_fails_closed() { let report = vec![0u8; 1184];