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.

Sign up for GitHub

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

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

Closed
wants to merge 2 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
444fe1f
Updated replace method for Uncontrolled Resource Consumption CVE-2022…
jeran-urban Nov 14, 2022
872acfc
Merge branch 'master' into CVE-2022-37599-patch-fix
jeran-urban Nov 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/interpolateName.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ function interpolateName(loaderContext, name, options = {}) {
.replace(/\.\.(\/)?/g, "_$1");
directory = directory.substr(0, directory.length - 1);
} else {
directory = resourcePath.replace(/\\/g, "/").replace(/\.\.(\/)?/g, "_$1");
directory = resourcePath.replaceAll(/\\/g, '/').replaceAll(/\.\.(\/)?/g, '_$1');
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

@jeran-urban jeran-urban Nov 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

.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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

@jeran-urban jeran-urban Nov 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

}

if (directory.length === 1) {
Expand Down