From 984992bd89bb1b7d5e0af28645307347ebeee09b Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Mon, 15 Jun 2026 06:49:27 +0300 Subject: [PATCH] fix(security): harden package-fetch archive extraction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Apply four hardenings to the npx/uvx package-source fetch extractor (internal/security/scanner/package_fetch.go), for both the tar and zip paths, while keeping behavior identical for well-formed small archives: 1. Decompression bomb across skipped members: read the gzip tar stream through a cappedReader so EVERY decompressed byte is bounded — including the bodies of members the extractor skips (oversized, symlink, traversal), which tar.Reader still decompresses while advancing to the next header. 2. safeJoin REJECTS path-traversal and absolute entries with an error instead of cleaning '../' into an in-dest path (the rewrite bug). 3. Oversized and partially-written files are charged to the file-count and byte caps (a truncated-but-large file still counts). 4. Directory creation is capped (charged to the same combined entry limit) before the file-count limit can be bypassed by an all-directories archive — for both tar and zip. extractTarballGz/extractZip take an explicit maxFileBytes so the per-file cap is testable. Adds TDD fixtures: gzip bomb of skipped members, traversal reject (not rewrite), oversized-charged, dir cap. Related #670 --- internal/security/scanner/package_fetch.go | 143 +++++++++++---- .../security/scanner/package_fetch_test.go | 173 +++++++++++++++++- 2 files changed, 274 insertions(+), 42 deletions(-) diff --git a/internal/security/scanner/package_fetch.go b/internal/security/scanner/package_fetch.go index 7c195fa25..5daad507f 100644 --- a/internal/security/scanner/package_fetch.go +++ b/internal/security/scanner/package_fetch.go @@ -5,6 +5,7 @@ import ( "archive/zip" "compress/gzip" "context" + "errors" "fmt" "io" "os" @@ -52,6 +53,30 @@ const ( archiveZip ) +// errArchiveTooLarge is returned by cappedReader once the total number of bytes +// read past it exceeds the cap. It is the decompression-bomb backstop. +var errArchiveTooLarge = errors.New("decompressed archive exceeded size cap") + +// cappedReader counts every byte read through it and fails the read once the +// running total exceeds limit. Wrapping the gzip stream with it bounds the TOTAL +// decompressed output — including the bodies of tar members we skip (oversized, +// symlink, traversal), which the tar reader still decompresses while advancing +// to the next header. This closes the gzip-bomb-via-skipped-members hole. +type cappedReader struct { + r io.Reader + n int64 + limit int64 +} + +func (c *cappedReader) Read(p []byte) (int, error) { + n, err := c.r.Read(p) + c.n += int64(n) + if c.n > c.limit { + return n, errArchiveTooLarge + } + return n, err +} + // parsePackageSpec splits a package spec into its name and exact version. // Only exact pins are honored ("pkg@1.2.3", "pkg==1.2.3"); range/min specifiers // (">=", "~", "^") yield an empty version so the package manager resolves the @@ -154,11 +179,26 @@ func findDownloadedArchive(dir string) (path string, kind archiveKind, err error return "", 0, fmt.Errorf("no package archive found in %s", dir) } -// safeJoin joins destDir with a (possibly hostile) archive entry name and -// returns an error if the result would escape destDir (path traversal). +// safeJoin joins destDir with a (possibly hostile) archive entry name. It +// REJECTS — never sanitizes/rewrites — absolute paths and any path-traversal +// entry: an entry like "../../etc/passwd" must error out, not be silently +// cleaned into an in-dest path (the MCP-2444 bug). Archive entry names use '/' +// separators by convention; we treat both '/' and '\' as separators so a +// Windows-style traversal can't slip through. func safeJoin(destDir, name string) (string, error) { - cleaned := filepath.Clean("/" + name) // make absolute then strip leading / - target := filepath.Join(destDir, cleaned) + if name == "" { + return "", fmt.Errorf("empty archive entry name") + } + if filepath.IsAbs(name) || strings.HasPrefix(name, "/") || strings.HasPrefix(name, `\`) { + return "", fmt.Errorf("entry %q is an absolute path", name) + } + for _, part := range strings.FieldsFunc(name, func(r rune) bool { return r == '/' || r == '\\' }) { + if part == ".." { + return "", fmt.Errorf("entry %q contains a path-traversal component", name) + } + } + target := filepath.Join(destDir, filepath.Clean(name)) + // Defense in depth: confirm the result stays within destDir. rel, err := filepath.Rel(destDir, target) if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { return "", fmt.Errorf("entry %q escapes destination", name) @@ -168,51 +208,73 @@ func safeJoin(destDir, name string) (string, error) { // extractTarballGz extracts a gzip-compressed tar stream into destDir. It is // hardened for UNTRUSTED input: regular files and directories only (symlinks, -// hardlinks, devices skipped), path-traversal entries skipped, and bounded by -// maxFiles and maxTotalBytes. -func extractTarballGz(r io.Reader, destDir string, maxFiles int, maxTotalBytes int64) error { +// hardlinks, devices skipped), path-traversal entries rejected (not rewritten), +// and bounded by maxFiles (files + directories combined), maxTotalBytes (TOTAL +// decompressed bytes, including the bodies of skipped members — gzip-bomb guard) +// and maxFileBytes (any single file). +func extractTarballGz(r io.Reader, destDir string, maxFiles int, maxTotalBytes, maxFileBytes int64) error { gz, err := gzip.NewReader(r) if err != nil { return fmt.Errorf("gzip: %w", err) } defer gz.Close() - tr := tar.NewReader(gz) - var fileCount int + // Read the tar through a cappedReader so EVERY decompressed byte counts — + // including bodies of entries we skip, which tar.Reader decompresses while + // seeking the next header. This bounds a bomb even when all members are + // skipped (oversized/symlink/traversal). + capped := &cappedReader{r: gz, limit: maxTotalBytes} + tr := tar.NewReader(capped) + + var entryCount int // files + directories, both charged to maxFiles var totalBytes int64 for { hdr, err := tr.Next() if err == io.EOF { break } + if errors.Is(err, errArchiveTooLarge) { + return fmt.Errorf("package exceeds max total size (%d bytes)", maxTotalBytes) + } if err != nil { return fmt.Errorf("tar: %w", err) } + // Only regular files and directories. Symlinks/hardlinks are escape // vectors and are never needed for source scanning. switch hdr.Typeflag { case tar.TypeDir: + // Cap directory creation alongside files: charge it to the same + // entry limit BEFORE creating, so an all-directories archive can't + // bypass the file-count cap. + if entryCount >= maxFiles { + return fmt.Errorf("package exceeds max entry count (%d)", maxFiles) + } target, jerr := safeJoin(destDir, hdr.Name) if jerr != nil { - continue + continue // reject traversal/absolute dir entry } + entryCount++ _ = os.MkdirAll(target, 0o755) continue case tar.TypeReg, tar.TypeRegA: //nolint:staticcheck // TypeRegA for legacy tars // handled below default: - continue // symlink, hardlink, char/block device, fifo — skip + continue // symlink, hardlink, char/block device, fifo — skip (body still counted by capped) } target, jerr := safeJoin(destDir, hdr.Name) if jerr != nil { - continue + continue // reject traversal/absolute file entry } - if fileCount >= maxFiles { - return fmt.Errorf("package exceeds max file count (%d)", maxFiles) + if entryCount >= maxFiles { + return fmt.Errorf("package exceeds max entry count (%d)", maxFiles) } - if hdr.Size > fetchMaxFileBytes { - continue // skip oversized single file rather than abort + // Charge the entry now so an oversized or partially-written file still + // counts toward the caps (it consumed resources either way). + entryCount++ + if hdr.Size > maxFileBytes { + continue // skip oversized single file; its body is still counted by capped } if totalBytes+hdr.Size > maxTotalBytes { return fmt.Errorf("package exceeds max total size (%d bytes)", maxTotalBytes) @@ -221,31 +283,40 @@ func extractTarballGz(r io.Reader, destDir string, maxFiles int, maxTotalBytes i continue } written, werr := writeArchiveFile(target, tr, hdr.Size) - if werr != nil { - continue + totalBytes += written // charge bytes even on a partial/failed write + if errors.Is(werr, errArchiveTooLarge) { + return fmt.Errorf("package exceeds max total size (%d bytes)", maxTotalBytes) } - totalBytes += written - fileCount++ } return nil } // extractZip extracts a zip archive (e.g. a Python wheel) into destDir with the -// same untrusted-input hardening as extractTarballGz. -func extractZip(zipPath, destDir string, maxFiles int, maxTotalBytes int64) error { +// same untrusted-input hardening as extractTarballGz: directories and files are +// charged to a single entry cap (maxFiles), traversal/absolute entries are +// rejected, and a single entry that lies about its decompressed size is bounded +// by writeArchiveFile (deflate-bomb guard) with its written bytes charged. +func extractZip(zipPath, destDir string, maxFiles int, maxTotalBytes, maxFileBytes int64) error { zr, err := zip.OpenReader(zipPath) if err != nil { return fmt.Errorf("zip: %w", err) } defer zr.Close() - var fileCount int + var entryCount int // files + directories, both charged to maxFiles var totalBytes int64 for _, f := range zr.File { if f.FileInfo().IsDir() { - if target, jerr := safeJoin(destDir, f.Name); jerr == nil { - _ = os.MkdirAll(target, 0o755) + // Cap directory creation before the file-count limit can be bypassed. + if entryCount >= maxFiles { + return fmt.Errorf("package exceeds max entry count (%d)", maxFiles) } + target, jerr := safeJoin(destDir, f.Name) + if jerr != nil { + continue // reject traversal/absolute dir entry + } + entryCount++ + _ = os.MkdirAll(target, 0o755) continue } // Skip symlinks (mode bit) — escape vector. @@ -254,13 +325,16 @@ func extractZip(zipPath, destDir string, maxFiles int, maxTotalBytes int64) erro } target, jerr := safeJoin(destDir, f.Name) if jerr != nil { - continue + continue // reject traversal/absolute file entry } - if fileCount >= maxFiles { - return fmt.Errorf("package exceeds max file count (%d)", maxFiles) + if entryCount >= maxFiles { + return fmt.Errorf("package exceeds max entry count (%d)", maxFiles) } + // Charge the entry now so an oversized or partially-written file still + // counts toward the caps. + entryCount++ size := int64(f.UncompressedSize64) - if size > fetchMaxFileBytes { + if size > maxFileBytes { continue } if totalBytes+size > maxTotalBytes { @@ -276,11 +350,8 @@ func extractZip(zipPath, destDir string, maxFiles int, maxTotalBytes int64) erro } written, werr := writeArchiveFile(target, rc, size) rc.Close() - if werr != nil { - continue - } - totalBytes += written - fileCount++ + totalBytes += written // charge bytes even on a partial/failed write + _ = werr } return nil } @@ -365,7 +436,7 @@ func (r *SourceResolver) fetchNpmPackage(ctx context.Context, info ServerInfo, s return nil, err } defer f.Close() - if err := extractTarballGz(f, srcDir, fetchMaxFiles, fetchMaxTotalBytes); err != nil { + if err := extractTarballGz(f, srcDir, fetchMaxFiles, fetchMaxTotalBytes, fetchMaxFileBytes); err != nil { cleanup() return nil, fmt.Errorf("extract npm tarball: %w", err) } @@ -412,7 +483,7 @@ func (r *SourceResolver) fetchPythonPackage(ctx context.Context, info ServerInfo cleanup() return nil, fmt.Errorf("python download produced a non-wheel archive; refusing to build/extract an sdist (would execute untrusted code)") } - if err := extractZip(archive, srcDir, fetchMaxFiles, fetchMaxTotalBytes); err != nil { + if err := extractZip(archive, srcDir, fetchMaxFiles, fetchMaxTotalBytes, fetchMaxFileBytes); err != nil { cleanup() return nil, fmt.Errorf("extract wheel: %w", err) } diff --git a/internal/security/scanner/package_fetch_test.go b/internal/security/scanner/package_fetch_test.go index 8b448bd29..7a5a5a717 100644 --- a/internal/security/scanner/package_fetch_test.go +++ b/internal/security/scanner/package_fetch_test.go @@ -85,7 +85,7 @@ func TestExtractTarballGz_Normal(t *testing.T) { "package/lib/util.js": "module.exports = {}", }) dest := t.TempDir() - if err := extractTarballGz(bytes.NewReader(data), dest, 1000, 10<<20); err != nil { + if err := extractTarballGz(bytes.NewReader(data), dest, 1000, 10<<20, fetchMaxFileBytes); err != nil { t.Fatalf("extractTarballGz: %v", err) } for _, rel := range []string{"package/index.js", "package/package.json", "package/lib/util.js"} { @@ -102,11 +102,17 @@ func TestExtractTarballGz_RejectsZipSlip(t *testing.T) { }) dest := t.TempDir() // Extraction should not error fatally on the bad entry but MUST skip it. - _ = extractTarballGz(bytes.NewReader(data), dest, 1000, 10<<20) + _ = extractTarballGz(bytes.NewReader(data), dest, 1000, 10<<20, fetchMaxFileBytes) escaped := filepath.Join(filepath.Dir(filepath.Dir(dest)), "etc", "evil.txt") if _, err := os.Stat(escaped); err == nil { t.Fatalf("zip-slip: file escaped to %s", escaped) } + // safeJoin must REJECT the traversal entry, not rewrite it into an in-dest + // path (the MCP-2444 bug): the rewritten location must not exist either. + rewritten := filepath.Join(dest, "etc", "evil.txt") + if _, err := os.Stat(rewritten); err == nil { + t.Fatalf("traversal entry was rewritten into dest at %s instead of being rejected", rewritten) + } // The legitimate entry should still be present. if _, err := os.Stat(filepath.Join(dest, "package", "ok.js")); err != nil { t.Errorf("legit entry missing: %v", err) @@ -125,7 +131,7 @@ func TestExtractTarballGz_RejectsSymlinkEscape(t *testing.T) { tw.Close() gw.Close() dest := t.TempDir() - _ = extractTarballGz(bytes.NewReader(buf.Bytes()), dest, 1000, 10<<20) + _ = extractTarballGz(bytes.NewReader(buf.Bytes()), dest, 1000, 10<<20, fetchMaxFileBytes) link := filepath.Join(dest, "package", "evil-link") if _, err := os.Lstat(link); err == nil { t.Fatalf("symlink entry should have been skipped, found %s", link) @@ -137,11 +143,162 @@ func TestExtractTarballGz_SizeCap(t *testing.T) { "package/big.bin": strings.Repeat("A", 1024), }) dest := t.TempDir() - if err := extractTarballGz(bytes.NewReader(data), dest, 1000, 100); err == nil { + if err := extractTarballGz(bytes.NewReader(data), dest, 1000, 100, fetchMaxFileBytes); err == nil { t.Fatal("expected size-cap error, got nil") } } +// tarItem is an ordered tar entry (the map-based writeTarGz can't control order +// or emit directory/non-regular entries). +type tarItem struct { + name string + body string + typeflag byte +} + +// writeTarGzOrdered builds an in-memory .tar.gz from ordered entries. +func writeTarGzOrdered(t *testing.T, items []tarItem) []byte { + t.Helper() + var buf bytes.Buffer + gw := gzip.NewWriter(&buf) + tw := tar.NewWriter(gw) + for _, it := range items { + hdr := &tar.Header{Name: it.name, Mode: 0o644, Size: int64(len(it.body)), Typeflag: it.typeflag} + if it.typeflag == tar.TypeDir { + hdr.Mode = 0o755 + } + if err := tw.WriteHeader(hdr); err != nil { + t.Fatal(err) + } + if len(it.body) > 0 { + if _, err := tw.Write([]byte(it.body)); err != nil { + t.Fatal(err) + } + } + } + if err := tw.Close(); err != nil { + t.Fatal(err) + } + if err := gw.Close(); err != nil { + t.Fatal(err) + } + return buf.Bytes() +} + +// MCP-2444 bug #2: safeJoin must REJECT path-traversal / absolute entries with an +// error, never sanitize/rewrite them into an in-dest path. +func TestSafeJoin_RejectsTraversal(t *testing.T) { + dest := t.TempDir() + reject := []string{ + "../escape", + "../../etc/passwd", + "a/../../b", + "/etc/passwd", + "package/../../escape", + } + for _, name := range reject { + if got, err := safeJoin(dest, name); err == nil { + t.Errorf("safeJoin(%q) = %q, want error (path traversal must be rejected)", name, got) + } + } + allow := []string{"package/index.js", "pkg/sub/file.py", "flights-0.2.4.dist-info/METADATA"} + for _, name := range allow { + if _, err := safeJoin(dest, name); err != nil { + t.Errorf("safeJoin(%q) returned unexpected error: %v", name, err) + } + } +} + +// MCP-2444 bug #1: total DECOMPRESSED bytes must be bounded across SKIPPED tar +// members — a gzip bomb made of oversized (therefore skipped) members must still +// abort at the decompressed-byte cap rather than being decompressed in full. +func TestExtractTarballGz_GzipBombSkippedMembers(t *testing.T) { + const maxFileBytes = 1 << 10 // 1 KiB: every member below is oversized -> skipped + const maxTotalBytes = 8 << 10 + var items []tarItem + for i := 0; i < 16; i++ { + // 4 KiB body (> maxFileBytes => skipped) of highly-compressible data; + // 16 * 4 KiB = 64 KiB decompressed >> 8 KiB cap. + items = append(items, tarItem{name: "package/big" + string(rune('a'+i)) + ".bin", body: strings.Repeat("A", 4<<10), typeflag: tar.TypeReg}) + } + data := writeTarGzOrdered(t, items) + dest := t.TempDir() + if err := extractTarballGz(bytes.NewReader(data), dest, 100000, maxTotalBytes, maxFileBytes); err == nil { + t.Fatal("expected decompression-bomb abort across skipped members, got nil") + } +} + +// MCP-2444 bug #3: oversized files must be CHARGED to the file-count cap (a +// truncated-but-large file still counts), not silently skipped without charge. +func TestExtractTarballGz_OversizedFileCharged(t *testing.T) { + const maxFileBytes = 10 + items := []tarItem{ + {name: "package/a.bin", body: strings.Repeat("A", 100), typeflag: tar.TypeReg}, + {name: "package/b.bin", body: strings.Repeat("A", 100), typeflag: tar.TypeReg}, + {name: "package/c.bin", body: strings.Repeat("A", 100), typeflag: tar.TypeReg}, + } + data := writeTarGzOrdered(t, items) + dest := t.TempDir() + // maxFiles=2; three oversized files must trip the count cap on the third. + if err := extractTarballGz(bytes.NewReader(data), dest, 2, 10<<20, maxFileBytes); err == nil { + t.Fatal("expected file-count cap to trip on oversized (charged) files, got nil") + } +} + +// MCP-2444 bug #4: directory creation must be capped (charged toward the entry +// limit) so an all-directories archive cannot bypass the file-count limit. +func TestExtractTarballGz_DirCap(t *testing.T) { + var items []tarItem + for i := 0; i < 8; i++ { + items = append(items, tarItem{name: "package/d" + string(rune('a'+i)) + "/", typeflag: tar.TypeDir}) + } + data := writeTarGzOrdered(t, items) + dest := t.TempDir() + if err := extractTarballGz(bytes.NewReader(data), dest, 3, 10<<20, fetchMaxFileBytes); err == nil { + t.Fatal("expected directory-count cap to trip, got nil") + } +} + +// writeZipOrdered builds an in-memory .zip with ordered entries (dir entries use +// a trailing slash, matching the zip convention). +func writeZipOrdered(t *testing.T, names []string) []byte { + t.Helper() + var buf bytes.Buffer + zw := zip.NewWriter(&buf) + for _, name := range names { + if strings.HasSuffix(name, "/") { + if _, err := zw.Create(name); err != nil { + t.Fatal(err) + } + continue + } + w, err := zw.Create(name) + if err != nil { + t.Fatal(err) + } + if _, err := w.Write([]byte("x")); err != nil { + t.Fatal(err) + } + } + if err := zw.Close(); err != nil { + t.Fatal(err) + } + return buf.Bytes() +} + +// MCP-2444 bug #4 (zip path): directory entries must be capped too. +func TestExtractZip_DirCap(t *testing.T) { + names := []string{"da/", "db/", "dc/", "dd/", "de/"} + zipPath := filepath.Join(t.TempDir(), "dirs.whl") + if err := os.WriteFile(zipPath, writeZipOrdered(t, names), 0o644); err != nil { + t.Fatal(err) + } + dest := t.TempDir() + if err := extractZip(zipPath, dest, 3, 10<<20, fetchMaxFileBytes); err == nil { + t.Fatal("expected directory-count cap to trip in zip path, got nil") + } +} + // writeZip builds an in-memory .zip (wheel) from a map of relative path -> contents. func writeZip(t *testing.T, entries map[string]string) []byte { t.Helper() @@ -173,7 +330,7 @@ func TestExtractZip_Normal(t *testing.T) { t.Fatal(err) } dest := t.TempDir() - if err := extractZip(zipPath, dest, 1000, 10<<20); err != nil { + if err := extractZip(zipPath, dest, 1000, 10<<20, fetchMaxFileBytes); err != nil { t.Fatalf("extractZip: %v", err) } if _, err := os.Stat(filepath.Join(dest, "flights", "server.py")); err != nil { @@ -191,11 +348,15 @@ func TestExtractZip_RejectsZipSlip(t *testing.T) { t.Fatal(err) } dest := t.TempDir() - _ = extractZip(zipPath, dest, 1000, 10<<20) + _ = extractZip(zipPath, dest, 1000, 10<<20, fetchMaxFileBytes) escaped := filepath.Join(filepath.Dir(filepath.Dir(dest)), "etc", "evil.txt") if _, err := os.Stat(escaped); err == nil { t.Fatalf("zip-slip: file escaped to %s", escaped) } + // Reject, don't rewrite: the in-dest rewritten path must not exist either. + if _, err := os.Stat(filepath.Join(dest, "etc", "evil.txt")); err == nil { + t.Fatalf("traversal entry was rewritten into dest instead of being rejected") + } } // The crux of the security design: the fetch step must NEVER execute the