From 7717baa43d22cf8fa01210497e7d89382a471d77 Mon Sep 17 00:00:00 2001 From: Alex Goodman Date: Wed, 21 Oct 2026 11:20:30 -0400 Subject: [PATCH] fix: refuse archive entry names that forge paths or lines `entryName` only rejected NUL and over-long names, so whatever an archive called an entry flowed straight into `FullPath`, the file tree and every encoder. - a name containing `!/` could pose as a nested archive boundary and collide with a real nested entry - control characters and unicode line separators could add lines to SPDX tag-value output Both are now refused, and refused names (over-long ones included) mark the resolver truncated with the quoted name instead of being dropped silently. Signed-off-by: Alex Goodman --- internal/archive/README.md | 3 ++- internal/archive/extract.go | 2 +- internal/archive/extract_test.go | 22 ++++++++++++++++++ internal/archive/resolver.go | 38 +++++++++++++++++++++++--------- 4 files changed, 52 insertions(+), 13 deletions(-) diff --git a/internal/archive/README.md b/internal/archive/README.md index 41895689e..ef7d9dbaf 100644 --- a/internal/archive/README.md +++ b/internal/archive/README.md @@ -89,6 +89,7 @@ files. `AllLocations` enumerates every non-directory path, links included. Answe archive task is running, so the java cataloger leaves nested archives to the task instead of opening them itself. - `entryHeader`: keeps regular files, directories and links only (no devices or fifos), cleans entry names to - archive-relative paths, refuses names longer than PATH_MAX or containing NUL, and drops header + archive-relative paths, refuses names longer than PATH_MAX or containing control characters, line + separators or the `!/` archive path separator (marking the resolver truncated), and drops header fields nothing reads (PAX records, user and group names, sub-second and access times) since the header is held for the life of the archive. diff --git a/internal/archive/extract.go b/internal/archive/extract.go index fc21834b3..a199f9c6c 100644 --- a/internal/archive/extract.go +++ b/internal/archive/extract.go @@ -140,7 +140,7 @@ func (r *Resolver) extract(ctx context.Context, format archives.Extractor, conte return err } err := format.Extract(ctx, content, func(_ context.Context, f archives.FileInfo) error { - hdr, ok := entryHeader(f) + hdr, ok := r.entryHeader(f) if !ok || exclusions.Excludes(hdr.Name) { return nil } diff --git a/internal/archive/extract_test.go b/internal/archive/extract_test.go index e8f92ef69..0d62066f9 100644 --- a/internal/archive/extract_test.go +++ b/internal/archive/extract_test.go @@ -225,6 +225,28 @@ func TestResolver_extract_dataEdgeCases(t *testing.T) { }) } +func TestResolver_extract_unsafeEntryNamesAreRefusedAndReported(t *testing.T) { + jar := string(zipBytes(t, map[string]string{"META-INF/MANIFEST.MF": "real"})) + tests := []struct { + name string + forged string + }{ + {name: "archive path separator", forged: "a.jar!/META-INF/MANIFEST.MF"}, + {name: "line break", forged: "x\nlayerID: sha256:forged"}, + {name: "unicode line separator", forged: "x\u2028y"}, + {name: "C1 control", forged: "x\u0085y"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + data := zipBytes(t, map[string]string{"a.jar": jar, tt.forged: "forged"}) + r, _ := extractBytes(t, data, "outer.zip", nil) + assert.Equal(t, []string{"a.jar"}, storedNames(t, r)) + require.Error(t, r.Truncated) + assert.Contains(t, r.Truncated.Error(), fmt.Sprintf("%q", tt.forged)) + }) + } +} + func TestResolver_extract_excludedEntriesAreNeverStored(t *testing.T) { data := zipBytes(t, map[string]string{ "keep.txt": "kept", diff --git a/internal/archive/resolver.go b/internal/archive/resolver.go index 743c6eb1e..7fc4051e6 100644 --- a/internal/archive/resolver.go +++ b/internal/archive/resolver.go @@ -13,6 +13,7 @@ import ( "sort" "strings" "time" + "unicode" "github.com/dustin/go-humanize" "github.com/mholt/archives" @@ -460,21 +461,24 @@ const maxEntryNameBytes = 4096 // entryHeader builds the header stored for one archive entry, or reports false for an entry not worth // carrying: device nodes, fifos and sockets, which no cataloger reads, and names that cannot be a path -// inside the archive. +// inside the archive. A name refused as unsafe marks the resolver truncated. // // The name is cleaned to an archive-relative path with no leading slash, so "../x" names x at the // archive root rather than anything outside it. Symlinks are stored as headers, never followed on the // host; the resolver resolves their targets inside the archive. -func entryHeader(f archives.FileInfo) (tar.Header, bool) { +func (r *Resolver) entryHeader(f archives.FileInfo) (tar.Header, bool) { mode := f.Mode() if !mode.IsRegular() && !mode.IsDir() && mode.Type()&fs.ModeSymlink == 0 { log.WithFields("entry", f.NameInArchive, "mode", mode).Debug("skipping non-regular archive entry") return tar.Header{}, false } - name, ok := entryName(f.NameInArchive) - if !ok { - log.WithFields("entry-name-bytes", len(f.NameInArchive)).Trace("skipping archive entry with an unusable name") + name, err := entryName(f.NameInArchive) + if err != nil { + r.truncate(fmt.Errorf("an entry was skipped: %w", err)) + return tar.Header{}, false + } + if name == "" { return tar.Header{}, false } @@ -495,12 +499,24 @@ func entryHeader(f archives.FileInfo) (tar.Header, bool) { return *hdr, true } -// entryName cleans an archive entry's name to a path relative to the archive root, reporting false -// when it names nothing inside the archive. -func entryName(name string) (string, bool) { - if len(name) > maxEntryNameBytes || strings.ContainsRune(name, 0) { - return "", false +// entryName cleans an archive entry's name to a path relative to the archive root, returning "" when it +// names nothing inside the archive. This is the trust boundary for names: every one ends up in FullPath +// and the encoders, so a name that could forge an archive boundary ("!/") or a line in a line-oriented +// format (control characters, unicode line separators) is refused. +func entryName(name string) (string, error) { + if len(name) > maxEntryNameBytes { + return "", fmt.Errorf("entry name of %d bytes exceeds the %d byte cap", len(name), maxEntryNameBytes) + } + if strings.IndexFunc(name, isUnsafeNameRune) >= 0 { + return "", fmt.Errorf("entry name %q contains a control character", name) } cleaned := strings.TrimPrefix(path.Clean("/"+name), "/") - return cleaned, cleaned != "" + if strings.Contains(cleaned, file.ArchivePathSeparator) { + return "", fmt.Errorf("entry name %q contains the archive path separator %q", name, file.ArchivePathSeparator) + } + return cleaned, nil +} + +func isUnsafeNameRune(c rune) bool { + return unicode.IsControl(c) || c == '\u2028' || c == '\u2029' }