fix: bound quantifiers without rewriting character classes - #3068
Open
ashvinctrl wants to merge 1 commit into
Open
ashvinctrl wants to merge 1 commit into
ashvinctrl wants to merge 1 commit into
Conversation
set_pattern_upper_bound substituted every unescaped + and * in a regexCheck, including those inside a character class where they are ordinary members. The rewritten class accepts a different set of characters, so the fuzzer generated usernames the target's own regexCheck rejects and the false positive check reported Illegal against a healthy target. Walk the pattern instead of running blind substitutions, so bounds are applied only to quantifiers. Escaped literals are skipped, and a lower bound above the cap now raises the bound for that quantifier alone rather than for the rest of the pattern.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
set_pattern_upper_boundrewrites every unescaped+and*in aregexCheckbefore the false positive fuzzer generates a handle. Inside a character class those are ordinary members, not quantifiers, so the substitution changes which characters the class accepts.Two targets in the manifest are affected today:
Wordnik loses
+and gains{,,,}. CyberDefenders stops excluding*and starts excluding digits. Sampling 20k handles from each rewritten pattern, 54% and 26% of them fail the site's realregexCheck, so sherlock returns Illegal and the check reports a false positive against a healthy target:Running
-m validate_targets_fp --chunked-sites "Wordnik,CyberDefenders"15 times: 7 of 15 runs fail on master, 0 of 15 after this change. That job gates every data.json PR and also runs nightly over the full manifest in the exclusions updater, so the noise is recurring rather than one-off.The fix walks the pattern and applies bounds only where a quantifier can appear. Escaped literals are skipped, which also fixes
\+, where the+is a real quantifier that the old lookbehind declined to bound. A lower bound above the cap now lifts the bound for that one quantifier instead of leaking into the rest of the pattern, which is what thenonlocalin the previous version did.Behaviour outside character classes is unchanged:
+,*and{n,}still collapse to the same bounds, and generated handles are still capped at 15 characters.Tests cover the quantifier cases, the escaping rules, both manifest patterns above, and a sweep asserting that every
regexCheckin the manifest still accepts what its bounded form generates. Six of them fail against the current implementation. Patterns using lookarounds are skipped in the sweep, since rstr cannot honour those and they fail independently of bounding.