From 02440aa36241cff0e84fb24c11cbfc4aa6849888 Mon Sep 17 00:00:00 2001 From: Alec Thomas Date: Fri, 13 Nov 2026 11:37:05 +1100 Subject: [PATCH] fix(client): restore system tar for extraction #418 replaced `tar -xpf` with an in-process extractor that validates entry paths and symlink targets. Clients only extract archives that cachew service accounts wrote, so there is no untrusted input to defend against, and the extractor had real costs: - ~35% slower on trees of many small files (Go source tree: 5.6s -> 7.6s), mostly from repeated Lstat calls on every parent directory. - It rejected legitimate absolute or out-of-tree symlinks, so trees such as Python virtualenvs could no longer be restored. Extract now documents that cachew must only accept writes from trusted sources. Request-path validation and GitHub URL escaping from #418 are unchanged. Co-Authored-By: Claude Opus 5.5 --- client/archive.go | 302 +++----------------------------- client/archive_security_test.go | 177 ------------------- client/client_test.go | 22 +++ 3 files changed, 42 insertions(+), 459 deletions(-) diff --git a/client/archive.go b/client/archive.go index 3075c6c7..f3112ee5 100644 --- a/client/archive.go +++ b/client/archive.go @@ -1,7 +1,6 @@ package client import ( - "archive/tar" "bytes" "context" "fmt" @@ -12,7 +11,6 @@ import ( "path/filepath" "runtime" "strings" - "time" "github.com/alecthomas/errors" ) @@ -57,12 +55,11 @@ func Archive(ctx context.Context, w io.Writer, baseDir string, includePaths []st } // Extract decompresses a zstd+tar stream from r into directory, preserving -// file permissions, modification times and symlinks. threads controls zstd parallelism; 0 uses all -// CPU cores. +// file permissions, ownership, and symlinks. threads controls zstd +// parallelism; 0 uses all CPU cores. // -// Existing read-only directories are made owner-writable so entries can be -// replaced. Archived directory modes are applied after their contents, and a -// failed extract restores pre-existing read-only directories. +// r must come from a trusted source: entries are not validated beyond tar's +// defaults, so cachew must only allow writes from trusted sources. func Extract(ctx context.Context, r io.Reader, directory string, threads int) (rerr error) { if threads <= 0 { threads = runtime.GOMAXPROCS(0) @@ -72,6 +69,8 @@ func Extract(ctx context.Context, r io.Reader, directory string, threads int) (r return errors.Wrap(err, "failed to create target directory") } + // Relax read-only dirs (e.g. Hermit package trees) so tar can overwrite + // their contents, restoring the original modes if anything fails. relaxed, err := makeTreeWritable(directory) defer func() { if rerr != nil { @@ -83,16 +82,21 @@ func Extract(ctx context.Context, r io.Reader, directory string, threads int) (r } zstdCmd := exec.CommandContext(ctx, "zstd", "-dc", fmt.Sprintf("-T%d", threads)) //nolint:gosec + tarCmd := exec.CommandContext(ctx, "tar", "-xpf", "-", "-C", directory) + pr, pw, err := os.Pipe() if err != nil { return errors.Wrap(err, "failed to create pipe") } - var zstdStderr bytes.Buffer + var zstdStderr, tarStderr bytes.Buffer zstdCmd.Stdin = r zstdCmd.Stdout = pw zstdCmd.Stderr = &zstdStderr + tarCmd.Stdin = pr + tarCmd.Stderr = &tarStderr + if err := zstdCmd.Start(); err != nil { pw.Close() //nolint:errcheck,gosec pr.Close() //nolint:errcheck,gosec @@ -100,289 +104,23 @@ func Extract(ctx context.Context, r io.Reader, directory string, threads int) (r } pw.Close() //nolint:errcheck,gosec - extractErr := safeExtractTar(ctx, pr, directory) - if extractErr == nil { - // tar.Reader stops at the end-of-archive marker; drain the record padding so zstd isn't killed by SIGPIPE. - if _, err := io.Copy(io.Discard, pr); err != nil { - extractErr = errors.Wrap(err, "failed to drain archive stream") - } + if err := tarCmd.Start(); err != nil { + pr.Close() //nolint:errcheck,gosec + return errors.Join(errors.Wrap(err, "failed to start tar"), zstdCmd.Wait()) } pr.Close() //nolint:errcheck,gosec + zstdErr := zstdCmd.Wait() + tarErr := tarCmd.Wait() var errs []error - if extractErr != nil { - errs = append(errs, extractErr) - } if zstdErr != nil { errs = append(errs, errors.Errorf("zstd failed: %w: %s", zstdErr, zstdStderr.String())) } - return errors.Join(errs...) -} - -func safeExtractTar(ctx context.Context, r io.Reader, directory string) error { - tr := tar.NewReader(r) - var dirs []extractedDir - for { - if err := ctx.Err(); err != nil { - return errors.WithStack(err) - } - hdr, err := tr.Next() - switch { - case errors.Is(err, io.EOF): - return applyDirModes(dirs) - case err != nil: - return errors.Wrap(err, "failed to read tar header") - } - - if err := extractTarEntry(directory, hdr, tr, &dirs); err != nil { - return err - } - } -} - -func applyDirModes(dirs []extractedDir) error { - for i := len(dirs) - 1; i >= 0; i-- { - if err := os.Chmod(dirs[i].path, dirs[i].mode); err != nil { - return errors.Wrapf(err, "failed to chmod %s", dirs[i].path) - } - if err := os.Chtimes(dirs[i].path, time.Time{}, dirs[i].modTime); err != nil { - return errors.Wrapf(err, "failed to set times on %s", dirs[i].path) - } - } - return nil -} - -func extractTarEntry(directory string, hdr *tar.Header, r io.Reader, dirs *[]extractedDir) error { - target, rel, err := safeArchivePath(directory, hdr.Name) - if err != nil { - return err - } - mode := tarMode(hdr) - switch hdr.Typeflag { - case tar.TypeDir: - if err := mkdirAllWithin(directory, rel, mode|0o700); err != nil { - return err - } - *dirs = append(*dirs, extractedDir{path: target, mode: mode, modTime: hdr.ModTime}) - case tar.TypeReg, 0: - return extractRegularFile(directory, rel, target, mode, hdr.ModTime, r) - case tar.TypeSymlink: - return extractSymlink(directory, rel, target, hdr.Linkname) - case tar.TypeLink: - return extractHardLink(directory, rel, target, hdr.Linkname) - case tar.TypeXHeader, tar.TypeXGlobalHeader: - return nil - default: - return errors.Errorf("unsupported tar entry %q type %d", hdr.Name, hdr.Typeflag) - } - return nil -} - -func tarMode(hdr *tar.Header) fs.FileMode { - return fs.FileMode(uint32(hdr.Mode & int64(fs.ModePerm))) -} - -func extractRegularFile(directory, rel, target string, mode fs.FileMode, modTime time.Time, r io.Reader) error { - if err := mkdirAllWithin(directory, filepath.Dir(rel), 0o750); err != nil { - return err - } - if err := ensureNoSymlinkParents(directory, rel); err != nil { - return err - } - if err := rejectSymlink(target); err != nil { - return err - } - if err := removeExistingNonDir(target); err != nil { - return err - } - if err := writeRegularFile(target, mode, r); err != nil { - return err - } - if err := os.Chtimes(target, time.Time{}, modTime); err != nil { - return errors.Wrapf(err, "failed to set times on %s", target) - } - return nil -} - -func extractSymlink(directory, rel, target, linkname string) error { - if err := validateSymlinkTarget(rel, linkname); err != nil { - return err - } - if err := mkdirAllWithin(directory, filepath.Dir(rel), 0o750); err != nil { - return err - } - if err := ensureNoSymlinkParents(directory, rel); err != nil { - return err - } - if err := removeExistingNonDir(target); err != nil { - return err - } - if err := os.Symlink(linkname, target); err != nil { - return errors.Wrapf(err, "failed to create symlink %s", target) - } - return nil -} - -func extractHardLink(directory, rel, target, linkname string) error { - linkTarget, linkRel, err := safeArchivePath(directory, linkname) - if err != nil { - return err - } - if err := ensureNoSymlinkParents(directory, linkRel); err != nil { - return err - } - if err := rejectSymlink(linkTarget); err != nil { - return err - } - if err := mkdirAllWithin(directory, filepath.Dir(rel), 0o750); err != nil { - return err - } - if err := ensureNoSymlinkParents(directory, rel); err != nil { - return err - } - if err := removeExistingNonDir(target); err != nil { - return err - } - if err := os.Link(linkTarget, target); err != nil { - return errors.Wrapf(err, "failed to create hard link %s", target) - } - return nil -} - -func safeArchivePath(root, name string) (string, string, error) { - if strings.Contains(name, "\\") { - return "", "", errors.Errorf("archive path %q contains a path separator unsupported by tar", name) - } - rel := filepath.FromSlash(name) - if !filepath.IsLocal(rel) { - return "", "", errors.Errorf("archive path %q must be relative and stay within target directory", name) - } - rel = filepath.Clean(rel) - return filepath.Join(root, rel), rel, nil -} - -func validateSymlinkTarget(rel, linkname string) error { - if strings.Contains(linkname, "\\") { - return errors.Errorf("symlink target %q contains a path separator unsupported by tar", linkname) - } - linkTarget := filepath.FromSlash(linkname) - if filepath.IsAbs(linkTarget) || !filepath.IsLocal(filepath.Join(filepath.Dir(rel), linkTarget)) { - return errors.Errorf("symlink target %q must be relative and stay within target directory", linkname) - } - // ".." after a name would resolve relative to wherever that name points, so chained symlinks - // such as "a/b -> ..", "s -> a/b", "u -> s/.." could escape despite each being lexically local. - sawName := false - for part := range strings.SplitSeq(linkname, "/") { - switch part { - case "", ".": - case "..": - if sawName { - return errors.Errorf("symlink target %q must only use \"..\" as leading components", linkname) - } - default: - sawName = true - } - } - return nil -} - -func mkdirAllWithin(root, rel string, mode fs.FileMode) error { - if rel == "." { - return nil - } - current := root - for part := range strings.SplitSeq(rel, string(filepath.Separator)) { - current = filepath.Join(current, part) - info, err := os.Lstat(current) - switch { - case err == nil && info.Mode()&fs.ModeSymlink != 0: - return errors.Errorf("refusing to extract through symlink %s", current) - case err == nil && !info.IsDir(): - return errors.Errorf("refusing to create directory over non-directory %s", current) - case err == nil: - continue - case !errors.Is(err, os.ErrNotExist): - return errors.Wrapf(err, "failed to stat %s", current) - } - if err := os.Mkdir(current, mode); err != nil && !errors.Is(err, os.ErrExist) { - return errors.Wrapf(err, "failed to create directory %s", current) - } - } - return nil -} - -func ensureNoSymlinkParents(root, rel string) error { - dir := filepath.Dir(rel) - if dir == "." { - return nil - } - current := root - for part := range strings.SplitSeq(dir, string(filepath.Separator)) { - current = filepath.Join(current, part) - info, err := os.Lstat(current) - if err != nil { - return errors.Wrapf(err, "failed to stat %s", current) - } - if info.Mode()&fs.ModeSymlink != 0 { - return errors.Errorf("refusing to extract through symlink %s", current) - } - } - return nil -} - -func rejectSymlink(path string) error { - info, err := os.Lstat(path) //nolint:gosec - if errors.Is(err, os.ErrNotExist) { - return nil - } - if err != nil { - return errors.Wrapf(err, "failed to stat %s", path) - } - if info.Mode()&fs.ModeSymlink != 0 { - return errors.Errorf("refusing to replace symlink %s", path) - } - return nil -} - -func removeExistingNonDir(path string) error { - info, err := os.Lstat(path) //nolint:gosec - if errors.Is(err, os.ErrNotExist) { - return nil - } - if err != nil { - return errors.Wrapf(err, "failed to stat %s", path) - } - if info.IsDir() { - return errors.Errorf("refusing to replace directory %s", path) - } - if err := os.Remove(path); err != nil { //nolint:gosec - return errors.Wrapf(err, "failed to remove %s", path) - } - return nil -} - -func writeRegularFile(path string, mode fs.FileMode, r io.Reader) (rerr error) { - f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, mode) //nolint:gosec - if err != nil { - return errors.Wrapf(err, "failed to create file %s", path) - } - defer func() { - rerr = errors.Join(rerr, f.Close()) - }() - if _, err := io.Copy(f, r); err != nil { - return errors.Wrapf(err, "failed to write file %s", path) - } - if err := f.Chmod(mode); err != nil { - return errors.Wrapf(err, "failed to chmod %s", path) + if tarErr != nil { + errs = append(errs, errors.Errorf("tar failed: %w: %s", tarErr, tarStderr.String())) } - return nil -} - -type extractedDir struct { - path string - mode fs.FileMode - modTime time.Time + return errors.Join(errs...) } // dirMode is a directory path and its pre-relaxation mode. diff --git a/client/archive_security_test.go b/client/archive_security_test.go index 8790e4d7..a0237d7c 100644 --- a/client/archive_security_test.go +++ b/client/archive_security_test.go @@ -1,13 +1,10 @@ package client_test import ( - "archive/tar" "bytes" "os" - "os/exec" "path/filepath" "testing" - "time" "github.com/alecthomas/assert/v2" @@ -23,177 +20,3 @@ func TestArchiveRejectsIncludePathsOutsideBase(t *testing.T) { assert.Error(t, client.Archive(t.Context(), &out, base, []string{"../secret.txt"}, nil, 0)) assert.Error(t, client.Archive(t.Context(), &out, base, []string{outside}, nil, 0)) } - -func TestExtractRejectsTraversalEntries(t *testing.T) { - parent := t.TempDir() - dst := filepath.Join(parent, "dst") - payload := compressedTar(t, func(tw *tar.Writer) { - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "../../pwned", Mode: 0o600, Size: int64(len("pwned"))})) - _, err := tw.Write([]byte("pwned")) - assert.NoError(t, err) - }) - - err := client.Extract(t.Context(), bytes.NewReader(payload), dst, 0) - assert.Error(t, err) - _, statErr := os.Stat(filepath.Join(parent, "pwned")) - assert.True(t, os.IsNotExist(statErr)) -} - -func TestExtractRejectsSymlinkTraversal(t *testing.T) { - parent := t.TempDir() - dst := filepath.Join(parent, "dst") - payload := compressedTar(t, func(tw *tar.Writer) { - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "link", Typeflag: tar.TypeSymlink, Linkname: "..", Mode: 0o777})) - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "link/pwned", Mode: 0o600, Size: int64(len("pwned"))})) - _, err := tw.Write([]byte("pwned")) - assert.NoError(t, err) - }) - - err := client.Extract(t.Context(), bytes.NewReader(payload), dst, 0) - assert.Error(t, err) - _, statErr := os.Stat(filepath.Join(parent, "pwned")) - assert.True(t, os.IsNotExist(statErr)) -} - -func TestExtractAllowsSymlinkWithinTarget(t *testing.T) { - dst := t.TempDir() - payload := compressedTar(t, func(tw *tar.Writer) { - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "target", Mode: 0o600, Size: int64(len("ok"))})) - _, err := tw.Write([]byte("ok")) - assert.NoError(t, err) - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "sub", Typeflag: tar.TypeDir, Mode: 0o755})) - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "sub/link", Typeflag: tar.TypeSymlink, Linkname: "../target", Mode: 0o777})) - }) - - assert.NoError(t, client.Extract(t.Context(), bytes.NewReader(payload), dst, 0)) - target, err := os.Readlink(filepath.Join(dst, "sub", "link")) - assert.NoError(t, err) - assert.Equal(t, "../target", target) -} - -func TestExtractRejectsHardLinkThroughSymlinkedParent(t *testing.T) { - parent := t.TempDir() - outside := filepath.Join(parent, "outside") - assert.NoError(t, os.Mkdir(outside, 0o750)) - assert.NoError(t, os.WriteFile(filepath.Join(outside, "secret"), []byte("secret"), 0o600)) - dst := filepath.Join(parent, "dst") - assert.NoError(t, os.Mkdir(dst, 0o750)) - assert.NoError(t, os.Symlink(outside, filepath.Join(dst, "escape"))) - payload := compressedTar(t, func(tw *tar.Writer) { - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "stolen", Typeflag: tar.TypeLink, Linkname: "escape/secret"})) - }) - - err := client.Extract(t.Context(), bytes.NewReader(payload), dst, 0) - assert.Error(t, err) - _, statErr := os.Lstat(filepath.Join(dst, "stolen")) - assert.True(t, os.IsNotExist(statErr)) -} - -func TestExtractRejectsChainedSymlinkEscape(t *testing.T) { - for _, linkname := range []string{"s/..", "s/m/../..", "./s/.."} { - t.Run(linkname, func(t *testing.T) { - dst := filepath.Join(t.TempDir(), "dst") - payload := compressedTar(t, func(tw *tar.Writer) { - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "a", Typeflag: tar.TypeDir, Mode: 0o755})) - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "a/b", Typeflag: tar.TypeSymlink, Linkname: ".."})) - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "s", Typeflag: tar.TypeSymlink, Linkname: "a/b"})) - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "u", Typeflag: tar.TypeSymlink, Linkname: linkname})) - }) - - err := client.Extract(t.Context(), bytes.NewReader(payload), dst, 0) - assert.Error(t, err) - _, statErr := os.Lstat(filepath.Join(dst, "u")) - assert.True(t, os.IsNotExist(statErr)) - }) - } -} - -func TestExtractRejectsHardLinkThroughArchivedSymlinkChain(t *testing.T) { - parent := t.TempDir() - assert.NoError(t, os.WriteFile(filepath.Join(parent, "secret"), []byte("secret"), 0o600)) - dst := filepath.Join(parent, "dst") - payload := compressedTar(t, func(tw *tar.Writer) { - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "a", Typeflag: tar.TypeDir, Mode: 0o755})) - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "a/b", Typeflag: tar.TypeSymlink, Linkname: ".."})) - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "s", Typeflag: tar.TypeSymlink, Linkname: "a/b"})) - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "u", Typeflag: tar.TypeSymlink, Linkname: "s/.."})) - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "stolen", Typeflag: tar.TypeLink, Linkname: "u/secret"})) - }) - - err := client.Extract(t.Context(), bytes.NewReader(payload), dst, 0) - assert.Error(t, err) - _, statErr := os.Lstat(filepath.Join(dst, "stolen")) - assert.True(t, os.IsNotExist(statErr)) -} - -func TestExtractAllowsDanglingAndLoopingSymlinks(t *testing.T) { - dst := t.TempDir() - payload := compressedTar(t, func(tw *tar.Writer) { - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "dangling", Typeflag: tar.TypeSymlink, Linkname: "missing"})) - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "sub/up", Typeflag: tar.TypeSymlink, Linkname: "./../dangling"})) - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "loop1", Typeflag: tar.TypeSymlink, Linkname: "loop2"})) - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "loop2", Typeflag: tar.TypeSymlink, Linkname: "loop1"})) - }) - - assert.NoError(t, client.Extract(t.Context(), bytes.NewReader(payload), dst, 0)) - target, err := os.Readlink(filepath.Join(dst, "dangling")) - assert.NoError(t, err) - assert.Equal(t, "missing", target) -} - -func TestExtractRestoresModTimes(t *testing.T) { - dst := t.TempDir() - modTime := time.Date(2020, 1, 2, 3, 4, 5, 0, time.UTC) - payload := compressedTar(t, func(tw *tar.Writer) { - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "dir", Typeflag: tar.TypeDir, Mode: 0o555, ModTime: modTime})) - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "dir/file", Mode: 0o644, Size: int64(len("ok")), ModTime: modTime})) - _, err := tw.Write([]byte("ok")) - assert.NoError(t, err) - }) - - assert.NoError(t, client.Extract(t.Context(), bytes.NewReader(payload), dst, 0)) - t.Cleanup(func() { os.Chmod(filepath.Join(dst, "dir"), 0o755) }) //nolint:errcheck,gosec - fileInfo, err := os.Stat(filepath.Join(dst, "dir", "file")) - assert.NoError(t, err) - assert.True(t, fileInfo.ModTime().Equal(modTime), "file mtime %s", fileInfo.ModTime()) - dirInfo, err := os.Stat(filepath.Join(dst, "dir")) - assert.NoError(t, err) - assert.True(t, dirInfo.ModTime().Equal(modTime), "dir mtime %s", dirInfo.ModTime()) -} - -func TestExtractConsumesTrailingPadding(t *testing.T) { - dst := t.TempDir() - var tarBuf bytes.Buffer - tw := tar.NewWriter(&tarBuf) - assert.NoError(t, tw.WriteHeader(&tar.Header{Name: "file", Mode: 0o600, Size: int64(len("ok"))})) - _, err := tw.Write([]byte("ok")) - assert.NoError(t, err) - assert.NoError(t, tw.Close()) - // Larger than a pipe buffer, so zstd is still writing when the tar reader hits end-of-archive. - tarBuf.Write(make([]byte, 1<<20)) - - assert.NoError(t, client.Extract(t.Context(), bytes.NewReader(zstdCompress(t, tarBuf.Bytes())), dst, 0)) - content, err := os.ReadFile(filepath.Join(dst, "file")) - assert.NoError(t, err) - assert.Equal(t, "ok", string(content)) -} - -func compressedTar(t *testing.T, writeEntries func(*tar.Writer)) []byte { - t.Helper() - var tarBuf bytes.Buffer - tw := tar.NewWriter(&tarBuf) - writeEntries(tw) - assert.NoError(t, tw.Close()) - return zstdCompress(t, tarBuf.Bytes()) -} - -func zstdCompress(t *testing.T, data []byte) []byte { - t.Helper() - cmd := exec.Command("zstd", "-q", "-c") - cmd.Stdin = bytes.NewReader(data) - var out, stderr bytes.Buffer - cmd.Stdout = &out - cmd.Stderr = &stderr - assert.NoError(t, cmd.Run(), stderr.String()) - return out.Bytes() -} diff --git a/client/client_test.go b/client/client_test.go index 82925595..cf53725a 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -406,6 +406,28 @@ func TestArchiveExtract(t *testing.T) { assert.False(t, slices.Contains(names, "y.log")) } +func TestArchiveExtractPreservesOutOfTreeSymlinks(t *testing.T) { + src := t.TempDir() + links := map[string]string{"abs": "/usr/bin/env", "rel": "../outside"} + for name, target := range links { + assert.NoError(t, os.Symlink(target, filepath.Join(src, name))) + } + + var buf bytes.Buffer + assert.NoError(t, client.Archive(t.Context(), &buf, src, []string{"."}, nil, 0)) + + dst := filepath.Join(t.TempDir(), "out") + assert.NoError(t, client.Extract(t.Context(), &buf, dst, 0)) + + got := map[string]string{} + for name := range links { + target, err := os.Readlink(filepath.Join(dst, name)) + assert.NoError(t, err) + got[name] = target + } + assert.Equal(t, links, got) +} + func TestExtractOverReadOnlyTree(t *testing.T) { src := t.TempDir() assert.NoError(t, os.MkdirAll(filepath.Join(src, "pkg", "conf"), 0o755))