diff options
| -rw-r--r-- | crates/tor-proto/src/channel/circmap.rs | 17 | ||||
| -rw-r--r-- | crates/tor-proto/src/channel/reactor.rs | 72 |
2 files changed, 49 insertions, 40 deletions
diff --git a/crates/tor-proto/src/channel/circmap.rs b/crates/tor-proto/src/channel/circmap.rs index 3da745261..496b8aee0 100644 --- a/crates/tor-proto/src/channel/circmap.rs +++ b/crates/tor-proto/src/channel/circmap.rs @@ -283,10 +283,13 @@ impl CircMap { /// See whether 'id' is an opening circuit. If so, mark it "open" and /// return a oneshot::Sender that is waiting for its create cell. + /// + /// Returns `None` if `id` is not in our circuit map, + /// or if it is not an opening circuit. pub(super) fn advance_from_opening( &mut self, id: CircId, - ) -> Result<oneshot::Sender<CreateResponse>> { + ) -> Option<oneshot::Sender<CreateResponse>> { // TODO: there should be a better way to do // this. hash_map::Entry seems like it could be better, but // there seems to be no way to replace the object in-place as @@ -306,14 +309,12 @@ impl CircMap { padding_ctrl, }, ); - Ok(oneshot) + Some(oneshot) } else { panic!("internal error: inconsistent circuit state"); } } else { - Err(Error::ChanProto( - "Unexpected CREATED* cell not on opening circuit".into(), - )) + None } } @@ -428,7 +429,7 @@ mod test { CircEnt::Opening { .. } )); let adv = map_high.advance_from_opening(ids_high[0]); - assert!(adv.is_ok()); + assert!(adv.is_some()); assert!(matches!( *map_high.get_mut(ids_high[0]).unwrap(), CircEnt::OpenOrigin { .. } @@ -436,13 +437,13 @@ mod test { // Can't double-advance. let adv = map_high.advance_from_opening(ids_high[0]); - assert!(adv.is_err()); + assert!(adv.is_none()); // Can't advance an entry that is not there. We know "77" // can't be in map_high, since we only added high circids to // it. let adv = map_high.advance_from_opening(CircId::new(77).unwrap()); - assert!(adv.is_err()); + assert!(adv.is_none()); }); } } 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(); |