From 28ed5bacb0c7880caa67e9ecc70f2a74732daf8a Mon Sep 17 00:00:00 2001 From: Kunal Jaura Date: Tue, 22 Sep 2026 06:06:46 -0700 Subject: [PATCH] fix(go): stop SQLi false positives on constant/parameterized queries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An adversarial precision pass (running our Go pack over AWS deception-bench's code-level samples) exposed 6 false positives: our SQLi taint rule fired on constant, no-user-input queries like `db.Query("SELECT id FROM users LIMIT 50")`. Root cause: the Gin source pattern `$C.Query(...)` (for `c.Query("param")`) also matched database/sql's own `db.Query(...)` — so every constant DB query was read as BOTH a taint source and the sink, self-flowed, and false-positived. (It also means govwa's SQLi was being "caught" by this collision, not real dataflow.) Fix: - Drop `$C.Query(...)` from the shared request sources (it collides with db.Query/stmt.Query); Gin still covered by DefaultQuery/Param/PostForm/etc. - Redesign the SQLi taint rule to key on the real dynamic-build signal — source = fmt.Sprintf/concatenation, sink = single-arg query (gosec G201). A constant query has no source and is never flagged; a parameterized call is multi-arg and excluded by the sink; a Sprintf/concat-built query (inline or assign-then-execute, e.g. govwa) is still caught. - Inline the shared source list (semgrep's YAML-anchor handling was fragile). Verified: - deception-bench Go SQLi FPs: 6 -> 0. Constant/parameterized queries no longer flagged; Sprintf/concat-built queries still are. - govwa SQLi still caught; go-sast still 4/4; SAST-injection fixture gate still 53/53, 0 FP. Full suite: 2621 passed. - Mined the exact FP class (a bare constant query) into the safe/ fixtures as a permanent regression guard. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01EZ4QoTqRoYWE25CNoV2Wfy --- .../sast-injection/safe/SafeHandler.go | 9 ++ .../semgrep_rules/injection-go.yaml | 119 +++++++++++++----- 2 files changed, 98 insertions(+), 30 deletions(-) diff --git a/benchmarks/fixtures/sast-injection/safe/SafeHandler.go b/benchmarks/fixtures/sast-injection/safe/SafeHandler.go index 10ac248..d4de540 100644 --- a/benchmarks/fixtures/sast-injection/safe/SafeHandler.go +++ b/benchmarks/fixtures/sast-injection/safe/SafeHandler.go @@ -20,6 +20,15 @@ func GetUser(w http.ResponseWriter, r *http.Request) { _ = rows } +// Constant query — no user input at all. A taint rule must NOT flag this. (An +// earlier Gin `c.Query(...)` source pattern collided with database/sql's own +// `db.Query(...)`, making every constant query self-flow into a false positive; +// this pins that fix — surfaced by the AWS deception-bench precision pass.) +func ListUsers(w http.ResponseWriter, r *http.Request) { + rows, _ := db.Query("SELECT id, username, email FROM users LIMIT 50") + _ = rows +} + // exec without a shell — argv form, user input is a bare argument. func Ping(w http.ResponseWriter, r *http.Request) { host := r.URL.Query().Get("host") diff --git a/isitsecure/engine/code_analysis/semgrep_rules/injection-go.yaml b/isitsecure/engine/code_analysis/semgrep_rules/injection-go.yaml index e28a69d..eddab9c 100644 --- a/isitsecure/engine/code_analysis/semgrep_rules/injection-go.yaml +++ b/isitsecure/engine/code_analysis/semgrep_rules/injection-go.yaml @@ -27,39 +27,28 @@ rules: - pattern: $DB.QueryRow("..." + $X, ...) - pattern: $DB.Exec("..." + $X, ...) - # ---- SQL injection: user input → query STRING (intra-file taint) ---- - # Catches the assign-then-execute shape the inline pattern above misses. The - # sink focuses on the query-string argument, so a parameterized call — - # Query(sql, args...) with taint in an arg — is NOT flagged. + # ---- SQL injection: dynamically-built query STRING (intra-file taint) ---- + # Catches the assign-then-execute shape the inline pattern above misses: + # q := fmt.Sprintf("... %s", x); db.Query(q) + # The SOURCE is the string-building itself (Sprintf / concatenation), NOT the + # request object — a single-arg query is SQLi-risky precisely when its string + # was built dynamically (gosec G201). Keying on the build, not on a request + # read, means a constant query (`db.Query("SELECT ...")`) has no source and is + # never flagged, and a parameterized call (multi-arg, excluded by the sink) is + # never flagged either. - id: isitsecure-go-sqli-taint mode: taint languages: [go] severity: ERROR metadata: {category: injection_risk, isitsecure-severity: critical} - message: "User input flows into a raw SQL query — SQL injection." - pattern-sources: &go_sources - # net/http - - pattern: $R.URL.Query().Get(...) - - pattern: $R.FormValue(...) - - pattern: $R.PostFormValue(...) - - pattern: $R.Header.Get(...) - # gorilla/mux - - pattern: mux.Vars($R)[...] - # Gin - - pattern: $C.Query(...) - - pattern: $C.DefaultQuery(...) - - pattern: $C.Param(...) - - pattern: $C.PostForm(...) - - pattern: $C.GetHeader(...) - # Echo / Fiber - - pattern: $C.QueryParam(...) - - pattern: $C.FormValue(...) - - pattern: $C.Params(...) + message: "A dynamically-built string (fmt.Sprintf/concatenation) flows into a raw SQL query — SQL injection. Use a parameterized query." + pattern-sources: + - pattern: fmt.Sprintf(...) + - pattern: '"..." + $X' + - pattern: '$X + "..."' # Single-argument calls only: a placeholder query passes its user data as # extra args — Query(sql, id) — so a tainted value in a *multi-arg* call is - # the parameter, not the SQL, and must NOT flag. A tainted single-arg call - # is a string-built query. (The fmt.Sprintf/concat forms are caught by the - # pattern rule above regardless of arg count.) + # the parameter, not the SQL, and must NOT flag. pattern-sinks: - pattern-either: - pattern: $DB.Query($Q) @@ -98,7 +87,26 @@ rules: severity: ERROR metadata: {category: injection_risk, isitsecure-severity: critical} message: "User input flows into a shell command (sh -c / bash -c) — command injection." - pattern-sources: *go_sources + # Shared request-derived taint sources for the taint rules below. NOT + # `$C.Query(...)`: it collides with database/sql's `db.Query(...)`, so a bare + # `.Query()` on any receiver would read as a source (see the SQLi rule). + pattern-sources: + # net/http + - pattern: $R.URL.Query().Get(...) + - pattern: $R.FormValue(...) + - pattern: $R.PostFormValue(...) + - pattern: $R.Header.Get(...) + # gorilla/mux + - pattern: mux.Vars($R)[...] + # Gin (DefaultQuery is Gin-specific; plain c.Query is omitted — collision) + - pattern: $C.DefaultQuery(...) + - pattern: $C.Param(...) + - pattern: $C.PostForm(...) + - pattern: $C.GetHeader(...) + # Echo / Fiber + - pattern: $C.QueryParam(...) + - pattern: $C.FormValue(...) + - pattern: $C.Params(...) pattern-sinks: - patterns: - pattern-either: @@ -114,7 +122,24 @@ rules: severity: ERROR metadata: {category: injection_risk, isitsecure-severity: high} message: "User-controlled URL flows into an outbound HTTP request — SSRF." - pattern-sources: *go_sources + pattern-sources: + # net/http + - pattern: $R.URL.Query().Get(...) + - pattern: $R.FormValue(...) + - pattern: $R.PostFormValue(...) + - pattern: $R.Header.Get(...) + # gorilla/mux + - pattern: mux.Vars($R)[...] + # Gin (DefaultQuery is Gin-specific; plain c.Query is omitted — it + # collides with database/sql's db.Query, see the SQLi rule) + - pattern: $C.DefaultQuery(...) + - pattern: $C.Param(...) + - pattern: $C.PostForm(...) + - pattern: $C.GetHeader(...) + # Echo / Fiber + - pattern: $C.QueryParam(...) + - pattern: $C.FormValue(...) + - pattern: $C.Params(...) pattern-sinks: - pattern: http.Get(...) - pattern: http.Post(...) @@ -129,7 +154,24 @@ rules: severity: WARNING metadata: {category: injection_risk, isitsecure-severity: high} message: "File opened with a request-derived path — path traversal." - pattern-sources: *go_sources + pattern-sources: + # net/http + - pattern: $R.URL.Query().Get(...) + - pattern: $R.FormValue(...) + - pattern: $R.PostFormValue(...) + - pattern: $R.Header.Get(...) + # gorilla/mux + - pattern: mux.Vars($R)[...] + # Gin (DefaultQuery is Gin-specific; plain c.Query is omitted — it + # collides with database/sql's db.Query, see the SQLi rule) + - pattern: $C.DefaultQuery(...) + - pattern: $C.Param(...) + - pattern: $C.PostForm(...) + - pattern: $C.GetHeader(...) + # Echo / Fiber + - pattern: $C.QueryParam(...) + - pattern: $C.FormValue(...) + - pattern: $C.Params(...) # filepath.Base/Clean strip traversal sequences — data through them is safe. pattern-sanitizers: - pattern: filepath.Base(...) @@ -147,7 +189,24 @@ rules: severity: WARNING metadata: {category: injection_risk, isitsecure-severity: medium} message: "User input flows into an HTTP redirect target — open redirect." - pattern-sources: *go_sources + pattern-sources: + # net/http + - pattern: $R.URL.Query().Get(...) + - pattern: $R.FormValue(...) + - pattern: $R.PostFormValue(...) + - pattern: $R.Header.Get(...) + # gorilla/mux + - pattern: mux.Vars($R)[...] + # Gin (DefaultQuery is Gin-specific; plain c.Query is omitted — it + # collides with database/sql's db.Query, see the SQLi rule) + - pattern: $C.DefaultQuery(...) + - pattern: $C.Param(...) + - pattern: $C.PostForm(...) + - pattern: $C.GetHeader(...) + # Echo / Fiber + - pattern: $C.QueryParam(...) + - pattern: $C.FormValue(...) + - pattern: $C.Params(...) pattern-sinks: - pattern: http.Redirect($W, $R, $URL, ...) - patterns: