Skip to content

Limit str to 100 to avoid ReDoS of 0.3s#89

Merged
leo merged 1 commit into
vercel:masterfrom
karenyavine:avoid-redos
May 16, 2017
Merged

Limit str to 100 to avoid ReDoS of 0.3s#89
leo merged 1 commit into
vercel:masterfrom
karenyavine:avoid-redos

Conversation

@karenyavine

@karenyavine karenyavine commented Apr 12, 2017

Copy link
Copy Markdown

Hey,
The fix for CVE-2015-8315 was incomplete. Limiting the input to 10,000 chars reduced the risk significantly but it is still possible to cause a delay of 0.3s. Suggested to limit it to 100 chars as there is no reason for a date to be over that :)

PoC:

ms=require('ms');
ms('1'.repeat(9998) + 'Q')
//Takes about ~0.3s

Thanks,
Karen Yavine
Security Analyst @ snyk.io

@delagen

delagen commented May 16, 2017

Copy link
Copy Markdown

#90

@leo leo merged commit caae298 into vercel:master May 16, 2017
@leo

leo commented May 16, 2017

Copy link
Copy Markdown
Contributor

Sweet! Thank you. 😊

@karenyavine

Copy link
Copy Markdown
Author

Hey @leo!
Thanks for merging the PR!
Wondered when a new version will be released to npm?

@delagen

delagen commented May 16, 2017

Copy link
Copy Markdown

Sad that module authors does not care about performance at all.

@leo

leo commented May 16, 2017

Copy link
Copy Markdown
Contributor

@karenyavine It was just released with version 2.0.0! 🎉

@grnd

grnd commented May 17, 2017

Copy link
Copy Markdown

@leo, thanks for the fix and the release.
I was wondering why was this a breaking change, requiring a major release?

@leo

leo commented May 17, 2017

Copy link
Copy Markdown
Contributor

Because we lowered the limit. So if people have longer strings in place, it will break.

@delagen

delagen commented May 17, 2017

Copy link
Copy Markdown

@grnd @leo I offer solution to not simply decrease limit, but to rework parsing, that boost parse speed up to 2 times, look at #90, code looks cleaner and more simple

@grnd

grnd commented May 17, 2017

Copy link
Copy Markdown

+1 on @delagen's solution. Both the previous fix of limiting to 10000 chars, and the new one of limiting to 100 chars were merely a workaround.

tkadlec referenced this pull request in dashersw/cote Jun 5, 2017
Snyk notoriously reports on the ms package used by socket.io, which
actually is no vulnerability, and the author rejected snyk's fix.
It looks bad on the README, so removing snyk until they fix their attitude.
@yoavmmn yoavmmn mentioned this pull request Sep 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants