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.

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

Policy: Addressing CVEs raised by old versions of yargs #1839

Open
bcoe opened this issue Dec 17, 2020 · 15 comments
Open

Policy: Addressing CVEs raised by old versions of yargs #1839

bcoe opened this issue Dec 17, 2020 · 15 comments
Labels

Comments

@bcoe
Copy link
Member

bcoe commented Dec 17, 2020

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 bcoe pinned this issue Dec 17, 2020
@bcoe bcoe changed the title I will be closing all PRs and Issues related to CVEs that I've already patched I will be closing Issues related to CVEs that we've already patched Dec 17, 2020
@bcoe bcoe changed the title I will be closing Issues related to CVEs that we've already patched Policy: all issues for CVEs patched in latest release will be closed Dec 17, 2020
@bcoe bcoe changed the title Policy: all issues for CVEs patched in latest release will be closed Policy: all issues for CVEs patched in latest will be closed Dec 17, 2020
@bcoe bcoe changed the title Policy: all issues for CVEs patched in latest will be closed Policy: all issues for already patched CVEs will be closed Dec 17, 2020
@bcoe bcoe added the security label Dec 17, 2020
@bcoe bcoe changed the title Policy: all issues for already patched CVEs will be closed Policy: all issues/PRs for patched CVEs will be closed Dec 17, 2020
@bcoe bcoe changed the title Policy: all issues/PRs for patched CVEs will be closed Policy: Addressing CVEs raised by old versions of yargs Dec 17, 2020
@bcoe
Copy link
Member Author

bcoe commented Dec 17, 2020

@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
Copy link

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
Copy link
Member Author

bcoe commented May 3, 2021

@DullReferenceException this prototype pollution issue was already addressed in [email protected].

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
Copy link

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

@lirantal
Copy link

lirantal commented May 6, 2021

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

@bote795
Copy link

bote795 commented Sep 13, 2021

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
Copy link
Member Author

bcoe commented Sep 13, 2021

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

Refs: #2029

@bcoe
Copy link
Member Author

bcoe commented Sep 23, 2021

@bote795 patched in ansi-regex now 🎉

@woehr
Copy link

woehr commented Sep 23, 2021

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
Copy link
Member Author

bcoe commented Sep 23, 2021

@woehr [email protected] is pinned on [email protected], 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
Copy link

woehr commented Sep 23, 2021

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
Copy link
Member Author

bcoe commented Sep 23, 2021

@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
Copy link

joeylgutierrez commented Sep 29, 2021

I'm getting a security issue in Snyk for the latest version of [email protected] 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 [email protected] introduced by [email protected] > [email protected] > [email protected] > [email protected] and 9 other path(s) This issue was fixed in versions: 6.0.1, 5.0.1

@bcoe
Copy link
Member Author

bcoe commented Oct 4, 2021

@joeylgutierrez [email protected] pulls in [email protected] 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-
Copy link

Qix- commented Oct 13, 2021

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants