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

Server-Side-Request Forgery vulnerability introduced in npm 10.4 [BUG] <title> #7216

Closed
2 tasks done
CodeLiftSleep opened this issue Feb 12, 2024 · 34 comments · Fixed by #7242
Closed
2 tasks done

Server-Side-Request Forgery vulnerability introduced in npm 10.4 [BUG] <title> #7216

CodeLiftSleep opened this issue Feb 12, 2024 · 34 comments · Fixed by #7242
Labels
Bug thing that needs fixing Needs Triage needs review for next steps Release 10.x

Comments

@CodeLiftSleep
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

This issue exists in the latest npm version

  • I am using the latest npm

Current Behavior

image

Expected Behavior

no SNYK detected security vulnerabilities

Steps To Reproduce

  1. In this environment...
  2. With this config...
  3. Run '...'
  4. See error...

Nothing to reproduce, this is a security vulnerability.

Environment

  • npm: 10.4
  • Node.js: 20.9.0
  • OS Name: Windows 10
  • System Model Name: Dell Evo
  • npm config:
; copy and paste output from `npm config ls` here
@CodeLiftSleep CodeLiftSleep added Bug thing that needs fixing Needs Triage needs review for next steps Release 10.x labels Feb 12, 2024
@ljharb
Copy link
Contributor

ljharb commented Feb 12, 2024

That's a vulnerability in a package npm happens to use, and since the npm cli doesn't have any servers, it can't be vulnerable to SSRF. It's a false positive.

@ctcpip
Copy link

ctcpip commented Feb 13, 2024

adding the CVE here so it can show up in search: CVE-2023-42282 --- GHSA-78xj-cgh5-2h22

it may be a false positive for npm itself, but I expect we won't hear the end of this till the lib is patched or replaced.

edit: socks has replaced it with ip-address, so bumping socks will resolve

@jcnix
Copy link

jcnix commented Feb 13, 2024

You guys could simply not bundle your dependencies and allow consumers to address this. There is a readily available fix via npm audit fix which will update socks to 2.7.3 which replaces the ip package with ip-address

@UlisesGascon
Copy link

You can find more context in indutny/node-ip#136

Here you can find how ip was used in socks (comparing last release with the previous one). As far I see the method isPublic was not used by socks. ip.isPublic() seems to be the only affected method in this CVE.

@ert78gb
Copy link

ert78gb commented Feb 14, 2024

The vulnerability is not affected npm, but npm is part of the node and every official node docker image contains npm. Everyone who uses the official docker image got alert.
In my case the CI does not allow to publish new docker image if it contains new vulnerability.

Currently I have the following options:

  • add ip to whitelist independently it is used by npm or other library. In this case I will lose the alert if other package uses ip now or in the future. I also have to remove ip from the whitelist after the ecosystem updated to the newer ip version. So I have to double work.
  • switch to other base image that does not contains npm.
  • wait for npm to update socks and for new node docker image

I manage the whitelist at docker image level so does not matter which option I will choose it takes time for me.
My setup blocks the dependabot and development PRs too, so I can't wait too much. I added ip to the whitelist of the currently developed services, but I don't really would like to do it with others that just under maintenance.

I don't know how large effort need to update socks and release new node, but my assumption is, it is much less than everyone else find the alternative way to handle the situation.
My other assumption is answer to comment on this issue will need more effort than just update socks and release a new version.

I would like to kindly ask you if you have time please update socks and release a new version and manage the new release of node. I think it is the best option for the community.

@ljharb
Copy link
Contributor

ljharb commented Feb 14, 2024

You wouldn't need a new release of node, you'd just update npm in your docker image.

However yes, you're right, this is a fundamental flaw in the CVE system for any package that has more than one code path, and usually one just does option 1 or 3.

@ert78gb
Copy link

ert78gb commented Feb 14, 2024

You wouldn't need a new release of node, you'd just update npm in your docker image.

Do you mean run npm i -g npm ?

If yes then the prerequisite is a new npm release.
After that I have to modify every Docker file to add the npm i -g npm. It adds ~15 MB extra size to the docker image even if the latest npm version in the docker image. Just for comparison the average npm_modules and application code size is ~50MB of my services.
This is the reason why would be optimal to release a new node version too.

If I have to do this extra work I would go with option 2 but it also has blockers :) Google distroless supports only node LTS version, so I can't use the built in test runner of node.

I just would like to know what is the plan of the npm team. Because based on it I can do better decision.

p.s.
I am wondering why not so many people complaining about it.

@ljharb
Copy link
Contributor

ljharb commented Feb 14, 2024

Yes, that's what i mean. I'm not sure why it would add much size; npm is already contained within node.

@jcnix
Copy link

jcnix commented Feb 14, 2024

This would affect anyone using semantic-release, specifically @semantic-release/npm. I don't know why but they have a dependency on npm even though they basically just do an exec('npm publish') but with the locally installed npm binary.

@ljharb
Copy link
Contributor

ljharb commented Feb 14, 2024

In general, most CVEs in the JS ecosystem are false positives, due to dep graphs having lots of transitive dependencies and CVEs having no fundamental code path branching mechanism. Therefore, it is unreasonable to have any systems that block progress based on the presence of a CVE, without also having an easy "bypass" mechanism to unblock progress.

Nobody using npm is affected by this vulnerability - however they may be inconvenienced by subpar audit processes and/or tooling.

@ardunster
Copy link

I wasted half an hour trying to see how to fix this when npm install itself threw warnings due to semantic-release's dependency, and then npm audit fix refuses to fix the issue. Ironic.

Just want to add my voice to everyone requesting this get fixed on the npm side.

@AloisSeckar
Copy link

I just hope new version with a fix will come soon. Good to know the "vulnerability" isnt actually a vulnerability, but seeing the warnings during npm install and have the Dependabot alert opened is quite inconvenient.

I guess there is no way to temporary change my package.json to override it somehow, since it is a bundled dependency?

@brock-rb2t
Copy link

so, can we please just update socks and move on with it? we have auditors that aren't super understanding of the "false positive" response.

@ljharb
Copy link
Contributor

ljharb commented Feb 16, 2024

Then the auditors need to be educated, or you’re going to keep running into this problem.

@Simbiat
Copy link

Simbiat commented Feb 16, 2024

I'm sorry, have you worked with auditors? Educating them is rarely an option. You explaining that something is a false positive may result in them reducing a risk of that item in their overall assessment, but that's pretty much it. It's not a syntax warning in some IDE, that you can silence with a comment, unfortunately. And that is assuming, that their question would even reach someone who can educate them.
Since the false positive is caused by an actual dependency in NPM, and that dependency was actually updated, it makes sense to update that dependency in NPM.

@ert78gb
Copy link

ert78gb commented Feb 16, 2024

Everybody is happy if everything is green instead of have to explain something is "false positive" because ...

If someone using npm in more than 1 project then they have to add disclaimer to every false positive case. To do it in the proper way you have to investigate every case is it false positive or not.
npm is part of the node. So everybody who uses node in a bad situation even it is false positive.

Few enterprise company run full workstation scan in every hour/day and you have to report why is a vulnerable software on your machine. Education and better tooling cause less noise, but as I wrote in my earlier comment if npm and node releases a new version that will be a great help for the community.

@brock-rb2t
Copy link

it's just hard to imagine a lower effort change to make, that would have a wider impact than this... it's literally the npm package we're talking about here, it's been downloaded 5M times this week. @ljharb, I understand that it's a false positive, but what's with the pushback on issuing a patch?

@ljharb
Copy link
Contributor

ljharb commented Feb 16, 2024

@brock-rb2t its fine if npm issues a patch, and i hope they do. But, this scenario happens many times a year, and every time, the same thing happens - a bunch of auditors who don’t understand software complain, and then a bunch of engineers who just don’t want to deal with it complain, and a bunch of (usually unpaid volunteer) maintainers get burned out from fielding the complaints, which are often unactionable. What I’d prefer to see happen is everyone using the very expertise that got them hired to push back on the auditors so this massive burden on the OSS ecosystem can be reduced.

@brock-rb2t
Copy link

hey man, I can get behind that! I'll start pushing back. Also, I'd be happy to join in the effort supporting the OSS eco

@Soumalya-Github
Copy link

Is there any update on this issue ? Are there any release planned to align with the latest version of socks or ip ?

@pog-charlesinglese
Copy link

pog-charlesinglese commented Feb 21, 2024

Any update on this issue? This is a legitimate issue for our organization. As much as pushing back may make sense, it's not going to change the fact that npm is dependent on a vulnerable dependency that is being tracked via npm audit's global vulnerability database (GHSA-78xj-cgh5-2h22).

+-- make-fetch-happen@13.0.0
  | +-- @npmcli/agent@2.2.0
  | | +-- agent-base@7.1.0
  | | | `-- debug@4.3.4 deduped
  | | +-- http-proxy-agent@7.0.0
  | | | +-- agent-base@7.1.0 deduped
  | | | `-- debug@4.3.4 deduped
  | | +-- https-proxy-agent@7.0.2
  | | | +-- agent-base@7.1.0 deduped
  | | | `-- debug@4.3.4 deduped
  | | +-- lru-cache@10.1.0 deduped
  | | `-- socks-proxy-agent@8.0.2
  | |   +-- agent-base@7.1.0 deduped
  | |   +-- debug@4.3.4 deduped
  | |   `-- socks@2.7.1
  | |     +-- ip@2.0.0
  | |     `-- smart-buffer@4.2.0

@agforero
Copy link

Any update on this issue? This is a legitimate issue for our organization. As much as pushing back may make sense, it's not going to change the fact that npm is dependent on a vulnerable dependency that is being tracked via npm audit's global vulnerability database.

Seconding this. Could use an update ASAP.

@pog-charlesinglese
Copy link

FWIW, dependabot already has an open PR for this and we've spent more time on discourse than it would take to approve and merge the PR (#7238)

@ljharb
Copy link
Contributor

ljharb commented Feb 22, 2024

@pog-charlesinglese sure but nobody who's commented here, myself included, has the ability to do so, so that's not a meaningful comparison :-)

@AloisSeckar
Copy link

@pog-charlesinglese this just proves, that even 2 years dead repo is able to act faster than npm/cli team (unless it was them who ressurected it and made the patch directly in the source unlike others who switched to another library)

@vbrinza
Copy link

vbrinza commented Feb 22, 2024

Interested in a patch too

@wraithgar
Copy link
Member

You guys could simply not bundle your dependencies and allow consumers to address this.

This is possible for all other packages except npm. For, without npm already fully installed how could it install its own dependencies?

@wraithgar
Copy link
Member

#7242 Updates ip.

@Soumalya-Github
Copy link

Isn't there any patch release for the change in #7242 ?

@wraithgar
Copy link
Member

#7184 is the next release.

@Soumalya-Github
Copy link

Thanks! So for v9 also are we having a patch release or its only for v10 ?

@errodrigues
Copy link

Any hopes for v8 too?

@errodrigues
Copy link

Thanks! So for v9 also are we having a patch release or its only for v10 ?

I think v9.9.3 was just released with the fix.

See #7010

@wraithgar
Copy link
Member

Just a reminder that npm version 10 is the only supported npm version. We did a backport on v9 but that is only because we are trying to iron out the backporting process in general. npm 9 and 8 are not supported. We will not be updating npm 8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Needs Triage needs review for next steps Release 10.x
Projects
None yet
Development

Successfully merging a pull request may close this issue.

17 participants