Opened 3 months ago

Closed 4 weeks ago

#31088 closed defect (fixed)

Check IPv4 and IPv6 private addresses in descriptors

Reported by: teor Owned by: neel
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version: Tor: 0.2.3.21-rc
Severity: Normal Keywords: dgoulet-merge, ipv6, tor-relay, tor-client, tor-dirauth
Cc: neel Actual Points: 0.4
Parent ID: #24403 Points:
Reviewer: nickm Sponsor:

Child Tickets

TicketStatusOwnerSummaryComponent
#21003closedextend_info_describe should list IPv6 address (if present)Core Tor/Tor

Change History (33)

comment:1 Changed 3 months ago by neel

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

comment:2 Changed 3 months ago by neel

Status: assignedneeds_review

I have a PR here: https://github.com/torproject/tor/pull/1182

I'm not sure if tests are needed here. I don't think they're needed so I didn't include them here.

Also here:

And when clients connect:
https://github.com/torproject/tor/blob/f7e8b3b68c8e2cecfc7ff4072e9f00d316aaba4f/src/core/or/circuitbuild.c#L552

I didn't see any mention of separate IPv4 or IPv6 addresses here or in extend_info_t.

comment:3 Changed 3 months ago by dgoulet

Reviewer: nickm

comment:4 Changed 3 months ago by nickm

Status: needs_reviewneeds_revision

Two issues.

First, have a look at your checks in circuit_extend(): it will make the extend cell get rejected only when *BOTH* of the target addresses are internal. I don't think that's right.

Second, I see that in dirserv_router_has_valid_address() you're testing the address for is_null, but in circuit_extend() you aren't. What's the reasoning there?

comment:5 in reply to:  4 Changed 3 months ago by neel

Status: needs_revisionneeds_review

I have made the changes.

Replying to nickm:

Two issues.

First, have a look at your checks in circuit_extend(): it will make the extend cell get rejected only when *BOTH* of the target addresses are internal. I don't think that's right.

I fixed it.

Second, I see that in dirserv_router_has_valid_address() you're testing the address for is_null, but in circuit_extend() you aren't. What's the reasoning there?

I originally planned to do this in the if statement, but broke off it. I decided to remove the is_null check and pushed it also.

comment:6 Changed 2 months ago by nickm

I decided to remove the is_null check and pushed it also.

What is the reason for not checking for null addresses? Will a null address count as internal? Do we want it to?

comment:7 Changed 2 months ago by neel

I don't believe a null address will count as internal, but I removed the check because in tor_addr_is_internal_() at the end of the function on a null family (or any non-IPv4/IPv6):

  /* unknown address family... assume it's not safe for external use */
  /* rather than tor_assert(0) */
  log_warn(LD_BUG, "tor_addr_is_internal() called from %s:%d with a "
           "non-IP address of type %d", filename, lineno, (int)v_family);
  tor_fragile_assert();
  return 1;

So (I guess) it would report as internal anyways.

comment:8 in reply to:  7 Changed 2 months ago by teor

Replying to neel:

I don't believe a null address will count as internal, but I removed the check because in tor_addr_is_internal_() at the end of the function on a null family (or any non-IPv4/IPv6):

  /* unknown address family... assume it's not safe for external use */
  /* rather than tor_assert(0) */
  log_warn(LD_BUG, "tor_addr_is_internal() called from %s:%d with a "
           "non-IP address of type %d", filename, lineno, (int)v_family);
  tor_fragile_assert();
  return 1;

So (I guess) it would report as internal anyways.

We don't want to execute a tor_fragile_assert().

So the null address checks are required, and we should treat a null address as a missing address:

  • if one address is null, use the result for the other address
  • if both addresses are null, reject, because the request can never succeed

comment:9 in reply to:  2 ; Changed 2 months ago by teor

Status: needs_reviewneeds_revision

Replying to neel:

I have a PR here: https://github.com/torproject/tor/pull/1182

I'm not sure if tests are needed here. I don't think they're needed so I didn't include them here.

Tests are always needed. Please write tests.

Also here:

And when clients connect:
https://github.com/torproject/tor/blob/f7e8b3b68c8e2cecfc7ff4072e9f00d316aaba4f/src/core/or/circuitbuild.c#L552

I didn't see any mention of separate IPv4 or IPv6 addresses here or in extend_info_t.

That's because you're working on a child ticket of #24403, which will introduce separate IPv4 and IPv6 addresses in extend_info_t. (Or proposal 306 will introduce them if we do it first.)

Please open a separate ticket for the parts of this ticket that you can't do yet, because they depend on future planned changes.

comment:10 Changed 2 months ago by neel

Status: needs_revisionneeds_review

A test for dirserv_router_has_valid_address() has been implemented. However, dirserv_router_has_valid_address() does not mark null IPv6 as internal as relays may not have IPv6 addresses.

An internal IPv6 address in dirserv_router_has_valid_address() logic is when it passes tor_addr_is_internal() and isn't null (so we don't flag IPv4-only relays as internal).

A circuit_extend() test would not be trivial because it would involve circuit extending logic, but this is tested in the chutney tests. circuit_extend() is only called in connection_edge_process_relay_cell() on these use cases:

    case RELAY_COMMAND_EXTEND:
    case RELAY_COMMAND_EXTEND2:

And I did not see these be tested outside of chutney.

Setting as needs review.

comment:11 in reply to:  9 ; Changed 2 months ago by teor

Reviewer: nickmnickm, teor
Status: needs_reviewneeds_revision

Replying to teor:

Replying to neel:

Also here:

And when clients connect:
https://github.com/torproject/tor/blob/f7e8b3b68c8e2cecfc7ff4072e9f00d316aaba4f/src/core/or/circuitbuild.c#L552

I didn't see any mention of separate IPv4 or IPv6 addresses here or in extend_info_t.

That's because you're working on a child ticket of #24403, which will introduce separate IPv4 and IPv6 addresses in extend_info_t. (Or proposal 306 will introduce them if we do it first.)

Please open a separate ticket for the parts of this ticket that you can't do yet, because they depend on future planned changes.

You've added some code that isn't needed yet. Please reject all IPv6 extends in this patch, and open a separate child ticket of #24403 for allowing IPv6 extends.

comment:12 Changed 2 months ago by neel

Status: needs_revisionneeds_review

I made your requested changes. Setting as needs review.

comment:13 in reply to:  11 Changed 2 months ago by neel

Replying to teor:

Replying to teor:

Replying to neel:

Also here:

And when clients connect:
https://github.com/torproject/tor/blob/f7e8b3b68c8e2cecfc7ff4072e9f00d316aaba4f/src/core/or/circuitbuild.c#L552

I didn't see any mention of separate IPv4 or IPv6 addresses here or in extend_info_t.

That's because you're working on a child ticket of #24403, which will introduce separate IPv4 and IPv6 addresses in extend_info_t. (Or proposal 306 will introduce them if we do it first.)

Please open a separate ticket for the parts of this ticket that you can't do yet, because they depend on future planned changes.

You've added some code that isn't needed yet. Please reject all IPv6 extends in this patch, and open a separate child ticket of #24403 for allowing IPv6 extends.

Opened #31413 for this purpose.

comment:14 Changed 2 months ago by teor

Milestone: Tor: unspecifiedTor: 0.4.2.x-final
Status: needs_reviewneeds_revision
Version: Tor: 0.2.3.21-rc

comment:15 Changed 2 months ago by neel

Status: needs_revisionneeds_review

Made the changes requested.

comment:16 Changed 2 months ago by teor

Status: needs_reviewneeds_revision

There are still a few unnecessary changes, I added comments to the pull request.

I'll leave the final review to nickm.

I did a draft implementation of #21003 so that we can see the IPv6 addresses in router_describe().

comment:17 Changed 2 months ago by neel

Status: needs_revisionneeds_review

I removed the unnecessary changes.

comment:18 Changed 8 weeks ago by teor

I finished #21003 so that we can see the IPv6 addresses in router_describe() when an IPv6 address is rejected.

I think that's a necessary part of this patch: otherwise, the rejection messages are going to be very confusing.

comment:19 Changed 8 weeks ago by teor

Reviewer: nickm, teornickm

comment:20 Changed 7 weeks ago by nickm

This is looking better! I've made a new, squashed PR here, since the history of the old branch had grown a bit long to review.

I've added a couple of fixes to the unit test code; please let me know if you agree with them?

comment:21 Changed 7 weeks ago by teor

Where is the new PR?
I can't find it in:
https://github.com/torproject/tor/pulls

comment:22 Changed 7 weeks ago by nickm

Oh dear, I moved to my next task too quickly. The PR is at https://github.com/torproject/tor/pull/1265

comment:23 Changed 7 weeks ago by teor

Status: needs_reviewneeds_revision

I reviewed nickm's changes in the PR:

  • please fix the routerinfo comment
  • please remove the empty commit "Check for private IPv6 addresses in circuit_extend()"

comment:24 Changed 7 weeks ago by nickm

Status: needs_revisionneeds_review

Fixed in pr1182_squashed_v2, PR at https://github.com/torproject/tor/pull/1267

comment:25 Changed 7 weeks ago by teor

Actual Points: 0.4
Keywords: asn-merge dgoulet-merge consider-backport-after-authority-test consider-backport-after-0421-alpha 040-backport-maybe 041-backport-maybe added
Status: needs_reviewmerge_ready

Ok, looks good to me.

Neel, let us know if you have any concerns about Nick's extra commits.

We might decide to backport this change to our supported authority releases 0.4.0 and 0.4.1.
It's a low-risk change that improves code correctness, and relay operator feedback when IPv6 is misconfigured.
But we should make sure we test master on moria1 first.

This is not a security issue, because authorities can't reach private addresses anyway, so the relay will never be in the consensus.
So it is also ok not to backport it.

comment:26 Changed 7 weeks ago by asn

Resolution: fixed
Status: merge_readyclosed

merged!

comment:27 Changed 7 weeks ago by teor

Description: modified (diff)
Summary: Check IPv4 and IPv6 private addresses in descriptors, first hops, and extendsCheck IPv4 and IPv6 private addresses in descriptors

Oops I thought I edited this ticket's summary.

comment:28 Changed 7 weeks ago by teor

Milestone: Tor: 0.4.2.x-finalTor: 0.4.1.x-final
Resolution: fixed
Status: closedreopened

Please don't close tickets that are marked for backport: move them to the latest backport release instead.

comment:29 Changed 7 weeks ago by teor

Status: reopenedmerge_ready

comment:30 Changed 7 weeks ago by teor

Keywords: consider-backport-after-0421 added; consider-backport-after-0421-alpha removed

Oops, drop the -alpha from backport versions

comment:31 Changed 6 weeks ago by asn

Keywords: asn-merge removed

comment:32 Changed 4 weeks ago by nickm

This could be the cause of #31793.

comment:33 Changed 4 weeks ago by teor

Keywords: consider-backport-after-authority-test 040-backport-maybe 041-backport-maybe consider-backport-after-0421 removed
Resolution: fixed
Status: merge_readyclosed

Let's not backport this fix: it is only for authorities, and it caused #31793.

Note: See TracTickets for help on using tickets.