[Security] Fix ReDoS

[ready-research]

Fix ReDoS

Reported in https://www.huntr.dev/bounties/423e2208-6064-4150-b6f5-22f15f540259/, you can access this using GitHub.
Please validate using Mark as valid and also confirm the fix. Thank you.

Update:

This does not fix ReDoS, more over issue was not confirmed at least against Node.js 20. Though it does improve performance.
Correct description for this change is

Improve regex to handle nested comments more efficiently, ensure that the regex does not backtrack excessively

[jsdevel]

please add a test

[lagden]

done here: #1169

[ready-research]

Please validate the above huntr link using Mark as valid and also confirm the fix. Thank you.

[aartichella]

Hi @jsdevel , could you give an update on when this might be merged. This has an impact on our project.

[jsdevel]

@ready-research @aartichella i need a test added with this change and the build needs to pass.

[smokhov]

@jsdevel - A test was already merged from #1177 / #1169. Just need to merge the fix itself and respond to @ready-research to "mark as valid" in their form.

[jsdevel]

@ready-research please rebase/get the build to pass.

[w666]

Hi All,

Provided links on PRs do not contain any test, so I don't really get it.
As @jsdevel requested it requires a test.

[smokhov]

Hi All,

Provided links on PRs do not contain any test, so I don't really get it. As @jsdevel requested it requires a test.

Odd, I really thought the mentioned PR(s) really provided a test.

@w666 -- what kind of test and in which file would be suitable for this? I could maybe look into creating a test, but I am not at all an expert here. Does handleResponse() itself have a test? Would the test be in client-test.js, server-response-event-test.js, or some place elsewhere?

[w666]

Hi @smokhov,

I looked on this PR again, then on the example code provided on the huntr.dev, then double checked locally the provided fix and then unfixed code. In both cases it works about the same way.

I will investigate this CVE when I have time, but at this point there is not enough evidence.

Please correct me if I am wrong.

Example below: first is not fixed code, then provided fix

[smokhov]

It grows consistently from your samples. The fixed code's lowest 308/395 ms is 23% improvement. The other two are more significant 33-37%. It is possible it were worse before #1177 were merged? The higher you go is where it becomes more problematic (assuming busy environments with lots calls, but tbf I am not sure how prevalent those are to be DoS'ed...).

Overall, however, the fix is an improvement in runtime, if a proper test is provided, IMO it should be merged, but otherwise will leave it up to you to make the final determination.

[w666]

Okay, looked into it, provided description and the example on huntr is not redos, it does not fix the issue, maybe it was the case some time ago, but not now.

The real redos issue looks like this

let regexp = /^(\d+)*$/;

let str = "012345678901234567890123456789z";

console.log(regexp.test(str));

it consumes 100% cpu and runs for a very long time.
I agree that provided solution work a bit faster, but still need to take look on other options.

[w666]

Description for this PR should be like

Improve regex to handle nested comments more efficiently, ensure that the regex does not backtrack excessively

[w666]

I think files in Dummy__should_ignore_commented_envelope_in_response validate that regex still works. So, yeah, fix can be accepted, we already have a test.

Will keep this PR open for a bit, just in case.

[w666]

I had to revert this commit because it there was an error in the regex, somehow missed that.