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: remove catastrophic backtracking vulnerability #10019

Merged
merged 1 commit into from Feb 27, 2018

Conversation

@davisjam
Copy link
Contributor

davisjam commented Feb 24, 2018

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Change template substitution regex to exclude fields with whitespace.
This addresses possible O(n^2) catastrophic backtracking behavior.
The function behavior is unchanged.

Very unlikely to be exploited. For #10002.

Is there anything you'd like reviewers to focus on?

No.

@jsf-clabot

This comment has been minimized.

Copy link

jsf-clabot commented Feb 24, 2018

CLA assistant check
All committers have signed the CLA.

@eslint eslint bot added the triage label Feb 24, 2018
@davisjam davisjam force-pushed the davisjam:FixVulnRegex branch from 42ebc8c to 7499205 Feb 24, 2018
@davisjam davisjam changed the title security: fix catastrophic backtracking vulnerability Fix: remove catastrophic backtracking vulnerability Feb 24, 2018
Copy link
Contributor

ljharb left a comment

So it's just the \s* that's problematic here?

return text.replace(/\{\{([^{}]+?)\}\}/g, (fullMatch, term) => {

// Strip leading and trailing whitespace.
term = term.replace(/^\s+|\s+$/g, "");

This comment has been minimized.

Copy link
@ljharb

ljharb Feb 24, 2018

Contributor

term.trim()?

This comment has been minimized.

Copy link
@davisjam

davisjam Feb 24, 2018

Author Contributor

Haha! I knew there had to be a function for that.

Change template substitution regex to exclude fields with whitespace.
This addresses possible O(n^2) catastrophic backtracking behavior.

Very unlikely to be exploited. For #10002.
@davisjam davisjam force-pushed the davisjam:FixVulnRegex branch from 7499205 to 58ad0af Feb 24, 2018
@davisjam

This comment has been minimized.

Copy link
Contributor Author

davisjam commented Feb 24, 2018

So it's just the \s* that's problematic here?

A bit more complex than that. The original pattern contains a sub-sequence like this:

\s*[^{}]+\s*

Because the middle group's character class includes whitespace, it will match a superset of this pattern, which is easier to reason about:

\s*\s+\s*

If the input contains a long sequence of whitespace, the regex engine has to decide when to move from one \s to the next.
If the input doesn't match the pattern, the regex engine tries every subdivision of the whitespace looking for a match. It lays breadcrumbs each time it makes a decision to resolve ambiguity, and revisiting each breadcrumb is called backtracking. If there is a long sequence of whitespace, it has a lot of breadcrumbs to visit, and will get "stuck". This behavior is called catastrophic backtracking.

The blow-up in this case is O(n^2). In the worst case catastrophic backtracking can be O(2^n).

@davisjam

This comment has been minimized.

Copy link
Contributor Author

davisjam commented Feb 24, 2018

It would be helpful if a developer can comment on whether this is a very low-risk security problem or just a potential problem if someone else were to copy/paste later.

If a real problem, I will follow up with Snyk.io to get a low-severity vulnerability ID assigned.

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Feb 24, 2018

It's an eslint rule config; imo it's the lowest nonzero risk you can get :-)

@not-an-aardvark

This comment has been minimized.

Copy link
Member

not-an-aardvark commented Feb 24, 2018

I don't think this is realistically exploitable, because only an ESLint rule can provide a string to that regex, and an ESLint rule already can execute arbitrary code. So I'm fine with removing it for performance reasons, but I don't consider this to be a security issue in its current location.

@not-an-aardvark not-an-aardvark added bug core accepted and removed triage labels Feb 24, 2018
Copy link
Member

not-an-aardvark left a comment

Looks good to me, thanks!

@ljharb
ljharb approved these changes Feb 25, 2018
Copy link
Member

platinumazure left a comment

LGTM, this seems reasonable and doesn't make things much harder to read. Thanks for the PR!

@not-an-aardvark not-an-aardvark merged commit f6901d0 into eslint:master Feb 27, 2018
5 checks passed
5 checks passed
commit-message Commit message follows guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
This was referenced Mar 22, 2018
@renovate renovate bot mentioned this pull request Mar 23, 2018
@eslint eslint bot locked and limited conversation to collaborators Aug 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.