From 7dfebaf91e80a70494a382e280e9f40b1ea627ba Mon Sep 17 00:00:00 2001 From: Jon McClintock Date: Thu, 5 Oct 2017 16:24:29 +0000 Subject: [PATCH 1/3] 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") +} From 8eb9cc02a46de05ccb9edeb0f91fcabd064a487c Mon Sep 17 00:00:00 2001 From: Jon McClintock Date: Thu, 5 Oct 2017 16:24:29 +0000 Subject: [PATCH 2/3] Adjust SQL format-string rules to ignore inherently safe formats --- rules/sql.go | 37 ++++++++++++++++++++++++++++++------- 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/rules/sql.go b/rules/sql.go index 602f0ab..5996b9a 100644 --- a/rules/sql.go +++ b/rules/sql.go @@ -23,7 +23,19 @@ import ( type sqlStatement struct { gas.MetaData - pattern *regexp.Regexp + + // Contains a list of patterns which must all match for the rule to match. + patterns []*regexp.Regexp +} + +// See if the string matches the patterns for the statement. +func (s sqlStatement) Match(str string) bool { + for _, pattern := range s.patterns { + if !pattern.MatchString(str) { + return false + } + } + return true } type sqlStrConcat struct { @@ -42,7 +54,10 @@ 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 { + if !s.Match(str) { + return nil, nil + } if _, ok := node.Y.(*ast.BasicLit); ok { return nil, nil // string cat OK } @@ -58,9 +73,11 @@ func (s *sqlStrConcat) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) { // NewSQLStrConcat looks for cases where we are building SQL strings via concatenation func NewSQLStrConcat(conf gas.Config) (gas.Rule, []ast.Node) { - return &sqlStrConcat{ - sqlStatement: sqlStatement{ - pattern: regexp.MustCompile(`(?)(SELECT|DELETE|INSERT|UPDATE|INTO|FROM|WHERE) `), + return &SqlStrConcat{ + SqlStatement: SqlStatement{ + patterns: []*regexp.Regexp{ + regexp.MustCompile(`(?)(SELECT|DELETE|INSERT|UPDATE|INTO|FROM|WHERE) `), + }, MetaData: gas.MetaData{ Severity: gas.Medium, Confidence: gas.High, @@ -80,7 +97,10 @@ 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.pattern.MatchString(arg) && e == nil { + if arg, e := gas.GetString(node.Args[0]); e == nil { + if !s.Match(str) { + return nil, nil + } return gas.NewIssue(c, n, s.What, s.Severity, s.Confidence), nil } } @@ -92,7 +112,10 @@ func NewSQLStrFormat(conf gas.Config) (gas.Rule, []ast.Node) { rule := &sqlStrFormat{ calls: gas.NewCallList(), 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, From 1ca335016ae188ba88bf947d819c7ac744840e74 Mon Sep 17 00:00:00 2001 From: Jon McClintock Date: Mon, 22 Jan 2018 18:45:07 +0000 Subject: [PATCH 3/3] Rebase to master --- rules/sql.go | 13 +++++-------- testutils/source.go | 23 ++++++++++++++++++++++- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/rules/sql.go b/rules/sql.go index 5996b9a..c6505e3 100644 --- a/rules/sql.go +++ b/rules/sql.go @@ -29,7 +29,7 @@ type sqlStatement struct { } // See if the string matches the patterns for the statement. -func (s sqlStatement) Match(str string) bool { +func (s sqlStatement) MatchPatterns(str string) bool { for _, pattern := range s.patterns { if !pattern.MatchString(str) { return false @@ -55,7 +55,7 @@ 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); e == nil { - if !s.Match(str) { + if !s.MatchPatterns(str) { return nil, nil } if _, ok := node.Y.(*ast.BasicLit); ok { @@ -73,8 +73,8 @@ func (s *sqlStrConcat) Match(n ast.Node, c *gas.Context) (*gas.Issue, error) { // NewSQLStrConcat looks for cases where we are building SQL strings via concatenation func NewSQLStrConcat(conf gas.Config) (gas.Rule, []ast.Node) { - return &SqlStrConcat{ - SqlStatement: SqlStatement{ + return &sqlStrConcat{ + sqlStatement: sqlStatement{ patterns: []*regexp.Regexp{ regexp.MustCompile(`(?)(SELECT|DELETE|INSERT|UPDATE|INTO|FROM|WHERE) `), }, @@ -97,10 +97,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]); e == nil { - if !s.Match(str) { - return nil, 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 } } diff --git a/testutils/source.go b/testutils/source.go index 7eb0130..f606e4a 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -206,7 +206,28 @@ func main(){ panic(err) } defer rows.Close() -}`, 1}, { +}`, 1}, {` +// Format string false positive, safe string spec. +package main +import ( + "database/sql" + "fmt" + "os" + //_ "github.com/mattn/go-sqlite3" +) + +func main(){ + db, err := sql.Open("sqlite3", ":memory:") + if err != nil { + panic(err) + } + q := fmt.Sprintf("SELECT * FROM foo where id = %d", os.Args[1]) + rows, err := db.Query(q) + if err != nil { + panic(err) + } + defer rows.Close() +}`, 0}, { ` // Format string false positive package main