Vulnerable Regular Expressions

[cristianstaicu]

The following regular expressions used in underscore and unescapeHTML methods are vulnerable to ReDoS:

/([A-Z\d]+)([A-Z][a-z])/g
/\&([^;]+);/g

The slowdown is moderately low (for 50,000 characters around 2 seconds matching time). I would suggest one of the following:

• remove the regex,
• anchor the regex,
• limit the number of characters that can be matched by the repetition,
• limit the input size.

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

[kvanbere]

anchor the regex

Could you please elaborate on the fix, I'm curious. Thank you!

[cristianstaicu]

Sorry, that was a standard comment I sent around. In this case it would not work! The anchoring solution is well suited when the regex is used for validation purposes. Let us say you want to check an input string is an email address. Instead of having a simple regex like:

/[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]/

you should have something like:

/^[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]$/

Word bounadries can also be used as explained here:
https://www.regular-expressions.info/email.html

This will decrease the complexity of the matching since the engine will only try to match the regex once, not n (length of input string) times like in the case of the first regex.

[kvanbere]

Thanks for the explanation!

I hope this gets fixed soon, it's giving my package red lights :(

[burt202]

This issue now seems to being picked up by an nsp check making our builds red too. See https://nodesecurity.io/advisories/536

[danzGentrack]

The slowdown is moderately low (for 50,000 characters around 2 seconds matching time).

Just wondering why it is a problem? For example, the string "underscore" algorithm could be a very complex one and may need lots of seconds to process a 50,000 characters long string.

[cristianstaicu]

I recommend the following two articles for understanding the issue:
https://blog.liftsecurity.io/2014/11/03/regular-expression-dos-and-node.js/
https://dl.acm.org/citation.cfm?id=3065916

Basically, the problem comes from the fact that the regex matching is done in the main loop of Node.js and thus blocking this loop for 2 seconds is equivalent to blocking all the incoming requests for 2 seconds.

[danzGentrack]

Thanks for the info. The example in the first link is a classic evil regex and it can be written in a more efficient way.
var emailExpression = /^([a-zA-Z0-9_.-])+@(([a-zA-Z0-9-])+.)+([a-zA-Z0-9]{2,4})+$/;

The regex used in the "underscore" case is fine: /([A-Z\d]+)([A-Z][a-z])/g , which may not incur much performance penalty compared to an alternative implementation. I understand the impact of slowing regex matching to an application. My point is that if a regular expression is used correctly, then it is not a problem to be slow.

[revelt]

For posterity, Snyk also reports it:
https://snyk.io/vuln/npm:string:20170907

I'm looking forward to solution. The html-stripping features, for example, at the competition are very poor and I can't find the alternative for String.js..

[ericnorris]

Hey there! I haven't looked too closely at this issue but it was linked by @revelt above. I'd definitely suggest using the striptags library for removing HTML. It is based on the PHP function strip_tags, which is widely known to be safe.

In the README I link to two articles, one indicating the regular expressions are not safe for removing dangerous HTML (here), and the second demonstrating that PHP's strip_tags function is not vulnerable (here).

https://github.com/ericnorris/striptags take a look to see if that works for you - it uses a state machine very similar to the one that is in the PHP internals instead of a regular expression. As a result it is super fast and just as secure!

[stevenvachon]

This is a critical issue. The author and the contributors don't seem to care at all. Kill this project with fire.

[revelt]

@Steve come on, it's not that bad :) team is just regrouping, I'm sure guys will fix that eventually

[yieme]

@jprichardson, string.js is a nice library and I would prefer to continue using it; however, such a vulnerability isn't something that can be used in production. Are you aware of this issue or has the project been abandoned as the last update was about 11 months ago?