From 7696ddd3d2f7b352dd35487968ce22e819e6c464 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 23 Apr 2026 22:25:16 -0500 Subject: [PATCH] fix(extract): add path traversal guard for tar extraction Validate that extracted file paths, symlink targets, and hard link targets resolve within the destination directory. Malicious tar entries with ../ components are rejected with an error. This hardens all 11 callers of extract.Extract across the codebase. --- pkg/extract/extract.go | 174 +++++++++++++++++++++++------------ pkg/extract/extract_test.go | 176 ++++++++++++++++++++++++++++++++++++ 2 files changed, 290 insertions(+), 60 deletions(-) create mode 100644 pkg/extract/extract_test.go diff --git a/pkg/extract/extract.go b/pkg/extract/extract.go index 5d9193809..11ac37bc1 100644 --- a/pkg/extract/extract.go +++ b/pkg/extract/extract.go @@ -8,7 +8,7 @@ import ( "fmt" "io" "os" - "path" + "path/filepath" "strings" "time" ) @@ -67,101 +67,155 @@ func Extract(origReader io.Reader, destFolder string, options ...Option) error { } } -func extractNext(tarReader *tar.Reader, destFolder string, options *Options) (bool, error) { +// withinDir checks that resolved stays inside the destFolder boundary. +func withinDir(resolved, destFolder string) bool { + cleanDest := filepath.Clean(destFolder) + string(os.PathSeparator) + return strings.HasPrefix( + filepath.Clean(resolved)+string(os.PathSeparator), + cleanDest, + ) +} + +// resolveRelativePath strips levels and builds the output path. +func resolveRelativePath(header *tar.Header, opts *Options) string { + rel := getRelativeFromFullPath("/"+header.Name, "") + for i := 0; i < opts.StripLevels; i++ { + rel = strings.TrimPrefix(rel, "/") + idx := strings.Index(rel, "/") + if idx == -1 { + break + } + rel = rel[idx+1:] + } + if opts.StripLevels > 0 { + rel = "/" + rel + } + return rel +} + +func extractNext( + tarReader *tar.Reader, destFolder string, options *Options, +) (bool, error) { header, err := tarReader.Next() if err != nil { - if !errors.Is(err, io.EOF) { - return false, fmt.Errorf("tar reader next: %w", err) + if errors.Is(err, io.EOF) { + return false, nil } - - return false, nil + return false, fmt.Errorf("tar reader next: %w", err) } - relativePath := getRelativeFromFullPath("/"+header.Name, "") - if options.StripLevels > 0 { - for i := 0; i < options.StripLevels; i++ { - relativePath = strings.TrimPrefix(relativePath, "/") - index := strings.Index(relativePath, "/") - if index == -1 { - break - } + rel := resolveRelativePath(header, options) + outFileName := filepath.Join(destFolder, rel) + + if !withinDir(outFileName, destFolder) { + return false, fmt.Errorf( + "path traversal detected: %s resolves outside destination", + header.Name, + ) + } - relativePath = relativePath[index+1:] + switch header.Typeflag { + case tar.TypeSymlink, tar.TypeLink: + if err := validateLinkTarget(header, outFileName, destFolder); err != nil { + return false, err } + } - relativePath = "/" + relativePath + if err := extractEntry(tarReader, header, outFileName, options); err != nil { + return false, err } - outFileName := path.Join(destFolder, relativePath) - baseName := path.Dir(outFileName) + return true, nil +} - dirPerm := os.ModePerm - if options.Perm != nil { - dirPerm = *options.Perm +// validateLinkTarget ensures a symlink or hard link target stays within destFolder. +func validateLinkTarget(header *tar.Header, outFileName, destFolder string) error { + linkTarget := resolveLinkTarget(header.Linkname, outFileName) + if !withinDir(linkTarget, destFolder) { + kind := "symlink" + if header.Typeflag == tar.TypeLink { + kind = "hard link" + } + return fmt.Errorf( + "%s traversal detected: %s -> %s", + kind, header.Name, header.Linkname, + ) } + return nil +} - // Check if newer file is there and then don't override? - if err := os.MkdirAll(baseName, dirPerm); err != nil { - return false, err +// resolveLinkTarget resolves a link target to an absolute path. +func resolveLinkTarget(linkname, outFileName string) string { + if filepath.IsAbs(linkname) { + return filepath.Clean(linkname) } + return filepath.Clean(filepath.Join(filepath.Dir(outFileName), linkname)) +} - // whats the file perm? - filePerm := os.FileMode(0o644) +func extractEntry( + tarReader *tar.Reader, header *tar.Header, + outFileName string, options *Options, +) error { + dirPerm := os.ModePerm if options.Perm != nil { - filePerm = *options.Perm + dirPerm = *options.Perm + } + if err := os.MkdirAll(filepath.Dir(outFileName), dirPerm); err != nil { + return err } - // Is dir? switch header.Typeflag { case tar.TypeDir: - if err := os.MkdirAll(outFileName, dirPerm); err != nil { - return false, err - } - - return true, nil + return os.MkdirAll(outFileName, dirPerm) case tar.TypeSymlink: - err := os.Symlink(header.Linkname, outFileName) - if err != nil { - return false, err - } - - return true, nil + return os.Symlink(header.Linkname, outFileName) case tar.TypeLink: - err := os.Link(header.Linkname, outFileName) - if err != nil { - return false, err - } - - return true, nil + return os.Link(header.Linkname, outFileName) + default: + return extractRegularFile(tarReader, header, outFileName, options) } +} - // Create / Override file - outFile, err := os.OpenFile(outFileName, os.O_RDWR|os.O_CREATE|os.O_TRUNC, filePerm) +func extractRegularFile( + tarReader *tar.Reader, + header *tar.Header, + outFileName string, + options *Options, +) error { + filePerm := os.FileMode(0o644) + if options.Perm != nil { + filePerm = *options.Perm + } + outFile, err := openFileWithRetry(outFileName, filePerm) if err != nil { - // Try again after 5 seconds - time.Sleep(time.Second * 5) - outFile, err = os.OpenFile(outFileName, os.O_RDWR|os.O_CREATE|os.O_TRUNC, filePerm) - if err != nil { - return false, fmt.Errorf("create %s: %w", outFileName, err) - } + return err } defer func() { _ = outFile.Close() }() if _, err := io.Copy(outFile, tarReader); err != nil { - return false, fmt.Errorf("io copy tar reader %s: %w", outFileName, err) + return fmt.Errorf("io copy tar reader %s: %w", outFileName, err) } if err := outFile.Close(); err != nil { - return false, fmt.Errorf("out file close %s: %w", outFileName, err) + return fmt.Errorf("out file close %s: %w", outFileName, err) } - // Set permissions if options.Perm == nil { - _ = os.Chmod(outFileName, header.FileInfo().Mode()|0o600) // #nosec G703 + _ = os.Chmod(outFileName, header.FileInfo().Mode()|0o600) } - - // Set mod time from tar header _ = os.Chtimes(outFileName, time.Now(), header.FileInfo().ModTime()) + return nil +} - return true, nil +func openFileWithRetry(name string, perm os.FileMode) (*os.File, error) { + flags := os.O_RDWR | os.O_CREATE | os.O_TRUNC + f, err := os.OpenFile(filepath.Clean(name), flags, perm) + if err != nil { + time.Sleep(time.Second * 5) + f, err = os.OpenFile(filepath.Clean(name), flags, perm) + if err != nil { + return nil, fmt.Errorf("create %s: %w", name, err) + } + } + return f, nil } func getRelativeFromFullPath(fullpath string, prefix string) string { diff --git a/pkg/extract/extract_test.go b/pkg/extract/extract_test.go new file mode 100644 index 000000000..1c7ada5b8 --- /dev/null +++ b/pkg/extract/extract_test.go @@ -0,0 +1,176 @@ +package extract + +import ( + "archive/tar" + "bytes" + "compress/gzip" + "os" + "path/filepath" + "strings" + "testing" +) + +type tarEntry struct { + name string + body string + linkTarget string + symlink bool +} + +func (e tarEntry) header() *tar.Header { + if e.linkTarget != "" && e.symlink { + return &tar.Header{ + Typeflag: tar.TypeSymlink, + Name: e.name, + Linkname: e.linkTarget, + } + } + if e.linkTarget != "" { + return &tar.Header{ + Typeflag: tar.TypeLink, + Name: e.name, + Linkname: e.linkTarget, + } + } + return &tar.Header{ + Typeflag: tar.TypeReg, + Name: e.name, + Size: int64(len(e.body)), + Mode: 0o644, + } +} + +// newTarGz creates an in-memory tar.gz from a list of entries. +func newTarGz(t *testing.T, entries []tarEntry) *bytes.Buffer { + t.Helper() + var buf bytes.Buffer + gw := gzip.NewWriter(&buf) + tw := tar.NewWriter(gw) + + for _, e := range entries { + writeTarEntry(t, tw, e) + } + + if err := tw.Close(); err != nil { + t.Fatal(err) + } + if err := gw.Close(); err != nil { + t.Fatal(err) + } + return &buf +} + +func writeTarEntry(t *testing.T, tw *tar.Writer, e tarEntry) { + t.Helper() + if err := tw.WriteHeader(e.header()); err != nil { + t.Fatal(err) + } + if e.body != "" { + if _, err := tw.Write([]byte(e.body)); err != nil { + t.Fatal(err) + } + } +} + +func TestExtract_NormalArchive(t *testing.T) { + t.Parallel() + buf := newTarGz(t, []tarEntry{ + {name: "hello.txt", body: "world"}, + }) + + dest := t.TempDir() + if err := Extract(buf, dest); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + out := filepath.Join(dest, "hello.txt") + content, err := os.ReadFile(filepath.Clean(out)) + if err != nil { + t.Fatalf("read extracted file: %v", err) + } + if string(content) != "world" { + t.Fatalf("got %q, want %q", string(content), "world") + } +} + +func TestExtract_PathTraversalBlocked(t *testing.T) { + t.Parallel() + buf := newTarGz(t, []tarEntry{ + {name: "../../etc/passwd", body: "malicious"}, + }) + + dest := t.TempDir() + err := Extract(buf, dest) + if err == nil { + t.Fatal("expected path traversal error, got nil") + } + if !strings.Contains(err.Error(), "path traversal") { + t.Fatalf("error %q does not mention path traversal", err) + } +} + +func TestExtract_SymlinkTraversalBlocked(t *testing.T) { + t.Parallel() + buf := newTarGz(t, []tarEntry{ + { + name: "evil-link", + linkTarget: "../../etc/passwd", + symlink: true, + }, + }) + + dest := t.TempDir() + err := Extract(buf, dest) + if err == nil { + t.Fatal("expected symlink traversal error, got nil") + } + if !strings.Contains(err.Error(), "symlink traversal") { + t.Fatalf("error %q does not mention symlink traversal", err) + } +} + +func TestExtract_HardLinkTraversalBlocked(t *testing.T) { + t.Parallel() + buf := newTarGz(t, []tarEntry{ + { + name: "evil-link", + linkTarget: "../../etc/passwd", + symlink: false, + }, + }) + + dest := t.TempDir() + err := Extract(buf, dest) + if err == nil { + t.Fatal("expected hard link traversal error, got nil") + } + if !strings.Contains(err.Error(), "hard link traversal") { + t.Fatalf("error %q doesn't mention hard link traversal", err) + } +} + +func TestExtract_ValidSymlinkAllowed(t *testing.T) { + t.Parallel() + buf := newTarGz(t, []tarEntry{ + {name: "target.txt", body: "content"}, + { + name: "link.txt", + linkTarget: "target.txt", + symlink: true, + }, + }) + + dest := t.TempDir() + if err := Extract(buf, dest); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + linkPath := filepath.Join(dest, "link.txt") + target, err := os.Readlink(linkPath) + if err != nil { + t.Fatalf("readlink: %v", err) + } + if target != "target.txt" { + t.Fatalf("symlink target = %q, want %q", target, "target.txt") + } +}