fix(solutils): reject path traversal in downloadProgramArtifacts#22278
Open
genisis0x wants to merge 1 commit intosmartcontractkit:developfrom
Open
fix(solutils): reject path traversal in downloadProgramArtifacts#22278genisis0x wants to merge 1 commit intosmartcontractkit:developfrom
genisis0x wants to merge 1 commit intosmartcontractkit:developfrom
Conversation
`downloadProgramArtifacts` in deployment/utils/solutils/artifacts.go
extracted archive entries using only `filepath.Base(header.Name)` to
strip directory components. That is enough for benign names like
"../etc/passwd" (which collapses to "passwd"), but a name of just
".." caused `filepath.Join(targetDir, "..")` to resolve to the
parent of the target directory, allowing extraction to escape.
Harden the extractor:
- filepath.Clean + filepath.Base the archive name and reject "."
and "..".
- resolve the joined path with filepath.Abs and verify it stays
within targetDir using filepath.Rel; reject anything that walks
out.
Add `TestDownloadProgramArtifacts_RejectsPathTraversal` covering the
explicit-rejection cases (`..`, `./..`) and the defense-in-depth
cases (`../foo.so`, `../../etc/passwd`, `/etc/passwd`) where the
benign filename is accepted but no file is allowed to land outside
`targetDir`.
Closes smartcontractkit#21003.
genisis0x
requested review from
agusaldasoro,
b-gopalswami,
carte7000,
makramkd,
matYang,
ogtownsend and
winder
product-security-plaid-production
Bot
requested review from
ChrisAmora,
archseer and
george-dorin
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
1 participant
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.
Description
Fixes #21003.
downloadProgramArtifactsindeployment/utils/solutils/artifacts.goextracted archive entries by joining the target directory withfilepath.Base(header.Name). That sanitization is enough for benign payloads such as../etc/passwd, which collapses topasswd, but a header name of just..madefilepath.Join(targetDir, "..")resolve to the parent of the target directory, allowing extraction to escape.Fix
In
downloadProgramArtifacts:filepath.Clean+filepath.Basethe entry name and refuse./../ empty / root entries explicitly.filepath.Absand verify withfilepath.Relthat it stays withintargetDir. Anything that walks outside is rejected with a clear error.Tests
TestDownloadProgramArtifacts_RejectsPathTraversal(indeployment/utils/solutils/artifacts_test.go) covers two classes:..,./..../escape.so,../../etc/passwd,/etc/passwdfilepath.Basecollapses to a safe name; the file lands insidetempDirand the parent directory is verified untouched (defense in depth)The existing happy-path / 4xx-5xx / context-cancel tests continue to pass.
Risk
Confined to one helper used by Solana artifact downloads. No behaviour change for valid archives. New rejections only fire for entry names that should never appear in legitimate artifacts.