-
-
Notifications
You must be signed in to change notification settings - Fork 78
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.
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
Fix potential ReDoS (#37) #46
Conversation
Relevant:
I appreciate the work but that wasn't really the issue I had with backporting - cherry picking the fix would be trivial. My own personal opinions are outlined in full in the above two links. At this point I'm going to defer to @sindresorhus (I hate doing this but I don't want to be the only one having a say here). |
@Qix- I totally understand where you are coming from and respect it. For transparency I am the product manager of the npm CLI and am actively working on ways we can improve audit and minimize noise due to CVEs. This particular CVE is super noisy and it hitting a bunch of my repos in ways that cannot be resolved due to the transitive nature of the dependencies. I get all of this in my tree simply due to tap.
This cannot be resolved by any single package update and would require coordination across multiple projects. I recognize that doing these extra versions is additional labor and isn't fair, hence making the PRs myself to do the bit I can to help, but in lieu of another solution I cannot really see a path to resolving the warning effecting many developers right now. To be clear I think there is a bunch of work we can be doing at GitHub + npm to improve our platform so this type of labor isn't necessary and we are actively trying to figure out solutions. |
As an aside... as a member of the Node.js core team and as someone who helped implement our LTS program policies... I totally get if you say no from the perspective of support. I'm just trying to pitch in. |
It's not really about the labor. It's about the precedent of letting low-effort vulnerabilities cost us good-faith developers time, energy and happiness (soooo much drama surrounding this vulnerability alone...) for the monetary benefit of a couple of security researchers that have been reporting ReDos vulnerabilities piecemeal in order to rack up extra payout money on at least one bounty website. The labor isn't really a large part of the annoyance.
The automation / labor / maintenance aspect is moot, from where I stand. Would I would love if npm and Github would stop allowing such blatantly (and somewhat ironically) malicious vulnerability reports be filed, and to acknowledge that the CVE scoring system does not reflect modern software engineering practices and should not be used as the basis for "low / medium / high" severity ratings. A new scoring model would benefit literally everyone and de-incentivize low effort vulnerabilities be filed over and over and over again for some quick change. It would also protect the integrity and reputation of other, good-faith researchers who happen to find such vulnerabilities and wish to follow proper protocol for dealing with security issues to begin with. |
(Sorry for yet another long-winded post about this 🙃 I just want to make sure my stance is made clear). To be clear, I would absolutely love it if Github or npm, or both, would team up with the community to improve the security disclosure process overall. It's very antiquated and causes a lot of FUD and drama exactly like this, in my opinion. These days, it seems to be used more and more for quick change, too - which helps nobody. |
hey @Qix- I know your stance here, but we continue to get pings about folks having audit warnings related to this. Backporting this PR, which is ready to go, could significantly simplify workflows for many people. FWIW we are actively working on improving the status quo here over at GitHub / npm. We recently go the CWE information surfaced from the npm advisory API and are thinking about additional improvements we could make in the UX of audit to utilize this data. That said, even if we do fix it in npm 8, that account for less than 20% of registry traffic, so 80% of customers are still potentially affected by this. I understand and empathize with your position here, but just wanted to ask one more time if you would consider landing this and doing a release. If not please feel free to close the PR |
Sure, I haven't seen the folks that were submitting ReDos vulns for a while now. Hopefully the problem has gone away. Give me a bit. |
I did the cherry-pick locally, should be published as |
@Qix- one more thing... I opened a PR with the GitHub Advisory team to update the advisory to include that 4.1.1 is patched so folks won't get warned by npm audit anymore, I'm not sure what needs to be done for the upstream CVE as well, but will look into it |
Thank you @MylesBorins! I saw you had commented somewhere else but I've lost it now, maybe you can ping me again from where it was and I'll take a look. I think it was about a further backport? Just let me know. |
Thanks for all the work. To be clear btw, I have no problem with CVEs being filed for this and for proper security processes to take place - that's a good thing and I love that it happens (even against code I've written - I don't take it personally). It's just in this case, with these specific types of CVEs, it felt like the reporting and bounty process was being taken advantage of for money on Huntr.dev, causing a bunch of extra work as an added cost and setting what I perceived to be a harmful precedent in the OSS community. The intention was never to hide any of the CVEs - this is definitely a real vulnerability, and had it been reported correctly (it's not a high severity vulnerability in the slightest) and reported in-full instead of piecemeal as they had been, my reaction would have been much less dramatic. I guess I'm trying to say, if the CVE is valid, the CVE is valid - don't worry about removing it, unless something is wrong with the CVE report itself. The only exception might be the level itself, which is a reflection of the poor CVE scoring model and less the report itself - I don't think the score can be fixed without changing the scoring process entirely. If the versions are wrong or something, then let me know how I can help to get them updated. Hopefully I'm making sense there - I haven't finished my coffee yet! I'll cut a release for 3.x shortly for you, too. I appreciate the patience and working with me on this 🙂 |
@MylesBorins backported to |
Thanks I'll pick up next steps tomorrow
To clarify I don't intend to have the CVE revoked but rather updated to
ensure that automated tools recognize the patched versions of 3 + 4 as
fixed (and hopefully end the saga of any notifications about this one 😂)
…On Sun, Mar 27, 2022, 9:32 AM Qix ***@***.***> wrote:
@MylesBorins <https://github.com/MylesBorins> backported to 3.0.1, let me
know if there are any issues with that release.
—
Reply to this email directly, view it on GitHub
<#46 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADZYV5F6YU6NOAR7PZ2P2TVCBWRHANCNFSM5HJ5DFWQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Sounds good then :) Thank you again @MylesBorins |
This is a backport of 8d1d7cd on top of v4.1.0 to fix the regex dos as reported in CVE-2021-3807
A number of projects have transitive dependencies on a variety of versions of
ansi-regex
so we should backport as far back as possible. You will likely need to make a new branch from the v4.1.0 to target this PR against. The easiest way to do so isLet me know if you have any questions