From fa6ebe092ddd907eb676b0b44b9e0b4a24e9ffe3 Mon Sep 17 00:00:00 2001 From: Andrew Nesbitt Date: Mon, 3 Aug 2026 16:08:54 +0100 Subject: [PATCH 1/2] Add ExtractAll with os.Root confinement ExtractAll(r Reader, dir string) writes every entry under dir using os.OpenRoot so no operation can escape the target through a symlink. Entry names are validated with filepath.Localize and traversal attempts return ErrUnsafePath naming the offending entry. Existing objects at a destination path are removed before an O_EXCL create so in-root symlinks and hard links are replaced rather than followed or modified in place. File permissions are applied via fchmod on the open descriptor when the archive recorded a mode; directory permissions are applied deepest-first after all entries are written. FileInfo gains HasMode to distinguish a recorded mode from a synthesised or absent one; zip entries set it only when the creator system is Unix or macOS and a Unix st_mode is present. tarReader now derives Mode from header.FileInfo().Mode() so symlink and device typeflags surface as fs.ModeType bits, and marks TypeLink entries irregular. Both readers build a path index at open time so Extract is a map lookup and ExtractAll is linear in the entry count. Closes #22 --- README.md | 15 ++ archives.go | 1 + extract.go | 166 +++++++++++++++++ extract_test.go | 465 ++++++++++++++++++++++++++++++++++++++++++++++++ tar.go | 39 ++-- zip.go | 50 ++++-- 6 files changed, 716 insertions(+), 20 deletions(-) create mode 100644 extract.go create mode 100644 extract_test.go diff --git a/README.md b/README.md index ecc0b1d..6ddf134 100644 --- a/README.md +++ b/README.md @@ -81,6 +81,21 @@ reader, _ := archives.OpenWithPrefix("pkg.tgz", f, "package/") // files are now accessible without the package/ prefix ``` +### Extracting to disk + +`ExtractAll` writes every entry under a target directory, creating it and any intermediate directories. Entry names are validated with `filepath.Localize` so absolute paths and `..` segments that would escape the target return `ErrUnsafePath` naming the offending entry. + +```go +reader, _ := archives.Open("pkg.whl", f) +defer reader.Close() + +if err := archives.ExtractAll(reader, dir); err != nil { + return err +} +``` + +File permissions are preserved where the archive records them. Entries that the format marks as symlinks or other non-regular types are skipped. + ### Comparing versions The `diff` subpackage compares two archives and produces unified diffs. It classifies each file as added, deleted, modified, or binary, and includes line-level diff output for text files. diff --git a/archives.go b/archives.go index 6b10360..e5675ce 100644 --- a/archives.go +++ b/archives.go @@ -39,6 +39,7 @@ type FileInfo struct { ModTime time.Time // Modification time IsDir bool // Whether this is a directory Mode uint32 // File mode/permissions + HasMode bool // Whether Mode was recorded by the archive CompressedSize int64 // Compressed size (if available) } diff --git a/extract.go b/extract.go new file mode 100644 index 0000000..6b52557 --- /dev/null +++ b/extract.go @@ -0,0 +1,166 @@ +package archives + +import ( + "errors" + "fmt" + "io" + "io/fs" + "os" + "path" + "path/filepath" + "sort" + "strings" +) + +// ErrUnsafePath is returned by ExtractAll when an archive entry name would +// resolve outside the target directory. +var ErrUnsafePath = errors.New("archive entry escapes target directory") + +const ( + extractDirPerm = 0o755 + extractFilePerm = 0o644 +) + +type deferredChmod struct { + path string + perm fs.FileMode +} + +// ExtractAll writes every entry in r under dir, creating dir and any +// intermediate directories as needed. Entry permissions are preserved where +// the archive format records them; entries with no stored mode are written +// as 0644 (files) or 0755 (directories). +// +// All filesystem operations are confined to dir using os.Root, so a symlink +// under dir cannot redirect a write outside it. Entry names are additionally +// validated with filepath.Localize: absolute paths, names containing ".." +// elements that escape dir, and platform-invalid names cause ExtractAll to +// return ErrUnsafePath wrapping the offending entry name. Entries that the +// archive marks as non-regular (symlinks, devices) are skipped. +func ExtractAll(r Reader, dir string) error { + if err := os.MkdirAll(dir, extractDirPerm); err != nil { + return err + } + root, err := os.OpenRoot(dir) + if err != nil { + return err + } + defer func() { _ = root.Close() }() + + entries, err := r.List() + if err != nil { + return err + } + + var dirModes []deferredChmod + for _, entry := range entries { + dm, err := extractEntry(r, root, entry) + if err != nil { + return err + } + if dm != nil { + dirModes = append(dirModes, *dm) + } + } + + return applyDirModes(root, dirModes) +} + +func extractEntry(r Reader, root *os.Root, entry FileInfo) (*deferredChmod, error) { + name := path.Clean(strings.TrimSuffix(entry.Path, "/")) + if name == "." || name == "" { + return nil, nil + } + + local, err := filepath.Localize(name) + if err != nil { + return nil, fmt.Errorf("%w: %q", ErrUnsafePath, entry.Path) + } + + if entry.IsDir { + if err := root.MkdirAll(local, extractDirPerm); err != nil { + return nil, err + } + if entry.HasMode { + return &deferredChmod{path: local, perm: fs.FileMode(entry.Mode).Perm()}, nil + } + return nil, nil + } + + if fs.FileMode(entry.Mode)&fs.ModeType != 0 { + // Symlink, device, or other non-regular entry. Skip rather than + // error so archives containing incidental symlinks still extract. + return nil, nil + } + + if parent := filepath.Dir(local); parent != "." { + if err := root.MkdirAll(parent, extractDirPerm); err != nil { + return nil, err + } + } + + src, err := r.Extract(entry.Path) + if err != nil { + return nil, err + } + defer func() { _ = src.Close() }() + + perm := fs.FileMode(extractFilePerm) + if entry.HasMode { + perm = fs.FileMode(entry.Mode).Perm() + } + // Remove any existing object at the destination so an in-root symlink, + // a hard link to another inode, or a leftover file with different + // permissions is replaced with a fresh regular file rather than + // modified in place. + if err := root.Remove(local); err != nil && !errors.Is(err, fs.ErrNotExist) { + return nil, err + } + out, err := root.OpenFile(local, os.O_CREATE|os.O_EXCL|os.O_WRONLY, perm) + if err != nil { + return nil, err + } + if _, err := io.Copy(out, src); err != nil { + _ = out.Close() + return nil, fmt.Errorf("writing %s: %w", entry.Path, err) + } + if entry.HasMode { + // The mode passed to OpenFile is subject to the process umask; + // restore the recorded permissions on the open descriptor so no + // path lookup is involved. + if err := out.Chmod(perm); err != nil { + _ = out.Close() + return nil, err + } + } + return nil, out.Close() +} + +// applyDirModes sets recorded directory permissions after all entries have +// been written, deepest first, so restrictive parent modes cannot block +// chmod of their children. Each directory is opened through the root and +// chmod applied to the open descriptor to avoid the documented Root.Chmod +// symlink-replacement race. +func applyDirModes(root *os.Root, modes []deferredChmod) error { + sort.Slice(modes, func(i, j int) bool { + return depth(modes[i].path) > depth(modes[j].path) + }) + for _, m := range modes { + d, err := root.Open(m.path) + if err != nil { + return err + } + if err := d.Chmod(m.perm); err != nil { + _ = d.Close() + return err + } + if err := d.Close(); err != nil { + return err + } + } + return nil +} + +func depth(p string) int { + return strings.Count(p, string(filepath.Separator)) +} diff --git a/extract_test.go b/extract_test.go new file mode 100644 index 0000000..741ec2e --- /dev/null +++ b/extract_test.go @@ -0,0 +1,465 @@ +package archives + +import ( + "archive/tar" + "archive/zip" + "bytes" + "errors" + "io/fs" + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +func TestExtractAllZip(t *testing.T) { + reader, err := OpenBytes("test.zip", createTestZip()) + if err != nil { + t.Fatal(err) + } + defer func() { _ = reader.Close() }() + + dir := t.TempDir() + if err := ExtractAll(reader, dir); err != nil { + t.Fatal(err) + } + + assertFileContent(t, filepath.Join(dir, "README.md"), "# Test Package") + assertFileContent(t, filepath.Join(dir, "src", "main.go"), "package main") + assertFileContent(t, filepath.Join(dir, "src", "util", "helper.go"), "package util") + assertFileContent(t, filepath.Join(dir, "docs", "guide.md"), "# Guide") +} + +func TestExtractAllTarGz(t *testing.T) { + reader, err := OpenBytes("test.tgz", createTestTarGz()) + if err != nil { + t.Fatal(err) + } + defer func() { _ = reader.Close() }() + + dir := t.TempDir() + if err := ExtractAll(reader, dir); err != nil { + t.Fatal(err) + } + + assertFileContent(t, filepath.Join(dir, "package.json"), `{"name": "test"}`) + assertFileContent(t, filepath.Join(dir, "lib", "util.js"), "module.exports = {};") +} + +func TestExtractAllCreatesTargetDir(t *testing.T) { + reader, err := OpenBytes("test.zip", createTestZip()) + if err != nil { + t.Fatal(err) + } + defer func() { _ = reader.Close() }() + + dir := filepath.Join(t.TempDir(), "sub", "target") + if err := ExtractAll(reader, dir); err != nil { + t.Fatal(err) + } + assertFileContent(t, filepath.Join(dir, "README.md"), "# Test Package") +} + +func TestExtractAllPreservesTarMode(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("file modes are not preserved on windows") + } + + buf := new(bytes.Buffer) + tw := tar.NewWriter(buf) + if err := tw.WriteHeader(&tar.Header{Name: "conf/", Typeflag: tar.TypeDir, Mode: 0o550}); err != nil { + t.Fatal(err) + } + writeTarFile(t, tw, "conf/settings", "x", 0o640) + writeTarFile(t, tw, "bin/tool", "#!/bin/sh\n", 0o755) + writeTarFile(t, tw, "data.txt", "hello", 0o600) + _ = tw.Close() + + reader, err := OpenBytes("test.tar", buf.Bytes()) + if err != nil { + t.Fatal(err) + } + defer func() { _ = reader.Close() }() + + dir := t.TempDir() + if err := ExtractAll(reader, dir); err != nil { + t.Fatal(err) + } + + assertFileMode(t, filepath.Join(dir, "bin", "tool"), 0o755) + assertFileMode(t, filepath.Join(dir, "data.txt"), 0o600) + assertFileMode(t, filepath.Join(dir, "conf", "settings"), 0o640) + // Directory mode is applied after its contents are written and is not + // widened for owner access. + assertFileMode(t, filepath.Join(dir, "conf"), 0o550) + + // Restore write so t.TempDir cleanup can remove it. + _ = os.Chmod(filepath.Join(dir, "conf"), 0o750) +} + +func TestExtractAllPreservesZeroMode(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("file modes are not preserved on windows") + } + + buf := new(bytes.Buffer) + tw := tar.NewWriter(buf) + writeTarFile(t, tw, "locked", "x", 0o000) + _ = tw.Close() + + reader, err := OpenBytes("test.tar", buf.Bytes()) + if err != nil { + t.Fatal(err) + } + defer func() { _ = reader.Close() }() + + dir := t.TempDir() + if err := ExtractAll(reader, dir); err != nil { + t.Fatal(err) + } + assertFileMode(t, filepath.Join(dir, "locked"), 0o000) +} + +func TestExtractAllZipWithoutUnixModeUsesDefault(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("file modes are not preserved on windows") + } + + // createTestZip writes entries via zip.Writer.Create, which does not + // set ExternalAttrs, so no Unix mode is recorded. + reader, err := OpenBytes("test.zip", createTestZip()) + if err != nil { + t.Fatal(err) + } + defer func() { _ = reader.Close() }() + + dir := t.TempDir() + if err := ExtractAll(reader, dir); err != nil { + t.Fatal(err) + } + // The exact mode depends on the process umask; the check is that the + // synthesised 0666 was not applied via Chmod. + info, err := os.Stat(filepath.Join(dir, "README.md")) + if err != nil { + t.Fatal(err) + } + if perm := info.Mode().Perm(); perm&0o022 != 0 { + t.Fatalf("file mode = %o, want group/other write cleared", perm) + } +} + +func TestZipHasUnixMode(t *testing.T) { + tests := []struct { + name string + creator uint16 + attrs uint32 + want bool + }{ + {"Unix with mode", zipCreatorUnix, 0o100644 << zipUnixModeShift, true}, + {"macOS with mode", zipCreatorMacOSX, 0o100644 << zipUnixModeShift, true}, + {"Unix no mode", zipCreatorUnix, 0, false}, + {"FAT with high bits", 0, 0xffff0020, false}, + {"NTFS with high bits", 11, 0x00010020, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + h := &zip.FileHeader{ + CreatorVersion: tt.creator << zipCreatorShift, + ExternalAttrs: tt.attrs, + } + if got := zipHasUnixMode(h); got != tt.want { + t.Fatalf("zipHasUnixMode(creator=%d, attrs=%#x) = %v, want %v", + tt.creator, tt.attrs, got, tt.want) + } + }) + } +} + +func TestExtractAllRejectsTraversal(t *testing.T) { + tests := []struct { + name string + entry string + }{ + {"parent", "../escape.txt"}, + {"nested parent", "sub/../../escape.txt"}, + {"absolute", "/etc/passwd"}, + } + if runtime.GOOS == "windows" { + tests = append(tests, + struct{ name, entry string }{"drive", `C:\escape.txt`}, + struct{ name, entry string }{"backslash parent", `..\escape.txt`}, + ) + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reader, err := OpenBytes("test.tar", createTarWithEntry(t, tt.entry)) + if err != nil { + t.Fatal(err) + } + defer func() { _ = reader.Close() }() + + dir := t.TempDir() + err = ExtractAll(reader, dir) + if !errors.Is(err, ErrUnsafePath) { + t.Fatalf("ExtractAll(%q) error = %v, want ErrUnsafePath", tt.entry, err) + } + if !strings.Contains(err.Error(), tt.entry) { + t.Fatalf("error %q does not name offending entry %q", err, tt.entry) + } + assertNoEscape(t, dir) + }) + } +} + +func TestExtractAllAcceptsBenignDotSegments(t *testing.T) { + tests := []struct { + entry string + want string + }{ + {"./file.txt", "file.txt"}, + {"sub/./file.txt", filepath.Join("sub", "file.txt")}, + {"sub/inner/../file.txt", filepath.Join("sub", "file.txt")}, + } + + for _, tt := range tests { + t.Run(tt.entry, func(t *testing.T) { + reader, err := OpenBytes("test.tar", createTarWithEntry(t, tt.entry)) + if err != nil { + t.Fatal(err) + } + defer func() { _ = reader.Close() }() + + dir := t.TempDir() + if err := ExtractAll(reader, dir); err != nil { + t.Fatalf("ExtractAll(%q) failed: %v", tt.entry, err) + } + assertFileContent(t, filepath.Join(dir, tt.want), "x") + }) + } +} + +func TestExtractAllConfinedByRoot(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("os.Symlink requires elevated privileges on windows") + } + + outside := t.TempDir() + dir := t.TempDir() + // Plant a symlink under the target that points outside it. os.Root must + // refuse to follow it when writing sub/file. + if err := os.Symlink(outside, filepath.Join(dir, "sub")); err != nil { + t.Fatal(err) + } + + reader, err := OpenBytes("test.tar", createTarWithEntry(t, "sub/file")) + if err != nil { + t.Fatal(err) + } + defer func() { _ = reader.Close() }() + + if err := ExtractAll(reader, dir); err == nil { + t.Fatal("write through escaping symlink succeeded") + } + + entries, _ := os.ReadDir(outside) + if len(entries) != 0 { + t.Fatalf("write escaped to %s: %v", outside, entries) + } +} + +func TestExtractAllReplacesExistingObjects(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("os.Symlink requires elevated privileges on windows") + } + + dir := t.TempDir() + target := filepath.Join(dir, "target") + if err := os.WriteFile(target, []byte("original"), 0o755); err != nil { + t.Fatal(err) + } + // A pre-existing in-root symlink at the destination is replaced with a + // fresh regular file rather than followed. + if err := os.Symlink("target", filepath.Join(dir, "package.json")); err != nil { + t.Fatal(err) + } + // A pre-existing hard link at the destination is unlinked; the sibling + // name keeps the original inode content. + if err := os.Link(target, filepath.Join(dir, "index.js")); err != nil { + t.Fatal(err) + } + + reader, err := OpenBytes("test.tar", createTestTar()) + if err != nil { + t.Fatal(err) + } + defer func() { _ = reader.Close() }() + + if err := ExtractAll(reader, dir); err != nil { + t.Fatal(err) + } + + info, err := os.Lstat(filepath.Join(dir, "package.json")) + if err != nil { + t.Fatal(err) + } + if info.Mode()&fs.ModeSymlink != 0 { + t.Fatal("destination symlink was followed, not replaced") + } + assertFileContent(t, filepath.Join(dir, "package.json"), `{"name": "test"}`) + assertFileContent(t, filepath.Join(dir, "index.js"), "console.log('hello');") + // Original inode behind the hard link was not modified. + assertFileContent(t, target, "original") +} + +func TestExtractAllSkipsTarSpecialEntries(t *testing.T) { + buf := new(bytes.Buffer) + tw := tar.NewWriter(buf) + if err := tw.WriteHeader(&tar.Header{ + Name: "link", + Typeflag: tar.TypeSymlink, + Linkname: "../outside", + Mode: 0o777, + }); err != nil { + t.Fatal(err) + } + if err := tw.WriteHeader(&tar.Header{ + Name: "dev", + Typeflag: tar.TypeChar, + Mode: 0o644, + }); err != nil { + t.Fatal(err) + } + if err := tw.WriteHeader(&tar.Header{ + Name: "hardlink", + Typeflag: tar.TypeLink, + Linkname: "regular.txt", + Mode: 0o644, + }); err != nil { + t.Fatal(err) + } + writeTarFile(t, tw, "regular.txt", "ok", 0o644) + _ = tw.Close() + + reader, err := OpenBytes("test.tar", buf.Bytes()) + if err != nil { + t.Fatal(err) + } + defer func() { _ = reader.Close() }() + + dir := t.TempDir() + if err := ExtractAll(reader, dir); err != nil { + t.Fatal(err) + } + + for _, name := range []string{"link", "dev", "hardlink"} { + if _, err := os.Lstat(filepath.Join(dir, name)); !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("%s entry was written: %v", name, err) + } + } + assertFileContent(t, filepath.Join(dir, "regular.txt"), "ok") +} + +func TestExtractAllSkipsZipSymlink(t *testing.T) { + buf := new(bytes.Buffer) + zw := zip.NewWriter(buf) + hdr := &zip.FileHeader{Name: "link", Method: zip.Store} + hdr.SetMode(fs.ModeSymlink | 0o777) + w, _ := zw.CreateHeader(hdr) + _, _ = w.Write([]byte("../target")) + f, _ := zw.Create("regular.txt") + _, _ = f.Write([]byte("ok")) + _ = zw.Close() + + reader, err := OpenBytes("test.zip", buf.Bytes()) + if err != nil { + t.Fatal(err) + } + defer func() { _ = reader.Close() }() + + dir := t.TempDir() + if err := ExtractAll(reader, dir); err != nil { + t.Fatal(err) + } + + if _, err := os.Lstat(filepath.Join(dir, "link")); !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("symlink entry was written: %v", err) + } + assertFileContent(t, filepath.Join(dir, "regular.txt"), "ok") +} + +func TestExtractAllWithPrefix(t *testing.T) { + reader, err := OpenBytesWithPrefix("test.zip", createTestZip(), "src/") + if err != nil { + t.Fatal(err) + } + defer func() { _ = reader.Close() }() + + dir := t.TempDir() + if err := ExtractAll(reader, dir); err != nil { + t.Fatal(err) + } + assertFileContent(t, filepath.Join(dir, "main.go"), "package main") + assertFileContent(t, filepath.Join(dir, "util", "helper.go"), "package util") +} + +func createTarWithEntry(t *testing.T, name string) []byte { + t.Helper() + buf := new(bytes.Buffer) + tw := tar.NewWriter(buf) + writeTarFile(t, tw, name, "x", 0o644) + _ = tw.Close() + return buf.Bytes() +} + +func writeTarFile(t *testing.T, tw *tar.Writer, name, content string, mode int64) { + t.Helper() + err := tw.WriteHeader(&tar.Header{ + Name: name, + Size: int64(len(content)), + Mode: mode, + }) + if err != nil { + t.Fatal(err) + } + if _, err := tw.Write([]byte(content)); err != nil { + t.Fatal(err) + } +} + +func assertFileContent(t *testing.T, path, want string) { + t.Helper() + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading %s: %v", path, err) + } + if string(got) != want { + t.Fatalf("%s = %q, want %q", path, got, want) + } +} + +func assertFileMode(t *testing.T, path string, want fs.FileMode) { + t.Helper() + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat %s: %v", path, err) + } + if got := info.Mode().Perm(); got != want { + t.Fatalf("%s mode = %o, want %o", path, got, want) + } +} + +func assertNoEscape(t *testing.T, dir string) { + t.Helper() + parent := filepath.Dir(dir) + entries, err := os.ReadDir(parent) + if err != nil { + t.Fatal(err) + } + for _, e := range entries { + if e.Name() != filepath.Base(dir) { + t.Fatalf("file %q escaped into %s", e.Name(), parent) + } + } +} diff --git a/tar.go b/tar.go index cb83967..0358f8e 100644 --- a/tar.go +++ b/tar.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "io" + "io/fs" "strings" "github.com/ulikunitz/xz" @@ -20,6 +21,7 @@ var ErrDecompressLimit = errors.New("decompressed content exceeds size limit") type tarReader struct { raw []byte files []tarFileEntry + index map[string]int } type tarFileEntry struct { @@ -62,13 +64,22 @@ func openTar(raw []byte, compression string) (*tarReader, error) { return nil, fmt.Errorf("reading tar: %w", err) } + // FileInfo().Mode() combines header.Mode permission bits with type + // bits derived from Typeflag. It reports hard links as regular + // files, so mark them irregular explicitly since a hard-link entry + // carries no data of its own. + mode := header.FileInfo().Mode() + if header.Typeflag == tar.TypeLink { + mode |= fs.ModeIrregular + } info := FileInfo{ Path: header.Name, Name: extractName(header.Name), Size: header.Size, ModTime: header.ModTime, IsDir: header.Typeflag == tar.TypeDir, - Mode: uint32(header.Mode), + Mode: uint32(mode), + HasMode: true, } var data []byte @@ -90,7 +101,14 @@ func openTar(raw []byte, compression string) (*tarReader, error) { }) } - return &tarReader{raw: raw, files: files}, nil + index := make(map[string]int, len(files)) + for i, f := range files { + if _, seen := index[f.info.Path]; !seen { + index[f.info.Path] = i + } + } + + return &tarReader{raw: raw, files: files, index: index}, nil } func (t *tarReader) List() ([]FileInfo, error) { @@ -140,15 +158,15 @@ func (t *tarReader) ListDir(dirPath string) ([]FileInfo, error) { } func (t *tarReader) Extract(filePath string) (io.ReadCloser, error) { - for _, f := range t.files { - if f.info.Path == filePath { - if f.info.IsDir { - return nil, fmt.Errorf("path is a directory: %s", filePath) - } - return io.NopCloser(bytes.NewReader(f.data)), nil - } + i, ok := t.index[filePath] + if !ok { + return nil, fmt.Errorf("file not found: %s", filePath) + } + f := t.files[i] + if f.info.IsDir { + return nil, fmt.Errorf("path is a directory: %s", filePath) } - return nil, fmt.Errorf("file not found: %s", filePath) + return io.NopCloser(bytes.NewReader(f.data)), nil } func (t *tarReader) Hash(algo string) (string, error) { @@ -158,5 +176,6 @@ func (t *tarReader) Hash(algo string) (string, error) { func (t *tarReader) Close() error { t.raw = nil t.files = nil + t.index = nil return nil } diff --git a/zip.go b/zip.go index fd50185..025dd88 100644 --- a/zip.go +++ b/zip.go @@ -8,9 +8,23 @@ import ( "strings" ) +// zipUnixModeShift is the bit offset of the Unix st_mode field within a +// ZIP entry's external attributes word. Unix permissions are only present +// when the creator system in CreatorVersion is Unix or macOS; other creators +// may store unrelated data in the high word, and archive/zip.FileHeader.Mode +// then returns a synthesised 0666/0444 that callers should not treat as a +// stored mode. +const ( + zipUnixModeShift = 16 + zipCreatorShift = 8 + zipCreatorUnix = 3 + zipCreatorMacOSX = 19 +) + type zipReader struct { raw []byte reader *zip.Reader + index map[string]*zip.File } func openZip(raw []byte) (*zipReader, error) { @@ -19,9 +33,17 @@ func openZip(raw []byte) (*zipReader, error) { return nil, fmt.Errorf("opening zip: %w", err) } + index := make(map[string]*zip.File, len(reader.File)) + for _, f := range reader.File { + if _, seen := index[f.Name]; !seen { + index[f.Name] = f + } + } + return &zipReader{ raw: raw, reader: reader, + index: index, }, nil } @@ -77,17 +99,14 @@ func (z *zipReader) ListDir(dirPath string) ([]FileInfo, error) { } func (z *zipReader) Extract(filePath string) (io.ReadCloser, error) { - // Find the file - for _, f := range z.reader.File { - if f.Name == filePath { - if f.FileInfo().IsDir() { - return nil, fmt.Errorf("path is a directory: %s", filePath) - } - return f.Open() - } + f, ok := z.index[filePath] + if !ok { + return nil, fmt.Errorf("file not found: %s", filePath) } - - return nil, fmt.Errorf("file not found: %s", filePath) + if f.FileInfo().IsDir() { + return nil, fmt.Errorf("path is a directory: %s", filePath) + } + return f.Open() } func (z *zipReader) Hash(algo string) (string, error) { @@ -97,6 +116,7 @@ func (z *zipReader) Hash(algo string) (string, error) { func (z *zipReader) Close() error { z.raw = nil z.reader = nil + z.index = nil return nil } @@ -109,6 +129,16 @@ func fileInfoFromZip(f *zip.File) FileInfo { ModTime: f.Modified, IsDir: f.FileInfo().IsDir(), Mode: uint32(f.Mode()), + HasMode: zipHasUnixMode(&f.FileHeader), + } +} + +func zipHasUnixMode(h *zip.FileHeader) bool { + switch h.CreatorVersion >> zipCreatorShift { + case zipCreatorUnix, zipCreatorMacOSX: + return h.ExternalAttrs>>zipUnixModeShift != 0 + default: + return false } } From 5e040623574cd0ed352726e54e6d68e34b618960 Mon Sep 17 00:00:00 2001 From: Andrew Nesbitt Date: Mon, 3 Aug 2026 16:43:38 +0100 Subject: [PATCH 2/2] Document Mode field semantics and setuid drop in ExtractAll --- archives.go | 2 +- extract.go | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/archives.go b/archives.go index e5675ce..9081dff 100644 --- a/archives.go +++ b/archives.go @@ -38,7 +38,7 @@ type FileInfo struct { Size int64 // Uncompressed size in bytes ModTime time.Time // Modification time IsDir bool // Whether this is a directory - Mode uint32 // File mode/permissions + Mode uint32 // fs.FileMode value: permission bits plus fs.ModeType bits HasMode bool // Whether Mode was recorded by the archive CompressedSize int64 // Compressed size (if available) } diff --git a/extract.go b/extract.go index 6b52557..97f7cc2 100644 --- a/extract.go +++ b/extract.go @@ -105,6 +105,9 @@ func extractEntry(r Reader, root *os.Root, entry FileInfo) (*deferredChmod, erro } defer func() { _ = src.Close() }() + // Perm() masks to the low nine bits, so setuid/setgid/sticky are dropped + // deliberately: extracted registry archives are untrusted and should not + // be able to plant privilege-escalating bits on disk. perm := fs.FileMode(extractFilePerm) if entry.HasMode { perm = fs.FileMode(entry.Mode).Perm()