mirror of https://gitlab.torproject.org/tpo/core/arti http://git.dilluti0n.com/mirrors/arti.git/atom?h=main 2026-08-18T16:13:57Z 2026-08-18T16:13:57Z Gabriela Moldovan gabi@torproject.org 2026-08-11T15:51:46Z urn:sha1:43d07736928811fb36fa5e272f1f77b3653ebb28 This change prevents the channel reactor from sending DESTROY cells on already-closed (or non-existent) circuits. Upon receiving a DESTROY cell, the channel reactor removes the corresponding circuit entry, if any, from its circmap. It then passes the DESTROY to the circuit reactor for handling. The circuit reactor handles it by shutting down, and calling `Channel::close_circuit()` on drop. Previously, this would unconditionally send a DESTROY cell, which caused #2648 and #2646. This affects both clients and relays, because both circuit reactors call `Channel::close_circuit()` on drop. Closes #2648, #2646 2026-08-13T16:20:06Z gabi-250 gabi@torproject.org 2026-08-13T16:20:06Z urn:sha1:3f3cfef1791f52fe783553b119492678877b6aa7 proto: Silently drop DESTROY/RELAY/CREATED cells on unknown circuits Closes #2655 See merge request tpo/core/arti!4301 2026-08-12T14:35:07Z Steven Engler opara@torproject.org 2026-08-11T23:57:36Z urn:sha1:077a92b9e2f1ab5fd8caec277c5abbf1737882fd 2026-08-12T09:13:45Z Gabriela Moldovan gabi@torproject.org 2026-08-12T09:11:58Z urn:sha1:610e83d32e84a76b8c1faec6b962ac35ffa0db62 And say why it's okay to do so. 2026-08-11T09:47:38Z Gabriela Moldovan gabi@torproject.org 2026-08-10T18:24:02Z urn:sha1:03b177a5c82345dabd097d2afb9b267eedc49d20 These are no longer causing the channel reactor to shut down, so we need to update this test accordingly. 2026-08-11T09:47:38Z Gabriela Moldovan gabi@torproject.org 2026-08-10T18:04:04Z urn:sha1:2ce124af4453a59449daee5012069154ccbca5f8 2026-08-11T09:47:38Z Gabriela Moldovan gabi@torproject.org 2026-08-10T18:01:30Z urn:sha1:f943a331c212ed1d2ac5e8548a943a7903ff5c6f 2026-08-11T09:41:11Z Gabriela Moldovan gabi@torproject.org 2026-08-10T18:18:47Z urn:sha1:2f78e004ce8a83eda5cc6f145a00b3240051d69e If we're a relay, we need to tolerate CREATED* with unrecognized CircIds: for example, if we time out[^1] while trying to extend the circuit by another hop, we will send a DESTROY to the extending hop, which can race with the CREATED* response. In other words, a CREATED* cell arriving on a closed circuit shouldn't be treated as a protocol violation. There are, however, a few cases where a CREATED* with an unknown CircId *is* a protocol violation (and probably *should* cause us to close down the channel): * if the CREATED* is moving in the forward direction (towards the exit), or * if we have not previously sent a CREATE* with that particular CircId As before, distinguishing these from the "closed circuit" case above would involve some tricky logic, and the benefits are unclear, while the downsides of closing a channel when we shouldn't have are significant. It seems better to just drop these cells for now. Closes #2655 [^1]: at the time of writing, we don't have timeouts for the circuit extension logic, so what I've described here cannot actually happen today. However, we *do* have a TODO for it, so the time outs I've described here will be implemented at some point 2026-08-11T09:22:12Z Gabriela Moldovan gabi@torproject.org 2026-08-10T17:26:49Z urn:sha1:f4ad0e94ee01258962bf96d79bb3b4a17c6c71f6 2026-08-11T09:17:32Z Gabriela Moldovan gabi@torproject.org 2026-08-10T17:26:13Z urn:sha1:5f5617312fe0f1a4005505178255eb1119edb83d An unrecognized circuit ID is not always a protocol violation, so we shouldn't close down the channel if it happens. This change makes the channel reactor drop DESTROY and RELAY cells with unknown CircIds without closing down the channel. It affects both clients and relays. Instead of dropping these unconditionally, we could have implemented some more sophisticated checks to distinguish the bogus CircIds from the CircIds of closed circuits, but it's unclear if it's worth the added complexity (see discussion in #2655). This partly addresses #2646 (an unrecognized circuit ID shouldn't cause us to close down the channel if we're a relay). This commit partially undoes the changes from 4f567e4a9432b340c2799e600c8ceb3724ad3082, which was originally intended to mitigate flooding attacks. Part of #2655