diff --git a/cli/cookbook/extract.go b/cli/cookbook/extract.go index 03db84b..04e1235 100644 --- a/cli/cookbook/extract.go +++ b/cli/cookbook/extract.go @@ -31,8 +31,16 @@ var ( // archive creates (Supermarket tarballs are rooted at /...). // // Entries whose paths escape destDir are refused, so a hostile tarball -// can't write outside the destination. +// can't write outside the destination. Every write goes through an os.Root +// handle on destDir, so the kernel refuses an escape even when the lexical +// check cannot see one (a symlink already sitting in destDir, say). func ExtractArchive(r io.Reader, destDir string) (string, error) { + destRoot, err := os.OpenRoot(destDir) + if err != nil { + return "", fmt.Errorf("open destination %s: %w", destDir, err) + } + defer func() { _ = destRoot.Close() }() + gz, err := gzip.NewReader(r) if err != nil { return "", fmt.Errorf("open gzip: %w", err) @@ -51,7 +59,7 @@ func ExtractArchive(r io.Reader, destDir string) (string, error) { return "", fmt.Errorf("read tar: %w", err) } - target, err := safeJoin(destDir, hdr.Name) + rel, err := safeRel(destDir, hdr.Name) if err != nil { return "", err } @@ -61,11 +69,11 @@ func ExtractArchive(r io.Reader, destDir string) (string, error) { switch hdr.Typeflag { case tar.TypeDir: - if err := os.MkdirAll(target, extractDirMode); err != nil { - return "", fmt.Errorf("mkdir %s: %w", target, err) + if err := destRoot.MkdirAll(rel, extractDirMode); err != nil { + return "", fmt.Errorf("mkdir %s: %w", rel, err) } case tar.TypeReg: - if err := writeFile(tr, target, hdr.Name, &total); err != nil { + if err := writeFile(destRoot, tr, rel, hdr.Name, &total); err != nil { return "", err } default: @@ -83,15 +91,18 @@ func ExtractArchive(r io.Reader, destDir string) (string, error) { return filepath.Join(destDir, root), nil } -// safeJoin joins name onto destDir and verifies the result stays within -// destDir, guarding against path-traversal ("zip slip") entries. -func safeJoin(destDir, name string) (string, error) { +// safeRel resolves a tar entry name to a path relative to destDir, refusing +// entries that would escape it ("zip slip"). The result is what the os.Root +// handle is asked to create, so containment is checked twice: lexically here, +// for a clear error naming the offending entry, and again by the kernel when +// the file is actually opened. +func safeRel(destDir, name string) (string, error) { target := filepath.Join(destDir, filepath.FromSlash(name)) rel, err := filepath.Rel(destDir, target) if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { return "", fmt.Errorf("archive entry %q escapes the destination directory", name) } - return target, nil + return rel, nil } // topLevel returns the first real path segment of a tar entry name, or "" @@ -107,23 +118,25 @@ func topLevel(name string) string { return root } -// writeFile creates target (with parent directories) and copies the +// writeFile creates rel (with parent directories) under root and copies the // current tar entry into it, capping output so a zip bomb can't fill the // disk. total accumulates across every entry in one archive. -func writeFile(r io.Reader, target, name string, total *int64) error { - if err := os.MkdirAll(filepath.Dir(target), extractDirMode); err != nil { - return fmt.Errorf("mkdir %s: %w", filepath.Dir(target), err) +func writeFile(root *os.Root, r io.Reader, rel, name string, total *int64) error { + if dir := filepath.Dir(rel); dir != "." { + if err := root.MkdirAll(dir, extractDirMode); err != nil { + return fmt.Errorf("mkdir %s: %w", dir, err) + } } - f, err := os.OpenFile(target, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, extractFileMode) + f, err := root.OpenFile(rel, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, extractFileMode) if err != nil { - return fmt.Errorf("create %s: %w", target, err) + return fmt.Errorf("create %s: %w", rel, err) } if err := boundedCopy(f, r, name, total); err != nil { _ = f.Close() // already returning an error - return fmt.Errorf("write %s: %w", target, err) + return fmt.Errorf("write %s: %w", rel, err) } if err := f.Close(); err != nil { - return fmt.Errorf("close %s: %w", target, err) + return fmt.Errorf("close %s: %w", rel, err) } return nil } diff --git a/cli/cookbook/extract_test.go b/cli/cookbook/extract_test.go index 7b643f2..95d0be7 100644 --- a/cli/cookbook/extract_test.go +++ b/cli/cookbook/extract_test.go @@ -195,3 +195,25 @@ func TestExtractArchiveClampsFileMode(t *testing.T) { t.Errorf("extracted file mode = %o, want %o", got, extractFileMode) } } + +// TestExtractArchiveDoesNotFollowSymlinkOutOfDest covers the hole a purely +// lexical containment check leaves open. "nginx/metadata.rb" is inside destDir +// by every string comparison, so safeJoin passes it; if destDir already holds +// a "nginx" symlink pointing elsewhere, the create still lands on the far side +// of that link. Containment has to be enforced when the file is opened, not +// when its name is computed. +func TestExtractArchiveDoesNotFollowSymlinkOutOfDest(t *testing.T) { + dest := t.TempDir() + outside := t.TempDir() + if err := os.Symlink(outside, filepath.Join(dest, "nginx")); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + archive := buildCookbookTarball(t, map[string]string{"nginx/metadata.rb": "pwned"}) + if _, err := ExtractArchive(bytes.NewReader(archive), dest); err == nil { + t.Error("ExtractArchive wrote through a symlink in destDir without complaint") + } + if _, err := os.Stat(filepath.Join(outside, "metadata.rb")); err == nil { + t.Fatal("archive entry escaped destDir through a pre-existing symlink") + } +} diff --git a/cli/policyfile/extract.go b/cli/policyfile/extract.go index 62622a2..be0a645 100644 --- a/cli/policyfile/extract.go +++ b/cli/policyfile/extract.go @@ -58,8 +58,16 @@ func boundedCopy(dst io.Writer, src io.Reader, name string, total *int64) error // entries under dest, stripping the single leading path segment that // Supermarket tarballs wrap a cookbook in (e.g. "nginx/metadata.rb" lands at // dest/metadata.rb), so dest ends up holding the cookbook root directly. Paths -// that would escape dest are rejected. +// that would escape dest are rejected, and every write goes through an os.Root +// handle so the kernel refuses an escape the lexical check cannot see (a +// symlink already sitting in dest, say). func extractCookbookTarball(r io.Reader, dest string) error { + root, err := os.OpenRoot(dest) + if err != nil { + return fmt.Errorf("supermarket: open destination %s: %w", dest, err) + } + defer func() { _ = root.Close() }() + gz, err := gzip.NewReader(r) if err != nil { return fmt.Errorf("supermarket: open gzip: %w", err) @@ -80,20 +88,22 @@ func extractCookbookTarball(r io.Reader, dest string) error { if rel == "" { continue } - target := filepath.Join(dest, rel) - if !withinDir(dest, target) { + rel = filepath.FromSlash(rel) + if relEscapes(rel) { return fmt.Errorf("supermarket: unsafe path in tarball: %q", hdr.Name) } switch hdr.Typeflag { case tar.TypeDir: - if err := os.MkdirAll(target, extractDirMode); err != nil { + if err := root.MkdirAll(rel, extractDirMode); err != nil { return err } case tar.TypeReg: - if err := os.MkdirAll(filepath.Dir(target), extractDirMode); err != nil { - return err + if dir := filepath.Dir(rel); dir != "." { + if err := root.MkdirAll(dir, extractDirMode); err != nil { + return err + } } - f, err := os.OpenFile(target, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, extractFileMode) + f, err := root.OpenFile(rel, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, extractFileMode) if err != nil { return err } @@ -119,6 +129,22 @@ func stripLeadingSegment(name string) string { return strings.Trim(name[idx+1:], "/") } +// relEscapes reports whether a relative archive path would resolve outside +// the directory it is extracted into. The entry name is already relative by +// this point, so it is checked directly rather than joined onto the +// destination and relativized straight back off it. +// +// This is a lexical check and is not what enforces containment: it rejects a +// bad entry early, with an error naming it, while the os.Root handle refuses +// the escape at the syscall when the entry is created. +func relEscapes(rel string) bool { + if filepath.IsAbs(rel) { + return true + } + clean := filepath.Clean(rel) + return clean == ".." || strings.HasPrefix(clean, ".."+string(filepath.Separator)) +} + // withinDir reports whether target stays inside dir (no "../" escape). func withinDir(dir, target string) bool { rel, err := filepath.Rel(dir, target) diff --git a/cli/policyfile/extract_test.go b/cli/policyfile/extract_test.go new file mode 100644 index 0000000..8c0e25f --- /dev/null +++ b/cli/policyfile/extract_test.go @@ -0,0 +1,92 @@ +package policyfile + +import ( + "archive/tar" + "bytes" + "compress/gzip" + "os" + "path/filepath" + "testing" +) + +// buildTarball gzips a tarball from the given entries (name -> body). +func buildTarball(t *testing.T, files map[string]string) []byte { + t.Helper() + var buf bytes.Buffer + gz := gzip.NewWriter(&buf) + tw := tar.NewWriter(gz) + for name, body := range files { + hdr := &tar.Header{Name: name, Mode: 0o644, Typeflag: tar.TypeReg, Size: int64(len(body))} + if err := tw.WriteHeader(hdr); err != nil { + t.Fatal(err) + } + if _, err := tw.Write([]byte(body)); err != nil { + t.Fatal(err) + } + } + if err := tw.Close(); err != nil { + t.Fatal(err) + } + if err := gz.Close(); err != nil { + t.Fatal(err) + } + return buf.Bytes() +} + +// TestExtractCookbookTarballRejectsTraversal pins the plain "../" case. +func TestExtractCookbookTarballRejectsTraversal(t *testing.T) { + dest := t.TempDir() + archive := buildTarball(t, map[string]string{"nginx/../../escape.txt": "pwned"}) + if err := extractCookbookTarball(bytes.NewReader(archive), dest); err == nil { + t.Error("expected a traversal entry to be refused") + } + if _, err := os.Stat(filepath.Join(filepath.Dir(dest), "escape.txt")); err == nil { + t.Fatal("traversal entry escaped dest") + } +} + +// TestExtractCookbookTarballDoesNotFollowSymlinkOutOfDest covers what a +// lexical check cannot see. After the leading cookbook segment is stripped, +// "nginx/cache/evil.rb" resolves to dest/cache/evil.rb, which is inside dest +// by string comparison; if dest already holds a "cache" symlink the write +// still lands outside it. +func TestExtractCookbookTarballDoesNotFollowSymlinkOutOfDest(t *testing.T) { + dest := t.TempDir() + outside := t.TempDir() + if err := os.Symlink(outside, filepath.Join(dest, "cache")); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + archive := buildTarball(t, map[string]string{"nginx/cache/evil.rb": "pwned"}) + if err := extractCookbookTarball(bytes.NewReader(archive), dest); err == nil { + t.Error("extractCookbookTarball wrote through a symlink in dest without complaint") + } + if _, err := os.Stat(filepath.Join(outside, "evil.rb")); err == nil { + t.Fatal("archive entry escaped dest through a pre-existing symlink") + } +} + +// TestRelEscapes pins the lexical rules the extractor rejects entries on. +// "a/../b" normalizes back inside the destination and is fine; anything that +// climbs above it, or arrives absolute, is not. +func TestRelEscapes(t *testing.T) { + cases := []struct { + rel string + want bool + }{ + {"metadata.rb", false}, + {"recipes/default.rb", false}, + {"a/../b", false}, + {".", false}, + {"..", true}, + {"../escape.txt", true}, + {"a/../../escape.txt", true}, + // os.TempDir is absolute on every platform, unlike a hardcoded "/etc". + {filepath.Join(os.TempDir(), "escape.txt"), true}, + } + for _, tc := range cases { + if got := relEscapes(tc.rel); got != tc.want { + t.Errorf("relEscapes(%q) = %v, want %v", tc.rel, got, tc.want) + } + } +} diff --git a/cli/policyfile/rubyeval/loader.go b/cli/policyfile/rubyeval/loader.go index 3b35a82..6b8c689 100644 --- a/cli/policyfile/rubyeval/loader.go +++ b/cli/policyfile/rubyeval/loader.go @@ -204,8 +204,16 @@ func verifyFileSHA256(path, wantHex string) error { } // extractTarGz unpacks a .tar.gz archive under dest. It guards against path -// traversal (a "../" entry escaping dest is rejected). +// traversal (a "../" entry escaping dest is rejected) and writes through an +// os.Root handle, so the kernel refuses an escape the lexical check cannot +// see (a symlink already sitting in dest, say). func extractTarGz(archive []byte, dest string) error { + root, err := os.OpenRoot(dest) + if err != nil { + return fmt.Errorf("policyfile: open extraction dir %s: %w", dest, err) + } + defer func() { _ = root.Close() }() + gz, err := gzip.NewReader(bytes.NewReader(archive)) if err != nil { return err @@ -224,16 +232,22 @@ func extractTarGz(archive []byte, dest string) error { if !withinDir(dest, target) { return fmt.Errorf("policyfile: archive entry %q escapes extraction dir", hdr.Name) } + rel, err := filepath.Rel(dest, target) + if err != nil { + return fmt.Errorf("policyfile: archive entry %q escapes extraction dir", hdr.Name) + } switch hdr.Typeflag { case tar.TypeDir: - if err := os.MkdirAll(target, 0o755); err != nil { + if err := root.MkdirAll(rel, 0o755); err != nil { return err } case tar.TypeReg: - if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { - return err + if dir := filepath.Dir(rel); dir != "." { + if err := root.MkdirAll(dir, 0o755); err != nil { + return err + } } - f, err := os.OpenFile(target, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, os.FileMode(hdr.Mode)&0o777) + f, err := root.OpenFile(rel, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, os.FileMode(hdr.Mode)&0o777) if err != nil { return err } diff --git a/cli/policyfile/rubyeval/loader_test.go b/cli/policyfile/rubyeval/loader_test.go index 0f36690..d44df1d 100644 --- a/cli/policyfile/rubyeval/loader_test.go +++ b/cli/policyfile/rubyeval/loader_test.go @@ -169,3 +169,22 @@ func makeTarGz(t *testing.T, files map[string]string) []byte { } return buf.Bytes() } + +// TestExtractTarGzDoesNotFollowSymlinkOutOfDest covers the case the lexical +// check misses: "usr/evil" stays inside dest by string comparison, so a "usr" +// symlink already in dest redirects the write outside it. +func TestExtractTarGzDoesNotFollowSymlinkOutOfDest(t *testing.T) { + dir := t.TempDir() + outside := t.TempDir() + if err := os.Symlink(outside, filepath.Join(dir, "usr")); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + archive := makeTarGz(t, map[string]string{"usr/evil": "pwned"}) + if err := extractTarGz(archive, dir); err == nil { + t.Error("extractTarGz wrote through a symlink in dest without complaint") + } + if _, err := os.Stat(filepath.Join(outside, "evil")); err == nil { + t.Fatal("archive entry escaped dest through a pre-existing symlink") + } +}