diff options
Diffstat (limited to 'crates/tor-proto/src/channel/reactor.rs')
| -rw-r--r-- | crates/tor-proto/src/channel/reactor.rs | 72 |
1 files changed, 40 insertions, 32 deletions
diff --git a/crates/tor-proto/src/channel/reactor.rs b/crates/tor-proto/src/channel/reactor.rs index 6f38bdc62..2e8dd2837 100644 --- a/crates/tor-proto/src/channel/reactor.rs +++ b/crates/tor-proto/src/channel/reactor.rs @@ -633,10 +633,18 @@ impl<R: Runtime> Reactor<R> { return Err(Error::ChanProto("Relay cell without circuit ID".into())); }; - let mut ent = self - .circs - .get_mut(circid) - .ok_or_else(|| Error::ChanProto("Relay cell on nonexistent circuit".into()))?; + let Some(mut ent) = self.circs.get_mut(circid) else { + trace!(channel_id = %self, "Relay cell for nonexistent circuit {}", circid); + // Silently drop the RELAY cell, as per the spec: + // + // > When a node receives a RELAY or RELAY_EARLY cell, it checks the cell’s circID and + // > determines whether it has a corresponding circuit along that connection. + // > If not, the node drops the cell. + // + // See https://spec.torproject.org/tor-spec/routing-relay-cells.html#circuit-id-checks + drop(msg); + return Ok(()); + }; match &mut *ent { CircEnt::OpenOrigin { cell_sender: s, .. } => { @@ -746,7 +754,17 @@ impl<R: Runtime> Reactor<R> { return Err(Error::ChanProto("'Created' cell without circuit ID".into())); }; - let target = self.circs.advance_from_opening(circid)?; + let Some(target) = self.circs.advance_from_opening(circid) else { + trace!(channel_id = %self, "Unexpected CREATED* cell not on opening circuit {}", circid); + // Silently drop the cell: we can't easily distinguish cells with bogus CircIds + // from cells arriving on already-closed circuits, + // so we err on the side of keeping the channel open. + // + // See https://gitlab.torproject.org/tpo/core/arti/-/work_items/2655#note_3447841 + drop(msg); + return Ok(()); + }; + let created = msg.try_into()?; // TODO(nickm) I think that this one actually means the other side // is closed. See arti#269. @@ -809,7 +827,13 @@ impl<R: Runtime> Reactor<R> { // Got a DESTROY cell for a circuit we don't have. None => { trace!(channel_id = %self, "Destroy for nonexistent circuit {}", circid); - Err(Error::ChanProto("Destroy for nonexistent circuit".into())) + // Silently drop the cell: we can't easily distinguish cells with bogus CircIds + // from cells arriving on already-closed circuits, + // so we err on the side of keeping the channel open: + // + // See https://gitlab.torproject.org/tpo/core/arti/-/work_items/2655#note_3447841 + drop(msg); + Ok(()) } } } @@ -1103,30 +1127,22 @@ pub(crate) mod test { tor_rtcompat::test_with_all_runtimes!(|rt| async move { let (_chan, mut reactor, _output, mut input) = new_reactor(rt); - // shouldn't get created2 cells for nonexistent circuits + // Created2 cells for nonexistent circuits are dropped let created2_cell = msg::Created2::new(*b"hihi").into(); input .send(Ok(AnyChanCell::new(CircId::new(7), created2_cell))) .await .unwrap(); - let e = reactor.run_once().await.unwrap_err().unwrap_err(); - assert_eq!( - format!("{}", e), - "Channel protocol violation: Unexpected CREATED* cell not on opening circuit" - ); + reactor.run_once().await.unwrap(); - // Can't get a relay cell on a circuit we've never heard of. + // Relay cells on a circuit we've never heard of are dropped let relay_cell = msg::Relay::new(b"abc").into(); input .send(Ok(AnyChanCell::new(CircId::new(4), relay_cell))) .await .unwrap(); - let e = reactor.run_once().await.unwrap_err().unwrap_err(); - assert_eq!( - format!("{}", e), - "Channel protocol violation: Relay cell on nonexistent circuit" - ); + reactor.run_once().await.unwrap(); // There used to be tests here for other types, but now that we only // accept OpenClientChanCell, we know that the codec can't even try @@ -1193,16 +1209,12 @@ pub(crate) mod test { "Channel protocol violation: Relay cell on pending circuit before CREATED* received" ); - // If a relay cell is sent on a non-existent channel, that's an error. + // If a relay cell is sent on a non-existent circuit, it will be dropped. input .send(Ok(AnyChanCell::new(CircId::new(101), relaycell.clone()))) .await .unwrap(); - let e = reactor.run_once().await.unwrap_err().unwrap_err(); - assert_eq!( - format!("{}", e), - "Channel protocol violation: Relay cell on nonexistent circuit" - ); + reactor.run_once().await.unwrap(); // It's fine to get a relay cell on a DestroySent channel: that happens // when the other side hasn't noticed the Destroy yet. @@ -1293,16 +1305,12 @@ pub(crate) mod test { .unwrap(); reactor.run_once().await.unwrap(); - // Destroying a nonexistent circuit is an error. + // Destroying a nonexistent circuit is not an error (the DESTROY is dropped). input .send(Ok(AnyChanCell::new(CircId::new(101), destroycell.clone()))) .await .unwrap(); - let e = reactor.run_once().await.unwrap_err().unwrap_err(); - assert_eq!( - format!("{}", e), - "Channel protocol violation: Destroy for nonexistent circuit" - ); + reactor.run_once().await.unwrap(); }); } @@ -1344,10 +1352,10 @@ pub(crate) mod test { tor_rtcompat::test_with_all_runtimes!(|rt| async move { let (chan, reactor, _output, mut input) = new_reactor(rt); - // force an error by sending created2 cell for nonexistent circuit + // force an error by sending created2 cell without a CircId let created2_cell = msg::Created2::new(*b"hihi").into(); input - .send(Ok(AnyChanCell::new(CircId::new(7), created2_cell))) + .send(Ok(AnyChanCell::new(None, created2_cell))) .await .unwrap(); |