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

lib: fix parsing for ill-formed addresses #143

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ouuan
Copy link

@ouuan ouuan commented Feb 21, 2024

#138 does not completely fix CVE-2023-42282.

Some examples quoted from the failing tests added here:

  • 127.1, 127.0.1, 127.00.0x1, 127.0.0x0.1
  • 01200034567: Number('01200034567') === 1200034567 while 01200034567 === 167786871 in JavaScript, suprise!
  • 012.1.2.3: ip.isV6Format('012.1.2.3') === true, so it's not normalized :)
  • fe80::0001, we can have leading zeros, and, more: 000:0:0000::01, 000:0:0000:0:000:0:00:001
  • ::fFFf:127.0.0.1: there are /i in many places, but not here

Now I think we need a new CVE. A quick way to get one is to open a security advisory in this repository. @indutny Could you please enable private vulnerability reporting? Or does anyone know whether it is possible to properly revoke the "patched version" in the previous CVE? (Based on the activity level of this repository, the existence of the previous CVE, and the number of uncovered cases, I chose public disclosure here.)

UPD: I'm not sure whether editing the advisory or requesting a new CVE is more appreciated, also wondering how Dependabot would react to this modification, but I opened github/advisory-database#3617.


See #144 for my new attempt to fix it.


IP address handling is hard. I'm not sure how to properly implement it based on the current codes. (Note that my tests are still not exhaustive.) Maybe we should actually parse the addresses instead of relying on regular expressions. I didn't notice the toBuffer function. Maybe we should fix it and use it for other functions.

Also, the semantics of the functions on invalid addresses seem unclear and inconsistent. I suggest a new major release (v3) to change some return values on invalid addresses. Maybe also fix #61 there.

@ouuan ouuan mentioned this pull request Feb 21, 2024
@ouuan
Copy link
Author

ouuan commented Feb 21, 2024

The root problem is the API design. The API should return the normalized parsing result and the user should use that result instead of the original possibly ill-formed one. Otherwise, there are always potential parser differences, e.g. the address 012.1.2.3 is later used somewhere in the user's codes, where it is parsed as 12.1.2.3.

@ouuan
Copy link
Author

ouuan commented Feb 21, 2024

Maybe we should just reject malformed addresses. And we can use net.isIPv4 and net.isIPv6 in Node instead of our own version.

@adduss
Copy link

adduss commented Aug 7, 2024

@ouuan your PRs looks fine, do you to plan to merge them and release?

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

Successfully merging this pull request may close these issues.

2 participants