Policy: Addressing CVEs raised by old versions of yargs

[bcoe]

Rather than opening a new issue for a CVE, please start a conversation here.

We can decide on a case by case basis if it makes sense to attempt a back port, or to help the upstream dependency update yargs.

[bcoe]

@pjsharpe07 I can't bring myself to give you a hard no on back-porting, for the benefit of gulp 😆

Let's try the approach of having this catch all thread, and we can decide on a case by case basis how to approach things -- then I can close CVE PRs and direct people to this conversation.

[DullReferenceException]

It looks like the gulp project is unwilling to go the route of compatibility breakage, and they're sitting on an old version of yargs (^7.1.0) which is using yargs-parser@^5.0.1 which has a Prototype Pollution vulnerability. Think a backport is possible for yargs / yargs-parser?

[bcoe]

@DullReferenceException this prototype pollution issue was already addressed in yargs@5.0.1.

CC: @lirantal for visibility, we'd initially released the fix as 5.0.0-security.0, but this meant that people were pulling in 5.0.0, based on SemVer, which did not have the patch.

[DullReferenceException]

OK, thank you. I guess the issue is just that npm audit doesn't recognize that yargs-parser@5.0.1 fixes the issue. It does look like 5.0.0-security.0 is listed as good, just not 5.0.1.

[lirantal]

On the Snyk side, the database should be up to date now with 5.0.1 including the fix.

[bote795]

new issue detective for ansi-regex that requires an update from string-width@^4 to string-with@^5 via snyk side
https://app.snyk.io/vuln/SNYK-JS-ANSIREGEX-1583908

[bcoe]

new issue detective for ansi-regex that requires an update from string-width@^4 to string-with@^5 via snyk side

Refs: #2029

[bcoe]

@bote795 patched in ansi-regex now 🎉

[woehr]

The LTS nodejs (14.17.6) includes yargs 14.2.3 as a dependency, which still pulls in a vulnerable ansi-regex version. Does yargs 14 need to be updated so nodejs can update it?

[bcoe]

@woehr yargs@14.2.3 is pinned on ansi-regex@4.1.0, so would have issues.

Best approach might be trying to update the dependency in Node.js, what's yargs used for in Node? (I'm guessing it comes in with another dep?).

[woehr]

Best approach might be trying to update the dependency in Node.js, what's yargs used for in Node? (I'm guessing it comes in with another dep?).

It gets pulled in by npm (through libnpx).

[bcoe]

@Qix- you're going to hate me ... but how hard would pulling a patch all the way back to 4.x.x be? for the benefit of Node.js itself.

Kind of cool we've managed to cause an issue in the platform itself right?

[joeylgutierrez]

I'm getting a security issue in Snyk for the latest version of Yargs@17.2.1 for "string-width": "^4.2.0". This might be a known issue already but just wanted to make sure. It looks like "string-width" bumped their dependency of ansi-regex in "5.0.1"
✗ Regular Expression Denial of Service (ReDoS) [High Severity][https://snyk.io/vuln/SNYK-JS-ANSIREGEX-1583908] in ansi-regex@5.0.0 introduced by yargs@17.2.1 > string-width@4.2.0 > strip-ansi@6.0.0 > ansi-regex@5.0.0 and 9 other path(s) This issue was fixed in versions: 6.0.1, 5.0.1

[bcoe]

@joeylgutierrez yargs@17.2.1 pulls in ansi-regex@5.0.1 which is fixed:

└─┬ yargs@17.2.1
  ├─┬ cliui@7.0.4
  │ ├── string-width@4.2.3 deduped
  │ ├─┬ strip-ansi@6.0.1
  │ │ └── ansi-regex@5.0.1
  │ └─┬ wrap-ansi@7.0.0
  │   ├─┬ ansi-styles@4.3.0
  │   │ └─┬ color-convert@2.0.1
  │   │   └── color-name@1.1.4
  │   ├── string-width@4.2.3 deduped
  │   └── strip-ansi@6.0.1 deduped
  ├── escalade@3.1.1
  ├── get-caller-file@2.0.5
  ├── require-directory@2.1.1
  ├─┬ string-width@4.2.3
  │ ├── emoji-regex@8.0.0
  │ ├── is-fullwidth-code-point@3.0.0
  │ └── strip-ansi@6.0.1 deduped
  ├── y18n@5.0.8
  └── yargs-parser@20.2.9

If you have a local package-lock.json or yarn lock file, make sure you've updated the ansi-regex version to 5.0.1.

[Qix-]

So I haven't forgotten about the request to backport - I've been giving it some thought.

I can of course backport, but I'd rather not, for two reasons. Sorry for the long winded answer, I don't expect everyone to read it. I know that some of us come off a bit crude when responding to these sorts of issues so I want to explain, fully, my standpoint when it comes to trivial fixes, backports, the CVE process and these vulnerabilities.

Firstly, the reported ReDos vulnerabilities affect only specific cases when very large, unsanitized inputs are passed to the regex. Of course in some cases the processing time is exponential, but it doesn't affect all inputs - in the cases where the affected branch is activated, short inputs will see no significant slowdowns.

There is currently a sort-of battle being fought that a few maintainers have spoken with me about, but so far hasn't really been talked about publicly. A lot of our packages are getting hit with "high" CVE vulnerability reports because a certain security firm that has jumped onto the scene within the last year or two keeps reporting them one-by-one, seemingly to milk bounty programs of money.

Of course the intent here is speculative and reflects my own suspicions first and foremost, but I can say definitively that I'm not alone in going down this line of thinking.

The reason why they score "high" is two-fold: firstly, CVE scoring is widely considered outdated and poorly designed. It does not take into account that vulnerable code is usually behind layers of abstractions, or that certain "vulnerabilities" are not really vulnerabilities and instead are misuses from the downstream application code.

As an example (raised years ago by a different researcher), Chalk had a vulnerability report that unsanitized inputs to Chalk could include "harmful" escape sequences that controlled hardware. Well, yeah. Of course. We're not going to mangle your input - Chalk is purely additive. The same "vulnerability" would be there if you simply wrote to stdout/err. There was a bit of a scuffle over email about it and ultimately we had to simply ignore the researcher entirely after some rude remarks were made toward the project.

This sort of behavior is pretty uncommon in my personal experience, but the wave of ReDos vulnerabilities seems to be a new variant of this behavior. The researchers in question have been polite, but relentless, and instead of addressing all of them at once and assigning a reasonable CVE score, the reports are instead made on websites where thousands of collective dollars are being rewarded for very small amounts of work. Every ReDos reported so far could have been detected by a trivial automated test. None of this money goes to actually benefit the project or guard against regressions, or to even support the developers who maintain those libraries.

It feels really slimy, and it awards trivial amounts of work with lot of money whilst simultaneously damaging the good-faith open source projects. Not to mention, taking a LOT of time responding to the resulting issues (most of which are duplicates), requests (like this one) for backports, or other strange requests people seem to think they need fulfilled. Further, we've seen users migrate away from our libraries after these are filed as they perceive the code to be insecure and full of bugs - which simply isn't the case.

Further, a few times now I've personally requested that a CVE is not filed for such vulnerabilities since they are very edge case and can be fixed trivially. The amount of impact or harm is basically zero, and I've never personally heard of any of these libraries causing outages of any kind, much less from ReDos attacks.

Since the CVE score generally states that the ReDos is "remotely exploitable" (i.e. over a network) since the library could be used in a network application, that automatically sets the score to a very high medium. Then the other few points usually alloted to the score push it up to the "high" every single time.

In theory, that makes every single piece of code in the Node.js ecosystem (if not just about everywhere else) "remotely exploitable" and thus every vulnerability could be seen as "high". CVE scores are broken, in my opinion, and the researchers in question appear to be using them to make money. Again, this is my interpretation of the situation. I have nothing but respect for responsible disclosure processes and security research, but this feels like a blatant money-making grab.

Should we be implementing automated tests? Absolutely. Are ReDos vulnerabilities actually vulnerabilities? Of course. Should we patch them and push updates? Yes*.

The asterisk there brings me to my next point.

Small, edge-case vulnerabilities are everywhere. Since these reports keep coming up, we're going to keep getting requests to backport. A few times now, we've also been asked to backport full features to old major versions of the codebases, because we backported hotfixes shortly prior.

This is not a precedent I want to set. It very much increases the surface area to break legacy software, it increases the OSS maintainence overhead, it produces a lot of noise, and it creates a lot of previously-nonexistent surface area for human error to wreak havoc. This has happened a few times in the past where a bad publish broke a bunch of people (millions of people) and we had to put out fires and then respond to the tsunami of issues caused by people facing aggressive caches and whatnot.

At this point, I'd rather just encourage people to upgrade. ansi-regex hasn't changed in API much except for the required node version. The latest version of ansi-regex supports Node 12 and uses ESM, and ansi-regex@5 supports Node 8. I would highly push for them to update their dependencies as both of those majors have the mitigation (namely after @bcoe very politely requested to backport to v5, which was reasonable 🙂) instead of us backporting to an ancient major.

Sorry for the novel!