search

dodo / node-slug

slugifies even utf-8 chars!
MIT License
1.08k stars 92 forks source link

Vulnerable Regular Expression #82

Open cristianstaicu opened 7 years ago

cristianstaicu commented 7 years ago

The following regular expression used in parsing the input string is vulnerable to ReDoS:

/^\s+|\s+$/g

The slowdown is moderately low: for 50.000 characters around 2 seconds matching time. However, I would still suggest one of the following:

If needed, I can provide an actual example showing the slowdown.

evilpacket commented 6 years ago

As this issue is public, we've issued an advisory here https://nodesecurity.io/advisories/537 as well as requested help from the public to submit a PR / help patch this issue.

wesleytodd commented 6 years ago

If it helps anyone, here are some references for similar issues which have been fixed in different ways:

https://github.com/jshttp/forwarded/commit/d469116eda4931fbe1c0ccb29497b35930bfa328 https://github.com/jshttp/fresh/commit/21a0f0c2a5f447e0d40bc16be0c23fa98a7b46ec https://github.com/broofa/node-mime/commit/1df903fdeb9ae7eaa048795b8d580ce2c98f40b0 https://github.com/visionmedia/debug/commit/f53962e944a87e6ca9bb622a2a12dffc22a9bb5a

I don't have the time to work on this right now, but we have dealing with these issues reported by @cristianstaicu for the past couple of weeks, so it was easy for me to pull up these references as to how others have addressed the ReDOS issues.

vvanmol commented 6 years ago

Hello,

One of the NPM package we use had been blocked by our Nexus IQ Server because it has a dependency to slug which is blocked due to this vulnerability...

I would like to fix this and I have checked the different examples of solution provided by @wesleytodd but in this case the RegEx is used to trim a String...

According to you people would the use of the String.prototype.trim function be a valid solution for this case ? I'm no Security expert but I would definitely like to help...

cutesquirrel commented 6 years ago

👍

Trott commented 5 years ago

0.9.2 has been published with this issue fixed. https://www.npmjs.com/package/slug/v/0.9.2

@cristianstaicu Can you please close this issue?

rciccone91 commented 5 years ago

Hi guys, just one quick question, is this issue really fixed? Our Nexus IQ Server is still detecting this vulnerability. Maybe it's because this issues has not been closed yet? Thanks!

Trott commented 5 years ago

is this issue really fixed?

Yes.

Our Nexus IQ Server is still detecting this vulnerability. Maybe it's because this issues has not been closed yet?

Alas, I have publishing rights on the npm module but no write privileges on this repository. So I can publish a fix (which I have) but I can't close this issue.

Do make sure you are using 0.9.2. Previous versions do not have the fix.

vvanmol commented 5 years ago

Hello,

Sorry @Trott for the late reply. And thank you for taking over this package.

Thanks !!