From c4cdbca50d47ffc5583ce3ca2f834f30e434a236 Mon Sep 17 00:00:00 2001 From: nikpivkin Date: Fri, 3 May 2024 17:01:35 +0600 Subject: [PATCH 1/6] feat(misconf): support symlinks inside of Helm archives --- pkg/iac/scanners/helm/parser/parser.go | 6 +- pkg/iac/scanners/helm/parser/parser_tar.go | 110 +++++++++++++----- pkg/iac/scanners/helm/parser/parser_test.go | 22 ++++ .../archive-with-symlinks/chart.tar.gz | Bin 0 -> 247 bytes 4 files changed, 106 insertions(+), 32 deletions(-) create mode 100644 pkg/iac/scanners/helm/parser/testdata/archive-with-symlinks/chart.tar.gz diff --git a/pkg/iac/scanners/helm/parser/parser.go b/pkg/iac/scanners/helm/parser/parser.go index c8bc8a73bedd..ddcfac09682f 100644 --- a/pkg/iac/scanners/helm/parser/parser.go +++ b/pkg/iac/scanners/helm/parser/parser.go @@ -22,7 +22,7 @@ import ( "helm.sh/helm/v3/pkg/releaseutil" "github.com/aquasecurity/trivy/pkg/iac/debug" - detection2 "github.com/aquasecurity/trivy/pkg/iac/detection" + "github.com/aquasecurity/trivy/pkg/iac/detection" "github.com/aquasecurity/trivy/pkg/iac/scanners/options" ) @@ -133,7 +133,7 @@ func (p *Parser) ParseFS(ctx context.Context, target fs.FS, path string) error { return nil } - if detection2.IsArchive(path) { + if detection.IsArchive(path) { tarFS, err := p.addTarToFS(path) if errors.Is(err, errSkipFS) { // an unpacked Chart already exists @@ -320,5 +320,5 @@ func (p *Parser) required(path string, workingFS fs.FS) bool { return false } - return detection2.IsType(path, bytes.NewReader(content), detection2.FileTypeHelm) + return detection.IsType(path, bytes.NewReader(content), detection.FileTypeHelm) } diff --git a/pkg/iac/scanners/helm/parser/parser_tar.go b/pkg/iac/scanners/helm/parser/parser_tar.go index 5455ab780683..5c073dad9875 100644 --- a/pkg/iac/scanners/helm/parser/parser_tar.go +++ b/pkg/iac/scanners/helm/parser/parser_tar.go @@ -2,13 +2,13 @@ package parser import ( "archive/tar" - "bytes" "compress/gzip" "errors" "fmt" "io" "io/fs" "os" + "path" "path/filepath" "github.com/liamg/memoryfs" @@ -18,10 +18,10 @@ import ( var errSkipFS = errors.New("skip parse FS") -func (p *Parser) addTarToFS(path string) (fs.FS, error) { +func (p *Parser) addTarToFS(archivePath string) (fs.FS, error) { tarFS := memoryfs.CloneFS(p.workingFS) - file, err := tarFS.Open(path) + file, err := tarFS.Open(archivePath) if err != nil { return nil, fmt.Errorf("failed to open tar: %w", err) } @@ -29,7 +29,7 @@ func (p *Parser) addTarToFS(path string) (fs.FS, error) { var tr *tar.Reader - if detection.IsZip(path) { + if detection.IsZip(archivePath) { zipped, err := gzip.NewReader(file) if err != nil { return nil, fmt.Errorf("failed to create gzip reader: %w", err) @@ -41,6 +41,7 @@ func (p *Parser) addTarToFS(path string) (fs.FS, error) { } checkExistedChart := true + symlinks := make(map[string]string) for { header, err := tr.Next() @@ -55,57 +56,108 @@ func (p *Parser) addTarToFS(path string) (fs.FS, error) { // Do not add archive files to FS if the chart already exists // This can happen when the source chart is located next to an archived chart (with the `helm package` command) // The first level folder in the archive is equal to the Chart name - if _, err := tarFS.Stat(filepath.Dir(path) + "/" + filepath.Dir(header.Name)); err == nil { + if _, err := tarFS.Stat(filepath.Dir(archivePath) + "/" + filepath.Dir(header.Name)); err == nil { return nil, errSkipFS } checkExistedChart = false } // get the individual path and extract to the current directory - entryPath := header.Name + targetPath := path.Join(filepath.Dir(archivePath), filepath.Clean(header.Name)) switch header.Typeflag { case tar.TypeDir: - if err := tarFS.MkdirAll(entryPath, os.FileMode(header.Mode)); err != nil && !errors.Is(err, fs.ErrExist) { + if err := tarFS.MkdirAll(targetPath, os.FileMode(header.Mode)); err != nil && !errors.Is(err, fs.ErrExist) { return nil, err } case tar.TypeReg: - writePath := filepath.Dir(path) + "/" + entryPath - p.debug.Log("Unpacking tar entry %s", writePath) - - _ = tarFS.MkdirAll(filepath.Dir(writePath), fs.ModePerm) - - buf, err := copyChunked(tr, 1024) - if err != nil { + p.debug.Log("Unpacking tar entry %s", targetPath) + if err := copyFile(tarFS, tr, targetPath); err != nil { return nil, err } - - p.debug.Log("writing file contents to %s", writePath) - if err := tarFS.WriteFile(writePath, buf.Bytes(), fs.ModePerm); err != nil { - return nil, fmt.Errorf("write file error: %w", err) + case tar.TypeSymlink: + if filepath.IsAbs(header.Linkname) { + p.debug.Log("Symlink %s is absolute, skipping", header.Linkname) + continue } + + symlinks[targetPath] = path.Join(filepath.Dir(targetPath), header.Linkname) default: return nil, fmt.Errorf("header type %q is not supported", header.Typeflag) } } - if err := tarFS.Remove(path); err != nil { - return nil, fmt.Errorf("failed to remove tar from FS: %w", err) + for target, link := range symlinks { + fi, err := tarFS.Stat(link) + if err != nil { + p.debug.Log("stat error: %s", err) + continue + } + if fi.IsDir() { + if err := copyDir(tarFS, link, target); err != nil { + return nil, fmt.Errorf("copy dir error: %w", err) + } + continue + } + + f, err := tarFS.Open(link) + if err != nil { + return nil, fmt.Errorf("open symlink error: %w", err) + } + + if err := copyFile(tarFS, f, target); err != nil { + f.Close() + return nil, fmt.Errorf("copy file error: %w", err) + } + f.Close() + } + + if err := tarFS.Remove(archivePath); err != nil { + return nil, fmt.Errorf("remove tar from FS error: %w", err) } return tarFS, nil } -func copyChunked(src io.Reader, chunkSize int64) (*bytes.Buffer, error) { - buf := new(bytes.Buffer) - for { - if _, err := io.CopyN(buf, src, chunkSize); err != nil { - if errors.Is(err, io.EOF) { - break - } - return nil, fmt.Errorf("failed to copy: %w", err) +func copyFile(fsys *memoryfs.FS, src io.Reader, dst string) error { + if err := fsys.MkdirAll(filepath.Dir(dst), fs.ModePerm); err != nil && !errors.Is(err, fs.ErrExist) { + return fmt.Errorf("mkdir error: %w", err) + } + + b, err := io.ReadAll(src) + if err != nil { + return fmt.Errorf("read error: %w", err) + } + + if err := fsys.WriteFile(dst, b, fs.ModePerm); err != nil { + return fmt.Errorf("write file error: %w", err) + } + + return nil +} + +func copyDir(fsys *memoryfs.FS, src string, dst string) error { + walkFn := func(filePath string, entry fs.DirEntry, err error) error { + if err != nil { + return err + } + + if entry.IsDir() { + return nil + } + + dst := path.Join(dst, filePath[len(src):]) + + f, err := fsys.Open(filePath) + if err != nil { + return err + } + + if err := copyFile(fsys, f, dst); err != nil { + return fmt.Errorf("copy file error: %w", err) } + return nil } - return buf, nil + return fs.WalkDir(fsys, src, walkFn) } diff --git a/pkg/iac/scanners/helm/parser/parser_test.go b/pkg/iac/scanners/helm/parser/parser_test.go index 030b0efbb86f..b9a58238280b 100644 --- a/pkg/iac/scanners/helm/parser/parser_test.go +++ b/pkg/iac/scanners/helm/parser/parser_test.go @@ -22,4 +22,26 @@ func TestParseFS(t *testing.T) { } assert.Equal(t, expectedFiles, p.filepaths) }) + + t.Run("archive with symlinks", func(t *testing.T) { + // mkdir -p chart && cd $_ + // touch Chart.yaml + // mkdir -p dir && cp -p Chart.yaml dir/Chart.yaml + // mkdir -p sym-to-file && ln -s ../Chart.yaml sym-to-file/Chart.yaml + // ln -s dir sym-to-dir + // cd .. && tar -czvf chart.tar.gz chart && rm -rf chart + p, err := New(".") + require.NoError(t, err) + + fsys := os.DirFS(filepath.Join("testdata", "archive-with-symlinks")) + require.NoError(t, p.ParseFS(context.TODO(), fsys, "chart.tar.gz")) + + expectedFiles := []string{ + "chart/Chart.yaml", + "chart/dir/Chart.yaml", + "chart/sym-to-dir/Chart.yaml", + "chart/sym-to-file/Chart.yaml", + } + assert.Equal(t, expectedFiles, p.filepaths) + }) } diff --git a/pkg/iac/scanners/helm/parser/testdata/archive-with-symlinks/chart.tar.gz b/pkg/iac/scanners/helm/parser/testdata/archive-with-symlinks/chart.tar.gz new file mode 100644 index 0000000000000000000000000000000000000000..4e9ee88c4789593b3684ba994378deeff869e10b GIT binary patch literal 247 zcmV-pK2gpEx6hiJiWQ3SF(oJh-==!3SRQhu# zS5vgZ>vgiMEZ;n5d5<;z3Hz)6-E>dKqNz3Zkn>=~5B&p+e?p?fd5vTqG5_NKP_>FZ zAg=yF{9^`I|36{&-;K?tKX2qS(AXos|1+@Ee^AQj4)cHWUsm-|p%29Q$EpA4o~{0W x!L$GG&jQi@1LQ3IKcuPhWRjTw_5ZJT((&^jhb*)C&oB%_lM6)8s~P|j006+Qf~Wuh literal 0 HcmV?d00001 From 9841b99d92b92b61e366184c440193961858ba91 Mon Sep 17 00:00:00 2001 From: nikpivkin Date: Fri, 3 May 2024 18:18:18 +0600 Subject: [PATCH 2/6] fix linter issues --- pkg/iac/scanners/helm/parser/parser_tar.go | 52 ++++++++++++---------- pkg/iac/scanners/helm/test/parser_test.go | 12 ++--- 2 files changed, 35 insertions(+), 29 deletions(-) diff --git a/pkg/iac/scanners/helm/parser/parser_tar.go b/pkg/iac/scanners/helm/parser/parser_tar.go index 5c073dad9875..ce118cb5e7e4 100644 --- a/pkg/iac/scanners/helm/parser/parser_tar.go +++ b/pkg/iac/scanners/helm/parser/parser_tar.go @@ -81,35 +81,16 @@ func (p *Parser) addTarToFS(archivePath string) (fs.FS, error) { continue } - symlinks[targetPath] = path.Join(filepath.Dir(targetPath), header.Linkname) + symlinks[targetPath] = path.Join(filepath.Dir(targetPath), header.Linkname) // nolint:gosec // virtual file system is used default: return nil, fmt.Errorf("header type %q is not supported", header.Typeflag) } } for target, link := range symlinks { - fi, err := tarFS.Stat(link) - if err != nil { - p.debug.Log("stat error: %s", err) - continue - } - if fi.IsDir() { - if err := copyDir(tarFS, link, target); err != nil { - return nil, fmt.Errorf("copy dir error: %w", err) - } - continue - } - - f, err := tarFS.Open(link) - if err != nil { - return nil, fmt.Errorf("open symlink error: %w", err) + if err := copySymlink(tarFS, link, target); err != nil { + return nil, fmt.Errorf("copy symlink error: %w", err) } - - if err := copyFile(tarFS, f, target); err != nil { - f.Close() - return nil, fmt.Errorf("copy file error: %w", err) - } - f.Close() } if err := tarFS.Remove(archivePath); err != nil { @@ -119,6 +100,31 @@ func (p *Parser) addTarToFS(archivePath string) (fs.FS, error) { return tarFS, nil } +func copySymlink(fsys *memoryfs.FS, src, dst string) error { + fi, err := fsys.Stat(src) + if err != nil { + return nil + } + if fi.IsDir() { + if err := copyDir(fsys, src, dst); err != nil { + return fmt.Errorf("copy dir error: %w", err) + } + return nil + } + + f, err := fsys.Open(src) + if err != nil { + return fmt.Errorf("open symlink error: %w", err) + } + defer f.Close() + + if err := copyFile(fsys, f, dst); err != nil { + return fmt.Errorf("copy file error: %w", err) + } + + return nil +} + func copyFile(fsys *memoryfs.FS, src io.Reader, dst string) error { if err := fsys.MkdirAll(filepath.Dir(dst), fs.ModePerm); err != nil && !errors.Is(err, fs.ErrExist) { return fmt.Errorf("mkdir error: %w", err) @@ -136,7 +142,7 @@ func copyFile(fsys *memoryfs.FS, src io.Reader, dst string) error { return nil } -func copyDir(fsys *memoryfs.FS, src string, dst string) error { +func copyDir(fsys *memoryfs.FS, src, dst string) error { walkFn := func(filePath string, entry fs.DirEntry, err error) error { if err != nil { return err diff --git a/pkg/iac/scanners/helm/test/parser_test.go b/pkg/iac/scanners/helm/test/parser_test.go index 85a69469fb5d..989590d4268f 100644 --- a/pkg/iac/scanners/helm/test/parser_test.go +++ b/pkg/iac/scanners/helm/test/parser_test.go @@ -114,13 +114,13 @@ func Test_tar_is_chart(t *testing.T) { t.Logf("Running test: %s", test.testName) testPath := filepath.Join("testdata", test.archiveFile) - file, err := os.Open(testPath) - defer func() { _ = file.Close() }() - require.NoError(t, err) - - assert.Equal(t, test.isHelmChart, detection.IsHelmChartArchive(test.archiveFile, file)) + func() { + file, err := os.Open(testPath) + require.NoError(t, err) + defer file.Close() - _ = file.Close() + assert.Equal(t, test.isHelmChart, detection.IsHelmChartArchive(test.archiveFile, file)) + }() } } From 01144a735ba69598f5ca8cc80f97d7519ef84486 Mon Sep 17 00:00:00 2001 From: nikpivkin Date: Mon, 6 May 2024 13:17:13 +0600 Subject: [PATCH 3/6] fix: convert path separator to slash --- pkg/iac/scanners/helm/parser/parser_tar.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/pkg/iac/scanners/helm/parser/parser_tar.go b/pkg/iac/scanners/helm/parser/parser_tar.go index ce118cb5e7e4..4c859e092174 100644 --- a/pkg/iac/scanners/helm/parser/parser_tar.go +++ b/pkg/iac/scanners/helm/parser/parser_tar.go @@ -52,18 +52,22 @@ func (p *Parser) addTarToFS(archivePath string) (fs.FS, error) { return nil, fmt.Errorf("failed to get next entry: %w", err) } + name := filepath.ToSlash(header.Name) + if checkExistedChart { // Do not add archive files to FS if the chart already exists // This can happen when the source chart is located next to an archived chart (with the `helm package` command) // The first level folder in the archive is equal to the Chart name - if _, err := tarFS.Stat(filepath.Dir(archivePath) + "/" + filepath.Dir(header.Name)); err == nil { + if _, err := tarFS.Stat(path.Dir(archivePath) + "/" + path.Dir(name)); err == nil { return nil, errSkipFS } checkExistedChart = false } // get the individual path and extract to the current directory - targetPath := path.Join(filepath.Dir(archivePath), filepath.Clean(header.Name)) + targetPath := path.Join(path.Dir(archivePath), path.Clean(name)) + + link := filepath.ToSlash(header.Linkname) switch header.Typeflag { case tar.TypeDir: @@ -76,12 +80,12 @@ func (p *Parser) addTarToFS(archivePath string) (fs.FS, error) { return nil, err } case tar.TypeSymlink: - if filepath.IsAbs(header.Linkname) { - p.debug.Log("Symlink %s is absolute, skipping", header.Linkname) + if path.IsAbs(link) { + p.debug.Log("Symlink %s is absolute, skipping", link) continue } - symlinks[targetPath] = path.Join(filepath.Dir(targetPath), header.Linkname) // nolint:gosec // virtual file system is used + symlinks[targetPath] = path.Join(path.Dir(targetPath), link) // nolint:gosec // virtual file system is used default: return nil, fmt.Errorf("header type %q is not supported", header.Typeflag) } @@ -126,7 +130,7 @@ func copySymlink(fsys *memoryfs.FS, src, dst string) error { } func copyFile(fsys *memoryfs.FS, src io.Reader, dst string) error { - if err := fsys.MkdirAll(filepath.Dir(dst), fs.ModePerm); err != nil && !errors.Is(err, fs.ErrExist) { + if err := fsys.MkdirAll(path.Dir(dst), fs.ModePerm); err != nil && !errors.Is(err, fs.ErrExist) { return fmt.Errorf("mkdir error: %w", err) } From 5155dbd9aca2431df54ba27461075772d8f31eb9 Mon Sep 17 00:00:00 2001 From: nikpivkin Date: Tue, 7 May 2024 10:18:26 +0600 Subject: [PATCH 4/6] test: run helm tests as subtests --- pkg/iac/scanners/helm/test/parser_test.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/pkg/iac/scanners/helm/test/parser_test.go b/pkg/iac/scanners/helm/test/parser_test.go index 989590d4268f..762905c2abd4 100644 --- a/pkg/iac/scanners/helm/test/parser_test.go +++ b/pkg/iac/scanners/helm/test/parser_test.go @@ -111,16 +111,14 @@ func Test_tar_is_chart(t *testing.T) { } for _, test := range tests { - - t.Logf("Running test: %s", test.testName) - testPath := filepath.Join("testdata", test.archiveFile) - func() { + t.Run(test.testName, func(t *testing.T) { + testPath := filepath.Join("testdata", test.archiveFile) file, err := os.Open(testPath) require.NoError(t, err) defer file.Close() assert.Equal(t, test.isHelmChart, detection.IsHelmChartArchive(test.archiveFile, file)) - }() + }) } } From e0bc1ebe759f65dad7b89e11175be0263bca8713 Mon Sep 17 00:00:00 2001 From: nikpivkin Date: Tue, 7 May 2024 11:13:27 +0600 Subject: [PATCH 5/6] test: add testcase with recursive symlinks --- pkg/iac/scanners/helm/parser/parser_test.go | 4 ++++ .../testdata/archive-with-symlinks/chart.tar.gz | Bin 247 -> 312 bytes 2 files changed, 4 insertions(+) diff --git a/pkg/iac/scanners/helm/parser/parser_test.go b/pkg/iac/scanners/helm/parser/parser_test.go index b9a58238280b..9c8b05ce7696 100644 --- a/pkg/iac/scanners/helm/parser/parser_test.go +++ b/pkg/iac/scanners/helm/parser/parser_test.go @@ -29,6 +29,8 @@ func TestParseFS(t *testing.T) { // mkdir -p dir && cp -p Chart.yaml dir/Chart.yaml // mkdir -p sym-to-file && ln -s ../Chart.yaml sym-to-file/Chart.yaml // ln -s dir sym-to-dir + // mkdir rec-sym && touch rec-sym/Chart.yaml + // ln -s . ./rec-sym/a // cd .. && tar -czvf chart.tar.gz chart && rm -rf chart p, err := New(".") require.NoError(t, err) @@ -39,6 +41,8 @@ func TestParseFS(t *testing.T) { expectedFiles := []string{ "chart/Chart.yaml", "chart/dir/Chart.yaml", + "chart/rec-sym/Chart.yaml", + "chart/rec-sym/a/Chart.yaml", "chart/sym-to-dir/Chart.yaml", "chart/sym-to-file/Chart.yaml", } diff --git a/pkg/iac/scanners/helm/parser/testdata/archive-with-symlinks/chart.tar.gz b/pkg/iac/scanners/helm/parser/testdata/archive-with-symlinks/chart.tar.gz index 4e9ee88c4789593b3684ba994378deeff869e10b..a3183710c17f5dd1d04935cfd2731450a83df2b5 100644 GIT binary patch literal 312 zcmV-80muFyiwFSaw>f421MQbxYQr!LfO8aIAenz;`2ahKNgGyZC^+duZ$Bjt9a+nm zVKR*B3)n9vR{U9t-G9V1zcs9At%LV!?J@V-Lhd(|0W|2G3)(4doCnt^&l^_eI?XXr zDD!2aXN>9kd>joMh9BOueaF53C-kfTbnM&dHZtvlBL5D_zc)^c{~Bcf?@Qa=;&|qm zgVqB9e&-)?^$&=R(j38jNBooj!znTT14{BAgCYOmte5<+LH19zznw2FZ3B)u%KQiN zk0wz5Tc&t+i2pDD!|8lvr~_sGVX6PY1j+vzJo^9pvp}(bYeJySA+fik7o~*_|Nx$YQ6Zc!QOvlnWLosZ})$<-pK2gpEx6hiJiWQ3SF(oJh-==!3SRQhu# zS5vgZ>vgiMEZ;n5d5<;z3Hz)6-E>dKqNz3Zkn>=~5B&p+e?p?fd5vTqG5_NKP_>FZ zAg=yF{9^`I|36{&-;K?tKX2qS(AXos|1+@Ee^AQj4)cHWUsm-|p%29Q$EpA4o~{0W x!L$GG&jQi@1LQ3IKcuPhWRjTw_5ZJT((&^jhb*)C&oB%_lM6)8s~P|j006+Qf~Wuh From cedb039693d41f31766d5944796677ad9ac373e1 Mon Sep 17 00:00:00 2001 From: nikpivkin Date: Tue, 7 May 2024 11:45:53 +0600 Subject: [PATCH 6/6] refactor: lazy writing symlinks --- pkg/iac/scanners/helm/parser/parser_tar.go | 28 ++++++++++++---------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/pkg/iac/scanners/helm/parser/parser_tar.go b/pkg/iac/scanners/helm/parser/parser_tar.go index 4c859e092174..f8d692eaa977 100644 --- a/pkg/iac/scanners/helm/parser/parser_tar.go +++ b/pkg/iac/scanners/helm/parser/parser_tar.go @@ -116,13 +116,7 @@ func copySymlink(fsys *memoryfs.FS, src, dst string) error { return nil } - f, err := fsys.Open(src) - if err != nil { - return fmt.Errorf("open symlink error: %w", err) - } - defer f.Close() - - if err := copyFile(fsys, f, dst); err != nil { + if err := copyFileLazy(fsys, src, dst); err != nil { return fmt.Errorf("copy file error: %w", err) } @@ -158,12 +152,7 @@ func copyDir(fsys *memoryfs.FS, src, dst string) error { dst := path.Join(dst, filePath[len(src):]) - f, err := fsys.Open(filePath) - if err != nil { - return err - } - - if err := copyFile(fsys, f, dst); err != nil { + if err := copyFileLazy(fsys, filePath, dst); err != nil { return fmt.Errorf("copy file error: %w", err) } return nil @@ -171,3 +160,16 @@ func copyDir(fsys *memoryfs.FS, src, dst string) error { return fs.WalkDir(fsys, src, walkFn) } + +func copyFileLazy(fsys *memoryfs.FS, src, dst string) error { + if err := fsys.MkdirAll(path.Dir(dst), fs.ModePerm); err != nil && !errors.Is(err, fs.ErrExist) { + return fmt.Errorf("mkdir error: %w", err) + } + return fsys.WriteLazyFile(dst, func() (io.Reader, error) { + f, err := fsys.Open(src) + if err != nil { + return nil, err + } + return f, nil + }, fs.ModePerm) +}