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

Fix ReDOS #1405 #1408

Closed
wants to merge 1 commit into from
Closed

Conversation

Feder1co5oave
Copy link
Contributor

@Feder1co5oave Feder1co5oave commented Jan 13, 2019

Fixes #1405, reverts #1305
Something went wrong with the security review of that PR :(

I don't think we'll ever support multiple nested anything, anywhere. Regexes are just not made to do that easily.

I'd consider releasing this as a patch release to all minor versions available on npm.

@styfle
Copy link
Member

styfle commented Jan 13, 2019

@davisjam Can you look at this?

I would hate to revert because I believe that @Trott fixed that specific test case because they were running into a problem while rendering the Node.js docs with marked.

@Trott
Copy link
Contributor

Trott commented Jan 13, 2019

I would hate to revert because I believe that @Trott fixed that specific test case because they were running into a problem while rendering the Node.js docs with marked.

If it helps at all: We're not using marked in Node.js anymore, so reverting it won't hurt Node.js, at least.

@UziTech
Copy link
Member

UziTech commented Jan 14, 2019

@davisjam on #1305 (comment):

I will cryptically remark that its vulnerability status is unchanged.

I don't think this regex for link is any more secure then the one in #1305. It just happens to fix this redos attack.

I think this is a situation where we need to simplify the regex and use some other method to get the parts of the link, similar to what we do with tables.

@styfle styfle requested a review from davisjam January 14, 2019 19:28
@Feder1co5oave
Copy link
Contributor Author

@UziTech sorry I totally forgot about this issue.
I can't clearly explain why, but I'm pretty confident this regex is secure, while the one currently in master is not.
It has to do with the fact that, when trying to match /\([^\s\x00-\x1f\\()]*\)/, the engine requires an opening (, something that certainly is not a closing ), and then the closing ). Simple as that.
Whereas the offending /\([^\s\x00-\x1f\\]*\)/ is dangerous because to match it you need to:

  • find the (
  • match any characters in [^\s\x00-\x1f\\(]
    if a ) is encountered, we cannot know whether it is the ending ), or one in the middle. We must try both.
    • try matching it and as belonging to [^\s\x00-\x1f\\()], and keep going.
      • If $ is encountered, it might be because the last ) was in fact the ending one. Backtrack.
      • Anything in [^\s\x00-\x1f\\(] is found. Eat it.
      • Another ( is encountered. We cannot know whether.... branch once again, and then backtrack if applicable.

For every ) encountered in the input, exponential backtracking takes place.

@UziTech
Copy link
Member

UziTech commented Jan 30, 2019

@Feder1co5oave yes that could be why it fixes this situation, but I was saying the regex in general is no more secure. In other words we will have to change the regex in a different way to make it secure in the future.

A better solution would be to fix the regex (and possibly use some function instead of a single regex) now instead of in the future.

@atodorov
Copy link

atodorov commented Feb 5, 2019

How about a quick fix with this patch and then redo the regex into a function ?

@UziTech
Copy link
Member

UziTech commented Feb 7, 2019

I'd like to hear from @davisjam on this

@UziTech UziTech mentioned this pull request Feb 7, 2019
4 tasks
@Pablodotnet
Copy link

We need this to be merged to fix the vulnerability snyk is advertising https://snyk.io/vuln/SNYK-JS-MARKED-73637

@UziTech
Copy link
Member

UziTech commented Feb 14, 2019

#1414 also fixes the vulnerability and still allows urls with matching parentheses

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 this pull request may close these issues.

6 participants