380543 - PR_StringToNetAddr() fails to recognize trailing dot in IPv4 addresses

Status: RESOLVED INVALID

(NSPR :: NSPR, enhancement)

Description

[Wladimir Palant]

This came up when testing for bug 364129. The patch uses PR_StringToNetAddr() to recognize IP addresses but it still fails on addresses like "127.0.0.1." even though those a valid. Necko uses the same function for host name resolution and I found that it in fact takes the long route when resolving IP addresses with trailing dot - it sends a DNS request for "127.0.0.1" (without the trailing dot for some reason), gets a negative response back and somehow still manages to resolve the address correctly then. There is no DNS request for addresses without a trailing dot, these cases are handled correctly by PR_StringToNetAddr().

Tested in Minefield 20070512 build on Windows XP. The behavior might be different on other operating systems.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

There are numerous known issues with PR_StringToNetAddr.  Most of them are
described in bug 344809.  Perhaps this should be a dup of that one.

The crucial requirement for PR_StringToNetAddr is that it be consistent 
across platforms.  That's really the point of bug 344809.  I'm marking that
bug as blocking this one, but I mean to convey that the results must be 
consistent across platforms.

I'm not at all sure that all, or even most, implementations of inet_addr() 
and inet_aton() allow a trailing dot.  As I read it, the BSD code does not.
See http://www.gnu-darwin.org/shims/inet_addr.c

Comment 2

[Wladimir Palant]

I saw this bug but I wasn't sure this was an OS-dependent issue - PR_StringToNetAddr has several possible execution paths and I am not quite sure which is taken when. I agree that BSD code doesn't seem to account for the trailing dot. But the question is - does Necko accept addresses with a trailing dot on BSD even though PR_StringToNetAddr doesn't? That's the real point here, any address valid in a URL should be recognized by PR_StringToNetAddr as well. And if the OS implementation doesn't recognize trailing dots, maybe NSPR should deal with them before passing the string to the OS.

I tested other Windows applications. ping command behaves exactly like Necko, it sends a DNS request, fails and still recognizes the address somehow. Internet Explorer on the other hand will not accept an IP with a trailing dot - that's another possible solution for this problem.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

I'm unaware of any standard or reference implementation that allows trailing
dots on IP addresses.  Trailing dots are accepted for DNS names by the BSD
implementation, even though they are not part of any RFC.  But an all numeric
dotted decimal IP address is not a DNS name.  

Comment 4

[Wladimir Palant]

Then the bug is about PR_GetAddrInfoByName delegating the call to getaddrinfo on Windows without checking. I tried this and passing "127.0.0.1." to getaddrinfo will result in the behavior described above - a DNS request is sent, fails, and then getaddrinfo reports success. Dotted decimal addresses should be either accepted by PR_StringToNetAddr or they shouldn't be accepted at all.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

> the bug is about PR_GetAddrInfoByName delegating the call to getaddrinfo

That statement really confused me, at first.  But now I think you intended to 
write: the bug is about PR_StringToNetAddr delegating the call to getaddrinfo.
Yes?

I think the real complaint here is that getaddrinfo on Windows is generating
a DNS request.  Yes?

Comment 6

[Wan-Teh Chang]

The problem description is not clear to me.  Let me explain the
complicated control structure in PR_StringToNetAddr:

2238 PR_IMPLEMENT(PRStatus) PR_StringToNetAddr(const char *string, PRNetAddr *addr)
2239 {
2240     if (!_pr_initialized) _PR_ImplicitInitialization();
2241 
2242 #if !defined(_PR_HAVE_GETADDRINFO)
2243     return pr_StringToNetAddrFB(string, addr);
2244 #else
2245 #if defined(_PR_INET6_PROBE)
2246     if (!_pr_ipv6_is_present)
2247         return pr_StringToNetAddrFB(string, addr);
2248 #endif
2249     return pr_StringToNetAddrGAI(string, addr);
2250 #endif
2251 }

The _PR_HAVE_GETADDRINFO macro is defined if the getaddrinfo
function is available on some versions of that platform.
_PR_HAVE_GETADDRINFO is defined for Windows because
Windows XP has the getaddrinfo function.

The _PR_INET6_PROBE macro is defined if we must perform a
runtime check (the "probe") to determine if IPv6 is enabled.
This is the case for Windows -- even though Windows XP has
the IPv6 socket functions, IPv6 is not always enabled.  The
check is performed during NSPR library initialization, and
the result of the check is stored in the global variable
_pr_ipv6_is_present.

So, for most Windows XP computers, where IPv6 is not enabled,
PR_StringToNetAddr is essentially this:

2238 PR_IMPLEMENT(PRStatus) PR_StringToNetAddr(const char *string, PRNetAddr *addr)
2239 {
2240     if (!_pr_initialized) _PR_ImplicitInitialization();
2241
2247         return pr_StringToNetAddrFB(string, addr);
2251 }

Note that it won't call getaddrinfo.

On Windows XP computers where IPv6 is enabled, PR_StringToNetAddr is
essentially this:

2238 PR_IMPLEMENT(PRStatus) PR_StringToNetAddr(const char *string, PRNetAddr *addr)
2239 {
2240     if (!_pr_initialized) _PR_ImplicitInitialization();
2241
2249     return pr_StringToNetAddrGAI(string, addr);
2251 }

Note that it will call getaddrinfo.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, do you know of any reason why getaddrinfo needs to do a DNS request?
We're not asking it to do a reverse DNS lookup (Are we?  We shouldn't.) so
I see no reason why it would need to make a DNS request.  

I do think that, with respect to dotted decimal IPv4 addresses, 
PR_StringToNetAddr should accept the same syntax in all cases, whether 
calling the FB or GAI implementation.  

Comment 8

[Wan-Teh Chang]

When PR_StringToNetAddr calls getaddrinfo, it passes the
AI_NUMERICHOST flag.  It seems that getaddrinfo should know
that DNS requests aren't needed when the AI_NUMERICHOST flag
is specified.

Comment 9

[Wladimir Palant]

No, I meant PR_GetAddrInfoByName. Sorry, I didn't explain myself clearly. So here is the problem:

I wrote a patch for nsIEffectiveTLDService to add special treating for hostnames that are actually IP addresses in dotted decimal notation. Since we want to bind security checks to nsIEffectiveTLDService it is important that this catches all possible cases, the TLD for "1.2.3.4" is certainly not "4". The patch uses PR_StringToNetAddr.

Now I noticed a problematic case there. We can have a URL like http://127.0.0.1./ on Windows, the request will go to localhost so it is the same as http://127.0.0.1/. But PR_GetAddrInfoByName doesn't recognize "127.0.0.1." as an IP address. What happens in Necko when "127.0.0.1." is resolved:

1. PR_GetStringToNetAddr is called, fails
2. PR_GetAddrInfoByName is called and passes the call to getaddrinfo
3. getaddrinfo sends a DNS request (since we established above that "127.0.0.1." is not a valid IP address this actually makes sense)
4. The DNS request comes back reporting failure
5. getaddrinfo ignores the failure and returns 127.0.0.1 as result!

The last point is the problem here. Because of this Necko will accept the hostname "127.0.0.1." while nsIEffectiveTLDService doesn't recognize it as an IP address and returns a wrong result. But judging by the discussion here maybe I should change the patch in nsIEffectiveTLDService to remove any trailing dots before calling PR_GetStringToNetAddr - just to be on the safe side.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

I do not believe that 127.0.0.1.  (with the trailing dot) is a valid IP address
representation.  It is not a DNS name.  BSD's extensions to DNS names 
(including the trailing dot extension) do not apply to IP addresses.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

To be clear, IMO, this bug is invalid.  The correct and consistent behavior 
would not interpret 127.0.0.1. as an IP address on ANY platform, IMO.

Comment 12

[Wladimir Palant]

(In reply to comment #11)
> To be clear, IMO, this bug is invalid.  The correct and consistent behavior 
> would not interpret 127.0.0.1. as an IP address on ANY platform, IMO.

I see. If you don't think that this should be considered in NSPR - so be it. Bug 364129 already worked around this problem in nsEffectiveTLDService implementation.

Comment 13

[[e-mail]]

Should we RESO/INVALID this one then?

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

yes