Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix false negative for SQL injection when using DB.QueryRow.Scan() #757

Closed
wants to merge 17 commits into from
2 changes: 1 addition & 1 deletion cmd/gosec/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ func main() {
if err != nil {
logger.Fatal(err)
}
// get a bug

ruleList := loadRules(includeRules, excludeRules)
if len(ruleList.Rules) == 0 {
logger.Fatal("No rules are configured")
Expand Down
17 changes: 16 additions & 1 deletion rules/sql.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -282,7 +297,7 @@ func NewSQLStrFormat(id string, conf gosec.Config) (gosec.Rule, []ast.Node) {
noIssueQuoted: gosec.NewCallList(),
sqlStatement: sqlStatement{
patterns: []*regexp.Regexp{
regexp.MustCompile("(?i)(SELECT|DELETE|INSERT|UPDATE|INTO|FROM|WHERE) "),
regexp.MustCompile("(?i)(SELECT|DELETE|INSERT|UPDATE|INTO|FROM|WHERE)( |\n|\r|\t)"),
regexp.MustCompile("%[^bdoxXfFp]"),
},
MetaData: gosec.MetaData{
Expand Down
89 changes: 88 additions & 1 deletion testutils/source.go
Original file line number Diff line number Diff line change
Expand Up @@ -1168,7 +1168,94 @@ import (

func main(){
fmt.Sprintln()
}`}, 0, gosec.NewConfig()},
}`}, 0, 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\n name = '%s'", os.Args[1])
rows, err := db.Query(q)
if err != nil {
panic(err)
}
defer rows.Close()
}`}, 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()
}`}, 1, gosec.NewConfig()},
}

// SampleCodeG202 - SQL query string building via string concatenation
Expand Down