From e335f6c75aca2a39244390bd5f3c8ca66b2071d4 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 12 Jan 2022 15:46:36 -0500 Subject: [PATCH] Change RequireSendmeAuth to an enum. This is a fine example of why booleans are risky: it's far to easy to pass "animate:bool" into "inanimate:bool" like we did here. This is a followup from our fix to #294. --- crates/tor-proto/src/circuit.rs | 17 +++-- crates/tor-proto/src/circuit/reactor.rs | 85 ++++++++++++++++++------- 2 files changed, 69 insertions(+), 33 deletions(-) diff --git a/crates/tor-proto/src/circuit.rs b/crates/tor-proto/src/circuit.rs index 72d4c8bba..36e986576 100644 --- a/crates/tor-proto/src/circuit.rs +++ b/crates/tor-proto/src/circuit.rs @@ -74,6 +74,8 @@ use tor_cell::relaycell::StreamId; use crate::crypto::handshake::ntor::NtorPublicKey; +use self::reactor::RequireSendmeAuth; + /// The size of the buffer for communication between `ClientCirc` and its reactor. pub const CIRCUIT_BUFFER_SIZE: usize = 128; @@ -206,9 +208,7 @@ impl ClientCirc { linkspecs.retain(|ls| !matches!(ls, LinkSpec::Ed25519Id(_))); } // FlowCtrl=1 means that this hop supports authenticated SENDMEs - let supports_flowctrl_1 = target - .protovers() - .supports_known_subver(tor_protover::ProtoKind::FlowCtrl, 1); + let require_sendme_auth = RequireSendmeAuth::from_protocols(target.protovers()); let (tx, rx) = oneshot::channel(); @@ -216,7 +216,7 @@ impl ClientCirc { .unbounded_send(CtrlMsg::ExtendNtor { public_key: key, linkspecs, - supports_authenticated_sendme: supports_flowctrl_1, + require_sendme_auth, params: params.clone(), done: tx, }) @@ -476,7 +476,7 @@ impl PendingClientCirc { .unbounded_send(CtrlMsg::Create { recv_created: self.recvcreated, handshake: CircuitHandshake::CreateFast, - supports_authenticated_sendme: false, + require_sendme_auth: RequireSendmeAuth::No, params: params.clone(), done: tx, }) @@ -500,9 +500,8 @@ impl PendingClientCirc { Tg: tor_linkspec::CircTarget, { let (tx, rx) = oneshot::channel(); - let supports_flowctrl_1 = target - .protovers() - .supports_known_subver(tor_protover::ProtoKind::FlowCtrl, 1); + let require_sendme_auth = RequireSendmeAuth::from_protocols(target.protovers()); + self.circ .control .unbounded_send(CtrlMsg::Create { @@ -514,7 +513,7 @@ impl PendingClientCirc { }, ed_identity: *target.ed_identity(), }, - supports_authenticated_sendme: supports_flowctrl_1, + require_sendme_auth, params: params.clone(), done: tx, }) diff --git a/crates/tor-proto/src/circuit/reactor.rs b/crates/tor-proto/src/circuit/reactor.rs index 5ad4cab24..2d82b6041 100644 --- a/crates/tor-proto/src/circuit/reactor.rs +++ b/crates/tor-proto/src/circuit/reactor.rs @@ -78,7 +78,8 @@ pub(super) enum CtrlMsg { /// The handshake type to use for the first hop. handshake: CircuitHandshake, /// Whether the hop supports authenticated SENDME cells. - supports_authenticated_sendme: bool, + /// (And therefore, whether we should require them.) + require_sendme_auth: RequireSendmeAuth, /// Other parameters relevant for circuit creation. params: CircParameters, /// Oneshot channel to notify on completion. @@ -91,7 +92,8 @@ pub(super) enum CtrlMsg { /// Information about how to connect to the relay we're extending to. linkspecs: Vec, /// Whether the hop supports authenticated SENDME cells. - supports_authenticated_sendme: bool, + /// (And therefore, whether we should require them.) + require_sendme_auth: RequireSendmeAuth, /// Other parameters relevant for circuit extension. params: CircParameters, /// Oneshot channel to notify on completion. @@ -161,7 +163,7 @@ pub(super) struct CircHop { recvwindow: sendme::CircRecvWindow, /// If true, this hop is using an older link protocol and we /// shouldn't expect good authenticated SENDMEs from it. - auth_sendme_optional: bool, + auth_sendme_required: RequireSendmeAuth, /// Window used to say how many cells we can send. sendwindow: sendme::CircSendWindow, /// Buffer for messages we can't send to this hop yet due to congestion control. @@ -174,6 +176,34 @@ pub(super) struct CircHop { outbound: VecDeque<([u8; 20], ChanCell)>, } +/// Enumeration to determine whether we require circuit-level SENDME cells to be +/// authenticated. +/// +/// (This is an enumeration rather than a boolean to prevent accidental sense +/// inversion.) +#[derive(Copy, Clone, Debug, Eq, PartialEq)] +pub(super) enum RequireSendmeAuth { + /// Sendme authentication is expected from this hop, and therefore is + /// required. + Yes, + /// Sendme authentication is not expected from this hop, and therefore not + /// required. + No, +} + +impl RequireSendmeAuth { + /// Create an appropriate [`RequireSendmeAuth`] for a given set of relay + /// subprotocol versions. + pub(super) fn from_protocols(protocols: &tor_protover::Protocols) -> Self { + if protocols.supports_known_subver(tor_protover::ProtoKind::FlowCtrl, 1) { + // The relay supports FlowCtrl=1, and therefore will authenticate. + RequireSendmeAuth::Yes + } else { + RequireSendmeAuth::No + } + } +} + /// An indicator on what we should do when we receive a cell for a circuit. #[derive(Copy, Clone, Debug, Eq, PartialEq)] enum CellStatus { @@ -185,11 +215,11 @@ enum CellStatus { impl CircHop { /// Create a new hop. - pub(super) fn new(auth_sendme_optional: bool, initial_window: u16) -> Self { + pub(super) fn new(auth_sendme_required: RequireSendmeAuth, initial_window: u16) -> Self { CircHop { map: streammap::StreamMap::new(), recvwindow: sendme::CircRecvWindow::new(1000), - auth_sendme_optional, + auth_sendme_required, sendwindow: sendme::CircSendWindow::new(initial_window), outbound: VecDeque::new(), } @@ -227,7 +257,8 @@ where /// Handshake state. state: Option, /// Whether the hop supports authenticated SENDME cells. - supports_flowctrl_1: bool, + /// (And therefore, whether we require them.) + require_sendme_auth: RequireSendmeAuth, /// Parameters used for this extension. params: CircParameters, /// An identifier for logging about this reactor's circuit. @@ -263,7 +294,7 @@ where handshake_id: u16, key: &H::KeyType, linkspecs: Vec, - supports_flowctrl_1: bool, + require_sendme_auth: RequireSendmeAuth, params: CircParameters, reactor: &mut Reactor, ) -> Result { @@ -297,7 +328,7 @@ where Ok(Self { state: Some(state), - supports_flowctrl_1, + require_sendme_auth, params, unique_id, expected_hop: hop, @@ -355,7 +386,7 @@ where // If we get here, it succeeded. Add a new hop to the circuit. let (layer_fwd, layer_back) = layer.split(); reactor.add_hop( - self.supports_flowctrl_1, + self.require_sendme_auth, Box::new(layer_fwd), Box::new(layer_back), &self.params, @@ -580,7 +611,7 @@ impl Reactor { if let Some(CtrlMsg::Create { recv_created, handshake, - supports_authenticated_sendme, + require_sendme_auth, params, done, }) = create_message @@ -597,7 +628,7 @@ impl Reactor { recv_created, ed_identity, public_key, - supports_authenticated_sendme, + require_sendme_auth, ¶ms, ) .await @@ -626,7 +657,7 @@ impl Reactor { recvcreated: oneshot::Receiver, wrap: &W, key: &H::KeyType, - supports_flowctrl_1: bool, + require_sendme_auth: RequireSendmeAuth, params: &CircParameters, ) -> Result<()> where @@ -668,7 +699,7 @@ impl Reactor { let (layer_fwd, layer_back) = layer.split(); self.add_hop( - supports_flowctrl_1, + require_sendme_auth, Box::new(layer_fwd), Box::new(layer_back), params, @@ -693,7 +724,7 @@ impl Reactor { recvcreated, &wrap, &(), - false, + RequireSendmeAuth::No, params, ) .await @@ -708,7 +739,7 @@ impl Reactor { recvcreated: oneshot::Receiver, ed_identity: pk::ed25519::Ed25519Identity, pubkey: NtorPublicKey, - supports_flowctrl_1: bool, + require_sendme_auth: RequireSendmeAuth, params: &CircParameters, ) -> Result<()> { // Exit now if we have an Ed25519 or RSA identity mismatch. @@ -735,7 +766,7 @@ impl Reactor { recvcreated, &wrap, &pubkey, - supports_flowctrl_1, + require_sendme_auth, params, ) .await @@ -744,14 +775,13 @@ impl Reactor { /// Add a hop to the end of this circuit. fn add_hop( &mut self, - supports_flowctrl_1: bool, + require_sendme_auth: RequireSendmeAuth, fwd: Box, rev: Box, params: &CircParameters, ) { - let auth_sendme_optional = !supports_flowctrl_1; let hop = crate::circuit::reactor::CircHop::new( - auth_sendme_optional, + require_sendme_auth, params.initial_send_window(), ); self.hops.push(hop); @@ -835,7 +865,7 @@ impl Reactor { } } None => { - if !hop.auth_sendme_optional { + if hop.auth_sendme_required == RequireSendmeAuth::Yes { return Err(Error::CircProto("missing tag on circuit sendme".into())); } else { None @@ -985,7 +1015,7 @@ impl Reactor { CtrlMsg::ExtendNtor { public_key, linkspecs, - supports_authenticated_sendme, + require_sendme_auth, params, done, } => { @@ -994,7 +1024,7 @@ impl Reactor { 0x02, &public_key, linkspecs, - supports_authenticated_sendme, + require_sendme_auth, params, self, ) { @@ -1031,9 +1061,16 @@ impl Reactor { } => { use crate::circuit::test::DummyCrypto; + // This kinds of conversion is okay for testing, but just for testing. + let require_sendme_auth = if supports_flowctrl_1 { + RequireSendmeAuth::Yes + } else { + RequireSendmeAuth::No + }; + let fwd = Box::new(DummyCrypto::new(fwd_lasthop)); let rev = Box::new(DummyCrypto::new(rev_lasthop)); - self.add_hop(supports_flowctrl_1, fwd, rev, ¶ms); + self.add_hop(require_sendme_auth, fwd, rev, ¶ms); let _ = done.send(Ok(())); } #[cfg(test)] @@ -1158,7 +1195,7 @@ impl Reactor { // If we do need to send a circuit-level SENDME cell, do so. if send_circ_sendme { // This always sends a V1 (tagged) sendme cell, and thereby assumes - // that SendmeEmitMinVersion is at least 1. If the authorities + // that SendmeEmitMinVersion is no more than 1. If the authorities // every increase that parameter to a higher number, this will // become incorrect. (Higher numbers are not currently defined.) let sendme = Sendme::new_tag(tag);