From f699988c411a588b278158bf6852b1a8c63c7454 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 23 Jun 2022 08:58:25 -0400 Subject: [PATCH 1/4] tor-bytes: Split EncodeError from Error Since there is currently only one error type that can occur when encoding, it doesn't make sense to use the full Error type here. This split will help us downstream, as we no longer need to categorize tor_bytes::Error as "an error in encoding or decoding". I considered renaming Error to DecodeError, but that had pretty huge downstream effects, and didn't seem to be worth it. --- crates/tor-bytes/src/err.rs | 16 +++++++++++++--- crates/tor-bytes/src/lib.rs | 2 +- crates/tor-bytes/src/writer.rs | 8 ++++---- 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/crates/tor-bytes/src/err.rs b/crates/tor-bytes/src/err.rs index e42a20b6d..d0744d283 100644 --- a/crates/tor-bytes/src/err.rs +++ b/crates/tor-bytes/src/err.rs @@ -2,7 +2,7 @@ use thiserror::Error; -/// Error type for decoding and encoding Tor objects from and to bytes. +/// Error type for decoding Tor objects from bytes. // // TODO(nickm): This error type could use a redesign: it doesn't do a good job // of preserving context. At the least it should say what kind of object it @@ -20,8 +20,8 @@ pub enum Error { /// Called Reader::should_be_exhausted(), but found bytes anyway. #[error("Extra bytes at end of object")] ExtraneousBytes, - /// Invalid length value (eg, overflow) - #[error("Object length out of bounds")] + /// Invalid length value + #[error("Object length too large to represent as usize")] BadLengthValue, /// An attempt to parse an object failed for some reason related to its /// contents. @@ -48,3 +48,13 @@ impl PartialEq for Error { } } } + +/// Error type for encoding Tor objects to bytes. +#[derive(Error, Debug, Clone, Eq, PartialEq)] +#[non_exhaustive] +pub enum EncodeError { + /// We tried to encode an object with an attached length, but the length was + /// too large to encode in the available space. + #[error("Object length too large to encode")] + BadLengthValue, +} diff --git a/crates/tor-bytes/src/lib.rs b/crates/tor-bytes/src/lib.rs index 5491d6479..450e613dd 100644 --- a/crates/tor-bytes/src/lib.rs +++ b/crates/tor-bytes/src/lib.rs @@ -85,7 +85,7 @@ mod impls; mod reader; mod writer; -pub use err::Error; +pub use err::{EncodeError, Error}; pub use reader::Reader; pub use writer::Writer; diff --git a/crates/tor-bytes/src/writer.rs b/crates/tor-bytes/src/writer.rs index b7985888d..b28df33d5 100644 --- a/crates/tor-bytes/src/writer.rs +++ b/crates/tor-bytes/src/writer.rs @@ -4,9 +4,9 @@ use std::marker::PhantomData; use educe::Educe; +use crate::EncodeError; use crate::Writeable; use crate::WriteableOnce; -use crate::{Error, Result}; /// A byte-oriented trait for writing to small arrays. /// @@ -166,9 +166,9 @@ where /// In these cases you should have ensured, somehow, that overflow cannot happen. /// Ideally, by making your `Writeable` type incapable of holding values /// whose encoded length doesn't fit in the length field. - pub fn finish(self) -> Result<()> { + pub fn finish(self) -> Result<(), EncodeError> { let length = self.inner.len(); - let length: L = length.try_into().map_err(|_| Error::BadLengthValue)?; + let length: L = length.try_into().map_err(|_| EncodeError::BadLengthValue)?; self.outer.write(&length); self.outer.write(&self.inner); Ok(()) @@ -245,6 +245,6 @@ mod tests { let mut w = v.write_nested_u8len(); w.write_zeros(256); - assert_eq!(w.finish().err().unwrap(), Error::BadLengthValue); + assert_eq!(w.finish().err().unwrap(), EncodeError::BadLengthValue); } } From 92fd9fb0dec25ddd45a4428603db99bad8d72661 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 23 Jun 2022 14:35:26 -0400 Subject: [PATCH 2/4] tor-proto: clean up error names and messages This avoids adding additional information for now; that will come on the next commits. --- crates/tor-proto/src/channel/codec.rs | 6 +- crates/tor-proto/src/channel/handshake.rs | 26 +++--- crates/tor-proto/src/channel/reactor.rs | 20 ++--- crates/tor-proto/src/circuit.rs | 2 +- crates/tor-proto/src/circuit/halfstream.rs | 8 +- crates/tor-proto/src/crypto/handshake.rs | 12 +-- crates/tor-proto/src/crypto/handshake/fast.rs | 6 +- .../tor-proto/src/crypto/handshake/hs_ntor.rs | 4 +- crates/tor-proto/src/crypto/handshake/ntor.rs | 4 +- .../tor-proto/src/crypto/handshake/ntor_v3.rs | 4 +- crates/tor-proto/src/crypto/ll/kdf.rs | 4 +- crates/tor-proto/src/util/err.rs | 79 ++++++++++--------- 12 files changed, 90 insertions(+), 85 deletions(-) diff --git a/crates/tor-proto/src/channel/codec.rs b/crates/tor-proto/src/channel/codec.rs index 54a706586..d19515b90 100644 --- a/crates/tor-proto/src/channel/codec.rs +++ b/crates/tor-proto/src/channel/codec.rs @@ -17,10 +17,10 @@ pub(crate) enum CodecError { /// /// (This isn't wrapped in an Arc, because we don't need this type to be /// clone; it's crate-internal.) - #[error("Io error")] + #[error("Io error reading or writing a channel cell")] Io(#[from] IoError), - /// An error from the cell encoding/decoding logic. - #[error("encoding/decoding error")] + /// An error from the cell decoding logic. + #[error("Error decoding an incoming channel cell")] Cell(#[from] tor_cell::Error), } diff --git a/crates/tor-proto/src/channel/handshake.rs b/crates/tor-proto/src/channel/handshake.rs index 351afd1e0..f808685b4 100644 --- a/crates/tor-proto/src/channel/handshake.rs +++ b/crates/tor-proto/src/channel/handshake.rs @@ -685,14 +685,14 @@ pub(super) mod test { assert!(matches!(err, Error::HandshakeProto(_))); assert_eq!( format!("{}", err), - "handshake protocol violation: Doesn't seem to be a tor relay" + "Handshake protocol violation: Doesn't seem to be a tor relay" ); let err = connect_err(&hex!("0000 07 0004 1234 ffff")[..], rt.clone()).await; assert!(matches!(err, Error::HandshakeProto(_))); assert_eq!( format!("{}", err), - "handshake protocol violation: No shared link protocols" + "Handshake protocol violation: No shared link protocols" ); }); } @@ -721,7 +721,7 @@ pub(super) mod test { assert!(matches!(err, Error::HandshakeProto(_))); assert_eq!( format!("{}", err), - "handshake protocol violation: Duplicate certs cell" + "Handshake protocol violation: Duplicate certs cell" ); let mut buf = Vec::new(); @@ -734,7 +734,7 @@ pub(super) mod test { assert!(matches!(err, Error::HandshakeProto(_))); assert_eq!( format!("{}", err), - "handshake protocol violation: Duplicate authchallenge cell" + "Handshake protocol violation: Duplicate authchallenge cell" ); }); } @@ -749,7 +749,7 @@ pub(super) mod test { assert!(matches!(err, Error::HandshakeProto(_))); assert_eq!( format!("{}", err), - "handshake protocol violation: Missing certs cell" + "Handshake protocol violation: Missing certs cell" ); }); } @@ -765,7 +765,7 @@ pub(super) mod test { assert!(matches!(err, Error::HandshakeProto(_))); assert_eq!( format!("{}", err), - "handshake protocol violation: Unexpected cell type CREATE" + "Handshake protocol violation: Unexpected cell type CREATE" ); }); } @@ -844,7 +844,7 @@ pub(super) mod test { .unwrap(); assert_eq!( format!("{}", err), - "handshake protocol violation: Missing IDENTITY_V_SIGNING certificate" + "Handshake protocol violation: Missing IDENTITY_V_SIGNING certificate" ); } @@ -903,7 +903,7 @@ pub(super) mod test { assert_eq!( format!("{}", res), - format!("handshake protocol violation: {}", expect_err.unwrap()) + format!("Handshake protocol violation: {}", expect_err.unwrap()) ); } } @@ -929,7 +929,7 @@ pub(super) mod test { assert_eq!( format!("{}", err), - "handshake protocol violation: Peer ed25519 id not as expected" + "Handshake protocol violation: Peer ed25519 id not as expected" ); let err = certs_test( @@ -945,7 +945,7 @@ pub(super) mod test { assert_eq!( format!("{}", err), - "handshake protocol violation: Peer RSA id not as expected" + "Handshake protocol violation: Peer RSA id not as expected" ); let err = certs_test( @@ -961,7 +961,7 @@ pub(super) mod test { assert_eq!( format!("{}", err), - "handshake protocol violation: Peer cert did not authenticate TLS cert" + "Handshake protocol violation: Peer cert did not authenticate TLS cert" ); } @@ -991,7 +991,7 @@ pub(super) mod test { assert_eq!( format!("{}", res), - "handshake protocol violation: Invalid ed25519 signature in handshake" + "Handshake protocol violation: Invalid ed25519 signature in handshake" ); let mut certs = msg::Certs::new_empty(); @@ -1012,7 +1012,7 @@ pub(super) mod test { assert_eq!( format!("{}", res), - "handshake protocol violation: Bad RSA->Ed crosscert signature" + "Handshake protocol violation: Bad RSA->Ed crosscert signature" ); } diff --git a/crates/tor-proto/src/channel/reactor.rs b/crates/tor-proto/src/channel/reactor.rs index b71278f0e..370841a52 100644 --- a/crates/tor-proto/src/channel/reactor.rs +++ b/crates/tor-proto/src/channel/reactor.rs @@ -566,7 +566,7 @@ pub(crate) mod test { let (circ, _) = futures::join!(pending.create_firsthop_fast(&circparams), send_response); // Make sure statuses are as expected. - assert!(matches!(circ.err().unwrap(), Error::BadCircHandshake)); + assert!(matches!(circ.err().unwrap(), Error::BadCircHandshakeAuth)); reactor.run_once().await.unwrap(); @@ -604,13 +604,13 @@ pub(crate) mod test { let e = reactor.run_once().await.unwrap_err().unwrap_err(); assert_eq!( format!("{}", e), - "channel protocol violation: CREATE2 cell on client channel" + "Channel protocol violation: CREATE2 cell on client channel" ); let e = reactor.run_once().await.unwrap_err().unwrap_err(); assert_eq!( format!("{}", e), - "channel protocol violation: Unexpected CREATED* cell not on opening circuit" + "Channel protocol violation: Unexpected CREATED* cell not on opening circuit" ); // Can't get a relay cell on a circuit we've never heard of. @@ -622,7 +622,7 @@ pub(crate) mod test { let e = reactor.run_once().await.unwrap_err().unwrap_err(); assert_eq!( format!("{}", e), - "channel protocol violation: Relay cell on nonexistent circuit" + "Channel protocol violation: Relay cell on nonexistent circuit" ); // Can't get handshaking cells while channel is open. @@ -634,7 +634,7 @@ pub(crate) mod test { let e = reactor.run_once().await.unwrap_err().unwrap_err(); assert_eq!( format!("{}", e), - "channel protocol violation: VERSIONS cell after handshake is done" + "Channel protocol violation: VERSIONS cell after handshake is done" ); // We don't accept CREATED. @@ -646,7 +646,7 @@ pub(crate) mod test { let e = reactor.run_once().await.unwrap_err().unwrap_err(); assert_eq!( format!("{}", e), - "channel protocol violation: CREATED cell received, but we never send CREATEs" + "Channel protocol violation: CREATED cell received, but we never send CREATEs" ); }); } @@ -695,7 +695,7 @@ pub(crate) mod test { let e = reactor.run_once().await.unwrap_err().unwrap_err(); assert_eq!( format!("{}", e), - "channel protocol violation: Relay cell on pending circuit before CREATED* received" + "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. @@ -706,7 +706,7 @@ pub(crate) mod test { let e = reactor.run_once().await.unwrap_err().unwrap_err(); assert_eq!( format!("{}", e), - "channel protocol violation: Relay cell on nonexistent circuit" + "Channel protocol violation: Relay cell on nonexistent circuit" ); // It's fine to get a relay cell on a DestroySent channel: that happens @@ -729,7 +729,7 @@ pub(crate) mod test { let e = reactor.run_once().await.unwrap_err().unwrap_err(); assert_eq!( format!("{}", e), - "channel protocol violation: Too many cells received on destroyed circuit" + "Channel protocol violation: Too many cells received on destroyed circuit" ); }); } @@ -793,7 +793,7 @@ pub(crate) mod test { let e = reactor.run_once().await.unwrap_err().unwrap_err(); assert_eq!( format!("{}", e), - "channel protocol violation: Destroy for nonexistent circuit" + "Channel protocol violation: Destroy for nonexistent circuit" ); }); } diff --git a/crates/tor-proto/src/circuit.rs b/crates/tor-proto/src/circuit.rs index f01b7c6df..4be4a386e 100644 --- a/crates/tor-proto/src/circuit.rs +++ b/crates/tor-proto/src/circuit.rs @@ -1151,7 +1151,7 @@ mod test { let extended2 = relaymsg::Extended2::new(vec![99; 256]).into(); let cc = rmsg_to_ccmsg(0, extended2); let error = bad_extend_test_impl(&rt, 2.into(), cc).await; - assert!(matches!(error, Error::BadCircHandshake)); + assert!(matches!(error, Error::BadCircHandshakeAuth)); }); } diff --git a/crates/tor-proto/src/circuit/halfstream.rs b/crates/tor-proto/src/circuit/halfstream.rs index 838357d75..7c70c947e 100644 --- a/crates/tor-proto/src/circuit/halfstream.rs +++ b/crates/tor-proto/src/circuit/halfstream.rs @@ -99,7 +99,7 @@ mod test { let e = hs.handle_msg(&m).err().unwrap(); assert_eq!( format!("{}", e), - "circuit protocol violation: Received a SENDME when none was expected" + "Circuit protocol violation: Received a SENDME when none was expected" ); Ok(()) } @@ -124,7 +124,7 @@ mod test { let e = hs.handle_msg(&m).err().unwrap(); assert_eq!( format!("{}", e), - "circuit protocol violation: Received a data cell in violation of a window" + "Circuit protocol violation: Received a data cell in violation of a window" ); } @@ -143,7 +143,7 @@ mod test { let e = hs.handle_msg(&m).err().unwrap(); assert_eq!( format!("{}", e), - "circuit protocol violation: Bad CONNECTED cell on a closed stream!" + "Circuit protocol violation: Bad CONNECTED cell on a closed stream!" ); } @@ -154,7 +154,7 @@ mod test { let e = hs.handle_msg(&m).err().unwrap(); assert_eq!( format!("{}", e), - "circuit protocol violation: Bad EXTENDED2 cell on a closed stream!" + "Circuit protocol violation: Bad EXTENDED2 cell on a closed stream!" ); } } diff --git a/crates/tor-proto/src/crypto/handshake.rs b/crates/tor-proto/src/crypto/handshake.rs index 0af659efb..d8e1b926b 100644 --- a/crates/tor-proto/src/crypto/handshake.rs +++ b/crates/tor-proto/src/crypto/handshake.rs @@ -122,17 +122,17 @@ impl KeyGenerator for ShakeKeyGenerator { /// An error produced by a Relay's attempt to handle a client's onion handshake. #[derive(Clone, Debug, thiserror::Error)] pub(crate) enum RelayHandshakeError { - /// An error in parsing or encoding a handshake message. - #[error("problem in handshake format.")] + /// An error in parsing a handshake message. + #[error("Problem decoding onion handshake")] Fmt(#[from] tor_bytes::Error), /// The client asked for a key we didn't have. - #[error("unrecognized key or identity in handshake")] + #[error("Client asked for a key or ID that we don't have")] MissingKey, /// The client did something wrong with their handshake or cryptography. - #[error("bad handshake from client")] - BadHandshake, + #[error("Bad handshake from client")] + BadClientHandshake, /// An internal error. - #[error("internal error")] + #[error("Internal error")] Internal(#[from] tor_error::Bug), } diff --git a/crates/tor-proto/src/crypto/handshake/fast.rs b/crates/tor-proto/src/crypto/handshake/fast.rs index 07c31f052..aaaabef21 100644 --- a/crates/tor-proto/src/crypto/handshake/fast.rs +++ b/crates/tor-proto/src/crypto/handshake/fast.rs @@ -39,7 +39,7 @@ impl super::ClientHandshake for CreateFastClient { fn client2>(state: Self::StateType, msg: T) -> Result { let msg = msg.as_ref(); if msg.len() != FAST_S_HANDSHAKE_LEN { - return Err(Error::BadCircHandshake); + return Err(Error::BadCircHandshakeAuth); } let mut inp = Vec::new(); inp.extend(&state.0[..]); @@ -48,7 +48,7 @@ impl super::ClientHandshake for CreateFastClient { let kh_expect = LegacyKdf::new(0).derive(&inp[..], 20)?; if !bytes_eq(&kh_expect, &msg[20..40]) { - return Err(Error::BadCircHandshake); + return Err(Error::BadCircHandshakeAuth); } Ok(super::TapKeyGenerator::new(inp.into())) @@ -71,7 +71,7 @@ impl super::ServerHandshake for CreateFastServer { ) -> RelayHandshakeResult<(Self::KeyGen, Vec)> { let msg = msg.as_ref(); if msg.len() != FAST_C_HANDSHAKE_LEN { - return Err(RelayHandshakeError::BadHandshake); + return Err(RelayHandshakeError::BadClientHandshake); } let mut reply = vec![0_u8; FAST_S_HANDSHAKE_LEN]; rng.fill_bytes(&mut reply[0..20]); diff --git a/crates/tor-proto/src/crypto/handshake/hs_ntor.rs b/crates/tor-proto/src/crypto/handshake/hs_ntor.rs index 533c3ebeb..4de5e04a4 100644 --- a/crates/tor-proto/src/crypto/handshake/hs_ntor.rs +++ b/crates/tor-proto/src/crypto/handshake/hs_ntor.rs @@ -240,7 +240,7 @@ where // Validate the MAC! if my_mac_tag != mac_tag { - return Err(Error::BadCircHandshake); + return Err(Error::BadCircHandshakeAuth); } Ok(keygen) @@ -328,7 +328,7 @@ where let my_mac_tag = hs_ntor_mac(&mac_body, &mac_key)?; if my_mac_tag != mac_tag { - return Err(Error::BadCircHandshake); + return Err(Error::BadCircHandshakeAuth); } // Decrypt the ENCRYPTED_DATA from the intro cell diff --git a/crates/tor-proto/src/crypto/handshake/ntor.rs b/crates/tor-proto/src/crypto/handshake/ntor.rs index b33be1658..d7e494e78 100644 --- a/crates/tor-proto/src/crypto/handshake/ntor.rs +++ b/crates/tor-proto/src/crypto/handshake/ntor.rs @@ -185,7 +185,7 @@ where if okay.into() { Ok(keygen) } else { - Err(Error::BadCircHandshake) + Err(Error::BadCircHandshakeAuth) } } @@ -305,7 +305,7 @@ where if okay.into() { Ok((keygen, reply)) } else { - Err(RelayHandshakeError::BadHandshake) + Err(RelayHandshakeError::BadClientHandshake) } } diff --git a/crates/tor-proto/src/crypto/handshake/ntor_v3.rs b/crates/tor-proto/src/crypto/handshake/ntor_v3.rs index 9dc13dae8..5496bebab 100644 --- a/crates/tor-proto/src/crypto/handshake/ntor_v3.rs +++ b/crates/tor-proto/src/crypto/handshake/ntor_v3.rs @@ -468,7 +468,7 @@ fn server_handshake_ntor_v3_no_keygen( if okay.into() { Ok((reply, keystream)) } else { - Err(RelayHandshakeError::BadHandshake) + Err(RelayHandshakeError::BadClientHandshake) } } @@ -540,7 +540,7 @@ fn client_handshake_ntor_v3_part2( if okay.into() { Ok((server_reply, keystream)) } else { - Err(Error::BadCircHandshake) + Err(Error::BadCircHandshakeAuth) } } diff --git a/crates/tor-proto/src/crypto/ll/kdf.rs b/crates/tor-proto/src/crypto/ll/kdf.rs index 70c10809e..98102ba8a 100644 --- a/crates/tor-proto/src/crypto/ll/kdf.rs +++ b/crates/tor-proto/src/crypto/ll/kdf.rs @@ -64,7 +64,7 @@ impl Kdf for LegacyKdf { let mut result = Zeroizing::new(Vec::with_capacity(n_bytes + Sha1::output_size())); let mut k = self.idx; if n_bytes > Sha1::output_size() * (256 - (k as usize)) { - return Err(Error::InvalidOutputLength); + return Err(Error::InvalidKDFOutputLength); } while result.len() < n_bytes { @@ -93,7 +93,7 @@ impl Kdf for Ntor1Kdf<'_, '_> { let mut result = Zeroizing::new(vec![0; n_bytes]); hkdf.expand(self.m_expand, &mut result[..]) - .map_err(|_| Error::InvalidOutputLength)?; + .map_err(|_| Error::InvalidKDFOutputLength)?; Ok(result) } } diff --git a/crates/tor-proto/src/util/err.rs b/crates/tor-proto/src/util/err.rs index f24834bac..ce6a08bfd 100644 --- a/crates/tor-proto/src/util/err.rs +++ b/crates/tor-proto/src/util/err.rs @@ -14,90 +14,95 @@ use tor_error::{ErrorKind, HasKind}; pub enum Error { /// An error that occurred in the tor_bytes crate while decoding an /// object. - #[error("parsing error")] + // TODO: add context. + #[error("Unable to parse a binary object")] BytesErr(#[from] tor_bytes::Error), /// An error that occurred from the io system when using a /// channel. - #[error("io error on channel")] + #[error("IO error on channel with peer")] ChanIoErr(#[source] Arc), /// An error from the io system that occurred when trying to connect a channel. - #[error("io error in handshake")] + #[error("IO error while handshaking with peer")] HandshakeIoErr(#[source] Arc), /// An error occurred in the cell-handling layer. - #[error("cell encoding error")] + // TODO: add context. + #[error("Cell encoding or decoding error")] CellErr(#[source] tor_cell::Error), - /// We tried to produce too much output for some function. - #[error("couldn't produce that much output")] - InvalidOutputLength, + /// We tried to produce too much output for a key derivation function. + #[error("Tried to extract too many bytes from a KDF")] + InvalidKDFOutputLength, /// We tried to encrypt a message to a hop that wasn't there. - #[error("tried to encrypt to nonexistent hop")] + #[error("Tried to encrypt a cell for a nonexistent hop")] NoSuchHop, /// The authentication information on this cell was completely wrong, /// or the cell was corrupted. - #[error("bad relay cell authentication")] + #[error("Bad relay cell authentication")] BadCellAuth, - /// A circuit-extension handshake failed. - #[error("handshake failed")] - BadCircHandshake, + /// A circuit-extension handshake failed due to a mismatched authentication + /// value. + #[error("Circuit-extension handshake authentication failed")] + BadCircHandshakeAuth, /// Handshake protocol violation. - #[error("handshake protocol violation: {0}")] + #[error("Handshake protocol violation: {0}")] HandshakeProto(String), /// Handshake broken, maybe due to clock skew. /// /// (If the problem can't be due to clock skew, we return HandshakeProto /// instead.) - #[error("handshake failed due to expired certificates (possible clock skew)")] + #[error("Handshake failed due to expired certificates (possible clock skew)")] HandshakeCertsExpired { /// For how long has the circuit been expired? expired_by: Duration, }, /// Protocol violation at the channel level, other than at the handshake /// stage. - #[error("channel protocol violation: {0}")] + #[error("Channel protocol violation: {0}")] ChanProto(String), /// Protocol violation at the circuit level - #[error("circuit protocol violation: {0}")] + #[error("Circuit protocol violation: {0}")] CircProto(String), - /// Channel is closed. - #[error("{0}")] + /// Channel is closed, or became closed while we were trying to do some + /// operation. + #[error("Channel closed")] ChannelClosed(#[from] ChannelClosed), - /// Circuit is closed. - #[error("circuit closed")] + /// Circuit is closed, or became closed while we were trying to so some + /// operation. + #[error("Circuit closed")] CircuitClosed, /// Can't allocate any more circuit or stream IDs on a channel. - #[error("too many entries in map: can't allocate ID")] + #[error("Too many entries in map: can't allocate ID")] IdRangeFull, /// Couldn't extend a circuit because the extending relay or the /// target relay refused our request. - #[error("circuit extension handshake error: {0}")] + #[error("Circuit extension refused: {0}")] CircRefused(&'static str), /// Tried to make or use a stream to an invalid destination address. - #[error("invalid stream target address")] + #[error("Invalid stream target address")] BadStreamAddress, /// Received an End cell from the other end of a stream. - #[error("Received an End cell with reason {0}")] + #[error("Received an END cell with reason {0}")] EndReceived(EndReason), /// Stream was already closed when we tried to use it. - #[error("stream not connected")] + #[error("Stream not connected")] NotConnected, /// Stream protocol violation - #[error("stream protocol violation: {0}")] + #[error("Stream protocol violation: {0}")] StreamProto(String), /// Channel does not match target - #[error("channel mismatch: {0}")] + #[error("Peer identity mismatch: {0}")] ChanMismatch(String), /// There was a programming error somewhere in our code, or the calling code. #[error("Programming error")] Bug(#[from] tor_error::Bug), /// Remote DNS lookup failed. - #[error("remote resolve failed")] + #[error("Remote resolve failed")] ResolveError(#[source] ResolveError), } -/// Error which indicates that the channel was closed +/// Error which indicates that the channel was closed. #[derive(Error, Debug, Clone)] -#[error("channel closed")] +#[error("Channel closed")] pub struct ChannelClosed; impl HasKind for ChannelClosed { @@ -111,13 +116,13 @@ impl HasKind for ChannelClosed { #[non_exhaustive] pub enum ResolveError { /// A transient error which can be retried - #[error("received retriable transient error")] + #[error("Received retriable transient error")] Transient, /// A non transient error, which shouldn't be retried - #[error("received not retriable error")] + #[error("Received non-retriable error")] Nontransient, /// Could not parse the response properly - #[error("received unrecognized result")] + #[error("Received unrecognized result")] Unrecognized, } @@ -140,7 +145,7 @@ impl From for std::io::Error { Err(arc) => return std::io::Error::new(arc.kind(), arc), }, - InvalidOutputLength | NoSuchHop | BadStreamAddress => ErrorKind::InvalidInput, + InvalidKDFOutputLength | NoSuchHop | BadStreamAddress => ErrorKind::InvalidInput, NotConnected => ErrorKind::NotConnected, @@ -150,7 +155,7 @@ impl From for std::io::Error { BytesErr(_) | BadCellAuth - | BadCircHandshake + | BadCircHandshakeAuth | HandshakeProto(_) | ChanProto(_) | HandshakeCertsExpired { .. } @@ -179,10 +184,10 @@ impl HasKind for Error { E::ChanIoErr(_) => EK::LocalNetworkError, E::HandshakeIoErr(_) => EK::TorAccessFailed, E::CellErr(e) => e.kind(), - E::InvalidOutputLength => EK::Internal, + E::InvalidKDFOutputLength => EK::Internal, E::NoSuchHop => EK::BadApiUsage, E::BadCellAuth => EK::TorProtocolViolation, - E::BadCircHandshake => EK::TorProtocolViolation, + E::BadCircHandshakeAuth => EK::TorProtocolViolation, E::HandshakeProto(_) => EK::TorAccessFailed, E::HandshakeCertsExpired { .. } => EK::ClockSkew, E::ChanProto(_) => EK::TorProtocolViolation, From 35b7b8a47ad96f5eee032fab7900b3b29bf2fa73 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 23 Jun 2022 14:59:52 -0400 Subject: [PATCH 3/4] tor-proto: Split CellErr based on activity. Failing to encode is fundamentally different from failing to decode. We now treat those separately, and describe _what_ we failed to encode or decode. --- crates/tor-proto/src/channel/codec.rs | 9 +++-- crates/tor-proto/src/channel/handshake.rs | 8 +++-- crates/tor-proto/src/channel/reactor.rs | 5 +-- crates/tor-proto/src/circuit.rs | 3 +- crates/tor-proto/src/circuit/reactor.rs | 5 ++- crates/tor-proto/src/util/err.rs | 42 ++++++++++++++++++----- 6 files changed, 54 insertions(+), 18 deletions(-) diff --git a/crates/tor-proto/src/channel/codec.rs b/crates/tor-proto/src/channel/codec.rs index d19515b90..d381ffc82 100644 --- a/crates/tor-proto/src/channel/codec.rs +++ b/crates/tor-proto/src/channel/codec.rs @@ -21,7 +21,10 @@ pub(crate) enum CodecError { Io(#[from] IoError), /// An error from the cell decoding logic. #[error("Error decoding an incoming channel cell")] - Cell(#[from] tor_cell::Error), + DecCell(#[source] tor_cell::Error), + /// An error from the cell encoding logic. + #[error("Error encoding an outgoing channel cell")] + EncCell(#[source] tor_cell::Error), } /// Asynchronous wrapper around ChannelCodec in tor_cell, with implementation @@ -44,7 +47,7 @@ impl futures_codec::Encoder for ChannelCodec { type Error = CodecError; fn encode(&mut self, item: Self::Item, dst: &mut BytesMut) -> Result<(), Self::Error> { - self.0.write_cell(item, dst)?; + self.0.write_cell(item, dst).map_err(CodecError::EncCell)?; Ok(()) } } @@ -54,7 +57,7 @@ impl futures_codec::Decoder for ChannelCodec { type Error = CodecError; fn decode(&mut self, src: &mut BytesMut) -> Result, Self::Error> { - Ok(self.0.decode_cell(src)?) + self.0.decode_cell(src).map_err(CodecError::DecCell) } } diff --git a/crates/tor-proto/src/channel/handshake.rs b/crates/tor-proto/src/channel/handshake.rs index f808685b4..0b403e6ce 100644 --- a/crates/tor-proto/src/channel/handshake.rs +++ b/crates/tor-proto/src/channel/handshake.rs @@ -112,7 +112,10 @@ pub struct VerifiedChannel Error { match err { CodecError::Io(e) => Error::HandshakeIoErr(Arc::new(e)), - CodecError::Cell(e) => Error::HandshakeProto(format!("Invalid cell on handshake: {}", e)), + CodecError::DecCell(e) => { + Error::HandshakeProto(format!("Invalid cell on handshake: {}", e)) + } + CodecError::EncCell(e) => Error::from_cell_enc(e, "cell on handshake"), } } @@ -150,7 +153,8 @@ impl trace!("{}: sending versions", self.unique_id); // Send versions cell { - let my_versions = msg::Versions::new(LINK_PROTOCOLS)?; + let my_versions = msg::Versions::new(LINK_PROTOCOLS) + .map_err(|e| Error::from_cell_enc(e, "versions message"))?; self.tls .write_all(&my_versions.encode_for_handshake()) .await diff --git a/crates/tor-proto/src/channel/reactor.rs b/crates/tor-proto/src/channel/reactor.rs index 370841a52..bd810c961 100644 --- a/crates/tor-proto/src/channel/reactor.rs +++ b/crates/tor-proto/src/channel/reactor.rs @@ -47,7 +47,8 @@ pub(super) type ReactorResultChannel = oneshot::Sender>; fn codec_err_to_chan(err: CodecError) -> Error { match err { CodecError::Io(e) => crate::Error::ChanIoErr(Arc::new(e)), - CodecError::Cell(e) => e.into(), + CodecError::EncCell(err) => Error::from_cell_enc(err, "channel cell"), + CodecError::DecCell(err) => Error::from_cell_dec(err, "channel cell"), } } @@ -438,7 +439,7 @@ pub(crate) mod test { let dummy_target = OwnedChanTarget::new(vec![], [6; 32].into(), [10; 20].into()); let send1 = send1.sink_map_err(|e| { trace!("got sink error: {}", e); - CodecError::Cell(tor_cell::Error::ChanProto("dummy message".into())) + CodecError::DecCell(tor_cell::Error::ChanProto("dummy message".into())) }); let (chan, reactor) = crate::channel::Channel::new( link_protocol, diff --git a/crates/tor-proto/src/circuit.rs b/crates/tor-proto/src/circuit.rs index 4be4a386e..a27994a42 100644 --- a/crates/tor-proto/src/circuit.rs +++ b/crates/tor-proto/src/circuit.rs @@ -342,7 +342,8 @@ impl ClientCirc { let parameters = parameters.unwrap_or_default(); let begin_flags = parameters.begin_flags(); let optimistic = parameters.is_optimistic(); - let beginmsg = Begin::new(target, port, begin_flags)?; + let beginmsg = Begin::new(target, port, begin_flags) + .map_err(|e| Error::from_cell_enc(e, "begin message"))?; self.begin_data_stream(beginmsg.into(), optimistic).await } diff --git a/crates/tor-proto/src/circuit/reactor.rs b/crates/tor-proto/src/circuit/reactor.rs index c5da96f8f..b54733844 100644 --- a/crates/tor-proto/src/circuit/reactor.rs +++ b/crates/tor-proto/src/circuit/reactor.rs @@ -1006,7 +1006,10 @@ impl Reactor { return Ok(()); } } - let mut body: RelayCellBody = cell.encode(&mut rand::thread_rng())?.into(); + let mut body: RelayCellBody = cell + .encode(&mut rand::thread_rng()) + .map_err(|e| Error::from_cell_enc(e, "relay cell body"))? + .into(); let tag = self.crypto_out.encrypt(&mut body, hop)?; // NOTE(eta): Now that we've encrypted the cell, we *must* either send it or abort // the whole circuit (e.g. by returning an error). diff --git a/crates/tor-proto/src/util/err.rs b/crates/tor-proto/src/util/err.rs index ce6a08bfd..83574ac03 100644 --- a/crates/tor-proto/src/util/err.rs +++ b/crates/tor-proto/src/util/err.rs @@ -24,10 +24,24 @@ pub enum Error { /// An error from the io system that occurred when trying to connect a channel. #[error("IO error while handshaking with peer")] HandshakeIoErr(#[source] Arc), - /// An error occurred in the cell-handling layer. - // TODO: add context. - #[error("Cell encoding or decoding error")] - CellErr(#[source] tor_cell::Error), + /// An error occurred while trying to create or encode a cell. + #[error("Unable to generate or encode {object}")] + CellEncodeErr { + /// The object we were trying to create or encode. + object: &'static str, + /// The error that occurred. + #[source] + err: tor_cell::Error, + }, + /// An error occurred while trying to decode or parse a cell. + #[error("Error while parsing {object}")] + CellDecodeErr { + /// The object we were trying to decode. + object: &'static str, + /// The error that occurred. + #[source] + err: tor_cell::Error, + }, /// We tried to produce too much output for a key derivation function. #[error("Tried to extract too many bytes from a KDF")] InvalidKDFOutputLength, @@ -126,11 +140,19 @@ pub enum ResolveError { Unrecognized, } -impl From for Error { - fn from(err: tor_cell::Error) -> Error { +impl Error { + /// Create an error from a tor_cell error that has occurred while trying to + /// encode or create something of type `object` + pub(crate) fn from_cell_enc(err: tor_cell::Error, object: &'static str) -> Error { + Error::CellEncodeErr { object, err } + } + + /// Create an error from a tor_cell error that has occurred while trying to + /// decode something of type `object` + pub(crate) fn from_cell_dec(err: tor_cell::Error, object: &'static str) -> Error { match err { tor_cell::Error::ChanProto(msg) => Error::ChanProto(msg), - _ => Error::CellErr(err), + _ => Error::CellDecodeErr { err, object }, } } } @@ -161,7 +183,8 @@ impl From for std::io::Error { | HandshakeCertsExpired { .. } | ChannelClosed(_) | CircProto(_) - | CellErr(_) + | CellDecodeErr { .. } + | CellEncodeErr { .. } | ChanMismatch(_) | StreamProto(_) => ErrorKind::InvalidData, @@ -183,7 +206,8 @@ impl HasKind for Error { E::BytesErr(_) => EK::TorProtocolViolation, E::ChanIoErr(_) => EK::LocalNetworkError, E::HandshakeIoErr(_) => EK::TorAccessFailed, - E::CellErr(e) => e.kind(), + E::CellEncodeErr { err, .. } => err.kind(), + E::CellDecodeErr { err, .. } => err.kind(), E::InvalidKDFOutputLength => EK::Internal, E::NoSuchHop => EK::BadApiUsage, E::BadCellAuth => EK::TorProtocolViolation, From 38004a4f4dd3aa99b81b6585039e81dc77c17f32 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 23 Jun 2022 15:42:45 -0400 Subject: [PATCH 4/4] tor-proto: split and elaborate tor_bytes::Error instances Some of these were for decoding particular objects (we now say what kind of objects), and some were unrelated tor_cert errors that for some reason we had shoved into a tor_bytes::Error. There is now a separate tor_cert::CertError type, independent from tor_cert's use of `tor_bytes::Error` for parsing errors. --- Cargo.lock | 1 + crates/tor-cert/Cargo.toml | 1 + crates/tor-cert/semver.md | 1 + crates/tor-cert/src/err.rs | 24 +++++++++ crates/tor-cert/src/lib.rs | 54 +++++++++---------- crates/tor-cert/tests/invalid_certs.rs | 5 +- crates/tor-netdoc/src/doc/routerdesc.rs | 20 +++++-- crates/tor-netdoc/src/err.rs | 3 ++ crates/tor-proto/src/channel/handshake.rs | 19 +++++-- crates/tor-proto/src/circuit/reactor.rs | 3 +- .../tor-proto/src/crypto/handshake/hs_ntor.rs | 21 ++++++-- crates/tor-proto/src/crypto/handshake/ntor.rs | 8 ++- .../tor-proto/src/crypto/handshake/ntor_v3.rs | 8 ++- crates/tor-proto/src/util/err.rs | 32 ++++++++--- 14 files changed, 145 insertions(+), 55 deletions(-) create mode 100644 crates/tor-cert/semver.md create mode 100644 crates/tor-cert/src/err.rs diff --git a/Cargo.lock b/Cargo.lock index d3d397fa0..2e2db1c76 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3492,6 +3492,7 @@ dependencies = [ "digest 0.10.3", "hex-literal", "signature", + "thiserror", "tor-bytes", "tor-checkable", "tor-llcrypto", diff --git a/crates/tor-cert/Cargo.toml b/crates/tor-cert/Cargo.toml index 2c320266c..0cc9ea69d 100644 --- a/crates/tor-cert/Cargo.toml +++ b/crates/tor-cert/Cargo.toml @@ -15,6 +15,7 @@ repository = "https://gitlab.torproject.org/tpo/core/arti.git/" caret = { path = "../caret", version = "0.2.0" } digest = "0.10.0" signature = "1" +thiserror = "1" tor-bytes = { path = "../tor-bytes", version = "0.3.0" } tor-checkable = { path = "../tor-checkable", version = "0.3.0" } tor-llcrypto = { path = "../tor-llcrypto", version = "0.3.0" } diff --git a/crates/tor-cert/semver.md b/crates/tor-cert/semver.md new file mode 100644 index 000000000..05b44c67a --- /dev/null +++ b/crates/tor-cert/semver.md @@ -0,0 +1 @@ +BREAKING: No longer using tor_bytes::Error for non-decoding errors diff --git a/crates/tor-cert/src/err.rs b/crates/tor-cert/src/err.rs new file mode 100644 index 000000000..12e88d65f --- /dev/null +++ b/crates/tor-cert/src/err.rs @@ -0,0 +1,24 @@ +//! Define error types for the tor-cert crate. +//! +//! Most of the encoding/decoding functions here return [`tor_bytes::Error`], +//! but many of them (related to certificate-specific operations) do not. + +use thiserror::Error; + +/// An error related to checking or validating a certificate +#[derive(Clone, Debug, Error, Eq, PartialEq)] +#[non_exhaustive] +pub enum CertError { + /// The key on a certificate was not as expected. + #[error("Key on certificate was not as expected")] + KeyMismatch, + + /// We tried to get the signing key from a certificate that didn't include + /// one. + #[error("Missing signing key on certificate")] + MissingPubKey, + + /// We tried to validate a signature, and found that it was wrong. + #[error("Signature on certificate was invalid")] + BadSignature, +} diff --git a/crates/tor-cert/src/lib.rs b/crates/tor-cert/src/lib.rs index e42ddcb9e..afe45211b 100644 --- a/crates/tor-cert/src/lib.rs +++ b/crates/tor-cert/src/lib.rs @@ -83,16 +83,22 @@ #![allow(clippy::let_unit_value)] // This can reasonably be done for explicitness //! +mod err; pub mod rsa; use caret::caret_int; use signature::Verifier; -use tor_bytes::{Error, Result}; +use tor_bytes::{Error as BytesError, Result as BytesResult}; use tor_bytes::{Readable, Reader}; use tor_llcrypto::pk::*; use std::time; +pub use err::CertError; + +/// A Result defined to use CertError +type CertResult = std::result::Result; + caret_int! { /// Recognized values for Tor's certificate type field. /// @@ -241,7 +247,7 @@ impl CertifiedKey { } /// Try to extract a CertifiedKey from a Reader, given that we have /// already read its type as `key_type`. - fn from_reader(key_type: KeyType, r: &mut Reader<'_>) -> Result { + fn from_reader(key_type: KeyType, r: &mut Reader<'_>) -> BytesResult { Ok(match key_type { KeyType::ED25519_KEY => CertifiedKey::Ed25519(r.extract()?), KeyType::SHA256_OF_RSA => CertifiedKey::RsaSha256Digest(r.extract()?), @@ -343,7 +349,7 @@ impl Writeable for UnrecognizedExt { */ impl Readable for CertExt { - fn take_from(b: &mut Reader<'_>) -> Result { + fn take_from(b: &mut Reader<'_>) -> BytesResult { let len = b.take_u16()?; let ext_type: ExtType = b.take_u8()?.into(); let flags = b.take_u8()?; @@ -352,16 +358,16 @@ impl Readable for CertExt { Ok(match ext_type { ExtType::SIGNED_WITH_ED25519_KEY => { if body.len() != 32 { - return Err(Error::BadMessage("wrong length on Ed25519 key")); + return Err(BytesError::BadMessage("wrong length on Ed25519 key")); } CertExt::SignedWithEd25519(SignedWithEd25519Ext { pk: ed25519::PublicKey::from_bytes(body) - .map_err(|_| Error::BadMessage("invalid Ed25519 public key"))?, + .map_err(|_| BytesError::BadMessage("invalid Ed25519 public key"))?, }) } _ => { if (flags & 1) != 0 { - return Err(Error::BadMessage( + return Err(BytesError::BadMessage( "unrecognized certificate extension, with 'affects_validation' flag set.", )); } @@ -412,13 +418,13 @@ impl Ed25519Cert { /// Note that the resulting KeyUnknownCertificate is not checked /// for validity at all: you will need to provide it with an expected /// signing key, then check it for timeliness and well-signedness. - pub fn decode(cert: &[u8]) -> Result { + pub fn decode(cert: &[u8]) -> BytesResult { let mut r = Reader::from_slice(cert); let v = r.take_u8()?; if v != 1 { // This would be something other than a "v1" certificate. We don't // understand those. - return Err(Error::BadMessage("Unrecognized certificate version")); + return Err(BytesError::BadMessage("Unrecognized certificate version")); } let cert_type = r.take_u8()?.into(); let exp_hours = r.take_u32()?; @@ -519,13 +525,13 @@ impl KeyUnknownCert { /// /// On success, we can check whether the certificate is well-signed; /// otherwise, we can't check the certificate. - pub fn check_key(self, pkey: &Option) -> Result { + pub fn check_key(self, pkey: &Option) -> CertResult { let real_key = match (pkey, self.cert.cert.signed_with) { (Some(a), Some(b)) if a == &b => b, - (Some(_), Some(_)) => return Err(Error::BadMessage("Mismatched public key on cert")), + (Some(_), Some(_)) => return Err(CertError::KeyMismatch), (Some(a), None) => *a, (None, Some(b)) => b, - (None, None) => return Err(Error::BadMessage("Missing public key on cert")), + (None, None) => return Err(CertError::MissingPubKey), }; Ok(UncheckedCert { cert: Ed25519Cert { @@ -567,12 +573,9 @@ impl UncheckedCert { /// been checked, and a signature to validate. pub fn dangerously_split( self, - ) -> Result<(SigCheckedCert, ed25519::ValidatableEd25519Signature)> { + ) -> CertResult<(SigCheckedCert, ed25519::ValidatableEd25519Signature)> { use tor_checkable::SelfSigned; - let signing_key = self - .cert - .signed_with - .ok_or(Error::BadMessage("Missing public key on cert"))?; + let signing_key = self.cert.signed_with.ok_or(CertError::MissingPubKey)?; let signature = ed25519::ValidatableEd25519Signature::new(signing_key, self.signature, &self.text[..]); Ok((self.dangerously_assume_wellsigned(), signature)) @@ -592,17 +595,14 @@ impl UncheckedCert { } impl tor_checkable::SelfSigned for UncheckedCert { - type Error = Error; + type Error = CertError; - fn is_well_signed(&self) -> Result<()> { - let pubkey = &self - .cert - .signed_with - .ok_or(Error::BadMessage("Certificate was not in fact self-signed"))?; + fn is_well_signed(&self) -> CertResult<()> { + let pubkey = &self.cert.signed_with.ok_or(CertError::MissingPubKey)?; pubkey .verify(&self.text[..], &self.signature) - .map_err(|_| Error::BadMessage("Invalid certificate signature"))?; + .map_err(|_| CertError::BadSignature)?; Ok(()) } @@ -638,7 +638,7 @@ mod test { use hex_literal::hex; #[test] - fn parse_unrecognized_ext() -> Result<()> { + fn parse_unrecognized_ext() -> BytesResult<()> { // case one: a flag is set but we don't know it let b = hex!("0009 99 10 657874656e73696f6e"); let mut r = Reader::from_slice(&b); @@ -651,11 +651,11 @@ mod test { // handle the extension. let b = hex!("0009 99 11 657874656e73696f6e"); let mut r = Reader::from_slice(&b); - let e: Result = r.extract(); + let e: Result = r.extract(); assert!(e.is_err()); assert_eq!( e.err().unwrap(), - Error::BadMessage( + BytesError::BadMessage( "unrecognized certificate extension, with 'affects_validation' flag set." ) ); @@ -664,7 +664,7 @@ mod test { } #[test] - fn certified_key() -> Result<()> { + fn certified_key() -> BytesResult<()> { let b = hex!("4c27616d6f757220756e6974206365757820717527656e636861c3ae6e616974206c6520666572"); let mut r = Reader::from_slice(&b); diff --git a/crates/tor-cert/tests/invalid_certs.rs b/crates/tor-cert/tests/invalid_certs.rs index d15f63e90..47b669ccc 100644 --- a/crates/tor-cert/tests/invalid_certs.rs +++ b/crates/tor-cert/tests/invalid_certs.rs @@ -1,4 +1,5 @@ use tor_bytes::Error; +use tor_cert::CertError; //use tor_cert::rsa::RsaCrosscert; use tor_cert::Ed25519Cert; use tor_llcrypto::pk::ed25519; @@ -69,7 +70,7 @@ fn mismatched_signing_key() { // wasn't what the cert contained. assert_eq!( cert.check_key(&Some(not_that_key)).err().unwrap(), - Error::BadMessage("Mismatched public key on cert") + CertError::KeyMismatch ); // from testvec_certs. @@ -86,6 +87,6 @@ fn mismatched_signing_key() { // a signing-key extension in the cert. assert_eq!( cert.check_key(&None).err().unwrap(), - Error::BadMessage("Missing public key on cert") + CertError::MissingPubKey ); } diff --git a/crates/tor-netdoc/src/doc/routerdesc.rs b/crates/tor-netdoc/src/doc/routerdesc.rs index 1727c377b..c3b290985 100644 --- a/crates/tor-netdoc/src/doc/routerdesc.rs +++ b/crates/tor-netdoc/src/doc/routerdesc.rs @@ -405,7 +405,8 @@ impl RouterDesc { .parse_obj::("ED25519 CERT")? .check_cert_type(tor_cert::CertType::IDENTITY_V_SIGNING)? .into_unchecked() - .check_key(&None)?; + .check_key(&None) + .map_err(|err| EK::BadSignature.err().with_source(err))?; let sk = cert.peek_subject_key().as_ed25519().ok_or_else(|| { EK::BadObjectVal .at_pos(cert_tok.pos()) @@ -509,7 +510,7 @@ impl RouterDesc { if sign != 0 && sign != 1 { return Err(EK::BadArgument.at_pos(cc.arg_pos(0)).with_msg("not 0 or 1")); } - let ntor_as_ed = + let ntor_as_ed: ll::pk::ed25519::PublicKey = ll::pk::keymanip::convert_curve25519_to_ed25519_public(&ntor_onion_key, sign) .ok_or_else(|| { EK::BadArgument @@ -521,7 +522,8 @@ impl RouterDesc { .check_cert_type(tor_cert::CertType::NTOR_CC_IDENTITY)? .check_subject_key_is(identity_cert.peek_signing_key())? .into_unchecked() - .check_key(&Some(ntor_as_ed))? + .check_key(&Some(ntor_as_ed)) + .map_err(|err| EK::BadSignature.err().with_source(err))? }; // TAP key @@ -636,8 +638,16 @@ impl RouterDesc { }; // Now we're going to collect signatures and expiration times. - let (identity_cert, identity_sig) = identity_cert.dangerously_split()?; - let (crosscert_cert, cc_sig) = crosscert_cert.dangerously_split()?; + let (identity_cert, identity_sig) = identity_cert.dangerously_split().map_err(|err| { + EK::BadObjectVal + .with_msg("missing public key") + .with_source(err) + })?; + let (crosscert_cert, cc_sig) = crosscert_cert.dangerously_split().map_err(|err| { + EK::BadObjectVal + .with_msg("missing public key") + .with_source(err) + })?; let signatures: Vec> = vec![ Box::new(rsa_signature), Box::new(ed_signature), diff --git a/crates/tor-netdoc/src/err.rs b/crates/tor-netdoc/src/err.rs index 140ff5c11..7684d4e68 100644 --- a/crates/tor-netdoc/src/err.rs +++ b/crates/tor-netdoc/src/err.rs @@ -291,6 +291,9 @@ pub(crate) enum ParseErrorSource { /// An error when validating a signature. #[error("Invalid signature")] Signature(#[source] Arc), + /// An error when validating a signature on an embedded binary certificate. + #[error("Invalid certificate")] + CertSignature(#[from] tor_cert::CertError), /// Invalid protocol versions. #[error("Protocol versions")] Protovers(#[from] tor_protover::ParseError), diff --git a/crates/tor-proto/src/channel/handshake.rs b/crates/tor-proto/src/channel/handshake.rs index 0b403e6ce..e8d26ddc5 100644 --- a/crates/tor-proto/src/channel/handshake.rs +++ b/crates/tor-proto/src/channel/handshake.rs @@ -189,7 +189,9 @@ impl .await .map_err(io_err_to_handshake)?; let mut reader = Reader::from_slice(&msg); - reader.extract()? + reader + .extract() + .map_err(|e| Error::from_bytes_err(e, "versions cell"))? }; trace!("{}: received {:?}", self.unique_id, their_versions); @@ -404,7 +406,11 @@ impl Unver // clock skew.) // Check the identity->signing cert - let (id_sk, id_sk_sig) = id_sk.check_key(&None)?.dangerously_split()?; + let (id_sk, id_sk_sig) = id_sk + .check_key(&None) + .map_err(Error::HandshakeCertErr)? + .dangerously_split() + .map_err(Error::HandshakeCertErr)?; sigs.push(&id_sk_sig); let (id_sk_timeliness, id_sk) = check_timeliness(id_sk, now, self.clock_skew); @@ -421,8 +427,10 @@ impl Unver // Now look at the signing->TLS cert and check it against the // peer certificate. let (sk_tls, sk_tls_sig) = sk_tls - .check_key(&Some(*signing_key))? // TODO(nickm): this is a bad interface - .dangerously_split()?; + .check_key(&Some(*signing_key)) + .map_err(Error::HandshakeCertErr)? + .dangerously_split() + .map_err(Error::HandshakeCertErr)?; sigs.push(&sk_tls_sig); let (sk_tls_timeliness, sk_tls) = check_timeliness(sk_tls, now, self.clock_skew); @@ -465,7 +473,8 @@ impl Unver let rsa_cert = c .cert_body(CertType::RSA_ID_V_IDENTITY) .ok_or_else(|| Error::HandshakeProto("No RSA->Ed crosscert".into()))?; - let rsa_cert = tor_cert::rsa::RsaCrosscert::decode(rsa_cert)? + let rsa_cert = tor_cert::rsa::RsaCrosscert::decode(rsa_cert) + .map_err(|e| Error::from_bytes_err(e, "RSA identity cross-certificate"))? .check_signature(&pkrsa) .map_err(|_| Error::HandshakeProto("Bad RSA->Ed crosscert signature".into()))?; let (rsa_cert_timeliness, rsa_cert) = check_timeliness(rsa_cert, now, self.clock_skew); diff --git a/crates/tor-proto/src/circuit/reactor.rs b/crates/tor-proto/src/circuit/reactor.rs index b54733844..905441ec4 100644 --- a/crates/tor-proto/src/circuit/reactor.rs +++ b/crates/tor-proto/src/circuit/reactor.rs @@ -1245,7 +1245,8 @@ impl Reactor { tag_copy }; // Decode the cell. - let msg = RelayCell::decode(body.into())?; + let msg = + RelayCell::decode(body.into()).map_err(|e| Error::from_bytes_err(e, "relay cell"))?; let c_t_w = sendme::cell_counts_towards_windows(&msg); diff --git a/crates/tor-proto/src/crypto/handshake/hs_ntor.rs b/crates/tor-proto/src/crypto/handshake/hs_ntor.rs index 4de5e04a4..9ff583b4a 100644 --- a/crates/tor-proto/src/crypto/handshake/hs_ntor.rs +++ b/crates/tor-proto/src/crypto/handshake/hs_ntor.rs @@ -222,8 +222,12 @@ where { // Extract the public key of the service from the message let mut cur = Reader::from_slice(msg.as_ref()); - let Y: curve25519::PublicKey = cur.extract()?; - let mac_tag: MacTag = cur.extract()?; + let Y: curve25519::PublicKey = cur + .extract() + .map_err(|e| Error::from_bytes_err(e, "hs_ntor handshake"))?; + let mac_tag: MacTag = cur + .extract() + .map_err(|e| Error::from_bytes_err(e, "hs_ntor handshake"))?; // Get EXP(Y,x) and EXP(B,x) let xy = state.x.diffie_hellman(&Y); @@ -305,10 +309,17 @@ where { // Extract all the useful pieces from the message let mut cur = Reader::from_slice(msg.as_ref()); - let X: curve25519::PublicKey = cur.extract()?; + let X: curve25519::PublicKey = cur + .extract() + .map_err(|e| Error::from_bytes_err(e, "hs ntor handshake"))?; let remaining_bytes = cur.remaining(); - let ciphertext = &mut cur.take(remaining_bytes - 32)?.to_vec(); - let mac_tag: MacTag = cur.extract()?; + let ciphertext = &mut cur + .take(remaining_bytes - 32) + .map_err(|e| Error::from_bytes_err(e, "hs ntor handshake"))? + .to_vec(); + let mac_tag: MacTag = cur + .extract() + .map_err(|e| Error::from_bytes_err(e, "hs ntor handshake"))?; // Now derive keys needed for handling the INTRO1 cell let bx = proto_input.b.diffie_hellman(&X); diff --git a/crates/tor-proto/src/crypto/handshake/ntor.rs b/crates/tor-proto/src/crypto/handshake/ntor.rs index d7e494e78..74e14f332 100644 --- a/crates/tor-proto/src/crypto/handshake/ntor.rs +++ b/crates/tor-proto/src/crypto/handshake/ntor.rs @@ -169,8 +169,12 @@ where T: AsRef<[u8]>, { let mut cur = Reader::from_slice(msg.as_ref()); - let their_pk: PublicKey = cur.extract()?; - let auth: Authcode = cur.extract()?; + let their_pk: PublicKey = cur + .extract() + .map_err(|e| Error::from_bytes_err(e, "v3 ntor handshake"))?; + let auth: Authcode = cur + .extract() + .map_err(|e| Error::from_bytes_err(e, "v3 ntor handshake"))?; let xy = state.my_sk.diffie_hellman(&their_pk); let xb = state.my_sk.diffie_hellman(&state.relay_public.pk); diff --git a/crates/tor-proto/src/crypto/handshake/ntor_v3.rs b/crates/tor-proto/src/crypto/handshake/ntor_v3.rs index 5496bebab..4608038ac 100644 --- a/crates/tor-proto/src/crypto/handshake/ntor_v3.rs +++ b/crates/tor-proto/src/crypto/handshake/ntor_v3.rs @@ -485,8 +485,12 @@ fn client_handshake_ntor_v3_part2( verification: &[u8], ) -> Result<(Vec, impl digest::XofReader)> { let mut reader = Reader::from_slice(relay_handshake); - let y_pk: curve25519::PublicKey = reader.extract()?; - let auth: DigestVal = reader.extract()?; + let y_pk: curve25519::PublicKey = reader + .extract() + .map_err(|e| Error::from_bytes_err(e, "v3 ntor handshake"))?; + let auth: DigestVal = reader + .extract() + .map_err(|e| Error::from_bytes_err(e, "v3 ntor handshake"))?; let encrypted_msg = reader.into_rest(); let yx = state.my_sk.diffie_hellman(&y_pk); diff --git a/crates/tor-proto/src/util/err.rs b/crates/tor-proto/src/util/err.rs index 83574ac03..9004cf5d1 100644 --- a/crates/tor-proto/src/util/err.rs +++ b/crates/tor-proto/src/util/err.rs @@ -14,9 +14,14 @@ use tor_error::{ErrorKind, HasKind}; pub enum Error { /// An error that occurred in the tor_bytes crate while decoding an /// object. - // TODO: add context. - #[error("Unable to parse a binary object")] - BytesErr(#[from] tor_bytes::Error), + #[error("Unable to parse {object}")] + BytesErr { + /// What we were trying to parse. + object: &'static str, + /// The error that occurred while parsing it. + #[source] + err: tor_bytes::Error, + }, /// An error that occurred from the io system when using a /// channel. #[error("IO error on channel with peer")] @@ -42,6 +47,10 @@ pub enum Error { #[source] err: tor_cell::Error, }, + /// We found a problem with one of the certificates in the channel + /// handshake. + #[error("Problem with certificate on handshake")] + HandshakeCertErr(#[source] tor_cert::CertError), /// We tried to produce too much output for a key derivation function. #[error("Tried to extract too many bytes from a KDF")] InvalidKDFOutputLength, @@ -155,6 +164,12 @@ impl Error { _ => Error::CellDecodeErr { err, object }, } } + + /// Create an error for a tor_bytes error that occurred while parsing + /// something of type `object`. + pub(crate) fn from_bytes_err(err: tor_bytes::Error, object: &'static str) -> Error { + Error::BytesErr { err, object } + } } impl From for std::io::Error { @@ -175,10 +190,11 @@ impl From for std::io::Error { CircuitClosed => ErrorKind::ConnectionReset, - BytesErr(_) + BytesErr { .. } | BadCellAuth | BadCircHandshakeAuth | HandshakeProto(_) + | HandshakeCertErr(_) | ChanProto(_) | HandshakeCertsExpired { .. } | ChannelClosed(_) @@ -202,10 +218,14 @@ impl HasKind for Error { use Error as E; use ErrorKind as EK; match self { - E::BytesErr(BytesError::Bug(e)) => e.kind(), - E::BytesErr(_) => EK::TorProtocolViolation, + E::BytesErr { + err: BytesError::Bug(e), + .. + } => e.kind(), + E::BytesErr { .. } => EK::TorProtocolViolation, E::ChanIoErr(_) => EK::LocalNetworkError, E::HandshakeIoErr(_) => EK::TorAccessFailed, + E::HandshakeCertErr(_) => EK::TorProtocolViolation, E::CellEncodeErr { err, .. } => err.kind(), E::CellDecodeErr { err, .. } => err.kind(), E::InvalidKDFOutputLength => EK::Internal,