diff --git a/dstack/dstack-types/src/lib.rs b/dstack/dstack-types/src/lib.rs index 5317c9bcb..e78a499e0 100644 --- a/dstack/dstack-types/src/lib.rs +++ b/dstack/dstack-types/src/lib.rs @@ -1562,44 +1562,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: ` *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 ` 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, 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 = 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 ` `"); }; - 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| { - 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( @@ -1627,6 +1662,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 diff --git a/dstack/verifier/src/verification.rs b/dstack/verifier/src/verification.rs index 77752d4fc..c0e7ebbc7 100644 --- a/dstack/verifier/src/verification.rs +++ b/dstack/verifier/src/verification.rs @@ -18,7 +18,7 @@ use cc_eventlog::{ TdxEvent, }; use dstack_mr::{tdx::TdxRtmr0AcpiHashes, TdxMeasurements}; -use dstack_types::{TdxAttestationVariant, VmConfig}; +use dstack_types::{Sha256sumEntry, TdxAttestationVariant, VmConfig}; use hex_literal::hex; use ra_tls::attestation::{ AppInfo, Attestation, AttestationQuote, AttestationVerifier, DstackVerifiedReport, NitroPcrs, @@ -26,7 +26,7 @@ use ra_tls::attestation::{ }; use serde::{Deserialize, Serialize}; use sha2::{Digest as _, Sha256}; -use tokio::{io::AsyncWriteExt, process::Command}; +use tokio::io::AsyncWriteExt; use tracing::{debug, info, warn}; use crate::types::{ @@ -330,39 +330,34 @@ impl CvmVerifier { .all(|component| matches!(component, Component::Normal(_) | Component::CurDir)) } - /// A manifest name must be literally a file name, because - /// `prune_unlisted_image_files` matches manifest entries against the - /// `file_name()` of each top-level directory entry, and `sha256sum -c` - /// resolves them relative to the extraction root. - fn is_flat_manifest_name(name: &str) -> bool { - Path::new(name) - .file_name() - .is_some_and(|file_name| file_name == OsStr::new(name)) - } - - fn validate_image_manifest_paths(files_doc: &str) -> Result<()> { - for (line_index, line) in files_doc.lines().enumerate() { - if line.trim().is_empty() { - continue; - } - let mut fields = line.split_whitespace(); - let _digest = fields - .next() - .context("image manifest entry is missing a digest")?; - let name = fields - .next() - .context("image manifest entry is missing a path")?; - if fields.next().is_some() { - bail!("image manifest line {} has extra fields", line_index + 1); - } - if !Self::is_flat_manifest_name(name) { - bail!("image manifest line {} has an unsafe path", line_index + 1); - } - if name == "sha256sum.txt" { + /// Check every file the image manifest lists against the digest it binds. + /// + /// This is the whole content binding for a downloaded image: nothing else + /// looks at the bytes of `bzImage`, `ovmf.fd` or the initrd before they are + /// measured. It used to be `sha256sum -c`, whose exit status does not mean + /// what it looks like -- GNU coreutils skips an improperly formatted line + /// with a warning and still exits 0, so a line this process accepted and + /// coreutils did not left a file unchecked and still measured. Hashing the + /// files here keeps one parser and one grammar over the one file that + /// `os_image_hash` commits to. + fn verify_image_manifest(extracted_dir: &Path, files_doc: &str) -> Result> { + let entries = dstack_types::parse_sha256sum_manifest(files_doc.as_bytes()) + .map_err(anyhow::Error::msg) + .context("failed to parse the image manifest")?; + for entry in &entries { + if entry.name == "sha256sum.txt" { bail!("image manifest must not recursively list sha256sum.txt"); } + let mut file = fs_err::File::open(extracted_dir.join(&entry.name)) + .with_context(|| format!("image is missing manifest entry {}", entry.name))?; + let mut hasher = Sha256::new(); + std::io::copy(&mut file, &mut hasher) + .with_context(|| format!("failed to read manifest entry {}", entry.name))?; + if hasher.finalize().as_slice() != entry.hash { + bail!("{} does not match its digest in sha256sum.txt", entry.name); + } } - Ok(()) + Ok(entries) } fn extract_image_archive(tarball_path: &Path, extracted_dir: &Path) -> Result<()> { @@ -397,12 +392,11 @@ impl CvmVerifier { Ok(()) } - fn prune_unlisted_image_files(extracted_dir: &Path, files_doc: &str) -> Result<()> { - let listed_files: Vec<&OsStr> = files_doc - .lines() - .flat_map(|line| line.split_whitespace().nth(1)) - .map(|s| s.as_ref()) - .collect(); + fn prune_unlisted_image_files(extracted_dir: &Path, manifest: &[Sha256sumEntry]) -> Result<()> { + let listed_files: Vec<&OsStr> = manifest + .iter() + .map(|entry| entry.name.as_ref()) + .collect::>(); let files = fs_err::read_dir(extracted_dir).context("Failed to read directory")?; for file in files { let file = file.context("Failed to read directory entry")?; @@ -1036,26 +1030,10 @@ impl CvmVerifier { let sha256sum_path = extracted_dir.join("sha256sum.txt"); let files_doc = fs_err::read_to_string(&sha256sum_path).context("Failed to read sha256sum.txt")?; - Self::validate_image_manifest_paths(&files_doc)?; - - // Verify checksum - let output = Command::new("sha256sum") - .arg("-c") - .arg("sha256sum.txt") - .current_dir(&extracted_dir) - .output() - .await - .context("Failed to verify checksum")?; - - if !output.status.success() { - bail!( - "Checksum verification failed: {}", - String::from_utf8_lossy(&output.stderr) - ); - } + let manifest = Self::verify_image_manifest(&extracted_dir, &files_doc)?; // Remove the files that are not listed in sha256sum.txt - Self::prune_unlisted_image_files(&extracted_dir, &files_doc)?; + Self::prune_unlisted_image_files(&extracted_dir, &manifest)?; // All image modes are addressed by sha256(sha256sum.txt). Extra // measurement CBOR files are ordinary sha256sum.txt entries and do not @@ -1706,6 +1684,17 @@ mod tests { assert_eq!(entries.len(), 1, "temporary cache files must not survive"); } + /// Lay out an extracted image whose manifest is `files_doc`, with each + /// named file holding the payload given for it. + fn image_dir_with_manifest(files_doc: &str, files: &[(&str, &[u8])]) -> tempfile::TempDir { + let dir = tempfile::tempdir().expect("temp image directory"); + fs_err::write(dir.path().join("sha256sum.txt"), files_doc).unwrap(); + for (name, payload) in files { + fs_err::write(dir.path().join(name), payload).unwrap(); + } + dir + } + #[test] fn measurement_cache_key_ignores_unmeasured_fields() { let base: VmConfig = serde_json::from_value(serde_json::json!({ @@ -1740,19 +1729,64 @@ mod tests { #[test] fn image_cache_pruning_keeps_checksum_identity() { - let dir = tempfile::tempdir().expect("temp image directory"); - let files_doc = "00 metadata.json\n"; - fs_err::write(dir.path().join("sha256sum.txt"), files_doc).unwrap(); - fs_err::write(dir.path().join("metadata.json"), "{}").unwrap(); - fs_err::write(dir.path().join("unmeasured"), "remove me").unwrap(); + let files_doc = format!("{} metadata.json\n", hex::encode(Sha256::digest(b"{}"))); + let dir = image_dir_with_manifest( + &files_doc, + &[("metadata.json", b"{}"), ("unmeasured", b"remove me")], + ); - CvmVerifier::prune_unlisted_image_files(dir.path(), files_doc).unwrap(); + let manifest = CvmVerifier::verify_image_manifest(dir.path(), &files_doc).unwrap(); + CvmVerifier::prune_unlisted_image_files(dir.path(), &manifest).unwrap(); assert!(dir.path().join("sha256sum.txt").exists()); assert!(dir.path().join("metadata.json").exists()); assert!(!dir.path().join("unmeasured").exists()); } + /// The manifest is the only thing binding the downloaded bytes to + /// `os_image_hash`, so a line that does not check out has to stop the + /// image -- including the lines GNU `sha256sum -c` skips with a warning + /// while still exiting 0, which is how an unchecked file used to reach the + /// measurement step. + #[test] + fn every_manifest_entry_is_checked_before_the_image_is_accepted() { + let good = hex::encode(Sha256::digest(b"{}")); + let zero = "00".repeat(32); + let listed_but_wrong = format!("{good} metadata.json\n{zero} bzImage\n"); + let files: &[(&str, &[u8])] = &[("metadata.json", b"{}"), ("bzImage", b"kernel")]; + + for files_doc in [ + // The digest is simply wrong. + listed_but_wrong.clone(), + // One space, not two: an improperly formatted line that + // `sha256sum -c` warns about and skips, exit 0. + listed_but_wrong.replace(&format!("{zero} bzImage"), &format!("{zero} bzImage")), + // Binary-mode marker: `sha256sum -c` checks `bzImage`, a + // whitespace split sees a file named `*bzImage`. + listed_but_wrong.replace(&format!("{zero} bzImage"), &format!("{zero} *bzImage")), + // Listed but absent. + format!("{good} metadata.json\n{zero} missing\n"), + ] { + let dir = image_dir_with_manifest(&files_doc, files); + assert!( + CvmVerifier::verify_image_manifest(dir.path(), &files_doc).is_err(), + "accepted {files_doc:?}" + ); + } + + let good_doc = format!( + "{good} metadata.json\n{} bzImage\n", + hex::encode(Sha256::digest(b"kernel")) + ); + let dir = image_dir_with_manifest(&good_doc, files); + assert_eq!( + CvmVerifier::verify_image_manifest(dir.path(), &good_doc) + .unwrap() + .len(), + 2 + ); + } + #[test] fn image_paths_must_be_confined_and_manifest_paths_must_be_flat() { for path in ["../escape", "/absolute", "nested/../escape"] { @@ -1772,10 +1806,6 @@ mod tests { } let digest = "00".repeat(32); - assert!( - CvmVerifier::validate_image_manifest_paths(&format!("{digest} metadata.json\n")) - .is_ok() - ); for path in [ "../escape", "/absolute", @@ -1784,8 +1814,10 @@ mod tests { ".", "sha256sum.txt", ] { + let files_doc = format!("{digest} {path}\n"); + let dir = image_dir_with_manifest(&files_doc, &[]); assert!( - CvmVerifier::validate_image_manifest_paths(&format!("{digest} {path}\n")).is_err(), + CvmVerifier::verify_image_manifest(dir.path(), &files_doc).is_err(), "{path}" ); }