-
Notifications
You must be signed in to change notification settings - Fork 7.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[bugfix] Fix redos in preprocessRFC2822 regex #6015
[bugfix] Fix redos in preprocessRFC2822 regex #6015
Conversation
Fixes: [moment#2936](moment#6012) Directly match the comment tokens in preprocessRFC2822 regex to resolve the problem [Regular Expression Denial of Service (ReDoS)moment#6012](moment#6012)
change the direct matching regex to a local backtracking regex to support all characters in the token comment
@vovikhangcdv can you explain a bit how this is being fixed? From the linked issue it looks like any open-close relationship that we have to track (not sure if there are more), has the same issue. Also is this compatible all the way back to Roman Empire JavaScript? |
Hi @ichernev,
Firstly, look back to the original regex:
This process will be executed for the full input string and repeated with substring (remove the start character) until match all the three steps or all characters are checked. Consider the evil payload format: Now, look to the suggested fix: The matching process would be similar to the old regex, except that step 2 uses non-greedy searching but a "look-ahead" mechanism. It will immediately fail when it matches the second
The mission of
I am not sure what you asking about. But if you are concerned about the compatibility of the fix, I have tested the fix and confirm that it passed 160,555 test cases of unit tests. Or if I missing something, please let me know, and we will figure it out together. References: |
Hi is there update to this fix? Thank you |
Hi @ichernev, could you review the PR? |
Hi @ichernev, any update for this PR? Thanks! |
Hey, sorry for the delay, I'll release a build in the coming days with the fix, I was a bit worried about the lookahead (if it was supported), but the current implementation uses whitelist, which is better. |
Good to see you back. If you can confirm the issue, could you validate my report on Hunter?. It will help my work too. Thank you, @ichernev. |
How about we change |
I can merge the fix with |
update regex to avoid matching more open brackets from @ichernev suggestion
Hey @ichernev, |
As a researcher, being credited on |
Reading through the specification of the date format, it looks like formally a comment (stuff in brackets) can have a nested comment inside:
note how To be fair, the Ruby standard library fails to parse dates with comments, so I'm not 100% sure we should deal with this crap. But if we do, there should be code to track open/closed parenthesis, if there is anything unbalanced the parsing should fail (because there are no brackets allowed in the actual string). This code is linear, so won't introduce bottlenecks. If we keep the current comment approach (disallow open and close paren inside comment), it will "capture" (and remove) only the inner most comment, and the parsing will fail if there are nested comments (which, given the legacy-ness of all of this might be fine :)). |
Yeh, I had noticed about nested comment in RFC2822 when trying to fix the issue before and was quite struggling with it :)). But looking at the old version of what |
Advisory link: GHSA-wc69-rhjr-hc9g |
@ichernev - would it be possible to add the following report URL to the advisory? |
Mitigation for CVE-2023-22467 (see moment/moment#6015 (comment))
* chore: upgrade luxon Mitigation for CVE-2023-22467 (see moment/moment#6015 (comment)) * chore: update `@types/luxon` and fix usage
Fixes: #6012
Using the "look-ahead" mechanism regex in
preprocessRFC2822()
to resolve the problem Regular Expression Denial of Service (ReDoS)#6012