fix: remediate ReDOS further

[Trott]

No description provided.

[Trott]

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

[Trott]

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

[Trott]

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

[Trott]

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]

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]

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]

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]

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]

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]

Hey, Any updates on this? Steve - did you manage to take a look at the PR? Thanks! Hadas

…

On Fri, Oct 1, 2021 at 5:53 AM Rich Trott ***@***.***> wrote: If this is too large a change set to comfortably merge, I wonder if it would be sufficient to change the regular expression to /^(?:\r|\n)+|[^\r\n](?:\r|\n)+$/g. @hadasbloom <https://github.com/hadasbloom> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ARN4JER7BUBIEXAXIF7FKJTUEUPELANCNFSM5ESTJZCA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

-- Hadas Bloom Security Analyst | Snyk <https://snyk.io/> ***@***.***

[AndKiel]

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

[vtothviktor]

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

[Trott]

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.

[hadasbloom]

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 :)