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.

[dgoulet]

Thanks neel for you patch!

So it took me a while to wrap my head around your fix and the current behavior. I do understand that we need to pick an Exit node that matches our IP criteria so that connection_ap_get_begincell_flags() doesn't get confused.

The connection_ap_can_use_exit() is the right place to look for the SocksPort flags vs the Exit policy but it appears it *is* doing the right thing already with selecting the address family and then using compare_tor_addr_to_node_policy() to keep the chosen exit node or not.

Seems to me your patch that adds node_policy_is_general_exit() is doing roughly the same thing but only for port 80/443 (general exit policy). Can you enlighten me on how it is different or fixes things.

I'm asking here because there are two issues. The first one is that it is unclear _why_ your code does what it does and second it duplicates most of its code from other functions which in turn turns out to be mostly what compare_tor_addr_to_node_policy() does...

Thanks!

[neel]

The new patch's PR is here: ​https://github.com/torproject/tor/pull/140

Travis and Coveralls passes. AppVeyor passes for 32 bit Windows, and fails for 64-bit, but the failure happens on FAIL: src/test/test_key_expiration.sh and I believe this FAIL line is not related to my code.

[neel]

Also the new patch/PR is based around circuit_is_acceptable().

[dgoulet]

Left comments on the PR.

[neel]

Thank you for the feedback on the PR.

My question is, should we just give up on this patch? I noticed this code in connection_ap_can_use_exit() which (like you said and I noticed by reading the code) does the same thing my previous patch did:

else if (!conn->entry_cfg.ipv4_traffic && conn->entry_cfg.ipv6_traffic) {
  tor_addr_make_null(&addr, AF_INET6);
  addrp = &addr;
} else if (conn->entry_cfg.ipv4_traffic && !conn->entry_cfg.ipv6_traffic) {
  tor_addr_make_null(&addr, AF_INET);
  addrp = &addr;
}

If we shouldn't give up, what should I do indead (I can still write a patch if we do this route)?

[dgoulet]

I honestly don't know what is the problem if we kind of check for it :S...

I would refer to @teor here to maybe point us in the right direction.

[teor]

As far as I can tell, this feature is already implemented correctly. The bug I'm seeing must be in some other part of tor.

Thanks for your hard work, sorry it wasn't needed.