fix(sev-snp-qvl): an empty SNP report in a /verify body aborts the verifier

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:#}"
+    );
+}

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<ParsedAmdSnpReport> {
-    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<AttestationReport> {
+    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<ParsedAmdSnpReport> {
+    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<VerifiedAmdSnpReport> {
-    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<VerifiedAmdSnpReport> {
-    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];