Fix tar slip path traversal in codesign archive extraction#2291
Fix tar slip path traversal in codesign archive extraction#2291rhamitarora wants to merge 3 commits into
Conversation
Validate that each tar entry resolves within the extraction directory before writing, preventing a malicious archive from escaping via '..' entries (CodeQL SM03409). Add unit tests covering a traversal entry and a benign entry. Jira: ARO-26358
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
WalkthroughThe PR hardens two tar extraction paths against path traversal attacks. In ChangesTar-Slip Protection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @rhamitarora. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rhamitarora 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cli/admin/internal/codesign/machoresign.go (1)
125-166:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClean up
MkdirTempdirectories on error returns in extraction.The new rejection branch returns
""(Line 165), so callers cannot remove the created temp dir. Repeated invalid archives can accumulate stale dirs under/tmp.Suggested fix
-func extractTarToTmpAndSign(path string) (string, error) { +func extractTarToTmpAndSign(path string) (retDir string, retErr error) { tempDir, err := os.MkdirTemp(os.TempDir(), "oc-release-extract-*") if err != nil { return "", err } + defer func() { + if retErr != nil { + _ = os.RemoveAll(tempDir) + } + }() cleanTempDir := filepath.Clean(tempDir)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/admin/internal/codesign/machoresign.go` around lines 125 - 166, The function extractTarToTmpAndSign creates a temporary directory with os.MkdirTemp but does not clean it up when returning errors, causing stale directories to accumulate. Add a deferred cleanup function immediately after creating tempDir that removes the directory if the function exits with an error, or alternatively add explicit os.RemoveAll(tempDir) calls before each error return statement (including the tar slip validation check at line 165 that validates tempExtractionPath).
🧹 Nitpick comments (1)
pkg/cli/admin/internal/codesign/machoresign_test.go (1)
48-90: ⚡ Quick winRefactor these two cases into a single table-driven test.
Both cases share setup/execution and differ only by archive entries and expected outcome; consolidating into table-driven form will align with repo test conventions and reduce duplication.
As per coding guidelines, "Unit tests should use co-located
*_test.gofiles with table-driven tests and standardtesting.T."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/admin/internal/codesign/machoresign_test.go` around lines 48 - 90, Consolidate the two test functions TestExtractTarToTmpAndSign_RejectsPathTraversal and TestExtractTarToTmpAndSign_ExtractsRegularFile into a single table-driven test. Create a test cases slice where each case contains a name, the archive entries to pass to writeTarGz, a boolean indicating whether an error is expected, and assertions to verify (such as checking if a file exists or doesn't exist). Replace both test functions with a single test function that iterates over the test cases, executing the common setup and extractTarToTmpAndSign call for each case, then running the case-specific assertions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/cli/admin/internal/codesign/machoresign.go`:
- Around line 125-166: The function extractTarToTmpAndSign creates a temporary
directory with os.MkdirTemp but does not clean it up when returning errors,
causing stale directories to accumulate. Add a deferred cleanup function
immediately after creating tempDir that removes the directory if the function
exits with an error, or alternatively add explicit os.RemoveAll(tempDir) calls
before each error return statement (including the tar slip validation check at
line 165 that validates tempExtractionPath).
---
Nitpick comments:
In `@pkg/cli/admin/internal/codesign/machoresign_test.go`:
- Around line 48-90: Consolidate the two test functions
TestExtractTarToTmpAndSign_RejectsPathTraversal and
TestExtractTarToTmpAndSign_ExtractsRegularFile into a single table-driven test.
Create a test cases slice where each case contains a name, the archive entries
to pass to writeTarGz, a boolean indicating whether an error is expected, and
assertions to verify (such as checking if a file exists or doesn't exist).
Replace both test functions with a single test function that iterates over the
test cases, executing the common setup and extractTarToTmpAndSign call for each
case, then running the case-specific assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4c193e4d-3228-4069-9072-61ccb5d72ed0
📒 Files selected for processing (2)
pkg/cli/admin/internal/codesign/machoresign.gopkg/cli/admin/internal/codesign/machoresign_test.go
Move the path-containment check in unpackLayer above the parent directory creation so the '..' sanitizer dominates every filesystem operation (CodeQL go/zipslip at archive.go). Simplify the codesign extraction guard to a single strings.HasPrefix so the static analyzer recognizes it as a sanitizer barrier. Jira: ARO-26358
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cli/image/archive/archive.go`:
- Around line 147-155: The containment check in the filepath validation logic
only checks if the relative path `rel` starts with ".." followed by a path
separator, but misses the exact case where `rel` equals exactly "..". Modify the
condition that currently uses `strings.HasPrefix(rel,
".."+string(os.PathSeparator))` to also explicitly check for the equality case
`rel == ".."` by combining both conditions with an OR operator, ensuring that
both the exact ".." case and the prefix case that indicates path traversal are
caught before the breakoutError is returned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1f61c187-1842-418c-82f8-af4c777ff6c5
📒 Files selected for processing (2)
pkg/cli/admin/internal/codesign/machoresign.gopkg/cli/image/archive/archive.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cli/admin/internal/codesign/machoresign.go
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
filepath.Rel returns exactly ".." (no trailing separator) when an entry normalizes to the parent of dest, which the HasPrefix-only guard missed, allowing a directory escape. Also reject rel == "..". Jira: ARO-26358
1 participant
Jira: ARO-26358
Jira: ARO-26359
Jira: ARO-26373
Summary by CodeRabbit
This PR fixes two CodeQL go/zipslip path-traversal alerts (SM03409, Jira ARO-26358, , Jira ARO-26359, Jira ARO-26373) where untrusted tar entry names containing .. were used in filesystem operations, allowing a malicious archive to write outside the intended extraction directory. In machoresign.go, extractTarToTmpAndSign now computes the clean temp dir once and rejects any entry whose resolved path escapes it via a single strings.HasPrefix boundary check before opening files. In archive.go, the existing containment guard in unpackLayer is moved above the parent-directory creation so it dominates every filesystem sink, with no behavioral change for valid archives. New unit tests cover both a malicious ../ entry (rejected, nothing written outside the dir) and a benign entry (extracted successfully). Validated with make verify plus builds and tests for both packages; since these are first-party sources, the issue is fixed in code rather than suppressed.
Release Notes
Bug Fixes
Tests
Added unit tests covering both malicious archive entries (a ../ path that is rejected safely, with an assertion that nothing is written outside the extraction area) and valid entries (extracted as expected). Both affected packages build and pass: go test -tags 'include_gcs include_oss containers_image_openpgp gssapi' ./pkg/cli/image/archive/... internal..
Verification
— Ran the full suite make verify (CLI conventions, build, generated completions, kube-version) — all green. Re-ran the CodeQL go/zipslip analysis against the branch after applying the changes and confirmed both alerts (machoresign.go:146 and archive.go:120) are now resolved, whereas the prior scan on main (oc-zipslip-main.sarif) still reported them — confirming the fix clears the issue.
Summary by CodeRabbit
Bug Fixes
Tests