Opened 3 months ago

Closed 3 weeks ago

#25036 closed defect (fixed)

Tor 0.3.2 rejects connections to raw ipv6 addresses

Reported by: pastly Owned by: tbb-team
Priority: High Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version: Tor: 0.3.2.1-alpha
Severity: Normal Keywords: regression, ipv6, 032-backport, 033-must, review-group-34, 033-triage-20180320, 033-included-20180320
Cc: teor, rl1987, arma Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description (last modified by pastly)

OS X 10.11.6, Tor Browser 7.5

I have to configure TB to use a socks5 proxy to reach the internet (I predict this is unrelated). I disabled safe logging.

Attempting to go to https://[2a00:1450:401b:800::200e]/ (google) produces the following Tor logs.

1/26/18, 14:51:25.600 [NOTICE] Bootstrapped 85%: Finishing handshake with first hop 
1/26/18, 14:51:48.700 [NOTICE] Bootstrapped 90%: Establishing a Tor circuit 
1/26/18, 14:51:50.000 [NOTICE] Tor has successfully opened a circuit. Looks like client functionality is working. 
1/26/18, 14:51:50.000 [NOTICE] Bootstrapped 100%: Done 
1/26/18, 14:51:50.900 [NOTICE] New control connection opened from 127.0.0.1. 
1/26/18, 14:51:51.000 [NOTICE] New control connection opened from 127.0.0.1. 
1/26/18, 14:54:06.900 [WARN] Your application (using socks5 to port 443) gave Tor a malformed hostname: "2a00:1450:401b:800::200e". Rejecting the connection. 
1/26/18, 14:54:06.900 [WARN] Your application (using socks5 to port 443) gave Tor a malformed hostname: "2a00:1450:401b:800::200e". Rejecting the connection. 
1/26/18, 14:55:38.200 [WARN] Your application (using socks5 to port 443) gave Tor a malformed hostname: "2a00:1450:401b:800::200e". Rejecting the connection. 
1/26/18, 14:55:38.200 [WARN] Your application (using socks5 to port 443) gave Tor a malformed hostname: "2a00:1450:401b:800::200e". Rejecting the connection. 

https://ipv6.google.com works with every 6th circuit. I'm sure this is unrelated and has to do with exits half supporing ipv6 or something.

Child Tickets

TicketStatusOwnerSummaryComponent
#25055closedstring_is_valid_hostname() returns true for IPv4 addressesCore Tor/Tor
#25211closedTor fails to resolve ipv6.google.comCore Tor/Tor

Change History (32)

comment:1 Changed 3 months ago by pastly

Description: modified (diff)

comment:2 Changed 3 months ago by cypherpunks

Mmm... nice link for testing "Secure Connection Failed"...

comment:3 in reply to:  2 ; Changed 3 months ago by pastly

Replying to cypherpunks:

Mmm... nice link for testing "Secure Connection Failed"...

That's what happens for me too. That's the point.

comment:4 in reply to:  3 Changed 3 months ago by cypherpunks

Replying to pastly:

Replying to cypherpunks:

Mmm... nice link for testing "Secure Connection Failed"...

That's what happens for me too. That's the point.

That happens not every time and is a different bug, fwiw. But warnings in the log look like a Tor bug.

comment:5 Changed 3 months ago by gk

Here is what a Tor 0.3.1.9 gives me just dropped into the same Tor Browser 7.5 with the error in this bug:

[01-26 19:14:18] TorLauncher DBUG: Event response: 650 STATUS_CLIENT WARN DANGEROUS_SOCKS PROTOCOL=SOCKS5 ADDRESS=2a00:1450:401b:800::200e:443
[01-26 19:14:18] TorLauncher NOTE: WARN DANGEROUS_SOCKS PROTOCOL=SOCKS5 ADDRESS=2a00:1450:401b:800::200e:443

So, this does indeed to be a Tor issue then, no?

comment:6 Changed 3 months ago by gk

Component: Applications/Tor BrowserCore Tor/Tor
Summary: Tor Browser 7.5 rejects connections to raw ipv6 addressesTor 0.3.2 rejects connections to raw ipv6 addresses
Version: Tor: 0.3.2.1-alpha

The first bad commit is 7f05f896630e857ad2803e80b48924f026f66eb7 which was supposed to fix #22461.

comment:7 Changed 3 months ago by pastly

Milestone: Tor: 0.3.3.x-final

Giving this a (hopefully reasonable) milestone because I've heard that if Core Tor/Tor tickets don't have a milestone, they tend to get lost.

comment:8 Changed 3 months ago by nickm

Cc: teor rl1987 arma added
Keywords: regression ipv6 added
Priority: MediumHigh

adding keywords; cc'ing rl1987 as patch author, teor as ipv6 guru, and arma as reviewer.

comment:9 Changed 3 months ago by nickm

Keywords: 032-backport added

comment:10 Changed 3 months ago by teor

Using string_is_valid_hostname() excludes IPv6 addresses.
(It doesn't exclude IPv4 addresses, because they are numbers separated by dots. Let's deal with that in #25055.)

/** Return true iff <b>string</b> matches a pattern of DNS names
 * that we allow Tor clients to connect to.
 *
 * Note: This allows certain technically invalid characters ('_') to cope
 * with misconfigured zones that have been encountered in the wild.
 */
int
string_is_valid_hostname(const char *string)

We should check both calls introduced by this patch. I think the second call should be modified to include IPv4 and IPv6 addresses. And we should add a unit test for this use case.

comment:11 Changed 3 months ago by arma

I think we should make this function return yes for every string that is a legit destination address -- that is, something that is ok to send in a relay begin cell. (Does that make #25055 a duplicate of this ticket?)

Speaking of unit tests, it might also be smart to test using socks4a with an ipv6 address as the provided destination name -- I would expect it to work, but based on this ticket I bet it currently doesn't.

I suspect socks4-with-ipv6 has been broken the whole time too.

comment:12 Changed 2 months ago by dgoulet

Status: newneeds_information

Quick question on this. Are we sure this is not Tor Browser sending a SOCKS5 request with the type "fqdn" used with an IPv6 address?

That string_is_valid_hostname() check is done when we get an atyp = 0x03 in the SOCKS5 request which means tor should expect a FQDN thus returning the error here is fine. The IPv6 address is handled before.

Why is TB sending a fqdn request at all with this address: https://[2a00:1450:401b:800::200e]/ ?

comment:13 in reply to:  12 ; Changed 2 months ago by teor

Status: needs_informationnew

Replying to dgoulet:

Quick question on this. Are we sure this is not Tor Browser sending a SOCKS5 request with the type "fqdn" used with an IPv6 address?

That string_is_valid_hostname() check is done when we get an atyp = 0x03 in the SOCKS5 request which means tor should expect a FQDN thus returning the error here is fine. The IPv6 address is handled before.

Why is TB sending a fqdn request at all with this address: https://[2a00:1450:401b:800::200e]/ ?

Technically, you are correct: Tor Browser should not send IP addresses as strings at all.
But if it does, we should handle them sensibly.

Here are the possible types:

ATYP   address type of following address
             o  IP V4 address: X'01'
             o  DOMAINNAME: X'03'
             o  IP V6 address: X'04'

See section 4 in https://tools.ietf.org/html/rfc1928

Here are the specs for each type:

In an address field (DST.ADDR, BND.ADDR), the ATYP field specifies
   the type of address contained within the field:

          o  X'01'

   the address is a version-4 IP address, with a length of 4 octets

          o  X'03'

   the address field contains a fully-qualified domain name.  The first
   octet of the address field contains the number of octets of name that
   follow, there is no terminating NUL octet.

          o  X'04'

   the address is a version-6 IP address, with a length of 16 octets.

So IPv4 and IPv6 should be binary encoded. And all strings should be FQDNs.

Tor Browser is obviously sending IPv4 and IPv6 addresses as text, in violation of the spec. Tor has always accepted IPv4 addresses as text, in violation of the spec. Therefore, we should accept IPv6 addresses as text, just like we did in 0.3.1. Because we can't break a feature like this.

comment:14 in reply to:  13 ; Changed 2 months ago by dgoulet

Replying to teor:

Replying to dgoulet:

Quick question on this. Are we sure this is not Tor Browser sending a SOCKS5 request with the type "fqdn" used with an IPv6 address?

That string_is_valid_hostname() check is done when we get an atyp = 0x03 in the SOCKS5 request which means tor should expect a FQDN thus returning the error here is fine. The IPv6 address is handled before.

Why is TB sending a fqdn request at all with this address: https://[2a00:1450:401b:800::200e]/ ?

Tor Browser is obviously sending IPv4 and IPv6 addresses as text, in violation of the spec. Tor has always accepted IPv4 addresses as text, in violation of the spec. Therefore, we should accept IPv6 addresses as text, just like we did in 0.3.1. Because we can't break a feature like this.

Well Tor Browser ships its own "tor" so it can fix this and just use a "tor" that have this working?

I do see the importance of not breaking a feature even though it means violating the spec but with TB, it controls the whole chain. If TB can't fix this, then I agree that tor should just revert to accepting IPv4/IPv6 string values.

comment:15 in reply to:  14 Changed 2 months ago by teor

Replying to dgoulet:

Replying to teor:

Replying to dgoulet:

Quick question on this. Are we sure this is not Tor Browser sending a SOCKS5 request with the type "fqdn" used with an IPv6 address?

That string_is_valid_hostname() check is done when we get an atyp = 0x03 in the SOCKS5 request which means tor should expect a FQDN thus returning the error here is fine. The IPv6 address is handled before.

Why is TB sending a fqdn request at all with this address: https://[2a00:1450:401b:800::200e]/ ?

Tor Browser is obviously sending IPv4 and IPv6 addresses as text, in violation of the spec. Tor has always accepted IPv4 addresses as text, in violation of the spec. Therefore, we should accept IPv6 addresses as text, just like we did in 0.3.1. Because we can't break a feature like this.

Well Tor Browser ships its own "tor" so it can fix this and just use a "tor" that have this working?

I do see the importance of not breaking a feature even though it means violating the spec but with TB, it controls the whole chain. If TB can't fix this, then I agree that tor should just revert to accepting IPv4/IPv6 string values.

Many SOCKS5 clients use Tor 0.3.2. Tor needs to allow them all to send IP addresses as hostnames, as we did in 0.3.1.

Tor Browser may do a temporary fix, that's up to them.

comment:16 Changed 2 months ago by arma

I think we should ignore the spec and do the right thing. The right thing is that if you get an address string that is usable as a destination address, you should use it.

comment:17 in reply to:  16 Changed 2 months ago by dgoulet

Replying to arma:

I think we should ignore the spec and do the right thing. The right thing is that if you get an address string that is usable as a destination address, you should use it.

Usability, I agree and less hassle of broken apps opening tickets. Flip side, we let flourish an ecosystem of broken applications that uses SOCKS the wrong way and in this case, Tor Browser.

But I won't argue more, I think TB knows what to do to do it right and "tor" can just be lenient with what format the address comes in (binary or text).

comment:18 Changed 2 months ago by rl1987

Status: newneeds_review

Preliminary patch for this (and #25055): https://github.com/rl1987/tor/commits/bugs_25036_25055

I'm not exactly sure if it's okay to feed bracketed IPv6 string into tor_inet_pton, but it seems to work.

comment:19 Changed 2 months ago by rl1987

Raised #25211 for ipv6.google.com problem.

comment:20 in reply to:  18 ; Changed 2 months ago by teor

Status: needs_reviewneeds_revision

Replying to rl1987:

Preliminary patch for this (and #25055): https://github.com/rl1987/tor/commits/bugs_25036_25055

Did you replace every use of string_is_valid_hostname()?

Please add another unit test for a bracketed IPv6 address, rather than replacing the existing raw IPv6 address test.

I'm not exactly sure if it's okay to feed bracketed IPv6 string into tor_inet_pton, but it seems to work.

Is tor_inet_pton() used for bracketed IPv6 strings elsewhere in the codebase?
If not, how are they handled? Do it the same way.
If so, please update the function comment on tor_inet_pton(), and add a unit test for tor_inet_pton() with a bracketed IPv6 address.

comment:21 in reply to:  20 ; Changed 2 months ago by rl1987

Replying to teor:

Replying to rl1987:

Preliminary patch for this (and #25055): https://github.com/rl1987/tor/commits/bugs_25036_25055

Did you replace every use of string_is_valid_hostname()?

Yes, there were two of them in SOCKS parsing code.

Please add another unit test for a bracketed IPv6 address, rather than replacing the existing raw IPv6 address test.

Do we want to allow IPv6 addresses without brackets? RFC2732 seems to require brackets in IPv6 URLs, but does not explicitly forbid bracket-less form.

Is tor_inet_pton() used for bracketed IPv6 strings elsewhere in the codebase?
If not, how are they handled? Do it the same way.
If so, please update the function comment on tor_inet_pton(), and add a unit test for tor_inet_pton() with a bracketed IPv6 address.

In 0dda450d41b1fcfc108f1d7ee4bc3417a7494ce3 I did it the way tor_addr_parse is doing - if brackets are detected, they are stripped out and resulting string is passed to tor_inet_pton. Also I realised the testcase was wrong all along - it was comparing to -1.

comment:22 in reply to:  21 Changed 2 months ago by teor

Replying to rl1987:

Replying to teor:

Replying to rl1987:

Preliminary patch for this (and #25055): https://github.com/rl1987/tor/commits/bugs_25036_25055

Did you replace every use of string_is_valid_hostname()?

Yes, there were two of them in SOCKS parsing code.

Please add another unit test for a bracketed IPv6 address, rather than replacing the existing raw IPv6 address test.

Do we want to allow IPv6 addresses without brackets? RFC2732 seems to require brackets in IPv6 URLs, but does not explicitly forbid bracket-less form.

We can allow bracketless IPv6 without ambiguity, because the hostname and port are separate in the socks protocol.
So, yes, please allow bracketless IPv6.

Is tor_inet_pton() used for bracketed IPv6 strings elsewhere in the codebase?
If not, how are they handled? Do it the same way.
If so, please update the function comment on tor_inet_pton(), and add a unit test for tor_inet_pton() with a bracketed IPv6 address.

In 0dda450d41b1fcfc108f1d7ee4bc3417a7494ce3 I did it the way tor_addr_parse is doing - if brackets are detected, they are stripped out and resulting string is passed to tor_inet_pton. Also I realised the testcase was wrong all along - it was comparing to -1.

Thanks. These changes look good. You could call strlen() once if you wanted to.

comment:23 Changed 2 months ago by dgoulet

Keywords: 033-must added

Some ticket dgoulet thinks must go in 033.

comment:24 Changed 2 months ago by rl1987

Status: needs_revisionneeds_review

comment:25 Changed 2 months ago by teor

Status: needs_reviewneeds_revision

Review in #25055.

comment:26 Changed 7 weeks ago by rl1987

Status: needs_revisionneeds_review

comment:27 Changed 7 weeks ago by nickm

Keywords: review-group-34 added

comment:28 Changed 5 weeks ago by dgoulet

Reviewer: dgoulet

Reviewer week 03/16th

comment:29 Changed 4 weeks ago by nickm

Keywords: 033-triage-20180320 added

Marking all tickets reached by current round of 033 triage.

comment:30 Changed 4 weeks ago by nickm

Keywords: 033-included-20180320 added

Mark 033-must tickets as triaged-in for 0.3.3

comment:31 Changed 3 weeks ago by dgoulet

Status: needs_reviewneeds_information

Hmmm ok so review of the clean branch is happening in #25055 and mike is on it. So, once it is closed, we should consider this ticket done as well. For now, going in needs_information awaiting the faith of the other ticket.

comment:32 Changed 3 weeks ago by nickm

Resolution: fixed
Status: needs_informationclosed

#25055 is merged

Note: See TracTickets for help on using tickets.