From d6087934b4181f8974524f4c905205c4bfe3ed55 Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Sun, 20 Sep 2026 05:35:41 -0700 Subject: [PATCH] fix(sev-snp-qvl): an empty SNP report in a /verify body aborts the verifier fetch_and_verify takes the AMD KDS branch whenever cert_chain is empty, and that branch handed the report straight to AttestationReport::from_bytes. The `sev` crate reads bytes[0..4] there, and bytes[392] for a version it does not know, without looking at the length, so a report shorter than that is an index out of range -- and release builds are panic = "abort", so it takes down dstack-verifier or dstack-kms rather than failing one request. Both fields are free-form in the attestation blob a POST /verify body carries, and the parse runs before any KDS fetch, signature check or report_data comparison, so an empty report is enough. The other three callers of the same parser already reject anything that is not 1184 bytes, each with its own copy of the check. Make the precondition the parser's own: one decode_amd_snp_report that checks the length and then decodes, and four callers that cannot skip it. thread '...' panicked at sev-6.0.0/src/firmware/guest/types/snp.rs:193:45: range end index 4 out of range for slice of length 0 --- dstack/dstack-attest/tests/sev_snp_verify.rs | 41 +++++++++++ dstack/sev-snp-qvl/src/lib.rs | 77 ++++++++++++++------ 2 files changed, 95 insertions(+), 23 deletions(-) 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];