[dev-server] CVE-2024-29415: "ip SSRF improper categorization in isPublic"

[jdcoldsmith]

This issue is just meant to raise the flag about the current vulnerability with the ip package that @web/dev-server depends on. It seems that there currently is no fix for this from ip.
GHSA-2p57-rm9w-gvfp

[jdcoldsmith]

I see from this comment that people seem to be switching to using the ip-address package instead.

[dmihalcik]

It looks like the feature used - guessing the current public ipv4 address of the server - is not available in ip-address.

Some options I see:

• Implementing a small function that replicates what small subset of the ip.address method that is used
• adding the public ip address or hostname of the server to the config and using that as appropriate
• some mixture of these two

Source code of ip.address: https://github.com/indutny/node-ip/blob/main/lib/ip.js#L383

I propose exporting a new function in core lib, which loops through addresses by nic, picking the first non loopack ipv4 address found.

An enhancement would be allowing this value to be passed in to more places as a configuration object, to better handle cases where there are mutliple addresses that meet this criteria (if that is desired).

[jdcoldsmith]

Great points @dmihalcik! Another possibility could be the public-ip package. It seems to be very straightforward and accomplish the small need of this package.

[bashmish]

In our codebase we use only one method from ip

given it's always a call to ip.address() without arguments, it's important to check what the defaults are

I think the defaults will be name="private" and family="ipv4", given the last implementation

// currently used functon
// node_modules/ip/lib/ip.js

//
// ### function address (name, family)
// #### @name {string|'public'|'private'} **Optional** Name or security
//      of the network interface.
// #### @family {ipv4|ipv6} **Optional** IP family of the address (defaults
//      to ipv4).
//
// Returns the address for the network interface on the current system with
// the specified `name`:
//   * String: First `family` address of the interface.
//             If not found see `undefined`.
//   * 'public': the first public ip address of family.
//   * 'private': the first private ip address of family.
//   * undefined: First address with `ipv4` or loopback address `127.0.0.1`.
//
ip.address = function (name, family) {
  var interfaces = os.networkInterfaces();

  //
  // Default to `ipv4`
  //
  family = _normalizeFamily(family);

  //
  // If a specific network interface has been named,
  // return the address.
  //
  if (name && name !== 'private' && name !== 'public') {
    var res = interfaces[name].filter((details) => {
      var itemFamily = _normalizeFamily(details.family);
      return itemFamily === family;
    });
    if (res.length === 0) {
      return undefined;
    }
    return res[0].address;
  }

  var all = Object.keys(interfaces).map((nic) => {
    //
    // Note: name will only be `public` or `private`
    // when this is called.
    //
    var addresses = interfaces[nic].filter((details) => {
      details.family = _normalizeFamily(details.family);
      if (details.family !== family || ip.isLoopback(details.address)) {
        return false;
      } if (!name) {
        return true;
      }

      return name === 'public' ? ip.isPrivate(details.address)
        : ip.isPublic(details.address);
    });

    return addresses.length ? addresses[0].address : undefined;
  }).filter(Boolean);

  return !all.length ? ip.loopback(family) : all[0];
};

[bashmish]

so looks like we need internal-ip from the same author as public-ip

specifically internalIpV4()

import {internalIpV4} from 'internal-ip';

console.log(await internalIpV4());
//=> '10.0.0.79'

but it's just my guess based on quick analyses of the libraries

UPDATE

this is confusing though

return name === 'public' ? ip.isPrivate(details.address)
        : ip.isPublic(details.address);

[bashmish]

did a test locally

// index.mjs
import ip from "ip";
import { internalIpV4 } from "internal-ip";
import { publicIpv4 } from "public-ip";

console.log("ip.address()", ip.address());
console.log("await internalIpV4()", await internalIpV4());
console.log("await publicIpv4()", await publicIpv4());

> node index.mjs

ip.address() 192.168.1.104
await internalIpV4() 192.168.1.0
await publicIpv4() xxx.xxx.xxx.xxx (it's certainly not what we need)

[jdcoldsmith]

Wait maybe I am missing something but ip.address() and internalIpV4() produced different results. Is that ok?

[bashmish]

Wait maybe I am missing something but ip.address() and internalIpV4() produced different results. Is that ok?

no, it's not, updated the comment

[bashmish]

given the difference, I think we actually need this https://github.com/silverwind/default-gateway

gonna test now

UPDATE

also not

// index.mjs
import ip from "ip";
import { gateway4sync } from "default-gateway";
import { internalIpV4 } from "internal-ip";
import { publicIpv4 } from "public-ip";

console.log("ip.address()", ip.address());
console.log("gateway4sync().gateway", gateway4sync().gateway);
console.log("await internalIpV4()", await internalIpV4());
console.log("await publicIpv4()", await publicIpv4());

> node index.mjs

ip.address() 192.168.1.104
gateway4sync().gateway 192.168.1.1
await internalIpV4() 192.168.1.0
await publicIpv4() xxx.xxx.xxx.xxx (it's certainly not what we need)

[bashmish]

did a bit research on what's going on internally
internal-ip should be the one we need, but it's either buggy, or I'm missing smth, but 0 at the end is not correct, it should just give 192.168.1.104

I can't find any working alternaitve though, feel free to suggest one

otherwise I can look into making our own utility for that, which is basically a simplified version of the old ip package implementation

PS: e.g. storybook just implemented a custom utility in their codebase https://github.com/storybookjs/storybook/pull/27529/files

[smurugavels]

Any ETA on the fix?
Thank you for all the good work here.

[bashmish]

Found a working one, looks good https://www.npmjs.com/package/local-ip-url
Will make a PR shortly

// index.mjs
import ip from "ip";
import localIpUrl from "local-ip-url";
import { gateway4sync } from "default-gateway";
import { internalIpV4 } from "internal-ip";

console.log("ip.address()", ip.address());
console.log("localIpUrl()", localIpUrl());
console.log("gateway4sync().gateway", gateway4sync().gateway);
console.log("await internalIpV4()", await internalIpV4());

> node index.mjs

ip.address() 192.168.1.104
localIpUrl() 192.168.1.104
gateway4sync().gateway 192.168.1.1
await internalIpV4() 192.168.1.0

[bashmish]

Unfortunately local-ip-url code seems to be a copy of ip with same security issue, so it's not really fixing the problem, but rather hiding it and opening a possibility soon to run into the same security bug.

I figured internal-ip worked well until v8 sindresorhus/internal-ip#48
So if we use internal-ip@7.x.x it's fine.
It does't have any checks for public IPs which I think is the root cause of the security issue, because such check if done wrongly can lead to opening certain attacks.
So we are fine to use internal-ip without worrying about this security issue reappearing.

[KearseTrevor]

I'd like to thank you for the effort you've put in on this - I know you hit a bit of a pivot point but is there any ETA on the internal-ip@7.x.x approach?

[bashmish]

It's released. For exact versions of new packages you can check #2744

[jdcoldsmith]

Thank you very much @bashmish for your efforts on this!