Skip to content

Commit 41467d5

Browse files
committed
fix(verifier): check the image manifest in-process instead of trusting sha256sum -c
The whole content binding for a downloaded OS image was `sha256sum -c sha256sum.txt` plus its exit status. GNU coreutils skips an improperly formatted line with a warning and still exits 0: 5891b5... a.txt 0000...0000 b.txt <- one space $ sha256sum -c sha256sum.txt a.txt: OK sha256sum: WARNING: 1 line is improperly formatted exit=0 validate_image_manifest_paths split on whitespace, so it accepted that line, and prune_unlisted_image_files did too, so b.txt survived the prune. bzImage, ovmf.fd and the initrd were then measured with no content check at all. `<hash> *bzImage` went wrong the other way: coreutils checks `bzImage`, the whitespace split sees `*bzImage`, and the file that was checked is the one that gets pruned. Parse the manifest once with dstack_types::sha256sum::parse and hash each listed file here. One parser, one grammar, and a missing or mismatched file is an error rather than a warning on someone else's stderr.
1 parent 8a71300 commit 41467d5

1 file changed

Lines changed: 75 additions & 83 deletions

File tree

‎dstack/verifier/src/verification.rs‎

Lines changed: 75 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -18,15 +18,15 @@ use cc_eventlog::{
1818
TdxEvent,
1919
};
2020
use dstack_mr::{tdx::TdxRtmr0AcpiHashes, TdxMeasurements};
21-
use dstack_types::{TdxAttestationVariant, VmConfig};
21+
use dstack_types::{sha256sum, TdxAttestationVariant, VmConfig};
2222
use hex_literal::hex;
2323
use ra_tls::attestation::{
2424
AppInfo, Attestation, AttestationQuote, AttestationVerifier, DstackVerifiedReport, NitroPcrs,
2525
VerifiedAttestation, VersionedAttestation,
2626
};
2727
use serde::{Deserialize, Serialize};
2828
use sha2::{Digest as _, Sha256};
29-
use tokio::{io::AsyncWriteExt, process::Command};
29+
use tokio::io::AsyncWriteExt;
3030
use tpm_qvl::verify::VerifiedReport as TpmVerifiedReport;
3131
use tracing::{debug, info, warn};
3232

@@ -331,39 +331,25 @@ impl CvmVerifier {
331331
.all(|component| matches!(component, Component::Normal(_) | Component::CurDir))
332332
}
333333

334-
/// A manifest name must be literally a file name, because
335-
/// `prune_unlisted_image_files` matches manifest entries against the
336-
/// `file_name()` of each top-level directory entry, and `sha256sum -c`
337-
/// resolves them relative to the extraction root.
338-
fn is_flat_manifest_name(name: &str) -> bool {
339-
Path::new(name)
340-
.file_name()
341-
.is_some_and(|file_name| file_name == OsStr::new(name))
342-
}
343-
344-
fn validate_image_manifest_paths(files_doc: &str) -> Result<()> {
345-
for (line_index, line) in files_doc.lines().enumerate() {
346-
if line.trim().is_empty() {
347-
continue;
348-
}
349-
let mut fields = line.split_whitespace();
350-
let _digest = fields
351-
.next()
352-
.context("image manifest entry is missing a digest")?;
353-
let name = fields
354-
.next()
355-
.context("image manifest entry is missing a path")?;
356-
if fields.next().is_some() {
357-
bail!("image manifest line {} has extra fields", line_index + 1);
358-
}
359-
if !Self::is_flat_manifest_name(name) {
360-
bail!("image manifest line {} has an unsafe path", line_index + 1);
361-
}
362-
if name == "sha256sum.txt" {
363-
bail!("image manifest must not recursively list sha256sum.txt");
334+
/// Check every file the image manifest lists against the digest it binds.
335+
/// This is the only content check on `bzImage`, `ovmf.fd` and the initrd
336+
/// before they are measured.
337+
fn verify_image_manifest(
338+
extracted_dir: &Path,
339+
files_doc: &str,
340+
) -> Result<Vec<sha256sum::Entry>> {
341+
let entries = sha256sum::parse(files_doc.as_bytes()).map_err(anyhow::Error::msg)?;
342+
for entry in &entries {
343+
let mut file = fs_err::File::open(extracted_dir.join(&entry.name))
344+
.with_context(|| format!("image is missing manifest entry {}", entry.name))?;
345+
let mut hasher = Sha256::new();
346+
std::io::copy(&mut file, &mut hasher)
347+
.with_context(|| format!("failed to read manifest entry {}", entry.name))?;
348+
if hasher.finalize().as_slice() != entry.hash {
349+
bail!("{} does not match its digest in sha256sum.txt", entry.name);
364350
}
365351
}
366-
Ok(())
352+
Ok(entries)
367353
}
368354

369355
fn extract_image_archive(tarball_path: &Path, extracted_dir: &Path) -> Result<()> {
@@ -398,12 +384,11 @@ impl CvmVerifier {
398384
Ok(())
399385
}
400386

401-
fn prune_unlisted_image_files(extracted_dir: &Path, files_doc: &str) -> Result<()> {
402-
let listed_files: Vec<&OsStr> = files_doc
403-
.lines()
404-
.flat_map(|line| line.split_whitespace().nth(1))
405-
.map(|s| s.as_ref())
406-
.collect();
387+
fn prune_unlisted_image_files(
388+
extracted_dir: &Path,
389+
manifest: &[sha256sum::Entry],
390+
) -> Result<()> {
391+
let listed_files: Vec<&OsStr> = manifest.iter().map(|entry| entry.name.as_ref()).collect();
407392
let files = fs_err::read_dir(extracted_dir).context("Failed to read directory")?;
408393
for file in files {
409394
let file = file.context("Failed to read directory entry")?;
@@ -1037,26 +1022,10 @@ impl CvmVerifier {
10371022
let sha256sum_path = extracted_dir.join("sha256sum.txt");
10381023
let files_doc =
10391024
fs_err::read_to_string(&sha256sum_path).context("Failed to read sha256sum.txt")?;
1040-
Self::validate_image_manifest_paths(&files_doc)?;
1041-
1042-
// Verify checksum
1043-
let output = Command::new("sha256sum")
1044-
.arg("-c")
1045-
.arg("sha256sum.txt")
1046-
.current_dir(&extracted_dir)
1047-
.output()
1048-
.await
1049-
.context("Failed to verify checksum")?;
1050-
1051-
if !output.status.success() {
1052-
bail!(
1053-
"Checksum verification failed: {}",
1054-
String::from_utf8_lossy(&output.stderr)
1055-
);
1056-
}
1025+
let manifest = Self::verify_image_manifest(&extracted_dir, &files_doc)?;
10571026

10581027
// Remove the files that are not listed in sha256sum.txt
1059-
Self::prune_unlisted_image_files(&extracted_dir, &files_doc)?;
1028+
Self::prune_unlisted_image_files(&extracted_dir, &manifest)?;
10601029

10611030
// All image modes are addressed by sha256(sha256sum.txt). Extra
10621031
// measurement CBOR files are ordinary sha256sum.txt entries and do not
@@ -1705,6 +1674,17 @@ mod tests {
17051674
assert_eq!(entries.len(), 1, "temporary cache files must not survive");
17061675
}
17071676

1677+
/// Lay out an extracted image whose manifest is `files_doc`, with each
1678+
/// named file holding the payload given for it.
1679+
fn image_dir_with_manifest(files_doc: &str, files: &[(&str, &[u8])]) -> tempfile::TempDir {
1680+
let dir = tempfile::tempdir().expect("temp image directory");
1681+
fs_err::write(dir.path().join("sha256sum.txt"), files_doc).unwrap();
1682+
for (name, payload) in files {
1683+
fs_err::write(dir.path().join(name), payload).unwrap();
1684+
}
1685+
dir
1686+
}
1687+
17081688
#[test]
17091689
fn measurement_cache_key_ignores_unmeasured_fields() {
17101690
let base: VmConfig = serde_json::from_value(serde_json::json!({
@@ -1739,21 +1719,52 @@ mod tests {
17391719

17401720
#[test]
17411721
fn image_cache_pruning_keeps_checksum_identity() {
1742-
let dir = tempfile::tempdir().expect("temp image directory");
1743-
let files_doc = "00 metadata.json\n";
1744-
fs_err::write(dir.path().join("sha256sum.txt"), files_doc).unwrap();
1745-
fs_err::write(dir.path().join("metadata.json"), "{}").unwrap();
1746-
fs_err::write(dir.path().join("unmeasured"), "remove me").unwrap();
1722+
let files_doc = format!("{} metadata.json\n", hex::encode(Sha256::digest(b"{}")));
1723+
let dir = image_dir_with_manifest(
1724+
&files_doc,
1725+
&[("metadata.json", b"{}"), ("unmeasured", b"remove me")],
1726+
);
17471727

1748-
CvmVerifier::prune_unlisted_image_files(dir.path(), files_doc).unwrap();
1728+
let manifest = CvmVerifier::verify_image_manifest(dir.path(), &files_doc).unwrap();
1729+
CvmVerifier::prune_unlisted_image_files(dir.path(), &manifest).unwrap();
17491730

17501731
assert!(dir.path().join("sha256sum.txt").exists());
17511732
assert!(dir.path().join("metadata.json").exists());
17521733
assert!(!dir.path().join("unmeasured").exists());
17531734
}
17541735

17551736
#[test]
1756-
fn image_paths_must_be_confined_and_manifest_paths_must_be_flat() {
1737+
fn every_manifest_entry_is_checked_before_the_image_is_accepted() {
1738+
let good = hex::encode(Sha256::digest(b"{}"));
1739+
let zero = "00".repeat(32);
1740+
let files: &[(&str, &[u8])] = &[("metadata.json", b"{}"), ("bzImage", b"kernel")];
1741+
1742+
for files_doc in [
1743+
format!("{good} metadata.json\n{zero} bzImage\n"),
1744+
format!("{good} metadata.json\n{zero} missing\n"),
1745+
] {
1746+
let dir = image_dir_with_manifest(&files_doc, files);
1747+
assert!(
1748+
CvmVerifier::verify_image_manifest(dir.path(), &files_doc).is_err(),
1749+
"accepted {files_doc:?}"
1750+
);
1751+
}
1752+
1753+
let good_doc = format!(
1754+
"{good} metadata.json\n{} bzImage\n",
1755+
hex::encode(Sha256::digest(b"kernel"))
1756+
);
1757+
let dir = image_dir_with_manifest(&good_doc, files);
1758+
assert_eq!(
1759+
CvmVerifier::verify_image_manifest(dir.path(), &good_doc)
1760+
.unwrap()
1761+
.len(),
1762+
2
1763+
);
1764+
}
1765+
1766+
#[test]
1767+
fn image_archive_paths_must_be_confined() {
17571768
for path in ["../escape", "/absolute", "nested/../escape"] {
17581769
assert!(
17591770
!CvmVerifier::is_confined_archive_path(Path::new(path)),
@@ -1769,25 +1780,6 @@ mod tests {
17691780
"{path}"
17701781
);
17711782
}
1772-
1773-
let digest = "00".repeat(32);
1774-
assert!(
1775-
CvmVerifier::validate_image_manifest_paths(&format!("{digest} metadata.json\n"))
1776-
.is_ok()
1777-
);
1778-
for path in [
1779-
"../escape",
1780-
"/absolute",
1781-
"nested/artifact",
1782-
"./metadata.json",
1783-
".",
1784-
"sha256sum.txt",
1785-
] {
1786-
assert!(
1787-
CvmVerifier::validate_image_manifest_paths(&format!("{digest} {path}\n")).is_err(),
1788-
"{path}"
1789-
);
1790-
}
17911783
}
17921784

17931785
#[test]

0 commit comments

Comments
 (0)