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

Security Advisory: NPM ip package still incorrectly identifies some private IP addresses as public #150

Open
ouuan opened this issue May 6, 2024 · 19 comments

Comments

@ouuan
Copy link

ouuan commented May 6, 2024

Description

The problem described in CVE-2023-42282 is not completely fixed in v2.0.1 and v1.1.9. The isPublic() function still identifies some private IP addresses as public. This may lead to SSRF attacks if the isPublic()/isPrivate()/isLoopback() functions are used to guard outgoing network requests.

PoC

See #143

  • 127.1, 127.0.1, 127.00.0x1, 127.0.0x0.1
  • 01200034567
  • 012.1.2.3
  • fe80::0001, 000:0:0000::01, 000:0:0000:0:000:0:00:001
  • ::fFFf:127.0.0.1

Patches

No patch has been applied to the original package yet.

A patch has been proposed at #144.

Suggestions

The ip package is not actively maintained. The author is not responsive.

Users of this package are advised to use alternative packages with similar functions even if their use of the ip package is not affected by this vulnerability, in order to get better support and avoid other potential vulnerabilities and bugs.

Alternatively, users may check their usage of the isPublic()/isPrivate()/isLoopback() functions to see if they are used to guard sensitive network requests (e.g. check whether the IP address provided in user input is private or public before sending out a request to it). Also follow the Server Side Request Forgery Prevention - OWASP Cheat Sheet. It's likely that even a correctly implemented isPublic() function is not enough to prevent SSRF attacks.

@ouuan
Copy link
Author

ouuan commented May 28, 2024

This is CVE-2024-29415 now.

@kth-tw
Copy link

kth-tw commented May 29, 2024

Please fix this issue.

@vtulse
Copy link

vtulse commented May 31, 2024

Please fix this issue.

+1

@ouuan
Copy link
Author

ouuan commented Jun 3, 2024

To be clear, GitHub identifies this as a high-severity issue in GHSA-2p57-rm9w-gvfp, but it depends on the actual use case. I think it's none or low in most cases, moderate in some cases, and rarely of high severity. You may read the suggestions above. However, I think the lack of maintenance of this package is arguably more severe than this CVE itself.

@ffraenz
Copy link

ffraenz commented Jun 3, 2024

Unrelated to SSRF attacks, though behavior I did not expect: ip.isPublic('') evaluates to true because of the way it internally calls !ip.isPrivate('').

@jorenbroekema
Copy link

If someone finds a maintained alternative to this package, please let us know here :)!

@inquinity
Copy link

If someone finds a maintained alternative to this package, please let us know here :)!

npm is now using this package internally (instead of ip.js): https://www.npmjs.com/package/ip-address

@cbovis
Copy link

cbovis commented Jun 5, 2024

For anyone affected by this because of Storybook there's a discussion of relevance at storybookjs/storybook#26014. A PR has been merged to stop using the ip dependency but not yet released.

Edit: has been released in version 8.1.6

@pwilder-sig
Copy link

pwilder-sig commented Jun 5, 2024

Won't help in every case but looks useful in mine: https://www.npmjs.com/package/is-in-subnet

Edit: Full disclosure, no association with this project and I'll note it has a much, much smaller user base. Caveat emptor.

Edit 2: Also some disagreement on what a "private subnet" is between libs so this may not be a 1:1 replacement.

@ffraenz
Copy link

ffraenz commented Jun 6, 2024

@inquinity I was looking into the package you proposed and tried to come up with a replacement for ip.isPublic. However, the package seems to lack knowledge about special IPv4 subnets and private IPv6 addresses.

Here's what I came up with so far. Did I miss something?

Code not tested.

import ip from 'ip-address'

const ip4LoopbackSubnet = new ip.Address4('127.0.0.0/8')
const ip4LinkLocalSubnet = new ip.Address4('169.254.0.0/16')
const ip4PrivateASubnet = new ip.Address4('10.0.0.0/8')
const ip4PrivateBSubnet = new ip.Address4('172.16.0.0/12')
const ip4PrivateCSubnet = new ip.Address4('192.168.0.0/16')

const isPublicIp = (
  address: string
): boolean => {
  try {
    // Throws if address is not a valid IPv4 address
    const instance = new ip.Address4(address)
    // Exclude non-host IP addresses
    if (instance.subnetMask < 32) {
      return false
    }
    // Exclude non-public IPv4 addresses
    if (
      instance.isInSubnet(ip4LoopbackSubnet) ||
      instance.isInSubnet(ip4LinkLocalSubnet) ||
      instance.isInSubnet(ip4PrivateASubnet) ||
      instance.isInSubnet(ip4PrivateBSubnet) ||
      instance.isInSubnet(ip4PrivateCSubnet)
    ) {
      return false
    }
    return true
  } catch (error: unknown) {
    //
  }
  try {
    // Throws if address is not a valid IPv6 address
    const instance = new ip.Address6(address)
    // Exclude non-host IP addresses
    if (instance.subnetMask < 128) {
      return false
    }
    // Exclude non-public IPv6 addresses
    // TODO: Private IPv6 addresses
    if (instance.isLinkLocal() || instance.isLoopback()) {
      return false
    }
    return true
  } catch (error: unknown) {
    //
  }
  return false
}

@danm
Copy link

danm commented Jun 7, 2024

If someone finds a maintained alternative to this package, please let us know here :)!

npm is now using this package internally (instead of ip.js): https://www.npmjs.com/package/ip-address

as is Socks https://github.com/JoshGlazebrook/socks/blob/master/package.json#L47

@DmytroShalaiev
Copy link

Is the CVE issue still related to this repo?

@tomerh2001
Copy link

Any updates?

@DmytroShalaiev
Copy link

Any plans for update?

@lecode-github
Copy link

Any updates?

@mikehall314
Copy link

I trust this isn't bad form, but I have forked this repo and published a fix.

https://www.npmjs.com/package/neoip

The 2.x release should be drop-in compatible with this library.
The 3.x release has been refactored internally to avoid this class of problems, but has a slightly different API.

If it is bad form to mention here, please delete this comment.

@adri-blanco
Copy link

Thank you so much @mikehall314, we are maintaining an old project and we added the fork to the resolutions to fix the issue! ❤️

As a workaround, this is what we added in our package.json, until this is fixed:

  "resolutions": {
    "**/ip": "https://registry.npmjs.org/neoip/-/neoip-2.1.0.tgz"
  }

@SudeshnaBayshann
Copy link

SudeshnaBayshann commented Oct 10, 2024

https://www.npmjs.com/package/ip-address

Is the IP package issue already resolved? If so, can some one please share the corrected version number? I am using npm 10.8.1 and IP version 2.0.1 but still facing the issue. What's the best alternative package (net/ip-address)? Thanks.

@tom-sanderson-bt
Copy link

I trust this isn't bad form, but I have forked this repo and published a fix.

https://www.npmjs.com/package/neoip

The 2.x release should be drop-in compatible with this library. The 3.x release has been refactored internally to avoid this class of problems, but has a slightly different API.

If it is bad form to mention here, please delete this comment.

@mikehall314 any reason there is no issues tab in the fork?

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

No branches or pull requests