vincenzopalazzo
/
arti
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge branch 'ticket_914' into 'main' 

Remove support for receiving unauthenticated SENDMEs.

Closes #914

See merge request tpo/core/arti!1283
This commit is contained in:
Nick Mathewson 2023-06-28 10:54:05 +00:00
parent eb54691c50 f4a4c11627
commit f778d32a0e
2 changed files with 9 additions and 118 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
crates/tor-proto/src/circuit.rs
View File

@ -81,9 +81,6 @@ use tor_cell::relaycell::StreamId;
// use std::time::Duration;
use crate::crypto::handshake::ntor::NtorPublicKey;
use self::reactor::RequireSendmeAuth;
pub use path::{Path, PathEntry};
/// The size of the buffer for communication between `ClientCirc` and its reactor.
@ -463,8 +460,6 @@ impl ClientCirc {
if !params.extend_by_ed25519_id() {
linkspecs.retain(|ls| ls.lstype() != LinkSpecType::ED25519ID);
}
// FlowCtrl=1 means that this hop supports authenticated SENDMEs
let require_sendme_auth = RequireSendmeAuth::from_protocols(target.protovers());
let (tx, rx) = oneshot::channel();
@ -474,7 +469,6 @@ impl ClientCirc {
peer_id,
public_key: key,
linkspecs,
require_sendme_auth,
params: params.clone(),
done: tx,
})
@ -789,7 +783,6 @@ impl PendingClientCirc {
.unbounded_send(CtrlMsg::Create {
recv_created: self.recvcreated,
handshake: CircuitHandshake::CreateFast,
require_sendme_auth: RequireSendmeAuth::No,
params: params.clone(),
done: tx,
})
@ -813,7 +806,6 @@ impl PendingClientCirc {
Tg: tor_linkspec::CircTarget,
{
let (tx, rx) = oneshot::channel();
let require_sendme_auth = RequireSendmeAuth::from_protocols(target.protovers());
self.circ
.control
@ -830,7 +822,6 @@ impl PendingClientCirc {
.ed_identity()
.ok_or(Error::MissingId(RelayIdType::Ed25519))?,
},
require_sendme_auth,
params: params.clone(),
done: tx,
})
@ -1180,7 +1171,6 @@ mod test {
let (tx, rx) = oneshot::channel();
circ.control
.unbounded_send(CtrlMsg::AddFakeHop {
supports_flowctrl_1: true,
fwd_lasthop: idx == 2,
rev_lasthop: idx == u8::from(next_msg_from),
params,

117
crates/tor-proto/src/circuit/reactor.rs
View File

@ -97,9 +97,6 @@ pub(super) enum CtrlMsg {
recv_created: oneshot::Receiver<CreateResponse>,
/// The handshake type to use for the first hop.
handshake: CircuitHandshake,
/// Whether the hop supports authenticated SENDME cells.
/// (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.
@ -115,9 +112,6 @@ pub(super) enum CtrlMsg {
public_key: NtorPublicKey,
/// Information about how to connect to the relay we're extending to.
linkspecs: Vec<EncodedLinkSpec>,
/// Whether the hop supports authenticated SENDME cells.
/// (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.
@ -189,7 +183,6 @@ pub(super) enum CtrlMsg {
/// (tests only) Add a hop to the list of hops on this circuit, with dummy cryptography.
#[cfg(test)]
AddFakeHop {
supports_flowctrl_1: bool,
fwd_lasthop: bool,
rev_lasthop: bool,
params: CircParameters,
@ -219,9 +212,6 @@ pub(super) struct CircHop {
map: streammap::StreamMap,
/// Window used to say how many cells we can receive.
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_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.
@ -238,42 +228,6 @@ pub(super) struct CircHop {
outbound: VecDeque<(bool, AnyRelayCell)>,
}
/// 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.
//
// TODO(nickm): At some point in the future, once there are no 0.3.5 relays
// on the Tor network, we can safely require authenticated SENDMEs from all
// relays.
//
// At that point, if we have a relay implementation in Rust, it should look
// at the network parameter `SendmeAcceptMinVersion` when deciding whether
// to require authenticated SENDMEs.
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 {
@ -285,11 +239,10 @@ enum CellStatus {
impl CircHop {
/// Create a new hop.
pub(super) fn new(auth_sendme_required: RequireSendmeAuth, initial_window: u16) -> Self {
pub(super) fn new(initial_window: u16) -> Self {
CircHop {
map: streammap::StreamMap::new(),
recvwindow: sendme::CircRecvWindow::new(1000),
auth_sendme_required,
sendwindow: sendme::CircSendWindow::new(initial_window),
outbound: VecDeque::new(),
}
@ -358,9 +311,6 @@ where
peer_id: OwnedChanTarget,
/// Handshake state.
state: Option<H::StateType>,
/// Whether the hop supports authenticated SENDME cells.
/// (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.
@ -400,7 +350,6 @@ where
handshake_id: u16,
key: &H::KeyType,
linkspecs: Vec<EncodedLinkSpec>,
require_sendme_auth: RequireSendmeAuth,
params: CircParameters,
reactor: &mut Reactor,
done: ReactorResultChannel<()>,
@ -437,7 +386,6 @@ where
Ok::<CircuitExtender<_, _, _, _>, Error>(Self {
peer_id,
state: Some(state),
require_sendme_auth,
params,
unique_id,
expected_hop: hop,
@ -492,7 +440,6 @@ where
let (layer_fwd, layer_back) = layer.split();
reactor.add_hop(
path::HopDetail::Relay(self.peer_id.clone()),
self.require_sendme_auth,
Box::new(layer_fwd),
Box::new(layer_back),
&self.params,
@ -793,7 +740,6 @@ impl Reactor {
if let Some(CtrlMsg::Create {
recv_created,
handshake,
require_sendme_auth,
params,
done,
}) = create_message
@ -806,14 +752,8 @@ impl Reactor {
public_key,
ed_identity,
} => {
self.create_firsthop_ntor(
recv_created,
ed_identity,
public_key,
require_sendme_auth,
&params,
)
.await
self.create_firsthop_ntor(recv_created, ed_identity, public_key, &params)
.await
}
};
let _ = done.send(ret); // don't care if sender goes away
@ -839,7 +779,6 @@ impl Reactor {
recvcreated: oneshot::Receiver<CreateResponse>,
wrap: &W,
key: &H::KeyType,
require_sendme_auth: RequireSendmeAuth,
params: &CircParameters,
) -> Result<()>
where
@ -884,7 +823,6 @@ impl Reactor {
self.add_hop(
path::HopDetail::Relay(peer_id),
require_sendme_auth,
Box::new(layer_fwd),
Box::new(layer_back),
params,
@ -909,7 +847,6 @@ impl Reactor {
recvcreated,
&wrap,
&(),
RequireSendmeAuth::No,
params,
)
.await
@ -924,7 +861,6 @@ impl Reactor {
recvcreated: oneshot::Receiver<CreateResponse>,
ed_identity: pk::ed25519::Ed25519Identity,
pubkey: NtorPublicKey,
require_sendme_auth: RequireSendmeAuth,
params: &CircParameters,
) -> Result<()> {
// Exit now if we have an Ed25519 or RSA identity mismatch.
@ -942,7 +878,6 @@ impl Reactor {
recvcreated,
&wrap,
&pubkey,
require_sendme_auth,
params,
)
.await
@ -952,15 +887,11 @@ impl Reactor {
fn add_hop(
&mut self,
peer_id: path::HopDetail,
require_sendme_auth: RequireSendmeAuth,
fwd: Box<dyn OutboundClientLayer + 'static + Send>,
rev: Box<dyn InboundClientLayer + 'static + Send>,
params: &CircParameters,
) {
let hop = crate::circuit::reactor::CircHop::new(
require_sendme_auth,
params.initial_send_window(),
);
let hop = crate::circuit::reactor::CircHop::new(params.initial_send_window());
self.hops.push(hop);
self.crypto_in.add_layer(rev);
self.crypto_out.add_layer(fwd);
@ -1069,11 +1000,9 @@ impl Reactor {
}
}
None => {
if hop.auth_sendme_required == RequireSendmeAuth::Yes {
return Err(Error::CircProto("missing tag on circuit sendme".into()));
} else {
None
}
// Versions of Tor <=0.3.5 would omit a SENDME tag in this case;
// but we don't support those any longer.
return Err(Error::CircProto("missing tag on circuit sendme".into()));
}
};
hop.sendwindow.put(auth)?;
@ -1237,7 +1166,6 @@ impl Reactor {
peer_id,
public_key,
linkspecs,
require_sendme_auth,
params,
done,
} => {
@ -1247,7 +1175,6 @@ impl Reactor {
0x02,
&public_key,
linkspecs,
require_sendme_auth,
params,
self,
done,
@ -1267,19 +1194,7 @@ impl Reactor {
// describe why the virtual hop was added, or something?
let peer_id = path::HopDetail::Virtual;
// NOTE: This is not 100% correct! In theory we probably
// should be looking at the sendme_auth_accept_min_version
// parameter, to see whether we can count on the other party to
// send us authenticated SENDMEs. See comments in
// RequireSendmeAuth::from_protocols.
//
// But "Yes" is completely safe, however, since Tor <=0.3.5 is
// emphatically unsupported. We don't need to add any machinery
// here to support "No", since unauthenticated SENDMEs are never
// coming back. See #914.
let require_sendme_auth = RequireSendmeAuth::Yes;
self.add_hop(peer_id, require_sendme_auth, outbound, inbound, &params);
self.add_hop(peer_id, outbound, inbound, &params);
let _ = done.send(Ok(()));
}
CtrlMsg::BeginStream {
@ -1314,7 +1229,6 @@ impl Reactor {
}
#[cfg(test)]
CtrlMsg::AddFakeHop {
supports_flowctrl_1,
fwd_lasthop,
rev_lasthop,
params,
@ -1322,13 +1236,6 @@ 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 dummy_peer_id = OwnedChanTarget::builder()
.ed_identity([4; 32].into())
.rsa_identity([5; 20].into())
@ -1337,13 +1244,7 @@ impl Reactor {
let fwd = Box::new(DummyCrypto::new(fwd_lasthop));
let rev = Box::new(DummyCrypto::new(rev_lasthop));
self.add_hop(
path::HopDetail::Relay(dummy_peer_id),
require_sendme_auth,
fwd,
rev,
&params,
);
self.add_hop(path::HopDetail::Relay(dummy_peer_id), fwd, rev, &params);
let _ = done.send(Ok(()));
}
#[cfg(test)]