-
-
Notifications
You must be signed in to change notification settings - Fork 79
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
Fix potential ReDoS #37
Conversation
Hi, normally we'd appreciate an email prior to submitting security patches, please keep this in mind as it's a pretty typical part of responsible disclosure. Can you also provide some context for the redos? What sorts of input will cause issues? |
Thanks for reminding. Sorry, I can't find your email. Could you tell me your email? |
Well cat is out of the bag now, just write here. You can find emails in the commit messages of repositories by the way. |
Proof of Concept import ansiRegex from 'ansi-regex';
for(var i = 1; i <= 50000; i++) {
var time = Date.now();
var attack_str = "\u001B["+";".repeat(i*10000);
ansiRegex().test(attack_str)
var time_cost = Date.now() - time;
console.log("attack_str.length: " + attack_str.length + ": " + time_cost+" ms")
} The ReDOS is mainly due to the sub-patterns |
Thank you for the reproduction and the patch, was able to reproduce. I'll push out an update immediately. |
Published as |
Thanks. It would have been good with a regression test to ensure we don't accidentally regress the regex in the future. |
CVE-2021-3807 was assigned for this issue. |
Yet another example of how laughably broken CVE scores are. |
@yetingli first of all, big thanks for the contribution! 🙏🏼 Secondly, thanks to this PR I've also learned today about security policies within repos/github and noticed this too: https://github.com/chalk/ansi-regex/security/policy —perhaps this can help smoothen things out in the future. 😊 |
This is a backport of chalk@8d1d7cd the test suite on the 3.0.0 branch is broken but I've manually verified that no additional tests are broken and that this patch fixes the REDOS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No description provided.