Skip to content

Commit 2f5a213

Browse files
egibsegibs
authored
fix: address more fuzzing errors; miscellaneous improvements (#1364)
Signed-off-by: egibs <20933572+egibs@users.noreply.github.com>
1 parent dd0e3d0 commit 2f5a213

12 files changed

Lines changed: 238 additions & 99 deletions

File tree

‎pkg/action/diff.go‎

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -519,15 +519,6 @@ func formatReportKey(res ScanResult, fr *malcontent.FileReport, isReport bool) s
519519
return formatKey(res, CleanPath(fr.Path, res.tmpRoot))
520520
}
521521

522-
func behaviorExists(b *malcontent.Behavior, behaviors []*malcontent.Behavior) bool {
523-
for _, tb := range behaviors {
524-
if b.ID == tb.ID {
525-
return true
526-
}
527-
}
528-
return false
529-
}
530-
531522
// fileDiff handles files that exist in both source and destination.
532523
func fileDiff(ctx context.Context, c malcontent.Config, fr, tr *malcontent.FileReport, rpath, apath string, d *malcontent.DiffReport, src ScanResult, dest ScanResult, archiveOrImage, isReport, isMoved bool) {
533524
if ctx.Err() != nil {
@@ -563,17 +554,26 @@ func fileDiff(ctx context.Context, c malcontent.Config, fr, tr *malcontent.FileR
563554
abs.PreviousPath = fr.Path
564555
}
565556

557+
srcBehaviorIDs := make(map[string]struct{}, len(fr.Behaviors))
558+
for _, b := range fr.Behaviors {
559+
srcBehaviorIDs[b.ID] = struct{}{}
560+
}
561+
destBehaviorIDs := make(map[string]struct{}, len(tr.Behaviors))
562+
for _, b := range tr.Behaviors {
563+
destBehaviorIDs[b.ID] = struct{}{}
564+
}
565+
566566
// if destination behavior is not in the source
567567
for _, tb := range tr.Behaviors {
568-
if !behaviorExists(tb, fr.Behaviors) {
568+
if _, ok := srcBehaviorIDs[tb.ID]; !ok {
569569
tb.DiffAdded = true
570570
abs.Behaviors = append(abs.Behaviors, tb)
571571
}
572572
}
573573

574574
// if source behavior is not in the destination
575575
for _, fb := range fr.Behaviors {
576-
if !behaviorExists(fb, tr.Behaviors) {
576+
if _, ok := destBehaviorIDs[fb.ID]; !ok {
577577
fb.DiffRemoved = true
578578
abs.Behaviors = append(abs.Behaviors, fb)
579579
}
@@ -758,8 +758,6 @@ func formatKey(res ScanResult, name string) string {
758758
switch {
759759
case res.imageURI != "":
760760
return fmt.Sprintf("%s ∴ %s", res.imageURI, name)
761-
case res.tmpRoot != "":
762-
return fmt.Sprintf("%s ∴ %s", res.tmpRoot, name)
763761
case res.base != "":
764762
return fmt.Sprintf("%s ∴ %s", res.base, name)
765763
default:

‎pkg/action/diff_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -233,10 +233,10 @@ func TestFormatKey(t *testing.T) {
233233
want: "/bin/ls",
234234
},
235235
{
236-
name: "with tmpRoot prepends tmpRoot",
236+
name: "with tmpRoot falls through to plain path",
237237
res: ScanResult{tmpRoot: "/tmp/extract", imageURI: ""},
238238
file: "/tmp/extract/bin/ls",
239-
want: "/tmp/extract ∴ /tmp/extract/bin/ls",
239+
want: "/tmp/extract/bin/ls",
240240
},
241241
{
242242
name: "with image URI prepends imageURI",

‎pkg/archive/archive.go‎

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,31 @@ func evalSymlinks(path string) (string, bool) {
6060
return resolved, true
6161
}
6262

63+
// symlinkEscapesDir checks whether a symlink at target resolves outside dir.
64+
func symlinkEscapesDir(target, dir string) bool {
65+
fi, err := os.Lstat(target)
66+
if err != nil || fi.Mode()&os.ModeSymlink == 0 {
67+
return false
68+
}
69+
70+
evalTarget, err := filepath.EvalSymlinks(target)
71+
if err != nil {
72+
// Dangling symlinks (target doesn't exist) are not path traversals.
73+
return !errors.Is(err, fs.ErrNotExist)
74+
}
75+
76+
evalDir, err := filepath.EvalSymlinks(dir)
77+
if err != nil {
78+
return true
79+
}
80+
81+
rel, err := filepath.Rel(evalDir, evalTarget)
82+
if err != nil {
83+
return false
84+
}
85+
return rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator))
86+
}
87+
6388
// isValidPath checks if the target file is within the given directory.
6489
func IsValidPath(target, dir string) bool {
6590
if strings.Contains(target, "\x00") || strings.Contains(dir, "\x00") {
@@ -69,20 +94,8 @@ func IsValidPath(target, dir string) bool {
6994
cleanTarget := filepath.Clean(target)
7095
cleanDir := filepath.Clean(dir)
7196

72-
// avoid evaluating symlinks if the target is not a symlink
73-
if fi, err := os.Lstat(cleanTarget); err == nil && fi.Mode()&os.ModeSymlink == os.ModeSymlink {
74-
var evalTarget, evalDir, rel string
75-
76-
if evalTarget, err = filepath.EvalSymlinks(cleanTarget); err != nil {
77-
return false
78-
}
79-
if evalDir, err = filepath.EvalSymlinks(cleanDir); err != nil {
80-
return false
81-
}
82-
if rel, err = filepath.Rel(evalDir, evalTarget); err == nil &&
83-
(rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator))) {
84-
return false
85-
}
97+
if symlinkEscapesDir(cleanTarget, cleanDir) {
98+
return false
8699
}
87100

88101
switch {

‎pkg/archive/fuzz_test.go‎

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,10 @@ import (
1717
"github.com/chainguard-dev/malcontent/pkg/programkind"
1818
)
1919

20+
// maxFuzzSize is the maximum input size for fuzz tests to stay well under
21+
// Go's 100MB fuzzer shared memory capacity and avoid OOM in parsers.
22+
const maxFuzzSize = 10 * 1024 * 1024
23+
2024
// readTestFile reads a file using file.GetContents for consistency with production code.
2125
func readTestFile(path string) ([]byte, error) {
2226
f, err := os.Open(path)
@@ -52,6 +56,9 @@ func FuzzExtractTar(f *testing.F) {
5256
f.Add([]byte{0x1f, 0x8b, 0x08, 0x00}) // gzip magic bytes only
5357

5458
f.Fuzz(func(t *testing.T, data []byte) {
59+
if len(data) > maxFuzzSize {
60+
return
61+
}
5562
tmpFile, err := os.CreateTemp("", "fuzz-tar-*.tar.gz")
5663
if err != nil {
5764
t.Skip("failed to create temp file")
@@ -107,6 +114,9 @@ func FuzzExtractZip(f *testing.F) {
107114
f.Add([]byte{0x50, 0x4b, 0x03, 0x04}) // full zip signature
108115

109116
f.Fuzz(func(t *testing.T, data []byte) {
117+
if len(data) > maxFuzzSize {
118+
return
119+
}
110120
tmpFile, err := os.CreateTemp("", "fuzz-zip-*.zip")
111121
if err != nil {
112122
t.Skip("failed to create temp file")
@@ -196,6 +206,9 @@ func FuzzExtractArchive(f *testing.F) {
196206
f.Add([]byte{0x1f, 0x8b, 0x08, 0x00}, ".gz") // gzip header
197207

198208
f.Fuzz(func(t *testing.T, data []byte, ext string) {
209+
if len(data) > maxFuzzSize {
210+
return
211+
}
199212
if _, ok := programkind.ArchiveMap[ext]; !ok {
200213
return
201214
}
@@ -294,6 +307,9 @@ func FuzzExtractGzip(f *testing.F) {
294307
f.Add(make([]byte, 1024*1024)) // large zeros (compression bomb test)
295308

296309
f.Fuzz(func(t *testing.T, data []byte) {
310+
if len(data) > maxFuzzSize {
311+
return
312+
}
297313
tmpFile, err := os.CreateTemp("", "fuzz-gz-*.gz")
298314
if err != nil {
299315
t.Skip()
@@ -338,6 +354,9 @@ func FuzzExtractBz2(f *testing.F) {
338354
f.Add(make([]byte, 1024*1024)) // large zeros
339355

340356
f.Fuzz(func(t *testing.T, data []byte) {
357+
if len(data) > maxFuzzSize {
358+
return
359+
}
341360
tmpFile, err := os.CreateTemp("", "fuzz-bz2-*.bz2")
342361
if err != nil {
343362
t.Skip()
@@ -390,6 +409,9 @@ func FuzzExtractZstd(f *testing.F) {
390409
f.Add(make([]byte, 1024*1024)) // large zeros
391410

392411
f.Fuzz(func(t *testing.T, data []byte) {
412+
if len(data) > maxFuzzSize {
413+
return
414+
}
393415
tmpFile, err := os.CreateTemp("", "fuzz-zst-*.zst")
394416
if err != nil {
395417
t.Skip()
@@ -444,6 +466,9 @@ func FuzzExtractZlib(f *testing.F) {
444466
f.Add(make([]byte, 1024*1024)) // large zeros
445467

446468
f.Fuzz(func(t *testing.T, data []byte) {
469+
if len(data) > maxFuzzSize {
470+
return
471+
}
447472
tmpFile, err := os.CreateTemp("", "fuzz-zlib-*.zlib")
448473
if err != nil {
449474
t.Skip()
@@ -497,7 +522,7 @@ func FuzzExtractRPM(f *testing.F) {
497522
f.Add([]byte("not rpm")) // invalid
498523

499524
f.Fuzz(func(t *testing.T, data []byte) {
500-
if len(data) < 96 || !bytes.Equal(data[:4], rpmMagic) {
525+
if len(data) < 96 || len(data) > maxFuzzSize || !bytes.Equal(data[:4], rpmMagic) {
501526
return
502527
}
503528

@@ -552,6 +577,9 @@ func FuzzExtractDeb(f *testing.F) {
552577
f.Add([]byte("not deb")) // invalid
553578

554579
f.Fuzz(func(t *testing.T, data []byte) {
580+
if len(data) > maxFuzzSize {
581+
return
582+
}
555583
tmpFile, err := os.CreateTemp("", "fuzz-deb-*.deb")
556584
if err != nil {
557585
t.Skip()
@@ -593,6 +621,9 @@ func FuzzExtractUPX(f *testing.F) {
593621
f.Add([]byte("not upx")) // invalid
594622

595623
f.Fuzz(func(t *testing.T, data []byte) {
624+
if len(data) > maxFuzzSize {
625+
return
626+
}
596627
tmpFile, err := os.CreateTemp("", "fuzz-upx-*")
597628
if err != nil {
598629
t.Skip()

‎pkg/archive/symlink_test.go‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
package archive
55

66
import (
7+
"archive/tar"
8+
"bytes"
79
"context"
810
"os"
911
"path/filepath"
@@ -195,6 +197,57 @@ func TestExtractNestedArchiveCollision(t *testing.T) {
195197
}
196198
}
197199

200+
// TestDanglingSymlinkExtraction verifies that a tar containing a dangling symlink
201+
// (target doesn't exist) extracts without error and all extracted paths pass IsValidPath.
202+
func TestDanglingSymlinkExtraction(t *testing.T) {
203+
t.Parallel()
204+
205+
// Build a tar in memory with a dangling symlink (points to nonexistent file within dir)
206+
var buf bytes.Buffer
207+
tw := tar.NewWriter(&buf)
208+
if err := tw.WriteHeader(&tar.Header{
209+
Name: "link",
210+
Typeflag: tar.TypeSymlink,
211+
Linkname: "nonexistent",
212+
}); err != nil {
213+
t.Fatal(err)
214+
}
215+
tw.Close()
216+
217+
tmpFile, err := os.CreateTemp("", "dangling-symlink-*.tar")
218+
if err != nil {
219+
t.Fatal(err)
220+
}
221+
defer os.Remove(tmpFile.Name())
222+
tmpFile.Write(buf.Bytes())
223+
tmpFile.Close()
224+
225+
tmpDir, err := os.MkdirTemp("", "dangling-symlink-extract-*")
226+
if err != nil {
227+
t.Fatal(err)
228+
}
229+
defer os.RemoveAll(tmpDir)
230+
231+
// Extraction should succeed
232+
if err := ExtractTar(context.Background(), tmpDir, tmpFile.Name()); err != nil {
233+
t.Fatalf("ExtractTar failed on dangling symlink: %v", err)
234+
}
235+
236+
// Every extracted path must pass IsValidPath (this is what the fuzzer checks)
237+
err = filepath.WalkDir(tmpDir, func(path string, _ os.DirEntry, err error) error {
238+
if err != nil {
239+
return err
240+
}
241+
if !IsValidPath(path, tmpDir) {
242+
t.Errorf("IsValidPath returned false for dangling symlink: %s", path)
243+
}
244+
return nil
245+
})
246+
if err != nil {
247+
t.Fatalf("WalkDir failed: %v", err)
248+
}
249+
}
250+
198251
func TestHandleSymlink(t *testing.T) {
199252
t.Parallel()
200253
tmpDir, err := os.MkdirTemp("", "symlink-test-*")

‎pkg/compile/fuzz_test.go‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,10 @@ import (
1414
"time"
1515
)
1616

17+
// maxFuzzSize is the maximum input size for fuzz tests to stay well under
18+
// Go's 100MB fuzzer shared memory capacity and avoid OOM in parsers.
19+
const maxFuzzSize = 10 * 1024 * 1024
20+
1721
// FuzzRemoveRules tests the removeRules function with random inputs.
1822
func FuzzRemoveRules(f *testing.F) {
1923
for _, root := range getAllRuleFS() {
@@ -75,6 +79,9 @@ rule keep_me { condition: true }
7579
`), "remove_me_1,remove_me_2")
7680

7781
f.Fuzz(func(t *testing.T, data []byte, rulesToRemove string) {
82+
if len(data) > maxFuzzSize {
83+
return
84+
}
7885
var rules []string
7986
if rulesToRemove != "" {
8087
rules = strings.Split(rulesToRemove, ",")
@@ -138,7 +145,7 @@ func FuzzRecursiveCompile(f *testing.F) {
138145
floatPattern := regexp.MustCompile(`\d+\.\d+`)
139146

140147
f.Fuzz(func(_ *testing.T, data []byte) {
141-
if len(data) > 50*1024*1024 {
148+
if len(data) > maxFuzzSize {
142149
return
143150
}
144151

‎pkg/programkind/fuzz_test.go‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,13 @@ import (
1313
"github.com/chainguard-dev/malcontent/pkg/file"
1414
)
1515

16+
// maxFuzzSize is the maximum input size for fuzz tests to stay well under
17+
// Go's 100MB fuzzer shared memory capacity and avoid OOM in parsers.
18+
const maxFuzzSize = 10 * 1024 * 1024
19+
1620
// FuzzFile tests file type detection with random inputs.
1721
func FuzzFile(f *testing.F) {
18-
// Limit seed file size to avoid excessive memory usage during fuzzing.
19-
const maxSeedSize int64 = 50 * 1024 * 1024 // 50MB
22+
const maxSeedSize int64 = maxFuzzSize
2023

2124
samplesDir := "../../out/chainguard-sandbox/malcontent-samples"
2225
err := filepath.WalkDir(samplesDir, func(path string, d os.DirEntry, _ error) error {
@@ -64,7 +67,7 @@ func FuzzFile(f *testing.F) {
6467
f.Add([]byte("UPX!"), "test.upx") // UPX magic
6568

6669
f.Fuzz(func(t *testing.T, data []byte, filename string) {
67-
if len(data) > 50*1024*1024 {
70+
if len(data) > maxFuzzSize {
6871
return
6972
}
7073
if len(filename) > 255 || filepath.Clean(filename) != filename {

‎pkg/render/fuzz_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ func FuzzRenderDifferential(f *testing.F) {
4747
}
4848

4949
f.Fuzz(func(t *testing.T, riskLevel int8, filePath, behaviorName, behaviorDesc string, hasDiff bool) {
50+
filePath = sanitizeUTF8(filePath)
5051
if filePath == "" || yamlIgnore[filePath] {
5152
return
5253
}

‎pkg/render/json.go‎

Lines changed: 1 addition & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -48,31 +48,7 @@ func (r JSON) Full(ctx context.Context, c *malcontent.Config, rep *malcontent.Re
4848
if ctx.Err() != nil {
4949
return false
5050
}
51-
52-
if key == nil || value == nil {
53-
return true
54-
}
55-
if path, ok := key.(string); ok {
56-
if r, ok := value.(*malcontent.FileReport); ok {
57-
if r.Skipped == "" {
58-
r.ArchiveRoot = ""
59-
r.FullPath = ""
60-
61-
cleanPath := sanitizeUTF8(path)
62-
63-
r.Path = sanitizeUTF8(r.Path)
64-
65-
for _, b := range r.Behaviors {
66-
if b != nil {
67-
b.ID = sanitizeUTF8(b.ID)
68-
b.Description = sanitizeUTF8(b.Description)
69-
}
70-
}
71-
72-
jr.Files[cleanPath] = r
73-
}
74-
}
75-
}
51+
sanitizeFileReport(key, value, jr.Files)
7652
return true
7753
})
7854

0 commit comments

Comments
 (0)