Opened 10 months ago

Last modified 6 weeks ago

#24000 merge_ready 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.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: ipv6, 034-triage-20180328, merge-deferred, 035-triaged-in-20180711
Cc: 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 (11)

comment:1 Changed 7 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 7 months ago by nickm

Status: acceptedneeds_review

Please review my branch bug24000.

comment:3 Changed 7 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 7 months ago by nickm

Keywords: review-group-31 added

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

Keywords: 034-triage-20180328 added

comment:9 Changed 5 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 6 weeks ago by nickm

Keywords: merge-deferred added

comment:11 Changed 6 weeks ago by nickm

Keywords: 035-triaged-in-20180711 added
Note: See TracTickets for help on using tickets.