Opened 16 months ago

Last modified 10 days ago

#21346 needs_review defect

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

comment:1 Changed 13 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 8 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 3 weeks ago by neel

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

comment:4 Changed 3 weeks 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 3 weeks ago by neel (previous) (diff)

comment:5 Changed 3 weeks 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 3 weeks 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 3 weeks ago by neel

Thank You.

Also, where should I look to make these changes?

comment:8 Changed 3 weeks 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 3 weeks 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 3 weeks 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 3 weeks 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 3 weeks 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 3 weeks 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 2 weeks 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 2 weeks ago by nickm

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

comment:16 Changed 12 days ago by dgoulet

Reviewer: dgoulet

comment:17 Changed 12 days 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 12 days 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 10 days 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 10 days ago by neel

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

comment:21 Changed 10 days 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 10 days ago by teor

Status: needs_revisionneeds_review
Note: See TracTickets for help on using tickets.