Clear taint label branch frontier
ober
31db293dcd0c73e5915314424eb1f58879161432
--- a/HANDOFF_OPUS_4_8.md +++ b/HANDOFF_OPUS_4_8.md @@ -31,6 +31,7 @@ taint/dataflow, path and target semantics, autofix, and output schemas. Recent checkpoints before this handoff: ```text +34227c0 Clear final global taint frontier 268a616 Clear field-sensitive taint frontier 24796db Advance field-sensitive taint parity 971b714 Advance taint exception and field parity @@ -70,7 +71,7 @@ make test Result: ```text -187 tests, 187 passed, 0 failed +188 tests, 188 passed, 0 failed ``` Local oracle: @@ -109,16 +110,16 @@ Result: upstream-sweep: 3 passed, 0 mismatched, 0 jerboa errors, 0 current errors, 3 compared ``` -Focused case cleared in this checkpoint: +Focused label guardrail cleared in this checkpoint: ```sh -SEMGREP_CURRENT=/Users/user/.local/bin/semgrep CASE_REGEX='^taint_final_globals1$' LIST_MISMATCHES=1 MAX_DIFFS=240 tests/oracle/upstream-sweep.sh +SEMGREP_CURRENT=/Users/user/.local/bin/semgrep CASE_REGEX='^(taint_labels2|taint_labels3|taint_typestate|taint_source_requires|taint_source_requires_reopen)$' LIST_MISMATCHES=1 MAX_DIFFS=260 tests/oracle/upstream-sweep.sh ``` Result: ```text -upstream-sweep: 1 passed, 0 mismatched, 0 jerboa errors, 0 current errors, 1 compared +upstream-sweep: 3 passed, 0 mismatched, 0 jerboa errors, 0 current errors, 3 compared ``` Current first-220 upstream sweep: @@ -130,7 +131,7 @@ SEMGREP_CURRENT=/Users/user/.local/bin/semgrep MAX_CASES=220 LIST_MISMATCHES=1 M Result: ```text -upstream-sweep: 196 passed, 22 mismatched, 0 jerboa errors, 2 current errors, 220 compared +upstream-sweep: 198 passed, 20 mismatched, 0 jerboa errors, 2 current errors, 220 compared ``` The full 241-case sweep was not rerun after this checkpoint. The last full @@ -151,26 +152,25 @@ errors. ## What Changed In This Checkpoint -This checkpoint clears `taint_final_globals1`, building on `268a616` which -cleared the first field-sensitive taint frontier. +This checkpoint clears `taint_labels2` and `taint_labels3`, building on +`34227c0` which cleared `taint_final_globals1`. Implementation changes in `src/semgrep/scan.ss`: -- The string-key object pattern helper now supports single-quoted key - metavariables, bare value metavariables, and an optional parent object - property such as `headers: { '$KEY': $H }`. -- The helper still preserves the existing quoted-value behavior: a quoted - metavariable value in the pattern only matches quoted source values. -- Parent object matching is used to keep `headers: { ... }` patterns scoped to - entries inside a `headers` object instead of every quoted key/value entry. -- This lets a focused sink pattern under `fetch(..., { ... })` bind both - `'Content-Type': 'application/json'` and `'Custom': hashvalue`; taint then - reports only the focused value whose labels reach the sink. +- Assignment kills now recognize mutually exclusive Python `if`/`elif`/`else` + sibling branches by indentation and branch-chain start. +- Assignments in one sibling branch no longer kill labels produced in another + sibling branch merely because they appear later textually. +- Straight-line assignment kills inside the same branch remain unchanged, so + `b = sanitize()` still kills prior `b = a` taint in the same branch. +- This removes label false positives where a `CLEANED` label from one branch + should coexist with taint from an alternate branch at the post-dominator. New smoke coverage in `tests/smoke.ss`: ```text scan JavaScript taint focused header value under fetch +scan taint labels keep sibling if else assignments separate scan Python taint filters impossible exception branches scan taint field source reaches opaque base sink scan taint whole object source honors field cleans @@ -419,13 +419,54 @@ Expected matched line for this case is now exactly: 14 ``` +`taint_labels2` is now cleared: + +```sh +SEMGREP_CURRENT=/Users/user/.local/bin/semgrep CASE_REGEX='^taint_labels2$' LIST_MISMATCHES=1 MAX_DIFFS=260 tests/oracle/upstream-sweep.sh +``` + +What changed: + +- Target file: + `/Users/user/mine/semgrep/tests/rules/taint_labels2.py`. +- Shape: + labels `TAINTED` from `source(...)`, `CLEANED` from `sanitize(...)`, and a + sink requiring `TAINTED and not CLEANED`. +- Jerboa previously had a false positive at line 9 after an `if` branch cleaned + `b` and an `else` branch assigned tainted `b`. +- The later textual `else` assignment no longer kills the `CLEANED` label from + its mutually exclusive sibling branch. +- The straight-line case in `bar` stays clean, and `baz` line 24 remains the + only expected finding. + +Expected matched line for this case is now exactly: + +```text +24 +``` + +`taint_labels3` is now cleared: + +```sh +SEMGREP_CURRENT=/Users/user/.local/bin/semgrep CASE_REGEX='^taint_labels3$' LIST_MISMATCHES=1 MAX_DIFFS=220 tests/oracle/upstream-sweep.sh +``` + +What changed: + +- Target file: + `/Users/user/mine/semgrep/tests/rules/taint_labels3.py`. +- Shape: + labels `P` and `Q` from separate sources, sink requiring `P and Q`. +- The fix preserves the expected line 6 finding where `q(a)` combines with the + prior `p()` label. +- It also avoids the branch-union false positive in `boo()`, where `a = p()` + and `a = q()` occur in mutually exclusive `if`/`else` branches. + ## Current First-220 Frontier -The current first-220 sweep has these 22 mismatches: +The current first-220 sweep has these 20 mismatches: ```text -taint_labels2 -taint_labels3 taint_lambda2 taint_lambda4 taint_match_on_source @@ -464,12 +505,12 @@ vardef_assign_true1 vardef_assign_true2 ``` -## Immediate Next Case: taint_labels2 +## Immediate Next Case: taint_lambda2 Focused command: ```sh -SEMGREP_CURRENT=/Users/user/.local/bin/semgrep CASE_REGEX='^taint_labels2$' LIST_MISMATCHES=1 MAX_DIFFS=260 tests/oracle/upstream-sweep.sh +SEMGREP_CURRENT=/Users/user/.local/bin/semgrep CASE_REGEX='^taint_lambda2$' LIST_MISMATCHES=1 MAX_DIFFS=260 tests/oracle/upstream-sweep.sh ``` Current result: @@ -482,84 +523,68 @@ Rule: ```yaml rules: -- id: tainting - mode: taint +- id: test + message: Test + severity: ERROR languages: - - python - pattern-sinks: - - requires: TAINTED and not CLEANED - pattern: sink(...) + - javascript + - typescript + mode: taint pattern-sources: - - label: TAINTED - pattern: source(...) - - label: CLEANED - pattern: sanitize(...) - message: | - This confirms taint mode works. - severity: ERROR + - label: TAINT + pattern: req.query + - label: DB + pattern: client(...) + pattern-sinks: + - patterns: + - pattern-either: + - pattern: $DB.sink($SINK) + requires: TAINT and DB ``` Target: ```text -/Users/user/mine/semgrep/tests/rules/taint_labels2.py +/Users/user/mine/semgrep/tests/rules/taint_lambda2.js ``` Relevant target: ```text -1 def foo(): -2 a = source() -3 if cond(): -4 b = a -5 b = sanitize() -6 else: -7 b = a -8 #todoruleid: tainting -9 sink(b) -11 def bar(): -12 a = source() -13 if cond(): -14 b = a -15 b = sanitize() -16 #OK: tainting -17 sink(b) -19 def baz(): -20 a = source() -21 if cond(): -22 b = a -23 #ruleid: tainting -24 sink(b) -``` - -Packaged Semgrep expects only: +1 let db; +2 foo(() => { +3 db = client(); +4 }); +6 bar((req, res) => { +7 // ruleid: test +8 db.sink(req.query.id); +9 }); +``` + +Packaged Semgrep expects: ```text -24 +8 ``` -Jerboa currently has a false positive at line 9: +Jerboa currently reports no findings: ```diff -@@ -1 +1,2 @@ - (finding "tainting" ".../taint_labels2.py" 24 3 ... "ERROR" "This confirms taint mode works.\n" "") -+(finding "tainting" ".../taint_labels2.py" 9 3 ... "ERROR" "This confirms taint mode works.\n" "") +@@ -1 +0,0 @@ +-(finding "test" ".../taint_lambda2.js" 8 3 ... "ERROR" "Test" "") ``` Interpretation for the next fix: -- The false positive is caused by label/path imprecision around `if`/`else`. -- Jerboa appears to let the later `else` assignment `b = a` kill or dominate - the `CLEANED` label from the `if` branch, leaving a reachable `TAINTED` label - without `CLEANED` at line 9. -- Semgrep does not report line 9 for this rule. It does report `baz` line 24, - where there is a possible tainted assignment without any cleaning label. -- A likely fix is to make clean/taint assignment kills aware of mutually - exclusive Python `if`/`else` sibling branches, so assignments in one branch do - not kill labels produced in the other branch merely because they occur later - textually. -- Preserve the straight-line behavior in `bar`: `b = sanitize()` after `b = a` - in the same branch should still prevent line 17 from reporting. +- The likely missing flow is a combination of two facts at the same sink: + `db` receives the `DB` label from `db = client()` in one arrow callback, and + `req.query` inside `req.query.id` receives `TAINT` in another callback. +- Semgrep allows both labels to meet at `db.sink(req.query.id)`. +- First confirm whether Jerboa is missing the `DB` propagation from the + assignment inside the first lambda or the `TAINT` compatibility from + `req.query` to the descendant argument `req.query.id`. +- Preserve the guardrails around source-call arguments: this should not be + solved by treating all arguments of all source calls as tainted. ## Resolved Earlier Frontier --- a/lib/semgrep/scan.sls +++ b/lib/semgrep/scan.sls @@ -7893,6 +7893,96 @@ [(eq? (python-clause-kind trimmed) 'try) prev-start] [else (loop prev-start)])))))))))) + (def (python-if-clause-kind trimmed) + (cond + [(and (sg-string-prefix? "if " trimmed) + (sg-string-suffix? ":" trimmed)) + 'if] + [(and (sg-string-prefix? "elif " trimmed) + (sg-string-suffix? ":" trimmed)) + 'elif] + [(sg-string-prefix? "else:" trimmed) 'else] + [else #f])) + (def (python-parent-if-clause source offset) + (let* ([line-start (line-start-before source offset)] + [target-indent (line-indent-at-offset source offset)]) + (let loop ([current-start line-start] + [current-indent target-indent]) + (let ([prev-start (python-previous-line-start + source + current-start)]) + (if (not prev-start) + #f + (let ([trimmed (python-significant-trimmed-line + source + prev-start)]) + (if (not trimmed) + (loop prev-start current-indent) + (let ([indent (line-indent-at-offset + source + prev-start)]) + (if (>= indent current-indent) + (loop prev-start current-indent) + (let ([kind (python-if-clause-kind trimmed)]) + (if kind + (list + (cons 'kind kind) + (cons 'indent indent) + (cons 'start prev-start)) + (loop prev-start indent)))))))))))) + (def (python-if-chain-start source clause) + (let ([kind (alist-ref/default clause 'kind #f)] + [clause-start (alist-ref/default clause 'start #f)] + [clause-indent (alist-ref/default clause 'indent 0)]) + (cond + [(eq? kind 'if) clause-start] + [(or (eq? kind 'elif) (eq? kind 'else)) + (let loop ([current-start clause-start]) + (let ([prev-start (python-previous-line-start + source + current-start)]) + (if (not prev-start) + #f + (let ([trimmed (python-significant-trimmed-line + source + prev-start)]) + (if (not trimmed) + (loop prev-start) + (let ([indent (line-indent-at-offset + source + prev-start)]) + (cond + [(< indent clause-indent) #f] + [(> indent clause-indent) (loop prev-start)] + [(eq? (python-if-clause-kind trimmed) 'if) + prev-start] + [else (loop prev-start)])))))))] + [else #f]))) + (def (python-if-branch-info source finding) + (let ([clause (python-parent-if-clause + source + (finding-start-offset finding))]) + (and clause + (let ([chain-start (python-if-chain-start source clause)]) + (and chain-start + (list + (cons 'start (alist-ref/default clause 'start #f)) + (cons 'indent (alist-ref/default clause 'indent 0)) + (cons 'chain-start chain-start))))))) + (def (python-findings-in-mutually-exclusive-if-branches? + source + a + b) + (let ([a-branch (python-if-branch-info source a)] + [b-branch (python-if-branch-info source b)]) + (and a-branch + b-branch + (= (alist-ref/default a-branch 'indent -1) + (alist-ref/default b-branch 'indent -2)) + (= (alist-ref/default a-branch 'chain-start -1) + (alist-ref/default b-branch 'chain-start -2)) + (not (= (alist-ref/default a-branch 'start -1) + (alist-ref/default b-branch 'start -1)))))) (def (python-pass-line? trimmed) (or (string=? trimmed "pass") (sg-string-prefix? "pass #" trimmed))) @@ -8973,6 +9063,10 @@ source-text assignment-finding sink) + (not (python-findings-in-mutually-exclusive-if-branches? + source-text + source + assignment-finding)) (> (finding-start-offset assignment-finding) (finding-start-offset source)) (< (finding-start-offset assignment-finding) --- a/src/.jerbuild-hashes +++ b/src/.jerbuild-hashes @@ -3,11 +3,11 @@ ("src/semgrep/output/json.ss" . "293881CFA2ADB7BC") ("src/semgrep/lang.ss" . "7E5441BD00A7F1D4") ("src/semgrep/parse/parse-target.ss" . "E74854DDDACF6BA") - ("src/semgrep/scan.ss" . "48ECEC3C7FA6D0BA") - ("src/semgrep/fix.ss" . "2E5B65B1FEF3B2B1") + ("src/semgrep/scan.ss" . "1DF2E7B4678CEE76") ("src/semgrep/output/text.ss" . "BE476CB84B807FBA") - ("src/semgrep/rule.ss" . "E12C108153C181FA") + ("src/semgrep/fix.ss" . "2E5B65B1FEF3B2B1") ("src/semgrep/schema/lang.ss" . "CAE2CA859C9A9FD0") + ("src/semgrep/rule.ss" . "E12C108153C181FA") ("src/semgrep/match/structural.ss" . "F7B63A9A6FA028B") ("src/semgrep/main.ss" . "A4EC9E7F2A09D25E") ("src/semgrep/cli.ss" . "D56FC2D2EB449BA6")) --- a/src/semgrep/scan.ss +++ b/src/semgrep/scan.ss @@ -8388,6 +8388,87 @@ [(eq? (python-clause-kind trimmed) 'try) prev-start] [else (loop prev-start)])))))))))) +(def (python-if-clause-kind trimmed) + (cond + [(and (sg-string-prefix? "if " trimmed) + (sg-string-suffix? ":" trimmed)) + 'if] + [(and (sg-string-prefix? "elif " trimmed) + (sg-string-suffix? ":" trimmed)) + 'elif] + [(sg-string-prefix? "else:" trimmed) 'else] + [else #f])) + +(def (python-parent-if-clause source offset) + (let* ([line-start (line-start-before source offset)] + [target-indent (line-indent-at-offset source offset)]) + (let loop ([current-start line-start] [current-indent target-indent]) + (let ([prev-start (python-previous-line-start source current-start)]) + (if (not prev-start) + #f + (let ([trimmed (python-significant-trimmed-line source prev-start)]) + (if (not trimmed) + (loop prev-start current-indent) + (let ([indent (line-indent-at-offset source prev-start)]) + (if (>= indent current-indent) + (loop prev-start current-indent) + (let ([kind (python-if-clause-kind trimmed)]) + (if kind + (list (cons 'kind kind) + (cons 'indent indent) + (cons 'start prev-start)) + (loop prev-start indent)))))))))))) + +(def (python-if-chain-start source clause) + (let ([kind (alist-ref/default clause 'kind #f)] + [clause-start (alist-ref/default clause 'start #f)] + [clause-indent (alist-ref/default clause 'indent 0)]) + (cond + [(eq? kind 'if) clause-start] + [(or (eq? kind 'elif) (eq? kind 'else)) + (let loop ([current-start clause-start]) + (let ([prev-start (python-previous-line-start source current-start)]) + (if (not prev-start) + #f + (let ([trimmed (python-significant-trimmed-line + source + prev-start)]) + (if (not trimmed) + (loop prev-start) + (let ([indent (line-indent-at-offset + source + prev-start)]) + (cond + [(< indent clause-indent) #f] + [(> indent clause-indent) (loop prev-start)] + [(eq? (python-if-clause-kind trimmed) 'if) + prev-start] + [else (loop prev-start)])))))))] + [else #f]))) + +(def (python-if-branch-info source finding) + (let ([clause (python-parent-if-clause + source + (finding-start-offset finding))]) + (and clause + (let ([chain-start (python-if-chain-start source clause)]) + (and chain-start + (list (cons 'start (alist-ref/default clause 'start #f)) + (cons 'indent (alist-ref/default clause 'indent 0)) + (cons 'chain-start chain-start))))))) + +(def (python-findings-in-mutually-exclusive-if-branches? source a b) + (let ([a-branch (python-if-branch-info source a)] + [b-branch (python-if-branch-info source b)]) + (and a-branch + b-branch + (= (alist-ref/default a-branch 'indent -1) + (alist-ref/default b-branch 'indent -2)) + (= (alist-ref/default a-branch 'chain-start -1) + (alist-ref/default b-branch 'chain-start -2)) + (not (= (alist-ref/default a-branch 'start -1) + (alist-ref/default b-branch 'start -1)))))) + (def (python-pass-line? trimmed) (or (string=? trimmed "pass") (sg-string-prefix? "pass #" trimmed))) @@ -9427,6 +9508,10 @@ to-binding (same-simple-function-scope? source-text source assignment-finding) (same-simple-function-scope? source-text assignment-finding sink) + (not (python-findings-in-mutually-exclusive-if-branches? + source-text + source + assignment-finding)) (> (finding-start-offset assignment-finding) (finding-start-offset source)) (< (finding-start-offset assignment-finding) --- a/tests/smoke.ss +++ b/tests/smoke.ss @@ -1737,6 +1737,18 @@ "source(a)\nother(b)\nsink(a)\nsink(b)\n")]) (check (length findings) => 0))) +(test-case "scan taint labels keep sibling if else assignments separate" + (let* ([taint-config + "rules:\n - id: demo.taint.labels.branches\n mode: taint\n languages: [python]\n message: labeled branch sink\n severity: WARNING\n pattern-sources:\n - label: TAINTED\n pattern: source(...)\n - label: CLEANED\n pattern: sanitize(...)\n pattern-sinks:\n - requires: TAINTED and not CLEANED\n pattern: sink(...)\n"] + [findings + (scan-config-string + taint-config + "python" + "demo.py" + "def foo():\n a = source()\n if cond():\n b = a\n b = sanitize()\n else:\n b = a\n sink(b)\n\ndef bar():\n a = source()\n if cond():\n b = a\n b = sanitize()\n sink(b)\n\ndef baz():\n a = source()\n if cond():\n b = a\n sink(b)\n")]) + (check (length findings) => 1) + (check (finding-start-line (car findings)) => 21))) + (test-case "scan taint source requires prior label" (let* ([taint-config "rules:\n - id: demo.taint.source-requires\n mode: taint\n languages: [python]\n message: required source label\n severity: WARNING\n pattern-sources:\n - label: P\n pattern: p(...)\n - label: Q\n requires: P\n pattern: q(...)\n pattern-sinks:\n - requires: P and Q\n pattern: sink($X)\n"]