fix(extractors): reject a regex group the pattern cannot produce - #7612
fix(extractors): reject a regex group the pattern cannot produce#7612DPS0340 wants to merge 1 commit into
Conversation
ExtractRegex skips any submatch where len(match) < RegexGroup+1, and
FindAllStringSubmatch returns exactly NumSubexp()+1 entries. So a group
index beyond the pattern's capture count can never match: the template
compiles, runs, and silently extracts nothing. An empty result is
indistinguishable from 'the pattern did not match the target', which is
the same silent-failure class as the negative-group case already
rejected two lines above.
Scanning nuclei-templates (13451 files, 2100 regex extractors with
group > 0) finds 4 real instances, and they look like genuine bugs
rather than false positives. The clearest is
smtp-credentials-exposure.yaml, where two patterns share one group: 1:
regex:
- smtp_username":".*" <- 0 groups
- <smtp_username>(.*)</smtp_username> <- 1 group
Against a body containing both forms it extracts only the XML one; the
JSON credential is missed with no indication. That template has been
half-working since it was written.
Validation runs on the cached-regex branch too. The compile path
short-circuits on a cache hit, so checking only the freshly-compiled
branch would let the same pattern through unvalidated as soon as any
other template had already compiled it.
The fuzz harness picks a group index independently of its patterns (its
own defaults include the group-less https?://[^\s"']+), so it now
clamps the group to what the narrowest pattern captures -- otherwise it
would fail at compile time on inputs it deliberately generates, instead
of exercising extraction.
Tests fail without the fix: removing both call sites gives 2 failures,
and removing only the cached-branch check still fails the cache test.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughRegex extractor compilation now rejects positive group indexes beyond a pattern’s capture count, including cached regexes. Fuzz-generated extractors also clamp positive group indexes to supported capture counts. ChangesRegex Group Safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Extractor
participant CompileExtractors
participant RegexCache
Extractor->>CompileExtractors: compile regex extractor
CompileExtractors->>RegexCache: load or compile regexp.Regexp
RegexCache-->>CompileExtractors: compiled regex
CompileExtractors->>CompileExtractors: validate RegexGroup against NumSubexp()
CompileExtractors-->>Extractor: compiled extractor or error
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Follow-up with end-to-end evidence, plus a correction to something implied in the description. The four templates are now fixed upstream: projectdiscovery/nuclei-templates#16663. After that lands the scan reports 0 of 2,100 regex extractors with an out-of-range group, so this check should not reject anything in the official set. This PR alone does not surface the error through That is not a fault in this change; it is #7602, which #7603 fixes. Applied together they compose exactly as intended: And against the real templates, before/after: So the useful ordering is #7603 first (or alongside), otherwise this check only fires at scan time rather than at validation time. They are independent and each stands alone — I'd rather flag the interaction than have it discovered in review. Cherry-picked #7603 onto this branch locally to confirm they merge cleanly; no conflicts. |
|
Correcting something I should have flagged when I opened this: the red functional tests are mine, not infrastructure. I pointed at the goleveldb crash on #7605 in the description and left it there, which reads as if the CI on this PR were the same story. It is not. Three fewer templates, on all three platforms, deterministically. That is exactly the count this PR rejects: 4 patterns across 3 files. Re-scanned against current The three templates are broken today. Running what Each matches and extracts nothing, because So the disagreement is not about whether these are broken. It is about what a broken extractor should cost, and that is your call, not mine: A. Fix the templates first. They are wrong regardless of this PR, and I would send that separately. Once B. Warn instead of erroring. Log the mismatch at compile time and let the extractor load. The count stays equal, CI passes, and the silent failure becomes visible without being fatal. The cost is that it diverges from the negative-group check two lines above, which errors — I followed that precedent deliberately, but you may prefer consistency in the other direction. C. Not worth it. If a template that silently extracts nothing is acceptable, close this. I would rather that than leave it open as noise. I lean B, and I did not open with it because A was the more principled fix and I did not check what it did to Worth separating: #7605 and #7603 do not touch this code path and their CI is unrelated (#7605's Windows failure reproduces on #7598, which is not mine). |
|
Option A is now on the table rather than hypothetical: nuclei-templates#16665 fixes all three templates. Four changed lines, verified against a running target with nuclei built from Re-scanned after the change: 13,451 yaml files, 2,100 regex extractors with That leaves the ordering question, which is still yours: merging this before the templates land means the dev binary rejects templates the released binary still ships. If you would rather not have that window at all, B (warn instead of error) avoids it entirely and I will push it on request. The choice between them is a real judgement call about what a broken extractor should cost, and I do not think it is mine to make. Either way the templates are worth fixing. |
There was a problem hiding this comment.
Do NOT touch the harness.
The whole point of the fix is to make the harness actually work. Leave it alone.
Each of these declares `group: 1` on a pattern with zero capture groups. `ExtractRegex` reads `FindAllStringSubmatch`, which returns exactly `NumSubexp()+1` entries per match, so index 1 is always out of range: the template matches, reports, and extracts nothing. Verified against a local server with nuclei built from dev. Before: [oracle-containers-panel:word-1] [http] [info] http://127.0.0.1:8732/ [smtp-credentials-exposure:dsl-1] [http] [high] http://127.0.0.1:8732/ After: [oracle-containers-panel:word-1] ... ["10.1.3.5.0"] [smtp-credentials-exposure:dsl-1] ... ["Username: admin@example.com","PASSWORD: s3cr3t"] smtp-credentials-exposure is the one that matters most: the JSON pattern sits beside an XML pattern that does capture, so on a JSON body the template fires and silently omits the credential it exists to report. `[^"]*` rather than `.*` so a value cannot run past its closing quote into the next field. The other two extract a version string and a context path, both empty today. Scan of the tree: 13,451 yaml files, 2,100 regex extractors with group > 0, these 3 files (4 patterns) affected. Nothing else changes. Found while adding a compile-time check for this in nuclei (projectdiscovery/nuclei#7612).
2 participants
Closes #7611.
The bug
A
regexextractor whosegroupexceeds the pattern's capture count compiles, runs, and silently extracts nothing —ExtractRegexskips every submatch becauseFindAllStringSubmatchreturns exactlyNumSubexp()+1entries. Same silent-failure class as the negative group already rejected two lines above.It affects shipped templates
Scanned
nuclei-templates: 13,451 yaml files, 2,100 regex extractors withgroup > 0, 4 affected. That ratio matters — it says this is a defect, not behaviour templates depend on.The clearest is
smtp-credentials-exposure.yaml, where two patterns share onegroup: 1and only the second captures:Against a body containing both, it extracts
map[bob:{}]— the JSON credential is silently missed, and has been since the template was written.Two details worth flagging in review
Validation runs on the cached branch too. The compile path short-circuits on a cache hit:
Checking only the freshly-compiled branch would let a pattern through unvalidated as soon as any other template had compiled it first.
TestExtractor_CompileValidatesGroupOnCachedRegexcovers exactly this, and I verified it catches a half-fix: removing only the cached-branch check still fails that one test.The fuzz harness needed a change. It picks a group index independently of its patterns, and its own defaults include the group-less
https?://[^\s"']+, so it would now fail at compile time on inputs it deliberately generates rather than exercising extraction. It clamps the group to what the narrowest pattern captures. I'd rather point this out than have it look like an unexplained edit to test scaffolding.Verification
Tests fail without the fix — removing both call sites gives 2 failures; removing only the cached-branch check gives 1.
Follow-up, not included here
The four templates need their
groupcorrected (or the non-capturing pattern amended) innuclei-templates. I've left that out of this PR since it's a different repo, but they aren't extracting what they claim to today and I'm happy to send it if useful.Separately: the red
Tests (windows-latest)on my other PR (#7605) is a goleveldb crash that also reproduces on #7598, which isn't mine — details in that thread. Mentioning it here only because this PR may show the same failure.Summary by CodeRabbit
Bug Fixes
Tests