Fix unbounded lightfuzz queue growth from parameter re-discovery loop - #3405
Fix unbounded lightfuzz queue growth from parameter re-discovery loop#3405liquidsec wants to merge 3 commits into
Conversation
A parameter is identified by its name and type, so the query string of the request that revealed it is context rather than identity. Sites that rotate CSRF tokens or honeypot parameter names on every page load made each observation unique, so dedup never fired and lightfuzz's queue grew without bound. Scans that set url_querystring_collapse=False still treat sibling values as significant, normalized so ordering does not matter.
Every baseline response lightfuzz emits is mined by excavate, which hands the parameters it finds back to lightfuzz. Stop emitting past max_baseline_generations (default 10) so the cycle is bounded even where dedup cannot collapse it.
📊 Performance Benchmark Report
📈 Detailed Results (All Benchmarks)
🎯 Performance Summary! 1 regression ⚠️
30 unchanged ✅🔍 Significant Changes (>10%)
🐍 Python Version 3.11.16 |
Servers that append a session id to the path on every redirect emit an unbounded supply of URLs that dedup treats as distinct. Ingress now drops them past url_max_path_param_repeats (default 10). Only ";key=value" path parameters count; ordinary path segments do not, since a deep path that repeats a directory name is finite and depth is bounded by web_spider_depth.
33e4960 to
7029c11
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #3405 +/- ##
======================================
+ Coverage 90% 90% +1%
======================================
Files 454 454
Lines 47081 47229 +148
======================================
+ Hits 42320 42479 +159
+ Misses 4761 4750 -11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
singlerider
left a comment
singlerider
left a comment
There was a problem hiding this comment.
@liquidsec, I pulled 7029c11 and worked it over. The diagnosis is right and the three-commit split is the correct shape: dedup is the mechanism that should collapse the loop, the generation cap is the backstop for where it structurally cannot, and the path-parameter rule is a separate ingress problem that happens to feed the same queue. Each commit stands alone, which made this reviewable one piece at a time.
What I ran on the branch: the four new tests, plus test_events.py + test_helpers.py (32), test_scan.py + test_config.py + test_manager_deduplication.py (16), test_module_lightfuzz.py (121), test_module_excavate.py (52), test_module_paramminer_getparams.py (8), and paramminer_headers + paramminer_cookies + reflected_parameters + hunt (20). All green. ruff check and ruff format --check clean. I mention the green suite because it is load-bearing for the first finding: nothing currently in the tree pins the behavior that changes, so CI passing does not speak to it either way.
Two blockers, both reproduced against the branch and diffed against dev. Commit 3 I have no objection to.
🔴 Blocking, commit 1 drops parameter names from the key, not just values
_dedup_url under url_querystring_collapse: True returns url.partition("?")[0], so the entire query string leaves the key. That is a wider cut than this setting means anywhere else in the codebase. URL_UNVERIFIED._data_id, earlier in the same file, collapses only values and keeps the sorted parameter names, and defaults.yml states the same contract: "collapse parameter values down to a single value per parameter".
On dev, same config, URL_UNVERIFIED keys:
/contact?csrf=7f01d97aec3100c7&id=6 -> ...:/contact:csrf|id
/contact?csrf=a08157098935259&id=6 -> ...:/contact:csrf|id
/contact?csrf=c19ae748d80ed939&id=6&aYBNT794ROfpo=0978126345
-> ...:/contact:aYBNT794ROfpo|csrf|id
The rotating-value CSRF case you led with already collapses under that rule. The rotating-name honeypot case does not, and that is what commit 2 is for. Your own PR body makes the argument: "This is what bounds the loop where dedup cannot."
The cost of the wider cut, on a front controller. Verified end to end, lightfuzz/lightfuzz-light config (url_querystring_remove: False, url_querystring_collapse: True), two responses fed through the real excavate.handle_event, the emitted dicts then run through the real ScanIngress.handle_event:
excavate emits (real handle_event, Set-Cookie on both pages):
COOKIE SESSID @ https://example.com/index.php?page=login
COOKIE SESSID @ https://example.com/index.php?page=admin
ingress verdicts:
branch -> login ACCEPTED, admin DROPPED ("event was already emitted by its module")
dev -> login ACCEPTED, admin ACCEPTED
Same divergence for paramminer. Those two I built the WEB_PARAMETER to the shape the module actually emits, url = event.url off the HTTP_RESPONSE (paramminer_headers.py:190, inherited by paramminer_getparams), and ran it through real ingress rather than driving the network path. The reconstruction is faithful because is_incoming_duplicate keys on event.module._outgoing_dedup_hash(event), which for both modules is the inherited hash((event, self.name, event.internal, event.always_emit)), so only the event hash and the module name matter:
paramminer_headers HEADER [X-Foo] branch: login ACCEPTED, admin DROPPED dev: both ACCEPTED
paramminer_getparams GETPARAM [id] branch: login ACCEPTED, admin DROPPED dev: both ACCEPTED
?page=login and ?action=delete on index.php are not the same page. On a PHP or ASP front controller that is the login form and the admin form, and on this branch the second one is dropped at ingress and never fuzzed. The GETPARAM-consistency argument in the PR body does not transfer: for a GETPARAM the query string is the parameter, so excavate stripping it is not analogous to discarding the page a COOKIE or a HEADER was observed on.
Keeping sorted names and collapsing values, the way URL_UNVERIFIED already does, fixes the case you hit without this cost. Two assertions in test_web_parameter_querystring_dedup flip if you go that way (rotating_name and bare base become distinct again), and I think the flipped versions are the more defensible ones to have in the tree.
Same function, collapse: False branch: parse_qs defaults to keep_blank_values=False, so blank-valued parameters vanish from the key entirely. On the branch:
/p?a=&b=1 and /p?b=1 -> same key
/p?debug=&id=1 and /p?id=1 -> same key
On dev those are distinct, because the WEB_PARAMETER key was the raw URL. collapse: False is the mode whose entire purpose is treating sibling values as significant (lightfuzz-max, lightfuzz-xss), so silently merging debug= into its own absence is backwards there. keep_blank_values=True covers it. Ordering normalization in that branch is a genuine improvement and I would keep it.
If you want to keep the full strip anyway, the thing that would move me is numbers from the scan that triggered this: queue depth and unique WEB_PARAMETER count under name-preserving collapse + the cap, versus full strip + the cap. If the cap alone flattens it, the coverage loss buys nothing.
🔴 Blocking, baseline_generations stops counting at the first data-equal parent
while 1: ... if parent is None or parent == e: break. Event.__eq__ is hash(self) == hash(other), and __hash__ is hash(self.id) where id is type + sha1(data_id). So == is data equality, not identity, and two distinct events with equal data terminate the walk.
That matters because the break abandons the rest of the ancestry, so every lightfuzz generation above such a pair stops being counted, permanently, for that subtree. Built on real scan.make_event events, alternating HTTP_RESPONSE (lightfuzz) / WEB_PARAMETER (excavate) the way the actual loop does, with one data-equal WEB_PARAMETER pair inserted mid-chain:
true lightfuzz ancestors of leaf: 6
baseline_generations(leaf): 2
Undercounting here is not an off-by-one. It is the emit gate never firing for that branch of the chain, which is precisely the unbounded queue this commit exists to prevent.
On whether such a pair can be admitted: ScanIngress does reject event == event.get_parent(), but get_parent() skips parents flagged _omit, and WEB_PARAMETER and HTTP_RESPONSE are both in the default omit_event_types. With the parent marked omitted, the guard does not fire and ingress returns accept, which is how I built the chain above. I did not manage to trigger it in an unmodified scan, and I would not claim it happens in the wild without that. The point is that the walk depends on a non-obvious property of Event.__eq__ that nothing in the PR checks, and the failure mode when the assumption breaks is the exact one the commit is defending against.
The fix is one character, parent is e, which is what the walk means. Note that get_parents() has the same e == parent break, so switching to it does not help here, I checked and it truncates identically.
Which brings me to the tests. _generation_chain builds SimpleNamespace(module=..., parent=..., host=...), so both new lightfuzz tests pin the shape of the walk against objects that cannot express Event.__eq__, cannot be _omitted, and cannot disagree with the walk in any way. Neither test would have failed with the identity bug present, and neither will fail if ancestry semantics move under it later. This is the one piece of the PR whose entire job is bounding an unbounded queue and the only piece with no real event behind it. Build the chain from scan.make_event(...) with real parents the way test_events.py does, then the identity fix has something that would have caught it.
🟢 The path-parameter rule is keyed on structure, not on a name list
url_max_path_param_repeats counts repeats of any ;key=value key rather than matching JSESSIONID, PHPSESSID, sid and whatever the next framework calls it. That is the version of this that does not need maintenance. Deliberately not counting ordinary path segments, on the reasoning that repetition there is finite and depth is already bounded by web_spider_depth, is the right call and the docstring says so where the next reader will find it.
Enforcing at ingress through the existing blacklisted tag rather than inventing a new rejection path is also right, and asserting handle_event returns (False, "event is blacklisted") pins the rejection before any module queue sees it, which is the property that actually matters. The 0-disables case is covered. I checked the counting against urlparse peeling trailing matrix params into .params, multi-segment paths, encoded semicolons, and empty ;;;; tokens; it holds.
🟢 The cap is applied at emit, not in filter_event
Calling this out because the reasoning is the whole point and it is written down: filter_event runs after dequeue, so gating there would have let the queue grow anyway. Same for the one-shot _baseline_generation_cap_hit verbose, which keeps a bounded scan from producing unbounded logs.
Config plumbing is complete on all three: defaults.yml, models.py, the scanner attribute, and the module Field description carrying its own justification. Nothing here needs a doc edit chased by hand.
2 participants
Problem
lightfuzz emits a baseline
HTTP_RESPONSEfor each parameter it probes. excavate mines those responses and emits the parameters it finds, which come back to lightfuzz, which probes them and emits more baselines. The loop only terminates if dedup collapses the repeats.WEB_PARAMETER._data_idkeyed on the full URL including its query string. On pages that vary their form on every load (rotating CSRF tokens, randomly named honeypot fields), each generation produced a unique key, so dedup never fired and lightfuzz's incoming queue grew without bound.The same failure happens when the varying part is in the path rather than the query string, for example a session id appended to the path on every redirect.
Changes
Three independent fixes, one per commit.
1. Dedupe
WEB_PARAMETERon the page, not the full URL. A parameter is identified by its name and type, both already separate fields in the dedup key, so the query string of whichever request revealed it is context rather than identity. This also makes POSTPARAM and COOKIE consistent with GETPARAM, whose query string excavate already strips.url_querystring_collapse: Falsestill opts back in to treating sibling parameter values as significant, solightfuzz-maxandlightfuzz-xsskeep fuzzing each variant separately. Those keys are now normalized, so parameter ordering no longer affects dedup.2. Cap lightfuzz baseline response generations. New
max_baseline_generations(default 10). lightfuzz stops emitting baseline responses once a parameter is that many lightfuzz generations deep, starving excavate of new material. Applied at emit time rather than infilter_event, which runs after dequeue and so would not have prevented the queue from growing.This is what bounds the loop where dedup cannot, which is the
url_querystring_collapse: Falsecase above. Legitimate post-submit discovery runs a few generations deep, so 10 leaves ample headroom.3. Reject URLs whose path repeats a path parameter. New
url_max_path_param_repeats(default 10), enforced at ingress via the existingblacklistedtag so the event never reaches a module queue. The rule is structural, keyed on repetition rather than a list of known session parameter names.Only
;key=valuepath parameters count. Ordinary path segments deliberately do not: a deep path that repeats a directory name is finite, and depth is already bounded byweb_spider_depth. Set to 0 to disable.