mutate: reject path traversal and symlink escape in Extract#2227
mutate: reject path traversal and symlink escape in Extract#2227Subserial merged 4 commits intogoogle:mainfrom
Conversation
The extract() function passes tar entry names and symlink targets through to the output stream without validating that they stay within the extraction root. This allows a malicious container image to: 1. Write files outside the extraction directory via ../ prefixes (filepath.Clean preserves leading ../ sequences) 2. Create symlinks pointing to arbitrary host paths via absolute or ../-prefixed Linkname values 3. Use absolute paths in entry names to target fixed host locations Add validation after filepath.Clean to skip entries with path traversal (../ prefix) or absolute paths in both Name and Linkname fields. This follows the mitigation pattern established by Docker (moby/moby) and containerd after CVE-2018-15664 and CVE-2019-14271. Add table-driven tests covering all five attack vectors (path traversal, absolute path, symlink escape via absolute and relative targets, hardlink escape) plus a positive case confirming safe relative symlinks are preserved.
|
Friendly ping — could someone take a look when you get a chance? Happy to address any feedback. |
|
Looks like just a few small errors. You should be able to run |
Absolute entry names (e.g. /etc/shadow) are now stripped of their leading slash and emitted as relative paths rather than being silently dropped. This preserves existing behaviour for layers that store files with absolute paths (used by crane edit fs and reflected in TestEditFilesystem / TestCraneFilesystem) while still preventing injection of host-absolute paths when consumers extract the tar to disk. Symlink and hardlink entries with absolute or dot-dot targets continue to be rejected outright, as those can never be safe. Update TestExtractRejectsPathTraversal to document the normalization behaviour, and update TestCraneFilesystem to use a relative path consistent with what Extract now emits.
|
Thanks for looking @Subserial! I ran `./hack/presubmit.sh` locally and everything passes (gofmt, `go test ./...`, crane doc gen, rebase integration test, krane build, k8schain tests). Could you point out the specific errors you noticed? Happy to fix them. |
Fix golangci-lint staticcheck SA1019 warning.
|
Found and fixed the issues from the first commit's CI:
All checks pass locally: `golangci-lint run`, `go test -race ./...`, `./hack/presubmit.sh`. |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #2227 +/- ##
===========================================
- Coverage 71.67% 52.70% -18.98%
===========================================
Files 123 164 +41
Lines 9935 11108 +1173
===========================================
- Hits 7121 5854 -1267
- Misses 2115 4547 +2432
- Partials 699 707 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
mutate.Extract()(used bycrane exportandcrane flatten) passes tar entry names and symlink targets to the output stream without validating they stay within the extraction root. A malicious container image can exploit this to:../../prefixes (filepath.Cleanpreserves leading../sequences)header.Linknameis never validated)This affects any consumer that extracts the output of
mutate.Extract(), includingcrane export <image> - | tar xf - -C /dir.Changes
header.Namehas a../prefix or is an absolute pathheader.Linknamehas a../prefix or is an absolute pathReferences
docker cpdocker cp