Require positive confirmation for lightfuzz code-change findings - #3357
Require positive confirmation for lightfuzz code-change findings#3357liquidsec wants to merge 6 commits 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 🚀
30 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 46568 +241
======================================
+ Hits 41588 41800 +212
- Misses 4739 4768 +29 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
# Conflicts: # bbot/core/helpers/diff.py
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.
|
Trimmed in Kept what the code can't show (entity spellings, magic-header bytes, why an extra request exists), cut what restated it. The FP test class comments stay, since the app shape each negative test models isn't visible in a handler that just returns status codes. |
|
@singlerider all comments have been addressed |
There was a problem hiding this comment.
All prior blockers cleared. Verified locally.
- crypto interior-zeros rule and blanket 403 both yanked,
crypto.pyback to dev verbatim. 'and the other three entity spellings now stripped.- thresholds named, comment density down.
Ran the file: 119/119. Stashed the source fixes and re-ran the five FP tests plus test_event_archived_provenance against dev: all six fail, so none pass vacuously. CI green on 3b12370.
Non-blocking: archive_url is in json() but not the round-trip. event_from_json reads from data, and the pydantic/SQL Event models drop unknown keys, so a persisted-then-reloaded finding loses the [ARCHIVED] marker. Fine for output.txt/json (both render off the live event); only matters for SQL/mongo consumers. Worth a PR-body line or a follow-up.
Approving.
| return f"z{payload[1:]}" if payload[0] != "z" else f"q{payload[1:]}" | ||
| try: | ||
| data = bytes.fromhex(payload) if encoding == "hex" else base64.b64decode(payload) | ||
| except Exception: |
There was a problem hiding this comment.
It would be nice to set a better precedent of error propagation. Like, if there's an error here, it will drop silently. Per the Zen of Python:
Errors should never pass silently.
Unless explicitly silenced.
2 participants
Two lightfuzz submodules asserted a vulnerability from a status-code flip, 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.Two 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 in raw, URL-encoded and HTML-escaped form, 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.dev's YARA-gated 403 check is unchanged. Suppressing every transition into 403 was tried and reverted: it costs real findings on apps that answer a SQL error with 403, and the boolean gate already rejects unsigned block pages.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.
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.crypto.pyis untouched.