[GHSA-78xj-cgh5-2h22] NPM IP package vulnerable to Server-Side Request Forgery (SSRF) attacks

[G-Rath]

Updates

• Affected products
• Description

Comments
Include v2.0.0 as vulnerable

[G-Rath]

I've verified this by doing the PoC locally using v2:

@dawidurbanski has also independently done the same

[shelbyc]

@G-Rath thank you for the information about the PoC results you found and the results @dawidurbanski found! They are helpful to have.

[advisory-database]

Hi @G-Rath! Thank you so much for contributing to the GitHub Advisory Database. This database is free, open, and accessible to all, and it's people like you who make it great. Thanks for choosing to help others. We hope you send in more contributions in the future!

[indutny]

Hello!

I'm the maintainer of the module, and while I don't disagree that the issue described in this report exists, I believe that the security impact of the bug is rather dubious. While I didn't really intend the module to be used for any security related checks, I'm very curious how an untrusted input could end up being passed into ip.isPrivate or ip.isPublic and then used for verifying where the network connection came from. After all, if you want to prevent access to the service based on the ip address, you would get the ip address from the OS and it will be properly formatted 100% of the times.

Given the above, I'd like to dispute this report and see if we can actually remove the advisory from my module.

Thank you!

[shelbyc]

Hi @indutny, if you wish to dispute the validity of GHSA-78xj-cgh5-2h22, a good step to take is to dispute the underlying CVE, CVE-2023-42282. The CVE Numbering Authority that issued CVE-2023-42282 is MITRE, so you will need to contact MITRE.

MITRE accepts CVE dispute requests at https://cveform.mitre.org/. Under Select a request type, choose Request an update to an existing CVE entry, and under Type of update requested, choose Rejection. Under Link to references corroborating request rationale, include a link to your comment in this thread so that there is a publicly available reference of your intent to dispute the CVE.

[indutny]

@shelbyc does MITRE validate the reports? In my past experience they used to reserve and provide CVE numbers without any verification. While I don't mind submitting a dispute claim to them, this feels like it shouldn't be my responsibility.

[indutny]

I'd also comment that it is strange that I wasn't involved in any step of this process from its beginning, but yet the package users and myself had to experience its effects.

[shelbyc]

@indutny Thank you for your patience while I discussed GHSA-78xj-cgh5-2h22 with my colleagues! We think that GHSA-78xj-cgh5-2h22 still has potential, albeit low, security impact. We believe it makes sense to keep the advisory but to lower the severity to low.

With respect to your other two points:

does MITRE validate the reports? In my past experience they used to reserve and provide CVE numbers without any verification. While I don't mind submitting a dispute claim to them, this feels like it shouldn't be my responsibility.

MITRE does not validate reports by, for example, attempting to replicate proofs of concept. CVEs are just tracking numbers and, by themselves, don't say anything about whether a bug is valid or what kind of security impact a bug has (or is likely to have) on the users of a product that receives a CVE. GitHub has a blog post available talking more about removing the stigma of a CVE: https://github.blog/2022-04-22-removing-the-stigma-of-a-cve/

I'd also comment that it is strange that I wasn't involved in any step of this process from its beginning, but yet the package users and myself had to experience its effects.

Project maintainers have some resources available to minimize the chances of a researcher disclosing a vulnerability without them. Having a security policy available on the repository where a researcher can go to privately report concerns to maintainers provides researchers a place to go to coordinate with maintainers, and GitHub has documentation available for learning about security policies:

• https://github.com/ossf/oss-vulnerability-guide/blob/main/maintainer-guide.md#establish-projects-vulnerability-handling-expectations-aka-security-policy
• https://docs.github.com/en/code-security/getting-started/adding-a-security-policy-to-your-repository

In addition to having a security policy, some maintainers enable Private Vulnerability Reporting at the repository or organization level so that researchers can privately report their concerns directly to the maintainer on GitHub:

• https://docs.github.com/en/code-security/security-advisories/working-with-repository-security-advisories/configuring-private-vulnerability-reporting-for-an-organization
• https://docs.github.com/en/code-security/security-advisories/working-with-repository-security-advisories/configuring-private-vulnerability-reporting-for-a-repository

[indutny]

@shelbyc thank you! I appreciate you discussing it and making such determination. It looks like the advisory page wasn't updated yet. When do you expect this change to go through?

I didn't know about Private Vulnerability Reporting. It is now enabled for all my repositories, thank you!

[shelbyc]

@indutny thanks for the heads up! You should be able to see the severity set to low on GHSA-78xj-cgh5-2h22 now. I'm glad you enabled Private Vulnerability Reporting and I hope you find it helpful. 🙂 Have a good weekend!

[philipwhiuk]

What we are seeing here is a bug in the library and a CVE in an app that chooses to use the library in this way (if there is such a thing - one suspects not). There is no security vulnerability in the library itself. It's a mistake to assign the CVE to the node-ip project.

The reference to SSRFs is entirely speculative and does not apply to the library. It could have said 'remote code execution' or 'SQL injection' or literally anything. The library merely returns true when it should return false.

The researcher has not demonstrated a CVE in an application.

[philipwhiuk]

@shelbyc how did you come to the conclusion that the library (and not an app using the library) is vulnerable to SSRF?

At worst this seems more like CWE-20 but really it's just a bug.

[webknjaz]

MITRE accepts CVE dispute requests at cveform.mitre.org.

FTR, when a similar spam CVE was posted against aiohttp, me and a number of other people attempted using that form to request that it'd be discarded as nonsense. However, none of said people (me included) ever heard back from MITRE: aio-libs/aiohttp#6772 (comment). We ended up asking several major databases (PyPA Advisory DB, Snyk, GitHub etc.) to withdraw, but so far nothing indicates that communicating with MITRE is even possible.

[Daniel15]

It seems like the person that found the issue did try to reach out to you privately first? indutny/node-ip#126

[chris1911-jenkins]

Also, detecting a private IP address as public will result in false positives, only.
Should (and that is a bad idea) someone use that function for security reasons, a private IP address would be classified as public. If that one trusts public IP addresses more than private ones, only then would this be an issue.

I still cannot follow the reasoning behind this CVE.
Both being a library and yielding false positives are reasons for filing a bug - nothing more.

Can we somehow protect open source developers from people abusing CVEs like in this case?

[kevinbackhouse]

I'm a colleague of @shelbyc at GitHub Security Lab and was involved in the discussion that we had about this CVE. There are a few points that I think are worth clarifying:

1. Many of the vulnerabilities in the GitHub Advisory Database are imported from other sources such as NVD. In this particular case, the original vulnerability report was https://huntr.com/bounties/bfc3b23f-ddc0-4ee7-afab-223b07115ed3 (which is also where the SSRF claim originated) and the CVE was issued by MITRE.

2. The way that the CVE system works, to get a CVE rejected you need to dispute it with the issuing CNA, in this case MITRE. So we can change how we describe the CVE in the GitHub Advisory Database, but GitHub cannot unilaterally reject the CVE.

3. It's often very difficult to decide whether a bug in a library is a vulnerability or not, because it's impossible to know how the library might be used. In this case the vulnerability reporter has described a scenario in which the bug might cause an SSRF. From the perspective of anybody who isn't using the library in that way, this bug is not a vulnerability. We changed the severity to "low" to reflect that.

[HoLyVieR]

I'd just like to point out that the statement of @philipwhiuk is fairly misguided. Considering the amount of thumbs up the comment as gathered, it seems worthwhile to point out the fallacy.

What we are seeing here is a bug in the library and a CVE in an app that chooses to use the library in this way (if there is such a thing - one suspects not). There is no security vulnerability in the library itself. It's a mistake to assign the CVE to the node-ip project.
The reference to SSRFs is entirely speculative and does not apply to the library. It could have said 'remote code execution' or 'SQL injection' or literally anything. The library merely returns true when it should return false.

The main problem with the statement is that if we follow this logic we could argue that nothing can ever be a vulnerability in a library as any problematic behavior as to first be exposed by an application in order to be exploitable. This is because library are not standalone application. This is why it's typically fine to work in hypothetical when it comes to library (as long as the hypothetical is reasonable).

The library merely returns true when it should return false.

With this logic we could argue that a library validating whether a string contains safe HTML returning true when it should return false is not a vulnerability. We could argue that a library validating whether someone is authenticated returning true when it should return false is not a vulnerability. etc. If the boolean returned as security implication, it absolutely can be a vulnerability.

The researcher has not demonstrated a CVE in an application.

This is a bad take in practice. Most code is proprietary and therefore not publicly accessible or citable. While it's maybe not the best, the alternative is that we wouldn't file CVE for vulnerability that don't have public code affected. This would in turn mean proprietary code could also be affected and would not be notified.

---

@indutny

I'd also comment that it is strange that I wasn't involved in any step of this process from its beginning, but yet the package users and myself had to experience its effects.

I'd like to comment a bit of the "its effects". The main issue right now is not in the CVE filling and the database inclusion. It's highly desirable to have an open database of vulnerability even with the counterpart of the variable quality of its content. The main issue is that a lot of company treat the CVE database as an absolute source of truth and don't consider usage when determining whether they should patch or not. In my opinion we shouldn't bend backward the CVE reporting and it's database because company misuses and misinterprets it's content. In this case, the content is accurate. If you use this library for validating whether an address is internal or not (and then use the address to query data), you could end up being vulnerable to SSRF. It's the role of the company to interpret this data and to dismiss it if they don't use the library for security purposes.

[eslerm]

@shelbyc has excellent advice on how to dispute a CVE MITRE assigns.

@webknjaz if you do not hear back from MITRE, the proper channel is to raise a dispute against MITRE with the CVE Board. The CVE Board notes are made public. Disputing bogus CVEs publicly, like on oss-security, also seems to help. I can help followup if you like.

On a side note, sqlite3 does not provide an authentication layer (of course). This has lead to many bogus CVE attempts. Because of this, sqlite3 has proactively written about their security scope and what does and does not count as a vulnerability under that scope: https://www.sqlite.org/cves.html. This is really helpful to have written before a bogus CVE assignment. It's more difficult for MITRE to defend a bogus assignment.

FOSS projects shouldn't be burdened with all this extra labor. Bogus CVEs need to be called out more often and the dispute process needs to become transparent for this to improve.

[ouuan]

While I didn't really intend the module to be used for any security related checks, I'm very curious how an untrusted input could end up being passed into ip.isPrivate or ip.isPublic and then used for verifying where the network connection came from. After all, if you want to prevent access to the service based on the ip address, you would get the ip address from the OS and it will be properly formatted 100% of the times.

Should (and that is a bad idea) someone use that function for security reasons, a private IP address would be classified as public. If that one trusts public IP addresses more than private ones, only then would this be an issue.

It is not to block incoming network requests but to block outgoing network requests. That is what "server-side request" in the name "SSRF" means. The problem is that the server can be used to penetrate the internal network.

I'd also comment that it is strange that I wasn't involved in any step of this process from its beginning

See indutny/node-ip#126 and indutny/node-ip#143. @indutny just did not respond to any issues and pull requests. I also tried to reach @indutny on Mastodon via PM on Feb 23, because he is active on it, and received no response.

BTW, I am the one who is not involved in this disputation.

[ouuan]

I agree that the severity should be low if there needs one. Also for GHSA-2p57-rm9w-gvfp. But I'd like to quote Go Security team's policy here:

The Go Security team does not assign traditional fine-grained severity labels (e.g CRITICAL, HIGH, MEDIUM, LOW) to security issues because severity depends highly on how a user is using the affected API or functionality.

It is not low for all, but none for most and moderate to high for some. The reason that I still requested a CVE is that the library has 20 million downloads per week, so it could be used improperly somewhere even if this is rare. Analysts should read the advisory instead of taking every single CVE in any dependencies as actual risk.

BTW, I highly recommend marking the package as deprecated on npm if it is not going to be actively maintained. I also suggested using other libraries even if the user is not affected by this issue, because this library is not maintained, and there are many known bugs even if not considering security vulnerabilities.

[Daniel15]

It is not to block incoming network requests but to block outgoing network requests.

+1

For example, on https://dnstools.ws/ I block pings to private IPs. That's using C# though, and the .NET Framework's IPAddress class stores IPs in a normalized format, so checking for a private IPv4 address is straightforward:

public static bool IsPrivate(this IPAddress address)
{
	var bytes = address.GetAddressBytes();
	return bytes[0] switch
	{
		10 => true,
		127 => true,
		172 => (bytes[1] >= 16 && bytes[1] < 32),
		192 => (bytes[1] == 168),
		_ => false
	};
}

I think the core issue with this library (as also mentioned in indutny/node-ip#143) is that it doesn't enforce that IPs used with the library have been normalized. A better API design would be to first have to normalize the IP, then all the other methods only accept normalized IPs.

[Uzlopak]

isIPv4 in nodejs is not allowing that format of IP. You should call isIP before you process it in node-ip