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

Quadratic regex in readline.js #26596

Closed
davisjam opened this issue Mar 11, 2019 · 6 comments
Closed

Quadratic regex in readline.js #26596

davisjam opened this issue Mar 11, 2019 · 6 comments
Labels
readline Issues and PRs related to the built-in readline module.

Comments

@davisjam
Copy link
Contributor

Version: master branch
Subsystem: readline

lib/readline.js contains this code snippet:

Interface.prototype._wordLeft = function() {
  if (this.cursor > 0) { 
    var leading = this.line.slice(0, this.cursor);
    var match = leading.match(/(?:[^\w\s]+|\w+|)\s*$/);
    this._moveCursor(-match[0].length);
  }
};

This regex is quadratic: /(?:[^\w\s]+|\w+|)\s*$/.

I would be shocked if this were a viable ReDoS vector (hence the public bug report), but if this.line can be long (100K chars?) then it might present a performance problem.

I have not investigated reachability/triggerability nor the use cases of readline.

@davisjam davisjam added the readline Issues and PRs related to the built-in readline module. label Mar 11, 2019
@BridgeAR
Copy link
Member

It is very unlikely that this.line would exceed even 100 characters. But it is theoretically possible. I do not worry about the performance in this case due to the average short input per line.

@Fishrock123
Copy link
Contributor

I don't think it's a massive problem if a copy-paste takes a second to parse or something

@sam-github
Copy link
Contributor

If its just our REPL, its hard to see this as an issue.

If readline is attached over TLS to a remote user, and used to implement some remote protocol, would we care then?

Also, what would it take to fix this?

@nodejs/security @nodejs/security-wg

@cjihrig
Copy link
Contributor

cjihrig commented Mar 18, 2019

At one point, @mscdex left the following comment:

FWIW it's important to remember that readline can be used outside of a repl context....

I'm not sure why it was deleted, but any use of readline outside of the REPL (or similar) could be an issue.

@addaleax
Copy link
Member

To clarify, this function is only called in response to key presses.

Also, what would it take to fix this?

Manually unrolling the regexp should work, I think.

@Fishrock123
Copy link
Contributor

Seems like a fix was landed in e4e2b0c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
readline Issues and PRs related to the built-in readline module.
Projects
None yet
Development

No branches or pull requests

6 participants