Repository navigation
Conversation
Both extraction sites in `crates/prover/src/build.rs` invoked tar with `-P` (`--absolute-names`), which permits absolute member names and `..` components. The archives reaching them are downloaded over the network, so a substituted or compromised artifact could write outside the build directory. Route both through a shared `extract_tarball` helper that omits `-P`. Nothing needs it: the tarballs are produced with relative paths by the upload path in this same file, and the repository's other two extraction sites (the CUDA server download and the toolchain installer) already extract without `-P`. Add two tests that build genuinely hostile archives, one with a `../` member and one with an absolute member, and assert neither writes above the extraction directory. Both fail if `-P` is restored.
Author
|
Ten days in and no workflow has run on this yet. Could a maintainer approve the run? It is a path traversal fix so I would rather it not sit. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2533.
A note on where this lives: the issue points at
crates/sdk/src/install.rs, which is now an eight-line re-export. The implementation is incrates/prover/src/build.rs.There are two extraction sites there, not one.
download_circuit_artifactspulls the circuit artifacts over HTTPS, and the dev-artifact path pulls from S3 viaaws s3 cp. Both passed-P, so both accepted absolute member names and..components. Only the first is mentioned in the issue.Both now go through a shared
extract_tarballhelper that omits-P. Nothing needed it. The tarballs are produced with relative paths by the upload path in this same file (tar -czf ... -C <dir> .), and the repository's other two extraction sites, the CUDA server download and the toolchain installer, already extract without-P. This brings the outliers in line rather than introducing a new convention.Two tests build genuinely hostile archives, one with a
../member and one with an absolute member, and assert neither writes above the extraction directory. I checked they fail with-Prestored, so they pin the behaviour rather than just passing. The absolute-path test demonstrates the overwrite directly: without the fix, a file outside the extraction directory is reverted to the archive's copy.The two tars behave differently on the hostile input, so the tests assert only the property that matters. GNU tar strips the offending components and continues; bsdtar refuses the entry and exits non-zero. Both confine the write, which is what is asserted.
I have not attempted to address symlink-based escapes, which are a separate class and would mean moving off the
tarbinary entirely. Happy to look at that separately if you want it.