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))