Skip to content

mutate: reject path traversal and symlink escape in Extract#2227

Merged
Subserial merged 4 commits intogoogle:mainfrom
KevinZhao:fix/extract-path-traversal
Mar 17, 2026
Merged

mutate: reject path traversal and symlink escape in Extract#2227
Subserial merged 4 commits intogoogle:mainfrom
KevinZhao:fix/extract-path-traversal

Conversation

@KevinZhao
Copy link
Copy Markdown
Contributor

Summary

mutate.Extract() (used by crane export and crane 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:

  • Path traversal: Write files outside the extraction directory via ../../ prefixes (filepath.Clean preserves leading ../ sequences)
  • Symlink escape: Create symlinks pointing to arbitrary host paths (header.Linkname is never validated)
  • Absolute path injection: Target fixed host locations via absolute paths in entry names

This affects any consumer that extracts the output of mutate.Extract(), including crane export <image> - | tar xf - -C /dir.

Changes

  • Skip tar entries where header.Name has a ../ prefix or is an absolute path
  • Skip symlink/hardlink entries where header.Linkname has a ../ prefix or is an absolute path
  • Add table-driven tests covering all five attack vectors plus a positive case for safe relative symlinks

References

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.
@KevinZhao
Copy link
Copy Markdown
Contributor Author

Friendly ping — could someone take a look when you get a chance? Happy to address any feedback.

@Subserial
Copy link
Copy Markdown
Contributor

Looks like just a few small errors. You should be able to run ./hack/presubmit.sh for local testing.

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.
@KevinZhao
Copy link
Copy Markdown
Contributor Author

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.
@KevinZhao
Copy link
Copy Markdown
Contributor Author

Found and fixed the issues from the first commit's CI:

  1. Test failures (commit 2 already fixed): `TestCraneFilesystem` and `TestEditFilesystem` failed because absolute paths like `/some/file` were being skipped. Changed behavior to normalize (strip leading `/`) instead of skip, which preserves entries while preventing host-path injection.

  2. golangci-lint (commit 3, just pushed): `tarball.LayerFromReader` is deprecated (SA1019). Replaced with `tarball.LayerFromOpener` in the test helper.

All checks pass locally: `golangci-lint run`, `go test -race ./...`, `./hack/presubmit.sh`.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 16, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 52.70%. Comparing base (8b3c303) to head (2300d1b).
⚠️ Report is 72 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (8b3c303) and HEAD (2300d1b). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (8b3c303) HEAD (2300d1b)
2 1
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Subserial Subserial merged commit 400c263 into google:main Mar 17, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants