circuit_send_intermediate_onion_skin() and extend_cell_format() should check for IPv6
When circuit_send_intermediate_onion_skin() and extend_cell_format() handle tor_addr_t, they assume they are IPv4.
But in #23502 (moved), 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.
Activity
changed milestone to %Tor: unspecified
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 fromextend_cell_format().Trac:
Status: new to accepted
Owner: N/A to nickmDoes 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 (moved).
I'm not sure if we need to feature gate them with a consensus parameter like #24868 (moved). Are there any privacy implications of a small number of relays doing IPv6 extends?
Trac:
Parent: N/A to #24403 (moved)The 0.3.3 freeze deadline has passed, all these children of #24403 (moved) belong in 0.3.4
Trac:
Milestone: Tor: 0.3.3.x-final to Tor: 0.3.4.x-final
We only check
orport_ipv4incheck_extend_cell()so this is only a gate keeping for theextend_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->addris 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 ?
Trac:
Reviewer: N/A to dgoulet
Status: needs_review to merge_readyI 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.
Trac:
Milestone: Tor: 0.3.4.x-final to Tor: 0.3.5.x-final-
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?Trac:
Status: merge_ready to needs_information
Cc: N/A to teor 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 (moved), 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.
Trac:
Status: needs_information to needs_revisionCovered by #24405 (moved)
Trac:
Parent: #24403 (moved) to #24405 (moved)moved to tpo/core/tor#24000 (closed)