fix(verifier): the image manifest check accepts lines sha256sum silently skipped, and the --debug RTMR diff is dead code

dstack/dstack-types/src/lib.rs

@@ -1545,44 +1545,79 @@ pub fn image_hash_from_sha256sum(checksum_file: &[u8]) -> [u8; 32] {
     sha256(checksum_file)
 }
 
-pub fn sha256sum_entry_hash(checksum_file: &[u8], filename: &str) -> Result<[u8; 32], String> {
+/// One `sha256sum.txt` entry: a file name and the digest it is bound to.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct Sha256sumEntry {
+    pub hash: [u8; 32],
+    pub name: String,
+}
+
+/// Parse `sha256sum.txt`, the file whose digest is `os_image_hash`.
+///
+/// Every consumer of this file must read it the same way, because it is the
+/// only thing binding `os_image_hash` to the bytes that get measured. GNU
+/// `sha256sum`'s own grammar is wider than a whitespace split: `<digest> *name`
+/// is binary mode and names `name`, a line beginning with `\` is escaped, and
+/// an improperly formatted line is *skipped with a warning* while `-c` still
+/// exits 0. A parser that disagrees with it on any of those lets one side check
+/// a file the other side never looked at.
+///
+/// So accept exactly the one line shape `sha256sum <files>` emits -- 64 hex
+/// digits, two spaces, a flat file name -- and reject everything else. All
+/// dstack images are assembled with that command (`os/image/assemble.sh`).
+pub fn parse_sha256sum_manifest(checksum_file: &[u8]) -> Result<Vec<Sha256sumEntry>, String> {
     let text = std::str::from_utf8(checksum_file)
         .map_err(|e| format!("sha256sum.txt is not valid UTF-8: {e}"))?;
-    let mut found = None;
-    for (line_no, line) in text.lines().enumerate() {
-        let line = line.trim();
+    let mut entries: Vec<Sha256sumEntry> = Vec::new();
+    for (index, line) in text.split('\n').enumerate() {
+        let line_no = index + 1;
         if line.is_empty() {
             continue;
         }
-        let mut parts = line.split_whitespace();
-        let Some(hash_hex) = parts.next() else {
-            continue;
+        let invalid = |reason: &str| Err(format!("sha256sum.txt line {line_no} {reason}"));
+        let Some((hash_hex, name)) = line.split_once("  ") else {
+            return invalid("is not `<sha256>  <name>`");
         };
-        let Some(path) = parts.next() else {
-            return Err(format!(
-                "sha256sum.txt line {} is missing filename",
-                line_no + 1
-            ));
-        };
-        if path != filename {
-            continue;
-        }
-        if found.is_some() {
-            return Err(format!(
-                "sha256sum.txt contains duplicate {filename} entries"
-            ));
+        if hash_hex.len() != 64 {
+            return invalid("does not start with a 64-digit sha256");
         }
         let hash = hex::decode(hash_hex)
-            .map_err(|e| format!("sha256sum.txt {filename} hash is not valid hex: {e}"))?;
-        let hash: [u8; 32] = hash.try_into().map_err(|hash: Vec<u8>| {
-            format!(
-                "sha256sum.txt {filename} hash has invalid length {}, expected 32",
-                hash.len()
-            )
-        })?;
-        found = Some(hash);
+            .map_err(|e| format!("sha256sum.txt line {line_no} digest is not valid hex: {e}"))?;
+        let hash: [u8; 32] = hash
+            .try_into()
+            .map_err(|_| format!("sha256sum.txt line {line_no} digest is not 32 bytes"))?;
+        if !is_flat_manifest_name(name) {
+            return invalid("does not name a plain file in the image root");
+        }
+        if entries.iter().any(|entry| entry.name == name) {
+            return invalid("repeats a name listed earlier");
+        }
+        entries.push(Sha256sumEntry {
+            hash,
+            name: name.to_string(),
+        });
     }
-    found.ok_or_else(|| format!("sha256sum.txt is missing {filename}"))
+    Ok(entries)
+}
+
+/// A manifest name is resolved relative to the image root by every consumer, so
+/// it must be a plain file name: no directory components, no `.`/`..`, and no
+/// character a checksum tool would strip, escape, or read as a mode marker.
+fn is_flat_manifest_name(name: &str) -> bool {
+    !name.is_empty()
+        && name != "."
+        && name != ".."
+        && !name
+            .chars()
+            .any(|c| c == '/' || c == '\\' || c.is_whitespace() || c.is_control())
+}
+
+pub fn sha256sum_entry_hash(checksum_file: &[u8], filename: &str) -> Result<[u8; 32], String> {
+    parse_sha256sum_manifest(checksum_file)?
+        .into_iter()
+        .find(|entry| entry.name == filename)
+        .map(|entry| entry.hash)
+        .ok_or_else(|| format!("sha256sum.txt is missing {filename}"))
 }
 
 pub fn verify_measurement_material(
@@ -1610,6 +1645,79 @@ pub fn verify_measurement_material(
     Ok(())
 }
 
+#[cfg(test)]
+mod sha256sum_manifest_tests {
+    use super::*;
+
+    const DIGEST: &str = "0000000000000000000000000000000000000000000000000000000000000000";
+
+    #[test]
+    fn manifest_entries_accept_only_the_canonical_sha256sum_line() {
+        let entries =
+            parse_sha256sum_manifest(format!("{DIGEST}  metadata.json\n").as_bytes()).unwrap();
+        assert_eq!(entries.len(), 1);
+        assert_eq!(entries[0].name, "metadata.json");
+
+        // Every one of these is a line `sha256sum -c` reads differently from a
+        // whitespace split: the binary-mode marker and the `./` prefix name a
+        // different file than the token does, a single space is an improperly
+        // formatted line that `sha256sum -c` skips while still exiting 0, and
+        // trailing junk becomes part of the name.
+        for line in [
+            format!("{DIGEST} *metadata.json"),
+            format!("{DIGEST}  ./metadata.json"),
+            format!("{DIGEST} metadata.json"),
+            format!("{DIGEST}  metadata.json  junk"),
+            format!("{DIGEST}   metadata.json"),
+            format!("{DIGEST}  metadata.json\r"),
+            format!("{DIGEST}  ../metadata.json"),
+            format!("{DIGEST}  sub/metadata.json"),
+            format!("{DIGEST}  .."),
+            "00  metadata.json".to_string(),
+            format!("{DIGEST}  "),
+            DIGEST.to_string(),
+        ] {
+            assert!(
+                parse_sha256sum_manifest(line.as_bytes()).is_err(),
+                "accepted {line:?}"
+            );
+        }
+    }
+
+    #[test]
+    fn manifest_rejects_duplicate_names() {
+        let doc = format!("{DIGEST}  metadata.json\n{DIGEST}  metadata.json\n");
+        assert!(parse_sha256sum_manifest(doc.as_bytes()).is_err());
+    }
+
+    /// The entry lookup must read the same grammar as the manifest parser, or a
+    /// line one accepts and the other resolves to a different name lets the two
+    /// disagree about which bytes `os_image_hash` commits to.
+    #[test]
+    fn entry_lookup_rejects_lines_the_manifest_parser_rejects() {
+        let hash = sha256(b"payload");
+        let doc = format!("{}  measurement.tdx.cbor\n", hex::encode(hash));
+        assert_eq!(
+            sha256sum_entry_hash(doc.as_bytes(), TDX_MEASUREMENT_FILENAME).unwrap(),
+            hash
+        );
+
+        for doc in [
+            format!("{}  measurement.tdx.cbor  junk\n", hex::encode(hash)),
+            format!("{} *measurement.tdx.cbor\n", hex::encode(hash)),
+            format!(
+                "{}  measurement.tdx.cbor\n{DIGEST} *measurement.tdx.cbor\n",
+                hex::encode(hash)
+            ),
+        ] {
+            assert!(
+                sha256sum_entry_hash(doc.as_bytes(), TDX_MEASUREMENT_FILENAME).is_err(),
+                "accepted {doc:?}"
+            );
+        }
+    }
+}
+
 /// Image-invariant GCP TDX measurement material. GCP's TPM event log measures
 /// the UKI as a PE/COFF Authenticode SHA-256 digest. The unified image identity
 /// remains `sha256(sha256sum.txt)`; this material is bound to that identity by

dstack/verifier/README.md

@@ -35,7 +35,7 @@ against the returned evidence.
   "is_valid": true,
   "details": {
     "quote_verified": true,
-    "event_log_verified": true,  // See "Verification Process" for semantics
+    "event_log_verified": true,  // runtime event log only; see "Verification Process"
     "os_image_hash_verified": true,
     "acpi_tables_verified": true,         // true only when TDX ACPI table contents are verified
     "os_image_is_dev": false,             // true=dev image, false=prod, null=unknown/N/A
@@ -141,17 +141,20 @@ certificate alone:
 cargo run --bin dstack-verifier -- --verify-cert endpoint-cert.pem
 ```
 
-The input may be PEM or DER. On success, the verifier prints JSON and writes the
-same result next to the input as `endpoint-cert.pem.ratls-verification.json`.
-The verification checks that:
+The input may be PEM or DER. The verifier prints JSON and writes the same result
+next to the input as `endpoint-cert.pem.ratls-verification.json`; `is_valid`
+says whether every check below passed, `reason` says which one did not, and the
+exit status is non-zero when `is_valid` is `false`. The verification checks
+that:
 
 1. the certificate contains a dstack RA-TLS attestation extension;
 2. the embedded attestation verifies against the platform root, including AWS
    NitroTPM documents for EC2;
 3. the attestation `report_data` is
    `QuoteContentType::RaTlsCert(SubjectPublicKeyInfo)`, so the verified
    attestation is bound to this exact TLS public key; and
-4. the reported `app_info.os_image_hash` is bound to the attested boot
+4. the attested app identity decodes; and
+5. the reported `app_info.os_image_hash` is bound to the attested boot
    measurement, surfaced as `app_info.os_image_hash_verified`. This binding is
    self-contained (no image download) for AWS NitroTPM, SEV-SNP, Nitro Enclave,
    GCP TDX, and TDX lite. It is reported as `false` for the TDX legacy
@@ -237,7 +240,7 @@ $ curl -s -d @quote.json localhost:8080/verify | jq
 The verifier performs the following verification steps:
 
 1. **Quote Verification**: Validates the platform quote using the platform verifier: DCAP for TDX, AMD SNP report verification for SEV-SNP, NSM for Nitro Enclaves, and AWS NitroTPM attestation-document verification for EC2 NitroTPM.
-2. **Event Log Verification**: Replays event logs to ensure RTMR/PCR values match and extracts app information. For RTMR3 and AWS NitroTPM PCR14 launch measurements, both the digest and payload integrity are verified. For TDX RTMR 0-2 boot-time measurements, only the digests are verified; the payload content is not validated as dstack does not define semantics for these payloads.
+2. **Event Log Verification**: Replays the runtime event log — RTMR3 on TDX, the corresponding launch measurement on SEV-SNP and AWS NitroTPM PCR14 — to ensure it reproduces the quoted register, and extracts app information from its payloads. Both the digests and the payloads are verified there, because `app_id`, `compose_hash` and the rest are read out of them. `event_log_verified` reports that step only. The boot-time event log a TDX quote carries is not replayed: RTMR 0-2 are verified in step 3 by comparing the quoted values against measurements recomputed from the OS image, which does not depend on the host's event log, and dstack defines no semantics for its payloads.
 3. **OS Image Hash Verification**:
    - Treats `vm_config` and any attached measurement material as untrusted inputs until they are bound to the hardware quote
    - For the full-image TDX path, downloads or loads the image identified by `os_image_hash`, checks the image checksum manifest, uses dstack-mr to compute expected MRTD/RTMR0-2, and compares them against the verified measurements from the quote

dstack/verifier/src/main.rs

@@ -220,32 +220,20 @@ async fn run_cert_oneshot(file_path: &str, config: &Config) -> anyhow::Result<()
         .await
         .map_err(|e| anyhow::anyhow!("failed to verify RA-TLS certificate: {:#}", e))?;
 
-    let app_info = verified.decode_app_info(false).ok();
+    let app_info = verified.decode_app_info(false);
     // Bind the reported os_image_hash to the attested boot measurement. For
     // every platform except TDX legacy this is a self-contained check (no image
     // download); relying parties should only trust `os_image_hash` when
     // `os_image_hash_verified` is true.
     let os_image_hash_verified =
         verify_cert_os_image_hash(&verified.attestation, config, &attestation_verifier).await;
-    let output = serde_json::json!({
-        "is_valid": true,
-        "details": {
-            "tee_variant": verified.attestation.quote.variant(),
-            "report_data": hex::encode(verified.attestation.report_data),
-            "public_key_der": hex::encode(&verified.public_key_der),
-            "app_info": app_info.map(|info| serde_json::json!({
-                "app_id": hex::encode(info.app_id),
-                "compose_hash": hex::encode(info.compose_hash),
-                "instance_id": hex::encode(info.instance_id),
-                "device_id": hex::encode(info.device_id),
-                "mr_system": hex::encode(info.mr_system),
-                "mr_aggregated": hex::encode(info.mr_aggregated),
-                "os_image_hash": hex::encode(info.os_image_hash),
-                "os_image_hash_verified": os_image_hash_verified,
-                "key_provider_info": hex::encode(info.key_provider_info),
-            })),
-        }
-    });
+    let output = cert_oneshot_result(
+        verified.attestation.quote.variant(),
+        &verified.attestation.report_data,
+        &verified.public_key_der,
+        app_info,
+        os_image_hash_verified,
+    );
 
     let output_path = format!("{file_path}.ratls-verification.json");
     fs::write(&output_path, serde_json::to_string_pretty(&output)?).map_err(|e| {
@@ -258,9 +246,53 @@ async fn run_cert_oneshot(file_path: &str, config: &Config) -> anyhow::Result<()
     info!("stored certificate verification result at {}", output_path);
     println!("{}", serde_json::to_string_pretty(&output)?);
 
+    if output["is_valid"] != serde_json::json!(true) {
+        anyhow::bail!("{}", output["reason"].as_str().unwrap_or("invalid"));
+    }
     Ok(())
 }
 
+/// Render the `--verify-cert` result.
+///
+/// `is_valid` is the one field a relying party reads, so it has to cover every
+/// step that ran, not just the signature check. App-info decoding is one of
+/// those steps: a certificate whose attested app identity does not decode has
+/// no `app_id`, `compose_hash` or `os_image_hash` to act on, and reporting it
+/// as valid with a null `app_info` puts the burden of noticing on the caller.
+/// Same shape as the HTTP `/verify` response: `is_valid` plus a `reason`.
+fn cert_oneshot_result(
+    tee_variant: ra_tls::attestation::TeeVariant,
+    report_data: &[u8],
+    public_key_der: &[u8],
+    app_info: Result<ra_tls::attestation::AppInfo>,
+    os_image_hash_verified: bool,
+) -> serde_json::Value {
+    let reason = app_info
+        .as_ref()
+        .err()
+        .map(|err| format!("failed to decode app info from the certificate: {err:#}"));
+    serde_json::json!({
+        "is_valid": reason.is_none(),
+        "reason": reason,
+        "details": {
+            "tee_variant": tee_variant,
+            "report_data": hex::encode(report_data),
+            "public_key_der": hex::encode(public_key_der),
+            "app_info": app_info.ok().map(|info| serde_json::json!({
+                "app_id": hex::encode(info.app_id),
+                "compose_hash": hex::encode(info.compose_hash),
+                "instance_id": hex::encode(info.instance_id),
+                "device_id": hex::encode(info.device_id),
+                "mr_system": hex::encode(info.mr_system),
+                "mr_aggregated": hex::encode(info.mr_aggregated),
+                "os_image_hash": hex::encode(info.os_image_hash),
+                "os_image_hash_verified": os_image_hash_verified,
+                "key_provider_info": hex::encode(info.key_provider_info),
+            })),
+        }
+    })
+}
+
 /// Verify that an RA-TLS certificate's `os_image_hash` is bound to its attested
 /// boot measurement.
 ///
@@ -434,6 +466,64 @@ image_download_timeout_secs = 7
     }
 }
 
+#[cfg(test)]
+mod cert_oneshot_result_tests {
+    use super::*;
+    use ra_tls::attestation::{AppInfo, TeeVariant};
+
+    fn app_info() -> AppInfo {
+        serde_json::from_value(serde_json::json!({
+            "app_id": "00".repeat(20),
+            "compose_hash": "11".repeat(32),
+            "instance_id": "22".repeat(20),
+            "device_id": "33".repeat(32),
+            "mr_system": "44".repeat(32),
+            "mr_aggregated": "55".repeat(32),
+            "os_image_hash": "66".repeat(32),
+            "key_provider_info": "",
+        }))
+        .unwrap()
+    }
+
+    /// `is_valid` is the field a relying party branches on, so it must not say
+    /// "valid" about a certificate whose app identity never decoded -- the
+    /// result then carries no app_id, compose_hash or os_image_hash at all.
+    #[test]
+    fn a_certificate_whose_app_info_does_not_decode_is_not_reported_valid() {
+        let valid = cert_oneshot_result(
+            TeeVariant::DstackTdx,
+            &[0u8; 64],
+            b"spki",
+            Ok(app_info()),
+            true,
+        );
+        assert_eq!(valid["is_valid"], serde_json::json!(true));
+        assert_eq!(valid["reason"], serde_json::Value::Null);
+        assert_eq!(
+            valid["details"]["app_info"]["compose_hash"],
+            "11".repeat(32)
+        );
+
+        let undecodable = cert_oneshot_result(
+            TeeVariant::DstackTdx,
+            &[0u8; 64],
+            b"spki",
+            Err(anyhow::anyhow!("no app-id event")),
+            true,
+        );
+        assert_eq!(undecodable["is_valid"], serde_json::json!(false));
+        assert!(undecodable["reason"]
+            .as_str()
+            .unwrap()
+            .contains("no app-id event"));
+        assert_eq!(
+            undecodable["details"]["app_info"],
+            serde_json::Value::Null,
+            "there is no app info to report"
+        );
+    }
+}
+
 #[cfg(test)]
 mod certificate_profile_tests {
     use super::*;