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

net.isIP fails to detect different formats #40966

Closed
abaetu opened this issue Nov 25, 2021 · 9 comments · Fixed by #41028
Closed

net.isIP fails to detect different formats #40966

abaetu opened this issue Nov 25, 2021 · 9 comments · Fixed by #41028
Labels
net Issues and PRs related to the net subsystem.

Comments

@abaetu
Copy link

abaetu commented Nov 25, 2021

Version

v16.13.0

Platform

Darwin xxx.local 20.6.0 Darwin Kernel Version 20.6.0: Tue Oct 12 18:33:38 PDT 2021; root:xnu-7195.141.8~1/RELEASE_ARM64_T8101 arm64

Subsystem

net

What steps will reproduce the bug?

net.isIP("192.0250.1.1")
0
net.isIP("030052000401")
0
net.isIP("0xc0.168.1.1")
0
net.isIP("3232235777")
0
net.isIP("127.42.258")
0
net.isIP("127.66051")
0
net.isIP("10.1.1.0xff")

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

4

What do you see instead?

0

Additional information

I was looking for a way of checking if a hostname in an URL is an IP (v4 or V6) and found the "net" nodejs module. But it is unable to detect different formats. Open a browser and put any of the strings in the description, with a http:// in front, and see that it is correctly converted to the canonical form.
Also would be great if you could add the conversion to canonical form for both ipv4 and ipv6 (https://datatracker.ietf.org/doc/html/rfc5952#section-4)

@VoltrexKeyva VoltrexKeyva added the net Issues and PRs related to the net subsystem. label Nov 25, 2021
@mscdex
Copy link
Contributor

mscdex commented Nov 25, 2021

I've never seen an instance where any of the formats in those examples would have been intentional (unless for perhaps malicious reasons).

@abaetu
Copy link
Author

abaetu commented Nov 25, 2021

The usage may be for malicious reasons. Correctly parsing the ips' could be for anti-malicious reasons.

@kamikredstone
Copy link

Doesn't this return 0, which is false, because these formats are not correct in terms of IP?

@abaetu
Copy link
Author

abaetu commented Nov 25, 2021

They are correct. Theck out the inet_aton that knows how to parse them:

Python 2.7.16 (default, Sep 6 2021, 07:39:44)
[GCC Apple LLVM 12.0.5 (clang-1205.0.19.59.6) [+internal-os, ptrauth-isa=deploy on darwin
Type "help", "copyright", "credits" or "license" for more information.

import socket
import struct
IP32Bit = socket.inet_aton("0xc0.168.1.1")
IP32Bit
'\xc0\xa8\x01\x01'
socket.inet_ntoa(IP32Bit)
'192.168.1.1'

@Trott
Copy link
Member

Trott commented Nov 26, 2021

Ref: https://datatracker.ietf.org/doc/html/draft-main-ipaddr-text-rep-02#section-3.1
Ref: #40174

TL;DR: We (probably) should not permit leading zeroes.

@Trott
Copy link
Member

Trott commented Nov 26, 2021

TL;DR: We (probably) should not permit leading zeroes.

And honestly, I strongly suspect the same rationale applies to the rest of those formats. Supporting them probably does more harm than good.

@richardlau
Copy link
Member

Related: #38860

@abaetu
Copy link
Author

abaetu commented Nov 26, 2021

Indeed, supporting such formats by the Browser, ping from Microsoft, curl and others might do more harm than good, since it's a good way of obfuscating malicious URLs and bypass security solutions.

But on the other side, there are the good guys that are trying to detect such behaviour. For that, we need a way of converting such browser-supported non-standard notation to decimal-point-notation.

As @richardlau remarqued above, i am not the only person requesting this feature. There are libraries doing just that (ipaddr.js for example), but i was surprised that the net nodejs module is not.
Thank you and have a nice weekend!

@Trott
Copy link
Member

Trott commented Nov 26, 2021

i was surprised that the net nodejs module is not.

I do think we should update the documentation at least to call out that we're not verifying all IP address formats, just the common/conventional one.

Trott added a commit to Trott/io.js that referenced this issue Nov 30, 2021
Trott added a commit to Trott/io.js that referenced this issue Dec 2, 2021
Trott added a commit to Trott/io.js that referenced this issue Dec 2, 2021
nodejs-github-bot pushed a commit that referenced this issue Dec 2, 2021
Closes: #40966

PR-URL: #41028
Fixes: #40966
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
danielleadams pushed a commit that referenced this issue Dec 13, 2021
Closes: #40966

PR-URL: #41028
Fixes: #40966
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
danielleadams pushed a commit that referenced this issue Dec 14, 2021
Closes: #40966

PR-URL: #41028
Fixes: #40966
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
danielleadams pushed a commit that referenced this issue Jan 31, 2022
Closes: #40966

PR-URL: #41028
Fixes: #40966
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
danielleadams pushed a commit that referenced this issue Jan 31, 2022
Closes: #40966

PR-URL: #41028
Fixes: #40966
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Jan 31, 2022
Closes: nodejs#40966

PR-URL: nodejs#41028
Fixes: nodejs#40966
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
danielleadams pushed a commit that referenced this issue Feb 1, 2022
Closes: #40966

PR-URL: #41028
Fixes: #40966
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants