fix(verify): bound RegExp "audience" matching against an oversized aud claim (ReDoS) - #1044
Open
yfwmaniish wants to merge 1 commit into
Open
fix(verify): bound RegExp "audience" matching against an oversized aud claim (ReDoS)#1044yfwmaniish wants to merge 1 commit into
yfwmaniish wants to merge 1 commit into
Conversation
…d claim (ReDoS) A RegExp `audience` is tested via `RegExp#test(targetAudience)`, where targetAudience comes straight from the token's aud claim - fully attacker-controlled. An application whose audience regex has catastrophic-backtracking potential (nested quantifiers, ambiguous alternation - a documented-common shape for multi-tenant/wildcard audience matching) can be driven into a multi-second-or-worse hang by a single crafted token; reproduced directly against current master (2s for a 24-char crafted aud against /(a+)+$/, growing exponentially from there). Adds a `maxAudienceLength` option (default 256): an aud claim longer than this is treated as a non-match rather than being handed to the regex at all, so a single verify() call is bounded regardless of how the application's regex is written. Only applies to RegExp audiences; string audience checks (plain equality) are unaffected. Being upfront about what this does and doesn't do: this is a defense-in-depth bound, not a complete fix. It closes the unbounded-length attack surface, but a regex that's already catastrophic at ~20-25 characters (like the /(a+)+$/ example above) isn't stopped by a 256-char default, since the malicious input is shorter than the cap - no cap can be both tight enough to block that and loose enough to allow realistic audience strings through. The actual fix for that case is for the application's own audience regex to not have catastrophic-backtracking potential in the first place; documented as such in the README alongside the new option. Fixes auth0#1031.
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
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.
Fixes #1031.
What
A RegExp
audienceis tested viaRegExp#test(targetAudience)inverify.js, wheretargetAudiencecomes straight from the token'saudclaim - fully attacker-controlled:If the application's audience regex has catastrophic-backtracking potential (nested quantifiers, ambiguous alternation - a documented, common shape for multi-tenant/wildcard-subdomain audience matching), a single crafted token can hang
verify()for seconds to minutes. Reproduced directly against currentmaster:Time grows exponentially with input length from there.
Fix
Added a
maxAudienceLengthoption (default256). Anaudclaim longer than this is treated as a non-match rather than being passed to the regex at all, so a singleverify()call is bounded regardless of how the application's regex is written. Only applies to RegExp audiences - string audience checks (plain equality) are O(n) and unaffected.Being upfront about the limits of this fix
I want to be precise here rather than overclaiming. This is a defense-in-depth bound, not a complete fix for catastrophic-backtracking regexes:
aud(kilobytes) and drive the hang arbitrarily high. Now the worst case is bounded to whatevermaxAudienceLengthcharacters cost against the application's specific regex./(a+)+$/example above is already catastrophic at ~20-25 characters - well under the 256-char default. No length cap can be both tight enough to block a 24-character attack and loose enough to let realistic audience strings (tenant subdomains, URLs - often 30-60+ characters) through. I checked: a cap tight enough to stop this specific example would reject essentially every realistic RegExp-audience use case, so I didn't set the default that low.maxAudienceLengthis configurable, so an application that knows its audience format is short (e.g. always under 32 chars) can tighten it meaningfully.I think this is still worth merging as a real, tested, backward-compatible improvement, but wanted to be explicit rather than have this read as "fixes ReDoS" when it's better described as "removes the unbounded-length case and gives applications a knob."
Testing
maxAudienceLengthvalidation tests (mirrors the existingclockTimestamp/allowInvalidAsymmetricKeyTypesoption-validation style) - including a test forNaN, which caught a real bug in my first attempt at the validation guard (NaN <= 0isfalse, so it silently passed through; fixed by inverting to!(value > 0))./(a+)+$/pattern with a 300-charaud(past the default cap, so it's fast either way in CI - never runs the actual unbounded-length case in the test suite), asserting the call completes in well under 500ms and returns the normal "audience invalid" error.maxAudienceLengthis respected, and stringaudiencechecks are unaffected by the cap.verify.js, kept the tests - the 4 new validation tests and the "custom maxAudienceLength" test all fail as expected (the ReDoS-timing test itself was excluded from this run via--grep --invert, since without the fix it would hit genuine catastrophic backtracking against the 300-char input and hang rather than fail cleanly - confirmed separately, at a bounded scale, via the reproduction above). Restored.mocha- 519/519 passing (1 pre-existing pending, unrelated).eslintclean on all three changed source/test files.