Closed coderaiser closed 4 months ago
This questionable security issue relates to exponential complexity when parsing {
or }
characters to sanitize the input.
Given the current code still runs pretty easily with 10k brackets and this is way more than any practical use case, we could simply an additional validation for the max number of brackets around here: https://github.com/micromatch/braces/blob/98414f9f1fabe021736e26836d8306d5de747e0d/lib/parse.js#L38-L40
Any progress on this? Can it be merged?
When this PR will get Merged?
Waiting for this PR to be merged.
PoC
for (let repeats = 1; repeats <= maxRepeats; repeats += 1) {
const payload = '{'.repeat(repeats*90000);
console.log(`Testing with ${repeats} repeats...`);
const startTime = Date.now();
braces(payload);
const endTime = Date.now();
const executionTime = endTime - startTime;
console.log(`Regex executed in ${executionTime / 1000}s.\n`);
}
First, I want to mention that I don't take this lightly and I do think it's very important to resolve vulnerabilities quickly. I've just reviewed the PR, which probably shouldn't have been merged, and I very carefully reviewed the code and the snyk report.
maxSymbols
times in the string. Which means you'd be able to pass 1024 characters 2,162,688 times (that is, if the maxLength
option didn't already exist).Since the PR won't solve the problem, and it introduces some performance degradations (like looping over the entire string again instead of counting in the parser where it's already done; not breaking when the max is reached, etc), I'm not going to publish this.
I would, however, consider adding support for process.env.MAX_BRACES_LENGTH
or something like that, so that you can guarantee that this is respected by any braces
in the tree (but that will only work for the new version and higher). Another thing I would consider is just setting the maxLength
to a much lower default number, like 10,000, which braces handles in 0.008s.
devDependencies
would be exploited by someone who isn't in control of the code. I'd be happy to merge in a PR that does something like what I suggested earlier.
As final thoughts, these types of reports place a huge burden on maintainers like me, and while both snyk and the vulnerability-finders get paid a lot of $ for finding any possible thing that can be labeled a vulnerability (and they are incentivized to lower that bar as far as possible), none of the dollars go to open source maintainers - and it shouldn't in this case, because that would create a bigger problem.
However, the companies that use open source and benefit from it are another story. I love the community and love open source, but I find it absurd to see people from companies like IBM on this thread complaining about my responsiveness to the issue, whilst doing nothing to solve the real problem, which is a lack of funding. It's disheartening to see someone complain about me not spending time on this, and then actively trying to hurt my reputation or ask people to switch away from my projects, instead of caring if I get compensated equitably for my time, or if I'm struggling to pay my bills.
Hi @jonschlinkert , I have "node_modules/micromatch" and "node_modules/braces" in our dotnet project
now we see that vulnerabilities with these node modules with cvss scores below braces:^3.0.1: CVE-2024-4068(7.5) micromatch:^4.0.0: CVE-2024-4067(7.5) can someone please let us know if we have any solution ?
This was resolved in 3.0.3
Fixes #36 https://security.snyk.io/vuln/SNYK-JS-BRACES-6838727