Opened 3 years ago
Closed 17 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 3 years ago by
Keywords: | 031-deferred-20170425 added |
---|---|
Milestone: | Tor: 0.3.1.x-final → Tor: 0.3.2.x-final |
comment:2 Changed 2 years ago by
Keywords: | 032-unreached added |
---|---|
Milestone: | Tor: 0.3.2.x-final → Tor: unspecified |
Mark a large number of tickets that I do not think we will do for 0.3.2.
comment:3 Changed 20 months ago by
Cc: | neel@… added |
---|---|
Owner: | set to neel |
Status: | new → assigned |
comment:4 follow-up: 6 Changed 20 months ago by
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.
comment:5 Changed 20 months ago by
The second if
statement I am talking about is the top one in the second snippet.
comment:6 Changed 20 months ago by
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
||
withoptions->NoIPv4Traffic
in theif
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 theif
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:8 Changed 20 months ago by
Exit selection is called something like choose_good_exit_server().
Stream attachment is called something like ap_attach_stream().
comment:9 follow-up: 10 Changed 19 months ago by
Thank you again.
Sorry to ask you one more time, but I have a few more questions:
- Is
connection_ap_attach_pending()
the stream attachment code? - Would it be okay to use
node_has_ipv6_addr()
ornode_has_ipv6_orport()
along with checking for!ap_conn->entry_cfg.ipv4_traffic
to see if a node has IPv6 support? If so, wouldnode_has_ipv6_addr()
suffice or would I neednode_has_ipv6_orport()
?
comment:10 Changed 19 months ago by
Replying to neel:
Thank you again.
Sorry to ask you one more time, but I have a few more questions:
- Is
connection_ap_attach_pending()
the stream attachment code?
Yes.
- Would it be okay to use
node_has_ipv6_addr()
ornode_has_ipv6_orport()
along with checking for!ap_conn->entry_cfg.ipv4_traffic
to see if a node has IPv6 support? If so, wouldnode_has_ipv6_addr()
suffice or would I neednode_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 19 months ago by
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 19 months ago by
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 19 months ago by
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 19 months ago by
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 19 months ago by
Milestone: | Tor: unspecified → Tor: 0.3.4.x-final |
---|---|
Status: | assigned → needs_review |
comment:16 Changed 19 months ago by
Reviewer: | → dgoulet |
---|
comment:17 Changed 19 months ago by
Status: | needs_review → needs_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 19 months ago by
Milestone: | Tor: 0.3.4.x-final → Tor: 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 19 months ago by
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 19 months ago by
BTW commits after 5dbf70f903b16173c2f77e185fea1cadbc35ab3a are all my "patch" which accidentally went to "master".
comment:21 Changed 19 months ago by
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 19 months ago by
Status: | needs_revision → needs_review |
---|
comment:23 Changed 18 months ago by
Status: | needs_review → needs_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 18 months ago by
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:27 Changed 18 months ago by
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 17 months ago by
Status: | needs_revision → needs_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 17 months ago by
Keywords: | 035-removed added |
---|
comment:30 Changed 17 months ago by
Resolution: | → not a bug |
---|---|
Status: | needs_information → closed |
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.
Triage: batch-defer unowned items of priority Medium or lower to 0.3.2.