From 81cda2f91fbe1bf4735feb55febcae03e697a92b Mon Sep 17 00:00:00 2001 From: Rahul Gadi Date: Tue, 20 Aug 2024 04:43:40 -0400 Subject: [PATCH] Allow excluding analyzers globally (#1180) * This change does not exclude analyzers for inline comment * Changed the expected issues count for G103, G109 samples for test. Previously G115 has been included in the issue count * Show analyzers IDs(G115, G602) in gosec usage help * See #1175 --- analyzer.go | 18 ++++-- analyzer_test.go | 41 ++++++++++++++ analyzers/analyzers_set.go | 38 +++++++++++++ analyzers/analyzers_test.go | 62 ++++++++++++++++++++ analyzers/analyzerslist.go | 94 +++++++++++++++++++++++++++++++ analyzers/anaylzers_suite_test.go | 13 +++++ analyzers/util.go | 8 --- cmd/gosec/main.go | 39 ++++++++++++- rules/rules_test.go | 8 --- testutils/g103_samples.go | 2 +- testutils/g109_samples.go | 4 +- 11 files changed, 301 insertions(+), 26 deletions(-) create mode 100644 analyzers/analyzers_set.go create mode 100644 analyzers/analyzers_test.go create mode 100644 analyzers/analyzerslist.go create mode 100644 analyzers/anaylzers_suite_test.go diff --git a/analyzer.go b/analyzer.go index 07498ce..bfa7e19 100644 --- a/analyzer.go +++ b/analyzer.go @@ -182,7 +182,7 @@ type Analyzer struct { showIgnored bool trackSuppressions bool concurrency int - analyzerList []*analysis.Analyzer + analyzerSet *analyzers.AnalyzerSet mu sync.Mutex } @@ -213,7 +213,7 @@ func NewAnalyzer(conf Config, tests bool, excludeGenerated bool, trackSuppressio concurrency: concurrency, excludeGenerated: excludeGenerated, trackSuppressions: trackSuppressions, - analyzerList: analyzers.BuildDefaultAnalyzers(), + analyzerSet: analyzers.NewAnalyzerSet(), } } @@ -236,6 +236,15 @@ func (gosec *Analyzer) LoadRules(ruleDefinitions map[string]RuleBuilder, ruleSup } } +// LoadAnalyzers instantiates all the analyzers to be used when analyzing source +// packages +func (gosec *Analyzer) LoadAnalyzers(analyzerDefinitions map[string]analyzers.AnalyzerDefinition, analyzerSuppressed map[string]bool) { + for id, def := range analyzerDefinitions { + r := def.Create(def.ID, def.Description) + gosec.analyzerSet.Register(r, analyzerSuppressed[id]) + } +} + // Process kicks off the analysis process for a given package func (gosec *Analyzer) Process(buildTags []string, packagePaths ...string) error { config := &packages.Config{ @@ -415,7 +424,7 @@ func (gosec *Analyzer) CheckAnalyzers(pkg *packages.Package) { generatedFiles := gosec.generatedFiles(pkg) - for _, analyzer := range gosec.analyzerList { + for _, analyzer := range gosec.analyzerSet.Analyzers { pass := &analysis.Pass{ Analyzer: analyzer, Fset: pkg.Fset, @@ -666,7 +675,7 @@ func (gosec *Analyzer) getSuppressionsAtLineInFile(file string, line string, id suppressions := append(generalSuppressions, ruleSuppressions...) // Track external suppressions of this rule. - if gosec.ruleset.IsRuleSuppressed(id) { + if gosec.ruleset.IsRuleSuppressed(id) || gosec.analyzerSet.IsSuppressed(id) { ignored = true suppressions = append(suppressions, issue.SuppressionInfo{ Kind: "external", @@ -705,4 +714,5 @@ func (gosec *Analyzer) Reset() { gosec.issues = make([]*issue.Issue, 0, 16) gosec.stats = &Metrics{} gosec.ruleset = NewRuleSet() + gosec.analyzerSet = analyzers.NewAnalyzerSet() } diff --git a/analyzer_test.go b/analyzer_test.go index e3a1260..30ca170 100644 --- a/analyzer_test.go +++ b/analyzer_test.go @@ -24,6 +24,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/securego/gosec/v2" + "github.com/securego/gosec/v2/analyzers" "github.com/securego/gosec/v2/rules" "github.com/securego/gosec/v2/testutils" "golang.org/x/tools/go/packages" @@ -1110,6 +1111,7 @@ var _ = Describe("Analyzer", func() { It("should be able to scan generated files if NOT excluded when using the analyzes", func() { customAnalyzer := gosec.NewAnalyzer(nil, true, false, false, 1, logger) customAnalyzer.LoadRules(rules.Generate(false).RulesInfo()) + customAnalyzer.LoadAnalyzers(analyzers.Generate(false).AnalyzersInfo()) pkg := testutils.NewTestPackage() defer pkg.Close() pkg.AddFile("foo.go", ` @@ -1132,6 +1134,7 @@ var _ = Describe("Analyzer", func() { It("should be able to skip generated files if excluded when using the analyzes", func() { customAnalyzer := gosec.NewAnalyzer(nil, true, true, false, 1, logger) customAnalyzer.LoadRules(rules.Generate(false).RulesInfo()) + customAnalyzer.LoadAnalyzers(analyzers.Generate(false).AnalyzersInfo()) pkg := testutils.NewTestPackage() defer pkg.Close() pkg.AddFile("foo.go", ` @@ -1499,6 +1502,44 @@ var _ = Describe("Analyzer", func() { Expect(issues[0].Suppressions[0].Justification).To(Equal("Globally suppressed.")) }) + It("should not report an error if the analyzer is not included", func() { + sample := testutils.SampleCodeG602[0] + source := sample.Code[0] + analyzer.LoadAnalyzers(analyzers.Generate(true, analyzers.NewAnalyzerFilter(false, "G115")).AnalyzersInfo()) + + controlPackage := testutils.NewTestPackage() + defer controlPackage.Close() + controlPackage.AddFile("cipher.go", source) + err := controlPackage.Build() + Expect(err).ShouldNot(HaveOccurred()) + err = analyzer.Process(buildTags, controlPackage.Path) + Expect(err).ShouldNot(HaveOccurred()) + controlIssues, _, _ := analyzer.Report() + Expect(controlIssues).Should(HaveLen(sample.Errors)) + Expect(controlIssues[0].Suppressions).To(HaveLen(1)) + Expect(controlIssues[0].Suppressions[0].Kind).To(Equal("external")) + Expect(controlIssues[0].Suppressions[0].Justification).To(Equal("Globally suppressed.")) + }) + + It("should not report an error if the analyzer is excluded", func() { + sample := testutils.SampleCodeG602[0] + source := sample.Code[0] + analyzer.LoadAnalyzers(analyzers.Generate(true, analyzers.NewAnalyzerFilter(true, "G602")).AnalyzersInfo()) + + controlPackage := testutils.NewTestPackage() + defer controlPackage.Close() + controlPackage.AddFile("cipher.go", source) + err := controlPackage.Build() + Expect(err).ShouldNot(HaveOccurred()) + err = analyzer.Process(buildTags, controlPackage.Path) + Expect(err).ShouldNot(HaveOccurred()) + issues, _, _ := analyzer.Report() + Expect(issues).Should(HaveLen(sample.Errors)) + Expect(issues[0].Suppressions).To(HaveLen(1)) + Expect(issues[0].Suppressions[0].Kind).To(Equal("external")) + Expect(issues[0].Suppressions[0].Justification).To(Equal("Globally suppressed.")) + }) + It("should track multiple suppressions if the violation is multiply suppressed", func() { sample := testutils.SampleCodeG101[0] source := sample.Code[0] diff --git a/analyzers/analyzers_set.go b/analyzers/analyzers_set.go new file mode 100644 index 0000000..e2fe51c --- /dev/null +++ b/analyzers/analyzers_set.go @@ -0,0 +1,38 @@ +// (c) Copyright gosec's authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package analyzers + +import "golang.org/x/tools/go/analysis" + +type AnalyzerSet struct { + Analyzers []*analysis.Analyzer + AnalyzerSuppressedMap map[string]bool +} + +// NewAnalyzerSet constructs a new AnalyzerSet +func NewAnalyzerSet() *AnalyzerSet { + return &AnalyzerSet{nil, make(map[string]bool)} +} + +// Register adds a trigger for the supplied analyzer +func (a *AnalyzerSet) Register(analyzer *analysis.Analyzer, isSuppressed bool) { + a.Analyzers = append(a.Analyzers, analyzer) + a.AnalyzerSuppressedMap[analyzer.Name] = isSuppressed +} + +// IsSuppressed will return whether the Analyzer is suppressed. +func (a *AnalyzerSet) IsSuppressed(ruleID string) bool { + return a.AnalyzerSuppressedMap[ruleID] +} diff --git a/analyzers/analyzers_test.go b/analyzers/analyzers_test.go new file mode 100644 index 0000000..898040b --- /dev/null +++ b/analyzers/analyzers_test.go @@ -0,0 +1,62 @@ +package analyzers_test + +import ( + "fmt" + "log" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/securego/gosec/v2" + "github.com/securego/gosec/v2/analyzers" + "github.com/securego/gosec/v2/testutils" +) + +var _ = Describe("gosec analyzers", func() { + var ( + logger *log.Logger + config gosec.Config + analyzer *gosec.Analyzer + runner func(string, []testutils.CodeSample) + buildTags []string + tests bool + ) + + BeforeEach(func() { + logger, _ = testutils.NewLogger() + config = gosec.NewConfig() + analyzer = gosec.NewAnalyzer(config, tests, false, false, 1, logger) + runner = func(analyzerId string, samples []testutils.CodeSample) { + for n, sample := range samples { + analyzer.Reset() + analyzer.SetConfig(sample.Config) + analyzer.LoadAnalyzers(analyzers.Generate(false, analyzers.NewAnalyzerFilter(false, analyzerId)).AnalyzersInfo()) + pkg := testutils.NewTestPackage() + defer pkg.Close() + for i, code := range sample.Code { + pkg.AddFile(fmt.Sprintf("sample_%d_%d.go", n, i), code) + } + err := pkg.Build() + Expect(err).ShouldNot(HaveOccurred()) + Expect(pkg.PrintErrors()).Should(BeZero()) + err = analyzer.Process(buildTags, pkg.Path) + Expect(err).ShouldNot(HaveOccurred()) + issues, _, _ := analyzer.Report() + if len(issues) != sample.Errors { + fmt.Println(sample.Code) + } + Expect(issues).Should(HaveLen(sample.Errors)) + } + } + }) + + Context("report correct errors for all samples", func() { + It("should detect integer conversion overflow", func() { + runner("G115", testutils.SampleCodeG115) + }) + + It("should detect out of bounds slice access", func() { + runner("G602", testutils.SampleCodeG602) + }) + }) +}) diff --git a/analyzers/analyzerslist.go b/analyzers/analyzerslist.go new file mode 100644 index 0000000..4bf9ca9 --- /dev/null +++ b/analyzers/analyzerslist.go @@ -0,0 +1,94 @@ +// (c) Copyright gosec's authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package analyzers + +import ( + "golang.org/x/tools/go/analysis" +) + +// AnalyzerDefinition contains the description of an analyzer and a mechanism to +// create it. +type AnalyzerDefinition struct { + ID string + Description string + Create AnalyzerBuilder +} + +// AnalyzerBuilder is used to register an analyzer definition with the analyzer +type AnalyzerBuilder func(id string, description string) *analysis.Analyzer + +// AnalyzerList contains a mapping of analyzer ID's to analyzer definitions and a mapping +// of analyzer ID's to whether analyzers are suppressed. +type AnalyzerList struct { + Analyzers map[string]AnalyzerDefinition + AnalyzerSuppressed map[string]bool +} + +// AnalyzersInfo returns all the create methods and the analyzer suppressed map for a +// given list +func (al *AnalyzerList) AnalyzersInfo() (map[string]AnalyzerDefinition, map[string]bool) { + builders := make(map[string]AnalyzerDefinition) + for _, def := range al.Analyzers { + builders[def.ID] = def + } + return builders, al.AnalyzerSuppressed +} + +// AnalyzerFilter can be used to include or exclude an analyzer depending on the return +// value of the function +type AnalyzerFilter func(string) bool + +// NewAnalyzerFilter is a closure that will include/exclude the analyzer ID's based on +// the supplied boolean value. +func NewAnalyzerFilter(action bool, analyzerIDs ...string) AnalyzerFilter { + analyzerlist := make(map[string]bool) + for _, analyzer := range analyzerIDs { + analyzerlist[analyzer] = true + } + return func(analyzer string) bool { + if _, found := analyzerlist[analyzer]; found { + return action + } + return !action + } +} + +var defaultAnalyzers = []AnalyzerDefinition{ + {"G115", "Type conversion which leads to integer overflow", newConversionOverflowAnalyzer}, + {"G602", "Possible slice bounds out of range", newSliceBoundsAnalyzer}, +} + +// Generate the list of analyzers to use +func Generate(trackSuppressions bool, filters ...AnalyzerFilter) *AnalyzerList { + analyzerMap := make(map[string]AnalyzerDefinition) + analyzerSuppressedMap := make(map[string]bool) + + for _, analyzer := range defaultAnalyzers { + analyzerSuppressedMap[analyzer.ID] = false + addToAnalyzerList := true + for _, filter := range filters { + if filter(analyzer.ID) { + analyzerSuppressedMap[analyzer.ID] = true + if !trackSuppressions { + addToAnalyzerList = false + } + } + } + if addToAnalyzerList { + analyzerMap[analyzer.ID] = analyzer + } + } + return &AnalyzerList{Analyzers: analyzerMap, AnalyzerSuppressed: analyzerSuppressedMap} +} diff --git a/analyzers/anaylzers_suite_test.go b/analyzers/anaylzers_suite_test.go new file mode 100644 index 0000000..32d7301 --- /dev/null +++ b/analyzers/anaylzers_suite_test.go @@ -0,0 +1,13 @@ +package analyzers_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestAnalyzers(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Analyzers Suite") +} diff --git a/analyzers/util.go b/analyzers/util.go index 49e49e6..e88afe7 100644 --- a/analyzers/util.go +++ b/analyzers/util.go @@ -35,14 +35,6 @@ type SSAAnalyzerResult struct { SSA *buildssa.SSA } -// BuildDefaultAnalyzers returns the default list of analyzers -func BuildDefaultAnalyzers() []*analysis.Analyzer { - return []*analysis.Analyzer{ - newConversionOverflowAnalyzer("G115", "Type conversion which leads to integer overflow"), - newSliceBoundsAnalyzer("G602", "Possible slice bounds out of range"), - } -} - // getSSAResult retrieves the SSA result from analysis pass func getSSAResult(pass *analysis.Pass) (*SSAAnalyzerResult, error) { result, ok := pass.ResultOf[buildssa.Analyzer] diff --git a/cmd/gosec/main.go b/cmd/gosec/main.go index 62424bb..efc7f5d 100644 --- a/cmd/gosec/main.go +++ b/cmd/gosec/main.go @@ -25,6 +25,7 @@ import ( "strings" "github.com/securego/gosec/v2" + "github.com/securego/gosec/v2/analyzers" "github.com/securego/gosec/v2/autofix" "github.com/securego/gosec/v2/cmd/vflag" "github.com/securego/gosec/v2/issue" @@ -177,14 +178,23 @@ func usage() { // sorted rule list for ease of reading rl := rules.Generate(*flagTrackSuppressions) - keys := make([]string, 0, len(rl.Rules)) + al := analyzers.Generate(*flagTrackSuppressions) + keys := make([]string, 0, len(rl.Rules)+len(al.Analyzers)) for key := range rl.Rules { keys = append(keys, key) } + for key := range al.Analyzers { + keys = append(keys, key) + } sort.Strings(keys) for _, k := range keys { - v := rl.Rules[k] - fmt.Fprintf(os.Stderr, "\t%s: %s\n", k, v.Description) + var description string + if rule, ok := rl.Rules[k]; ok { + description = rule.Description + } else if analyzer, ok := al.Analyzers[k]; ok { + description = analyzer.Description + } + fmt.Fprintf(os.Stderr, "\t%s: %s\n", k, description) } fmt.Fprint(os.Stderr, "\n") } @@ -245,6 +255,26 @@ func loadRules(include, exclude string) rules.RuleList { return rules.Generate(*flagTrackSuppressions, filters...) } +func loadAnalyzers(include, exclude string) *analyzers.AnalyzerList { + var filters []analyzers.AnalyzerFilter + if include != "" { + logger.Printf("Including analyzers: %s", include) + including := strings.Split(include, ",") + filters = append(filters, analyzers.NewAnalyzerFilter(false, including...)) + } else { + logger.Println("Including analyzers: default") + } + + if exclude != "" { + logger.Printf("Excluding analyzers: %s", exclude) + excluding := strings.Split(exclude, ",") + filters = append(filters, analyzers.NewAnalyzerFilter(true, excluding...)) + } else { + logger.Println("Excluding analyzers: default") + } + return analyzers.Generate(*flagTrackSuppressions, filters...) +} + func getRootPaths(paths []string) []string { rootPaths := make([]string, 0) for _, path := range paths { @@ -412,9 +442,12 @@ func main() { logger.Fatal("No rules are configured") } + analyzerList := loadAnalyzers(includeRules, excludeRules) + // Create the analyzer analyzer := gosec.NewAnalyzer(config, *flagScanTests, *flagExcludeGenerated, *flagTrackSuppressions, *flagConcurrency, logger) analyzer.LoadRules(ruleList.RulesInfo()) + analyzer.LoadAnalyzers(analyzerList.AnalyzersInfo()) excludedDirs := gosec.ExcludedDirsRegExp(flagDirsExclude) var packages []string diff --git a/rules/rules_test.go b/rules/rules_test.go index 63b263f..9a7d65a 100644 --- a/rules/rules_test.go +++ b/rules/rules_test.go @@ -111,10 +111,6 @@ var _ = Describe("gosec rules", func() { runner("G114", testutils.SampleCodeG114) }) - It("should detect integer conversion overflow", func() { - runner("G115", testutils.SampleCodeG115) - }) - It("should detect sql injection via format strings", func() { runner("G201", testutils.SampleCodeG201) }) @@ -225,9 +221,5 @@ var _ = Describe("gosec rules", func() { runner("G601", testutils.SampleCodeG601) } }) - - It("should detect out of bounds slice access", func() { - runner("G602", testutils.SampleCodeG602) - }) }) }) diff --git a/testutils/g103_samples.go b/testutils/g103_samples.go index d1cb38d..feeb6b6 100644 --- a/testutils/g103_samples.go +++ b/testutils/g103_samples.go @@ -27,7 +27,7 @@ func main() { intPtr = (*int)(unsafe.Pointer(addressHolder)) fmt.Printf("\nintPtr=%p, *intPtr=%d.\n\n", intPtr, *intPtr) } -`}, 3, gosec.NewConfig()}, +`}, 2, gosec.NewConfig()}, {[]string{` package main diff --git a/testutils/g109_samples.go b/testutils/g109_samples.go index e05a2bd..9dc7841 100644 --- a/testutils/g109_samples.go +++ b/testutils/g109_samples.go @@ -20,7 +20,7 @@ func main() { value := int32(bigValue) fmt.Println(value) } -`}, 2, gosec.NewConfig()}, +`}, 1, gosec.NewConfig()}, {[]string{` package main @@ -38,7 +38,7 @@ func main() { fmt.Println(bigValue) } } -`}, 2, gosec.NewConfig()}, +`}, 1, gosec.NewConfig()}, {[]string{` package main