Query builders hide SQL vulnerabilities

[ubombi]

Summary

Using query builders along with manual string formatting or concatenation isn't detected.

Steps to reproduce the behavior

sb = squirrel.StatementBuilder.Select("1")                                                                                                                                                                                   
sb = sb.Where(
    fmt.Sprintf(
        "somefield IN ( SELECT foo.bar FROM foo JOIN bar ON (bar.id = foo.bar_id) WHERE foo.value IN ('%v') )", 
        strings.Join(inputArray, "','"),
    ),
)

/* q, args, _ := sb.ToSql()
err = r.db.GetContext(ctx, &res, q, args...)
...
*/

go v1.21.5 | Linux amd64 |

Expected behavior

G201

Actual behavior

Nothing found

Possible fix

I'm not familiar with the codebase and just brute-forced what I needed, and I'm not sure if it's a proper way to do it. If you would find approach this appropriate, I can make a proper PR with all sqlCallIdents

diff --git a/rules/sql.go b/rules/sql.go
index 61222bf..7459e2f 100644
--- a/rules/sql.go
+++ b/rules/sql.go
@@ -52,6 +52,10 @@ var sqlCallIdents = map[string]map[string]int{
                "Prepare":         0,
                "PrepareContext":  1,
        },
+       "github.com/Masterminds/squirrel.SelectBuilder": {
+               "Where": 0,
+               // and all other methods
+       },
 }
 
 // findQueryArg locates the argument taking raw SQL
@@ -268,6 +272,13 @@ func (s *sqlStrFormat) checkQuery(call *ast.CallExpr, ctx *gosec.Context) (*issu
                return nil, err
        }
 
+       if call, ok := query.(*ast.CallExpr); ok && call.Fun != nil {
+               issue := s.checkFormatting(call, ctx)
+               if issue != nil {
+                       return issue, err
+               }
+       }
+
        if ident, ok := query.(*ast.Ident); ok && ident.Obj != nil {
                decl := ident.Obj.Decl
                if assign, ok := decl.(*ast.AssignStmt); ok {

Cons

This opens a path to the support of 3rd party libraries, and there are a lot of them

[ccojocar]

Thanks for opening this issue. I am not sure about the fix. I would avoid to hardcode 3rd party libraries into the rule. Maybe something configurable would be more appropriate.

[ubombi]

I don't like that part too. On the other hand, not many people (who use ORMs / QBs) would configure gosec, using it as part of golangci-lint.

[ubombi]

This issue is closed as complete, but does not reference any commit.
@ccojocar was it actually fixed? If not, do you have any preference, regarding this feature?

Are you against of mentioning 3rd party libraries in gosec, or just want to keep them away from SQL rule itself. (E.G: keeping in separate package)

[ccojocar]

I would to cover standard library for now. Thanks