Bug 182427 - Add a way to check if a host is an IP address
Summary: Add a way to check if a host is an IP address
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-02 03:46 PST by Carlos Garcia Campos
Modified: 2018-02-09 09:33 PST (History)
15 users (show)

See Also:


Attachments
Patch (8.66 KB, patch)
2018-02-02 03:51 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (8.67 KB, patch)
2018-02-02 04:19 PST, Carlos Garcia Campos
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.55 MB, application/zip)
2018-02-02 05:30 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.55 MB, application/zip)
2018-02-02 05:37 PST, Build Bot
no flags Details
Patch (9.15 KB, patch)
2018-02-02 05:56 PST, Carlos Garcia Campos
achristensen: review+
Details | Formatted Diff | Diff
Patch for landing (9.15 KB, patch)
2018-02-06 01:04 PST, Carlos Garcia Campos
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.26 MB, application/zip)
2018-02-06 02:52 PST, Build Bot
no flags Details
Patch for landing (9.34 KB, patch)
2018-02-09 04:44 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2018-02-02 03:46:53 PST
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.
Comment 1 Carlos Garcia Campos 2018-02-02 03:51:01 PST
Created attachment 332955 [details]
Patch
Comment 2 Carlos Garcia Campos 2018-02-02 04:19:49 PST
Created attachment 332956 [details]
Patch
Comment 3 Build Bot 2018-02-02 05:30:02 PST
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
Comment 4 Build Bot 2018-02-02 05:30:04 PST
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
Comment 5 Build Bot 2018-02-02 05:37:19 PST
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
Comment 6 Build Bot 2018-02-02 05:37:21 PST
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
Comment 7 Carlos Garcia Campos 2018-02-02 05:56:13 PST
Created attachment 332963 [details]
Patch
Comment 8 Michael Catanzaro 2018-02-02 08:55:55 PST
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.
Comment 9 Carlos Garcia Campos 2018-02-02 09:07:49 PST
(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.
Comment 10 Alex Christensen 2018-02-02 10:18:31 PST
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.
Comment 11 Alexey Proskuryakov 2018-02-02 12:10:02 PST
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.
Comment 12 Michael Catanzaro 2018-02-02 12:39:12 PST
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.
Comment 13 Alexey Proskuryakov 2018-02-02 13:30:12 PST
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.
Comment 14 Carlos Garcia Campos 2018-02-05 00:04:41 PST
Committed r228086: <https://trac.webkit.org/changeset/228086>
Comment 15 Radar WebKit Bug Importer 2018-02-05 00:06:56 PST
<rdar://problem/37227012>
Comment 16 Carlos Garcia Campos 2018-02-05 00:43:56 PST
(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.
Comment 17 Charlie Turner 2018-02-05 01:27:33 PST
(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/)"
Comment 18 Michael Catanzaro 2018-02-05 07:59:19 PST
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.
Comment 19 Matt Lewis 2018-02-05 13:25:17 PST
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
Comment 20 Matt Lewis 2018-02-05 13:36:23 PST
Reverted r228086 for reason:

This introduced a failure with API test URLTest.HostIsIPAddress.

Committed r228118: <https://trac.webkit.org/changeset/228118>
Comment 21 Carlos Garcia Campos 2018-02-05 23:50:22 PST
(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)
Comment 22 Alexey Proskuryakov 2018-02-05 23:59:09 PST
This address is accepted by inet_pton, so the failing subtest is wrong.
Comment 23 Carlos Garcia Campos 2018-02-06 00:29:56 PST
(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.
Comment 24 Carlos Garcia Campos 2018-02-06 01:04:51 PST
Created attachment 333159 [details]
Patch for landing

Disable the failing test in Mac.
Comment 25 Build Bot 2018-02-06 02:52:27 PST
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
Comment 26 Build Bot 2018-02-06 02:52:28 PST
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
Comment 27 Michael Catanzaro 2018-02-06 06:00:35 PST
(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.
Comment 28 Michael Catanzaro 2018-02-06 06:04:10 PST
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.
Comment 29 Carlos Garcia Campos 2018-02-06 06:28:18 PST
(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.
Comment 30 Alexey Proskuryakov 2018-02-06 10:35:07 PST
> 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.
Comment 31 Carlos Garcia Campos 2018-02-07 22:37:22 PST
Committed r228261: <https://trac.webkit.org/changeset/228261>
Comment 32 Matt Lewis 2018-02-08 10:55:37 PST
Reverted r228261 for reason:

This broke an internal build

Committed r228282: <https://trac.webkit.org/changeset/228282>
Comment 33 Carlos Alberto Lopez Perez 2018-02-08 14:11:02 PST
(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
Comment 34 Carlos Garcia Campos 2018-02-08 22:26:23 PST
(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?
Comment 35 Wenson Hsieh 2018-02-09 00:57:32 PST
(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!
Comment 36 Carlos Garcia Campos 2018-02-09 01:06:20 PST
(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.
Comment 37 Carlos Garcia Campos 2018-02-09 04:44:18 PST
Created attachment 333477 [details]
Patch for landing
Comment 38 Brian Burg 2018-02-09 09:09:26 PST
Comment on attachment 333477 [details]
Patch for landing

Let's try again.
Comment 39 WebKit Commit Bot 2018-02-09 09:33:02 PST
Comment on attachment 333477 [details]
Patch for landing

Clearing flags on attachment: 333477

Committed r228323: <https://trac.webkit.org/changeset/228323>
Comment 40 WebKit Commit Bot 2018-02-09 09:33:04 PST
All reviewed patches have been landed.  Closing bug.