Skip to content

fix(verifier): confine image archive extraction - #881

Merged
kvinwang merged 2 commits into
masterfrom
codex/fix-verifier-archive-confinement
Aug 4, 2026
Merged

fix(verifier): confine image archive extraction#881
kvinwang merged 2 commits into
masterfrom
codex/fix-verifier-archive-confinement

Conversation

@kvinwang

@kvinwang kvinwang commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Problem

Verifier extracted downloaded image archives with the system tar command. Archive-controlled absolute paths, .. components, links, or special entries could escape or weaken the intended extraction boundary.

Fix

Use the workspace's existing flate2 and tar crates and extract each entry with tar::Entry::unpack_in.

  • Require archive paths to contain only normal relative components.
  • Allow only regular files and directories.
  • Reject symlinks, hardlinks, and special entries.
  • Treat unpack_in confinement failure as an error.
  • Keep the existing sha256sum -c verification and cache pruning.
  • Require the existing flat image manifest to reference only safe top-level filenames.

This intentionally avoids adding a custom checksum parser, hashing implementation, or recursive cache layout.

Verification

  • cargo test -p dstack-verifier: 16 passed.
  • cargo clippy -p dstack-verifier -- -D warnings -D clippy::expect_used -D clippy::unwrap_used --allow unused_variables: passed.
  • cargo fmt --check --all: passed.
  • git diff --check: passed.

Copilot AI review requested due to automatic review settings July 31, 2026 02:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (3)

dstack/verifier/src/verification.rs:507

  • prune_unlisted_image_files takes the raw second field from sha256sum.txt. If the manifest uses the *filename binary-mode marker format, this will treat *filename as the literal file name and may prune the real file even though sha256sum -c verified it. Strip a leading * before comparing to top-level directory entry names (matching the normalization in validate_image_manifest_paths).
        let listed_files: Vec<&OsStr> = files_doc
            .lines()
            .flat_map(|line| line.split_whitespace().nth(1))
            .map(|s| s.as_ref())
            .collect();

dstack/verifier/src/verification.rs:465

  • sha256sum manifest lines may prefix the filename with * (binary-mode marker) which sha256sum -c treats as metadata, not part of the path. Here the code treats the raw token as the path, so a line like ... *metadata.json would pass the current is_flat_manifest_name check but later prune_unlisted_image_files would look for a literal *metadata.json entry and could delete the real metadata.json. Consider normalizing the parsed name by stripping a leading * before validating/using it.

This issue also appears on line 503 of the same file.

            let name = fields
                .next()
                .context("image manifest entry is missing a path")?;

dstack/verifier/src/verification.rs:1254

  • extract_image_archive performs gzip decompression and tar extraction using blocking I/O and CPU work. Calling it directly inside download_image (an async function) can block a Tokio worker thread for large images, reducing concurrency for the verifier service. Consider running extraction inside tokio::task::spawn_blocking and awaiting the join handle.
        file.flush()
            .await
            .context("Failed to flush image archive")?;
        drop(file);
        Self::extract_image_archive(&tarball_path, &extracted_dir)?;

@kvinwang
kvinwang merged commit 88e5c67 into master Aug 4, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants