Add Go for-range taint propagation
ober
f014c570cc2ea459117478f6c307ac61ea054760
--- a/HANDOFF_OPUS_4_8.md +++ b/HANDOFF_OPUS_4_8.md @@ -44,31 +44,49 @@ languages, so closing them is the tip of the larger work: constructs. Real tree-sitter grammars for these languages would replace the approximation wholesale. -2. **Advanced pattern operators still unimplemented** (each verified returning - 0 matches): typed metavariables `($X : T).$F`; multiple focus-metavariables - `focus-metavariable: [$A, $B]`; the deep-expression operator `<... $X ...>`; - and multi-target assignment taint (`a, b := f()` — the `:=` propagator only - handles a single LHS). `command-injection` needs the first three plus - multi-assign; `zip-traversal` needs deep-expression inside `pattern-not-inside`. - -3. **A real dataflow taint engine.** Reachability today is a set of heuristics - (source-before-sink, range-contains, token/access-path compatibility, - same-range, focus-aware binding). It is delicately tuned — changes here - repeatedly regressed the js structural-matcher taint tests. A proper CFG/ - dataflow engine would replace the heuristics. - -4. **Remove remaining overfitted fallbacks.** There is at least one fixture- - reading fallback still present: a comment whose text contains a substring of - the rule id makes a nearby sink fire (found while closing no_duplicate; - verified `// duplicate`/`// findings`/`// taint` match, `// xyzzy` does not). - These should be deleted once real matching covers their cases. - -The fixes in this run incrementally improved (2): generic argument-position -metavar binding (strings/`$`-vars), optional comma-ellipsis, multiline-in-parens -ellipsis, and several taint-reachability branches (same-range, self-binding -source-in-sink) — all gated to avoid regressing the structural-matcher -languages. Continue by either implementing the operators in (2) for the generic -matcher (approximations risk overfitting) or, more durably, adding grammars (1). +2. **The generic matcher binds a metavar to its FIRST value, then only finds + same-value occurrences.** Verified: with `pattern: $R.File` over a file where + `asrc.File` and `bsrc.File` both appear, only `asrc.File` matches (the regex + locks `$R=asrc`). So two structurally-identical flows with *different* bindings + are not enumerated as separate matches. (Same-binding repeats DO both match — + `reader.File` twice both hit.) This blocks any rule whose source/sink recurs + with a different metavar value, and is a matcher-level, not taint-level, gap. + +3. **Advanced pattern operators still unimplemented**: typed metavariables + `($X : T).$F` (needs a type env the regex matcher lacks); multiple + focus-metavariables `focus-metavariable: [$A, $B]`; the deep-expression + operator `<... $X ...>`. `command-injection` needs typed-metavars + multiple + focus + (2) above; `zip-traversal` needs `<...>` inside `pattern-not-inside` + to drop the HasPrefix-guarded safe sink. 13 `tests/rules` fixtures already use + `<...>`, so a generic `<...>` implementation must not regress them. + +4. **A real dataflow taint engine.** Reachability is heuristics (source-before- + sink, range-contains, token/access-path compatibility, same-range, focus-aware + binding). Delicately tuned — the handoff's warning held up: the zip-traversal + chain `reader.File`→(for-range)`file`→(`:=`)`path`→`os.OpenFile(path)` fires + for the FIRST function but not the second, because the `propagated-source` + token/exact flags + `source-taints-binding?` don't re-complete an identical + chain downstream. A CFG/dataflow engine would replace this. + +5. **Remove the fixture-keyed Go/PHP handlers.** `scan-go-patterns-rule` + (scan.ss ~26673) is a `cond` over ~25 exact rule ids (`go-rule-id?`), several + dispatching to handlers that match literal source strings — + `scan-go-receiver-foo-rule`→`"c.Foo()"`, `scan-go-db-exec-rule`→`"db.Exec("`, + `scan-go-taint-labels-empty-rule`→`"http.Get(url)"`. Plus a comment-substring + fallback (`// duplicate`/`// taint` fire a nearby sink). These prop up part of + the broad "437 passed" and are exactly the overfitting to delete once real + matching (1) covers their cases. + +This run ADDED a real feature: **Go for-range taint propagation** +(`scan-go-range-propagators`, scan.ss ~31204) — `for _, v := range src` +propagates taint `src`→`v`, which semgrep does and Jerboa lacked. Validated +clean (smoke 321/321; all 9 taint dirs unchanged; broad `tests/rules` sweep +reconfirmed). It correctly +taints the loop var (verified: zip-traversal's `file` is now tainted and reaches +`file.Name`/`filepath.Join` in the first function) but does NOT close +zip-traversal alone, which is gated on (2)/(3)/(4). Continue via grammars (1) or +a real taint engine (4); approximating (2)/(3) in the regex matcher risks the +13 `<...>` fixtures and the structural-matcher taint tests. ## Commands --- a/lib/semgrep/scan.sls +++ b/lib/semgrep/scan.sls @@ -31212,6 +31212,121 @@ (or (char=? c #\space) (char=? c #\tab)))) (loop (- e 1)) e)))) + (def (go-range-loop-value-start source vars-start vars-end) + (let loop ([i (- vars-end 1)]) + (cond + [(< i vars-start) vars-start] + [(char=? (string-ref source i) #\,) + (skip-horizontal-forward source (+ i 1))] + [else (loop (- i 1))]))) + (def (go-range-loop-parts source first line-end) + (and (< (+ first 4) line-end) + (char=? (string-ref source first) #\f) + (char=? (string-ref source (+ first 1)) #\o) + (char=? (string-ref source (+ first 2)) #\r) + (let ([c3 (string-ref source (+ first 3))]) + (or (char=? c3 #\space) (char=? c3 #\tab))) + (let ([assign (string-find-substring-from + source + ":=" + (+ first 3))] + [range-pos (string-find-substring-from + source + "range" + (+ first 3))]) + (and assign + range-pos + (< assign range-pos) + (< range-pos line-end) + (> range-pos (+ first 3)) + (let ([before (string-ref source (- range-pos 1))] + [after-idx (+ range-pos 5)]) + (and (or (char=? before #\space) + (char=? before #\tab)) + (< after-idx line-end) + (let ([after (string-ref source after-idx)]) + (or (char=? after #\space) + (char=? after #\tab))))) + (let* ([vars-start (skip-horizontal-forward + source + (+ first 3))] + [vars-end (go-line-trimmed-end + source + vars-start + assign)] + [v-start (go-range-loop-value-start + source + vars-start + vars-end)] + [v-end (go-line-trimmed-end + source + v-start + vars-end)] + [src-start (skip-horizontal-forward + source + (+ range-pos 5))] + [brace (string-find-substring-from + source + "{" + src-start)] + [src-limit (if (and brace (< brace line-end)) + brace + line-end)] + [src-end (go-line-trimmed-end + source + src-start + src-limit)]) + (and (< v-start v-end) + (< src-start src-end) + (identifier-token-char? + (string-ref source v-start)) + (not (and (= (- v-end v-start) 1) + (char=? + (string-ref source v-start) + #\_))) + (list v-start v-end src-start src-end))))))) + (def (scan-go-range-propagators rule path source) + (let ([len (string-length source)]) + (let loop ([line-start 0] [acc '()]) + (if (> line-start len) + (reverse acc) + (let* ([line-end (line-end-after source line-start)] + [first (line-first-nonspace + source + line-start + line-end)] + [parts (go-range-loop-parts source first line-end)] + [next (if (< line-end len) + (+ line-end 1) + (+ len 1))]) + (if (not parts) + (loop next acc) + (let* ([v-start (car parts)] + [v-end (cadr parts)] + [src-start (caddr parts)] + [src-end (cadddr parts)] + [l-binding (metavariable-binding-for-range + "L" + source + v-start + v-end)] + [r-binding (metavariable-binding-for-range + "R" + source + src-start + src-end)] + [finding (finding-for-range-with-bindings rule path source first src-end + (list + (cons "L" l-binding) + (cons "R" r-binding)))] + [propagator (list (cons 'finding finding) + (cons 'from "$R") (cons 'to "$L") + (cons 'by-side-effect #t) + (cons 'label #f) + (cons 'requires #f) + (cons 'replace-labels #f) + (cons 'implicit-assignment #t))]) + (loop next (cons propagator acc))))))))) (def (scan-go-implicit-assignment-propagators rule path @@ -33083,10 +33198,15 @@ (scan-implicit-assignment-propagators rule language path source target-root) (if (go-language? language) - (scan-go-implicit-assignment-propagators - rule - path - source) + (append + (scan-go-implicit-assignment-propagators + rule + path + source) + (scan-go-range-propagators + rule + path + source)) '()))] [sanitizers (scan-taint-specs rule (alist-ref/default taint 'sanitizers '()) language path source target-root #f)] --- a/src/.jerbuild-hashes +++ b/src/.jerbuild-hashes @@ -3,11 +3,11 @@ ("src/semgrep/output/json.ss" . "293881CFA2ADB7BC") ("src/semgrep/lang.ss" . "6982E07679D20836") ("src/semgrep/parse/parse-target.ss" . "E74854DDDACF6BA") - ("src/semgrep/scan.ss" . "E4428A1FD577875E") + ("src/semgrep/scan.ss" . "621FB6C8F6C1151A") + ("src/semgrep/fix.ss" . "2E5B65B1FEF3B2B1") + ("src/semgrep/output/text.ss" . "BE476CB84B807FBA") ("src/semgrep/rule.ss" . "E12C108153C181FA") ("src/semgrep/schema/lang.ss" . "CAE2CA859C9A9FD0") - ("src/semgrep/output/text.ss" . "BE476CB84B807FBA") - ("src/semgrep/fix.ss" . "2E5B65B1FEF3B2B1") ("src/semgrep/match/structural.ss" . "6FE77014EE9FDCE4") ("src/semgrep/main.ss" . "A4EC9E7F2A09D25E") ("src/semgrep/cli.ss" . "EBDC4B1DAD3F13CC")) --- a/src/semgrep/scan.ss +++ b/src/semgrep/scan.ss @@ -31201,6 +31201,89 @@ (loop (- e 1)) e)))) +(def (go-range-loop-value-start source vars-start vars-end) + ;; Value variable = text after the rightmost comma in the loop-var list + ;; (`for k, v := range s` -> v); when there is no comma it is the whole list. + (let loop ([i (- vars-end 1)]) + (cond + [(< i vars-start) vars-start] + [(char=? (string-ref source i) #\,) + (skip-horizontal-forward source (+ i 1))] + [else (loop (- i 1))]))) + +(def (go-range-loop-parts source first line-end) + ;; Recognize `for [$K, ] $V := range $SRC ... {` on one line; return + ;; (value-start value-end src-start src-end) for the taint edge $SRC -> $V, + ;; or #f. Skips the blank identifier `_` as a value. + (and (< (+ first 4) line-end) + (char=? (string-ref source first) #\f) + (char=? (string-ref source (+ first 1)) #\o) + (char=? (string-ref source (+ first 2)) #\r) + (let ([c3 (string-ref source (+ first 3))]) + (or (char=? c3 #\space) (char=? c3 #\tab))) + (let ([assign (string-find-substring-from source ":=" (+ first 3))] + [range-pos (string-find-substring-from source "range" (+ first 3))]) + (and assign range-pos + (< assign range-pos) + (< range-pos line-end) + (> range-pos (+ first 3)) + (let ([before (string-ref source (- range-pos 1))] + [after-idx (+ range-pos 5)]) + (and (or (char=? before #\space) (char=? before #\tab)) + (< after-idx line-end) + (let ([after (string-ref source after-idx)]) + (or (char=? after #\space) (char=? after #\tab))))) + (let* ([vars-start (skip-horizontal-forward source (+ first 3))] + [vars-end (go-line-trimmed-end source vars-start assign)] + [v-start (go-range-loop-value-start source vars-start vars-end)] + [v-end (go-line-trimmed-end source v-start vars-end)] + [src-start (skip-horizontal-forward source (+ range-pos 5))] + [brace (string-find-substring-from source "{" src-start)] + [src-limit (if (and brace (< brace line-end)) brace line-end)] + [src-end (go-line-trimmed-end source src-start src-limit)]) + (and (< v-start v-end) + (< src-start src-end) + (identifier-token-char? (string-ref source v-start)) + (not (and (= (- v-end v-start) 1) + (char=? (string-ref source v-start) #\_))) + (list v-start v-end src-start src-end))))))) + +(def (scan-go-range-propagators rule path source) + ;; Go `for _, v := range src` propagates taint from the ranged source to the + ;; loop value variable, mirroring the implicit-assignment propagators. + (let ([len (string-length source)]) + (let loop ([line-start 0] [acc '()]) + (if (> line-start len) + (reverse acc) + (let* ([line-end (line-end-after source line-start)] + [first (line-first-nonspace source line-start line-end)] + [parts (go-range-loop-parts source first line-end)] + [next (if (< line-end len) (+ line-end 1) (+ len 1))]) + (if (not parts) + (loop next acc) + (let* ([v-start (car parts)] + [v-end (cadr parts)] + [src-start (caddr parts)] + [src-end (cadddr parts)] + [l-binding (metavariable-binding-for-range + "L" source v-start v-end)] + [r-binding (metavariable-binding-for-range + "R" source src-start src-end)] + [finding (finding-for-range-with-bindings + rule path source first src-end + (list (cons "L" l-binding) + (cons "R" r-binding)))] + [propagator (list + (cons 'finding finding) + (cons 'from "$R") + (cons 'to "$L") + (cons 'by-side-effect #t) + (cons 'label #f) + (cons 'requires #f) + (cons 'replace-labels #f) + (cons 'implicit-assignment #t))]) + (loop next (cons propagator acc))))))))) + (def (scan-go-implicit-assignment-propagators rule path source) (let ([len (string-length source)]) (let loop ([line-start 0] [acc '()]) @@ -33060,7 +33143,9 @@ source target-root) (if (go-language? language) - (scan-go-implicit-assignment-propagators rule path source) + (append + (scan-go-implicit-assignment-propagators rule path source) + (scan-go-range-propagators rule path source)) '()))] [sanitizers (scan-taint-specs rule