diff --git a/performance/Cargo.toml b/performance/Cargo.toml index bb8cb89..2142c0d 100644 --- a/performance/Cargo.toml +++ b/performance/Cargo.toml @@ -3,7 +3,7 @@ authors = ["ZeroTier, Inc. ", "Adam Ierymenko { println!("[alice] ERROR {:?}", e); - if let ReceiveError::ByzantineFault(e, _) = e { - assert!(!e.unnatural()) + if let ReceiveError::ByzantineFault(e) = e { + assert!(!e.unnatural) } } } @@ -302,8 +302,8 @@ fn bob_main( Ok(_) => {} Err(e) => { println!("[bob] ERROR {:?}", e); - if let ReceiveError::ByzantineFault(e, _) = e { - assert!(!e.unnatural()) + if let ReceiveError::ByzantineFault(e) = e { + assert!(!e.unnatural) } } } diff --git a/performance/examples/benchmark.rs b/performance/examples/benchmark.rs index 415fc8b..c0dd19d 100644 --- a/performance/examples/benchmark.rs +++ b/performance/examples/benchmark.rs @@ -170,8 +170,8 @@ fn alice_main( Ok(_) => {} Err(e) => { println!("[alice] ERROR {:?}", e); - if let ReceiveError::ByzantineFault(e, _) = e { - assert!(!e.unnatural()) + if let ReceiveError::ByzantineFault(e) = e { + assert!(!e.unnatural) } } } @@ -260,8 +260,8 @@ fn bob_main( Ok(_) => {} Err(e) => { println!("[bob] ERROR {:?}", e); - if let ReceiveError::ByzantineFault(e, _) = e { - assert!(!e.unnatural()) + if let ReceiveError::ByzantineFault(e) = e { + assert!(!e.unnatural) } } } diff --git a/performance/src/result.rs b/performance/src/result.rs index 877ddd9..5811f4e 100644 --- a/performance/src/result.rs +++ b/performance/src/result.rs @@ -1,5 +1,5 @@ use std::error::Error; -use std::fmt::Display; +use std::fmt; use std::sync::Arc; use crate::application::CryptoLayer; @@ -41,6 +41,14 @@ pub enum SendError { DataTooLarge, } +/// The contained session has just expired. +/// +/// An expired session is no longer "owned" by the ZSSP context. +/// Therefore it is no longer capable of sending, receiving or being serviced, +/// so it should be dropped. +#[derive(Clone)] +pub struct ExpiredError(pub Arc>); + /// A type of fault occurred because we received a bad packet. /// /// An unauthenticated attacker can intentionally trigger any of these, so it is best to @@ -71,11 +79,22 @@ pub enum FaultType { /// Because an unauthenticated remote peer can force these to occur with specific /// contained information, it is recommended in production to either drop these /// immediately, or log them safely to a local output stream and then drop them. -#[derive(Debug)] -pub struct ByzantineFault { +pub struct ByzantineFault { + /// The session associated with this fault, if there was one. + /// + /// The sender specified this session within their packet, but they were not authenticated, + /// so the sender could theoretically be anyone. + pub session: Option>>, + /// This field is true if this fault caused the session it was attached to to expire. + /// An expired session is no longer "owned" by the ZSSP context. + /// Therefore it is no longer capable of sending, receiving or being serviced, + /// so it should be dropped. + /// + /// If this returns true then it is guaranteed that the `session` field is occupied. + pub caused_expiration: bool, /// The type of fault that has occurred. Be cautious if you choose to read this /// value, as an attacker has control over it. - pub(crate) error: FaultType, + pub error: FaultType, /// Some byzantine faults within ZSSP are naturally occurring, i.e. they can occur /// between two well behaved and trusted parties executing the protocol. /// This boolean is false if this is one of these faults. If you go to the file and @@ -88,7 +107,7 @@ pub struct ByzantineFault { /// persevered (i.e. bits have been flipped) to be unnatural. /// ZSSP also considers collisions of what are supposed to be uniform random /// numbers to be unnatural. - pub(crate) unnatural: bool, + pub unnatural: bool, /// The file of this implementation of ZSSP from which this error was generated. #[cfg(feature = "debug")] pub(crate) file: &'static str, @@ -99,43 +118,6 @@ pub struct ByzantineFault { #[cfg(feature = "debug")] pub(crate) line: u32, } -// I don't like getter methods but in this case they are the only way to implement -// conditionally compiled struct fields without the feature flag causing breaking changes. -impl ByzantineFault { - /// The type of fault that has occurred. Be cautious if you choose to read this - /// value, as an attacker has control over it. - pub fn error(&self) -> FaultType { - self.error - } - /// Some byzantine faults within ZSSP are naturally occurring, i.e. they can occur - /// between two well behaved and trusted parties executing the protocol. - /// This boolean is false if this is one of these faults. If you go to the file and - /// line number specified by this error you will find a comment describing - /// how and why exactly this fault can occur naturally. - /// - /// Faults that can occur because the underlying communication medium is lossy and - /// sequentially inconsistent (as in UDP) are considered naturally occurring. - /// However ZSSP considers faults that occur because data integrity has not been - /// persevered (i.e. bits have been flipped) to be unnatural. - /// ZSSP also considers collisions of what are supposed to be uniform random - /// numbers to be unnatural. - pub fn unnatural(&self) -> bool { - self.unnatural - } - /// The file of this implementation of ZSSP from which this error was generated. - #[cfg(feature = "debug")] - pub fn file(&self) -> &'static str { - self.file - } - /// The line number of this implementation of ZSSP from which this error was - /// generated. As such this number uniquely identifies each possible fault that - /// can occur during ZSSP. Advanced user can use this information to debug more - /// complicated usages of ZSSP. - #[cfg(feature = "debug")] - pub fn line(&self) -> u32 { - self.line - } -} /// An error that occurred during the receipt of a given packet. /// @@ -150,7 +132,7 @@ pub enum ReceiveError { /// Because an unauthenticated remote peer can force these to occur with specific /// contained information, it is recommended in production to either drop these /// immediately, or log them safely to a local output stream and then drop them. - ByzantineFault(ByzantineFault, Option>>), + ByzantineFault(ByzantineFault), /// Rekeying failed and session secret has reached its hard usage count limit. /// The associated session will no longer function and has to be dropped. @@ -171,30 +153,31 @@ pub enum ReceiveError { macro_rules! fault { ($name:expr, $unnatural:ident) => { - ReceiveError::ByzantineFault( - crate::result::ByzantineFault { - #[cfg(feature = "debug")] - file: file!(), - #[cfg(feature = "debug")] - line: line!(), - error: $name, - unnatural: $unnatural, - }, - None, - ) + ReceiveError::ByzantineFault(crate::result::ByzantineFault { + #[cfg(feature = "debug")] + file: file!(), + #[cfg(feature = "debug")] + line: line!(), + error: $name, + unnatural: $unnatural, + session: None, + caused_expiration: false, + }) }; ($name:expr, $unnatural:ident, $session:ident) => { - ReceiveError::ByzantineFault( - crate::result::ByzantineFault { - #[cfg(feature = "debug")] - file: file!(), - #[cfg(feature = "debug")] - line: line!(), - error: $name, - unnatural: $unnatural, - }, - Some($session.clone()), - ) + fault!($name, $unnatural, $session, false) + }; + ($name:expr, $unnatural:ident, $session:ident, $e:ident) => { + ReceiveError::ByzantineFault(crate::result::ByzantineFault { + #[cfg(feature = "debug")] + file: file!(), + #[cfg(feature = "debug")] + line: line!(), + error: $name, + unnatural: $unnatural, + session: Some($session.clone()), + caused_expiration: $e, + }) }; } pub(crate) use fault; @@ -265,8 +248,8 @@ pub enum SessionEvent { DowngradedRatchetKey, } -impl Display for OpenError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { +impl fmt::Display for OpenError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { OpenError::IdentityTooLarge => f.write_str("identity too large"), OpenError::StorageError(e) => e.fmt(f), @@ -275,8 +258,8 @@ impl Display for OpenError { } impl Error for OpenError {} -impl Display for SendError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { +impl fmt::Display for SendError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let str = match self { SendError::MtuTooSmall => "mtu too small", SendError::SessionExpired => "session has expired", @@ -288,8 +271,23 @@ impl Display for SendError { } impl Error for SendError {} -impl Display for FaultType { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { +impl fmt::Debug for ExpiredError +where + Session: fmt::Debug, +{ + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_tuple("ExpiredError").field(&self.0).finish() + } +} +impl fmt::Display for ExpiredError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "session expired") + } +} +impl Error for ExpiredError where Session: fmt::Debug {} + +impl fmt::Display for FaultType { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let str = match self { FaultType::UnknownLocalKeyId => "packet contained an unknown key id", FaultType::InvalidPacket => "invalid packet received", @@ -302,25 +300,57 @@ impl Display for FaultType { } impl Error for FaultType {} -impl Display for ByzantineFault { +// I don't like getter methods but in this case they are the only way to implement +// conditionally compiled struct fields without the feature flag causing breaking changes. +impl ByzantineFault { + /// The file of this implementation of ZSSP from which this error was generated. #[cfg(feature = "debug")] - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + pub fn file(&self) -> &'static str { + self.file + } + /// The line number of this implementation of ZSSP from which this error was + /// generated. As such this number uniquely identifies each possible fault that + /// can occur during ZSSP. Advanced user can use this information to debug more + /// complicated usages of ZSSP. + #[cfg(feature = "debug")] + pub fn line(&self) -> u32 { + self.line + } +} +impl fmt::Debug for ByzantineFault +where + Session: fmt::Debug, +{ + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("ByzantineFault") + .field("session", &self.session) + .field("caused_expiration", &self.caused_expiration) + .field("error", &self.error) + .field("unnatural", &self.unnatural) + .field("file", &self.file) + .field("line", &self.line) + .finish() + } +} +impl fmt::Display for ByzantineFault { + #[cfg(feature = "debug")] + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{} ({}:{})", self.error, self.file, self.line) } #[cfg(not(feature = "debug"))] - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { self.error.fmt(f) } } -impl Error for ByzantineFault {} +impl Error for ByzantineFault where Session: fmt::Debug {} -impl std::fmt::Debug for ReceiveError +impl fmt::Debug for ReceiveError where - Crypto::SessionData: std::fmt::Debug, + Crypto::SessionData: fmt::Debug, { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - Self::ByzantineFault(arg0, arg1) => f.debug_tuple("ByzantineFault").field(arg0).field(arg1).finish(), + Self::ByzantineFault(arg) => f.debug_tuple("ByzantineFault").field(arg).finish(), Self::MaxKeyLifetimeExceeded(arg0) => f.debug_tuple("MaxKeyLifetimeExceeded").field(arg0).finish(), Self::Rejected => f.write_str("Rejected"), Self::StorageError(arg0) => f.debug_tuple("StorageError").field(arg0).finish(), @@ -328,11 +358,10 @@ where } } } - -impl Display for ReceiveError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { +impl fmt::Display for ReceiveError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - ReceiveError::ByzantineFault(e, _) => e.fmt(f), + ReceiveError::ByzantineFault(e) => e.fmt(f), ReceiveError::MaxKeyLifetimeExceeded(_) => f.write_str("max key lifetime exceeded"), ReceiveError::Rejected => f.write_str("attempt to establish session rejected"), ReceiveError::StorageError(e) => e.fmt(f), @@ -340,4 +369,4 @@ impl Display for ReceiveError { } } } -impl Error for ReceiveError where Crypto::SessionData: std::fmt::Debug {} +impl Error for ReceiveError where Crypto::SessionData: fmt::Debug {} diff --git a/performance/src/zeta.rs b/performance/src/zeta.rs index a62cadf..cde05a5 100644 --- a/performance/src/zeta.rs +++ b/performance/src/zeta.rs @@ -594,12 +594,16 @@ pub(crate) fn received_x2_trans= COUNTER_WINDOW_MAX_SKIP_AHEAD || &n[AES_GCM_NONCE_SIZE - 3..] != &x2[x2.len() - 3..] { return Err(fault!(FailedAuth, true, session)); } + + if !matches!(&state.beta, ZetaAutomata::A1(_)) { + return Err(fault!(FailedAuth, true, session)); + } let mut should_warn_missing_ratchet = false; let mut result = (|| { let a1 = if let ZetaAutomata::A1(a1) = &state.beta { a1 } else { - return Err(fault!(FailedAuth, true, session)); + unreachable!(); }; let mut noise = a1.noise.clone(); let mut i = 0; @@ -749,14 +753,15 @@ pub(crate) fn received_x2_trans { + match &mut result { + Err(ReceiveError::ByzantineFault(_)) => { let kex_lock = session.state_machine_lock.lock().unwrap(); let state = session.state.read().unwrap(); let current_time = app.time(); - timeout_trans(app, ctx, session, kex_lock, state, current_time, send); + // We can only reach this point if we are in state A1, and state A1 cannot expire. + debug_assert!(timeout_trans(app, ctx, session, kex_lock, state, current_time, send).is_ok()); } - Ok((ref mut packet, _)) => send(packet, Some(&session.state.read().unwrap().hk_send)), + Ok((packet, _)) => send(packet, Some(&session.state.read().unwrap().hk_send)), _ => {} } result.map(|(_, reduced_service_time)| (should_warn_missing_ratchet, reduced_service_time)) @@ -1047,7 +1052,7 @@ pub(crate) fn received_c1_trans { drop(state); session.expire(); - Err(fault!(ExpiredCounter, true, session)) + Err(fault!(ExpiredCounter, true, session, true)) } Err(false) => Err(fault!(OutOfSequence, true, session)), } @@ -1141,7 +1146,7 @@ pub(crate) fn received_d_trans( Ok(()) } /// Corresponds to the timeout timer Transition Algorithm described in Section 4.1 - Definition 3. -/// Returns `None` if this session should be expired. +/// Returns `Err(())` if this session should be expired. fn timeout_trans>( app: &mut App, ctx: &Arc>, @@ -1150,9 +1155,9 @@ fn timeout_trans>( state: RwLockReadGuard<'_, MutableState>, current_time: i64, send: impl FnOnce(&mut [u8], Option<&Crypto::PrpEnc>), -) -> Option { +) -> Result { match &state.beta { - ZetaAutomata::Null => None, + ZetaAutomata::Null => Err(()), ZetaAutomata::A1(_) | ZetaAutomata::A3 { .. } => { let identity = match &state.beta { ZetaAutomata::A1(a1) => &a1.identity, @@ -1199,7 +1204,7 @@ fn timeout_trans>( drop(kex_lock); send(&mut x1, None); - Some(resend_timer) + Ok(resend_timer) } ZetaAutomata::S2 => { // Corresponds to Transition Algorithm 6 found in Section 4.3. @@ -1245,33 +1250,33 @@ fn timeout_trans>( let state = session.state.read().unwrap(); match send_control(session, &state, PACKET_TYPE_REKEY_INIT, k1, send) { - Err(true) => None, - _ => Some(resend_timer), + Err(true) => Err(()), + _ => Ok(resend_timer), } } ZetaAutomata::S1 { .. } => { log!(app, TimeoutKeyConfirm(session)); - None + Err(()) } ZetaAutomata::R1 { .. } => { log!(app, TimeoutK1(session)); - None + Err(()) } ZetaAutomata::R2 { .. } => { log!(app, TimeoutK2(session)); - None + Err(()) } } } /// Corresponds to the timer rules of the Zeta State Machine found in Section 4.1 - Definition 3. -/// Returns `None` if this session should be expired. +/// Returns `Err(())` if this session should be expired. pub(crate) fn process_timers>( app: &mut App, ctx: &Arc>, session: &Arc>, current_time: i64, send: impl FnOnce(&mut [u8], Option<&Crypto::PrpEnc>), -) -> Option { +) -> Result { let kex_lock = session.state_machine_lock.lock().unwrap(); let state = session.state.read().unwrap(); if state.timeout_timer <= current_time { @@ -1284,16 +1289,16 @@ pub(crate) fn process_timers> // Corresponds to the resend timer rules found in Section 4.1 - Definition 3. let (packet_type, control_payload) = match &state.beta { - ZetaAutomata::Null => return None, + ZetaAutomata::Null => return Err(()), ZetaAutomata::A1(a1) => { log!(app, ResentX1(session)); send(&mut a1.x1.clone(), None); - return Some(resend_next); + return Ok(resend_next); } ZetaAutomata::A3(a3) => { log!(app, ResentX3(session)); send(&mut a3.x3.clone(), Some(&state.hk_send)); - return Some(resend_next); + return Ok(resend_next); } ZetaAutomata::S1 => { log!(app, ResentKeyConfirm(session)); @@ -1301,7 +1306,7 @@ pub(crate) fn process_timers> c1.extend([0u8; HEADER_SIZE]); (PACKET_TYPE_KEY_CONFIRM, c1) } - ZetaAutomata::S2 => return Some(state.timeout_timer), + ZetaAutomata::S2 => return Ok(state.timeout_timer), ZetaAutomata::R1 { k1, .. } => { log!(app, ResentK1(session)); (PACKET_TYPE_REKEY_INIT, k1.clone()) @@ -1313,11 +1318,11 @@ pub(crate) fn process_timers> }; match send_control(session, &state, packet_type, control_payload, send) { - Err(true) => None, - _ => Some(resend_next), + Err(true) => Err(()), + _ => Ok(resend_next), } } else { - Some(ts) + Ok(ts) } } } @@ -1382,7 +1387,7 @@ pub(crate) fn received_k1_trans::new(); k2.extend([0u8; HEADER_SIZE]); @@ -1461,12 +1466,15 @@ pub(crate) fn received_k1_trans Ok(reduced_service_time), Err(false) => Err(fault!(OutOfSequence, true, session)), - Err(true) => Err(fault!(ExpiredCounter, true, session)), + Err(true) => Err(fault!(ExpiredCounter, true, session, true)), } })(); - if matches!(result, Err(ReceiveError::ByzantineFault { .. })) { - session.expire(); + match &result { + Err(ReceiveError::ByzantineFault(fault)) if fault.caused_expiration => { + session.expire(); + } + _ => {} } result } @@ -1493,23 +1501,19 @@ pub(crate) fn received_k2_trans Ok(reduced_service_time), Err(false) => Err(fault!(OutOfSequence, true, session)), - Err(true) => Err(fault!(ExpiredCounter, true, session)), + Err(true) => Err(fault!(ExpiredCounter, true, session, true)), } } else { - unreachable!() + // Some rekey packet may have arrived extremely delayed. + return Err(fault!(OutOfSequence, false, session)); } })(); - if matches!(result, Err(ReceiveError::ByzantineFault { .. })) { - session.expire(); + match &result { + Err(ReceiveError::ByzantineFault(fault)) if fault.caused_expiration => { + session.expire(); + } + _ => {} } result } diff --git a/performance/src/zssp.rs b/performance/src/zssp.rs index 77bff82..b2c3e9a 100644 --- a/performance/src/zssp.rs +++ b/performance/src/zssp.rs @@ -17,7 +17,7 @@ use crate::fragged::Assembled; use crate::handshake_cache::UnassociatedHandshakeCache; use crate::indexed_heap::IndexedBinaryHeap; use crate::proto::*; -use crate::result::{fault, FaultType, OpenError, ReceiveError, ReceiveOk, SendError, SessionEvent}; +use crate::result::{fault, ExpiredError, FaultType, OpenError, ReceiveError, ReceiveOk, SendError, SessionEvent}; use crate::zeta::*; #[cfg(feature = "logging")] use crate::LogEvent::*; @@ -631,9 +631,14 @@ impl Context { /// /// * `app` - Interface to application using ZSSP /// * `send_to` - Function to get a sender and an MTU to send something over an active session - pub fn service>(&self, mut app: App, send_to: impl SendTo) -> i64 { + pub fn service>(&self, mut app: App, mut send_to: impl SendTo) -> i64 { let current_time = app.time(); - let next_service_time = self.service_inner(app, send_to, current_time); + let next_service_time = loop { + match self.service_inner(&mut app, send_to, current_time) { + Ok(ts) => break ts, + Err((_, s)) => send_to = s, + } + }; let max_interval = Crypto::SETTINGS .fragment_assembly_timeout .min(Crypto::SETTINGS.rekey_timeout) @@ -649,21 +654,33 @@ impl Context { /// an `Option`. This option can contain an updated, reduced timestamp at which this /// function ought to be called again. /// + /// If this returns an error then it means that a session has timed-out and has been expired. + /// An expired session is no longer "owned" by the ZSSP context. + /// Therefore it is no longer capable of sending, receiving or being serviced, + /// so it should be dropped. + /// + /// A return type of `Err` effectively means that this function should be called again immediately. + /// This function should be called repeatedly in a loop until `Ok` is returned. + /// /// This function should only be used if the caller has direct access to a scheduler that allows /// them to dynamically modify the interval at which this function is repeatedly called. /// /// * `app` - Interface to application using ZSSP /// * `send_to` - Function to get a sender and an MTU to send something over an active session - pub fn service_scheduled>(&self, mut app: App, send_to: impl SendTo) -> i64 { - let current_time = app.time(); - self.service_inner(app, send_to, current_time) - } - fn service_inner>( + pub fn service_scheduled>( &self, mut app: App, - mut send_to: impl SendTo, + send_to: impl SendTo, + ) -> Result> { + let current_time = app.time(); + self.service_inner(&mut app, send_to, current_time).map_err(|e| e.0) + } + fn service_inner, F: SendTo>( + &self, + app: &mut App, + mut send_to: F, current_time: i64, - ) -> i64 { + ) -> Result, F)> { let ctx = &self.0; let mut session_queue = ctx.session_queue.lock().unwrap(); let mut queue_service_time = i64::MAX; @@ -682,17 +699,18 @@ impl Context { continue; } }; - let result = process_timers(&mut app, ctx, &session, current_time, |packet, hk_send| { + let result = process_timers(app, ctx, &session, current_time, |packet, hk_send| { if let Some((sender, mut mtu)) = send_to.init_send(&session) { mtu = mtu.max(MIN_TRANSPORT_MTU); send_with_fragmentation(sender, mtu, packet, hk_send); } }); - if let Some(next_timer) = result { + if let Ok(next_timer) = result { queue_service_time = queue_service_time.min(next_timer); session_queue.change_priority(queue_idx, Reverse(next_timer)); } else { session.expire_inner(Some(ctx), Some(&mut session_queue)); + return Err((ExpiredError(session), send_to)); } } // This is the only place where `ctx.next_service_time` can be increased. This only works @@ -713,7 +731,7 @@ impl Context { let t2 = defrag_service_time.min(handshake_service_time); let t1 = ctx.next_service_time.fetch_min(t2, Ordering::Relaxed); - t1.min(t2) + Ok(t1.min(t2)) } /// Returns the exact timestamp at which either `Context::service` or /// `Context::service_scheduled` should be called again.