Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#24469 closed defect (fixed)

Cannibalizing a circuit should check that first hop is in our guard state

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-circuit
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by dgoulet)

I noticed this on my v3 hidden service info logs which happened on Nov 22nd:

Nov 22 20:04:24.000 [info] internal (high-uptime) circ (length 4, last hop ThomasBernhardsHose): $175921396C7C426309AB03775A9930B6F611F794(open) $77159B89F39708B27CAC528FF32DD786569A11A5(open) $EE2D39A31F09EDD15B887B6EE7AB1396E52C3730(open) $A40E1C039224FA8072C7C84F729236FD738C69DA(open)
Nov 22 20:04:24.000 [info] connection_edge_process_relay_cell(): circuit_send_next_onion_skin() failed.
Nov 22 20:04:24.000 [warn] connection_edge_process_relay_cell (at origin) failed.
Nov 22 20:04:24.000 [warn] circuit_receive_relay_cell (backward) failed. Closing.

So that circuit above has a Guard (175921396C7C426309AB03775A9930B6F611F794) that was removed a minute or so before:

Nov 22 20:03:39.000 [info] sampled_guards_update_from_consensus(): Removing sampled guard lovejoy ($175921396C7C426309AB03775A9930B6F611F794): it was sampled over 120 days ago, and confirmed over 60 days ago.

Turns out that the circuit was cannibalized but tor made it failed because I assume our guard state wasn't available for that circuit which ultimately triggered those warnings. asn informed me that it is important that the circuit with old guard(s) stay alive for a while to help mitigate Guard discovery attacks.

Bottom line, I think our cannibalized function should exclude any circuit that doesn't match our guard state. In the meantime, those warnings will appear in the logs.

Child Tickets

Change History (6)

comment:1 Changed 2 years ago by dgoulet

Description: modified (diff)

comment:2 Changed 2 years ago by dgoulet

Owner: set to dgoulet
Status: newaccepted

comment:3 Changed 2 years ago by dgoulet

Status: acceptedneeds_review

Thanks to nickm, I've added a function in entrynodes.c to learn if a circuit guard state is likely to succeed.

As much as I would like a unit test here, there seems to be _no_ existing tests for cannibalizing circuits... Which is kind of worrying in some ways but also makes it very tedious for this bug to get fixed. Might be time for a ticket to get those and make it a big high prio? Or we think that patch just can't go in without unit tests?

Both circuit_launch_by_extend_info() and circuit_find_to_cannibalize() appears untested in our test suites.

See branch: bug24469_033_01

comment:4 Changed 2 years ago by nickm

lgtm; merging. Maybe open a new ticket about improving cannibalization tests?

comment:5 Changed 2 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

comment:6 in reply to:  4 Changed 2 years ago by dgoulet

Replying to nickm:

lgtm; merging. Maybe open a new ticket about improving cannibalization tests?

Yes! Done #25118

Note: See TracTickets for help on using tickets.