Opened 22 months ago

Closed 5 months ago

#21346 closed defect (not a bug)

Clients with NoIPv4Traffic should only choose IPv6-supporting Exits

Reported by: teor Owned by: neel
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: ipv6, 031-deferred-20170425, 032-unreached 035-removed
Cc: neel@… Actual Points:
Parent ID: #21311 Points: 0.5
Reviewer: dgoulet Sponsor:

Description

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

Child Tickets

Change History (30)

comment:1 Changed 19 months ago by nickm

Keywords: 031-deferred-20170425 added
Milestone: Tor: 0.3.1.x-finalTor: 0.3.2.x-final

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

comment:2 Changed 14 months ago by nickm

Keywords: 032-unreached added
Milestone: Tor: 0.3.2.x-finalTor: unspecified

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

comment:3 Changed 7 months ago by neel

Cc: neel@… added
Owner: set to neel
Status: newassigned

comment:4 Changed 7 months ago by 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.

Last edited 7 months ago by neel (previous) (diff)

comment:5 Changed 7 months ago by neel

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

comment:6 in reply to:  4 Changed 7 months ago by 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

comment:7 Changed 7 months ago by neel

Thank You.

Also, where should I look to make these changes?

comment:8 Changed 7 months ago by teor

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

comment:9 Changed 7 months ago by 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()?

comment:10 in reply to:  9 Changed 7 months ago by 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.

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

comment:11 Changed 7 months ago by 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()?

comment:12 Changed 7 months ago by 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.

comment:13 Changed 7 months ago by 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?

comment:14 Changed 6 months ago by 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.

comment:15 Changed 6 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.4.x-final
Status: assignedneeds_review

comment:16 Changed 6 months ago by dgoulet

Reviewer: dgoulet

comment:17 Changed 6 months ago by dgoulet

Status: needs_reviewneeds_revision

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

comment:18 Changed 6 months ago by nickm

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

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

comment:19 Changed 6 months ago by 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.

comment:20 Changed 6 months ago by neel

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

comment:21 Changed 6 months ago by 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.

comment:22 Changed 6 months ago by teor

Status: needs_revisionneeds_review

comment:23 Changed 5 months ago by dgoulet

Status: needs_reviewneeds_revision

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!

comment:24 Changed 5 months ago by 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.

comment:25 Changed 5 months ago by neel

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

comment:26 Changed 5 months ago by dgoulet

Left comments on the PR.

comment:27 Changed 5 months ago by 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)?

comment:28 Changed 5 months ago by dgoulet

Status: needs_revisionneeds_information

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.

comment:29 Changed 5 months ago by dgoulet

Keywords: 035-removed added

comment:30 Changed 5 months ago by teor

Resolution: not a bug
Status: needs_informationclosed

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.

Note: See TracTickets for help on using tickets.