From 99b2db3978562689cef956a71281abb84ff0ce47 Mon Sep 17 00:00:00 2001 From: Nikita Pivkin Date: Fri, 8 Nov 2024 07:21:49 +0600 Subject: [PATCH] fix(misconf): handle null properties in CloudFormation templates (#7813) Signed-off-by: nikpivkin --- .../cloudformation/parser/file_context.go | 10 ++++ .../scanners/cloudformation/parser/parser.go | 33 +++++++------ .../cloudformation/parser/parser_test.go | 49 +++++++++++++++++++ 3 files changed, 77 insertions(+), 15 deletions(-) diff --git a/pkg/iac/scanners/cloudformation/parser/file_context.go b/pkg/iac/scanners/cloudformation/parser/file_context.go index 949add1ca7c4..e1c8cfa87f40 100644 --- a/pkg/iac/scanners/cloudformation/parser/file_context.go +++ b/pkg/iac/scanners/cloudformation/parser/file_context.go @@ -1,6 +1,8 @@ package parser import ( + "github.com/samber/lo" + "github.com/aquasecurity/trivy/pkg/iac/ignore" iacTypes "github.com/aquasecurity/trivy/pkg/iac/types" ) @@ -71,3 +73,11 @@ func (t *FileContext) missingParameterValues() []string { } return missing } + +func (t *FileContext) stripNullProperties() { + for _, resource := range t.Resources { + resource.Inner.Properties = lo.OmitBy(resource.Inner.Properties, func(k string, v *Property) bool { + return v.IsNil() + }) + } +} diff --git a/pkg/iac/scanners/cloudformation/parser/parser.go b/pkg/iac/scanners/cloudformation/parser/parser.go index 696dfcf16349..24d85ba4bbbb 100644 --- a/pkg/iac/scanners/cloudformation/parser/parser.go +++ b/pkg/iac/scanners/cloudformation/parser/parser.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "io/fs" + "path" "path/filepath" "strings" @@ -83,7 +84,7 @@ func (p *Parser) ParseFS(ctx context.Context, fsys fs.FS, dir string) (FileConte return contexts, nil } -func (p *Parser) ParseFile(ctx context.Context, fsys fs.FS, path string) (fctx *FileContext, err error) { +func (p *Parser) ParseFile(ctx context.Context, fsys fs.FS, filePath string) (fctx *FileContext, err error) { defer func() { if e := recover(); e != nil { err = fmt.Errorf("panic during parse: %s", e) @@ -105,15 +106,15 @@ func (p *Parser) ParseFile(ctx context.Context, fsys fs.FS, path string) (fctx * } sourceFmt := YamlSourceFormat - if strings.HasSuffix(strings.ToLower(path), ".json") { + if path.Ext(filePath) == ".json" { sourceFmt = JsonSourceFormat } - f, err := fsys.Open(filepath.ToSlash(path)) + f, err := fsys.Open(filePath) if err != nil { return nil, err } - defer func() { _ = f.Close() }() + defer f.Close() content, err := io.ReadAll(f) if err != nil { @@ -123,7 +124,7 @@ func (p *Parser) ParseFile(ctx context.Context, fsys fs.FS, path string) (fctx * lines := strings.Split(string(content), "\n") fctx = &FileContext{ - filepath: path, + filepath: filePath, lines: lines, SourceFormat: sourceFmt, } @@ -131,26 +132,28 @@ func (p *Parser) ParseFile(ctx context.Context, fsys fs.FS, path string) (fctx * switch sourceFmt { case YamlSourceFormat: if err := yaml.Unmarshal(content, fctx); err != nil { - return nil, NewErrInvalidContent(path, err) + return nil, NewErrInvalidContent(filePath, err) } - fctx.Ignores = ignore.Parse(string(content), path, "") + fctx.Ignores = ignore.Parse(string(content), filePath, "") case JsonSourceFormat: if err := jfather.Unmarshal(content, fctx); err != nil { - return nil, NewErrInvalidContent(path, err) + return nil, NewErrInvalidContent(filePath, err) } } + fctx.stripNullProperties() + fctx.overrideParameters(p.overridedParameters) if params := fctx.missingParameterValues(); len(params) > 0 { - p.logger.Warn("Missing parameter values", log.FilePath(path), log.String("parameters", strings.Join(params, ", "))) + p.logger.Warn("Missing parameter values", log.FilePath(filePath), log.String("parameters", strings.Join(params, ", "))) } fctx.lines = lines fctx.SourceFormat = sourceFmt - fctx.filepath = path + fctx.filepath = filePath - p.logger.Debug("Context loaded from source", log.FilePath(path)) + p.logger.Debug("Context loaded from source", log.FilePath(filePath)) // the context must be set to conditions before resources for _, c := range fctx.Conditions { @@ -158,7 +161,7 @@ func (p *Parser) ParseFile(ctx context.Context, fsys fs.FS, path string) (fctx * } for name, r := range fctx.Resources { - r.configureResource(name, fsys, path, fctx) + r.configureResource(name, fsys, filePath, fctx) } return fctx, nil @@ -190,10 +193,10 @@ func (p *Parser) parseParams() error { return nil } -func (p *Parser) parseParametersFile(path string) (Parameters, error) { - f, err := p.configsFS.Open(path) +func (p *Parser) parseParametersFile(filePath string) (Parameters, error) { + f, err := p.configsFS.Open(filePath) if err != nil { - return nil, fmt.Errorf("parameters file %q open error: %w", path, err) + return nil, fmt.Errorf("parameters file %q open error: %w", filePath, err) } var parameters Parameters diff --git a/pkg/iac/scanners/cloudformation/parser/parser_test.go b/pkg/iac/scanners/cloudformation/parser/parser_test.go index aa058c4df855..99dd62294eee 100644 --- a/pkg/iac/scanners/cloudformation/parser/parser_test.go +++ b/pkg/iac/scanners/cloudformation/parser/parser_test.go @@ -440,3 +440,52 @@ Conditions: require.NoError(t, err) require.Len(t, files, 1) } + +func Test_TemplateWithNullProperty(t *testing.T) { + src := `AWSTemplateFormatVersion: "2010-09-09" +Resources: + TestBucket: + Type: "AWS::S3::Bucket" + Properties: + BucketName:` + + fsys := testutil.CreateFS(t, map[string]string{ + "main.yaml": src, + }) + + files, err := New().ParseFS(context.TODO(), fsys, ".") + require.NoError(t, err) + require.Len(t, files, 1) + + file := files[0] + + res := file.GetResourceByLogicalID("TestBucket") + + assert.True(t, res.GetProperty("BucketName").IsNil()) +} + +func Test_TemplateWithNullNestedProperty(t *testing.T) { + src := `AWSTemplateFormatVersion: "2010-09-09" +Description: "BAD" +Resources: + TestBucket: + Type: "AWS::S3::Bucket" + Properties: + BucketName: test + PublicAccessBlockConfiguration: + BlockPublicAcls: null` + + fsys := testutil.CreateFS(t, map[string]string{ + "main.yaml": src, + }) + + files, err := New().ParseFS(context.TODO(), fsys, ".") + require.NoError(t, err) + require.Len(t, files, 1) + + file := files[0] + + res := file.GetResourceByLogicalID("TestBucket") + + assert.True(t, res.GetProperty("PublicAccessBlockConfiguration.BlockPublicAcls").IsNil()) +}