From 51215c325626b4459781a42d235b43408057f0c6 Mon Sep 17 00:00:00 2001 From: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Date: Sun, 6 Sep 2026 13:14:46 -0700 Subject: [PATCH 1/2] fix: validate tar entry paths against escaping the target directory in 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: https://github.com/shipwright-io/build/issues/2322 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-sonnet-5 (via Claude Code) --- pkg/bundle/bundle.go | 6 +++--- pkg/bundle/bundle_test.go | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/pkg/bundle/bundle.go b/pkg/bundle/bundle.go index ee5b207016..ed167b8fca 100644 --- a/pkg/bundle/bundle.go +++ b/pkg/bundle/bundle.go @@ -270,10 +270,10 @@ func Unpack(in io.Reader, targetPath string) (*UnpackDetails, error) { continue } - // #nosec G305 path traversal is checked by validating that the resulting path does not contain unexpected special elements + // #nosec G305 path traversal is checked by validating that the resulting path does not escape targetPath var target = filepath.Join(targetPath, header.Name) - if strings.Contains(target, "/../") { - return nil, fmt.Errorf("targetPath validation failed, path contains unexpected special elements") + if rel, err := filepath.Rel(targetPath, target); err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return nil, fmt.Errorf("targetPath validation failed, %q escapes the target directory", header.Name) } switch header.Typeflag { diff --git a/pkg/bundle/bundle_test.go b/pkg/bundle/bundle_test.go index 265974fb73..ab0d79516c 100644 --- a/pkg/bundle/bundle_test.go +++ b/pkg/bundle/bundle_test.go @@ -5,6 +5,8 @@ package bundle_test import ( + "archive/tar" + "bytes" "fmt" "log" "net/http/httptest" @@ -85,6 +87,38 @@ var _ = Describe("Bundle", func() { }) }) }) + + DescribeTable("should reject a tar entry that attempts to escape the target directory", + func(entryName string) { + withTempDir(func(outerDir string) { + targetDir := filepath.Join(outerDir, "target") + Expect(os.Mkdir(targetDir, os.FileMode(0755))).To(Succeed()) + + // where the malicious entry would land if it were not rejected + escapedPath := filepath.Join(targetDir, entryName) + + var buf bytes.Buffer + tw := tar.NewWriter(&buf) + Expect(tw.WriteHeader(&tar.Header{ + Name: entryName, + Typeflag: tar.TypeReg, + Mode: 0644, + Size: int64(len("owned")), + })).To(Succeed()) + _, err := tw.Write([]byte("owned")) + Expect(err).ToNot(HaveOccurred()) + Expect(tw.Close()).To(Succeed()) + + _, err = Unpack(&buf, targetDir) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("escapes the target directory")) + + Expect(escapedPath).ToNot(BeAnExistingFile()) + }) + }, + Entry("single level traversal", "../canary"), + Entry("deep traversal matching the reported issue", "../../../../etc/canary"), + ) }) Context("packing/pushing and pulling/unpacking", func() { From acdd37e88a2a2c3e598081b7fcf50637169001d5 Mon Sep 17 00:00:00 2001 From: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Date: Tue, 8 Sep 2026 04:56:56 -0700 Subject: [PATCH 2/2] test: isolate deep-traversal fixture to the temp dir instead of /etc 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> --- pkg/bundle/bundle_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/bundle/bundle_test.go b/pkg/bundle/bundle_test.go index ab0d79516c..75046d9b0a 100644 --- a/pkg/bundle/bundle_test.go +++ b/pkg/bundle/bundle_test.go @@ -91,8 +91,11 @@ var _ = Describe("Bundle", func() { DescribeTable("should reject a tar entry that attempts to escape the target directory", func(entryName string) { withTempDir(func(outerDir string) { - targetDir := filepath.Join(outerDir, "target") - Expect(os.Mkdir(targetDir, os.FileMode(0755))).To(Succeed()) + // nested four levels deep so that the "../../../../etc/canary" + // payload still escapes targetDir while resolving to a path + // inside outerDir, not the host's real /etc + targetDir := filepath.Join(outerDir, "a", "b", "c", "target") + Expect(os.MkdirAll(targetDir, os.FileMode(0755))).To(Succeed()) // where the malicious entry would land if it were not rejected escapedPath := filepath.Join(targetDir, entryName)