From 515a08f94893d5cade4362d2c0451f94923404ce Mon Sep 17 00:00:00 2001 From: Adam Ierymenko Date: Thu, 5 Jan 2023 16:10:25 -0500 Subject: [PATCH] (1) break out Error for readability, (2) remove NOP packet type since it is no longer useful. --- zssp/src/constants.rs | 5 +- zssp/src/error.rs | 75 +++++++++++++++++ zssp/src/lib.rs | 4 +- zssp/src/zssp.rs | 184 ++++++++++++------------------------------ 4 files changed, 132 insertions(+), 136 deletions(-) create mode 100644 zssp/src/error.rs diff --git a/zssp/src/constants.rs b/zssp/src/constants.rs index 7ff145de5..fc3b6600b 100644 --- a/zssp/src/constants.rs +++ b/zssp/src/constants.rs @@ -79,9 +79,8 @@ pub(crate) const COUNTER_MAX_ALLOWED_OOO: usize = 16; // Packet types can range from 0 to 15 (4 bits) -- 0-3 are defined and 4-15 are reserved for future use pub(crate) const PACKET_TYPE_DATA: u8 = 0; -pub(crate) const PACKET_TYPE_NOP: u8 = 1; -pub(crate) const PACKET_TYPE_INITIAL_KEY_OFFER: u8 = 2; // "alice" -pub(crate) const PACKET_TYPE_KEY_COUNTER_OFFER: u8 = 3; // "bob" +pub(crate) const PACKET_TYPE_INITIAL_KEY_OFFER: u8 = 1; // "alice" +pub(crate) const PACKET_TYPE_KEY_COUNTER_OFFER: u8 = 2; // "bob" // Key usage labels for sub-key derivation using NIST-style KBKDF (basically just HMAC KDF). pub(crate) const KBKDF_KEY_USAGE_LABEL_HMAC: u8 = b'M'; // HMAC-SHA384 authentication for key exchanges diff --git a/zssp/src/error.rs b/zssp/src/error.rs new file mode 100644 index 000000000..70f1f42f6 --- /dev/null +++ b/zssp/src/error.rs @@ -0,0 +1,75 @@ +use crate::sessionid::SessionId; + +pub enum Error { + /// The packet was addressed to an unrecognized local session (should usually be ignored). + UnknownLocalSessionId(SessionId), + + /// Packet was not well formed. + InvalidPacket, + + /// An invalid parameter was supplied to the function. + InvalidParameter, + + /// Packet failed one or more authentication (MAC) checks. + /// + /// **IMPORTANT**: Do not reply to a peer who has sent a packet that has failed authentication. + /// Any response at all will leak to an attacker what authentication step their packet failed at + /// (timing attack), which lowers the total authentication entropy they have to brute force. + /// There is a safe way to reply if absolutely necessary; by sending the reply back after a constant + /// amount of time, but this is very difficult to get correct. + FailedAuthentication, + + /// New session was rejected by the application layer. + NewSessionRejected, + + /// Rekeying failed and session secret has reached its hard usage count limit. + MaxKeyLifetimeExceeded, + + /// Attempt to send using session without established key. + SessionNotEstablished, + + /// Packet ignored by rate limiter. + RateLimited, + + /// The other peer specified an unrecognized protocol version. + UnknownProtocolVersion, + + /// Caller supplied data buffer is too small to receive data. + DataBufferTooSmall, + + /// Data object is too large to send, even with fragmentation. + DataTooLarge, + + /// An unexpected buffer overrun occured while attempting to encode or decode a packet. + /// + /// This can only ever happen if exceptionally large key blobs or metadata are being used, + /// or as the result of an internal encoding bug. + UnexpectedBufferOverrun, +} + +impl std::fmt::Display for Error { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::UnknownLocalSessionId(id) => f.write_str(format!("UnknownLocalSessionId({})", id).as_str()), + Self::InvalidPacket => f.write_str("InvalidPacket"), + Self::InvalidParameter => f.write_str("InvalidParameter"), + Self::FailedAuthentication => f.write_str("FailedAuthentication"), + Self::NewSessionRejected => f.write_str("NewSessionRejected"), + Self::MaxKeyLifetimeExceeded => f.write_str("MaxKeyLifetimeExceeded"), + Self::SessionNotEstablished => f.write_str("SessionNotEstablished"), + Self::RateLimited => f.write_str("RateLimited"), + Self::UnknownProtocolVersion => f.write_str("UnknownProtocolVersion"), + Self::DataBufferTooSmall => f.write_str("DataBufferTooSmall"), + Self::DataTooLarge => f.write_str("DataTooLarge"), + Self::UnexpectedBufferOverrun => f.write_str("UnexpectedBufferOverrun"), + } + } +} + +impl std::error::Error for Error {} + +impl std::fmt::Debug for Error { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + std::fmt::Display::fmt(self, f) + } +} diff --git a/zssp/src/lib.rs b/zssp/src/lib.rs index 99bbed923..ed3068740 100644 --- a/zssp/src/lib.rs +++ b/zssp/src/lib.rs @@ -1,5 +1,6 @@ mod applicationlayer; mod counter; +mod error; mod sessionid; mod tests; mod zssp; @@ -7,5 +8,6 @@ mod zssp; pub mod constants; pub use crate::applicationlayer::ApplicationLayer; +pub use crate::error::Error; pub use crate::sessionid::SessionId; -pub use crate::zssp::{Error, ReceiveContext, ReceiveResult, Session}; +pub use crate::zssp::{ReceiveContext, ReceiveResult, Session}; diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index b34184dd0..93a2efecb 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -22,55 +22,9 @@ use zerotier_utils::varint; use crate::applicationlayer::ApplicationLayer; use crate::constants::*; use crate::counter::{Counter, CounterValue, CounterWindow}; +use crate::error::Error; use crate::sessionid::SessionId; -pub enum Error { - /// The packet was addressed to an unrecognized local session (should usually be ignored). - UnknownLocalSessionId(SessionId), - - /// Packet was not well formed. - InvalidPacket, - - /// An invalid parameter was supplied to the function. - InvalidParameter, - - /// Packet failed one or more authentication (MAC) checks. - /// - /// **IMPORTANT**: Do not reply to a peer who has sent a packet that has failed authentication. - /// Any response at all will leak to an attacker what authentication step their packet failed at - /// (timing attack), which lowers the total authentication entropy they have to brute force. - /// There is a safe way to reply if absolutely necessary; by sending the reply back after a constant - /// amount of time, but this is very difficult to get correct. - FailedAuthentication, - - /// New session was rejected by the application layer. - NewSessionRejected, - - /// Rekeying failed and session secret has reached its hard usage count limit. - MaxKeyLifetimeExceeded, - - /// Attempt to send using session without established key. - SessionNotEstablished, - - /// Packet ignored by rate limiter. - RateLimited, - - /// The other peer specified an unrecognized protocol version. - UnknownProtocolVersion, - - /// Caller supplied data buffer is too small to receive data. - DataBufferTooSmall, - - /// Data object is too large to send, even with fragmentation. - DataTooLarge, - - /// An unexpected buffer overrun occured while attempting to encode or decode a packet. - /// - /// This can only ever happen if exceptionally large key blobs or metadata are being used, - /// or as the result of an internal encoding bug. - UnexpectedBufferOverrun, -} - /// Result generated by the packet receive function, with possible payloads. pub enum ReceiveResult<'a, H: ApplicationLayer> { /// Packet is valid, no action needs to be taken. @@ -99,7 +53,7 @@ pub enum ReceiveResult<'a, H: ApplicationLayer> { /// initiated a session or a rekey event. Initiator is Alice, responder is Bob. /// /// We require that after every rekeying event Alice and Bob switch roles. -#[derive(PartialEq)] +#[derive(PartialEq, Eq)] pub enum Role { Alice, Bob, @@ -136,12 +90,12 @@ pub struct Session { } struct SessionMutableState { - remote_session_id: Option, // The other side's 48-bit session ID - send_counters: [Counter; 2], // Outgoing packet counter and nonce state, starts at 1 - session_keys: [Option; 2], // Buffers to store current, next, and last active key - cur_session_key_id: bool, // Pointer used for keys[] circular buffer - offer: Option, // Most recent ephemeral offer sent to remote - last_remote_offer: i64, // Time of most recent ephemeral offer (ms) + remote_session_id: Option, // The other side's 48-bit session ID + send_counters: [Counter; 2], // Outgoing packet counter and nonce state, starts at 1 + session_keys: [Option; 2], // Buffers to store current and previous session key + cur_session_key_id: bool, // Pointer used for keys[] circular buffer + offer: Option, // Most recent ephemeral offer sent to remote + last_remote_offer: i64, // Time of most recent ephemeral offer (ms) } /// A shared symmetric session key. @@ -184,33 +138,6 @@ struct EphemeralOffer { #[repr(C, packed)] struct CanonicalHeader(pub u64, pub u32); -impl std::fmt::Display for Error { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - Self::UnknownLocalSessionId(id) => f.write_str(format!("UnknownLocalSessionId({})", id).as_str()), - Self::InvalidPacket => f.write_str("InvalidPacket"), - Self::InvalidParameter => f.write_str("InvalidParameter"), - Self::FailedAuthentication => f.write_str("FailedAuthentication"), - Self::NewSessionRejected => f.write_str("NewSessionRejected"), - Self::MaxKeyLifetimeExceeded => f.write_str("MaxKeyLifetimeExceeded"), - Self::SessionNotEstablished => f.write_str("SessionNotEstablished"), - Self::RateLimited => f.write_str("RateLimited"), - Self::UnknownProtocolVersion => f.write_str("UnknownProtocolVersion"), - Self::DataBufferTooSmall => f.write_str("DataBufferTooSmall"), - Self::DataTooLarge => f.write_str("DataTooLarge"), - Self::UnexpectedBufferOverrun => f.write_str("UnexpectedBufferOverrun"), - } - } -} - -impl std::error::Error for Error {} - -impl std::fmt::Debug for Error { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - std::fmt::Display::fmt(self, f) - } -} - impl Session { /// Create a new session and send an initial key offer message to the other end. /// @@ -324,7 +251,7 @@ impl Session { PACKET_TYPE_DATA, remote_session_id.into(), counter, - key_id + key_id, )?; // Get an initialized AES-GCM cipher and re-initialize with a 96-bit IV built from remote session ID, @@ -394,7 +321,12 @@ impl Session { let state = self.state.read().unwrap(); let key_id = state.cur_session_key_id; if let Some(key) = state.session_keys[key_id as usize].as_ref() { - Some((key.secret_fingerprint, key.creation_time, self.ratchet_counts[key_id as usize].load(Ordering::Relaxed), key.jedi)) + Some(( + key.secret_fingerprint, + key.creation_time, + self.ratchet_counts[key_id as usize].load(Ordering::Relaxed), + key.jedi, + )) } else { None } @@ -419,13 +351,16 @@ impl Session { ) { let state = self.state.read().unwrap(); let current_key_id = state.cur_session_key_id; - if (state.session_keys[state.cur_session_key_id as usize] - .as_ref() - .map_or(true, |key| key.role == Role::Bob && (assume_key_is_too_old || key.lifetime.should_rekey(state.send_counters[current_key_id as usize].current(), current_time)))) - && state - .offer - .as_ref() - .map_or(true, |o| (current_time - o.creation_time) > Application::REKEY_RATE_LIMIT_MS) + if (state.session_keys[state.cur_session_key_id as usize].as_ref().map_or(true, |key| { + key.role == Role::Bob + && (assume_key_is_too_old + || key + .lifetime + .should_rekey(state.send_counters[current_key_id as usize].current(), current_time)) + })) && state + .offer + .as_ref() + .map_or(true, |o| (current_time - o.creation_time) > Application::REKEY_RATE_LIMIT_MS) { if let Some(remote_s_public) = P384PublicKey::from_bytes(&self.remote_s_public_p384_bytes) { // This routine handles sending a rekeying packet, resending lost rekeying packets, and resending lost initial offer packets @@ -450,7 +385,11 @@ impl Session { &self.noise_ss, state.session_keys[current_key_id as usize].as_ref(), self.ratchet_counts[current_key_id as usize].load(Ordering::Relaxed), - if has_existing_session { Some(&self.header_check_cipher) } else { None }, + if has_existing_session { + Some(&self.header_check_cipher) + } else { + None + }, mtu, current_time, &mut offer, @@ -573,9 +512,9 @@ impl ReceiveContext { } } else { unlikely_branch(); // We want data receive to be the priority branch, this is only occasionally used - // Salt with a known value so new sessions can be established - // NOTE: this check is trivial to bypass by just replaying recorded packets - // This check isn't security critical so that is fine + // Salt with a known value so new sessions can be established + // NOTE: this check is trivial to bypass by just replaying recorded packets + // This check isn't security critical so that is fine if verify_header_check_code(incoming_packet, 1u64, &self.incoming_init_header_check_cipher) { let canonical_header = CanonicalHeader::make(SessionId::NIL, packet_type, counter); if fragment_count > 1 { @@ -643,12 +582,7 @@ impl ReceiveContext { current_time: i64, ) -> Result, Error> { debug_assert!(fragments.len() >= 1); - - // The first 'if' below should capture both DATA and NOP but not other types. Sanity check this. - debug_assert_eq!(PACKET_TYPE_DATA, 0); - debug_assert_eq!(PACKET_TYPE_NOP, 1); - - if packet_type <= PACKET_TYPE_NOP { + if packet_type == PACKET_TYPE_DATA { if let Some(session) = session { let state = session.state.read().unwrap(); if let Some(session_key) = state.session_keys[key_id as usize].as_ref() { @@ -699,12 +633,7 @@ impl ReceiveContext { if aead_authentication_ok { if session.receive_windows[key_id as usize].message_authenticated(counter) { - if packet_type == PACKET_TYPE_DATA { - return Ok(ReceiveResult::OkData(&mut data_buf[..data_len])); - } else { - unlikely_branch(); - return Ok(ReceiveResult::Ok); - } + return Ok(ReceiveResult::OkData(&mut data_buf[..data_len])); } } } @@ -907,7 +836,7 @@ impl ReceiveContext { state: RwLock::new(SessionMutableState { send_counters: [send_counter, Counter::new()], remote_session_id: Some(alice_session_id), - session_keys: [None, None],//this is the only value which will be writen later + session_keys: [None, None], //this is the only value which will be writen later cur_session_key_id: false, offer: None, last_remote_offer: current_time, @@ -920,7 +849,7 @@ impl ReceiveContext { }), reply_counter, false, - None + None, ) } else { return Err(Error::NewSessionRejected); @@ -1052,18 +981,14 @@ impl ReceiveContext { let packet_end = idx; if session.receive_windows[key_id as usize].message_authenticated(counter) { // Initial key offers should only check this if this is a rekey - let session_key = SessionKey::new( - session_key, - Role::Bob, - current_time, - hybrid_kk.is_some(), - ); + let session_key = SessionKey::new(session_key, Role::Bob, current_time, hybrid_kk.is_some()); let mut state = session.state.write().unwrap(); let _ = state.session_keys[new_key_id as usize].replace(session_key); let _ = state.remote_session_id.replace(alice_session_id); let ratchet_count = session.ratchet_counts[key_id as usize].load(Ordering::SeqCst); - if state.cur_session_key_id != new_key_id {//this prevents anything from being reset twice if the key offer was made twice + if state.cur_session_key_id != new_key_id { + //this prevents anything from being reset twice if the key offer was made twice debug_assert!(new_key_id != key_id); // receive_windows only has race conditions with the counter of the remote party. It is theoretically possible that the local host receives counters under new_key_id while the receive_window is still in the process of resetting, but this is very unlikely. If it does happen, two things could happen: // 1) The received counter is less than what is currently stored in the window, so a valid packet would be rejected. @@ -1091,7 +1016,6 @@ impl ReceiveContext { } else { return Ok(ReceiveResult::Ignored); } - } PACKET_TYPE_KEY_COUNTER_OFFER => { @@ -1147,7 +1071,6 @@ impl ReceiveContext { // Check that this is a counter offer to the original offer we sent. if !(offer.id.eq(offer_id)) { return Ok(ReceiveResult::Ignored); - } // Kyber1024 key agreement if enabled. @@ -1196,12 +1119,7 @@ impl ReceiveContext { if session.receive_windows[key_id as usize].message_authenticated(counter) { // Alice has now completed and validated the full hybrid exchange. - let session_key = SessionKey::new( - session_key, - Role::Alice, - current_time, - hybrid_kk.is_some(), - ); + let session_key = SessionKey::new(session_key, Role::Alice, current_time, hybrid_kk.is_some()); let new_key_id = offer.key_id; drop(state); @@ -1304,7 +1222,11 @@ fn send_ephemeral_offer( } if let Some(current_key) = current_key { idx = safe_write_all(&mut packet_buf, idx, &[0x01])?; - idx = safe_write_all(&mut packet_buf, idx, &public_fingerprint_of_secret(current_key.ratchet_key.as_bytes())[..16])?; + idx = safe_write_all( + &mut packet_buf, + idx, + &public_fingerprint_of_secret(current_key.ratchet_key.as_bytes())[..16], + )?; } else { idx = safe_write_all(&mut packet_buf, idx, &[0x00])?; } @@ -1397,7 +1319,7 @@ fn create_packet_header( packet_type: u8, recipient_session_id: SessionId, counter: CounterValue, - key_id: bool + key_id: bool, ) -> Result<(), Error> { let fragment_count = ((packet_len as f32) / (mtu - HEADER_SIZE) as f32).ceil() as usize; @@ -1417,7 +1339,10 @@ fn create_packet_header( // [112-115]/[14-] packet type (0-15) // [116-121]/[-] number of fragments (0..63 for 1..64 fragments total) // [122-127]/[-15] fragment number (0, 1, 2, ...) - memory::store_raw((counter.to_u32().wrapping_shl(1) | (key_id as u32)).to_le(), &mut header_destination_buffer[4..]); + memory::store_raw( + (counter.to_u32().wrapping_shl(1) | (key_id as u32)).to_le(), + &mut header_destination_buffer[4..], + ); memory::store_raw( (u64::from(recipient_session_id) | (packet_type as u64).wrapping_shl(48) | ((fragment_count - 1) as u64).wrapping_shl(52)) .to_le(), @@ -1536,12 +1461,7 @@ fn parse_dec_key_offer_after_header( impl SessionKey { /// Create a new symmetric shared session key and set its key expiration times, etc. - fn new( - key: Secret<64>, - role: Role, - current_time: i64, - jedi: bool, - ) -> Self { + fn new(key: Secret<64>, role: Role, current_time: i64, jedi: bool) -> Self { let a2b: Secret = kbkdf512(key.as_bytes(), KBKDF_KEY_USAGE_LABEL_AES_GCM_ALICE_TO_BOB).first_n_clone(); let b2a: Secret = kbkdf512(key.as_bytes(), KBKDF_KEY_USAGE_LABEL_AES_GCM_BOB_TO_ALICE).first_n_clone(); let (receive_key, send_key) = match role {