From f4c1aeeeeaaad000ca91d00bb50b688d1de8f8cf Mon Sep 17 00:00:00 2001 From: Monica Moniot Date: Mon, 13 Nov 2023 11:52:12 -0500 Subject: [PATCH] added more debug info --- performance/examples/basic_test.rs | 12 +-- performance/examples/benchmark.rs | 10 +-- performance/src/result.rs | 83 ++++++++++++----- performance/src/zeta.rs | 137 +++++++++++++++-------------- performance/src/zssp.rs | 87 +++++++++--------- 5 files changed, 184 insertions(+), 145 deletions(-) diff --git a/performance/examples/basic_test.rs b/performance/examples/basic_test.rs index 0f8e449..526094a 100644 --- a/performance/examples/basic_test.rs +++ b/performance/examples/basic_test.rs @@ -187,9 +187,6 @@ fn alice_main( pkt, &mut output_data, ) { - Ok((Unassociated, _)) => { - //println!("[alice] ok"); - } Ok((Associated(_, event), _)) => match event { Established => { up = true; @@ -201,9 +198,12 @@ fn alice_main( Control => (), _ => panic!(), }, + Ok(_) => { + //println!("[alice] ok"); + } Err(e) => { println!("[alice] ERROR {:?}", e); - if let ReceiveError::ByzantineFault(e) = e { + if let ReceiveError::ByzantineFault(e, _) = e { assert!(!e.unnatural()) } } @@ -278,7 +278,6 @@ fn bob_main( pkt, &mut output_data, ) { - Ok((Unassociated, _)) => {} Ok((Associated(s, event), _)) => match event { NewSession | NewDowngradedSession => { println!("[bob] new session, took {}s", current_time as f32 / 1000.0); @@ -300,9 +299,10 @@ fn bob_main( Control => (), _ => panic!(), }, + Ok(_) => {} Err(e) => { println!("[bob] ERROR {:?}", e); - if let ReceiveError::ByzantineFault(e) = e { + if let ReceiveError::ByzantineFault(e, _) = e { assert!(!e.unnatural()) } } diff --git a/performance/examples/benchmark.rs b/performance/examples/benchmark.rs index c304d14..415fc8b 100644 --- a/performance/examples/benchmark.rs +++ b/performance/examples/benchmark.rs @@ -156,9 +156,6 @@ fn alice_main( pkt, &mut output_data, ) { - Ok((Unassociated, _)) => { - //println!("[alice] ok"); - } Ok((Associated(_, event), _)) => match event { Established => { up = true; @@ -170,9 +167,10 @@ fn alice_main( Control => (), _ => panic!(), }, + Ok(_) => {} Err(e) => { println!("[alice] ERROR {:?}", e); - if let ReceiveError::ByzantineFault(e) = e { + if let ReceiveError::ByzantineFault(e, _) = e { assert!(!e.unnatural()) } } @@ -238,7 +236,6 @@ fn bob_main( pkt, &mut output_data, ) { - Ok((Unassociated, _)) => {} Ok((Associated(s, event), _)) => match event { NewSession | NewDowngradedSession => { println!("[bob] new session, took {}s", current_time as f32 / 1000.0); @@ -260,9 +257,10 @@ fn bob_main( Control => (), _ => panic!(), }, + Ok(_) => {} Err(e) => { println!("[bob] ERROR {:?}", e); - if let ReceiveError::ByzantineFault(e) = e { + if let ReceiveError::ByzantineFault(e, _) = e { assert!(!e.unnatural()) } } diff --git a/performance/src/result.rs b/performance/src/result.rs index 007d9f2..877ddd9 100644 --- a/performance/src/result.rs +++ b/performance/src/result.rs @@ -138,8 +138,11 @@ impl ByzantineFault { } /// An error that occurred during the receipt of a given packet. -#[derive(Debug)] -pub enum ReceiveError { +/// +/// Keep in mind that when one of these occurs it inherently means that the packet from the remote +/// peer has either not been authenticated or has failed authentication. As such, an attacker could +/// trigger any of these. These errors should only be used for debugging and tracing. +pub enum ReceiveError { /// A type of fault that can occur because a remote peer sent us a bad packet. /// Such packets will be ignored by ZSSP but a user of ZSSP might want to log /// them for debugging or tracing. @@ -147,11 +150,11 @@ 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), + ByzantineFault(ByzantineFault, Option>>), /// Rekeying failed and session secret has reached its hard usage count limit. /// The associated session will no longer function and has to be dropped. - MaxKeyLifetimeExceeded, + MaxKeyLifetimeExceeded(Arc>), /// Either the `ApplicationLayer::incoming_session` or `ApplicationLayer::check_accept_session` /// callback rejected the remote peer's attempt to establish a new session. @@ -163,19 +166,35 @@ pub enum ReceiveError { /// An error was returned by the `output_buffer` passed to receive. /// The received packet was dropped. - WriteError(std::io::Error), + WriteError(std::io::Error, Arc>), } 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, - }) + ReceiveError::ByzantineFault( + crate::result::ByzantineFault { + #[cfg(feature = "debug")] + file: file!(), + #[cfg(feature = "debug")] + line: line!(), + error: $name, + unnatural: $unnatural, + }, + None, + ) + }; + ($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()), + ) }; } pub(crate) use fault; @@ -183,12 +202,17 @@ pub(crate) use fault; /// Result generated by the context packet receive function, with possible payloads. #[derive(Clone)] pub enum ReceiveOk { - /// Packet superficially appeared valid but is not associated with a session yet. + /// The received packet superficially appeared valid but is not associated with a session yet. /// This can occur because the packet was only a fragment of a larger packet, /// or if it was a control packet that does not go through full Noise authentication. Unassociated, - /// Packet was authentic and belongs to this specific session. + /// The received packet was authentic and belongs to this specific session. Associated(Arc>, SessionEvent), + /// The received packet was a fragment of a larger packet. + /// + /// ***The authenticity of this fragment cannot be fully known yet.*** + /// This return value should only be used for debugging and tracing purposes. + Fragment(Arc>), } /// Something that can occur to an associated session when a packet is received successfully, /// including receiving a payload of decrypted, authenticated data. @@ -290,15 +314,30 @@ impl Display for ByzantineFault { } impl Error for ByzantineFault {} -impl Display for ReceiveError { +impl std::fmt::Debug for ReceiveError +where + Crypto::SessionData: std::fmt::Debug, +{ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - 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), - ReceiveError::WriteError(e) => e.fmt(f), + Self::ByzantineFault(arg0, arg1) => f.debug_tuple("ByzantineFault").field(arg0).field(arg1).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(), + Self::WriteError(arg0, arg1) => f.debug_tuple("WriteError").field(arg0).field(arg1).finish(), } } } -impl Error for ReceiveError {} + +impl Display for ReceiveError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + 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), + ReceiveError::WriteError(e, _) => e.fmt(f), + } + } +} +impl Error for ReceiveError where Crypto::SessionData: std::fmt::Debug {} diff --git a/performance/src/zeta.rs b/performance/src/zeta.rs index e6199a8..a62cadf 100644 --- a/performance/src/zeta.rs +++ b/performance/src/zeta.rs @@ -3,7 +3,7 @@ use std::collections::HashMap; use std::io::Write; use std::num::NonZeroU32; use std::ops::{Deref, DerefMut}; -use std::sync::atomic::{AtomicBool, AtomicI64, AtomicU64, Ordering}; +use std::sync::atomic::{AtomicI64, AtomicU64, Ordering}; use std::sync::{Arc, Mutex, MutexGuard, RwLock, RwLockReadGuard, Weak}; use arrayvec::ArrayVec; @@ -425,7 +425,7 @@ pub(crate) fn received_x1_trans), -) -> Result, ReceiveError> { +) -> Result, ReceiveError> { use FaultType::*; // <- s // ... @@ -445,13 +445,13 @@ pub(crate) fn received_x1_trans), -) -> Result<(bool, Option), ReceiveError> { +) -> Result<(bool, Option), ReceiveError> { use FaultType::*; // <- e, ee, ekem1, psk // -> s, se if HANDSHAKE_RESPONSE_SIZE != x2.len() { - return Err(fault!(InvalidPacket, true)); + return Err(fault!(InvalidPacket, true, session)); } let kex_lock = session.state_machine_lock.lock().unwrap(); @@ -588,25 +588,25 @@ 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)); + 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)); + return Err(fault!(FailedAuth, true, session)); }; let mut noise = a1.noise.clone(); let mut i = 0; // Process message pattern 2 e token. let e_remote = noise .read_e_no_init(hash, hmac, &mut i, &x2) - .ok_or(fault!(FailedAuth, true))?; + .ok_or_else(|| fault!(FailedAuth, true, session))?; // Process message pattern 2 ee token. noise.mix_dh(hmac, &a1.e_secret, &e_remote); // Process message pattern 2 ekem1 token. @@ -614,14 +614,14 @@ pub(crate) fn received_x2_trans::new(); x3.extend([0u8; HEADER_SIZE]); @@ -734,7 +734,7 @@ pub(crate) fn received_x2_trans), -) -> Result<(Arc>, bool, Option), ReceiveError> { +) -> Result<(Arc>, bool, Option), ReceiveError> { use FaultType::*; // -> s, se if x3.len() < HANDSHAKE_COMPLETION_MIN_SIZE { @@ -813,7 +813,8 @@ pub(crate) fn received_x3_trans), -) -> Result<(bool, Option), ReceiveError> { +) -> Result<(bool, Option), ReceiveError> { use FaultType::*; if c1.len() != KEY_CONFIRMATION_SIZE { - return Err(fault!(InvalidPacket, true)); + return Err(fault!(InvalidPacket, true, session)); } let kex_lock = session.state_machine_lock.lock().unwrap(); @@ -982,18 +983,18 @@ pub(crate) fn received_c1_trans { drop(state); session.expire(); - Err(fault!(ExpiredCounter, true)) + Err(fault!(ExpiredCounter, true, session)) } - Err(false) => Err(fault!(OutOfSequence, true)), + Err(false) => Err(fault!(OutOfSequence, true, session)), } } /// Corresponds to the trivial Transition Algorithm described for processing C_2 packets found in @@ -1060,11 +1061,11 @@ pub(crate) fn received_c2_trans Result, ReceiveError> { +) -> Result, ReceiveError> { use FaultType::*; if c2.len() != ACKNOWLEDGEMENT_SIZE { - return Err(fault!(InvalidPacket, true)); + return Err(fault!(InvalidPacket, true, session)); } let kex_lock = session.state_machine_lock.lock().unwrap(); @@ -1072,20 +1073,20 @@ pub(crate) fn received_c2_trans( kid: NonZeroU32, n: &[u8; AES_GCM_NONCE_SIZE], d: &[u8], -) -> Result<(), ReceiveError> { +) -> Result<(), ReceiveError> { use FaultType::*; if d.len() != SESSION_REJECTED_SIZE { - return Err(fault!(InvalidPacket, true)); + return Err(fault!(InvalidPacket, true, session)); } let kex_lock = session.state_machine_lock.lock().unwrap(); let state = session.state.read().unwrap(); if Some(kid) != state.key_ref(true).recv.kid || !matches!(&state.beta, ZetaAutomata::A3 { .. }) { - return Err(fault!(OutOfSequence, true)); + return Err(fault!(OutOfSequence, true, session)); } let tag = d[..].try_into().unwrap(); if !Crypto::Aead::decrypt_in_place(state.key_ref(true).recv.kek.as_ref().unwrap(), n, &[], &mut [], tag) { - return Err(fault!(FailedAuth, true)); + return Err(fault!(FailedAuth, true, session)); } let (_, c) = from_nonce(n); if !session.window.update(c) { - return Err(fault!(ExpiredCounter, true)); + return Err(fault!(ExpiredCounter, true, session)); } drop(state); @@ -1329,7 +1330,7 @@ pub(crate) fn received_k1_trans), -) -> Result, ReceiveError> { +) -> Result, ReceiveError> { use FaultType::*; // -> s // <- s @@ -1337,7 +1338,7 @@ pub(crate) fn received_k1_trans psk, e, es, ss // <- e, ee, se if k1.len() != REKEY_SIZE { - return Err(fault!(InvalidPacket, true)); + return Err(fault!(InvalidPacket, true, session)); } let kex_lock = session.state_machine_lock.lock().unwrap(); @@ -1345,7 +1346,7 @@ pub(crate) fn received_k1_trans true, @@ -1354,18 +1355,18 @@ pub(crate) fn received_k1_trans::new(); k2.extend([0u8; HEADER_SIZE]); @@ -1459,8 +1460,8 @@ pub(crate) fn received_k1_trans Ok(reduced_service_time), - Err(false) => Err(fault!(OutOfSequence, true)), - Err(true) => Err(fault!(ExpiredCounter, true)), + Err(false) => Err(fault!(OutOfSequence, true, session)), + Err(true) => Err(fault!(ExpiredCounter, true, session)), } })(); @@ -1478,11 +1479,11 @@ pub(crate) fn received_k2_trans), -) -> Result, ReceiveError> { +) -> Result, ReceiveError> { use FaultType::*; // <- e, ee, se if k2.len() != REKEY_SIZE { - return Err(fault!(InvalidPacket, true)); + return Err(fault!(InvalidPacket, true, session)); } let kex_lock = session.state_machine_lock.lock().unwrap(); @@ -1490,22 +1491,22 @@ pub(crate) fn received_k2_trans Ok(reduced_service_time), - Err(false) => Err(fault!(OutOfSequence, true)), - Err(true) => Err(fault!(ExpiredCounter, true)), + Err(false) => Err(fault!(OutOfSequence, true, session)), + Err(true) => Err(fault!(ExpiredCounter, true, session)), } } else { unreachable!() @@ -1707,7 +1708,7 @@ pub(crate) fn receive_payload_in_place( nonce: &[u8; AES_GCM_NONCE_SIZE], fragments: &mut [Crypto::IncomingPacketBuffer], mut output_buffer: impl Write, -) -> Result<(), ReceiveError> { +) -> Result<(), ReceiveError> { use FaultType::*; debug_assert!(!fragments.is_empty()); @@ -1717,10 +1718,12 @@ pub(crate) fn receive_payload_in_place( state.keys[1].nk.as_ref() } else { // Should be unreachable unless we are leaking kids somewhere. - return Err(fault!(UnknownLocalKeyId, true)); + return Err(fault!(UnknownLocalKeyId, true, session)); }; - let mut cipher = specified_key.ok_or(fault!(OutOfSequence, true))?.start_dec(nonce); + let mut cipher = specified_key + .ok_or_else(|| fault!(OutOfSequence, true, session))? + .start_dec(nonce); let (_, c) = from_nonce(nonce); // NOTE: This only works because we check the size of every received fragment in the receive @@ -1736,25 +1739,25 @@ pub(crate) fn receive_payload_in_place( cipher.decrypt_in_place(&mut fragment[..tag_idx]); if !cipher.finish((&fragment[tag_idx..]).try_into().unwrap()) { - return Err(fault!(FailedAuth, true)); + return Err(fault!(FailedAuth, true, session)); } if !session.window.update(c) { // This error is marked as not happening naturally, but it could occur if something about // the transport protocol is duplicating packets. - return Err(fault!(ExpiredCounter, true)); + return Err(fault!(ExpiredCounter, true, session)); } for i in 0..fragments.len() - 1 { let result = output_buffer.write(&fragments[i].as_ref()[HEADER_SIZE..]); if let Err(e) = result { - return Err(ReceiveError::WriteError(e)); + return Err(ReceiveError::WriteError(e, session.clone())); } } let fragment = &fragments[fragments.len() - 1].as_ref()[HEADER_SIZE..]; let result = output_buffer.write(&fragment[..tag_idx]); if let Err(e) = result { - return Err(ReceiveError::WriteError(e)); + return Err(ReceiveError::WriteError(e, session.clone())); } Ok(()) diff --git a/performance/src/zssp.rs b/performance/src/zssp.rs index 6ae7018..77bff82 100644 --- a/performance/src/zssp.rs +++ b/performance/src/zssp.rs @@ -71,7 +71,9 @@ impl ContextInner { } } -fn parse_fragment_header(incoming_fragment: &[u8]) -> Result<(usize, usize, [u8; AES_GCM_NONCE_SIZE]), ReceiveError> { +fn parse_fragment_header( + incoming_fragment: &[u8], +) -> Result<(usize, usize, [u8; AES_GCM_NONCE_SIZE]), ReceiveError> { let fragment_no = incoming_fragment[FRAGMENT_NO_IDX] as usize; let fragment_count = incoming_fragment[FRAGMENT_COUNT_IDX] as usize; if fragment_no >= fragment_count || fragment_count > MAX_FRAGMENTS { @@ -206,7 +208,7 @@ impl Context { remote_address: &impl Hash, mut incoming_fragment_buf: Crypto::IncomingPacketBuffer, output_buffer: impl Write, - ) -> Result<(ReceiveOk, Option), ReceiveError> { + ) -> Result<(ReceiveOk, Option), ReceiveError> { use crate::result::FaultType::*; let ctx = &self.0; send_unassociated_mtu = send_unassociated_mtu.max(MIN_TRANSPORT_MTU); @@ -234,40 +236,38 @@ impl Context { ); } - { - //vrfy - if packet_type == PACKET_TYPE_HANDSHAKE_RESPONSE { - if !matches!(&state.beta, ZetaAutomata::A1(_)) { - // A resent handshake response from Bob may have arrived out of order, - // after we already received one. - return Err(fault!(OutOfSequence, false)); - } - if incoming_counter >= COUNTER_WINDOW_MAX_SKIP_AHEAD { - return Err(fault!(ExpiredCounter, true)); - } - } else if PACKET_TYPE_USES_COUNTER_RANGE.contains(&packet_type) { - // For DOS resistant reply-protection we need to check that the given counter is - // in the window of valid counters immediately. - // But for packets larger than 1 fragment we can't actually record the - // counter as received until we've authenticated the packet. - // So we check the counter window twice, and only update it the second time - // after the packet has been authenticated. - if !session.window.check(incoming_counter) { - // This can occur naturally if packets arrive way out of order, or - // if they are duplicates. - // This can also be naturally triggered if Bob has just successfully - // received the first session key and is reject all of Alice's resends. - // This can also occur if a session was manually expired, but not - // dropped, and the remote party is still sending us data. - return Err(fault!(ExpiredCounter, false)); - } - } else if packet_type == PACKET_TYPE_HANDSHAKE_COMPLETION { - // This can be triggered if Bob successfully received a session key and - // needs to reject all of Alice's resends of PACKET_TYPE_NOISE_XK_PATTERN_3. - return Err(fault!(InvalidPacket, false)); - } else { - return Err(fault!(InvalidPacket, true)); + //vrfy + if packet_type == PACKET_TYPE_HANDSHAKE_RESPONSE { + if !matches!(&state.beta, ZetaAutomata::A1(_)) { + // A resent handshake response from Bob may have arrived out of order, + // after we already received one. + return Err(fault!(OutOfSequence, false, session)); } + if incoming_counter >= COUNTER_WINDOW_MAX_SKIP_AHEAD { + return Err(fault!(ExpiredCounter, true, session)); + } + } else if PACKET_TYPE_USES_COUNTER_RANGE.contains(&packet_type) { + // For DOS resistant reply-protection we need to check that the given counter is + // in the window of valid counters immediately. + // But for packets larger than 1 fragment we can't actually record the + // counter as received until we've authenticated the packet. + // So we check the counter window twice, and only update it the second time + // after the packet has been authenticated. + if !session.window.check(incoming_counter) { + // This can occur naturally if packets arrive way out of order, or + // if they are duplicates. + // This can also be naturally triggered if Bob has just successfully + // received the first session key and is reject all of Alice's resends. + // This can also occur if a session was manually expired, but not + // dropped, and the remote party is still sending us data. + return Err(fault!(ExpiredCounter, false, session)); + } + } else if packet_type == PACKET_TYPE_HANDSHAKE_COMPLETION { + // This can be triggered if Bob successfully received a session key and + // needs to reject all of Alice's resends of PACKET_TYPE_NOISE_XK_PATTERN_3. + return Err(fault!(InvalidPacket, false, session)); + } else { + return Err(fault!(InvalidPacket, true, session)); } // Handle defragmentation. @@ -282,7 +282,8 @@ impl Context { &mut fragment_buffer, ); if fragment_buffer.is_empty() { - return Ok((ReceiveOk::Unassociated, None)); + drop(state); + return Ok((ReceiveOk::Fragment(session), None)); } else { // We have not yet authenticated the sender so we do not report // receiving a packet from them. @@ -308,12 +309,12 @@ impl Context { &mut fragment_buffer, ); if fragment_buffer.is_empty() { - return Ok((ReceiveOk::Unassociated, None)); + return Ok((ReceiveOk::Fragment(session), None)); } else { for fragment in fragment_buffer.as_ref() { buffer .try_extend_from_slice(&fragment.as_ref()[HEADER_SIZE..]) - .map_err(|_| fault!(InvalidPacket, true))?; + .map_err(|_| fault!(InvalidPacket, true, session))?; } // We have not yet authenticated the sender so we do not report // receiving a packet from them. @@ -407,7 +408,7 @@ impl Context { log!(app, DIsAuthClosedSession(&session)); (SessionEvent::Rejected, None) } - _ => return Err(fault!(InvalidPacket, true)), // This is unreachable. + _ => return Err(fault!(InvalidPacket, true, session)), // This is unreachable. } }; Ok((ReceiveOk::Associated(session, ret.0), ret.1)) @@ -428,11 +429,9 @@ impl Context { ReceivedRawFragment(packet_type, incoming_counter, fragment_no, fragment_count) ); - { - //vrfy - if packet_type != PACKET_TYPE_HANDSHAKE_COMPLETION || incoming_counter != 0 { - return Err(fault!(InvalidPacket, true)); - } + //vrfy + if packet_type != PACKET_TYPE_HANDSHAKE_COMPLETION || incoming_counter != 0 { + return Err(fault!(InvalidPacket, true)); } let mut buffer = ArrayVec::::new();