From 4047236bd9554b92914067843666719da1cfc08e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 3 Aug 2023 16:12:56 -0400 Subject: [PATCH 1/9] Wrap a long line in hscrypto/Cargo.toml. --- crates/tor-hscrypto/Cargo.toml | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/crates/tor-hscrypto/Cargo.toml b/crates/tor-hscrypto/Cargo.toml index 5daab5cba..42cd803c1 100644 --- a/crates/tor-hscrypto/Cargo.toml +++ b/crates/tor-hscrypto/Cargo.toml @@ -13,7 +13,14 @@ repository = "https://gitlab.torproject.org/tpo/core/arti.git/" [features] default = [] -full = ["safelog/full", "tor-basic-utils/full", "tor-bytes/full", "tor-llcrypto/full", "tor-units/full", "tor-error/full"] +full = [ + "safelog/full", + "tor-basic-utils/full", + "tor-bytes/full", + "tor-llcrypto/full", + "tor-units/full", + "tor-error/full", +] [dependencies] data-encoding = "2.3.1" # want MSVC i686 build fix, data-encoding/issues/33 From e7f803529f966a339d244047d9029687df209d49 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 3 Aug 2023 16:16:37 -0400 Subject: [PATCH 2/9] llcrypto: New SimpleMac trait This will be useful in preference to the regular Mac trait for the places where we need to pass a Mac key around, but we don't need to support incremental operation. Part of arti#993, where we want to expose a MAC object without exposing sensitive data. --- crates/tor-llcrypto/semver.md | 1 + crates/tor-llcrypto/src/lib.rs | 2 ++ crates/tor-llcrypto/src/traits.rs | 17 +++++++++++++++++ 3 files changed, 20 insertions(+) create mode 100644 crates/tor-llcrypto/semver.md create mode 100644 crates/tor-llcrypto/src/traits.rs diff --git a/crates/tor-llcrypto/semver.md b/crates/tor-llcrypto/semver.md new file mode 100644 index 000000000..2a2cfd6da --- /dev/null +++ b/crates/tor-llcrypto/semver.md @@ -0,0 +1 @@ +ADDED: traits module. diff --git a/crates/tor-llcrypto/src/lib.rs b/crates/tor-llcrypto/src/lib.rs index 0632b7d6d..eca695c09 100644 --- a/crates/tor-llcrypto/src/lib.rs +++ b/crates/tor-llcrypto/src/lib.rs @@ -43,4 +43,6 @@ pub mod cipher; pub mod d; pub mod pk; +#[cfg(feature = "hsv3-service")] // TODO HSS: Remove this feature gate +pub mod traits; pub mod util; diff --git a/crates/tor-llcrypto/src/traits.rs b/crates/tor-llcrypto/src/traits.rs new file mode 100644 index 000000000..7834bafeb --- /dev/null +++ b/crates/tor-llcrypto/src/traits.rs @@ -0,0 +1,17 @@ +//! Cryptographic traits for general use throughout Arti. + +use subtle::Choice; + +/// A simple trait to describe a keyed message authentication code. +/// +/// Unlike RustCrypto's +/// [`crypto_mac::Mac`](https://docs.rs/crypto-mac/latest/crypto_mac/trait.Mac.html), +/// this trait does not support incremental processing. +pub trait ShortMac { + /// Calculate a message authentication code for `input` using this key. + fn mac(&self, input: &[u8]) -> crate::util::ct::CtByteArray; + + /// Check whether `mac` is a valid message authentication code for `input` + /// using this key. + fn validate(&self, input: &[u8], mac: &[u8; MAC_LEN]) -> Choice; +} From 0759fdf681be3c3c95451db1fb6acde350b9650d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 3 Aug 2023 16:20:36 -0400 Subject: [PATCH 3/9] hscrypto: Expose hs_mac as a SimpleMac. --- Cargo.lock | 1 + crates/tor-hscrypto/Cargo.toml | 1 + crates/tor-hscrypto/semver.md | 1 + crates/tor-hscrypto/src/ops.rs | 17 ++++++++++++++++- 4 files changed, 19 insertions(+), 1 deletion(-) create mode 100644 crates/tor-hscrypto/semver.md diff --git a/Cargo.lock b/Cargo.lock index 3fd5ac300..1ca351d02 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4653,6 +4653,7 @@ dependencies = [ "safelog", "serde", "signature 1.6.4", + "subtle", "thiserror", "tor-basic-utils", "tor-bytes", diff --git a/crates/tor-hscrypto/Cargo.toml b/crates/tor-hscrypto/Cargo.toml index 42cd803c1..96bab2851 100644 --- a/crates/tor-hscrypto/Cargo.toml +++ b/crates/tor-hscrypto/Cargo.toml @@ -33,6 +33,7 @@ rand_core = "0.6.2" safelog = { path = "../safelog", version = "0.3.2" } serde = { version = "1.0.103", features = ["derive"] } signature = "1" +subtle = "2" thiserror = "1" tor-basic-utils = { path = "../tor-basic-utils", version = "0.7.3" } tor-bytes = { version = "0.7.2", path = "../tor-bytes" } diff --git a/crates/tor-hscrypto/semver.md b/crates/tor-hscrypto/semver.md new file mode 100644 index 000000000..dbe6521f3 --- /dev/null +++ b/crates/tor-hscrypto/semver.md @@ -0,0 +1 @@ +ADDED: HsMacKey type. diff --git a/crates/tor-hscrypto/src/ops.rs b/crates/tor-hscrypto/src/ops.rs index cad95c5ba..dcda9329a 100644 --- a/crates/tor-hscrypto/src/ops.rs +++ b/crates/tor-hscrypto/src/ops.rs @@ -1,5 +1,4 @@ //! Mid-level cryptographic operations used in the onion service protocol. - use tor_llcrypto::d::Sha3_256; use tor_llcrypto::util::ct::CtByteArray; @@ -27,6 +26,22 @@ pub fn hs_mac(key: &[u8], msg: &[u8]) -> CtByteArray { a.into() } +/// Reference to a slice of bytes usable to compute the [`hs_mac`] function. +#[derive(Copy, Clone, derive_more::From)] +pub struct HsMacKey<'a>(&'a [u8]); + +impl<'a> tor_llcrypto::traits::ShortMac for HsMacKey<'a> { + fn mac(&self, input: &[u8]) -> CtByteArray { + hs_mac(self.0, input) + } + + fn validate(&self, input: &[u8], mac: &[u8; HS_MAC_LEN]) -> subtle::Choice { + use subtle::ConstantTimeEq; + let m = hs_mac(self.0, input); + m.as_ref().ct_eq(mac) + } +} + #[cfg(test)] mod test { // @@ begin test lint list maintained by maint/add_warning @@ From 926cc65a9e8f465fabefc0cd9e1bc5e69e88b215 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 3 Aug 2023 16:32:01 -0400 Subject: [PATCH 4/9] cell: make establish_intro accept impl> This allows us to allow passing in opaque HsMacKey objects, rather than untyped byte slices. Additionally, we now check both MAC and signature unconditionally, to avoid the large timing side-channel. The small timing side-channel of combining booleans with `&` is considered safe. Part of #993. --- crates/tor-cell/semver.md | 1 + crates/tor-cell/src/relaycell/hs/est_intro.rs | 29 +++++++++---------- 2 files changed, 15 insertions(+), 15 deletions(-) create mode 100644 crates/tor-cell/semver.md diff --git a/crates/tor-cell/semver.md b/crates/tor-cell/semver.md new file mode 100644 index 000000000..c8d977be2 --- /dev/null +++ b/crates/tor-cell/semver.md @@ -0,0 +1 @@ +ADDED: establish_intro functions now take any impl>. diff --git a/crates/tor-cell/src/relaycell/hs/est_intro.rs b/crates/tor-cell/src/relaycell/hs/est_intro.rs index 2c662f29b..8a460d57a 100644 --- a/crates/tor-cell/src/relaycell/hs/est_intro.rs +++ b/crates/tor-cell/src/relaycell/hs/est_intro.rs @@ -3,9 +3,10 @@ use caret::caret_int; use tor_bytes::{EncodeError, EncodeResult, Readable, Reader, Result, Writeable, Writer}; use tor_error::bad_api_usage; -use tor_hscrypto::ops::{hs_mac, HS_MAC_LEN}; +use tor_hscrypto::ops::{HsMacKey, HS_MAC_LEN}; use tor_llcrypto::{ pk::ed25519::{self, Ed25519Identity, ED25519_SIGNATURE_LEN}, + traits::ShortMac as _, util::ct::CtByteArray, }; use tor_units::BoundedInt32; @@ -279,10 +280,10 @@ impl EstablishIntroDetails { /// The MAC key is derived from the circuit handshake between the onion /// service and the introduction point. The Ed25519 keypair must match the /// one given as the auth_key for this body. - pub fn sign_and_encode( + pub fn sign_and_encode<'a>( self, keypair: &ed25519::Keypair, - mac_key: &[u8], + mac_key: impl Into>, ) -> crate::Result> { use tor_llcrypto::pk::ed25519::Signer; if Ed25519Identity::from(&keypair.public) != self.auth_key { @@ -292,7 +293,8 @@ impl EstablishIntroDetails { let mut output = Vec::new(); output.write(&self)?; - let mac = hs_mac(mac_key, &output[..]); + let mac_key: HsMacKey<'_> = mac_key.into(); + let mac = mac_key.mac(&output[..]); output.write(&mac)?; let signature = { let mut signed_material = Vec::from(SIG_PREFIX); @@ -346,20 +348,17 @@ impl EstablishIntro { /// /// On success, return the [`EstablishIntroDetails`] describing how to function /// as an introduction point for this service. On failure, return an error. - pub fn check_and_unwrap( + pub fn check_and_unwrap<'a>( self, - mac_key: &[u8], + mac_key: impl Into>, ) -> std::result::Result { use tor_llcrypto::pk::ValidatableSignature; - // There is a timing side-channel here where, if an attacker wants, they - // could tell which of the two fields was incorrect. But that shouldn't - // be exploitable for anything. - // - // TODO use subtle here anyway, perhaps? - if hs_mac(mac_key, &self.mac_plaintext) != self.handshake_auth { - return Err(EstablishIntroSigError::Invalid); - } - if !self.sig.is_valid() { + + let mac_key: HsMacKey<'_> = mac_key.into(); + let mac_okay = mac_key.validate(&self.mac_plaintext, &self.handshake_auth); + let sig_okay = self.sig.is_valid(); + + if !(bool::from(mac_okay) & sig_okay) { return Err(EstablishIntroSigError::Invalid); } From 0ffa6eddf5c6ad6542102a4d28b287e744a38aeb Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 3 Aug 2023 17:15:51 -0400 Subject: [PATCH 5/9] proto: Add (not-yet-exposed) code to remember and use KH values These values are computed as part of the circuit extension handshake, and are used as MAC keys to bind `ESTABLISH_INTRO` messages to a particular circuit so that they can't be replayed. Part of #993. --- crates/tor-proto/src/circuit/handshake.rs | 2 +- crates/tor-proto/src/circuit/reactor.rs | 4 +- crates/tor-proto/src/crypto.rs | 1 + crates/tor-proto/src/crypto/binding.rs | 53 +++++++++++++++++++++++ crates/tor-proto/src/crypto/cell.rs | 38 ++++++++++------ 5 files changed, 82 insertions(+), 16 deletions(-) create mode 100644 crates/tor-proto/src/crypto/binding.rs diff --git a/crates/tor-proto/src/circuit/handshake.rs b/crates/tor-proto/src/circuit/handshake.rs index 5d1905884..d13b84718 100644 --- a/crates/tor-proto/src/circuit/handshake.rs +++ b/crates/tor-proto/src/circuit/handshake.rs @@ -57,7 +57,7 @@ impl RelayProtocol { let seed_needed = Tor1Hsv3RelayCrypto::seed_len(); let seed = keygen.expand(seed_needed)?; let layer = Tor1Hsv3RelayCrypto::initialize(&seed)?; - let (fwd, back) = layer.split(); + let (fwd, back, _) = layer.split(); let (fwd, back) = match role { HandshakeRole::Initiator => (fwd, back), HandshakeRole::Responder => (back, fwd), diff --git a/crates/tor-proto/src/circuit/reactor.rs b/crates/tor-proto/src/circuit/reactor.rs index f3dd5af6c..cb2a8e238 100644 --- a/crates/tor-proto/src/circuit/reactor.rs +++ b/crates/tor-proto/src/circuit/reactor.rs @@ -490,7 +490,7 @@ where debug!("{}: Handshake complete; circuit extended.", self.unique_id); // If we get here, it succeeded. Add a new hop to the circuit. - let (layer_fwd, layer_back) = layer.split(); + let (layer_fwd, layer_back, _) = layer.split(); reactor.add_hop( path::HopDetail::Relay(self.peer_id.clone()), Box::new(layer_fwd), @@ -991,7 +991,7 @@ impl Reactor { debug!("{}: Handshake complete; circuit created.", self.unique_id); - let (layer_fwd, layer_back) = layer.split(); + let (layer_fwd, layer_back, _) = layer.split(); let peer_id = self.channel.target().clone(); self.add_hop( diff --git a/crates/tor-proto/src/crypto.rs b/crates/tor-proto/src/crypto.rs index 60b545059..6fd4e1da2 100644 --- a/crates/tor-proto/src/crypto.rs +++ b/crates/tor-proto/src/crypto.rs @@ -6,6 +6,7 @@ //! * `handshake` implements the ntor handshake. //! * `ll` provides building blocks for other parts of the protocol. +mod binding; pub(crate) mod cell; pub(crate) mod handshake; pub(crate) mod ll; diff --git a/crates/tor-proto/src/crypto/binding.rs b/crates/tor-proto/src/crypto/binding.rs new file mode 100644 index 000000000..8275d5ebe --- /dev/null +++ b/crates/tor-proto/src/crypto/binding.rs @@ -0,0 +1,53 @@ +//! Types related to binding messages to specific circuits + +#![allow(dead_code, unreachable_pub)] // XXXX remove. + +use tor_hscrypto::ops::HsMacKey; +use zeroize::Zeroizing; + +/// Number of bytes of circuit binding material negotiated per circuit hop. +pub(crate) const CIRC_BINDING_LEN: usize = 20; + +/// Cryptographic information used to bind a message to a specific circuit. +/// +/// This information is used in some of our protocols (currently only the onion +/// services protocol) to prove that a given message was referring to a specific +/// hop on a specific circuit, and was not replayed from another circuit. +/// +/// In `tor-spec` and `rend-spec`, this value is called `KH`. +#[derive(Clone)] +pub struct CircuitBinding( + // We use a Box here to avoid moves that would bypass the zeroize-on-drop + // semantics. + // + // (This is not super-critical, since the impact of leaking one of these + // keys is slight, but it's best not to leak them at all.) + Box>, +); + +impl From<[u8; CIRC_BINDING_LEN]> for CircuitBinding { + fn from(value: [u8; CIRC_BINDING_LEN]) -> Self { + Self(Box::new(Zeroizing::new(value))) + } +} + +impl CircuitBinding { + /// Return a view of this key suitable for computing the MAC function used + /// to authenticate onion services' ESTABLISH_INTRODUCE messages. + /// + /// Note that this is not a general-purpose MAC; please avoid adding new + /// users of it. See notes on [`hs_mac`](tor_hscrypto::ops::hs_mac) for + /// more information. + #[cfg(feature = "hs-service")] + pub fn hs_mac(&self) -> HsMacKey<'_> { + HsMacKey::from(self.dangerously_into_bytes()) + } + + /// Return a view of this key as a byte-slice. + /// + /// This is potentially dangerous, since we don't want to expose this + /// information: We only want to use it as a MAC key. + fn dangerously_into_bytes(&self) -> &[u8] { + &(**self.0)[..] + } +} diff --git a/crates/tor-proto/src/crypto/cell.rs b/crates/tor-proto/src/crypto/cell.rs index 710f62788..4fff94e25 100644 --- a/crates/tor-proto/src/crypto/cell.rs +++ b/crates/tor-proto/src/crypto/cell.rs @@ -36,6 +36,8 @@ use tor_error::internal; use generic_array::GenericArray; +use super::binding::CircuitBinding; + /// Type for the body of a relay cell. #[derive(Clone, derive_more::From, derive_more::Into)] pub(crate) struct RelayCellBody(BoxedCellBody); @@ -75,8 +77,8 @@ where B: InboundClientLayer, { /// Consume this ClientLayer and return a paired forward and reverse - /// crypto layer. - fn split(self) -> (F, B); + /// crypto layer, and a [`CircuitBinding`] object + fn split(self) -> (F, B, CircuitBinding); } /// Represents a relay's view of the crypto state on a given circuit. @@ -259,6 +261,8 @@ pub(crate) type Tor1Hsv3RelayCrypto = /// I am calling this design `tor1`; it does not have a generally recognized /// name. pub(crate) mod tor1 { + use crate::crypto::binding::CIRC_BINDING_LEN; + use super::*; use cipher::{KeyIvInit, StreamCipher}; use digest::Digest; @@ -298,11 +302,13 @@ pub(crate) mod tor1 { fwd: CryptState, /// State for en/decrypting cells sent towards the client. back: CryptState, + /// A circuit binding key. + binding: CircuitBinding, } impl CryptInit for CryptStatePair { fn seed_len() -> usize { - SC::KeySize::to_usize() * 2 + D::OutputSize::to_usize() * 2 + SC::KeySize::to_usize() * 2 + D::OutputSize::to_usize() * 2 + CIRC_BINDING_LEN } fn initialize(seed: &[u8]) -> Result { // This corresponds to the use of the KDF algorithm as described in @@ -319,6 +325,10 @@ pub(crate) mod tor1 { let bdinit = &seed[dlen..dlen * 2]; // This is Db in the spec. let fckey = &seed[dlen * 2..dlen * 2 + keylen]; // This is Kf in the spec. let bckey = &seed[dlen * 2 + keylen..dlen * 2 + keylen * 2]; // this is Kb in the spec. + let binding_key: &[u8; CIRC_BINDING_LEN] = &seed + [dlen * 2 + keylen * 2..dlen * 2 + keylen * 2 + CIRC_BINDING_LEN] + .try_into() + .expect("Unable to convert a 20-byte slice to a 20-byte array!?"); let fwd = CryptState { cipher: SC::new(fckey.try_into().expect("Wrong length"), &Default::default()), digest: D::new().chain_update(fdinit), @@ -329,7 +339,8 @@ pub(crate) mod tor1 { digest: D::new().chain_update(bdinit), last_digest_val: GenericArray::default(), }; - Ok(CryptStatePair { fwd, back }) + let binding = CircuitBinding::from(*binding_key); + Ok(CryptStatePair { fwd, back, binding }) } } @@ -338,8 +349,8 @@ pub(crate) mod tor1 { SC: StreamCipher, D: Digest + Clone, { - fn split(self) -> (CryptState, CryptState) { - (self.fwd, self.back) + fn split(self) -> (CryptState, CryptState, CircuitBinding) { + (self.fwd, self.back, self.binding) } } @@ -484,7 +495,7 @@ mod test { cc_in: &mut InboundClientCrypt, pair: Tor1RelayCrypto, ) { - let (outbound, inbound) = pair.split(); + let (outbound, inbound, _) = pair.split(); cc_out.add_layer(Box::new(outbound)); cc_in.add_layer(Box::new(inbound)); } @@ -573,12 +584,13 @@ mod test { use digest::XofReader; use digest::{ExtendableOutput, Update}; - const K1: &[u8; 72] = - b" 'My public key is in this signed x509 object', said Tom assertively."; - const K2: &[u8; 72] = - b"'Let's chart the pedal phlanges in the tomb', said Tom cryptographically"; - const K3: &[u8; 72] = - b" 'Segmentation fault bugs don't _just happen_', said Tom seethingly."; + // (The ....s at the end here are the KH ca) + const K1: &[u8; 92] = + b" 'My public key is in this signed x509 object', said Tom assertively. (N-PREG-VIRYL)"; + const K2: &[u8; 92] = + b"'Let's chart the pedal phlanges in the tomb', said Tom cryptographically. (PELCG-GBR-TENCU)"; + const K3: &[u8; 92] = + b" 'Segmentation fault bugs don't _just happen_', said Tom seethingly. (P-GUVAT-YL)"; const SEED: &[u8;108] = b"'You mean to tell me that there's a version of Sha-3 with no limit on the output length?', said Tom shakily."; From 61513de6d0f0c8878431d91bc3c95a23b3f084e8 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 4 Aug 2023 08:17:49 -0400 Subject: [PATCH 6/9] proto: Take CircuitBinding one step forward into Reactor::add_hop. --- crates/tor-proto/src/circuit.rs | 4 ++-- crates/tor-proto/src/circuit/handshake.rs | 7 +++++-- crates/tor-proto/src/circuit/reactor.rs | 23 ++++++++++++++++++----- crates/tor-proto/src/crypto.rs | 2 +- 4 files changed, 26 insertions(+), 10 deletions(-) diff --git a/crates/tor-proto/src/circuit.rs b/crates/tor-proto/src/circuit.rs index 7b647ea2f..b673e6f4f 100644 --- a/crates/tor-proto/src/circuit.rs +++ b/crates/tor-proto/src/circuit.rs @@ -591,11 +591,11 @@ impl ClientCirc { seed: impl handshake::KeyGenerator, params: CircParameters, ) -> Result<()> { - let (outbound, inbound) = protocol.construct_layers(role, seed)?; + let (outbound, inbound, binding) = protocol.construct_layers(role, seed)?; let (tx, rx) = oneshot::channel(); let message = CtrlMsg::ExtendVirtual { - cell_crypto: (outbound, inbound), + cell_crypto: (outbound, inbound, binding), params, done: tx, }; diff --git a/crates/tor-proto/src/circuit/handshake.rs b/crates/tor-proto/src/circuit/handshake.rs index d13b84718..f8803a648 100644 --- a/crates/tor-proto/src/circuit/handshake.rs +++ b/crates/tor-proto/src/circuit/handshake.rs @@ -11,6 +11,7 @@ // that can wait IMO until we have a second circuit creation mechanism for use // with onion services. +use crate::crypto::binding::CircuitBinding; use crate::crypto::cell::{ ClientLayer, CryptInit, InboundClientLayer, OutboundClientLayer, Tor1Hsv3RelayCrypto, }; @@ -44,6 +45,7 @@ pub enum HandshakeRole { impl RelayProtocol { /// Construct the cell-crypto layers that are needed for a given set of /// circuit hop parameters. + #[allow(clippy::type_complexity)] // XXXX pub(crate) fn construct_layers( self, role: HandshakeRole, @@ -51,18 +53,19 @@ impl RelayProtocol { ) -> Result<( Box, Box, + Option, )> { match self { RelayProtocol::HsV3 => { let seed_needed = Tor1Hsv3RelayCrypto::seed_len(); let seed = keygen.expand(seed_needed)?; let layer = Tor1Hsv3RelayCrypto::initialize(&seed)?; - let (fwd, back, _) = layer.split(); + let (fwd, back, binding) = layer.split(); let (fwd, back) = match role { HandshakeRole::Initiator => (fwd, back), HandshakeRole::Responder => (back, fwd), }; - Ok((Box::new(fwd), Box::new(back))) + Ok((Box::new(fwd), Box::new(back), Some(binding))) } } } diff --git a/crates/tor-proto/src/circuit/reactor.rs b/crates/tor-proto/src/circuit/reactor.rs index cb2a8e238..484bd07fa 100644 --- a/crates/tor-proto/src/circuit/reactor.rs +++ b/crates/tor-proto/src/circuit/reactor.rs @@ -22,6 +22,7 @@ use crate::circuit::unique_id::UniqId; use crate::circuit::{ sendme, streammap, CircParameters, Create2Wrap, CreateFastWrap, CreateHandshakeWrap, }; +use crate::crypto::binding::CircuitBinding; use crate::crypto::cell::{ ClientLayer, CryptInit, HopNum, InboundClientCrypt, InboundClientLayer, OutboundClientCrypt, OutboundClientLayer, RelayCellBody, Tor1RelayCrypto, @@ -134,6 +135,7 @@ pub(super) enum CtrlMsg { cell_crypto: ( Box, Box, + Option, ), /// A set of parameters used to configure this hop. params: CircParameters, @@ -490,11 +492,12 @@ where debug!("{}: Handshake complete; circuit extended.", self.unique_id); // If we get here, it succeeded. Add a new hop to the circuit. - let (layer_fwd, layer_back, _) = layer.split(); + let (layer_fwd, layer_back, binding) = layer.split(); reactor.add_hop( path::HopDetail::Relay(self.peer_id.clone()), Box::new(layer_fwd), Box::new(layer_back), + Some(binding), &self.params, ); Ok(MetaCellDisposition::ConversationFinished) @@ -937,7 +940,14 @@ 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), fwd, rev, params); + let binding = None; + self.add_hop( + path::HopDetail::Relay(dummy_peer_id), + fwd, + rev, + binding, + params, + ); let _ = done.send(Ok(())); } @@ -991,13 +1001,14 @@ impl Reactor { debug!("{}: Handshake complete; circuit created.", self.unique_id); - let (layer_fwd, layer_back, _) = layer.split(); + let (layer_fwd, layer_back, binding) = layer.split(); let peer_id = self.channel.target().clone(); self.add_hop( path::HopDetail::Relay(peer_id), Box::new(layer_fwd), Box::new(layer_back), + Some(binding), params, ); Ok(()) @@ -1062,12 +1073,14 @@ impl Reactor { peer_id: path::HopDetail, fwd: Box, rev: Box, + binding: Option, params: &CircParameters, ) { 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); + drop(binding); // XXXX let mut mutable = self.mutable.lock().expect("poisoned lock"); Arc::make_mut(&mut mutable.path).push_hop(peer_id); } @@ -1382,13 +1395,13 @@ impl Reactor { params, done, } => { - let (outbound, inbound) = cell_crypto; + let (outbound, inbound, binding) = cell_crypto; // TODO HS: Perhaps this should describe the onion service, or // describe why the virtual hop was added, or something? let peer_id = path::HopDetail::Virtual; - self.add_hop(peer_id, outbound, inbound, ¶ms); + self.add_hop(peer_id, outbound, inbound, binding, ¶ms); let _ = done.send(Ok(())); } CtrlMsg::BeginStream { diff --git a/crates/tor-proto/src/crypto.rs b/crates/tor-proto/src/crypto.rs index 6fd4e1da2..29ba52d17 100644 --- a/crates/tor-proto/src/crypto.rs +++ b/crates/tor-proto/src/crypto.rs @@ -6,7 +6,7 @@ //! * `handshake` implements the ntor handshake. //! * `ll` provides building blocks for other parts of the protocol. -mod binding; +pub(crate) mod binding; pub(crate) mod cell; pub(crate) mod handshake; pub(crate) mod ll; From 65a0ac5512f7a147e58c64faacc9cb87cb0e4ecf Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 4 Aug 2023 08:51:26 -0400 Subject: [PATCH 7/9] proto: API to expose the `CircuitBinding` type. Closes #993 --- crates/tor-proto/semver.md | 3 +++ crates/tor-proto/src/circuit.rs | 34 ++++++++++++++++++++++++- crates/tor-proto/src/circuit/reactor.rs | 5 ++-- crates/tor-proto/src/crypto/binding.rs | 3 +-- 4 files changed, 40 insertions(+), 5 deletions(-) diff --git a/crates/tor-proto/semver.md b/crates/tor-proto/semver.md index 2ca6fcd9f..0447b2448 100644 --- a/crates/tor-proto/semver.md +++ b/crates/tor-proto/semver.md @@ -14,3 +14,6 @@ ADDED: `ClientCirc::start_conversation()` to eventually replace BREAKING: `ClientCirc::allow_stream_requests` is now async BREAKING: `IncomingStream::discard` now takes `mut self` instead of `self` and returns a `Result<(), Bug>` +ADDED: `ClientCirc::binding_key` + + diff --git a/crates/tor-proto/src/circuit.rs b/crates/tor-proto/src/circuit.rs index b673e6f4f..f30964055 100644 --- a/crates/tor-proto/src/circuit.rs +++ b/crates/tor-proto/src/circuit.rs @@ -57,12 +57,14 @@ use crate::circuit::reactor::{ CircuitHandshake, CtrlMsg, Reactor, RECV_WINDOW_INIT, STREAM_READER_BUFFER, }; pub use crate::circuit::unique_id::UniqId; +pub use crate::crypto::binding::CircuitBinding; use crate::crypto::cell::HopNum; use crate::stream::{ AnyCmdChecker, DataCmdChecker, DataStream, ResolveCmdChecker, ResolveStream, StreamParameters, StreamReader, }; use crate::{Error, ResolveError, Result}; +use educe::Educe; use tor_cell::{ chancell::{self, msg::AnyChanMsg, CircId}, relaycell::msg::{AnyRelayMsg, Begin, Resolve, Resolved, ResolvedVal}, @@ -171,7 +173,8 @@ pub struct ClientCirc { } /// Mutable state shared by [`ClientCirc`] and [`Reactor`]. -#[derive(Debug)] +#[derive(Educe)] +#[educe(Debug)] struct MutableState { /// Information about this circuit's path. /// @@ -179,6 +182,16 @@ struct MutableState { /// client code; when we need to add a hop (which is less frequent) we use /// [`Arc::make_mut()`]. path: Arc, + + /// Circuit binding keys [q.v.][`CircuitBinding`] information for each hop + /// in the circuit's path. + /// + /// NOTE: Right now, there is a `CircuitBinding` for every hop. There's a + /// fair chance that this will change in the future, and I don't want other + /// code to assume that a `CircuitBinding` _must_ exist, so I'm making this + /// an `Option`. + #[educe(Debug(ignore))] + binding: Vec>, } /// A ClientCirc that needs to send a create cell and receive a created* cell. @@ -342,6 +355,25 @@ impl ClientCirc { &self.channel } + /// Return the cryptographic material used to prove knowledge of a shared + /// secret with with `hop`. + /// + /// See [`CircuitBinding`] for more information on how this is used. + /// + /// Return None if we have no circuit binding information for the hop, or if + /// the hop does not exist. + pub fn binding_key(&self, hop: HopNum) -> Option { + self.mutable + .lock() + .expect("poisoned lock") + .binding + .get::(hop.into()) + .cloned() + .flatten() + // NOTE: I'm not thrilled to have to copy this information, but we use + // it very rarely, so it's not _that_ bad IMO. + } + /// Start an ad-hoc protocol exchange to the specified hop on this circuit /// /// To use this: diff --git a/crates/tor-proto/src/circuit/reactor.rs b/crates/tor-proto/src/circuit/reactor.rs index 484bd07fa..dc2f506d4 100644 --- a/crates/tor-proto/src/circuit/reactor.rs +++ b/crates/tor-proto/src/circuit/reactor.rs @@ -643,7 +643,8 @@ impl Reactor { let crypto_out = OutboundClientCrypt::new(); let (control_tx, control_rx) = mpsc::unbounded(); let path = Arc::new(path::Path::default()); - let mutable = Arc::new(Mutex::new(MutableState { path })); + let binding = Vec::new(); + let mutable = Arc::new(Mutex::new(MutableState { path, binding })); let (reactor_closed_tx, reactor_closed_rx) = oneshot::channel(); @@ -1080,9 +1081,9 @@ impl Reactor { self.hops.push(hop); self.crypto_in.add_layer(rev); self.crypto_out.add_layer(fwd); - drop(binding); // XXXX let mut mutable = self.mutable.lock().expect("poisoned lock"); Arc::make_mut(&mut mutable.path).push_hop(peer_id); + mutable.binding.push(binding); } /// Handle a RELAY cell on this circuit with stream ID 0. diff --git a/crates/tor-proto/src/crypto/binding.rs b/crates/tor-proto/src/crypto/binding.rs index 8275d5ebe..2ffcb0efa 100644 --- a/crates/tor-proto/src/crypto/binding.rs +++ b/crates/tor-proto/src/crypto/binding.rs @@ -1,7 +1,6 @@ //! Types related to binding messages to specific circuits -#![allow(dead_code, unreachable_pub)] // XXXX remove. - +#[cfg(feature = "hs-service")] use tor_hscrypto::ops::HsMacKey; use zeroize::Zeroizing; From d46e638ff04644c24ba7269f7da66d311f486e28 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 4 Aug 2023 09:29:50 -0400 Subject: [PATCH 8/9] proto: Fix a type-complexity warning. --- crates/tor-proto/src/circuit.rs | 6 ++++-- crates/tor-proto/src/circuit/handshake.rs | 24 ++++++++++++++++------- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/crates/tor-proto/src/circuit.rs b/crates/tor-proto/src/circuit.rs index f30964055..7372a8ea1 100644 --- a/crates/tor-proto/src/circuit.rs +++ b/crates/tor-proto/src/circuit.rs @@ -623,11 +623,13 @@ impl ClientCirc { seed: impl handshake::KeyGenerator, params: CircParameters, ) -> Result<()> { - let (outbound, inbound, binding) = protocol.construct_layers(role, seed)?; + use self::handshake::BoxedClientLayer; + + let BoxedClientLayer { fwd, back, binding } = protocol.construct_layers(role, seed)?; let (tx, rx) = oneshot::channel(); let message = CtrlMsg::ExtendVirtual { - cell_crypto: (outbound, inbound, binding), + cell_crypto: (fwd, back, binding), params, done: tx, }; diff --git a/crates/tor-proto/src/circuit/handshake.rs b/crates/tor-proto/src/circuit/handshake.rs index f8803a648..a771aa4ac 100644 --- a/crates/tor-proto/src/circuit/handshake.rs +++ b/crates/tor-proto/src/circuit/handshake.rs @@ -42,19 +42,25 @@ pub enum HandshakeRole { Responder, } +/// A set of type-erased cryptographic layers to use for a single hop at a +/// client. +pub(crate) struct BoxedClientLayer { + /// The outbound cryptographic layer to use for this hop + pub(crate) fwd: Box, + /// The inbound cryptogarphic layer to use for this hop + pub(crate) back: Box, + /// A circuit binding key for this hop. + pub(crate) binding: Option, +} + impl RelayProtocol { /// Construct the cell-crypto layers that are needed for a given set of /// circuit hop parameters. - #[allow(clippy::type_complexity)] // XXXX pub(crate) fn construct_layers( self, role: HandshakeRole, keygen: impl KeyGenerator, - ) -> Result<( - Box, - Box, - Option, - )> { + ) -> Result { match self { RelayProtocol::HsV3 => { let seed_needed = Tor1Hsv3RelayCrypto::seed_len(); @@ -65,7 +71,11 @@ impl RelayProtocol { HandshakeRole::Initiator => (fwd, back), HandshakeRole::Responder => (back, fwd), }; - Ok((Box::new(fwd), Box::new(back), Some(binding))) + Ok(BoxedClientLayer { + fwd: Box::new(fwd), + back: Box::new(back), + binding: Some(binding), + }) } } } From 603175b9776f023a77a926daa2b5c1bff721cbf3 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 2 Aug 2023 15:22:26 -0400 Subject: [PATCH 9/9] Start working on the backend for an IptEstablisher. This should be enough now to establish real introduction points, though there is still a lot of work to do. Part of #976. This has been rebased and edited to incorporate discussions from !1465. --- Cargo.lock | 2 + crates/tor-hsservice/Cargo.toml | 4 +- crates/tor-hsservice/src/svc/ipt_establish.rs | 251 +++++++++++++++++- 3 files changed, 249 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1ca351d02..d72665936 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4684,7 +4684,9 @@ dependencies = [ "futures", "rand_core 0.6.4", "thiserror", + "tor-cell", "tor-circmgr", + "tor-error", "tor-hscrypto", "tor-keymgr", "tor-linkspec", diff --git a/crates/tor-hsservice/Cargo.toml b/crates/tor-hsservice/Cargo.toml index 621e55ed1..e358f03b6 100644 --- a/crates/tor-hsservice/Cargo.toml +++ b/crates/tor-hsservice/Cargo.toml @@ -29,13 +29,15 @@ async-trait = "0.1.54" futures = "0.3.14" rand_core = "0.6.2" thiserror = "1" +tor-cell = { version = "0.12.1", path = "../tor-cell", features = ["hs"] } tor-circmgr = { version = "0.10.0", path = "../tor-circmgr", features = ["hs-service"] } +tor-error = { version = "0.5.3", path = "../tor-error" } tor-hscrypto = { version = "0.3.0", path = "../tor-hscrypto" } tor-keymgr = { version = "0.2.0", path = "../tor-keymgr" } tor-linkspec = { version = "0.8.1", path = "../tor-linkspec" } tor-llcrypto = { version = "0.5.2", path = "../tor-llcrypto" } tor-netdir = { version = "0.9.3", path = "../tor-netdir" } -tor-proto = { version = "0.12.0", path = "../tor-proto" } +tor-proto = { version = "0.12.0", path = "../tor-proto", features = ["hs-service", "send-control-msg"] } tor-rtcompat = { version = "0.9.1", path = "../tor-rtcompat" } [dev-dependencies] diff --git a/crates/tor-hsservice/src/svc/ipt_establish.rs b/crates/tor-hsservice/src/svc/ipt_establish.rs index db7af27ca..7504b835e 100644 --- a/crates/tor-hsservice/src/svc/ipt_establish.rs +++ b/crates/tor-hsservice/src/svc/ipt_establish.rs @@ -8,9 +8,21 @@ use std::sync::Arc; -use futures::channel::mpsc; +use futures::{ + channel::{mpsc, oneshot}, + StreamExt as _, +}; +use tor_cell::relaycell::{ + hs::est_intro::EstablishIntroDetails, + msg::{AnyRelayMsg, IntroEstablished, Introduce2}, + RelayMsg as _, +}; use tor_circmgr::hspool::HsCircPool; -use tor_netdir::{NetDirProvider, Relay}; +use tor_error::{internal, into_internal}; +use tor_hscrypto::pk::HsIntroPtSessionIdKeypair; +use tor_linkspec::CircTarget; +use tor_netdir::{NetDir, NetDirProvider, Relay}; +use tor_proto::circuit::{ConversationInHandler, MetaCellDisposition}; use tor_rtcompat::Runtime; use crate::RendRequest; @@ -36,12 +48,43 @@ impl Drop for IptEstablisher { } } -/// An error from trying to create in introduction point establisher. -/// -/// TODO HSS: This is probably too narrow a definition; do something else -/// instead. +/// An error from trying to work with an IptEstablisher. #[derive(Clone, Debug, thiserror::Error)] -pub(crate) enum IptError {} +pub(crate) enum IptError { + /// We couldn't get a network directory to use when building circuits. + #[error("No network directory available")] + NoNetdir(#[source] tor_netdir::Error), + + /// The network directory provider is shutting down without giving us the + /// netdir we asked for. + #[error("Network directory provider is shutting down")] + NetdirProviderShutdown, + + /// We encountered an error while building a circuit to an intro point. + #[error("Unable to build circuit to introduction point")] + BuildCircuit(#[source] tor_circmgr::Error), + + /// We encountered an error while building and signing our establish_intro + /// message. + #[error("Unable to construct signed ESTABLISH_INTRO message")] + CreateEstablishIntro(#[source] tor_cell::Error), + + /// We encountered an error while sending our establish_intro + /// message. + #[error("Unable to send an ESTABLISH_INTRO message")] + SendEstablishIntro(#[source] tor_proto::Error), + + /// We did not receive an INTRO_ESTABLISHED message like we wanted. + #[error("Did not receive INTRO_ESTABLISHED message")] + // TODO HSS: I'd like to receive more information here. What happened + // instead? But the information might be in the MsgHandler, might be in the + // Circuit,... + ReceiveAck, + + /// We encountered a programming error. + #[error("Internal error")] + Bug(#[from] tor_error::Bug), +} impl IptEstablisher { /// Try to set up, and maintain, an IPT at `Relay` @@ -112,3 +155,197 @@ pub(crate) struct IptStatus { /// retired based on having processed too many requests. pub(crate) wants_to_retire: Result<(), IptWantsToRetire>, } + +tor_cell::restricted_msg! { + /// An acceptable message to receive from an introduction point. + enum IptMsg : RelayMsg { + IntroEstablished, + Introduce2, + } +} + +/// Try, once, to make a circuit to a single relay and establish an introduction +/// point there. +/// +/// Does not retry. Does not time out except via `HsCircPool`. +async fn establish_intro_once( + pool: Arc>, + netdir_provider: Arc, + target: T, + ipt_sid_keypair: &HsIntroPtSessionIdKeypair, + // TODO HSS: Take other extensions. Ideally we would take not only + // DoSParams, but a full IntoIterator or ExtList. + // But both of those types are private in tor-cell. We should consider + // whether that's what we want. +) -> Result<(), IptError> +where + R: Runtime, + T: CircTarget, +{ + let circuit = { + let netdir = + wait_for_netdir(netdir_provider.as_ref(), tor_netdir::Timeliness::Timely).await?; + let kind = tor_circmgr::hspool::HsCircKind::SvcIntro; + pool.get_or_launch_specific(netdir.as_ref(), kind, target) + .await + .map_err(IptError::BuildCircuit)? + // note that netdir is dropped here, to avoid holding on to it any + // longer than necessary. + }; + let intro_pt_hop = circuit + .last_hop_num() + .map_err(into_internal!("Somehow built a circuit with no hops!?"))?; + + let establish_intro = { + let ipt_sid_id = ipt_sid_keypair.as_ref().public.into(); + let details = EstablishIntroDetails::new(ipt_sid_id); + let circuit_binding_key = circuit + .binding_key(intro_pt_hop) + .ok_or(internal!("No binding key for introduction point!?"))?; + if false { + // TODO HSS: Insert extensions as needed. + } + let body: Vec = details + .sign_and_encode(ipt_sid_keypair.as_ref(), circuit_binding_key.hs_mac()) + .map_err(IptError::CreateEstablishIntro)?; + + // TODO HSS: This is ugly, but it is the sensible way to munge the above + // body into a format that AnyRelayCell will accept without doing a + // redundant parse step. + // + // One alternative would be allowing start_conversation to take an `impl + // RelayMsg` rather than an AnyRelayMsg. + // + // Or possibly, when we feel like it, we could rename one or more of + // these "Unrecognized"s to Unparsed or Uninterpreted. If we do that, however, we'll + // potentially face breaking changes up and down our crate stack. + AnyRelayMsg::Unrecognized(tor_cell::relaycell::msg::Unrecognized::new( + tor_cell::relaycell::RelayCmd::ESTABLISH_INTRO, + body, + )) + }; + + let (established_tx, established_rx) = oneshot::channel(); + let (introduce_tx, introduce_rx) = mpsc::unbounded(); + + let handler = IptMsgHandler { + established_tx: Some(established_tx), + introduce_tx, + }; + let conversation = circuit + .start_conversation(Some(establish_intro), handler, intro_pt_hop) + .await + .map_err(IptError::SendEstablishIntro)?; + // At this point, we have `await`ed for the Conversation to exist, so we know + // that the message was sent. We have to wait for any actual `established` + // message, though. + + let established = established_rx.await.map_err(|_| IptError::ReceiveAck)?; + + // TODO HSS: handle all the extension data in the established field. + + // TODO HSS: Return the introduce_rx stream along with any related types. + // Or should we have taken introduce_tx as an argument? (@diziet endorses + // the "take it as an argument" idea.) + + // TODO HSS: Return the circuit too, of course. + + // TODO HSS: How shall we know if the other side has closed the circuit? We could wait + // for introduce_rx.next() to yield None, but that will only work if we use + // one mpsc::Sender per circuit... + todo!() +} + +/// Get a NetDir from `provider`, waiting until one exists. +/// +/// TODO: perhaps this function would be more generally useful if it were not here? +async fn wait_for_netdir( + provider: &dyn NetDirProvider, + timeliness: tor_netdir::Timeliness, +) -> Result, IptError> { + if let Ok(nd) = provider.netdir(timeliness) { + return Ok(nd); + } + + let mut stream = provider.events(); + loop { + // We need to retry `provider.netdir()` before waiting for any stream events, to + // avoid deadlock. + // + // TODO HSS: propagate _some_ possible errors here. + if let Ok(nd) = provider.netdir(timeliness) { + return Ok(nd); + } + match stream.next().await { + Some(_) => {} + None => { + return Err(IptError::NetdirProviderShutdown); + } + } + } +} + +/// MsgHandler type to implement a conversation with an introduction point. +/// +/// This, like all MsgHandlers, is installed at the circuit's reactor, and used +/// to handle otherwise unrecognized message types. +#[derive(Debug)] +struct IptMsgHandler { + /// A oneshot sender used to report our IntroEstablished message. + /// + /// If this is None, then we already sent an IntroEstablished and we shouldn't + /// send any more. + established_tx: Option>, + /// A channel used to report Introduce2 messages. + // + // TODO HSS: I don't like having this be unbounded, but `handle_msg` can't + // block. On the other hand maybe we can just discard excessive introduce2 + // messages if we're under high load? I think that's what C tor does, + // especially when under DoS conditions. + // + // See discussion at + // https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/1465#note_2928349 + introduce_tx: mpsc::UnboundedSender, +} + +impl tor_proto::circuit::MsgHandler for IptMsgHandler { + fn handle_msg( + &mut self, + conversation: ConversationInHandler<'_, '_, '_>, + any_msg: AnyRelayMsg, + ) -> tor_proto::Result { + // TODO HSS: Implement rate-limiting. + // + // TODO HSS: Is CircProto right or should this be a new error type? + let msg: IptMsg = any_msg.try_into().map_err(|m: AnyRelayMsg| { + tor_proto::Error::CircProto(format!("Invalid message type {}", m.cmd())) + })?; + + if match msg { + IptMsg::IntroEstablished(established) => match self.established_tx.take() { + Some(tx) => tx.send(established).map_err(|_| ()), + None => { + return Err(tor_proto::Error::CircProto( + "Received a redundant INTRO_ESTABLISHED".into(), + )); + } + }, + IptMsg::Introduce2(introduce2) => { + if self.established_tx.is_some() { + return Err(tor_proto::Error::CircProto( + "Received an INTRODUCE2 message before INTRO_ESTABLISHED".into(), + )); + } + self.introduce_tx.unbounded_send(introduce2).map_err(|_| ()) + } + } == Err(()) + { + // If the above return an error, we failed to send. That means that + // we need to close the circuit, since nobody is listening on the + // other end of the tx. + return Ok(MetaCellDisposition::CloseCirc); + } + + Ok(MetaCellDisposition::Consumed) + } +}