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: use rtrim, not unsafe /X+$/ #1260

Merged
merged 3 commits into from
Jun 3, 2018
Merged

Conversation

davisjam
Copy link
Contributor

@davisjam davisjam commented May 9, 2018

Problem:
replace(/X+$/, '') is vulnerable to REDOS

Solution:
Replace these instances with a custom rtrim.

Problem:
replace(/X+$/, '') is vulnerable to REDOS

Solution:
Replace all instances I could find with a custom rtrim
@davisjam
Copy link
Contributor Author

davisjam commented May 9, 2018

This is an uncontentious excerpt from #1226. It affects "clean-up" regexes, not "parsing" regexes.

@davisjam davisjam requested a review from UziTech May 9, 2018 15:10
lib/marked.js Outdated
} else {
allButC = true;
}
var mustMatchC = !allButC;
Copy link
Member

@UziTech UziTech May 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone calls rtrim(cap, '\n', false) allButC will be true? seems like a potential source of many future errors.

I would just replace the mustMatchC's below with !allButC

It should be fine for allButC to be truthy or falsey

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll tweak the naming.

@davisjam
Copy link
Contributor Author

(I am on vacation until end of May so won't be acting on this until then @joshbruce @styfle @UziTech).

@davisjam
Copy link
Contributor Author

davisjam commented Jun 2, 2018

@UziTech How's the new version look? I think it's more readable now, especially the clearer variable names in the final "step left" section.

lib/marked.js Outdated
// /c*$/ is vulnerable to REDOS.
// invert: Remove suffix of non-c chars instead. Default false.
function rtrim(str, c, invert) {
if (typeof invert === 'undefined' || !invert) {
Copy link
Member

@styfle styfle Jun 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this if statement necessary?
It seems like this would be the same (falsly by default) without explicitly setting it to false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears not. Too many years of C have addled my brain.

@davisjam
Copy link
Contributor Author

davisjam commented Jun 2, 2018

@styfle 0610f9f removes the if check you flagged.

@styfle styfle changed the title security: rtrim, not unsafe /X+$/ idiom security: use rtrim, not unsafe /X+$/ idiom Jun 3, 2018
@styfle styfle changed the title security: use rtrim, not unsafe /X+$/ idiom security: use rtrim, not unsafe /X+$/ Jun 3, 2018
@styfle styfle merged commit 37a9f1f into markedjs:master Jun 3, 2018
@styfle styfle added this to the 0.5.0 - Commonmark Compliance milestone Aug 15, 2018
This was referenced Apr 6, 2020
zhenalexfan pushed a commit to zhenalexfan/MarkdownHan that referenced this pull request Nov 8, 2021
security: use rtrim, not unsafe /X+$/
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.

3 participants