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

remove catastrophic backtracking vulnerability #7115

Merged
merged 1 commit into from Mar 4, 2018

Conversation

Projects
None yet
2 participants
@davisjam
Contributor

davisjam commented Feb 26, 2018

Problem:
A regex to parse function names was vulnerable to catastrophic
backtracking.

Solution:
Alter the regex to make it safe.
The new regex matches almost the same language.
The only differences should not be a problem in the input space.

This regex is not exploitable for REDOS as currently used.
This change is for future-proofing.

@davisjam

This comment has been minimized.

Contributor

davisjam commented Feb 26, 2018

One point of confusion.

Railroad diagram: old
Railraod diagram: new

Should the final character in the regex be an opening brace '{' rather than an opening paren '('?

@daleharvey

This comment has been minimized.

Member

daleharvey commented Mar 3, 2018

It's pulling the function name out of function hello() { so ( seems correct, this is failing our lint rules about regex escapes however

@davisjam davisjam force-pushed the davisjam:FixCatastrophicBacktracking branch from 358d70f to 858ae57 Mar 3, 2018

@davisjam

This comment has been minimized.

Contributor

davisjam commented Mar 3, 2018

@daleharvey

  1. Fixed the linting problem.
  2. Thanks for clarifying the input format. My own fault for submitting the original PR while tired.

I can't find a place where this function is tested, though that may be my own unfamiliarity with the project. Can you point me to it?

remove catastrophic backtracking vulnerability
Problem:
A regex to parse function names was vulnerable to catastrophic
backtracking.

Solution:
Alter the regex to make it safe.
The new regex matches the same language.

This regex is not exploitable for REDOS as currently used.
This change is for future-proofing.

@davisjam davisjam force-pushed the davisjam:FixCatastrophicBacktracking branch from 858ae57 to 9348778 Mar 3, 2018

@daleharvey

This comment has been minimized.

Member

daleharvey commented Mar 4, 2018

We dont have unit tests for all functions and most of the time depend on the integration tests to ensureall is good which they are here so happy to merge, thanks so much :)

@daleharvey daleharvey merged commit 64894da into pouchdb:master Mar 4, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment