Add the rule ID to issues (securego#188)

ccojocar · Mar 12, 2018 · 2115402 · 2115402
1 parent a036755
commit 2115402
Show file tree

Hide file tree

Showing 19 changed files with 33 additions and 31 deletions.
diff --git a/issue.go b/issue.go
@@ -38,6 +38,7 @@ const (
 type Issue struct {
 	Severity   Score  `json:"severity"`   // issue severity (how problematic it is)
 	Confidence Score  `json:"confidence"` // issue confidence (how sure we are we found it)
+	RuleID     string `json:"rule_id"`    // Human readable explanation
 	What       string `json:"details"`    // Human readable explanation
 	File       string `json:"file"`       // File name we found it in
 	Code       string `json:"code"`       // Impacted code line
@@ -87,7 +88,7 @@ func codeSnippet(file *os.File, start int64, end int64, n ast.Node) (string, err
 }
 
 // NewIssue creates a new Issue
-func NewIssue(ctx *Context, node ast.Node, desc string, severity Score, confidence Score) *Issue {
+func NewIssue(ctx *Context, node ast.Node, ruleID, desc string, severity Score, confidence Score) *Issue {
 	var code string
 	fobj := ctx.FileSet.File(node.Pos())
 	name := fobj.Name()
@@ -112,6 +113,7 @@ func NewIssue(ctx *Context, node ast.Node, desc string, severity Score, confiden
 	return &Issue{
 		File:       name,
 		Line:       line,
+		RuleID:     ruleID,
 		What:       desc,
 		Confidence: confidence,
 		Severity:   severity,

diff --git a/issue_test.go b/issue_test.go
@@ -37,7 +37,7 @@ var _ = Describe("Issue", func() {
 			ast.Walk(v, ctx.Root)
 			Expect(target).ShouldNot(BeNil())
 
-			issue := gas.NewIssue(ctx, target, "", gas.High, gas.High)
+			issue := gas.NewIssue(ctx, target, "TEST", "", gas.High, gas.High)
 			Expect(issue).ShouldNot(BeNil())
 			Expect(issue.Code).Should(MatchRegexp(`"bar"`))
 			Expect(issue.Line).Should(Equal("2"))

diff --git a/rules/big.go b/rules/big.go
@@ -32,7 +32,7 @@ func (r *usingBigExp) ID() string {
 
 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
+		return gas.NewIssue(c, n, r.ID(), r.What, r.Severity, r.Confidence), nil
 	}
 	return nil, nil
 }

diff --git a/rules/bind.go b/rules/bind.go
@@ -39,7 +39,7 @@ func (r *bindsToAllNetworkInterfaces) Match(n ast.Node, c *gas.Context) (*gas.Is
 	}
 	if arg, err := gas.GetString(callExpr.Args[1]); err == nil {
 		if r.pattern.MatchString(arg) {
-			return gas.NewIssue(c, n, r.What, r.Severity, r.Confidence), nil
+			return gas.NewIssue(c, n, r.ID(), r.What, r.Severity, r.Confidence), nil
 		}
 	}
 	return nil, nil

diff --git a/rules/blacklist.go b/rules/blacklist.go
@@ -39,7 +39,7 @@ func (r *blacklistedImport) ID() string {
 func (r *blacklistedImport) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) {
 	if node, ok := n.(*ast.ImportSpec); ok {
 		if description, ok := r.Blacklisted[unquote(node.Path.Value)]; ok {
-			return gas.NewIssue(c, node, description, r.Severity, r.Confidence), nil
+			return gas.NewIssue(c, node, r.ID(), description, r.Severity, r.Confidence), nil
 		}
 	}
 	return nil, nil

diff --git a/rules/errors.go b/rules/errors.go
@@ -59,15 +59,15 @@ func (r *noErrorCheck) Match(n ast.Node, ctx *gas.Context) (*gas.Issue, error) {
 					return nil, nil
 				}
 				if id, ok := stmt.Lhs[pos].(*ast.Ident); ok && id.Name == "_" {
-					return gas.NewIssue(ctx, n, r.What, r.Severity, r.Confidence), nil
+					return gas.NewIssue(ctx, n, r.ID(), r.What, r.Severity, r.Confidence), nil
 				}
 			}
 		}
 	case *ast.ExprStmt:
 		if callExpr, ok := stmt.X.(*ast.CallExpr); ok && r.whitelist.ContainsCallExpr(stmt.X, ctx) == nil {
 			pos := returnsError(callExpr, ctx)
 			if pos >= 0 {
-				return gas.NewIssue(ctx, n, r.What, r.Severity, r.Confidence), nil
+				return gas.NewIssue(ctx, n, r.ID(), r.What, r.Severity, r.Confidence), nil
 			}
 		}
 	}

diff --git a/rules/fileperms.go b/rules/fileperms.go
@@ -54,7 +54,7 @@ func (r *filePermissions) Match(n ast.Node, c *gas.Context) (*gas.Issue, error)
 	if callexpr, matched := gas.MatchCallByPackage(n, c, r.pkg, r.calls...); matched {
 		modeArg := callexpr.Args[len(callexpr.Args)-1]
 		if mode, err := gas.GetInt(modeArg); err == nil && mode > r.mode {
-			return gas.NewIssue(c, n, r.What, r.Severity, r.Confidence), nil
+			return gas.NewIssue(c, n, r.ID(), r.What, r.Severity, r.Confidence), nil
 		}
 	}
 	return nil, nil

diff --git a/rules/hardcoded_credentials.go b/rules/hardcoded_credentials.go
@@ -69,7 +69,7 @@ func (r *credentials) matchAssign(assign *ast.AssignStmt, ctx *gas.Context) (*ga
 				for _, e := range assign.Rhs {
 					if val, err := gas.GetString(e); err == nil {
 						if r.ignoreEntropy || (!r.ignoreEntropy && r.isHighEntropyString(val)) {
-							return gas.NewIssue(ctx, assign, r.What, r.Severity, r.Confidence), nil
+							return gas.NewIssue(ctx, assign, r.ID(), r.What, r.Severity, r.Confidence), nil
 						}
 					}
 				}
@@ -88,7 +88,7 @@ func (r *credentials) matchValueSpec(valueSpec *ast.ValueSpec, ctx *gas.Context)
 			}
 			if val, err := gas.GetString(valueSpec.Values[index]); err == nil {
 				if r.ignoreEntropy || (!r.ignoreEntropy && r.isHighEntropyString(val)) {
-					return gas.NewIssue(ctx, valueSpec, r.What, r.Severity, r.Confidence), nil
+					return gas.NewIssue(ctx, valueSpec, r.ID(), r.What, r.Severity, r.Confidence), nil
 				}
 			}
 		}

diff --git a/rules/rand.go b/rules/rand.go
@@ -33,7 +33,7 @@ func (w *weakRand) ID() string {
 func (w *weakRand) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) {
 	for _, funcName := range w.funcNames {
 		if _, matched := gas.MatchCallByPackage(n, c, w.packagePath, funcName); matched {
-			return gas.NewIssue(c, n, w.What, w.Severity, w.Confidence), nil
+			return gas.NewIssue(c, n, w.ID(), w.What, w.Severity, w.Confidence), nil
 		}
 	}
 

diff --git a/rules/readfile.go b/rules/readfile.go
@@ -38,7 +38,7 @@ func (r *readfile) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) {
 			if ident, ok := arg.(*ast.Ident); ok {
 				obj := c.Info.ObjectOf(ident)
 				if _, ok := obj.(*types.Var); ok && !gas.TryResolve(ident, c) {
-					return gas.NewIssue(c, n, r.What, r.Severity, r.Confidence), nil
+					return gas.NewIssue(c, n, r.ID(), r.What, r.Severity, r.Confidence), nil
 				}
 			}
 		}

diff --git a/rules/rsa.go b/rules/rsa.go
@@ -34,7 +34,7 @@ func (w *weakKeyStrength) ID() string {
 func (w *weakKeyStrength) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) {
 	if callExpr := w.calls.ContainsCallExpr(n, c); callExpr != nil {
 		if bits, err := gas.GetInt(callExpr.Args[1]); err == nil && bits < (int64)(w.bits) {
-			return gas.NewIssue(c, n, w.What, w.Severity, w.Confidence), nil
+			return gas.NewIssue(c, n, w.ID(), w.What, w.Severity, w.Confidence), nil
 		}
 	}
 	return nil, nil

diff --git a/rules/sql.go b/rules/sql.go
@@ -72,7 +72,7 @@ func (s *sqlStrConcat) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) {
 				if second, ok := node.Y.(*ast.Ident); ok && s.checkObject(second) {
 					return nil, nil
 				}
-				return gas.NewIssue(c, n, s.What, s.Severity, s.Confidence), nil
+				return gas.NewIssue(c, n, s.ID(), s.What, s.Severity, s.Confidence), nil
 			}
 		}
 	}
@@ -107,7 +107,7 @@ func (s *sqlStrFormat) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) {
 	// TODO(gm) improve confidence if database/sql is being used
 	if node := s.calls.ContainsCallExpr(n, c); node != nil {
 		if arg, e := gas.GetString(node.Args[0]); s.MatchPatterns(arg) && e == nil {
-			return gas.NewIssue(c, n, s.What, s.Severity, s.Confidence), nil
+			return gas.NewIssue(c, n, s.ID(), s.What, s.Severity, s.Confidence), nil
 		}
 	}
 	return nil, nil

diff --git a/rules/ssh.go b/rules/ssh.go
@@ -18,7 +18,7 @@ func (r *sshHostKey) ID() string {
 
 func (r *sshHostKey) Match(n ast.Node, c *gas.Context) (gi *gas.Issue, err error) {
 	if _, matches := gas.MatchCallByPackage(n, c, r.pkg, r.calls...); matches {
-		return gas.NewIssue(c, n, r.What, r.Severity, r.Confidence), nil
+		return gas.NewIssue(c, n, r.ID(), r.What, r.Severity, r.Confidence), nil
 	}
 	return nil, nil
 }

diff --git a/rules/subproc.go b/rules/subproc.go
@@ -45,11 +45,11 @@ func (r *subprocess) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) {
 			if ident, ok := arg.(*ast.Ident); ok {
 				obj := c.Info.ObjectOf(ident)
 				if _, ok := obj.(*types.Var); ok && !gas.TryResolve(ident, c) {
-					return gas.NewIssue(c, n, "Subprocess launched with variable", gas.Medium, gas.High), nil
+					return gas.NewIssue(c, n, r.ID(), "Subprocess launched with variable", gas.Medium, gas.High), nil
 				}
 			}
 		}
-		return gas.NewIssue(c, n, "Subprocess launching should be audited", gas.Low, gas.High), nil
+		return gas.NewIssue(c, n, r.ID(), "Subprocess launching should be audited", gas.Low, gas.High), nil
 	}
 	return nil, nil
 }

diff --git a/rules/tempfiles.go b/rules/tempfiles.go
@@ -34,7 +34,7 @@ func (t *badTempFile) ID() string {
 func (t *badTempFile) Match(n ast.Node, c *gas.Context) (gi *gas.Issue, err error) {
 	if node := t.calls.ContainsCallExpr(n, c); node != nil {
 		if arg, e := gas.GetString(node.Args[0]); t.args.MatchString(arg) && e == nil {
-			return gas.NewIssue(c, n, t.What, t.Severity, t.Confidence), nil
+			return gas.NewIssue(c, n, t.ID(), t.What, t.Severity, t.Confidence), nil
 		}
 	}
 	return nil, nil

diff --git a/rules/templates.go b/rules/templates.go
@@ -33,7 +33,7 @@ func (t *templateCheck) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) {
 	if node := t.calls.ContainsCallExpr(n, c); node != nil {
 		for _, arg := range node.Args {
 			if _, ok := arg.(*ast.BasicLit); !ok { // basic lits are safe
-				return gas.NewIssue(c, n, t.What, t.Severity, t.Confidence), nil
+				return gas.NewIssue(c, n, t.ID(), t.What, t.Severity, t.Confidence), nil
 			}
 		}
 	}

diff --git a/rules/tls.go b/rules/tls.go
@@ -51,7 +51,7 @@ func (t *insecureConfigTLS) processTLSCipherSuites(n ast.Node, c *gas.Context) *
 			if ident, ok := cipher.(*ast.SelectorExpr); ok {
 				if !stringInSlice(ident.Sel.Name, t.goodCiphers) {
 					err := fmt.Sprintf("TLS Bad Cipher Suite: %s", ident.Sel.Name)
-					return gas.NewIssue(c, ident, err, gas.High, gas.High)
+					return gas.NewIssue(c, ident, t.ID(), err, gas.High, gas.High)
 				}
 			}
 		}
@@ -66,39 +66,39 @@ func (t *insecureConfigTLS) processTLSConfVal(n *ast.KeyValueExpr, c *gas.Contex
 		case "InsecureSkipVerify":
 			if node, ok := n.Value.(*ast.Ident); ok {
 				if node.Name != "false" {
-					return gas.NewIssue(c, n, "TLS InsecureSkipVerify set true.", gas.High, gas.High)
+					return gas.NewIssue(c, n, t.ID(), "TLS InsecureSkipVerify set true.", gas.High, gas.High)
 				}
 			} else {
 				// TODO(tk): symbol tab look up to get the actual value
-				return gas.NewIssue(c, n, "TLS InsecureSkipVerify may be true.", gas.High, gas.Low)
+				return gas.NewIssue(c, n, t.ID(), "TLS InsecureSkipVerify may be true.", gas.High, gas.Low)
 			}
 
 		case "PreferServerCipherSuites":
 			if node, ok := n.Value.(*ast.Ident); ok {
 				if node.Name == "false" {
-					return gas.NewIssue(c, n, "TLS PreferServerCipherSuites set false.", gas.Medium, gas.High)
+					return gas.NewIssue(c, n, t.ID(), "TLS PreferServerCipherSuites set false.", gas.Medium, gas.High)
 				}
 			} else {
 				// TODO(tk): symbol tab look up to get the actual value
-				return gas.NewIssue(c, n, "TLS PreferServerCipherSuites may be false.", gas.Medium, gas.Low)
+				return gas.NewIssue(c, n, t.ID(), "TLS PreferServerCipherSuites may be false.", gas.Medium, gas.Low)
 			}
 
 		case "MinVersion":
 			if ival, ierr := gas.GetInt(n.Value); ierr == nil {
 				if (int16)(ival) < t.MinVersion {
-					return gas.NewIssue(c, n, "TLS MinVersion too low.", gas.High, gas.High)
+					return gas.NewIssue(c, n, t.ID(), "TLS MinVersion too low.", gas.High, gas.High)
 				}
 				// TODO(tk): symbol tab look up to get the actual value
-				return gas.NewIssue(c, n, "TLS MinVersion may be too low.", gas.High, gas.Low)
+				return gas.NewIssue(c, n, t.ID(), "TLS MinVersion may be too low.", gas.High, gas.Low)
 			}
 
 		case "MaxVersion":
 			if ival, ierr := gas.GetInt(n.Value); ierr == nil {
 				if (int16)(ival) < t.MaxVersion {
-					return gas.NewIssue(c, n, "TLS MaxVersion too low.", gas.High, gas.High)
+					return gas.NewIssue(c, n, t.ID(), "TLS MaxVersion too low.", gas.High, gas.High)
 				}
 				// TODO(tk): symbol tab look up to get the actual value
-				return gas.NewIssue(c, n, "TLS MaxVersion may be too low.", gas.High, gas.Low)
+				return gas.NewIssue(c, n, t.ID(), "TLS MaxVersion may be too low.", gas.High, gas.Low)
 			}
 
 		case "CipherSuites":

diff --git a/rules/unsafe.go b/rules/unsafe.go
@@ -32,7 +32,7 @@ func (r *usingUnsafe) ID() string {
 
 func (r *usingUnsafe) Match(n ast.Node, c *gas.Context) (gi *gas.Issue, err error) {
 	if _, matches := gas.MatchCallByPackage(n, c, r.pkg, r.calls...); matches {
-		return gas.NewIssue(c, n, r.What, r.Severity, r.Confidence), nil
+		return gas.NewIssue(c, n, r.ID(), r.What, r.Severity, r.Confidence), nil
 	}
 	return nil, nil
 }

diff --git a/rules/weakcrypto.go b/rules/weakcrypto.go
@@ -32,7 +32,7 @@ func (r *usesWeakCryptography) ID() string {
 func (r *usesWeakCryptography) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) {
 	for pkg, funcs := range r.blacklist {
 		if _, matched := gas.MatchCallByPackage(n, c, pkg, funcs...); matched {
-			return gas.NewIssue(c, n, r.What, r.Severity, r.Confidence), nil
+			return gas.NewIssue(c, n, r.ID(), r.What, r.Severity, r.Confidence), nil
 		}
 	}
 	return nil, nil