Opened 13 months ago

Last modified 3 weeks ago

#21310 needs_revision defect

Exits should tell clients when they are connecting to an IPv6-only hostname

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version: Tor: 0.2.4.7-alpha
Severity: Normal Keywords: ipv6
Cc: Actual Points: 0.1
Parent ID: #21311 Points: 1
Reviewer: Sponsor:

Description (last modified by teor)

Edit: Turns out that these IPv6Exit option checks prevent clients ever seeing that they're trying to connect to an IPv6-only hostname

This code is wrong for at least two reasons:

  • it should also unset BEGIN_FLAG_IPV6_OK, and
  • it's way too early in the function: we might end up resolving an IPv6-only hostname, learn that it doesn't match our exit policy, and send the address back in the REASON_EXITPOLICY RELAY_END cell

(See https://gitweb.torproject.org/torspec.git/tree/tor-spec.txt#n1436 )
(Apparently this works anyway.)

  if (! options->IPv6Exit) {
    /* I don't care if you prefer IPv6; I can't give you any. */
    bcell.flags &= ~BEGIN_FLAG_IPV6_PREFERRED;
    /* If you don't want IPv4, I can't help. */
    if (bcell.flags & BEGIN_FLAG_IPV4_NOT_OK) {
      tor_free(address);
      relay_send_end_cell_from_edge(rh.stream_id, circ,
                                    END_STREAM_REASON_EXITPOLICY, NULL);
      return 0;
    }
  }

Child Tickets

Change History (15)

comment:1 Changed 13 months ago by teor

Description: modified (diff)
Milestone: Tor: unspecifiedTor: 0.3.1.x-final
Parent ID: #21311
Status: newneeds_review
Version: Tor: 0.2.4.7-alpha

These are fixed as part of #21311, as they touch the same code.

Can we get this in 0.3.0?

comment:2 Changed 13 months ago by teor

See my branch bug21310 on github.
It turns out it doesn't touch the same code, but it does require #21311 to work.

comment:3 Changed 13 months ago by teor

Actual Points: 0.1

comment:4 Changed 13 months ago by teor

Description: modified (diff)
Summary: Fix IPv6Exit support in connection_exit_begin_conn()Exits should tell clients when they are connecting to an IPv6-only hostname

comment:5 Changed 13 months ago by arma

-  if (! options->IPv6Exit) {
-    /* I don't care if you prefer IPv6; I can't give you any. */
-    bcell.flags &= ~BEGIN_FLAG_IPV6_PREFERRED;
-    /* If you don't want IPv4, I can't help. */
-    if (bcell.flags & BEGIN_FLAG_IPV4_NOT_OK) {
-      tor_free(address);
-      relay_send_end_cell_from_edge(rh.stream_id, circ,
-                                    END_STREAM_REASON_EXITPOLICY, NULL);
-      return 0;
-    }
-  }

Shouldn't we leave the "if you won't be ok with v4, and I won't exit to v6, then return an end cell immediately" clause in there?

I think if clients are going to say they're not ok with v4, then it's their responsibility to pick an exit that says it can do v6.

comment:6 in reply to:  1 ; Changed 13 months ago by arma

Replying to teor:

Can we get this in 0.3.0?

I'm a big fan of getting everything in as early as possible, but if 0.3.0 is meant to be frozen right about now, and this fix could destabilize things in ways we haven't tested much, getting it into 0.3.0 makes me a bit nervous.

That said, if we put it into 0.3.1 and there's a problem with it, it will be a long time until we notice the problem, and we'll have forgotten about this ticket in the mean time. So that isn't obviously a winning move either. :)

comment:7 in reply to:  6 Changed 13 months ago by teor

Replying to arma:

Replying to teor:

Can we get this in 0.3.0?

I'm a big fan of getting everything in as early as possible, but if 0.3.0 is meant to be frozen right about now, and this fix could destabilize things in ways we haven't tested much, getting it into 0.3.0 makes me a bit nervous.

You're right, I forgot we were doing the freeze this week. (I'm used to them being about a month later.)

That said, if we put it into 0.3.1 and there's a problem with it, it will be a long time until we notice the problem, and we'll have forgotten about this ticket in the mean time. So that isn't obviously a winning move either. :)

Just make me the IPv6 person :-)
Although even I forget things I've written by the time they're in stable.

comment:8 in reply to:  5 ; Changed 13 months ago by teor

Status: needs_reviewneeds_revision

Replying to arma:

-  if (! options->IPv6Exit) {
-    /* I don't care if you prefer IPv6; I can't give you any. */
-    bcell.flags &= ~BEGIN_FLAG_IPV6_PREFERRED;
-    /* If you don't want IPv4, I can't help. */
-    if (bcell.flags & BEGIN_FLAG_IPV4_NOT_OK) {
-      tor_free(address);
-      relay_send_end_cell_from_edge(rh.stream_id, circ,
-                                    END_STREAM_REASON_EXITPOLICY, NULL);
-      return 0;
-    }
-  }

Shouldn't we leave the "if you won't be ok with v4, and I won't exit to v6, then return an end cell immediately" clause in there?

I think if clients are going to say they're not ok with v4, then it's their responsibility to pick an exit that says it can do v6.

I think we could keep this in, but we can't adjust the flags on the cell.

comment:9 in reply to:  8 Changed 13 months ago by teor

Replying to teor:

Replying to arma:

...
I think if clients are going to say they're not ok with v4, then it's their responsibility to pick an exit that says it can do v6.

I think we could keep this in, but we can't adjust the flags on the cell.

What do current clients do?

It seems like tor logs a warning in connection_ap_get_begincell_flags, when it should actually fail and choose another exit. (Or, even better, only choose IPv6-supporting exits on connections with NoIPv4Traffic set, and, although redundant in the current network, only choose IPv4-supporting exits on exits with NoIPv6Traffic set. We can't do this with PreferIPv6, because that would put all Tor Browser users on 12% of the exits.)

So I guess we need to open another child ticket and make that change.

comment:10 Changed 13 months ago by teor

Opened #21346 for the client change

Last edited 13 months ago by teor (previous) (diff)

comment:11 Changed 9 months ago by nickm

Milestone: Tor: 0.3.1.x-finalTor: 0.3.2.x-final

0.3.1 feature freeze today: these don't seem like they will be all sorted out in time. Let's try to make progress in 0.3.2.

comment:12 Changed 8 months ago by teor

Owner: set to teor
Status: needs_revisionaccepted

comment:13 Changed 8 months ago by teor

Status: acceptedneeds_revision

comment:14 Changed 5 months ago by teor

Milestone: Tor: 0.3.2.x-finalTor: 0.3.3.x-final

I'm not going to get time to do these in 0.3.2.
Moving them to 0.3.3.

comment:15 Changed 3 weeks ago by teor

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final

Moving most of my assigned tickets to 0.3.4

Note: See TracTickets for help on using tickets.