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

"curl -6 --interface" binds to ULA when connecting to GUA #1764

Closed
jamsla opened this Issue Aug 11, 2017 · 8 comments

Comments

Projects
None yet
2 participants
@jamsla

jamsla commented Aug 11, 2017

On an interface configured with both an IPv6 global unicast address (GUA - 2000::/3) and a unique local address (ULA - fc00::/7), curl will bind to the ULA when trying to reach a global address if the --interface option is specified.

This occurs because the interface option bypasses the normal source address selection rules, and the Curl_ipv6_scope function (lib/if2ip.c) does not recognise ULAs, which are similar in function to the now-deprecated site-local addresses.

I did this

$ curl -v -6 --interface eno1 -o - -D - http://www.archlinux.org/check_network_status.txt

*   Trying 2a01:4f8:172:1d86::1...
* TCP_NODELAY set
* Local Interface eno1 is ip fdxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx using address family 10
* SO_BINDTODEVICE eno1 failed with errno 1: Operation not permitted; will do regular bind
* Local port: 0
* connect to 2a01:4f8:172:1d86::1 port 80 failed: Connection timed out
* Failed to connect to www.archlinux.org port 80: Connection timed out
* Closing connection 0
curl: (7) Failed to connect to www.archlinux.org port 80: Connection timed out

I expected the following

*   Trying 2a01:4f8:172:1d86::1...
* TCP_NODELAY set
* Local Interface eno1 is ip 2a00:23c4:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx using address family 10
* SO_BINDTODEVICE eno1 failed with errno 1: Operation not permitted; will do regular bind
* Local port: 0
* Connected to www.archlinux.org (2a01:4f8:172:1d86::1) port 80 (#0)
> GET /check_network_status.txt HTTP/1.1
    ...

curl/libcurl version

This was git master (7e949de):

curl 7.55.0-DEV (x86_64-pc-linux-gnu) libcurl/7.55.0-DEV OpenSSL/1.1.0f zlib/1.2.11 libpsl/0.17.0 (+libicu/59.1) libssh2/1.8.0 nghttp2/1.23.1 librtmp/2.3
Release-Date: [unreleased]
Protocols: dict file ftp ftps gopher http https imap imaps pop3 pop3s rtmp rtsp scp sftp smb smbs smtp smtps telnet tftp 
Features: AsynchDNS Debug TrackMemory IPv6 Largefile GSS-API Kerberos SPNEGO NTLM NTLM_WB SSL libz TLS-SRP HTTP2 UnixSockets HTTPS-proxy PSL 

operating system

$ uname -a
Linux bodger 4.12.4-1-ARCH #1 SMP PREEMPT Fri Jul 28 18:54:18 UTC 2017 x86_64 GNU/Linux

Possible fixes

I patched Curl_ipv6_scope locally to treat the ULA prefix in the same way as site-local addresses.

diff --git a/lib/if2ip.c b/lib/if2ip.c
index 3e74cc687..5bd81aca8 100644
--- a/lib/if2ip.c
+++ b/lib/if2ip.c
@@ -85,6 +85,9 @@ unsigned int Curl_ipv6_scope(const struct sockaddr *sa)
     default:
       break;
     }
+
+    if((b[0] & 0xFE) == 0xFC) /* Handle ULAs */
+      return IPV6_SCOPE_SITELOCAL;
   }
 #endif

This resolves the issue, but perhaps I should have defined another IPV6_SCOPE_ constant instead?

Another solution in my case would be to skip the bind call when setsockopt(sockfd, SOL_SOCKET, SO_BINDTODEVICE, ...) succeeds (see 'Context' below). Perhaps both changes would be a good idea?

Naturally, this approach will never be perfect without emulating the logic in RFC6724, and it has potential implications when temporary/privacy addresses are in use, or there are multiple global unicast prefixes, and so forth. For the most part this isn't a problem because the --interface option shouldn't be used (or could be used with an address), but...

Context

On my network, I have a 'smart' thermostat from a subsidiary of a certain internet giant, which insists on handing out a ULA prefix even while it accepts prefixes advertised by my router. This confuses the NetworkManager connectivity check, which uses libcurl to perform its connectivity test, with curl_easy_setopt(ehandle, CURLOPT_INTERFACE, "if!eno1"); to control the outgoing interface (see https://github.com/NetworkManager/NetworkManager/blob/master/src/nm-connectivity.c). As the ULA address is enumerated before the GUA address, the connectivity test always fails due to this issue.

Interestingly, although NetworkManager has CAP_NET_RAW allowing it to use SO_BINDTODEVICE in connect.c's bindlocal function, the latter goes on to perform a bind against the incorrect address anyway. If it did not do this, the IPv6 stack's own source address selection mechanism would work.

It's possible to debate whether the problem is in libcurl's behaviour or NetworkManager's use of it. I'm raising the issue here initially because the "interface" option as implemented doesn't seem to match expectations: doing just SO_BINDTODEVICE would be better, when possible. The alternative would require NetworkManager to supply a preferred address without the benefit of the resolved target address. Having said that, it seems to already have an algorithm for choosing an address to display in the little control panel thing - maybe that's a better guess than curl makes :)

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Aug 12, 2017

Member

I think making it a separate scope seems fair and a nicer fix. Would it be as simple as this 1764 patch ?

Member

bagder commented Aug 12, 2017

I think making it a separate scope seems fair and a nicer fix. Would it be as simple as this 1764 patch ?

@jamsla

This comment has been minimized.

Show comment
Hide comment
@jamsla

jamsla Aug 12, 2017

Unfortunately you'd need another few case statements to fit it into the existing switch, as it currently looks at the first ten bits of the address. Only 7 are fixed for ULAs, though in practice the 8th will always be 1 at the moment. The rest are essentially random - see RFC 4193 (https://tools.ietf.org/html/rfc4193).

jamsla commented Aug 12, 2017

Unfortunately you'd need another few case statements to fit it into the existing switch, as it currently looks at the first ten bits of the address. Only 7 are fixed for ULAs, though in practice the 8th will always be 1 at the moment. The rest are essentially random - see RFC 4193 (https://tools.ietf.org/html/rfc4193).

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Aug 12, 2017

Member

Aha, ok. Then let's make that a separate check before the switch() like this:

+    if((b[0] & 0xFE) == 0xFC) /* Handle ULAs */
+      return IPV6_SCOPE_UNIQUELOCAL;
     switch(w & 0xFFC0) {
     case 0xFE80:
       return IPV6_SCOPE_LINKLOCAL;
     case 0xFEC0:
       return IPV6_SCOPE_SITELOCAL;
-    case 0xFC00:
-      return IPV6_SCOPE_UNIQUELOCAL;

(diff from the previous patch)

Would that work? I'll make a separate PR with this to show.

Member

bagder commented Aug 12, 2017

Aha, ok. Then let's make that a separate check before the switch() like this:

+    if((b[0] & 0xFE) == 0xFC) /* Handle ULAs */
+      return IPV6_SCOPE_UNIQUELOCAL;
     switch(w & 0xFFC0) {
     case 0xFE80:
       return IPV6_SCOPE_LINKLOCAL;
     case 0xFEC0:
       return IPV6_SCOPE_SITELOCAL;
-    case 0xFC00:
-      return IPV6_SCOPE_UNIQUELOCAL;

(diff from the previous patch)

Would that work? I'll make a separate PR with this to show.

bagder added a commit that referenced this issue Aug 12, 2017

ipv6_scope: support unique local
Fixes #1764
Reported-by: James Slaughter
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Aug 12, 2017

Member

See #1773 for the updated version of the patch as a PR.

Member

bagder commented Aug 12, 2017

See #1773 for the updated version of the patch as a PR.

@jamsla

This comment has been minimized.

Show comment
Hide comment
@jamsla

jamsla Aug 12, 2017

Looks good to me!

jamsla commented Aug 12, 2017

Looks good to me!

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Aug 12, 2017

Member

Cool, thanks a lot. I'll just let the CI job try out the PR and if nothing bad pops out I'll merge that.

Member

bagder commented Aug 12, 2017

Cool, thanks a lot. I'll just let the CI job try out the PR and if nothing bad pops out I'll merge that.

@bagder bagder closed this in b748d7a Aug 13, 2017

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Aug 13, 2017

Member

Thanks!

Member

bagder commented Aug 13, 2017

Thanks!

@jamsla

This comment has been minimized.

Show comment
Hide comment
@jamsla

jamsla Aug 13, 2017

Thank you!

jamsla commented Aug 13, 2017

Thank you!

@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.