From 918146879fcc4e6f29d64406d0883ac49f86fca8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Audun=20Bj=C3=B8rnerud=20Mo?= Date: Fri, 21 Jul 2023 15:03:17 +0200 Subject: [PATCH 1/9] fix: correctly identify infixed concats as potential SQL injections --- helpers.go | 56 +++++++++++++++++++++++++++++++++++++++++---- rules/rulelist.go | 2 +- rules/sql.go | 55 +++++++++++++++++++++++++++++++++++++++++++- testutils/source.go | 22 ++++++++++++++++++ 4 files changed, 129 insertions(+), 6 deletions(-) diff --git a/helpers.go b/helpers.go index 1d84c453e4..2ca6b8946b 100644 --- a/helpers.go +++ b/helpers.go @@ -96,11 +96,48 @@ func GetChar(n ast.Node) (byte, error) { return 0, fmt.Errorf("Unexpected AST node type: %T", n) } +// GetStringRecursive will recursively walk down a tree of *ast.BinaryExpr. It will then concat the results, and return. +// Unlike the other getters, it does _not_ raise an error for unknown ast.Node types. At the base, the recursion will hit a non-BinaryExpr type, +// either BasicLit or other, so it's not an error case. It will only error if `strconv.Unquote` errors. This matters, because there's +// currently functionality that relies on error values being returned by GetString if and when it hits a non-basiclit string node type, +// hence for cases where recursion is needed, we use this separate function, so that we can still be backwards compatbile. +// +// This was added to handle a SQL injection concatenation case where the injected value is infixed between two strings, not at the start or end. See example below +// +// Do note that this will omit non-string values. So for example, if you were to use this node: +// ```go +// q := "SELECT * FROM foo WHERE name = '" + os.Args[0] + "' AND 1=1" // will result in "SELECT * FROM foo WHERE ” AND 1=1" +// +// ``` +func GetStringRecursive(n ast.Node) (string, error) { + if node, ok := n.(*ast.BasicLit); ok && node.Kind == token.STRING { + return strconv.Unquote(node.Value) + } + + if expr, ok := n.(*ast.BinaryExpr); ok { + var err error + x, xerr := GetStringRecursive(expr.X) + if xerr != nil { + err = errors.Join(err, xerr) + } + + y, yerr := GetStringRecursive(expr.Y) + if yerr != nil { + err = errors.Join(err, yerr) + } + + return x + y, err + } + + return "", nil +} + // GetString will read and return a string value from an ast.BasicLit func GetString(n ast.Node) (string, error) { if node, ok := n.(*ast.BasicLit); ok && node.Kind == token.STRING { return strconv.Unquote(node.Value) } + return "", fmt.Errorf("Unexpected AST node type: %T", n) } @@ -201,22 +238,21 @@ func GetCallStringArgsValues(n ast.Node, _ *Context) []string { return values } -// GetIdentStringValues return the string values of an Ident if they can be resolved -func GetIdentStringValues(ident *ast.Ident) []string { +func getIdentStringValues(ident *ast.Ident, stringFinder func(ast.Node) (string, error)) []string { values := []string{} obj := ident.Obj if obj != nil { switch decl := obj.Decl.(type) { case *ast.ValueSpec: for _, v := range decl.Values { - value, err := GetString(v) + value, err := stringFinder(v) if err == nil { values = append(values, value) } } case *ast.AssignStmt: for _, v := range decl.Rhs { - value, err := GetString(v) + value, err := stringFinder(v) if err == nil { values = append(values, value) } @@ -226,6 +262,18 @@ func GetIdentStringValues(ident *ast.Ident) []string { return values } +// getIdentStringRecursive returns the string of values of an Ident if they can be resolved +// The difference between this and GetIdentStringValues is that it will attempt to resolve the strings recursively, +// if it is passed a *ast.BinaryExpr. See GetStringRecursive for details +func GetIdentStringValuesRecursive(ident *ast.Ident) []string { + return getIdentStringValues(ident, GetStringRecursive) +} + +// GetIdentStringValues return the string values of an Ident if they can be resolved +func GetIdentStringValues(ident *ast.Ident) []string { + return getIdentStringValues(ident, GetString) +} + // GetBinaryExprOperands returns all operands of a binary expression by traversing // the expression tree func GetBinaryExprOperands(be *ast.BinaryExpr) []ast.Node { diff --git a/rules/rulelist.go b/rules/rulelist.go index d856eccad2..9bf7eebe04 100644 --- a/rules/rulelist.go +++ b/rules/rulelist.go @@ -79,8 +79,8 @@ func Generate(trackSuppressions bool, filters ...RuleFilter) RuleList { {"G114", "Use of net/http serve function that has no support for setting timeouts", NewHTTPServeWithoutTimeouts}, // injection - {"G201", "SQL query construction using format string", NewSQLStrFormat}, {"G202", "SQL query construction using string concatenation", NewSQLStrConcat}, + {"G201", "SQL query construction using format string", NewSQLStrFormat}, {"G203", "Use of unescaped data in HTML templates", NewTemplateCheck}, {"G204", "Audit use of command execution", NewSubproc}, diff --git a/rules/sql.go b/rules/sql.go index 4085b5d26a..a16b64910f 100644 --- a/rules/sql.go +++ b/rules/sql.go @@ -98,6 +98,32 @@ func (s *sqlStrConcat) ID() string { return s.MetaData.ID } +// findInjectionInBranch walks diwb a set if expressions, and will create new issues if it finds SQL injections +// This method assumes you've already verified that the branch contains SQL syntax +func (s *sqlStrConcat) findInjectionInBranch(ctx *gosec.Context, branch []ast.Expr) *ast.BinaryExpr { + for _, node := range branch { + be, ok := node.(*ast.BinaryExpr) + if !ok { + continue + } + + operands := gosec.GetBinaryExprOperands(be) + + for _, op := range operands { + if _, ok := op.(*ast.BasicLit); ok { + continue + } + + if ident, ok := op.(*ast.Ident); ok && s.checkObject(ident, ctx) { + continue + } + + return be + } + } + return nil +} + // see if we can figure out what it is func (s *sqlStrConcat) checkObject(n *ast.Ident, c *gosec.Context) bool { if n.Obj != nil { @@ -140,6 +166,32 @@ func (s *sqlStrConcat) checkQuery(call *ast.CallExpr, ctx *gosec.Context) (*issu } } + // Handle the case where an injection occurs as an infixed string concatenation, ie "SELECT * FROM foo WHERE name = '" + os.Args[0] + "' AND 1=1" + if id, ok := query.(*ast.Ident); ok { + var match bool + for _, str := range gosec.GetIdentStringValuesRecursive(id) { + if s.MatchPatterns(str) { + match = true + break + } + } + + if !match { + return nil, nil + } + + switch decl := id.Obj.Decl.(type) { + case *ast.AssignStmt: + if injection := s.findInjectionInBranch(ctx, decl.Lhs); injection != nil { + return ctx.NewIssue(injection, s.ID(), s.What, s.Severity, s.Confidence), nil + } + + if injection := s.findInjectionInBranch(ctx, decl.Rhs); injection != nil { + return ctx.NewIssue(injection, s.ID(), s.What, s.Severity, s.Confidence), nil + } + } + } + return nil, nil } @@ -157,6 +209,7 @@ func (s *sqlStrConcat) Match(n ast.Node, ctx *gosec.Context) (*issue.Issue, erro return s.checkQuery(sqlQueryCall, ctx) } } + return nil, nil } @@ -165,7 +218,7 @@ func NewSQLStrConcat(id string, _ gosec.Config) (gosec.Rule, []ast.Node) { rule := &sqlStrConcat{ 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)"), }, MetaData: issue.MetaData{ ID: id, diff --git a/testutils/source.go b/testutils/source.go index d016d2100b..0bd45a0eb7 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -1596,6 +1596,28 @@ func main() { // SampleCodeG202 - SQL query string building via string concatenation SampleCodeG202 = []CodeSample{ {[]string{` + // infixed concatenation +package main + +import ( + "database/sql" + "os" +) + +func main(){ + db, err := sql.Open("sqlite3", ":memory:") + if err != nil { + panic(err) + } + + q := "INSERT INTO foo (name) VALUES ('" + os.Args[0] + "')" + rows, err := db.Query(q) + if err != nil { + panic(err) + } + defer rows.Close() +}`}, 1, gosec.NewConfig()}, + {[]string{` package main import ( From 4518383a17eeef2ef43f48d93e92e21b16d98954 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Audun=20Bj=C3=B8rnerud=20Mo?= Date: Fri, 21 Jul 2023 17:05:24 +0200 Subject: [PATCH 2/9] fixup! fix: correctly identify infixed concats as potential SQL injections --- rules/rulelist.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rules/rulelist.go b/rules/rulelist.go index 9bf7eebe04..d856eccad2 100644 --- a/rules/rulelist.go +++ b/rules/rulelist.go @@ -79,8 +79,8 @@ func Generate(trackSuppressions bool, filters ...RuleFilter) RuleList { {"G114", "Use of net/http serve function that has no support for setting timeouts", NewHTTPServeWithoutTimeouts}, // injection - {"G202", "SQL query construction using string concatenation", NewSQLStrConcat}, {"G201", "SQL query construction using format string", NewSQLStrFormat}, + {"G202", "SQL query construction using string concatenation", NewSQLStrConcat}, {"G203", "Use of unescaped data in HTML templates", NewTemplateCheck}, {"G204", "Audit use of command execution", NewSubproc}, From 01b40ef9b9e381706e88208411eb1bba3cafad50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Audun=20Bj=C3=B8rnerud=20Mo?= Date: Fri, 21 Jul 2023 17:05:44 +0200 Subject: [PATCH 3/9] fixup! fixup! fix: correctly identify infixed concats as potential SQL injections --- testutils/source.go | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/testutils/source.go b/testutils/source.go index 0bd45a0eb7..0d596a79c8 100644 --- a/testutils/source.go +++ b/testutils/source.go @@ -1635,7 +1635,8 @@ func main(){ panic(err) } defer rows.Close() -}`}, 1, gosec.NewConfig()}, {[]string{` +}`}, 1, gosec.NewConfig()}, + {[]string{` // case insensitive match package main @@ -1654,7 +1655,8 @@ func main(){ panic(err) } defer rows.Close() -}`}, 1, gosec.NewConfig()}, {[]string{` +}`}, 1, gosec.NewConfig()}, + {[]string{` // context match package main @@ -1674,7 +1676,8 @@ func main(){ panic(err) } defer rows.Close() -}`}, 1, gosec.NewConfig()}, {[]string{` +}`}, 1, gosec.NewConfig()}, + {[]string{` // DB transaction check package main @@ -1702,7 +1705,8 @@ func main(){ if err := tx.Commit(); err != nil { panic(err) } -}`}, 1, gosec.NewConfig()}, {[]string{` +}`}, 1, gosec.NewConfig()}, + {[]string{` // multiple string concatenation package main @@ -1721,7 +1725,8 @@ func main(){ panic(err) } defer rows.Close() -}`}, 1, gosec.NewConfig()}, {[]string{` +}`}, 1, gosec.NewConfig()}, + {[]string{` // false positive package main @@ -1740,7 +1745,8 @@ func main(){ panic(err) } defer rows.Close() -}`}, 0, gosec.NewConfig()}, {[]string{` +}`}, 0, gosec.NewConfig()}, + {[]string{` package main import ( @@ -1762,7 +1768,8 @@ func main(){ } defer rows.Close() } -`}, 0, gosec.NewConfig()}, {[]string{` +`}, 0, gosec.NewConfig()}, + {[]string{` package main const gender = "M" @@ -1788,7 +1795,8 @@ func main(){ } defer rows.Close() } -`}, 0, gosec.NewConfig()}, {[]string{` +`}, 0, gosec.NewConfig()}, + {[]string{` // ExecContext match package main @@ -1809,7 +1817,8 @@ func main() { panic(err) } fmt.Println(result) -}`}, 1, gosec.NewConfig()}, {[]string{` +}`}, 1, gosec.NewConfig()}, + {[]string{` // Exec match package main @@ -1829,7 +1838,8 @@ func main() { panic(err) } fmt.Println(result) -}`}, 1, gosec.NewConfig()}, {[]string{` +}`}, 1, gosec.NewConfig()}, + {[]string{` package main import ( From f091a61a802cd2907f4daf96cd14b6c536367798 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Audun=20Bj=C3=B8rnerud=20Mo?= Date: Fri, 21 Jul 2023 17:32:34 +0200 Subject: [PATCH 4/9] fixup! fix: correctly identify infixed concats as potential SQL injections --- helpers.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/helpers.go b/helpers.go index 2ca6b8946b..d76e7c9c46 100644 --- a/helpers.go +++ b/helpers.go @@ -118,12 +118,12 @@ func GetStringRecursive(n ast.Node) (string, error) { var err error x, xerr := GetStringRecursive(expr.X) if xerr != nil { - err = errors.Join(err, xerr) + err = fmt.Errorf("%w Error on X branch in recursion: %w", xerr, err) } y, yerr := GetStringRecursive(expr.Y) if yerr != nil { - err = errors.Join(err, yerr) + err = fmt.Errorf("%w Error on Y branch in recursion: %w", yerr, err) } return x + y, err From 8654b273a197e2fa62967023f08d86e593f02b30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Audun=20Bj=C3=B8rnerud=20Mo?= Date: Mon, 24 Jul 2023 16:13:05 +0200 Subject: [PATCH 5/9] fixup! fix: correctly identify infixed concats as potential SQL injections --- helpers.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/helpers.go b/helpers.go index d76e7c9c46..7716536eb7 100644 --- a/helpers.go +++ b/helpers.go @@ -118,12 +118,12 @@ func GetStringRecursive(n ast.Node) (string, error) { var err error x, xerr := GetStringRecursive(expr.X) if xerr != nil { - err = fmt.Errorf("%w Error on X branch in recursion: %w", xerr, err) + err = fmt.Errorf("%w Error on X branch in recursion: %v", err, xerr) } y, yerr := GetStringRecursive(expr.Y) if yerr != nil { - err = fmt.Errorf("%w Error on Y branch in recursion: %w", yerr, err) + err = fmt.Errorf("%w Error on Y branch in recursion: %v", err, err) } return x + y, err From 37c92581fdbaf24e5849d6f28cc48747cddd0006 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Audun=20Bj=C3=B8rnerud=20Mo?= Date: Mon, 24 Jul 2023 19:55:39 +0200 Subject: [PATCH 6/9] fixup! fix: correctly identify infixed concats as potential SQL injections --- rules/sql.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/rules/sql.go b/rules/sql.go index a16b64910f..61222bfdb3 100644 --- a/rules/sql.go +++ b/rules/sql.go @@ -182,10 +182,6 @@ func (s *sqlStrConcat) checkQuery(call *ast.CallExpr, ctx *gosec.Context) (*issu switch decl := id.Obj.Decl.(type) { case *ast.AssignStmt: - if injection := s.findInjectionInBranch(ctx, decl.Lhs); injection != nil { - return ctx.NewIssue(injection, s.ID(), s.What, s.Severity, s.Confidence), nil - } - if injection := s.findInjectionInBranch(ctx, decl.Rhs); injection != nil { return ctx.NewIssue(injection, s.ID(), s.What, s.Severity, s.Confidence), nil } From cfbeaa099b81df227ff1bbad8ce27d035e356eec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Audun=20Bj=C3=B8rnerud=20Mo?= Date: Tue, 25 Jul 2023 10:57:01 +0200 Subject: [PATCH 7/9] fixup! fixup! fix: correctly identify infixed concats as potential SQL injections --- helpers.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/helpers.go b/helpers.go index 7716536eb7..4ee9389804 100644 --- a/helpers.go +++ b/helpers.go @@ -107,8 +107,7 @@ func GetChar(n ast.Node) (byte, error) { // Do note that this will omit non-string values. So for example, if you were to use this node: // ```go // q := "SELECT * FROM foo WHERE name = '" + os.Args[0] + "' AND 1=1" // will result in "SELECT * FROM foo WHERE ” AND 1=1" -// -// ``` + func GetStringRecursive(n ast.Node) (string, error) { if node, ok := n.(*ast.BasicLit); ok && node.Kind == token.STRING { return strconv.Unquote(node.Value) @@ -117,13 +116,14 @@ func GetStringRecursive(n ast.Node) (string, error) { if expr, ok := n.(*ast.BinaryExpr); ok { var err error x, xerr := GetStringRecursive(expr.X) + // TODO: replace this error formatting strangeness with errors.Join when gosec is bumped to go 1.20 if xerr != nil { - err = fmt.Errorf("%w Error on X branch in recursion: %v", err, xerr) + err = fmt.Errorf("Error on X branch in recursion: %w", xerr) } y, yerr := GetStringRecursive(expr.Y) if yerr != nil { - err = fmt.Errorf("%w Error on Y branch in recursion: %v", err, err) + err = fmt.Errorf("%w Error on Y branch in recursion: %v", err, yerr.Error()) } return x + y, err From 7f6c568073a89fc283cb99b26c9164c4b204f114 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Audun=20Bj=C3=B8rnerud=20Mo?= Date: Tue, 25 Jul 2023 10:58:37 +0200 Subject: [PATCH 8/9] fixup! fixup! fixup! fix: correctly identify infixed concats as potential SQL injections --- helpers.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/helpers.go b/helpers.go index 4ee9389804..a37910fcd3 100644 --- a/helpers.go +++ b/helpers.go @@ -123,6 +123,8 @@ func GetStringRecursive(n ast.Node) (string, error) { y, yerr := GetStringRecursive(expr.Y) if yerr != nil { + // Linter dislikes using non-%w formatting verbs for errors, but also doesn't like formatting with two %w statements. + // So we just add the string representation of the underlying error err = fmt.Errorf("%w Error on Y branch in recursion: %v", err, yerr.Error()) } From 4fc261186ffd670e4255a40a9dcece8fdf9ffb99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Audun=20Bj=C3=B8rnerud=20Mo?= Date: Tue, 25 Jul 2023 12:22:59 +0200 Subject: [PATCH 9/9] fix(helpers): clean up error handling in recursive string getter --- helpers.go | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/helpers.go b/helpers.go index a37910fcd3..b4c23e5bba 100644 --- a/helpers.go +++ b/helpers.go @@ -114,21 +114,17 @@ func GetStringRecursive(n ast.Node) (string, error) { } if expr, ok := n.(*ast.BinaryExpr); ok { - var err error - x, xerr := GetStringRecursive(expr.X) - // TODO: replace this error formatting strangeness with errors.Join when gosec is bumped to go 1.20 - if xerr != nil { - err = fmt.Errorf("Error on X branch in recursion: %w", xerr) + x, err := GetStringRecursive(expr.X) + if err != nil { + return "", err } - y, yerr := GetStringRecursive(expr.Y) - if yerr != nil { - // Linter dislikes using non-%w formatting verbs for errors, but also doesn't like formatting with two %w statements. - // So we just add the string representation of the underlying error - err = fmt.Errorf("%w Error on Y branch in recursion: %v", err, yerr.Error()) + y, err := GetStringRecursive(expr.Y) + if err != nil { + return "", err } - return x + y, err + return x + y, nil } return "", nil