Updated replace method for Uncontrolled Resource Consumption CVE-2022-37599

[jeran-urban]

No description provided.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: jeran-urban / name: Jeran Urban (444fe1f)

[alexander-akait]

I don't see any problems with our prev code, look at the issue, links are incorrect (they referer on an old issues, which was fixed), so I think it is mistake in database

[jeran-urban]

so it is my understanding from looking at the tickets, issues, and vulnerabilities, there are 3 vulnerabilities that have been discussed:

• [CVE-2022-37601] - an older critical issue that was addressed in 2.0.3 and backported to 1.4.1 as well (addressed directly by fix: Resolve potential prototype polution exploit #217)
• [CVE-2022-37603] - a more recent high finding that was just addressed in 3.2.1 and backported to 2.0.4 and 1.4.2 (quoted as a problem against the url variable in this file) (addressed directly by Modify Regex to avoid ReDoS #221)

lastly

• [CVE-2022-37599] - a more recent high finding that has not yet been addressed in some systems (quoted as a problem against the resourcePath variable in this file) (no direct fix provided).

I did not see an issue with this code either as there are no wildcard characters that are exploitable and this regex itself is simple and has no known redos issues currently either. But given that as of the day I posted this PR, OSSIndex still showed this as a valid finding, as was Snyk, NVD, etc. and as of yesterday at 11:22am cst, multiple security scans were still showing this as a finding as well.

Given that, there is a difference behind the scenes for replace vs. replace all, that I was hoping this would address, and would address the remaining open finding from the various security agencies that were still reporting this as an active issue.

But as of now, the scans we are running show this package as compliant and the finding appears to be considered addressed by OSS (not the other agencies yet). I already do have tickets open to Snyk, NVD and Mitre for the CVE to update these to show the issues as fixed, and have added this CVE to those tickets, not just the CVE-2022-37603 issue.

Given all of that, as of now, this PR does not seem to be needed and there does not appear to be an issue with the code, as the parts replaceAll targets as issues for replace are not being used here. Closing now.

[jeran-urban]

I will still follow up once those companies have resolved the issues as stated in #216, hopefully the various auditing tools will soon catch up with the updates

[alexander-akait]

.replace(/\\/g, '/') just replace all \ to /, I don't see any security problems here

.replace(/\.\.(\/)?/g, '_$1'), replace ../ on /, that is really simple too

You can try to search on github and you will find a lot of such regexp, I really don't see any security problems with them

Even more

loader-utils/lib/interpolateName.js

Line 80 in d9f4e23

.replace(/\.\.(\/)?/g, '_$1');

will be problem too, but no reports on these lines

So I think it is a mistake, I sent a report to github about it, but no aswer

[jeran-urban]

agreed. If anything I was thinking that the replace.replace is more the culprit of the finding on that line more than anything since the finding was still open previously. But if the previous fix to the url regex is what fixed this, then it was just a badly reported line number that has been fixed. At some point I am sure Node.js will support a time duration limit to the Regex functions as other languages have, and using that will be an "if all else fails" to even the worst regex strings out there, but for now I think you guys fixed this issue. Hopefully Snyk and github advisory adopt the changes quickly. I think that will get rid of the most of the new issues getting opened around the same 3 findings

[jeran-urban]

although is the (\/)? needed in that regex @alexander-akait ? since it is 0 or one time, any .. found as group 0 will be replaced and / found as group 1 will persist in the replace. Given that expression, the second part is never needed and .replace(/\.\./g, '_') would result in the same result, correct?