From 3cd17be26f4ce02b466c8b33939414abadd3d57a Mon Sep 17 00:00:00 2001 From: Simon Wicky Date: Thu, 27 Jul 2023 10:56:35 +0200 Subject: [PATCH] error handling --- Cargo.lock | 57 ++++----- .../client-libs/mixnet-client/src/client.rs | 2 +- common/nymnoise/Cargo.toml | 1 + common/nymnoise/src/lib.rs | 109 ++++++++++++------ 4 files changed, 106 insertions(+), 63 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 417b96fc55..d78c53720c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -267,7 +267,7 @@ checksum = "16e62a023e7c117e27523144c5d2459f4397fcc3cab0085af8e2224f643a0193" dependencies = [ "proc-macro2", "quote", - "syn 2.0.16", + "syn 2.0.27", ] [[package]] @@ -278,7 +278,7 @@ checksum = "b9ccdd8f2a161be9bd5c023df56f1b2a0bd1d83872ae53b71a84a12c9bf6e842" dependencies = [ "proc-macro2", "quote", - "syn 2.0.16", + "syn 2.0.27", ] [[package]] @@ -818,7 +818,7 @@ dependencies = [ "heck 0.4.1", "proc-macro2", "quote", - "syn 2.0.16", + "syn 2.0.27", ] [[package]] @@ -1739,7 +1739,7 @@ checksum = "eecf8589574ce9b895052fa12d69af7a233f99e6107f5cb8dd1044f2a17bfdcb" dependencies = [ "proc-macro2", "quote", - "syn 2.0.16", + "syn 2.0.27", ] [[package]] @@ -2097,7 +2097,7 @@ checksum = "89ca545a94061b6365f2c7355b4b32bd20df3ff95f02da9329b34ccc3bd6ee72" dependencies = [ "proc-macro2", "quote", - "syn 2.0.16", + "syn 2.0.27", ] [[package]] @@ -2236,7 +2236,7 @@ checksum = "e77ac7b51b8e6313251737fcef4b1c01a2ea102bde68415b62c0ee9268fec357" dependencies = [ "proc-macro2", "quote", - "syn 2.0.16", + "syn 2.0.27", ] [[package]] @@ -4240,6 +4240,7 @@ dependencies = [ "pin-project", "sha2 0.10.7", "snow", + "thiserror", "tokio", ] @@ -4861,7 +4862,7 @@ checksum = "a948666b637a0f465e8564c73e89d4dde00d72d4d473cc972f390fc3dcee7d9c" dependencies = [ "proc-macro2", "quote", - "syn 2.0.16", + "syn 2.0.27", ] [[package]] @@ -5137,7 +5138,7 @@ dependencies = [ "proc-macro2", "proc-macro2-diagnostics 0.10.0", "quote", - "syn 2.0.16", + "syn 2.0.27", ] [[package]] @@ -5214,7 +5215,7 @@ dependencies = [ "pest_meta", "proc-macro2", "quote", - "syn 2.0.16", + "syn 2.0.27", ] [[package]] @@ -5245,7 +5246,7 @@ checksum = "39407670928234ebc5e6e580247dd567ad73a3578460c5990f9503df207e8f07" dependencies = [ "proc-macro2", "quote", - "syn 2.0.16", + "syn 2.0.27", ] [[package]] @@ -5425,9 +5426,9 @@ dependencies = [ [[package]] name = "proc-macro2" -version = "1.0.58" +version = "1.0.66" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fa1fb82fc0c281dd9671101b66b771ebbe1eaf967b96ac8740dcba4b70005ca8" +checksum = "18fb31db3f9bddb2ea821cde30a9f70117e3f119938b5ee630b7403aa6e2ead9" dependencies = [ "unicode-ident", ] @@ -5453,7 +5454,7 @@ checksum = "606c4ba35817e2922a308af55ad51bab3645b59eae5c570d4a6cf07e36bd493b" dependencies = [ "proc-macro2", "quote", - "syn 2.0.16", + "syn 2.0.27", "version_check", "yansi", ] @@ -5548,9 +5549,9 @@ checksum = "a993555f31e5a609f617c12db6250dedcac1b0a85076912c436e6fc9b2c8e6a3" [[package]] name = "quote" -version = "1.0.27" +version = "1.0.32" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8f4f29d145265ec1c483c7c654450edde0bfe043d3938d6972630663356d9500" +checksum = "50f3b39ccfb720540debaa0164757101c08ecb8d326b15358ce76a62c7e85965" dependencies = [ "proc-macro2", ] @@ -5843,7 +5844,7 @@ checksum = "8d2275aab483050ab2a7364c1a46604865ee7d6906684e08db0f090acf74f9e7" dependencies = [ "proc-macro2", "quote", - "syn 2.0.16", + "syn 2.0.27", ] [[package]] @@ -6395,7 +6396,7 @@ checksum = "8c805777e3930c8883389c602315a24224bcc738b63905ef87cd1420353ea93e" dependencies = [ "proc-macro2", "quote", - "syn 2.0.16", + "syn 2.0.27", ] [[package]] @@ -6428,7 +6429,7 @@ checksum = "bcec881020c684085e55a25f7fd888954d56609ef363479dc5a1305eb0d40cab" dependencies = [ "proc-macro2", "quote", - "syn 2.0.16", + "syn 2.0.27", ] [[package]] @@ -7005,9 +7006,9 @@ dependencies = [ [[package]] name = "syn" -version = "2.0.16" +version = "2.0.27" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a6f671d4b5ffdb8eadec19c0ae67fe2639df8684bd7bc4b83d986b8db549cf01" +checksum = "b60f673f44a8255b9c8c657daf66a596d435f2da81a555b06dc644d080ba45e0" dependencies = [ "proc-macro2", "quote", @@ -7167,22 +7168,22 @@ checksum = "222a222a5bfe1bba4a77b45ec488a741b3cb8872e5e499451fd7d0129c9c7c3d" [[package]] name = "thiserror" -version = "1.0.40" +version = "1.0.44" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "978c9a314bd8dc99be594bc3c175faaa9794be04a5a5e153caba6915336cebac" +checksum = "611040a08a0439f8248d1990b111c95baa9c704c805fa1f62104b39655fd7f90" dependencies = [ "thiserror-impl", ] [[package]] name = "thiserror-impl" -version = "1.0.40" +version = "1.0.44" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f9456a42c5b0d803c8cd86e73dd7cc9edd429499f37a3550d286d5e86720569f" +checksum = "090198534930841fab3a5d1bb637cde49e339654e606195f8d9c76eeb081dc96" dependencies = [ "proc-macro2", "quote", - "syn 2.0.16", + "syn 2.0.27", ] [[package]] @@ -7319,7 +7320,7 @@ checksum = "630bdcf245f78637c13ec01ffae6187cca34625e8c63150d424b59e55af2675e" dependencies = [ "proc-macro2", "quote", - "syn 2.0.16", + "syn 2.0.27", ] [[package]] @@ -7555,7 +7556,7 @@ checksum = "0f57e3ca2a01450b1a921183a9c9cbfda207fd822cef4ccb00a65402cbba7a74" dependencies = [ "proc-macro2", "quote", - "syn 2.0.16", + "syn 2.0.27", ] [[package]] @@ -8405,5 +8406,5 @@ checksum = "ce36e65b0d2999d2aafac989fb249189a141aee1f53c612c1f37d72631959f69" dependencies = [ "proc-macro2", "quote", - "syn 2.0.16", + "syn 2.0.27", ] diff --git a/common/client-libs/mixnet-client/src/client.rs b/common/client-libs/mixnet-client/src/client.rs index 720bf1ab9d..d7b3014b04 100644 --- a/common/client-libs/mixnet-client/src/client.rs +++ b/common/client-libs/mixnet-client/src/client.rs @@ -119,7 +119,7 @@ impl Client { let topology_ref = match topology_permit.try_get_raw_topology_ref() { Ok(topology) => topology, Err(err) => { - error!("Cannot connect to {address}, due to topology error - {err}"); + error!("Cannot perform Noise handshake to {address}, due to topology error - {err}"); return; } }; diff --git a/common/nymnoise/Cargo.toml b/common/nymnoise/Cargo.toml index f8fff90729..9cf8551839 100644 --- a/common/nymnoise/Cargo.toml +++ b/common/nymnoise/Cargo.toml @@ -12,6 +12,7 @@ pin-project = "1" log = "0.4.19" sha2 = "0.10.7" bytes = "1.0" +thiserror = "1.0.44" # internal nym-topology = { path = "../topology"} \ No newline at end of file diff --git a/common/nymnoise/src/lib.rs b/common/nymnoise/src/lib.rs index a0d3c0f37e..b10c195597 100644 --- a/common/nymnoise/src/lib.rs +++ b/common/nymnoise/src/lib.rs @@ -8,11 +8,13 @@ use pin_project::pin_project; use sha2::{Digest, Sha256}; use snow::error::Prerequisite; use snow::Builder; -use snow::Error as NoiseError; +use snow::Error; use snow::TransportState; use std::io; +use std::io::ErrorKind; use std::pin::Pin; use std::task::Poll; +use thiserror::Error; use tokio::io::AsyncReadExt; use tokio::io::AsyncWriteExt; use tokio::io::ReadBuf; @@ -26,6 +28,26 @@ const MAXMSGLEN: usize = 65535; const TAGLEN: usize = 16; const HEADER_SIZE: usize = 2; +#[derive(Error, Debug)] +pub enum NoiseError { + #[error("encountered a Noise decryption error")] + DecryptionError, + + #[error("encountered a Noise Protocol error - {0}")] + ProtocolError(Error), + #[error("encountered an IO error - {0}")] + IoError(#[from] io::Error), +} + +impl From for NoiseError { + fn from(err: Error) -> Self { + match err { + Error::Decrypt => NoiseError::DecryptionError, + err => NoiseError::ProtocolError(err), + } + } +} + /// Wrapper around a TcpStream #[pin_project] pub struct NoiseStream { @@ -58,6 +80,7 @@ impl AsyncRead for NoiseStream { match projected_self.inner_stream.poll_read(cx, &mut noise_buf) { Poll::Pending => return Poll::Pending, Poll::Ready(Ok(())) => { + //concat what we just with what we had let bytes_read = [ &projected_self.storage[..projected_self.storage.len()], noise_buf.filled(), @@ -65,34 +88,46 @@ impl AsyncRead for NoiseStream { .concat(); projected_self.storage.clear(); - //We can't read the length + //We can't read the length, store what we have if bytes_read.len() < HEADER_SIZE { projected_self.storage.put_slice(&bytes_read); - cx.waker().wake_by_ref(); //ideally register cx with the readyness of tcpstream + cx.waker().wake_by_ref(); //ideally register cx with the readiness of tcpstream return Poll::Pending; } let msg_len = ((bytes_read[0] as usize) << 8) + (bytes_read[1] as usize); - //we can't read the whole message + //we can't read the whole message, store what we have and return if bytes_read.len() < HEADER_SIZE + msg_len { projected_self.storage.put_slice(&bytes_read); - cx.waker().wake_by_ref(); //ideally register cx with the readyness of tcpstream + cx.waker().wake_by_ref(); //ideally register cx with the readiness of tcpstream return Poll::Pending; } //we have a full Noise message available let mut payload = vec![0u8; MAXMSGLEN]; - let len = projected_self - .noise - .read_message( - &bytes_read[HEADER_SIZE..HEADER_SIZE + msg_len], - &mut payload, - ) - .unwrap(); + let len = match projected_self.noise.read_message( + &bytes_read[HEADER_SIZE..HEADER_SIZE + msg_len], + &mut payload, + ) { + Ok(len) => len, + Err(_) => return Poll::Ready(Err(ErrorKind::InvalidData.into())), + }; + + //No place in the buffer, say that we have nothing. + //SW might not be acceptable, can create offset? + //Maybe return what we can, store the rest in unencrypted buffer for the next call + if len > buf.remaining() { + warn!("Not enough space in the buffer to return decrypted message"); + projected_self.storage.put_slice(&bytes_read); + return Poll::Pending; + } buf.put_slice(&payload[..len]); + + //Store any excess for next time projected_self .storage .put_slice(&bytes_read[HEADER_SIZE + msg_len..]); + return Poll::Ready(Ok(())); } Poll::Ready(Err(err)) => return Poll::Ready(Err(err)), @@ -108,22 +143,27 @@ impl AsyncWrite for NoiseStream { let projected_self = self.project(); let mut noise_buf = vec![0u8; MAXMSGLEN]; - let len = projected_self - .noise - .write_message(buf, &mut noise_buf) - .unwrap(); + let len = match projected_self.noise.write_message(buf, &mut noise_buf) { + Ok(len) => len, + Err(_) => return Poll::Ready(Err(ErrorKind::InvalidInput.into())), + }; let to_send = [&[(len >> 8) as u8, (len & 0xff) as u8], &noise_buf[..len]].concat(); match projected_self.inner_stream.poll_write(cx, &to_send) { Poll::Pending => return Poll::Pending, Poll::Ready(Err(err)) => return Poll::Ready(Err(err)), Poll::Ready(Ok(n)) => { + //didn't send a thing, no problem for the underlying stream + if n == 0 { + return Poll::Ready(Ok(0)); + } + //we sent the whole thing, no problem for the underlying stream //We must guarantee that the return number is <= buf.len() - if n >= HEADER_SIZE + TAGLEN { + if n == to_send.len() { return Poll::Ready(Ok(n - HEADER_SIZE - TAGLEN)); } - //We didn't write any meaningful info - return Poll::Ready(Ok(0)); + //We didn't write the whole message, the stream will be corrupted + return Poll::Ready(Err(ErrorKind::WriteZero.into())); } } } @@ -149,14 +189,14 @@ pub async fn upgrade_noise_initiator( local_public_key: &[u8], local_private_key: &[u8], ) -> Result { - debug!("Perform Noise Handshake, initiator side"); + trace!("Perform Noise Handshake, initiator side"); //Get init material let responder_addr = match conn.peer_addr() { Ok(addr) => addr, Err(err) => { error!("Unable to extract peer address from connection - {err}"); - return Err(Prerequisite::RemotePublicKey.into()); + return Err(Error::Prereq(Prerequisite::RemotePublicKey).into()); } }; let remote_pub_key = match topology.find_node_key_by_mix_host(responder_addr) { @@ -166,7 +206,7 @@ pub async fn upgrade_noise_initiator( "Cannot find public key for node with address {:?}", responder_addr ); - return Err(Prerequisite::RemotePublicKey.into()); + return Err(Error::Prereq(Prerequisite::RemotePublicKey).into()); } }; let secret = [local_public_key, &remote_pub_key].concat(); @@ -183,14 +223,14 @@ pub async fn upgrade_noise_initiator( let mut buf = vec![0u8; MAXMSGLEN]; // -> e, es let len = handshake.write_message(&[], &mut buf)?; - send(&mut conn, &buf[..len]).await; + send(&mut conn, &buf[..len]).await?; // <- e, ee - handshake.read_message(&recv(&mut conn).await.unwrap(), &mut buf)?; + handshake.read_message(&recv(&mut conn).await?, &mut buf)?; // -> s, se, psk let len = handshake.write_message(&[], &mut buf)?; - send(&mut conn, &buf[..len]).await; + send(&mut conn, &buf[..len]).await?; let noise = handshake.into_transport_mode()?; @@ -203,14 +243,14 @@ pub async fn upgrade_noise_responder( local_public_key: &[u8], local_private_key: &[u8], ) -> Result { - debug!("Perform Noise Handshake, responder side"); + trace!("Perform Noise Handshake, responder side"); //Get init material let initiator_addr = match conn.peer_addr() { Ok(addr) => addr, Err(err) => { error!("Unable to extract peer address from connection - {err}"); - return Err(Prerequisite::RemotePublicKey.into()); + return Err(Error::Prereq(Prerequisite::RemotePublicKey).into()); } }; let remote_pub_key = match topology.find_node_key_by_mix_host(initiator_addr) { @@ -220,7 +260,7 @@ pub async fn upgrade_noise_responder( "Cannot find public key for node with address {:?}", initiator_addr ); - return Err(Prerequisite::RemotePublicKey.into()); + return Err(Error::Prereq(Prerequisite::RemotePublicKey).into()); } }; let secret = [&remote_pub_key, local_public_key].concat(); @@ -235,14 +275,14 @@ pub async fn upgrade_noise_responder( //Actual Handshake let mut buf = vec![0u8; MAXMSGLEN]; // <- e, es - handshake.read_message(&recv(&mut conn).await.unwrap(), &mut buf)?; + handshake.read_message(&recv(&mut conn).await?, &mut buf)?; // -> e, ee let len = handshake.write_message(&[], &mut buf)?; - send(&mut conn, &buf[..len]).await; + send(&mut conn, &buf[..len]).await?; // <- s, se, psk - handshake.read_message(&recv(&mut conn).await.unwrap(), &mut buf)?; + handshake.read_message(&recv(&mut conn).await?, &mut buf)?; let noise = handshake.into_transport_mode()?; @@ -260,8 +300,9 @@ async fn recv(stream: &mut TcpStream) -> io::Result> { } /// Hyper-basic stream transport sender. 16-bit BE size followed by payload. -async fn send(stream: &mut TcpStream, buf: &[u8]) { +async fn send(stream: &mut TcpStream, buf: &[u8]) -> io::Result<()> { let msg_len_buf = [(buf.len() >> 8) as u8, (buf.len() & 0xff) as u8]; - stream.write_all(&msg_len_buf).await.unwrap(); - stream.write_all(buf).await.unwrap(); + stream.write_all(&msg_len_buf).await?; + stream.write_all(buf).await?; + Ok(()) }