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

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

Merged
merged 1 commit into from May 16, 2017

Conversation

4 participants
@karenyavine
Copy link
Contributor

karenyavine commented Apr 12, 2017

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

This comment has been minimized.

Copy link

Delagen commented May 16, 2017

#90

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@leo

This comment has been minimized.

Copy link
Member

leo commented May 16, 2017

Sweet! Thank you. 😊

@karenyavine

This comment has been minimized.

Copy link
Contributor

karenyavine commented May 16, 2017

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

@Delagen

This comment has been minimized.

Copy link

Delagen commented May 16, 2017

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

@leo

This comment has been minimized.

Copy link
Member

leo commented May 16, 2017

@karenyavine It was just released with version 2.0.0! 🎉

@popomore popomore referenced this pull request May 16, 2017

Merged

update "debug" to v1.0.0 #454

@grnd

This comment has been minimized.

Copy link

grnd commented May 17, 2017

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

@leo

This comment has been minimized.

Copy link
Member

leo commented May 17, 2017

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

@Delagen

This comment has been minimized.

Copy link

Delagen commented May 17, 2017

@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

This comment has been minimized.

Copy link

grnd commented May 17, 2017

+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

Remove snyk badge and add dependencies badge
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.

brennoo added a commit to brennoo/example-nodejs-express that referenced this pull request Aug 17, 2017

Upgrade express version to 0.15.X
Just to make sure this example if free of bugs, such as this one for ms@0.7.1 -> zeit/ms#89

brennoo added a commit to brennoo/example-nodejs-express that referenced this pull request Aug 17, 2017

Upgrade express version to 4.15.X
Just to make sure this example if free of bugs, such as this one for ms@0.7.1 -> zeit/ms#89

sjparkinson added a commit to Financial-Times/n-health that referenced this pull request Sep 13, 2017

@sjparkinson sjparkinson referenced this pull request Sep 13, 2017

Merged

Update ms package #72

@yoavmmn yoavmmn referenced this pull request Sep 25, 2017

Closed

CHANGELOG for 2.0.0 #98

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment