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: remediate ReDOS further

Merged
merged 2 commits into from Oct 23, 2021
Merged

Conversation

Trott
Copy link
Collaborator

@Trott Trott commented Sep 23, 2021

No description provided.

@Trott Trott mentioned this pull request Sep 23, 2021
test.js Outdated
@@ -23,6 +23,19 @@ it('should trim off \\r\\n', function () {
it('should not be susceptible to exponential backtracking', function () {
var start = Date.now();
trimOffNewlines('a' + '\r\n'.repeat(1000) + 'a');
trimOffNewlines(`a${'\r\n'.repeat(1000)}a`.repeat(1000));
Copy link
Collaborator Author

@Trott Trott Sep 23, 2021

Choose a reason for hiding this comment

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

This additional test fails with the current code (although perhaps not on a fast machine?) but succeeds with the change here.

Copy link
Collaborator Author

@Trott Trott Sep 23, 2021

Choose a reason for hiding this comment

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

Because of the template string, the test won't run in Node.js 0.10, which the package.json says is supported.

Copy link
Collaborator Author

@Trott Trott Sep 23, 2021

Choose a reason for hiding this comment

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

I've updated the test to work in Node.js 0.10.x but mocha and xo are still incompatible.

@Trott
Copy link
Collaborator Author

Trott commented Sep 23, 2021

Some simple benchmarking of the change here is pretty compelling:

Current version:

$ time node -e 'require("trim-off-newlines")(`a${"\r\n".repeat(1000)}a`.repeat(1000))'
node -e   8.22s user 0.13s system 92% cpu 9.069 total
$ 

With this change:

$ time node -e 'require("trim-off-newlines")(`a${"\r\n".repeat(1000)}a`.repeat(1000))'
node -e   0.05s user 0.10s system 45% cpu 0.316 total
$ 

From over 8 seconds to less than a tenth of a second is pretty persuasive.

@Trott
Copy link
Collaborator Author

Trott commented Sep 23, 2021

I've added the commit to pin the version of mocha to one that is compatible with 0.10.x so the tests can be run with node_modules/.bin/mocha. Unfortunately, npm test still fails because there is no version of xo that installs with 0.10.x-compatible dependencies.

@Trott
Copy link
Collaborator Author

Trott commented Sep 23, 2021

Although thinking about it more, this is much less performant with long strings that don't contain an overwhelming number of \r and \n.

Current:

$ time node -e 'require("trim-off-newlines")(`a`.repeat(1e8))'      
node -e 'require("trim-off-newlines")(`a`.repeat(1e8))'  0.16s user 0.05s system 95% cpu 0.219 total
$ 

With this PR:

$ time node -e 'require("trim-off-newlines")(`a`.repeat(1e8))'
node -e 'require("trim-off-newlines")(`a`.repeat(1e8))'  5.39s user 0.24s system 83% cpu 6.732 total
$ 

A more straightforward solution might be better.

@Trott
Copy link
Collaborator Author

Trott commented Sep 23, 2021

OK, I've updated it with an additional performance test and I think this is now fully ReDoS remediated. /ping @hadasbloom

(I still want to test at least one additional edge case, but that's general perf and not ReDoS.)

@Trott
Copy link
Collaborator Author

Trott commented Sep 23, 2021

Performance seems OK for what I believe to be the worst case with this algorithm, which is a non-newline character followed by many newline characters. And in 1.0.2, 1.0.1, and 1.0.0 of this module, these strings result in "maximum call stack size exceeded" so it is certainly more performant than that!

(First run below, which takes 1.61 seconds, is running with the code in this PR.)

$ time node -e 'require("trim-off-newlines")("a" + "\r\n".repeat(1e8))'
node -e 'require("trim-off-newlines")("a" + "\r\n".repeat(1e8))'  1.61s user 0.08s system 97% cpu 1.726 total
$ npm install trim-off-newlines@1.0.2 
...
$ $ time node -e 'require("trim-off-newlines")("a" + "\r\n".repeat(1e8))'
/Users/trott/temp/node_modules/trim-off-newlines/index.js:6
	return str.replace(regex, '');
	           ^

RangeError: Maximum call stack size exceeded
    at String.replace (<anonymous>)
    at module.exports (/Users/trott/temp/node_modules/trim-off-newlines/index.js:6:13)
    at [eval]:1:29
    at Script.runInThisContext (node:vm:129:12)
    at Object.runInThisContext (node:vm:305:38)
    at node:internal/process/execution:81:19
    at [eval]-wrapper:6:22
    at evalScript (node:internal/process/execution:80:60)
    at node:internal/main/eval_string:27:3
node -e 'require("trim-off-newlines")("a" + "\r\n".repeat(1e8))'  0.17s user 0.14s system 72% cpu 0.431 total
$ npm install trim-off-newlines@1.0.1
...
$ time node -e 'require("trim-off-newlines")("a" + "\r\n".repeat(1e8))'
/Users/trott/temp/node_modules/trim-off-newlines/index.js:6
	return str.replace(regex, '');
	           ^

RangeError: Maximum call stack size exceeded
    at String.replace (<anonymous>)
    at module.exports (/Users/trott/temp/node_modules/trim-off-newlines/index.js:6:13)
    at [eval]:1:29
    at Script.runInThisContext (node:vm:129:12)
    at Object.runInThisContext (node:vm:305:38)
    at node:internal/process/execution:81:19
    at [eval]-wrapper:6:22
    at evalScript (node:internal/process/execution:80:60)
    at node:internal/main/eval_string:27:3
node -e 'require("trim-off-newlines")("a" + "\r\n".repeat(1e8))'  0.16s user 0.13s system 90% cpu 0.316 total
$ npm install trim-off-newlines@1.0.0
...
$ time node -e 'require("trim-off-newlines")("a" + "\r\n".repeat(1e8))'
/Users/trott/temp/node_modules/trim-off-newlines/index.js:6
	return str.replace(regex, '');
	           ^

RangeError: Maximum call stack size exceeded
    at String.replace (<anonymous>)
    at module.exports (/Users/trott/temp/node_modules/trim-off-newlines/index.js:6:13)
    at [eval]:1:29
    at Script.runInThisContext (node:vm:129:12)
    at Object.runInThisContext (node:vm:305:38)
    at node:internal/process/execution:81:19
    at [eval]-wrapper:6:22
    at evalScript (node:internal/process/execution:80:60)
    at node:internal/main/eval_string:27:3
node -e 'require("trim-off-newlines")("a" + "\r\n".repeat(1e8))'  0.16s user 0.12s system 83% cpu 0.343 total
$ 

@Trott
Copy link
Collaborator Author

Trott commented Sep 23, 2021

Added a test case for the worst-case. I think this is ready for review.

Some of the test code is lengthy to remain compatible with Node.js 0.10. I would recommend landing this, then releasing a 2.x that drops support for everything prior to Node.js 12.x. Then, if desired, the test code could be rewritten to be more maintainable, using things liked String.prototype.repeat() instead of constructing strings in while loops. It will also allow us to update the dev dependencies, perhaps eliminating warnings from npm audit and other tools.

@hadasbloom
Copy link

hadasbloom commented Oct 3, 2021

@Trott Trott marked this pull request as draft Oct 3, 2021
@Trott Trott marked this pull request as ready for review Oct 3, 2021
@AndKiel
Copy link

AndKiel commented Oct 7, 2021

Hi @stevemao, any news on this? It's an issue for anyone using yarn audit in their pipelines...

Copy link

@vtothviktor vtothviktor left a comment

I guess if no matches found, you should return with the original string.

index.js Show resolved Hide resolved
@Trott
Copy link
Collaborator Author

Trott commented Oct 7, 2021

I guess if no matches found, you should return with the original string.

No, that's wrong. Please test your suggestions. The regex matches characters that aren't \r and \n. If there's no match, you are dealing with either a string that is made up of nothing but \r and \n or else an empty string. So the correct thing to return is an empty string.

With the suggested change, npm test fails. The current code is correct.

@stevemao stevemao merged commit 6226c95 into stevemao:master Oct 23, 2021
@Trott Trott deleted the fix-it-again branch Oct 23, 2021
@hadasbloom
Copy link

hadasbloom commented Oct 24, 2021

I've updated our Snyk advisory as well and it should be published correctly later today.
@Trott - Thank you so much for the help here and @stevemao for the merge and cooperation :)

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.

None yet

5 participants