diff --git a/internal/archive/extract_test.go b/internal/archive/extract_test.go index 0d62066f9..7068e05b0 100644 --- a/internal/archive/extract_test.go +++ b/internal/archive/extract_test.go @@ -215,6 +215,19 @@ func TestResolver_extract_dataEdgeCases(t *testing.T) { } }) + t.Run("an entry name with too many path components is skipped and reported", func(t *testing.T) { + atCap := strings.Repeat("a/", maxEntryNameDepth-1) + "f" + data := zipBytes(t, map[string]string{ + atCap: "at the cap", + strings.Repeat("b/", maxEntryNameDepth) + "f": "over the cap", + }) + r, _ := extractBytes(t, data, "test.zip", nil) + names := storedNames(t, r) + assert.Contains(t, names, atCap) + assert.Len(t, names, maxEntryNameDepth, "only the entry at the cap and its implied directories") + assert.ErrorContains(t, r.Truncated, "path components") + }) + t.Run("an over-long entry name is skipped", func(t *testing.T) { data := zipBytes(t, map[string]string{ strings.Repeat("a/", maxEntryNameBytes) + "f": "deep", @@ -526,12 +539,12 @@ func TestExtract_entryCountBombIsBoundedByTheMemoryLimit(t *testing.T) { } func TestExtract_deepPathBombIsBoundedByTheMemoryLimit(t *testing.T) { - // 100 entries whose 4 KiB names each imply 500 distinct directories would be 50,000 nodes; the - // implied directories are charged as nodes, so the memory limit bounds them too + // 100 entries whose names each imply 250 distinct directories would be 25,000 nodes; the implied + // directories are charged as nodes, so the memory limit bounds them too files := map[string]string{} for i := range 100 { var b strings.Builder - for j := range 500 { + for j := range maxEntryNameDepth - 6 { fmt.Fprintf(&b, "%03d-%03d/", i, j) } b.WriteString("f") diff --git a/internal/archive/resolver.go b/internal/archive/resolver.go index 7fc4051e6..5f0cd9286 100644 --- a/internal/archive/resolver.go +++ b/internal/archive/resolver.go @@ -109,13 +109,16 @@ func (r *Resolver) add(hdr tar.Header, content io.Reader) error { return nil } - // directories the path implies that do not exist yet are nodes this entry pays for + // directories the path implies that do not exist yet are nodes this entry pays for. Walking from the + // deepest parent up stops at the first that exists, since its ancestors exist too var newDirs []string - for i := 1; i < len(entryPath); i++ { - if entryPath[i] == '/' && !r.tree.HasPath(stereoscopeFile.Path(entryPath[:i])) { - newDirs = append(newDirs, entryPath[:i]) + for i := strings.LastIndexByte(entryPath, '/'); i > 0; i = strings.LastIndexByte(entryPath[:i], '/') { + if r.tree.HasPath(stereoscopeFile.Path(entryPath[:i])) { + break } + newDirs = append(newDirs, entryPath[:i]) } + slices.Reverse(newDirs) if !r.chargeIndex(approxIndexBytes(hdr) + int64(len(newDirs))*approxIndexBytesPerEntry) { return ErrDiskLimitReached } @@ -459,6 +462,10 @@ func (entryReader) Close() error { return nil } // so a longer name is a cheap way to inflate the index and no filesystem could hold it anyway. const maxEntryNameBytes = 4096 +// maxEntryNameDepth is the most path components an entry name may have. Adding an entry costs work per +// component, and no real archive nests anywhere near this deep. +const maxEntryNameDepth = 256 + // 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. A name refused as unsafe marks the resolver truncated. @@ -511,6 +518,9 @@ func entryName(name string) (string, error) { return "", fmt.Errorf("entry name %q contains a control character", name) } cleaned := strings.TrimPrefix(path.Clean("/"+name), "/") + if depth := strings.Count(cleaned, "/") + 1; depth > maxEntryNameDepth { + return "", fmt.Errorf("entry name of %d path components exceeds the %d component cap", depth, maxEntryNameDepth) + } if strings.Contains(cleaned, file.ArchivePathSeparator) { return "", fmt.Errorf("entry name %q contains the archive path separator %q", name, file.ArchivePathSeparator) }