Bug 182427 – Add a way to check if a host is an IP address

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Carlos Garcia Campos]

There are several places where this is needed. We currently just assume that any host ending in a digit is an IP address, except in PublicSuffix where platform specific code is used. We could add URL::hostIsIPAddress() platform specific implementations, falling back to current assumption if there isn't an implementation for the platform.

[Carlos Garcia Campos]

Created attachment 332955 [details]
Patch

[Carlos Garcia Campos]

Created attachment 332956 [details]
Patch

[EWS Watchlist]

Comment on attachment 332956 [details]
Patch

Attachment 332956 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6330249

New failing tests:
http/tests/security/set-domain-remove-subdomain-for-ip-address.html

[EWS Watchlist]

Created attachment 332958 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 332956 [details]
Patch

Attachment 332956 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6330271

New failing tests:
http/tests/security/set-domain-remove-subdomain-for-ip-address.html

[EWS Watchlist]

Created attachment 332959 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Carlos Garcia Campos]

Created attachment 332963 [details]
Patch

[Michael Catanzaro]

Comment on attachment 332963 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332963&action=review

Looks good, but I'll hold off on r+ because I bet Alex will want to review this.

> Source/WebCore/page/OriginAccessEntry.cpp:43
> +    , m_hostIsIPAddress(URL::hostIsIPAddress(m_host))

Might be better to drop this member variable in favor of direct calls to hostIsIPAddress(m_host)?

> Source/WebCore/page/OriginAccessEntry.cpp:70
> -    if (m_hostIsIPAddress && m_ipAddressSettings == TreatIPAddressAsIPAddress)
> +    if (m_ipAddressSettings == TreatIPAddressAsIPAddress && (m_hostIsIPAddress || URL::hostIsIPAddress(origin.host())))

I think this is right, but it's an unrelated functionality change. Previously there was no test to check if origin was an IP address, and now there is.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #8)
> Comment on attachment 332963 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332963&action=review
> 
> Looks good, but I'll hold off on r+ because I bet Alex will want to review
> this.
> 
> > Source/WebCore/page/OriginAccessEntry.cpp:43
> > +    , m_hostIsIPAddress(URL::hostIsIPAddress(m_host))
> 
> Might be better to drop this member variable in favor of direct calls to
> hostIsIPAddress(m_host)?

Probably out of scope for this patch.

> > Source/WebCore/page/OriginAccessEntry.cpp:70
> > -    if (m_hostIsIPAddress && m_ipAddressSettings == TreatIPAddressAsIPAddress)
> > +    if (m_ipAddressSettings == TreatIPAddressAsIPAddress && (m_hostIsIPAddress || URL::hostIsIPAddress(origin.host())))
> 
> I think this is right, but it's an unrelated functionality change.
> Previously there was no test to check if origin was an IP address, and now
> there is.

It's not unrelated, this patch revealed the bug, that's what caused the http/tests/security/set-domain-remove-subdomain-for-ip-address.html failure. With the previous code 0.0.1 was considered a valid IP, which was wrong, and matchesOrigin() returned false. We should also check that the origin host is not an IP before trying to match domains.

[Alex Christensen]

Comment on attachment 332963 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332963&action=review

> Source/WebCore/platform/URL.cpp:1042
> +    // Assume that any host that ends with a digit is trying to be an IP address.
> +    return !host.isEmpty() && isASCIIDigit(host[host.length() - 1]);

It's awful that this was ever used, but it's good that this is being used on fewer platforms.

[Alexey Proskuryakov]

Comment on attachment 332963 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332963&action=review

> Tools/ChangeLog:10
> +        * TestWebKitAPI/Tests/WebCore/URL.cpp:

There are more IP address formats than tested here, see e.g. Address Representations at https://en.m.wikipedia.org/wiki/IPv4

I think that it’s ok to make an incremental improvement, but I’m concerned that this quite incomplete check may be used for something important.

[Michael Catanzaro]

I think only WinCairo is using the naive implementation after this patch.

The GLib impl is here: https://git.gnome.org/browse/glib/tree/glib/ghostutils.c?id=40be86bb0e422247673ccc36fc3c493c882b4390#n683 and is only a hundred lines long; we could copy it into WebKit for use by WinCairo.

[Alexey Proskuryakov]

I think that it's also the Apple Windows port.

Mac does seem right.

Is the glib implementation even right? It doesn't seem like it recognizes "0x7f000001" as in "http://0x7f000001:8000/".

Either way, these abominations need to be added to the test. Perhaps a separate "extra bonus" test if we don't intend to fix the deficiencies in all ports right away.

[Carlos Garcia Campos]

Committed r228086: <https://trac.webkit.org/changeset/228086>

[Radar WebKit Bug Importer]

<rdar://problem/37227012>

[Carlos Garcia Campos]

(In reply to Alexey Proskuryakov from comment #13)
> I think that it's also the Apple Windows port.
> 
> Mac does seem right.
> 
> Is the glib implementation even right? It doesn't seem like it recognizes
> "0x7f000001" as in "http://0x7f000001:8000/".
> 
> Either way, these abominations need to be added to the test. Perhaps a
> separate "extra bonus" test if we don't intend to fix the deficiencies in
> all ports right away.

Maybe we can simply reuse what we have in URLParser and we don't need any platform specific code. Or we could combine both, since URLParser serializes the IP addresses, we could simply create a URL for the given doamin, and pass the parsed host as input of the platform specific entry point.

[Charlie Turner]

(In reply to Alexey Proskuryakov from comment #13)
> Is the glib implementation even right? It doesn't seem like it recognizes
> "0x7f000001" as in "http://0x7f000001:8000/".

It is considered a feature in GLib to reject these formats, see https://bugzilla.gnome.org/show_bug.cgi?id=679957

"The additional formats parsed by inet_aton() are not actually part of any standard, were not much used historically, and are only ever used now by phishers trying to obscure the domain name in URLs (eg, http://yourbank.com@1539136771/)"

[Michael Catanzaro]

Wow, we should definitely have some warning about tricks like http://yourbank.com@91.189.93.3/ (which is gnome.org). Firefox pops up a dialog that asks if you are really trying to visit 91.189.93.3 and authenticate with username yourbank.com, warning that someone may be trying to trick you. But we just accept it. That's bad.

[Matt Lewis]

This commit introduced a failure in API test URLTest.HostIsIPAddress on all Mac and iOS platforms. 

Failure:
/Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebCore/URL.cpp:248
Value of: URL::hostIsIPAddress("00123:4567:89AB:cdef:3210:7654:ba98:FeDc")
  Actual: true
Expected: false

https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/2733/steps/run-api-tests/logs/stdio
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/2733

[Matt Lewis]

Reverted r228086 for reason:

This introduced a failure with API test URLTest.HostIsIPAddress.

Committed r228118: <https://trac.webkit.org/changeset/228118>

[Carlos Garcia Campos]

(In reply to Matt Lewis from comment #19)
> This commit introduced a failure in API test URLTest.HostIsIPAddress on all
> Mac and iOS platforms. 
> 
> Failure:
> /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/
> WebCore/URL.cpp:248
> Value of: URL::hostIsIPAddress("00123:4567:89AB:cdef:3210:7654:ba98:FeDc")
>   Actual: true
> Expected: false
> 
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/2733/steps/run-
> api-tests/logs/stdio
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/2733

This commit is introducing that test not the failure, so it isn't a regression. It it's failing is because _web_looksLikeIPAddress() is accepting that wrong IP as a valid one, so there's little we can do. Reverting it doesn't fix anything. EWS didn't fail, so I would need that someone with a mac checks what test cases are failing and add a platform ifdef with a FIXME to disable those (I'm assuming there will be more)

[Alexey Proskuryakov]

This address is accepted by inet_pton, so the failing subtest is wrong.

[Carlos Garcia Campos]

(In reply to Alexey Proskuryakov from comment #22)
> This address is accepted by inet_pton, so the failing subtest is wrong.

Not according to RFC 4291: "x:x:x:x:x:x:x:x, where the 'x's are one to four hexadecimal digits of the eight 16-bit pieces of the address."

Our URLParser doesn't accept that one either.

[Carlos Garcia Campos]

Created attachment 333159 [details]
Patch for landing

Disable the failing test in Mac.

[EWS Watchlist]

Comment on attachment 333159 [details]
Patch for landing

Attachment 333159 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6379848

New failing tests:
transitions/transition-display-property.html

[EWS Watchlist]

Created attachment 333165 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #16)
> Maybe we can simply reuse what we have in URLParser and we don't need any
> platform specific code. Or we could combine both, since URLParser serializes
> the IP addresses, we could simply create a URL for the given doamin, and
> pass the parsed host as input of the platform specific entry point.

This failing test just proves the case for using URLParser instead of platform-specific functions to do work that is not in any way platform-specific.

[Michael Catanzaro]

Comment on attachment 333159 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=333159&action=review

> Source/WebCore/platform/mac/URLMac.mm:85
> +bool URL::hostIsIPAddress(const String& host)
> +{
> +    return [host _web_looksLikeIPAddress];
> +}

There's also a semantic problem here, in that you're implementing an "is" function in terms of a "looks like" function.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #28)
> Comment on attachment 333159 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333159&action=review
> 
> > Source/WebCore/platform/mac/URLMac.mm:85
> > +bool URL::hostIsIPAddress(const String& host)
> > +{
> > +    return [host _web_looksLikeIPAddress];
> > +}
> 
> There's also a semantic problem here, in that you're implementing an "is"
> function in terms of a "looks like" function.

Then we would allow the ipv4 addresses in the forms we don't want to accept. I guess we don't want to accept them when loading a URL either, so we would need to add an option to URLParser anyway.

[Alexey Proskuryakov]

> Our URLParser doesn't accept that one either.

This is an interesting observation, but I do not think that matching URLParser is what we are after here, at least not directly.

The new function added here just takes a String, so it doesn't have any pre-requisites, like the string being a normalized URL. There are certainly input paths into WebKit where we rely on external components more, such as when taking the initial request from the client, or when handling a redirect. All of these need to be vetted if/when we decide that we don't want to match inet_pton.

[Carlos Garcia Campos]

Committed r228261: <https://trac.webkit.org/changeset/228261>

[Matt Lewis]

Reverted r228261 for reason:

This broke an internal build

Committed r228282: <https://trac.webkit.org/changeset/228282>

[Carlos Alberto Lopez Perez]

(In reply to Matt Lewis from comment #32)
> Reverted r228261 for reason:
> 
> This broke an internal build
> 
> Committed r228282: <https://trac.webkit.org/changeset/228282>

Can you provide more info about what broke ? logs, etc

[Carlos Garcia Campos]

(In reply to Matt Lewis from comment #32)
> Reverted r228261 for reason:
> 
> This broke an internal build
> 
> Committed r228282: <https://trac.webkit.org/changeset/228282>

How am I supposed to deal with your internal builds?

[Wenson Hsieh]

(In reply to Carlos Alberto Lopez Perez from comment #33)
> (In reply to Matt Lewis from comment #32)
> > Reverted r228261 for reason:
> > 
> > This broke an internal build
> > 
> > Committed r228282: <https://trac.webkit.org/changeset/228282>
> 
> Can you provide more info about what broke ? logs, etc

Sorry for the delay — the original revision (r228261) caused an internal builder to start failing in PublicSuffixMac.mm because it could not find decodeHostName. This appears to be declared in WebCoreNSURLExtras.h, but the line where we #import "WebCoreNSURLExtras.h" was removed from PublicSuffixMac.mm in the original patch.

I can take a closer look tomorrow, but hopefully this helps a bit!

[Carlos Garcia Campos]

(In reply to Wenson Hsieh from comment #35)
> (In reply to Carlos Alberto Lopez Perez from comment #33)
> > (In reply to Matt Lewis from comment #32)
> > > Reverted r228261 for reason:
> > > 
> > > This broke an internal build
> > > 
> > > Committed r228282: <https://trac.webkit.org/changeset/228282>
> > 
> > Can you provide more info about what broke ? logs, etc
> 
> Sorry for the delay — the original revision (r228261) caused an internal
> builder to start failing in PublicSuffixMac.mm because it could not find
> decodeHostName. This appears to be declared in WebCoreNSURLExtras.h, but the
> line where we #import "WebCoreNSURLExtras.h" was removed from
> PublicSuffixMac.mm in the original patch.
> 
> I can take a closer look tomorrow, but hopefully this helps a bit!

Thanks for the details. It seems to me that simply adding the missing include to fix the internal build is less effort than rolling out the two patches again, but still, I'll submit a new patch with the missing include.

[Carlos Garcia Campos]

Created attachment 333477 [details]
Patch for landing

[Blaze Burg]

Comment on attachment 333477 [details]
Patch for landing

Let's try again.

[WebKit Commit Bot]

Comment on attachment 333477 [details]
Patch for landing

Clearing flags on attachment: 333477

Committed r228323: <https://trac.webkit.org/changeset/228323>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.