Clients with NoIPv4Traffic should only choose IPv6-supporting Exits

[teor]

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).

[nickm]

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

[nickm]

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

[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.

[neel]

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

[teor]

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

[neel]

Thank You.

Also, where should I look to make these changes?

[teor]

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

[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?
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()?

[teor]

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.

[neel]

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()?

[teor]

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.

[neel]

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?

[neel]

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.

[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()?

[nickm]

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. :)

[neel]

My updated GitHub branch is here: ​https://github.com/neelchauhan/tor

It is on the "master" branch (really). I know it is a bad idea but I will fix the branch issue after this patch is accepted or rejected. Sorry for this inconvenience.

[neel]

BTW commits after 5dbf70f903b16173c2f77e185fea1cadbc35ab3a are all my "patch" which accidentally went to "master".

[neel]

Sorry for another change, but the branch for this bug is now here:

​https://github.com/neelchauhan/tor/tree/b21346-001a

My "master" branch was reset to the original version.