From ae3149fae118fb22e1311b72adef953b88d59218 Mon Sep 17 00:00:00 2001 From: kaiili <35690781+kaiili@users.noreply.github.com> Date: Tue, 11 Jan 2022 22:33:29 +0800 Subject: [PATCH 1/3] Fix false negative for SQL injection when using DB.QueryRow.Scan() --- rules/sql.go | 15 ++++++++++ testutils/source.go | 69 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+) diff --git a/rules/sql.go b/rules/sql.go index 844eaf5d3e..9893822b79 100644 --- a/rules/sql.go +++ b/rules/sql.go @@ -261,6 +261,21 @@ func (s *sqlStrFormat) Match(n ast.Node, ctx *gosec.Context) (*gosec.Issue, erro switch stmt := n.(type) { case *ast.AssignStmt: for _, expr := range stmt.Rhs { + // when expr is CallExpr and Call.Fun.X is CallExpr ,check Call.Fun.X + if Call, ok := expr.(*ast.CallExpr); ok { + Selector, ok := Call.Fun.(*ast.SelectorExpr) + if !ok { + continue + } + sqlQueryCall, ok := Selector.X.(*ast.CallExpr) + // checkQuery when success get sqlQuery CallExpr and Selector contains Callexpr + if ok && s.ContainsCallExpr(sqlQueryCall, ctx) != nil { + issue, err := s.checkQuery(sqlQueryCall, ctx) + if err == nil && issue != nil { + return issue, err + } + } + } if sqlQueryCall, ok := expr.(*ast.CallExpr); ok && s.ContainsCallExpr(expr, ctx) != nil { return s.checkQuery(sqlQueryCall, ctx) } diff --git a/testutils/source.go b/testutils/source.go index 106d8a2b05..bf8e9f1d7f 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -1189,6 +1189,75 @@ func main(){ panic(err) } defer rows.Close() +<<<<<<< HEAD +}`}, 1, gosec.NewConfig()}, {[]string{` +// Format string with \n\r +package main + +import ( + "database/sql" + "fmt" + "os" +) + +func main(){ + db, err := sql.Open("sqlite3", ":memory:") + if err != nil { + panic(err) + } + q := fmt.Sprintf("SELECT * FROM foo where\nname = '%s'", os.Args[1]) + rows, err := db.Query(q) + if err != nil { + panic(err) + } + defer rows.Close() +}`}, 1, gosec.NewConfig()}, {[]string{` +// SQLI by db.Query(some).Scan(&other) +package main + +import ( + "database/sql" + "fmt" + "os" +) + +func main() { + var name string + db, err := sql.Open("sqlite3", ":memory:") + if err != nil { + panic(err) + } + q := fmt.Sprintf("SELECT name FROM users where id = '%s'", os.Args[1]) + row := db.QueryRow(q) + err = row.Scan(&name) + if err != nil { + panic(err) + } + defer db.Close() +}`}, 1, gosec.NewConfig()}, {[]string{` +// SQLI by db.Query(some).Scan(&other) +package main + +import ( + "database/sql" + "fmt" + "os" +) + +func main() { + var name string + db, err := sql.Open("sqlite3", ":memory:") + if err != nil { + panic(err) + } + q := fmt.Sprintf("SELECT name FROM users where id = '%s'", os.Args[1]) + err = db.QueryRow(q).Scan(&name) + if err != nil { + panic(err) + } + defer db.Close() +======= +>>>>>>> 58058af0c8275f11249a71f18fc548bbd7b97ccc }`}, 1, gosec.NewConfig()}, } From 83e31e83ff5255d49c9e38a5795c17db0faf16f5 Mon Sep 17 00:00:00 2001 From: kaiili <35690781+kaiili@users.noreply.github.com> Date: Wed, 12 Jan 2022 08:45:11 +0800 Subject: [PATCH 2/3] rename variable,remove useless comment --- rules/sql.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/rules/sql.go b/rules/sql.go index 9893822b79..e9cb41b292 100644 --- a/rules/sql.go +++ b/rules/sql.go @@ -261,14 +261,12 @@ func (s *sqlStrFormat) Match(n ast.Node, ctx *gosec.Context) (*gosec.Issue, erro switch stmt := n.(type) { case *ast.AssignStmt: for _, expr := range stmt.Rhs { - // when expr is CallExpr and Call.Fun.X is CallExpr ,check Call.Fun.X - if Call, ok := expr.(*ast.CallExpr); ok { - Selector, ok := Call.Fun.(*ast.SelectorExpr) + if call, ok := expr.(*ast.CallExpr); ok { + selector, ok := call.Fun.(*ast.SelectorExpr) if !ok { continue } - sqlQueryCall, ok := Selector.X.(*ast.CallExpr) - // checkQuery when success get sqlQuery CallExpr and Selector contains Callexpr + sqlQueryCall, ok := selector.X.(*ast.CallExpr) if ok && s.ContainsCallExpr(sqlQueryCall, ctx) != nil { issue, err := s.checkQuery(sqlQueryCall, ctx) if err == nil && issue != nil { From 6fec87a2fa1c03e31aa7df4fae781166437a511e Mon Sep 17 00:00:00 2001 From: kaiili <35690781+kaiili@users.noreply.github.com> Date: Wed, 12 Jan 2022 08:55:12 +0800 Subject: [PATCH 3/3] remove left over from rebase --- testutils/source.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/testutils/source.go b/testutils/source.go index bf8e9f1d7f..a6f2af83cd 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -1189,7 +1189,6 @@ func main(){ panic(err) } defer rows.Close() -<<<<<<< HEAD }`}, 1, gosec.NewConfig()}, {[]string{` // Format string with \n\r package main @@ -1256,8 +1255,6 @@ func main() { panic(err) } defer db.Close() -======= ->>>>>>> 58058af0c8275f11249a71f18fc548bbd7b97ccc }`}, 1, gosec.NewConfig()}, }