fix: validate tar entry paths against escaping the target directory in bundle Unpack - #2332
Open
pujitha24 wants to merge 2 commits into
Open
fix: validate tar entry paths against escaping the target directory in bundle Unpack#2332pujitha24 wants to merge 2 commits into
pujitha24 wants to merge 2 commits into
Conversation
…n bundle Unpack
Motivation:
pkg/bundle/bundle.go's Unpack function extracts tar entries from a
container image/OCI bundle and guarded against path traversal by
checking whether filepath.Join(targetPath, header.Name) contained the
literal substring "/../". filepath.Join calls filepath.Clean, which
resolves ".." segments before that check ever runs, so a tar entry
named e.g. "../../../../etc/passwd" is cleaned down to an absolute
path such as "/etc/passwd" that no longer contains "/../" and the
check never fires. A malicious bundle can therefore write files
anywhere the controller process has permission to write, entirely
outside the intended extraction directory.
Approach:
Replace the substring check with filepath.Rel(targetPath, target).
If the entry cannot be expressed as a path relative to targetPath
without a leading ".." component, the entry escapes the target
directory and is now rejected before anything is written to disk.
Tar entries handled by this function are limited to regular files and
directories (symlink entries already hit the "unsupported file type"
error), so this is the only path-construction site that needed
hardening here.
Validation:
go test ./pkg/bundle/... -race -v
-> all 5 specs pass, including two new table-driven cases that build
a raw tar stream with a "../canary" and a
"../../../../etc/canary" entry (the latter mirroring the exact
proof-of-concept from the report) and assert Unpack rejects both
and creates no file at the resolved escape path.
-> confirmed the new test fails against the old code (reverting only
bundle.go reproduces "Expected an error to have occurred. Got:
nil"), so it is a genuine regression test for this defect.
make build
-> succeeds.
golangci-lint run --timeout=10m ./pkg/bundle/...
-> 0 issues.
Report: shipwright-io#2322
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Assisted-by: claude-sonnet-5 (via Claude Code)
openshift-ci
Bot
added
the
release-note
pull-request-size
Bot
added
the
size/M
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Member
|
/ok-to-test |
openshift-ci
Bot
added
the
ok-to-test
Member
|
Hmm.. I wonder adding this label has no effect |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The deep traversal test relies on /etc/canary, making it environment-dependent and potentially unsafe.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Hardens bundle extraction against tar path traversal.
Changes:
- Validates extraction paths with
filepath.Rel. - Adds traversal regression tests.
File summaries
| File | Description |
|---|---|
pkg/bundle/bundle.go |
Rejects paths escaping the target directory. |
pkg/bundle/bundle_test.go |
Tests shallow and deep traversal attempts. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+94
to
+95
| targetDir := filepath.Join(outerDir, "target") | ||
| Expect(os.Mkdir(targetDir, os.FileMode(0755))).To(Succeed()) |
Author
|
/retest |
2 similar comments
Author
|
/retest |
Author
|
/retest |
Nest targetDir four levels under outerDir so the reported "../../../../etc/canary" payload still escapes targetDir but resolves inside the withTempDir sandbox instead of the host's real /etc, per Copilot review feedback. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
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.
Motivation:
pkg/bundle/bundle.go's Unpack function extracts tar entries from a
container image/OCI bundle and guarded against path traversal by
checking whether filepath.Join(targetPath, header.Name) contained the
literal substring "/../". filepath.Join calls filepath.Clean, which
resolves ".." segments before that check ever runs, so a tar entry
named e.g. "../../../../etc/passwd" is cleaned down to an absolute
path such as "/etc/passwd" that no longer contains "/../" and the
check never fires. A malicious bundle can therefore write files
anywhere the controller process has permission to write, entirely
outside the intended extraction directory.
Approach:
Replace the substring check with filepath.Rel(targetPath, target).
If the entry cannot be expressed as a path relative to targetPath
without a leading ".." component, the entry escapes the target
directory and is now rejected before anything is written to disk.
Tar entries handled by this function are limited to regular files and
directories (symlink entries already hit the "unsupported file type"
error), so this is the only path-construction site that needed
hardening here.
Validation:
go test ./pkg/bundle/... -race -v
-> all 5 specs pass, including two new table-driven cases that build
a raw tar stream with a "../canary" and a
"../../../../etc/canary" entry (the latter mirroring the exact
proof-of-concept from the report) and assert Unpack rejects both
and creates no file at the resolved escape path.
-> confirmed the new test fails against the old code (reverting only
bundle.go reproduces "Expected an error to have occurred. Got:
nil"), so it is a genuine regression test for this defect.
make build
-> succeeds.
golangci-lint run --timeout=10m ./pkg/bundle/...
-> 0 issues.
Report: #2322
Signed-off-by: Pujitha Paladugu 10557236+pujitha24@users.noreply.github.com
Assisted-by: claude-sonnet-5 (via Claude Code)
Fixes #2322