From 512281274ed0ae027d5b9c2c0b240ca77438d324 Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Mon, 3 Aug 2026 23:42:19 -0700 Subject: [PATCH 1/2] fix(verifier): confine image archive extraction --- dstack/Cargo.lock | 2 + dstack/verifier/Cargo.toml | 2 + dstack/verifier/src/verification.rs | 157 +++++++++++++++++++++++++--- 3 files changed, 144 insertions(+), 17 deletions(-) diff --git a/dstack/Cargo.lock b/dstack/Cargo.lock index cf0d8c2d5..63766de8f 100644 --- a/dstack/Cargo.lock +++ b/dstack/Cargo.lock @@ -2270,6 +2270,7 @@ dependencies = [ "dstack-types", "ez-hash", "figment", + "flate2", "fs-err", "hex", "hex-literal", @@ -2281,6 +2282,7 @@ dependencies = [ "serde-human-bytes", "serde_json", "sha2 0.10.9", + "tar", "tempfile", "tokio", "tpm-qvl", diff --git a/dstack/verifier/Cargo.toml b/dstack/verifier/Cargo.toml index e48d2e10a..99163fda6 100644 --- a/dstack/verifier/Cargo.toml +++ b/dstack/verifier/Cargo.toml @@ -34,6 +34,8 @@ tracing.workspace = true tracing-subscriber = { workspace = true, optional = true } reqwest.workspace = true tempfile.workspace = true +flate2.workspace = true +tar.workspace = true # Internal dependencies ra-tls.workspace = true diff --git a/dstack/verifier/src/verification.rs b/dstack/verifier/src/verification.rs index d38bfa1d8..c4663cd34 100644 --- a/dstack/verifier/src/verification.rs +++ b/dstack/verifier/src/verification.rs @@ -4,7 +4,7 @@ use std::{ ffi::OsStr, - path::{Path, PathBuf}, + path::{Component, Path, PathBuf}, sync::Arc, time::Duration, }; @@ -415,6 +415,68 @@ impl CvmVerifier { .is_some_and(|digest| digest == expected)) } + fn is_safe_relative_path(path: &Path) -> bool { + !path.as_os_str().is_empty() + && path + .components() + .all(|component| matches!(component, Component::Normal(_))) + } + + 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); + } + let path = Path::new(name); + if !Self::is_safe_relative_path(path) || path.components().count() != 1 { + bail!("image manifest line {} has an unsafe path", line_index + 1); + } + if path == Path::new("sha256sum.txt") { + bail!("image manifest must not recursively list sha256sum.txt"); + } + } + Ok(()) + } + + fn extract_image_archive(tarball_path: &Path, extracted_dir: &Path) -> Result<()> { + let file = fs_err::File::open(tarball_path).context("Failed to open image archive")?; + let decoder = flate2::read::GzDecoder::new(file); + let mut archive = tar::Archive::new(decoder); + for entry in archive.entries().context("Failed to read image archive")? { + let mut entry = entry.context("Failed to read image archive entry")?; + let path = entry + .path() + .context("Failed to decode image archive path")?; + if !Self::is_safe_relative_path(&path) { + bail!("image archive contains unsafe path {}", path.display()); + } + let entry_type = entry.header().entry_type(); + if !(entry_type.is_file() || entry_type.is_dir()) { + bail!( + "image archive contains unsupported entry {}", + path.display() + ); + } + if !entry + .unpack_in(extracted_dir) + .context("Failed to extract image archive entry")? + { + bail!("image archive entry escaped the extraction root"); + } + } + Ok(()) + } + fn prune_unlisted_image_files(extracted_dir: &Path, files_doc: &str) -> Result<()> { let listed_files: Vec<&OsStr> = files_doc .lines() @@ -1163,21 +1225,16 @@ impl CvmVerifier { let extracted_dir = tmp_dir.join("extracted"); fs_err::create_dir_all(&extracted_dir).context("Failed to create extraction directory")?; - // Extract the tarball - let output = Command::new("tar") - .arg("xzf") - .arg(&tarball_path) - .current_dir(&extracted_dir) - .output() + file.flush() .await - .context("Failed to extract tarball")?; + .context("Failed to flush image archive")?; + drop(file); + Self::extract_image_archive(&tarball_path, &extracted_dir)?; - if !output.status.success() { - bail!( - "Failed to extract tarball: {}", - String::from_utf8_lossy(&output.stderr) - ); - } + 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") @@ -1196,9 +1253,6 @@ impl CvmVerifier { } // Remove the files that are not listed in sha256sum.txt - 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::prune_unlisted_image_files(&extracted_dir, &files_doc)?; // All image modes are addressed by sha256(sha256sum.txt). Extra @@ -1414,6 +1468,75 @@ mod tests { assert!(!dir.path().join("unmeasured").exists()); } + #[test] + fn image_paths_must_be_confined_and_manifest_paths_must_be_flat() { + for path in ["", ".", "../escape", "/absolute", "nested/../escape"] { + assert!( + !CvmVerifier::is_safe_relative_path(Path::new(path)), + "{path}" + ); + } + assert!(CvmVerifier::is_safe_relative_path(Path::new( + "nested/artifact" + ))); + + let digest = "00".repeat(32); + assert!( + CvmVerifier::validate_image_manifest_paths(&format!("{digest} metadata.json\n")) + .is_ok() + ); + for path in ["../escape", "/absolute", "nested/artifact", "sha256sum.txt"] { + assert!( + CvmVerifier::validate_image_manifest_paths(&format!("{digest} {path}\n")).is_err() + ); + } + } + + #[test] + fn image_archive_rejects_links_and_accepts_regular_files() { + let directory = tempfile::tempdir().unwrap(); + let valid = directory.path().join("valid.tar.gz"); + { + let file = fs_err::File::create(&valid).unwrap(); + let encoder = flate2::write::GzEncoder::new(file, flate2::Compression::default()); + let mut archive = tar::Builder::new(encoder); + let payload = b"artifact"; + let mut header = tar::Header::new_gnu(); + header.set_size(payload.len() as u64); + header.set_mode(0o644); + header.set_cksum(); + archive + .append_data(&mut header, "nested/artifact", &payload[..]) + .unwrap(); + archive.finish().unwrap(); + } + let output = directory.path().join("valid-output"); + fs_err::create_dir(&output).unwrap(); + CvmVerifier::extract_image_archive(&valid, &output).unwrap(); + assert_eq!( + fs_err::read(output.join("nested/artifact")).unwrap(), + b"artifact" + ); + + let linked = directory.path().join("linked.tar.gz"); + { + let file = fs_err::File::create(&linked).unwrap(); + let encoder = flate2::write::GzEncoder::new(file, flate2::Compression::default()); + let mut archive = tar::Builder::new(encoder); + let mut header = tar::Header::new_gnu(); + header.set_entry_type(tar::EntryType::Symlink); + header.set_size(0); + header.set_mode(0o777); + header.set_link_name("../outside").unwrap(); + header.set_cksum(); + archive.append_data(&mut header, "link", &[][..]).unwrap(); + archive.finish().unwrap(); + } + let output = directory.path().join("linked-output"); + fs_err::create_dir(&output).unwrap(); + assert!(CvmVerifier::extract_image_archive(&linked, &output).is_err()); + } + #[tokio::test] async fn verifies_sev_snp_attestation_fixture_without_image_download() { let request: VerificationRequest = From 4f1f036fc10656e3234206ff55f26a45cb4bcbdd Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Tue, 4 Aug 2026 00:30:54 -0700 Subject: [PATCH 2/2] fix(verifier): accept dot-prefixed and multi-member image archives MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The archive path check rejected any `Component::CurDir`, but `tar -czf out.tar.gz .` prefixes every member with `./` and 16 of the 53 images published on download.dstack.org are packed that way. Those images are already whitelisted on-chain, so the stricter check made them permanently unverifiable. `tar::Entry::unpack_in` strips `.` components and cannot escape through them, so the local check was stricter than the confinement it backs up without gaining anything. Mirror the library rule instead: reject `..`, absolute paths and prefixes, accept `.`. Split the manifest rule out into `is_flat_manifest_name`. It expresses what `prune_unlisted_image_files` and `sha256sum -c` actually need — the name must literally be a file name — instead of reusing the archive rule plus a component count. Also switch to `MultiGzDecoder`. `GzDecoder` stops at the first member of a concatenated gzip stream and reports clean EOF, so `tar::Archive` would end iteration without an error and extract the archive partially. Verification: - Both published archive shapes now extract, pass `sha256sum -c`, prune to the same 5 files, and reproduce their on-chain `os_image_hash` (`f82e55d7...`, `1ae9dbdc...`). - All 53 published manifests re-checked against `is_flat_manifest_name`: 4 flat entries each, none affected. - `cargo test -p dstack-verifier --all-features`: 18 passed. - `cargo clippy -p dstack-verifier --all-features -- -D warnings -D clippy::expect_used -D clippy::unwrap_used`: passed. - `cargo fmt --check --all`, `git diff --check`: passed. --- dstack/verifier/src/verification.rs | 145 ++++++++++++++++++++++++---- 1 file changed, 128 insertions(+), 17 deletions(-) diff --git a/dstack/verifier/src/verification.rs b/dstack/verifier/src/verification.rs index c4663cd34..be4768db8 100644 --- a/dstack/verifier/src/verification.rs +++ b/dstack/verifier/src/verification.rs @@ -415,11 +415,31 @@ impl CvmVerifier { .is_some_and(|digest| digest == expected)) } - fn is_safe_relative_path(path: &Path) -> bool { - !path.as_os_str().is_empty() - && path - .components() - .all(|component| matches!(component, Component::Normal(_))) + /// Mirrors the confinement rule `tar::Entry::unpack_in` applies internally: + /// `..` components, absolute paths, and Windows prefixes escape the + /// extraction root, while `.` components are stripped and are harmless. + /// + /// This duplicates the library check on purpose. `Archive::unpack` discards + /// the `unpack_in` return value, so an escaping member is silently dropped + /// and extraction still reports success; checking here turns that into an + /// error and keeps the boundary from widening if the library's behavior + /// ever changes. It must not be *stricter* than the library, though: + /// rejecting `.` components would reject the `./`-prefixed archives that + /// `tar -czf out.tar.gz .` produces, and roughly a third of the images + /// published on download.dstack.org are packed that way. + fn is_confined_archive_path(path: &Path) -> bool { + path.components() + .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<()> { @@ -437,11 +457,10 @@ impl CvmVerifier { if fields.next().is_some() { bail!("image manifest line {} has extra fields", line_index + 1); } - let path = Path::new(name); - if !Self::is_safe_relative_path(path) || path.components().count() != 1 { + if !Self::is_flat_manifest_name(name) { bail!("image manifest line {} has an unsafe path", line_index + 1); } - if path == Path::new("sha256sum.txt") { + if name == "sha256sum.txt" { bail!("image manifest must not recursively list sha256sum.txt"); } } @@ -450,14 +469,17 @@ impl CvmVerifier { fn extract_image_archive(tarball_path: &Path, extracted_dir: &Path) -> Result<()> { let file = fs_err::File::open(tarball_path).context("Failed to open image archive")?; - let decoder = flate2::read::GzDecoder::new(file); + // `MultiGzDecoder`, not `GzDecoder`: the latter stops at the first gzip + // member and reports clean EOF, so a concatenated-member archive would + // extract partially without any error. + let decoder = flate2::read::MultiGzDecoder::new(file); let mut archive = tar::Archive::new(decoder); for entry in archive.entries().context("Failed to read image archive")? { let mut entry = entry.context("Failed to read image archive entry")?; let path = entry .path() .context("Failed to decode image archive path")?; - if !Self::is_safe_relative_path(&path) { + if !Self::is_confined_archive_path(&path) { bail!("image archive contains unsafe path {}", path.display()); } let entry_type = entry.header().entry_type(); @@ -1470,24 +1492,38 @@ mod tests { #[test] fn image_paths_must_be_confined_and_manifest_paths_must_be_flat() { - for path in ["", ".", "../escape", "/absolute", "nested/../escape"] { + for path in ["../escape", "/absolute", "nested/../escape"] { assert!( - !CvmVerifier::is_safe_relative_path(Path::new(path)), + !CvmVerifier::is_confined_archive_path(Path::new(path)), + "{path}" + ); + } + // `.` components are stripped by `unpack_in` and cannot escape, so the + // check must accept them: `tar -czf out.tar.gz .` prefixes every member + // with `./` and published images are packed that way. + for path in ["nested/artifact", "./metadata.json", ".", "./", ""] { + assert!( + CvmVerifier::is_confined_archive_path(Path::new(path)), "{path}" ); } - assert!(CvmVerifier::is_safe_relative_path(Path::new( - "nested/artifact" - ))); let digest = "00".repeat(32); assert!( CvmVerifier::validate_image_manifest_paths(&format!("{digest} metadata.json\n")) .is_ok() ); - for path in ["../escape", "/absolute", "nested/artifact", "sha256sum.txt"] { + for path in [ + "../escape", + "/absolute", + "nested/artifact", + "./metadata.json", + ".", + "sha256sum.txt", + ] { assert!( - CvmVerifier::validate_image_manifest_paths(&format!("{digest} {path}\n")).is_err() + CvmVerifier::validate_image_manifest_paths(&format!("{digest} {path}\n")).is_err(), + "{path}" ); } } @@ -1537,6 +1573,81 @@ mod tests { assert!(CvmVerifier::extract_image_archive(&linked, &output).is_err()); } + /// Images published on download.dstack.org come in two shapes: members + /// packed from a glob (`bzImage`, ...) and members packed from `.` + /// (`./`, `./bzImage`, ...). Both must extract to the same flat layout. + #[test] + fn image_archive_accepts_dot_prefixed_members() { + let directory = tempfile::tempdir().unwrap(); + let archive_path = directory.path().join("dot-prefixed.tar.gz"); + { + let file = fs_err::File::create(&archive_path).unwrap(); + let encoder = flate2::write::GzEncoder::new(file, flate2::Compression::default()); + let mut archive = tar::Builder::new(encoder); + let mut header = tar::Header::new_gnu(); + header.set_entry_type(tar::EntryType::Directory); + header.set_size(0); + header.set_mode(0o755); + header.set_cksum(); + archive.append_data(&mut header, "./", &[][..]).unwrap(); + let payload = b"artifact"; + let mut header = tar::Header::new_gnu(); + header.set_size(payload.len() as u64); + header.set_mode(0o644); + header.set_cksum(); + archive + .append_data(&mut header, "./metadata.json", &payload[..]) + .unwrap(); + archive.finish().unwrap(); + } + let output = directory.path().join("dot-prefixed-output"); + fs_err::create_dir(&output).unwrap(); + CvmVerifier::extract_image_archive(&archive_path, &output).unwrap(); + assert_eq!( + fs_err::read(output.join("metadata.json")).unwrap(), + b"artifact" + ); + } + + /// `GzDecoder` stops at the first member of a concatenated gzip stream and + /// reports clean EOF, which would truncate the archive without an error. + #[test] + fn image_archive_reads_every_gzip_member() { + let directory = tempfile::tempdir().unwrap(); + let mut tarball = tar::Builder::new(Vec::new()); + let payload = b"artifact"; + let mut header = tar::Header::new_gnu(); + header.set_size(payload.len() as u64); + header.set_mode(0o644); + header.set_cksum(); + tarball + .append_data(&mut header, "metadata.json", &payload[..]) + .unwrap(); + let tarball = tarball.into_inner().unwrap(); + + let archive_path = directory.path().join("multi-member.tar.gz"); + { + use std::io::Write; + + let mut file = fs_err::File::create(&archive_path).unwrap(); + // One gzip member per half of the tar stream. + for half in tarball.chunks(tarball.len().div_ceil(2)) { + let mut encoder = + flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::default()); + encoder.write_all(half).unwrap(); + file.write_all(&encoder.finish().unwrap()).unwrap(); + } + file.flush().unwrap(); + } + let output = directory.path().join("multi-member-output"); + fs_err::create_dir(&output).unwrap(); + CvmVerifier::extract_image_archive(&archive_path, &output).unwrap(); + assert_eq!( + fs_err::read(output.join("metadata.json")).unwrap(), + b"artifact" + ); + } + #[tokio::test] async fn verifies_sev_snp_attestation_fixture_without_image_download() { let request: VerificationRequest =