Add support for #excluding specific rules

holyspectral · Mar 2, 2018 · 1429033 · 1429033
1 parent 3713168
commit 1429033
Show file tree

Hide file tree

Showing 26 changed files with 287 additions and 93 deletions.
diff --git a/analyzer.go b/analyzer.go
@@ -25,6 +25,7 @@ import (
 	"os"
 	"path"
 	"reflect"
+	"regexp"
 	"strings"
 
 	"path/filepath"
@@ -43,6 +44,7 @@ type Context struct {
 	Root     *ast.File
 	Config   map[string]interface{}
 	Imports  *ImportTracker
+	Ignores  []map[string]bool
 }
 
 // Metrics used when reporting information about a scanning run.
@@ -87,9 +89,9 @@ func NewAnalyzer(conf Config, logger *log.Logger) *Analyzer {
 
 // LoadRules instantiates all the rules to be used when analyzing source
 // packages
-func (gas *Analyzer) LoadRules(ruleDefinitions ...RuleBuilder) {
-	for _, builder := range ruleDefinitions {
-		r, nodes := builder(gas.config)
+func (gas *Analyzer) LoadRules(ruleDefinitions map[string]RuleBuilder) {
+	for id, def := range ruleDefinitions {
+		r, nodes := def(id, gas.config)
 		gas.ruleset.Register(r, nodes...)
 	}
 }
@@ -147,41 +149,84 @@ func (gas *Analyzer) Process(packagePaths ...string) error {
 }
 
 // ignore a node (and sub-tree) if it is tagged with a "#nosec" comment
-func (gas *Analyzer) ignore(n ast.Node) bool {
+func (gas *Analyzer) ignore(n ast.Node) ([]string, bool) {
 	if groups, ok := gas.context.Comments[n]; ok && !gas.ignoreNosec {
 		for _, group := range groups {
 			if strings.Contains(group.Text(), "#nosec") {
 				gas.stats.NumNosec++
-				return true
+				return nil, true
+			}
+
+			if strings.Contains(group.Text(), "#exclude") {
+				gas.stats.NumNosec++
+
+				// Pull out the specific rules that are listed to be ignored.
+				re := regexp.MustCompile("!(G\\d{3})")
+				matches := re.FindAllStringSubmatch(group.Text(), -1)
+
+				// Find the rule IDs to ignore.
+				ignores := make([]string, 0)
+				for _, v := range matches {
+					ignores = append(ignores, v[1])
+				}
+				return ignores, false
 			}
 		}
 	}
-	return false
+	return nil, false
 }
 
 // Visit runs the GAS visitor logic over an AST created by parsing go code.
 // Rule methods added with AddRule will be invoked as necessary.
 func (gas *Analyzer) Visit(n ast.Node) ast.Visitor {
-	if !gas.ignore(n) {
+	// If we've reached the end of this branch, pop off the ignores stack.
+	if n == nil {
+		if len(gas.context.Ignores) > 0 {
+			gas.context.Ignores = gas.context.Ignores[1:]
+		}
+		return gas
+	}
 
-		// Track aliased and initialization imports
-		gas.context.Imports.TrackImport(n)
+	// Get any new rule exclusions.
+	ignoredRules, ignoreAll := gas.ignore(n)
+	if ignoreAll {
+		return nil
+	}
 
-		for _, rule := range gas.ruleset.RegisteredFor(n) {
-			issue, err := rule.Match(n, gas.context)
-			if err != nil {
-				file, line := GetLocation(n, gas.context)
-				file = path.Base(file)
-				gas.logger.Printf("Rule error: %v => %s (%s:%d)\n", reflect.TypeOf(rule), err, file, line)
-			}
-			if issue != nil {
-				gas.issues = append(gas.issues, issue)
-				gas.stats.NumFound++
-			}
+	// Now create the union of exclusions.
+	ignores := make(map[string]bool, 0)
+	if len(gas.context.Ignores) > 0 {
+		for k, v := range gas.context.Ignores[0] {
+			ignores[k] = v
 		}
-		return gas
 	}
-	return nil
+
+	for _, v := range ignoredRules {
+		ignores[v] = true
+	}
+
+	// Push the new set onto the stack.
+	gas.context.Ignores = append([]map[string]bool{ignores}, gas.context.Ignores...)
+
+	// Track aliased and initialization imports
+	gas.context.Imports.TrackImport(n)
+
+	for _, rule := range gas.ruleset.RegisteredFor(n) {
+		if _, ok := ignores[rule.ID()]; ok {
+			continue
+		}
+		issue, err := rule.Match(n, gas.context)
+		if err != nil {
+			file, line := GetLocation(n, gas.context)
+			file = path.Base(file)
+			gas.logger.Printf("Rule error: %v => %s (%s:%d)\n", reflect.TypeOf(rule), err, file, line)
+		}
+		if issue != nil {
+			gas.issues = append(gas.issues, issue)
+			gas.stats.NumFound++
+		}
+	}
+	return gas
 }
 
 // Report returns the current issues discovered and the metrics about the scan

diff --git a/analyzer_test.go b/analyzer_test.go
@@ -28,7 +28,7 @@ var _ = Describe("Analyzer", func() {
 	Context("when processing a package", func() {
 
 		It("should return an error if the package contains no Go files", func() {
-			analyzer.LoadRules(rules.Generate().Builders()...)
+			analyzer.LoadRules(rules.Generate().Builders())
 			dir, err := ioutil.TempDir("", "empty")
 			defer os.RemoveAll(dir)
 			Expect(err).ShouldNot(HaveOccurred())
@@ -38,7 +38,7 @@ var _ = Describe("Analyzer", func() {
 		})
 
 		It("should return an error if the package fails to build", func() {
-			analyzer.LoadRules(rules.Generate().Builders()...)
+			analyzer.LoadRules(rules.Generate().Builders())
 			pkg := testutils.NewTestPackage()
 			defer pkg.Close()
 			pkg.AddFile("wonky.go", `func main(){ println("forgot the package")}`)
@@ -51,7 +51,7 @@ var _ = Describe("Analyzer", func() {
 		})
 
 		It("should be able to analyze mulitple Go files", func() {
-			analyzer.LoadRules(rules.Generate().Builders()...)
+			analyzer.LoadRules(rules.Generate().Builders())
 			pkg := testutils.NewTestPackage()
 			defer pkg.Close()
 			pkg.AddFile("foo.go", `
@@ -72,7 +72,7 @@ var _ = Describe("Analyzer", func() {
 		})
 
 		It("should be able to analyze mulitple Go packages", func() {
-			analyzer.LoadRules(rules.Generate().Builders()...)
+			analyzer.LoadRules(rules.Generate().Builders())
 			pkg1 := testutils.NewTestPackage()
 			pkg2 := testutils.NewTestPackage()
 			defer pkg1.Close()
@@ -98,7 +98,7 @@ var _ = Describe("Analyzer", func() {
 			// Rule for MD5 weak crypto usage
 			sample := testutils.SampleCodeG401[0]
 			source := sample.Code
-			analyzer.LoadRules(rules.Generate(rules.NewRuleFilter(false, "G401")).Builders()...)
+			analyzer.LoadRules(rules.Generate(rules.NewRuleFilter(false, "G401")).Builders())
 
 			controlPackage := testutils.NewTestPackage()
 			defer controlPackage.Close()
@@ -114,7 +114,7 @@ var _ = Describe("Analyzer", func() {
 			// Rule for MD5 weak crypto usage
 			sample := testutils.SampleCodeG401[0]
 			source := sample.Code
-			analyzer.LoadRules(rules.Generate(rules.NewRuleFilter(false, "G401")).Builders()...)
+			analyzer.LoadRules(rules.Generate(rules.NewRuleFilter(false, "G401")).Builders())
 
 			nosecPackage := testutils.NewTestPackage()
 			defer nosecPackage.Close()
@@ -126,6 +126,57 @@ var _ = Describe("Analyzer", func() {
 			nosecIssues, _ := analyzer.Report()
 			Expect(nosecIssues).Should(BeEmpty())
 		})
+
+		It("should not report errors when an exclude comment is present for the correct rule", func() {
+			// Rule for MD5 weak crypto usage
+			sample := testutils.SampleCodeG401[0]
+			source := sample.Code
+			analyzer.LoadRules(rules.Generate(rules.NewRuleFilter(false, "G401")).Builders())
+
+			nosecPackage := testutils.NewTestPackage()
+			defer nosecPackage.Close()
+			nosecSource := strings.Replace(source, "h := md5.New()", "h := md5.New() // #exclude !G401", 1)
+			nosecPackage.AddFile("md5.go", nosecSource)
+			nosecPackage.Build()
+
+			analyzer.Process(nosecPackage.Path)
+			nosecIssues, _ := analyzer.Report()
+			Expect(nosecIssues).Should(BeEmpty())
+		})
+
+		It("should report errors when an exclude comment is present for a different rule", func() {
+			// Rule for MD5 weak crypto usage
+			sample := testutils.SampleCodeG401[0]
+			source := sample.Code
+			analyzer.LoadRules(rules.Generate(rules.NewRuleFilter(false, "G401")).Builders())
+
+			nosecPackage := testutils.NewTestPackage()
+			defer nosecPackage.Close()
+			nosecSource := strings.Replace(source, "h := md5.New()", "h := md5.New() // #exclude !G301", 1)
+			nosecPackage.AddFile("md5.go", nosecSource)
+			nosecPackage.Build()
+
+			analyzer.Process(nosecPackage.Path)
+			nosecIssues, _ := analyzer.Report()
+			Expect(nosecIssues).Should(HaveLen(sample.Errors))
+		})
+
+		It("should not report errors when an exclude comment is present for multiple rules, including the correct rule", func() {
+			// Rule for MD5 weak crypto usage
+			sample := testutils.SampleCodeG401[0]
+			source := sample.Code
+			analyzer.LoadRules(rules.Generate(rules.NewRuleFilter(false, "G401")).Builders())
+
+			nosecPackage := testutils.NewTestPackage()
+			defer nosecPackage.Close()
+			nosecSource := strings.Replace(source, "h := md5.New()", "h := md5.New() // #exclude !G301 !G401", 1)
+			nosecPackage.AddFile("md5.go", nosecSource)
+			nosecPackage.Build()
+
+			analyzer.Process(nosecPackage.Path)
+			nosecIssues, _ := analyzer.Report()
+			Expect(nosecIssues).Should(BeEmpty())
+		})
 	})
 
 	It("should be possible to overwrite nosec comments, and report issues", func() {
@@ -138,7 +189,7 @@ var _ = Describe("Analyzer", func() {
 		nosecIgnoreConfig := gas.NewConfig()
 		nosecIgnoreConfig.SetGlobal("nosec", "true")
 		customAnalyzer := gas.NewAnalyzer(nosecIgnoreConfig, logger)
-		customAnalyzer.LoadRules(rules.Generate(rules.NewRuleFilter(false, "G401")).Builders()...)
+		customAnalyzer.LoadRules(rules.Generate(rules.NewRuleFilter(false, "G401")).Builders())
 
 		nosecPackage := testutils.NewTestPackage()
 		defer nosecPackage.Close()

diff --git a/cmd/gas/main.go b/cmd/gas/main.go
@@ -206,7 +206,7 @@ func main() {
 
 	// Create the analyzer
 	analyzer := gas.NewAnalyzer(config, logger)
-	analyzer.LoadRules(ruleDefinitions.Builders()...)
+	analyzer.LoadRules(ruleDefinitions.Builders())
 
 	vendor := regexp.MustCompile(`[\\/]vendor([\\/]|$)`)
 

diff --git a/issue.go b/issue.go
@@ -47,6 +47,7 @@ type Issue struct {
 // MetaData is embedded in all GAS rules. The Severity, Confidence and What message
 // will be passed tbhrough to reported issues.
 type MetaData struct {
+	ID         string
 	Severity   Score
 	Confidence Score
 	What       string

diff --git a/issue_test.go b/issue_test.go
@@ -78,7 +78,7 @@ var _ = Describe("Issue", func() {
 
 			// Use SQL rule to check binary expr
 			cfg := gas.NewConfig()
-			rule, _ := rules.NewSQLStrConcat(cfg)
+			rule, _ := rules.NewSQLStrConcat("TEST", cfg)
 			issue, err := rule.Match(target, ctx)
 			Expect(err).ShouldNot(HaveOccurred())
 			Expect(issue).ShouldNot(BeNil())

diff --git a/rule.go b/rule.go
@@ -19,11 +19,12 @@ import (
 
 // The Rule interface used by all rules supported by GAS.
 type Rule interface {
+	ID() string
 	Match(ast.Node, *Context) (*Issue, error)
 }
 
 // RuleBuilder is used to register a rule definition with the analyzer
-type RuleBuilder func(c Config) (Rule, []ast.Node)
+type RuleBuilder func(id string, c Config) (Rule, []ast.Node)
 
 // A RuleSet maps lists of rules to the type of AST node they should be run on.
 // The anaylzer will only invoke rules contained in the list associated with the

diff --git a/rule_test.go b/rule_test.go
@@ -15,6 +15,10 @@ type mockrule struct {
 	callback func(n ast.Node, ctx *gas.Context) bool
 }
 
+func (m *mockrule) ID() string {
+	return "MOCK"
+}
+
 func (m *mockrule) Match(n ast.Node, ctx *gas.Context) (*gas.Issue, error) {
 	if m.callback(n, ctx) {
 		return m.issue, nil

diff --git a/rules/big.go b/rules/big.go
@@ -26,6 +26,10 @@ type usingBigExp struct {
 	calls []string
 }
 
+func (r *usingBigExp) ID() string {
+	return r.MetaData.ID
+}
+
 func (r *usingBigExp) Match(n ast.Node, c *gas.Context) (gi *gas.Issue, err error) {
 	if _, matched := gas.MatchCallByType(n, c, r.pkg, r.calls...); matched {
 		return gas.NewIssue(c, n, r.What, r.Severity, r.Confidence), nil
@@ -34,11 +38,12 @@ func (r *usingBigExp) Match(n ast.Node, c *gas.Context) (gi *gas.Issue, err erro
 }
 
 // NewUsingBigExp detects issues with modulus == 0 for Bignum
-func NewUsingBigExp(conf gas.Config) (gas.Rule, []ast.Node) {
+func NewUsingBigExp(id string, conf gas.Config) (gas.Rule, []ast.Node) {
 	return &usingBigExp{
 		pkg:   "*math/big.Int",
 		calls: []string{"Exp"},
 		MetaData: gas.MetaData{
+			ID:         id,
 			What:       "Use of math/big.Int.Exp function should be audited for modulus == 0",
 			Severity:   gas.Low,
 			Confidence: gas.High,

diff --git a/rules/bind.go b/rules/bind.go
@@ -28,6 +28,10 @@ type bindsToAllNetworkInterfaces struct {
 	pattern *regexp.Regexp
 }
 
+func (r *bindsToAllNetworkInterfaces) ID() string {
+	return r.MetaData.ID
+}
+
 func (r *bindsToAllNetworkInterfaces) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) {
 	callExpr := r.calls.ContainsCallExpr(n, c)
 	if callExpr == nil {
@@ -43,14 +47,15 @@ func (r *bindsToAllNetworkInterfaces) Match(n ast.Node, c *gas.Context) (*gas.Is
 
 // NewBindsToAllNetworkInterfaces detects socket connections that are setup to
 // listen on all network interfaces.
-func NewBindsToAllNetworkInterfaces(conf gas.Config) (gas.Rule, []ast.Node) {
+func NewBindsToAllNetworkInterfaces(id string, conf gas.Config) (gas.Rule, []ast.Node) {
 	calls := gas.NewCallList()
 	calls.Add("net", "Listen")
 	calls.Add("crypto/tls", "Listen")
 	return &bindsToAllNetworkInterfaces{
 		calls:   calls,
 		pattern: regexp.MustCompile(`^(0.0.0.0|:).*$`),
 		MetaData: gas.MetaData{
+			ID:         id,
 			Severity:   gas.Medium,
 			Confidence: gas.High,
 			What:       "Binds to all network interfaces",