Honor safe comparisons taint option
ober
9b67df76802dab97ae3673fd3defb1765638138c
--- a/HANDOFF_OPUS_4_8.md +++ b/HANDOFF_OPUS_4_8.md @@ -1,10 +1,10 @@ # Opus 4.8 Handoff: jerboa-semgrep Semgrep Parity -Date: 2026-05-29 02:12 MDT +Date: 2026-05-29 02:23 MDT Workspace: `/Users/user/mine/jerboa-semgrep` Sibling upstream Semgrep checkout: `/Users/user/mine/semgrep` Packaged Semgrep oracle: `/Users/user/.local/bin/semgrep` -Current pre-handoff HEAD: `d767d71 Clear method sequence propagator frontier` +Current pre-handoff HEAD: `a928d72 Clear JSX href taint frontier` The user wants this project carried forward until the pure Jerboa port reaches Semgrep parity. Continue from this state. Do not restart broad discovery from @@ -52,7 +52,7 @@ make test Result: ```text -205 tests, 205 passed, 0 failed +206 tests, 206 passed, 0 failed ``` Local oracle: @@ -67,10 +67,10 @@ Result: oracle: 42 passed, 0 failed ``` -Focused `taint_react` case: +Focused `taint_safe_comparisons` case: ```sh -SEMGREP_CURRENT=/Users/user/.local/bin/semgrep CASE_REGEX='^taint_react$' LIST_MISMATCHES=1 MAX_DIFFS=80 tests/oracle/upstream-sweep.sh +SEMGREP_CURRENT=/Users/user/.local/bin/semgrep CASE_REGEX='^taint_safe_comparisons$' LIST_MISMATCHES=1 MAX_DIFFS=80 tests/oracle/upstream-sweep.sh ``` Result: @@ -82,17 +82,17 @@ upstream-sweep: 1 passed, 0 mismatched, 0 jerboa errors, 0 current errors, 1 com Nearby taint guardrail: ```sh -SEMGREP_CURRENT=/Users/user/.local/bin/semgrep CASE_REGEX='^(taint_react|taint_safe_comparisons|taint_spread_record_big|taint_param_source3|taint_propagator2)$' LIST_MISMATCHES=1 MAX_DIFFS=260 tests/oracle/upstream-sweep.sh +SEMGREP_CURRENT=/Users/user/.local/bin/semgrep CASE_REGEX='^(taint_safe_comparisons|taint_react|taint_param_source3|taint_propagator2|taint_spread_record_big)$' LIST_MISMATCHES=1 MAX_DIFFS=260 tests/oracle/upstream-sweep.sh ``` Result: ```text -upstream-sweep: 3 passed, 2 mismatched, 0 jerboa errors, 0 current errors, 5 compared +upstream-sweep: 4 passed, 1 mismatched, 0 jerboa errors, 0 current errors, 5 compared ``` -The two remaining mismatches in that guardrail are `taint_safe_comparisons` -and the known wider frontier `taint_spread_record_big`. +The remaining mismatch in that guardrail is the known wider frontier +`taint_spread_record_big`. Full upstream sweep: @@ -103,7 +103,7 @@ SEMGREP_CURRENT=/Users/user/.local/bin/semgrep LIST_MISMATCHES=1 MAX_DIFFS=0 tes Result: ```text -upstream-sweep: 228 passed, 11 mismatched, 0 jerboa errors, 2 current errors, 241 compared +upstream-sweep: 229 passed, 10 mismatched, 0 jerboa errors, 2 current errors, 241 compared ``` The two current errors are packaged-Semgrep oracle errors, not Jerboa scanner @@ -112,47 +112,37 @@ the command above intentionally ran the full sorted 241-case sweep. ## What Changed In This Checkpoint -This checkpoint clears upstream `taint_react`, reducing the full sweep +This checkpoint clears upstream `taint_safe_comparisons`, reducing the full sweep frontier from: ```text -227 passed, 12 mismatched, 0 jerboa errors, 2 current errors +228 passed, 11 mismatched, 0 jerboa errors, 2 current errors ``` to: ```text -228 passed, 11 mismatched, 0 jerboa errors, 2 current errors +229 passed, 10 mismatched, 0 jerboa errors, 2 current errors ``` Implementation changes in `src/semgrep/scan.ss`: -- Added a JavaScript/JSX opening-tag attribute scanner for patterns shaped - like: - - ```text - <SourceLink ... href=$HREF .../> - ``` - -- The scanner parses the JSX tag name, attribute name, and metavariable from - the pattern, scans target opening tags, and binds the expression inside - braced JSX attributes such as `href={rule.source_uri}`. -- It returns a finding covering the opening tag so existing - `focus-metavariable` logic can narrow the report to `$HREF`. Existing taint - sanitizer logic then blocks safe `href={validateUrl(...)}` cases. -- The implementation is intentionally scoped to JSX opening tags with braced - attribute expressions. It does not attempt to become a general JSX structural - matcher. +- Added `taint-assume-safe-comparisons?` for the Semgrep rule option + `taint_assume_safe_comparisons`. +- Reused the existing comparison-safety checks for both direct comparison sinks + and implicit propagation from comparison expressions. +- Kept `taint_assume_safe_booleans` as an implying option for comparison + safety, matching existing behavior and upstream semantics. New smoke coverage in `tests/smoke.ss`: ```text -scan JavaScript taint focused JSX href attribute +scan taint assume safe comparisons blocks boolean results ``` ## Recently Cleared Cases -Clean in the three most recent checkpoints: +Clean in the four most recent checkpoints: ```text taint_param_source1 @@ -160,6 +150,7 @@ taint_param_source2 taint_param_source3 taint_propagator2 taint_react +taint_safe_comparisons ``` Relevant upstream files: @@ -175,33 +166,34 @@ Relevant upstream files: /Users/user/mine/semgrep/tests/rules/taint_propagator2.py /Users/user/mine/semgrep/tests/rules/taint_react.yaml /Users/user/mine/semgrep/tests/rules/taint_react.js +/Users/user/mine/semgrep/tests/rules/taint_safe_comparisons.yaml +/Users/user/mine/semgrep/tests/rules/taint_safe_comparisons.py ``` -Important isolation finding for `taint_react`: +Important isolation finding for `taint_safe_comparisons`: -- `pattern: rule` matched the relevant source tokens, including - `rule.source_uri`. -- `pattern: url` matched the callback parameter and uses. -- `pattern: validateUrl(...)` matched the two safe sanitizer calls. -- The sink formula returned zero before this checkpoint: +- The rule uses `options: { taint_assume_safe_comparisons: true }`, not + `taint_assume_safe_booleans`. +- Before this checkpoint, Jerboa reported both `sink(x)` and `sink(y)` for: - ```yaml - patterns: - - pattern-inside: | - (<SourceLink ... href=$HREF .../>) - - focus-metavariable: $HREF + ```python + x = "tainted" + sink(x) + y = x != "something" + sink(y) ``` -- The missing piece was JSX attribute binding for `$HREF`; the taint engine and - sanitizer blocking were already sufficient once that binding existed. +- Upstream reports only `sink(x)`. The implicit assignment propagator for + `y = x != "something"` must be blocked when safe comparisons are enabled. +- The existing code already had `text-contains-comparison?` checks in the right + two places; they were gated only on the wrong option helper. ## Current Full-Sweep Frontier The current full sorted upstream sweep compared 241 rule/target pairs and has -these 11 mismatches: +these 10 mismatches: ```text -taint_safe_comparisons taint_spread_record_big taint_unify_mvars ts_decorated_async_property @@ -220,63 +212,85 @@ Use this command to refresh the full list: SEMGREP_CURRENT=/Users/user/.local/bin/semgrep LIST_MISMATCHES=1 MAX_DIFFS=0 tests/oracle/upstream-sweep.sh ``` -## Recommended Next Target: taint_safe_comparisons +## Recommended Next Target: taint_spread_record_big -The next sorted frontier is `taint_safe_comparisons`. +The next sorted frontier is `taint_spread_record_big`. Focused command: ```sh -SEMGREP_CURRENT=/Users/user/.local/bin/semgrep CASE_REGEX='^taint_safe_comparisons$' LIST_MISMATCHES=1 MAX_DIFFS=80 tests/oracle/upstream-sweep.sh +SEMGREP_CURRENT=/Users/user/.local/bin/semgrep CASE_REGEX='^taint_spread_record_big$' LIST_MISMATCHES=1 MAX_DIFFS=120 tests/oracle/upstream-sweep.sh ``` Current result: ```text -MISMATCH taint_safe_comparisons -@@ -1 +1,2 @@ - (finding "tainting" ".../taint_safe_comparisons.py" 3 1 32 3 8 39 "ERROR" "Test" "") -+(finding "tainting" ".../taint_safe_comparisons.py" 6 1 75 6 8 82 "ERROR" "Test" "") +MISMATCH taint_spread_record_big +@@ -1,3 +0,0 @@ +-(finding "react-create-element-injection-props" ".../taint_spread_record_big.js" 11 41 347 11 45 351 "WARNING" "Injecting user-input into a new React Element may introduce an XSS vulnerability if the property contains a user-controllable object (such as a `dangerouslySetInnerHTML`)." "") +-(finding "react-create-element-injection-props" ".../taint_spread_record_big.js" 31 41 834 31 48 841 "WARNING" "Injecting user-input into a new React Element may introduce an XSS vulnerability if the property contains a user-controllable object (such as a `dangerouslySetInnerHTML`)." "") +-(finding "react-create-element-injection-props" ".../taint_spread_record_big.js" 38 38 1001 38 43 1006 "WARNING" "Injecting user-input into a new React Element may introduce an XSS vulnerability if the property contains a user-controllable object (such as a `dangerouslySetInnerHTML`)." "") upstream-sweep: 0 passed, 1 mismatched, 0 jerboa errors, 0 current errors, 1 compared ``` -Rule: +Rule shape: ```yaml rules: -- id: tainting +- id: react-create-element-injection-props mode: taint - options: - taint_assume_safe_comparisons: true pattern-sources: - - pattern: | - "tainted" + - patterns: + - pattern-inside: | + { ..., $PROPS, ... } = this.props + ... + - focus-metavariable: $PROPS + - patterns: + - pattern-either: + - pattern-inside: | + function ... (...,$PROPS,...) { + ... + } + - pattern-inside: | + ({...,$PROPS,...}) => {...} + - focus-metavariable: $PROPS pattern-sinks: - - pattern: sink(...) - languages: [python] + - patterns: + - pattern-inside: | + React.createElement("...", {...,...$PROPS,...},...); + - pattern: $PROPS + - patterns: + - pattern-either: + - pattern-inside: | + React.createElement("...", {...,dangerouslySetInnerHTML: {__html: $PROPS},...},...); + - pattern: $PROPS + languages: [javascript, typescript] ``` -Target: +Expected target findings: + +```javascript +const { i18n, text, attachments } = this.props; +React.createElement("div", { dangerouslySetInnerHTML: {__html: text} }) + +var comment = props.data +React.createElement("div", { dangerouslySetInnerHTML: {__html: comment} }) -```python -x = "tainted" -#ruleid: tainting -sink(x) -y = x != "something" -#ok: tainting -sink(y) +React.createElement("div", {...props}, 'a') ``` Likely fix area: -- The option is named `taint_assume_safe_comparisons`; current code has - `taint-assume-safe-booleans?` and blocks comparisons only when - `taint_assume_safe_booleans` is enabled. -- Implement option recognition for `taint_assume_safe_comparisons` and ensure - the assignment `y = x != "something"` either does not propagate taint to `y` - or is treated as safe when `sink(y)` is evaluated. -- Add a smoke test that keeps `sink(x)` positive but suppresses `sink(y)` for - the comparison-derived boolean. +- This is a false negative cluster around React `createElement` object + arguments and object spread. +- Source side must focus: + - destructured `this.props` fields such as `text` + - function parameters such as `props` +- Sink side must bind/focus `$PROPS` inside: + - `dangerouslySetInnerHTML: {__html: $PROPS}` + - object spread `{...$PROPS}` +- Sanitizers at lines 45 and 52 use `dompurify.sanitize(comment)` and + `dompurify1.sanitize(comment)` and should remain clean. ## Useful Commands @@ -332,10 +346,10 @@ SCM `function ...` lines are recognized as simple scopes. This avoids sibling leaks without breaking nested callback taint cases already covered by smoke tests. -- After touching comparison-taint logic, run at minimum: +- After touching React object/spread taint logic, run at minimum: ```sh make test - SEMGREP_CURRENT=/Users/user/.local/bin/semgrep CASE_REGEX='^(taint_safe_comparisons|taint_react|taint_param_source3|taint_propagator2)$' LIST_MISMATCHES=1 MAX_DIFFS=260 tests/oracle/upstream-sweep.sh + SEMGREP_CURRENT=/Users/user/.local/bin/semgrep CASE_REGEX='^(taint_spread_record_big|taint_safe_comparisons|taint_react|taint_param_source3)$' LIST_MISMATCHES=1 MAX_DIFFS=260 tests/oracle/upstream-sweep.sh SEMGREP_CURRENT=/Users/user/.local/bin/semgrep make oracle ``` --- a/lib/semgrep/scan.sls +++ b/lib/semgrep/scan.sls @@ -9842,6 +9842,11 @@ (finding-range-contains? sink source)))))))) (def (taint-assume-safe-booleans? rule) (rule-option-enabled? rule "taint_assume_safe_booleans")) + (def (taint-assume-safe-comparisons? rule) + (or (rule-option-enabled? + rule + "taint_assume_safe_comparisons") + (taint-assume-safe-booleans? rule))) (def (taint-assume-safe-numbers? rule) (rule-option-enabled? rule "taint_assume_safe_numbers")) (def (taint-assume-safe-indexes? rule) @@ -10680,7 +10685,7 @@ source sink source-text))) - (not (and (taint-assume-safe-booleans? rule) + (not (and (taint-assume-safe-comparisons? rule) (text-contains-comparison? (finding-text sink source-text)))) (not (taint-safe-function-use? @@ -10912,7 +10917,7 @@ (let ([from-text (and from-finding (finding-text from-finding source))]) (and from-finding - (or (and (taint-assume-safe-booleans? rule) + (or (and (taint-assume-safe-comparisons? rule) from-text (text-contains-comparison? from-text)) (and (taint-assume-safe-numbers? rule) --- 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" . "886D36FBE7824066") - ("src/semgrep/schema/lang.ss" . "CAE2CA859C9A9FD0") + ("src/semgrep/scan.ss" . "C630E187A8F50B3F") ("src/semgrep/rule.ss" . "E12C108153C181FA") - ("src/semgrep/fix.ss" . "2E5B65B1FEF3B2B1") + ("src/semgrep/schema/lang.ss" . "CAE2CA859C9A9FD0") ("src/semgrep/output/text.ss" . "BE476CB84B807FBA") + ("src/semgrep/fix.ss" . "2E5B65B1FEF3B2B1") ("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 @@ -10431,6 +10431,10 @@ (def (taint-assume-safe-booleans? rule) (rule-option-enabled? rule "taint_assume_safe_booleans")) +(def (taint-assume-safe-comparisons? rule) + (or (rule-option-enabled? rule "taint_assume_safe_comparisons") + (taint-assume-safe-booleans? rule))) + (def (taint-assume-safe-numbers? rule) (rule-option-enabled? rule "taint_assume_safe_numbers")) @@ -11195,7 +11199,7 @@ (null? (finding-metavars source))) (and (not (taint-state-token? source-state)) (direct-call-argument-source? source sink source-text))) - (not (and (taint-assume-safe-booleans? rule) + (not (and (taint-assume-safe-comparisons? rule) (text-contains-comparison? (finding-text sink source-text)))) (not (taint-safe-function-use? rule source-state sink source-text)) @@ -11467,7 +11471,7 @@ source) (let ([from-text (and from-finding (finding-text from-finding source))]) (and from-finding - (or (and (taint-assume-safe-booleans? rule) + (or (and (taint-assume-safe-comparisons? rule) from-text (text-contains-comparison? from-text)) (and (taint-assume-safe-numbers? rule) --- a/tests/smoke.ss +++ b/tests/smoke.ss @@ -1424,6 +1424,18 @@ (check (length findings) => 1) (check (finding-start-line (car findings)) => 1))) +(test-case "scan taint assume safe comparisons blocks boolean results" + (let* ([taint-config + "rules:\n - id: demo.taint.safe-comparisons\n mode: taint\n languages: [python]\n message: safe comparison taint\n severity: WARNING\n options:\n taint_assume_safe_comparisons: true\n pattern-sources:\n - pattern: |\n \"tainted\"\n pattern-sinks:\n - pattern: sink(...)\n"] + [findings + (scan-config-string + taint-config + "python" + "demo.py" + "x = \"tainted\"\nsink(x)\ny = x != \"safe\"\nsink(y)\nsink(x == \"safe\")\n")]) + (check (length findings) => 1) + (check (finding-start-line (car findings)) => 2))) + (test-case "scan taint control source reaches later sinks" (let* ([taint-config "rules:\n - id: demo.taint.control\n mode: taint\n languages: [python]\n message: control taint\n severity: WARNING\n pattern-sources:\n - control: true\n pattern: source(...)\n pattern-sinks:\n - pattern: sink(...)\n"]