Opened 13 months ago

Last modified 2 weeks ago

#24000 needs_revision defect

circuit_send_intermediate_onion_skin() and extend_cell_format() should check for IPv6

Reported by: teor Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: ipv6, 034-triage-20180328, merge-deferred, 035-triaged-in-20180711
Cc: teor Actual Points:
Parent ID: #24403 Points: 0.5
Reviewer: dgoulet Sponsor:

Description

When circuit_send_intermediate_onion_skin() and extend_cell_format() handle tor_addr_t, they assume they are IPv4.

But in #23502, we almost wrote code that sent them an IPv6 address. In this case, they put 0.0.0.0 in the extend cell, but they could issue a BUG() warning and refuse to send the cell instead.

Or they could send a proper IPv6 link specifier where the extend cell allows it.

Child Tickets

Change History (15)

comment:1 Changed 10 months ago by nickm

Owner: set to nickm
Status: newaccepted

The circuit_send_intermediate_onion_skin() function has this code at the top.

  if (tor_addr_family(&hop->extend_info->addr) != AF_INET) {
    log_warn(LD_BUG, "Trying to extend to a non-IPv4 address.");
    return - END_CIRC_REASON_INTERNAL;
  }

and I think a small change to check_extend_cell() should get the behavior we want from extend_cell_format().

comment:2 Changed 10 months ago by nickm

Status: acceptedneeds_review

Please review my branch bug24000.

comment:3 Changed 10 months ago by teor

Parent ID: #24403

Does this implement IPv6 extends on relays? Or are there other bugs we need to fix?
I think we should put relay IPv6 extends in 0.3.4, and add a protover for them. See #24403.

I'm not sure if we need to feature gate them with a consensus parameter like #24868. Are there any privacy implications of a small number of relays doing IPv6 extends?

comment:4 Changed 10 months ago by nickm

Keywords: review-group-31 added

comment:5 Changed 10 months ago by teor

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

The 0.3.3 freeze deadline has passed, all these children of #24403 belong in 0.3.4

comment:6 Changed 10 months ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewmerge_ready

We only check orport_ipv4 in check_extend_cell() so this is only a gate keeping for the extend_cell_format() that does this:

  tor_addr_copy(&ec.orport_ipv4.addr, &hop->extend_info->addr);
  ec.orport_ipv4.port = hop->extend_info->port;

All in all, if the extend_info->addr is not an IPv4, we have trouble in the woods which could happen with a coding error and sending non IPv4 in those cells in a release would be bad.

I think that check is OK to have and ultimately we will have a proper "extend_info_t -> extend cell" layer.

lgtm;

I just wonder if teor is OK with this considering is comment:3 ?

comment:7 Changed 10 months ago by nickm

Keywords: review-group-31 removed

Remove merge_ready items that are in 0.3.4 or for backport consideration into 0.3.2 from review-group-31

comment:8 Changed 8 months ago by nickm

Keywords: 034-triage-20180328 added

comment:9 Changed 8 months ago by teor

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

I am risk-averse and don't think we should merge this until we're ready to actually send IPv6 link specifiers. Because I don't know if this patch helps with bugs where we accidentally send IPv6 extends. And we can't really test it.

ln5 is working on a proposal for IPv6 extends, so we should be able to merge this in 0.3.5 or 0.3.6.

comment:10 Changed 4 months ago by nickm

Keywords: merge-deferred added

comment:11 Changed 4 months ago by nickm

Keywords: 035-triaged-in-20180711 added

comment:12 Changed 3 months ago by nickm

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

comment:13 in reply to:  9 ; Changed 3 weeks ago by dgoulet

Cc: teor added
Status: merge_readyneeds_information

Replying to teor:

I am risk-averse and don't think we should merge this until we're ready to actually send IPv6 link specifiers. Because I don't know if this patch helps with bugs where we accidentally send IPv6 extends. And we can't really test it.

ln5 is working on a proposal for IPv6 extends, so we should be able to merge this in 0.3.5 or 0.3.6.

Moving this out of merge_ready. Lets figure this out . Maybe point to a ticket or sth?

comment:14 in reply to:  13 Changed 2 weeks ago by teor

Status: needs_informationneeds_revision

Replying to dgoulet:

Replying to teor:

I am risk-averse and don't think we should merge this until we're ready to actually send IPv6 link specifiers. Because I don't know if this patch helps with bugs where we accidentally send IPv6 extends. And we can't really test it.

ln5 is working on a proposal for IPv6 extends, so we should be able to merge this in 0.3.5 or 0.3.6.

Moving this out of merge_ready. Lets figure this out ^. Maybe point to a ticket or sth?

When we do IPv6 reachability checks on relays in #24403, we can merge this code.

We don't really have a status for that, so I'll put it in "needs_revision", because we need to add more code to this branch before it's mergeable.

comment:15 Changed 2 weeks ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

Note: See TracTickets for help on using tickets.