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

Backport of security patch, for benefit of yargs #38

Closed
bcoe opened this issue Sep 14, 2021 · 29 comments
Closed

Backport of security patch, for benefit of yargs #38

bcoe opened this issue Sep 14, 2021 · 29 comments

Comments

@bcoe
Copy link

@bcoe bcoe commented Sep 14, 2021

I know it's a pain in the neck, but would you consider back-porting #37 to the 5.x.x release line, for the benefit of yargs.

Yargs is making the effort during the transition to ESM to support both CJS and ESM, which makes us unable to update to the latest version of string-width.

If you were willing to make an exception (I know you're pushing folks towards using ESM exclusively) it would be really valuable for yargs users using CJK character sets.

@Qix-
Copy link
Member

@Qix- Qix- commented Sep 14, 2021

Sure thing, give me a moment.

Loading

@Qix-
Copy link
Member

@Qix- Qix- commented Sep 14, 2021

Released as 5.0.1 :) Let me know if there's anything else.

Loading

@Qix- Qix- closed this Sep 14, 2021
@bacali95
Copy link

@bacali95 bacali95 commented Sep 15, 2021

Hello @yetingli, can you please edit the vulnerability information in Snyk (https://app.snyk.io/vuln/SNYK-JS-ANSIREGEX-1583908) since the vulnerability fix is backported to version 5.0.1? Thanks 😄

Loading

@yetingli
Copy link
Contributor

@yetingli yetingli commented Sep 15, 2021

Hey @bacali95, I don't seem to have permission to edit them. Hi Colin @colin-ife-snyk, can you please edit the vulnerability information? Thanks a lot!

Loading

@colin-ife-snyk
Copy link

@colin-ife-snyk colin-ife-snyk commented Sep 15, 2021

Hey @bacali95, I don't seem to have permission to edit them. Hi Colin @colin-ife-snyk, can you please edit the vulnerability information? Thanks a lot!

No problem. Will be updated later today.

Loading

@bacali95
Copy link

@bacali95 bacali95 commented Sep 15, 2021

Thanks a lot @yetingli @colin-ife-snyk 🌹

Loading

@sindresorhus
Copy link
Member

@sindresorhus sindresorhus commented Sep 16, 2021

The range in the report says <5.0.1 but it has not been proven that all versions below 5.0.0 are vulnerable. (Snyk: You should generally not just assume all previous versions are affected by default).

Loading

@sindresorhus
Copy link
Member

@sindresorhus sindresorhus commented Sep 21, 2021

Loading

@cardil
Copy link

@cardil cardil commented Sep 21, 2021

GitHub Advisory Database still lists this package as affected for <6.0.1, here: GHSA-93q8-gq69-wqmw

Anyone knows how to update that information as well?

This page https://docs.github.com/en/code-security/security-advisories/creating-a-security-advisory suggest the repo owner should be able to edit this information.

Loading

cardil added a commit to cardil/serverless-operator that referenced this issue Sep 21, 2021
cardil added a commit to cardil/serverless-operator that referenced this issue Sep 21, 2021
openshift-merge-robot pushed a commit to openshift-knative/serverless-operator that referenced this issue Sep 22, 2021
@cardil
Copy link

@cardil cardil commented Sep 22, 2021

I've raised the affected field on Github Securitylab discussion as well: github/securitylab#436

Loading

@bcoe
Copy link
Author

@bcoe bcoe commented Sep 23, 2021

@Qix- thank you very much for the back port 😄, I know this can be a pain in the neck to do.

Loading

@Qix-
Copy link
Member

@Qix- Qix- commented Sep 23, 2021

No problem at all, glad I could help :)

Loading

@sindresorhus
Copy link
Member

@sindresorhus sindresorhus commented Sep 23, 2021

I have confirmed now that this vulnerability exists in v4.1.0, v4.0.0, v3.0.0, but not v2.1.1 and lower.

Loading

@sindresorhus
Copy link
Member

@sindresorhus sindresorhus commented Sep 23, 2021

@yetingli For future reports, please include the affected version range, and a regression test if you decide to submit a fix. Thanks :)

Loading

@sindresorhus
Copy link
Member

@sindresorhus sindresorhus commented Sep 23, 2021

@colin-ife-snyk Can you please update the Snyk report? ⬆️

Loading

@yetingli
Copy link
Contributor

@yetingli yetingli commented Sep 23, 2021

@sindresorhus Thanks for reminding. I will pay attention to this in the future.

Loading

@benjifin
Copy link

@benjifin benjifin commented Sep 23, 2021

@sindresorhus updated as well now in our (Snyk's) backend - will take a few hours to propagate to prod. Thanks!

Loading

@G-Rath
Copy link

@G-Rath G-Rath commented Oct 21, 2021

@sindresorhus @Qix- what would the chances be of getting patches for v4 & v3? I really wish I didn't have to ask, but it seems that currently serverless is pulling in these versions through a few dependencies which I think are not going to get patched since it'd require serverless dropping a few node versions & I think switching to ESM modules (which would be a very big task) or inlining some of the modules, not to mention that at least tabtab seems to have been abandoned so is unlikely to get updated.

g-rath@DESKTOP-MUGH739:/c/Users/G-Rath/workspace/monitors$ npm ls ansi-regex@'>2.1.1 <5.0.1'
monitors@1.0.0 /c/Users/G-Rath/workspace/monitors
└─┬ serverless@2.64.1
  ├─┬ @serverless/cli@1.5.2
  │ └─┬ strip-ansi@5.2.0
  │   └── ansi-regex@4.1.0 * 
  ├─┬ @serverless/utils@5.19.0
  │ └─┬ log-node@8.0.1
  │   └─┬ has-ansi@4.0.1
  │     └── ansi-regex@4.1.0 *
  └─┬ tabtab@3.0.2
    └─┬ inquirer@6.5.2
      ├─┬ string-width@2.1.1
      │ └─┬ strip-ansi@4.0.0
      │   └── ansi-regex@3.0.0 *
      └─┬ strip-ansi@5.2.0
        └── ansi-regex@4.1.0 *

I'm happy to do as much as the work as possible to reduce the load off you if that would make it easier :)

Loading

@cardil
Copy link

@cardil cardil commented Oct 21, 2021

@G-Rath I think you would be good with the same approach I took, although hacky one: openshift-knative/serverless-operator@308e061

Loading

@G-Rath
Copy link

@G-Rath G-Rath commented Oct 21, 2021

@cardil cheers, that's a good workaround for the short-term but I'd like to avoid it if possible since this affects pretty much all of our projects - while I said serverless in my previous comment, really it also affects PHP & Ruby projects too, as they've got laravel-mix & @rails/webpacker so they both end up pulling in versions of big packages like webpack-dev-server & webpack-cli that in turn are pulling in versions of yargs that end up using v3 & v4 of ansi-regex, which we can't do much about because of the nature of these packages as being monolithic framework/"we'll handle the js build side for you" type cli tools.

This is also why I expect it'll take 2-3 years for them to eventually move off the packages that require the v3 & v4 versions of ansi-regex, because they don't tend to release new majors frequently because of that nature and that'll also mean the upgrades will likely require some elbow grease (and assuming all our clients would be happy to sign off on the work).

Getting patch releases for these other versions would mean we can avoid having to perpetually ignore these vulnerabilities (or preinstall patch them) for pretty much every new & existing project for those years (and not have to spend time justifying them to pentesters, govt heads-of-security, etc).

Loading

@ljharb
Copy link

@ljharb ljharb commented Oct 21, 2021

imo even 2-3 years is very optimistic.

Loading

@Qix-
Copy link
Member

@Qix- Qix- commented Oct 21, 2021

Probably not what you want to hear/read, but I've written on backports at-length here: yargs/yargs#1839 (comment)

If @sindresorhus feels differently and that a backport is warranted then of course he can veto my decision on that. My viewpoint will still remain, however.

Loading

@ljharb
Copy link

@ljharb ljharb commented Oct 21, 2021

The sad part is that while I agree with you on the problem, your stance won't actually change the perverse incentives in the ecosystem - it will just cause harm for users.

Loading

@Qix-
Copy link
Member

@Qix- Qix- commented Oct 21, 2021

I disagree. Incentivizing upgrades is exactly why vulnerability reports are displayed by default on all npm install runs. And it has largely worked.

As I explained in my comment, the vulnerability here is, in all reality, quite low severity. I've explained it a few times already but unless you're passing long, unsanitized user inputs to ansi-regex, you're not vulnerable. The CVE system cannot accurately reflect this.

Loading

@ljharb
Copy link

@ljharb ljharb commented Oct 21, 2021

I totally agree; many, many 9's of JS CVEs are false positives due to things like this.

Even though it's the security industry to blame, it's still the users who suffer, either by turning off audit warnings (and missing the rare actual vulnerabilities), or by seeing excessive warning output and perhaps being forced to needlessly upgrade things.

Loading

@Qix-
Copy link
Member

@Qix- Qix- commented Oct 21, 2021

Likewise, backporting this low-risk fix won't solve any of those issues either. In fact, if it forces them to "needlessly" upgrade things, then great. That's kind of the point of iterative development. The CVE report was arguably needless, too. In fact we've historically asked them not to file one and they did it behind our backs anyway.

Imagine if Linux or OpenSSL had to backport to all previous major versions. Nothing would ever progress.

Loading

@ljharb
Copy link

@ljharb ljharb commented Oct 21, 2021

"progress" isn't "dropping a node version" though :-/ aggressively publishing majors that drop node versions is the only reason backports become requested in the first place. I realize that debate isn't likely to go well, or is on topic here, but I still consider that to be the root cause.

Loading

@Qix-
Copy link
Member

@Qix- Qix- commented Oct 21, 2021

Unfortunately the javascript community has been completely fragmented for the better part of a decade with all of the changes TC-39 makes to the language, the whole Babel+Webpack craze forming this precedent that source transformation is the "norm", and the fact that ESM broke just about everyone. Moving forward means leaving some people behind in this scenario - to support everyone means to incur huge development and maintenance overhead in just about every single repository, bloating package sizes with all of the different supporting files (es5 / esm / typescript types / potentially typescript directly).

Conversely, not moving forward means packages rot, or you get rude individuals demanding typescript types, ES modules, etc. There is absolutely no in-between.

The truth is, most maintainers I'm aware of (including myself) are insanely tired of supporting all of the different "flavors" of environments people want to support. It's become kind of the expectation that when some new stack comes out, packages that are broken within that stack/environment/framework need to be "fixed" to support them.

Personally, I'm tired of it. No other code ecosystem on Earth has this problem. No other group of developers would be okay supporting bitrotted old majors as long as javascript maintainers are expected to.

To give you a more concrete answer, @sindresorhus and I generally like to follow the Node LTS release schedule - especially with Chalk, as it's a TTY library and thus focused almost entirely on Node. If you're on a platform that isn't supported by Node anymore, that's your choice. It's not our burden to bear.

Loading

@G-Rath
Copy link

@G-Rath G-Rath commented Nov 1, 2021

@Qix- I do generally agree with your view on this, but unfortunately it's not something that is always with our control (which greatly frustrates me too, and I know will frustrate you to hear) - while I've spent the last two weeks looking into how we can get things upgraded (including updating libraries to be using higher versions of ansi-regex where possible), I think I have reached the limit that I can do so.

One of the chief packages that we cannot upgrade is webpack-dev-server - we're locked into v3 by @rails/webpacker, which means we're stuck waiting for them to release v6 (which uses v4 of wds); this is complicated by the fact that the Rails team is currently planning to move off @rails/webpacker, leaving its future unclear and the likelihood of v6 being released anytime soon unlikely.

I plan to look into seeing if I can implement support for v4 of wds into v5 of @rails/webpacker, but I suspect that even if I could it would be unlikely to get reviewed, let alone merged and released.

This leaves us in a difficult spot having to deal with this, because while I'd love to do things like try and chuck out @rails/webpacker , that would not be feasible for us to do since it would take a huge amount of time (since it'd been to be done for all of our Ruby on Rails applications, of which there are many) and risk (since we'd need to be rolling our own solution, or try and leverage the currently-experimental replacement that could change under us).

This is a situation that frustrates me greatly, because I agree it's not your fault nor your burden to bear, and those who should be bearing it are not; likewise for me, I have to explain to clients and heads-of-securities about how "no it's not actually that big of a risk, it's 'high' because in theory but in reality... etc".

This all leads me back to here, asking if it would be possible to get just a v4 bugfix applying the security fix:

diff --git a/index.js b/index.js
index c254480..9e37ec3 100644
--- a/index.js
+++ b/index.js
@@ -6,7 +6,7 @@ module.exports = options => {
        }, options);

        const pattern = [
-               '[\\u001B\\u009B][[\\]()#;?]*(?:(?:(?:[a-zA-Z\\d]*(?:;[-a-zA-Z\\d\\/#&.:=?%@~_]*)*)?\\u0007)',
+               '[\\u001B\\u009B][[\\]()#;?]*(?:(?:(?:(?:;[-a-zA-Z\\d\\/#&.:=?%@~_]+)*|[a-zA-Z\\d]+(?:;[-a-zA-Z\\d\\/#&.:=?%@~_]*)*)?\\u0007)',
                '(?:(?:\\d{1,4}(?:;\\d{0,4})*)?[\\dA-PR-TZcf-ntqry=><~]))'
        ].join('|');

Based on my efforts to get libraries upgraded, I've found that the ones I couldn't are typically because they still support Node v6 meaning they're limited to using v4 of ansi-regex - I can workaround the higher-level libraries like has-ansi not having a release that supports both Node v6 and using ansi-regex v4 easily enough with inlining, but only if there's a "secure" v4 of ansi-regex.

Loading

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

Successfully merging a pull request may close this issue.

None yet
10 participants