From 7dfebaf91e80a70494a382e280e9f40b1ea627ba Mon Sep 17 00:00:00 2001
From: Jon McClintock <jon.mcclintock@mixpanel.com>
Date: Thu, 5 Oct 2017 16:24:29 +0000
Subject: [PATCH] Adjust SQL format-string rules to ignore inherently safe
 formats

---
 rules/sql.go      | 25 ++++++++++++++++++++-----
 rules/sql_test.go | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/rules/sql.go b/rules/sql.go
index 9b8b79f..85b9b31 100644
--- a/rules/sql.go
+++ b/rules/sql.go
@@ -23,7 +23,7 @@ import (
 
 type SqlStatement struct {
 	gas.MetaData
-	pattern *regexp.Regexp
+	patterns []*regexp.Regexp
 }
 
 type SqlStrConcat struct {
@@ -42,7 +42,12 @@ func (s *SqlStrConcat) checkObject(n *ast.Ident) bool {
 func (s *SqlStrConcat) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) {
 	if node, ok := n.(*ast.BinaryExpr); ok {
 		if start, ok := node.X.(*ast.BasicLit); ok {
-			if str, e := gas.GetString(start); s.pattern.MatchString(str) && e == nil {
+			if str, e := gas.GetString(start); e == nil {
+				for _, pattern := range s.patterns {
+					if !pattern.MatchString(str) {
+						return nil, nil
+					}
+				}
 				if _, ok := node.Y.(*ast.BasicLit); ok {
 					return nil, nil // string cat OK
 				}
@@ -59,7 +64,9 @@ func (s *SqlStrConcat) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) {
 func NewSqlStrConcat(conf map[string]interface{}) (gas.Rule, []ast.Node) {
 	return &SqlStrConcat{
 		SqlStatement: SqlStatement{
-			pattern: regexp.MustCompile(`(?)(SELECT|DELETE|INSERT|UPDATE|INTO|FROM|WHERE) `),
+			patterns: []*regexp.Regexp{
+				regexp.MustCompile(`(?)(SELECT|DELETE|INSERT|UPDATE|INTO|FROM|WHERE) `),
+			},
 			MetaData: gas.MetaData{
 				Severity:   gas.Medium,
 				Confidence: gas.High,
@@ -77,7 +84,12 @@ type SqlStrFormat struct {
 // Looks for "fmt.Sprintf("SELECT * FROM foo where '%s', userInput)"
 func (s *SqlStrFormat) Match(n ast.Node, c *gas.Context) (gi *gas.Issue, err error) {
 	if node := gas.MatchCall(n, s.call); node != nil {
-		if arg, e := gas.GetString(node.Args[0]); s.pattern.MatchString(arg) && e == nil {
+		if arg, e := gas.GetString(node.Args[0]); e == nil {
+			for _, pattern := range s.patterns {
+				if !pattern.MatchString(arg) {
+					return nil, nil
+				}
+			}
 			return gas.NewIssue(c, n, s.What, s.Severity, s.Confidence), nil
 		}
 	}
@@ -88,7 +100,10 @@ func NewSqlStrFormat(conf map[string]interface{}) (gas.Rule, []ast.Node) {
 	return &SqlStrFormat{
 		call: regexp.MustCompile(`^fmt\.Sprintf$`),
 		SqlStatement: SqlStatement{
-			pattern: regexp.MustCompile("(?)(SELECT|DELETE|INSERT|UPDATE|INTO|FROM|WHERE) "),
+			patterns: []*regexp.Regexp{
+				regexp.MustCompile("(?)(SELECT|DELETE|INSERT|UPDATE|INTO|FROM|WHERE) "),
+				regexp.MustCompile("%[^bdoxXfFp]"),
+			},
 			MetaData: gas.MetaData{
 				Severity:   gas.Medium,
 				Confidence: gas.High,
diff --git a/rules/sql_test.go b/rules/sql_test.go
index 8919f7a..31ad5bf 100644
--- a/rules/sql_test.go
+++ b/rules/sql_test.go
@@ -214,3 +214,35 @@ func TestSQLInjectionFalsePositiveD(t *testing.T) {
 
 	checkTestResults(t, issues, 0, "Not expected to match")
 }
+
+func TestSQLInjectionFalsePositiveE(t *testing.T) {
+	config := map[string]interface{}{"ignoreNosec": false}
+	analyzer := gas.NewAnalyzer(config, nil)
+	analyzer.AddRule(NewSqlStrFormat(config))
+
+	source := `
+        package main
+        import (
+                "database/sql"
+                "fmt"
+                "os"
+				"strconv"
+                //_ "github.com/mattn/go-sqlite3"
+        )
+        func main(){
+                db, err := sql.Open("sqlite3", ":memory:")
+                if err != nil {
+                        panic(err)
+                }
+				id, _ := strconv.Atoi(os.Args[1])
+                q := fmt.Sprintf("SELECT * FROM foo where id = %d", id)
+                rows, err := db.Query(q)
+                if err != nil {
+                        panic(err)
+                }
+                defer rows.Close()
+        }
+        `
+	issues := gasTestRunner(source, analyzer)
+	checkTestResults(t, issues, 0, "Not expected to match")
+}