Require positive confirmation for lightfuzz code-change findings - #3357
Require positive confirmation for lightfuzz code-change findings#3357liquidsec wants to merge 1 commit into
Conversation
Each of these asserted a vulnerability from a status-code flip or a coincidental statistical signal, with no content-level evidence that the vuln class was present. sqli: the '/'' status flip alone now proves nothing. Suppress any transition into 403, verify the flip is quote-specific with a benign one-vs-two-character control, and require a TRUE/FALSE boolean pair to produce a reproducible content differential before emitting. serial: Error Resolution now sends a same-encoding, same-length twin of the payload with a scrambled magic header. If that also resolves the error, the outcome doesn't depend on the payload's content, so the value is being parsed (e.g. as a URI host) rather than deserialized. crypto: run the structured-identifier discrimination on every candidate pair rather than only those with a leading-zero run, and reject pairs whose XOR zeros out at scattered offsets, which is a shared field template rather than two ciphertexts under one keystream. Also surface archived provenance on findings: FINDING now renders the snapshot URL in output.txt and as a top-level archive_url in output.json, so an archived severity is never read as a claim about the live host.
📊 Performance Benchmark Report
📈 Detailed Results (All Benchmarks)
🎯 Performance Summary+ 1 improvement 🚀
! 1 regression ⚠️
29 unchanged ✅🔍 Significant Changes (>10%)
🐍 Python Version 3.11.15 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #3357 +/- ##
======================================
- Coverage 90% 90% -0%
======================================
Files 450 450
Lines 46327 46450 +123
======================================
+ Hits 41588 41678 +90
- Misses 4739 4772 +33 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
# Conflicts: # bbot/core/helpers/diff.py
| # their plaintexts still coincide, which is the leading run, or a shared suffix. | ||
| # Trailing zeros are stripped first so shared-suffix plaintexts aren't caught here. | ||
| core = tail.rstrip(b"\x00") | ||
| if sum(1 for b in core if b == 0) >= 2: |
There was a problem hiding this comment.
This rule costs real detections. Ablated the two edits separately, 500 many-time-pad pairs of equal-length tokens sharing fixed fields:
| config | detected |
|---|---|
| dev | 500/500 |
| PR | 0/500 |
| gate removed only | 500/500 |
| interior rule only | 0/500 |
Dropping the zero_run >= 2 gate is harmless. This rule alone is the whole loss. Shared interior fields are common in exactly the tokens this check exists to find.
Could not find a narrower fix: contiguous run does not separate (FP 2, real avg 12.1), nor printability (0.38 vs 0.37). With two samples the cases look ambiguous. If so, this is a precision/recall tradeoff, worth stating in the PR body.
| @@ -68,6 +68,15 @@ def _is_structured_id_pair(bytes_a, bytes_b, xored, zero_run): | |||
| if len(bytes_a) == len(bytes_b) and len(tail) <= 4: | |||
There was a problem hiding this comment.
>= 2, <= 4, 0.7, 0.9, min(4, ...) and 0x55 are unnamed. These are the detection knobs someone tunes when a FP lands, and they are only findable by reading the function. Precedent: PER_PARENT_CAP in chaos.py.
| self.lightfuzz.waf_yara_rules, single_quote[3].text | ||
| # A transition into 403 is access control (usually a WAF matching its managed | ||
| # SQLi signature on the bare quote), not a code change driven by the query. | ||
| if 403 in (single_quote[3].status_code, double_single_quote[3].status_code): |
There was a problem hiding this comment.
Disabled just this rule and ran the whole file: 119/119 pass, including Test_Lightfuzz_sqli_unsigned_403_fp. The boolean gate already rejects that case on its own.
So it buys nothing the PR does not have, and drops apps whose SQL error handler returns 403. dev's yara check was narrower. Suggest dropping it, or adding a test that shows the gate is insufficient.
| def _strip_payload(text, payload): | ||
| """Remove reflected copies of a payload from a response body, in raw and encoded form.""" | ||
| for variant in (payload, quote(payload), payload.replace(" ", "+")): | ||
| text = text.replace(variant, "") |
There was a problem hiding this comment.
Strips raw, quote() and plus-form, but not ', which is how most frameworks reflect a quote.
Non-injectable app, envelope validation, no SQL:
| reflection | dev | PR |
|---|---|---|
| raw | 1 FP | 0 |
| HTML-escaped | 1 FP | 1 FP |
So the gate works, this is the one shape where it silently does not help. The re-probe passes because the reflection reproduces identically. Adding the entity form closes it.
| instance/table segment sits -- and the XOR of the diverging bytes lands almost entirely in | ||
| the ASCII-XOR range by coincidence, clearing the 0.9 score on its own.""" | ||
| ids = [ | ||
| "e42a5af4c700201072b211d4d8c2607c", |
There was a problem hiding this comment.
These two GUIDs are load-bearing but opaque. The docstring says they clear 0.9 "by coincidence", so the test depends on an exact byte coincidence with nothing asserting it still holds. If a threshold moves, this passes for the wrong reason.
Worth asserting the properties instead: no shared leading byte, 5 interior zeros, ascii score 0.812. Then a threshold change fails loudly.
There was a problem hiding this comment.
Verified the sqli work fixes real FPs. Two blockers, plus general cleanup.
Blocking: the interior-zeros rule in crypto.py takes many-time-pad detection from 500/500 to 0/500. The blanket 403 suppression is redundant, 119/119 pass without it. Details inline.
Comment density. The new detection code is roughly a third prose. crypto.py is +21 lines with ~8 comment lines, sqli.py +120 with ~14. Several restate the code, and one of them is actively wrong now: the interior-zeros comment asserts ciphertexts only zero at the leading run or shared suffix, which is the false premise driving the detection loss. Long explanatory blocks age badly, prefer naming things so they need less narration. LLMs tend to read comments s "truth." Excessive comments become an absolute drift surface.
Magic numbers. >= 2, <= 4, 0.7, 0.9, min(4, ...), 0x55, and the 403/429 literals are all unnamed detection thresholds. Precedent: PER_PARENT_CAP in chaos.py.
Hardcoded digests in tests. The two 32-hex GUIDs are load-bearing, and the docstring says they clear the 0.9 threshold "by coincidence". Nothing asserts the coincidence still holds, so a threshold change makes it pass for the wrong reason.
Clean: no tests removed (113 to 118, the 152 deleted lines are consolidation into sqli_injectable_response), corrupt_payload edge cases correct, archive_url pre-exists on dev.
2 participants
Three lightfuzz submodules asserted a vulnerability from a status-code flip or a coincidental statistical signal, without any content-level evidence that the vuln class was actually present. This came out of triaging a large batch of findings from real engagement scans, where each of these produced a recurring, systematic false-positive class.
sqli: the'vs''status flip proved nothing on its ownThe Code Change branch emitted
Possible SQL Injection(HIGH) purely from a status difference between the single-quote and doubled-quote probes. Two confounders produce that flip with no SQL involved:'gets a 403, the doubled''does not. The existing guard only suppressed this when the block page matched a WAF YARA signature, which the block pages responsible for these findings did not.Three gates now apply, cheapest first:
is_quote_specific()appends one vs. two benign characters, mirroring'/''in length with no SQL meaning. If the benign pair reproduces the same status triplet, the flip tracks value length or envelope validity.confirm_boolean_differential()is required to emit. A TRUE/FALSE pair must produce a content differential, with reflected payloads stripped and the TRUE payload re-sent to confirm the body is deterministic (an unstable page would otherwise fake a differential). The confirming pair is recorded in the finding description.Time-based blind detection is untouched.
serial: URI parsing read as deserializationError Resolution fired when a garbage control payload returned 500 and a serialized probe returned 200. On parameters the framework parses as a URL or host, that flip is URI parsing: .NET's
Uritreats everything before the first/as the hostname, so a payload whose leading token happens to be a valid host label parses, and the control payload does not. The payload's content never mattered.Before emitting, we now send a twin of the payload with the same encoding, length and trailing bytes but a scrambled magic header. It still parses identically and deserializes under nothing. If it resolves the error too, the outcome is independent of the payload's content and the finding is suppressed.
crypto: structured identifiers read as reused keystreamMany-Time-Pad detection fired on pairs of 32-hex record GUIDs from the same platform instance. Identifiers generated by one system share a field template, so they re-converge at fixed offsets and their XOR is full of zeros, and the XOR of the diverging bytes cleared the 0.9 ASCII-XOR score on its own.
_is_structured_id_pairwas already meant to catch this but was gated behindzero_run >= 2, and these pairs share no leading byte, so it never ran. It now runs on every candidate pair, and rejects pairs whose XOR zeros out at scattered offsets (trailing run stripped first, so genuinely shared-suffix plaintexts still pass).Archived provenance on findings
Detectors running over a Wayback-archived body emitted findings with live severity and no visible indication the evidence was historical. The only signal was
discovery_path.FINDINGnow renders the snapshot URL via the existing but unusedevent.archive_urlproperty: inoutput.txtas a trailing(archived: <url>), and inoutput.jsonas a top-levelarchive_urlkey. It is deliberately kept out ofdata, sinceFINDINGdedups on the whole data dict and putting it there would change finding identity.Severity is not altered. An archived finding is still worth surfacing, particularly information disclosure, where the named resources may still be live.
Notes
HttpCompare.parse_body()was extracted from_compare_syncso the sqli differential feedscompare_bodythe parsed body. Passing raw text silently bypasses theddiff_filtersthat mask dynamic content.sqli_injectable_response()helper rather than relaxing assertions, which are unchanged. Those handlers also parsed the query string positionally, which breaks on payloads containing=, so they now userequest.args.dev, so none pass vacuously.