Clients with NoIPv4Traffic should only choose IPv6-supporting Exits

Tor logs a warning when this happens in connection_ap_get_begincell_flags(), but it should actually fail.

Earlier in the process, it should choose an IPv6-supporting exit when NoIPv4Traffic is set (and choose an IPv4-supporting exit when NoIPv6 traffic is set).

changed milestone to %Tor: 0.3.5.x-final

added 031-deferred-20170425 032-unreached 035-removed component::core tor/tor ipv6 milestone::Tor: 0.3.5.x-final owner::neel parent::21311 points::0.5 priority::medium resolution::not a bug reviewer::dgoulet severity::normal status::closed type::defect labels

Triage: batch-defer unowned items of priority Medium or lower to 0.3.2.

Trac:
Milestone: Tor: 0.3.1.x-final to Tor: 0.3.2.x-final
Keywords: N/A deleted, 031-deferred-20170425 added

Mark a large number of tickets that I do not think we will do for 0.3.2.

Trac:
Keywords: N/A deleted, 032-unreached added
Milestone: Tor: 0.3.2.x-final to Tor: unspecified

Trac:
Status: new to assigned
Owner: N/A to neel
Cc: N/A to neel@neelc.org

in connection_ap_get_begincell_flags(), would it be okay that in:

  if (!ap_conn->entry_cfg.ipv4_traffic)
    flags |= BEGIN_FLAG_IPV4_NOT_OK;

I add an || with options->NoIPv4Traffic in the if statement.

And here:

  if (ap_conn->entry_cfg.ipv6_traffic && exitnode) {
    tor_addr_t a;
    tor_addr_make_null(&a, AF_INET6);
    if (compare_tor_addr_to_node_policy(&a, ap_conn->socks_request->port,
                                        exitnode)
        != ADDR_POLICY_REJECTED) {
      /* Only say "IPv6 OK" if the exit node supports IPv6. Otherwise there's
       * no point. */
      flags |= BEGIN_FLAG_IPV6_OK;
    }
  }

I add an && with !options->NoIPv6Traffic in the if statement.

Is this correct, or should I do something else instead?

UPDATE: correct a paragraph.

The second if statement I am talking about is the top one in the second snippet.

Replying to neel:

in connection_ap_get_begincell_flags(), would it be okay that in:

{{{ if (!ap_conn->entry_cfg.ipv4_traffic) flags |= BEGIN_FLAG_IPV4_NOT_OK; }}}

I add an || with options->NoIPv4Traffic in the if statement.

And here:

{{{ if (ap_conn->entry_cfg.ipv6_traffic && exitnode) { tor_addr_t a; tor_addr_make_null(&a, AF_INET6); if (compare_tor_addr_to_node_policy(&a, ap_conn->socks_request->port, exitnode) != ADDR_POLICY_REJECTED) { /* Only say "IPv6 OK" if the exit node supports IPv6. Otherwise there's * no point. */ flags |= BEGIN_FLAG_IPV6_OK; } } }}}

I add an && with !options->NoIPv6Traffic in the if statement.

Is this correct, or should I do something else instead?

UPDATE: correct a paragraph.

NoIPv4Traffic is not in options.

It is a SOCKSPort option flag, and it is the same as !ap_conn->entry_cfg.ipv4_traffic and ap_conn->entry_cfg.ipv6_traffic.

You need to:

• modify exit selection before the circuit so that only exits that support IPv6 are chosen when !ap_conn->entry_cfg.ipv4_traffic
• modify stream attachment after the circuit is built so that only streams with exits that support IPv6 are chosen when !ap_conn->entry_cfg.ipv4_traffic

Thank You.

Also, where should I look to make these changes?

Exit selection is called something like choose_good_exit_server(). Stream attachment is called something like ap_attach_stream().

Thank you again.

Sorry to ask you one more time, but I have a few more questions:

1. Is connection_ap_attach_pending() the stream attachment code?
2. Would it be okay to use node_has_ipv6_addr() or node_has_ipv6_orport() along with checking for !ap_conn->entry_cfg.ipv4_traffic to see if a node has IPv6 support? If so, would node_has_ipv6_addr() suffice or would I need node_has_ipv6_orport()?

Replying to neel:

Thank you again.

Sorry to ask you one more time, but I have a few more questions:

1. Is connection_ap_attach_pending() the stream attachment code?

Yes.

2. Would it be okay to use node_has_ipv6_addr() or node_has_ipv6_orport() along with checking for !ap_conn->entry_cfg.ipv4_traffic to see if a node has IPv6 support? If so, would node_has_ipv6_addr() suffice or would I need node_has_ipv6_orport()?

No, these functions check for an IPv6 ORPort. You need to check for an IPv6 exit policy that allows exiting to the port in the request.

When building a circuit, use exit_policy_is_general_exit(), but add a "sa_family_t family" argument to it for IPv6. It should look a bit like policy_is_reject_star().

When attaching a stream, use compare_tor_addr_to_node_policy() with a NULL address for a domain, or the IPv6 address.

Also, in connection_ap_can_use_exit(), if I call exit_policy_is_general_exit(), I think I am getting a NULL exit_policy if I access it via exit_node->ri->exit_policy. In microdesc_t, the exit_policy is a short_policy_t.

What is the right way to get an exit policy in connection_ap_can_use_exit() to pass to exit_policy_is_general_exit()?

compare_tor_addr_to_node_policy() uses whichever exit policy a node has available. Is there a node_policy_is_general_exit()? If not, you may have to write one.

Thank you so much for your help.

My patch is almost ready, but I have two more questions before I finish.

Should I:

• Modify the code to only choose IPv4 exits before the circuit if !ap_conn->entry_cfg.ipv6_traffic is set?
• Modify the code to only choose IPv4 exits in the stream attachment if !ap_conn->entry_cfg.ipv4_traffic is set?

I have a branch on GitHub which addresses this patch. The URL is: https://github.com/neelchauhan/tor/tree/b21346-001

It checks for IPv6-capable exits if !ap_conn->entry_cfg.ipv4_traffic is set but not for IPv4-capable exits if !ap_conn->entry_cfg.ipv4_traffic is set. If you need the latter, I can add it.

Trac:
Milestone: Tor: unspecified to Tor: 0.3.4.x-final
Status: assigned to needs_review

Trac:
Reviewer: N/A to dgoulet

Hmmm... I think the approach here is not ideal. It seems that the patch is looking at the choosen exit node if we can use it with IPv6 and then unmark it.

Instead, shouldn't we pick the node in the first place with IPv6 exit policy and not fail later? The function connection_ap_can_use_exit() seems to already performing validation on the exit node if the we have no IPv4 but IPv6 enabled.

So I think we should do this filtering around circuit_get_best()?

Trac:
Status: needs_review to needs_revision

Move most needs_revision tickets from 0.3.4 to 0.3.5: we're about to hit the feature freeze.

If you really need to get this into 0.3.4, please drop me a line. :)

Trac:
Milestone: Tor: 0.3.4.x-final to Tor: 0.3.5.x-final