Fix potential ReDoS (#37)

[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 is

git checkout v4.1.0 -b backport/v4

Let me know if you have any questions

[Qix-]

Relevant:

• Policy: Addressing CVEs raised by old versions of yargs yargs/yargs#1839 (comment)
• Backport of security patch, for benefit of yargs #38 (comment)

---

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).

[MylesBorins]

@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.

ansi-regex  >2.1.1 <5.0.1
Severity: moderate
 Inefficient Regular Expression Complexity in chalk/ansi-regex - https://github.com/advisories/GHSA-93q8-gq69-wqmw
fix available via `npm audit fix --force`
Will install tap@12.7.0, which is a breaking change
node_modules/jackspeak/node_modules/ansi-regex
node_modules/tap/node_modules/log-update/node_modules/ansi-regex
node_modules/tap/node_modules/string-length/node_modules/ansi-regex
  strip-ansi  4.0.0 - 5.2.0
  Depends on vulnerable versions of ansi-regex
  node_modules/jackspeak/node_modules/strip-ansi
  node_modules/tap/node_modules/log-update/node_modules/strip-ansi
  node_modules/tap/node_modules/string-length/node_modules/strip-ansi
    cliui  4.0.0 - 5.0.0
    Depends on vulnerable versions of strip-ansi
    node_modules/jackspeak/node_modules/cliui
      jackspeak  >=0.1.0
      Depends on vulnerable versions of cliui
      node_modules/jackspeak
        tap  >=13.0.0-rc.0
        Depends on vulnerable versions of ink
        Depends on vulnerable versions of jackspeak
        node_modules/tap
    string-length  2.0.0 - 3.1.0
    Depends on vulnerable versions of strip-ansi
    node_modules/tap/node_modules/string-length
      ink  0.3.1 - 3.0.8
      Depends on vulnerable versions of log-update
      Depends on vulnerable versions of string-length
      node_modules/tap/node_modules/ink
      treport  *
      Depends on vulnerable versions of string-length
      node_modules/tap/node_modules/treport
    string-width  2.1.0 - 4.1.0
    Depends on vulnerable versions of strip-ansi
    node_modules/jackspeak/node_modules/string-width
    node_modules/tap/node_modules/log-update/node_modules/string-width
      wrap-ansi  3.0.0 - 6.1.0
      Depends on vulnerable versions of string-width
      Depends on vulnerable versions of strip-ansi
      node_modules/tap/node_modules/log-update/node_modules/wrap-ansi
        log-update  2.1.0 - 3.4.0
        Depends on vulnerable versions of wrap-ansi
        node_modules/tap/node_modules/log-update

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.

[MylesBorins]

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.

[Qix-]

I recognize that doing these extra versions is additional labor and isn't fair

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.

• These are objectively low severity, hard to exploit vulnerabilities that do not warrant "high" on any scoring system.
• The researchers involved have inaccurately marked versions a number of times, and tend to push the highest possible CVE score when many of the scoring 'levers' are cranked up unfairly.
• I have previously asked these researchers not to report CVEs, which of course may prevent them from cashing out, in order to minimize FUD about these low severity vulnerabilities. They have agreed, and then not complied.
• By backporting, I am telling these researchers (who seem to have targeted the most used repositories so as to maximize their chances of having accepted reports) that their attempts at cash-grabbing at the expense of the OSS community work, that we will put up with it, and that it's lucrative. It puts money directly (and solely) into their pockets at the expense of the thousands of lost hours of work by all of the engineers/sysadmins/QA folks/security officers downstream from us that have to audit this, read all of my long-winded rants, and potentially push engineers away from the task at hand to make some \x1b[1;91mHIGH\x1b[m disappear from their terminal, with minimal-if-any security improvement overall.

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.

[Qix-]

(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.

[MylesBorins]

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

[Qix-]

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.

[Qix-]

I did the cherry-pick locally, should be published as 4.1.1. Let me know if there are any other issues. Thanks for circling back @MylesBorins, apologies for the friction here and thank you for bearing with 🙂

[MylesBorins]

@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

[Qix-]

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.

[MylesBorins]

Hey @Qix- it was this one

#47

Fyi the ghsa is updated to reference your 4.x backport as a fix

After we update 3.x I'll dig in how to get the CVE itself updated

[Qix-]

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 🙂

[Qix-]

@MylesBorins backported to 3.0.1, let me know if there are any issues with that release.

[MylesBorins]

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: ***@***.***>

[Qix-]

Sounds good then :) Thank you again @MylesBorins