From 3cc407cecd884f6d3d51938e163b1f182f59878b Mon Sep 17 00:00:00 2001 From: mamoniot Date: Sun, 25 Dec 2022 11:35:07 -0500 Subject: [PATCH 01/34] implemented proper windowing --- zssp/src/counter.rs | 109 ++++++++++++++++++++++++++++++++-------- zssp/src/zssp.rs | 120 ++++++++++++++++++++++---------------------- 2 files changed, 147 insertions(+), 82 deletions(-) diff --git a/zssp/src/counter.rs b/zssp/src/counter.rs index 9aa147670..861fe1f4a 100644 --- a/zssp/src/counter.rs +++ b/zssp/src/counter.rs @@ -1,4 +1,7 @@ -use std::sync::atomic::{AtomicU32, AtomicU64, Ordering}; +use std::sync::{ + atomic::{AtomicU64, Ordering}, + Mutex, RwLock, +}; use zerotier_crypto::random; @@ -23,7 +26,7 @@ impl Counter { /// Get the value most recently used to send a packet. #[inline(always)] pub fn previous(&self) -> CounterValue { - CounterValue(self.0.load(Ordering::SeqCst)) + CounterValue(self.0.load(Ordering::SeqCst).wrapping_sub(1)) } /// Get a counter value for the next packet being sent. @@ -56,33 +59,95 @@ impl CounterValue { } /// Incoming packet deduplication and replay protection window. -pub(crate) struct CounterWindow(AtomicU32, [AtomicU32; COUNTER_MAX_DELTA as usize]); +pub(crate) struct CounterWindowAlt(RwLock<(u32, [u32; COUNTER_MAX_DELTA as usize])>); -impl CounterWindow { +impl CounterWindowAlt { #[inline(always)] pub fn new(initial: u32) -> Self { - Self(AtomicU32::new(initial), std::array::from_fn(|_| AtomicU32::new(initial))) + Self(RwLock::new((initial, std::array::from_fn(|_| initial)))) } #[inline(always)] pub fn message_received(&self, received_counter_value: u32) -> bool { - let prev_max = self.0.fetch_max(received_counter_value, Ordering::AcqRel); - if received_counter_value >= prev_max || prev_max.wrapping_sub(received_counter_value) <= COUNTER_MAX_DELTA { - // First, the most common case: counter is higher than the previous maximum OR is no older than MAX_DELTA. - // In that case we accept the packet if it is not a duplicate. Duplicate check is this swap/compare. - self.1[(received_counter_value % COUNTER_MAX_DELTA) as usize].swap(received_counter_value, Ordering::AcqRel) - != received_counter_value - } else if received_counter_value.wrapping_sub(prev_max) <= COUNTER_MAX_DELTA { - // If the received value is lower and wraps when the previous max is subtracted, this means the - // unsigned integer counter has wrapped. In that case we write the new lower-but-actually-higher "max" - // value and then check the deduplication window. - self.0.store(received_counter_value, Ordering::Release); - self.1[(received_counter_value % COUNTER_MAX_DELTA) as usize].swap(received_counter_value, Ordering::AcqRel) - != received_counter_value - } else { - // If the received value is more than MAX_DELTA in the past and wrapping has NOT occurred, this packet - // is too old and is rejected. - false + let idx = (received_counter_value % COUNTER_MAX_DELTA) as usize; + let data = self.0.read().unwrap(); + let max_counter_seen = data.0; + let lower_window = max_counter_seen.wrapping_sub(COUNTER_MAX_DELTA / 2); + let upper_window = max_counter_seen.wrapping_add(COUNTER_MAX_DELTA / 2); + if lower_window < upper_window { + if (lower_window <= received_counter_value) & (received_counter_value < upper_window) { + if data.1[idx] != received_counter_value { + return true; + } + } + } else if (lower_window <= received_counter_value) | (received_counter_value < upper_window) { + if data.1[idx] != received_counter_value { + return true; + } } + return false; + } + + #[inline(always)] + pub fn message_authenticated(&self, received_counter_value: u32) -> bool { + let idx = (received_counter_value % COUNTER_MAX_DELTA) as usize; + let mut data = self.0.write().unwrap(); + let max_counter_seen = data.0; + let lower_window = max_counter_seen.wrapping_sub(COUNTER_MAX_DELTA / 2); + let upper_window = max_counter_seen.wrapping_add(COUNTER_MAX_DELTA / 2); + if lower_window < upper_window { + if (lower_window <= received_counter_value) & (received_counter_value < upper_window) { + if data.1[idx] != received_counter_value { + data.1[idx] = received_counter_value; + data.0 = max_counter_seen.max(received_counter_value); + return true; + } + } + } else if (lower_window <= received_counter_value) | (received_counter_value < upper_window) { + if data.1[idx] != received_counter_value { + data.1[idx] = received_counter_value; + data.0 = (max_counter_seen as i32).max(received_counter_value as i32) as u32; + return true; + } + } + return false; + } +} + +pub(crate) struct CounterWindow(Mutex<(usize, [u64; COUNTER_MAX_DELTA as usize])>); + +impl CounterWindow { + #[inline(always)] + pub fn new(initial: u32) -> Self { + let initial_nonce = (initial as u64).wrapping_shl(32); + Self(Mutex::new((0, std::array::from_fn(|_| initial_nonce)))) + } + + #[inline(always)] + pub fn message_received(&self, received_counter_value: u32, received_fragment_no: u8) -> bool { + let fragment_nonce = (received_counter_value as u64).wrapping_shl(32) | (received_fragment_no as u64); + //everything past this point must be atomic, i.e. these instructions must be run mutually exclusive to completion; + //atomic instructions are only ever atomic within themselves; + //sequentially consistent atomics do not guarantee that the thread is not preempted between individual atomic instructions + let mut data = self.0.lock().unwrap(); + let mut is_in = false; + let mut is_gt_min = false; + for nonce in data.1 { + is_in |= nonce == fragment_nonce; + let udist = nonce.abs_diff(fragment_nonce); + let sdist = (nonce as i64).abs_diff(fragment_nonce as i64); + if udist < sdist { + is_gt_min |= nonce < fragment_nonce; + } else { + is_gt_min |= (nonce as i64) < (fragment_nonce as i64); + } + } + if !is_in & is_gt_min { + let idx = data.0; + data.1[idx] = fragment_nonce; + data.0 = (idx + 1) % (COUNTER_MAX_DELTA as usize); + return true; + } + return false; } } diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index 1bd92b357..837c81cf8 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -113,6 +113,7 @@ pub struct Session { pub application_data: Application::Data, send_counter: Counter, // Outgoing packet counter and nonce state + receive_window: CounterWindow, // Receive window for anti-replay and deduplication psk: Secret<64>, // Arbitrary PSK provided by external code noise_ss: Secret<48>, // Static raw shared ECDH NIST P-384 key header_check_cipher: Aes, // Cipher used for header check codes (not Noise related) @@ -136,7 +137,6 @@ struct SessionKey { secret_fingerprint: [u8; 16], // First 128 bits of a SHA384 computed from the secret creation_time: i64, // Time session key was established creation_counter: CounterValue, // Counter value at which session was established - receive_window: CounterWindow, // Receive window for anti-replay and deduplication lifetime: KeyLifetime, // Key expiration time and counter ratchet_key: Secret<64>, // Ratchet key for deriving the next session key receive_key: Secret, // Receive side AES-GCM key @@ -486,45 +486,50 @@ impl ReceiveContext { { if let Some(session) = app.lookup_session(local_session_id) { if verify_header_check_code(incoming_packet, &session.header_check_cipher) { - let canonical_header = CanonicalHeader::make(local_session_id, packet_type, counter); - if fragment_count > 1 { - if fragment_count <= (MAX_FRAGMENTS as u8) && fragment_no < fragment_count { - let mut defrag = session.defrag.lock().unwrap(); - let fragment_gather_array = defrag.get_or_create_mut(&counter, || GatherArray::new(fragment_count)); - if let Some(assembled_packet) = fragment_gather_array.add(fragment_no, incoming_packet_buf) { - drop(defrag); // release lock - return self.receive_complete( - app, - remote_address, - &mut send, - data_buf, - counter, - canonical_header.as_bytes(), - assembled_packet.as_ref(), - packet_type, - Some(session), - mtu, - current_time, - ); + if session.receive_window.message_received(counter, fragment_no) { + let canonical_header = CanonicalHeader::make(local_session_id, packet_type, counter); + if fragment_count > 1 { + if fragment_count <= (MAX_FRAGMENTS as u8) && fragment_no < fragment_count { + let mut defrag = session.defrag.lock().unwrap(); + let fragment_gather_array = defrag.get_or_create_mut(&counter, || GatherArray::new(fragment_count)); + if let Some(assembled_packet) = fragment_gather_array.add(fragment_no, incoming_packet_buf) { + drop(defrag); // release lock + return self.receive_complete( + app, + remote_address, + &mut send, + data_buf, + counter, + canonical_header.as_bytes(), + assembled_packet.as_ref(), + packet_type, + Some(session), + mtu, + current_time, + ); + } + } else { + unlikely_branch(); + return Err(Error::InvalidPacket); } } else { - unlikely_branch(); - return Err(Error::InvalidPacket); + return self.receive_complete( + app, + remote_address, + &mut send, + data_buf, + counter, + canonical_header.as_bytes(), + &[incoming_packet_buf], + packet_type, + Some(session), + mtu, + current_time, + ); } } else { - return self.receive_complete( - app, - remote_address, - &mut send, - data_buf, - counter, - canonical_header.as_bytes(), - &[incoming_packet_buf], - packet_type, - Some(session), - mtu, - current_time, - ); + unlikely_branch(); + return Ok(ReceiveResult::Ignored); } } else { unlikely_branch(); @@ -658,36 +663,31 @@ impl ReceiveContext { session_key.return_receive_cipher(c); if aead_authentication_ok { - if session_key.receive_window.message_received(counter) { - // Select this key as the new default if it's newer than the current key. - if p > 0 - && state.session_keys[state.cur_session_key_idx] - .as_ref() - .map_or(true, |old| old.creation_counter < session_key.creation_counter) - { - drop(state); - let mut state = session.state.write().unwrap(); - state.cur_session_key_idx = key_idx; - for i in 0..KEY_HISTORY_SIZE { - if i != key_idx { - if let Some(old_key) = state.session_keys[key_idx].as_ref() { - // Release pooled cipher memory from old keys. - old_key.receive_cipher_pool.lock().unwrap().clear(); - old_key.send_cipher_pool.lock().unwrap().clear(); - } + // Select this key as the new default if it's newer than the current key. + if p > 0 + && state.session_keys[state.cur_session_key_idx] + .as_ref() + .map_or(true, |old| old.creation_counter < session_key.creation_counter) + { + drop(state); + let mut state = session.state.write().unwrap(); + state.cur_session_key_idx = key_idx; + for i in 0..KEY_HISTORY_SIZE { + if i != key_idx { + if let Some(old_key) = state.session_keys[key_idx].as_ref() { + // Release pooled cipher memory from old keys. + old_key.receive_cipher_pool.lock().unwrap().clear(); + old_key.send_cipher_pool.lock().unwrap().clear(); } } } + } - if packet_type == PACKET_TYPE_DATA { - return Ok(ReceiveResult::OkData(&mut data_buf[..data_len])); - } else { - unlikely_branch(); - return Ok(ReceiveResult::Ok); - } + if packet_type == PACKET_TYPE_DATA { + return Ok(ReceiveResult::OkData(&mut data_buf[..data_len])); } else { unlikely_branch(); - return Ok(ReceiveResult::Ignored); + return Ok(ReceiveResult::Ok); } } } From bcf646ecba66b5dc71a1b83528eea5f3a76cad03 Mon Sep 17 00:00:00 2001 From: mamoniot Date: Mon, 26 Dec 2022 18:05:00 -0500 Subject: [PATCH 02/34] reverted to earlier version --- zssp/src/counter.rs | 103 +++++++++++++++++++++++++++++-------- zssp/src/zssp.rs | 120 ++++++++++++++++++++++---------------------- 2 files changed, 141 insertions(+), 82 deletions(-) diff --git a/zssp/src/counter.rs b/zssp/src/counter.rs index 9aa147670..b368b81f2 100644 --- a/zssp/src/counter.rs +++ b/zssp/src/counter.rs @@ -1,4 +1,7 @@ -use std::sync::atomic::{AtomicU32, AtomicU64, Ordering}; +use std::sync::{ + atomic::{AtomicU64, Ordering}, + Mutex, RwLock, +}; use zerotier_crypto::random; @@ -23,7 +26,7 @@ impl Counter { /// Get the value most recently used to send a packet. #[inline(always)] pub fn previous(&self) -> CounterValue { - CounterValue(self.0.load(Ordering::SeqCst)) + CounterValue(self.0.load(Ordering::SeqCst).wrapping_sub(1)) } /// Get a counter value for the next packet being sent. @@ -56,33 +59,89 @@ impl CounterValue { } /// Incoming packet deduplication and replay protection window. -pub(crate) struct CounterWindow(AtomicU32, [AtomicU32; COUNTER_MAX_DELTA as usize]); +pub(crate) struct CounterWindowAlt(RwLock<(u32, [u32; COUNTER_MAX_DELTA as usize])>); -impl CounterWindow { +impl CounterWindowAlt { #[inline(always)] pub fn new(initial: u32) -> Self { - Self(AtomicU32::new(initial), std::array::from_fn(|_| AtomicU32::new(initial))) + Self(RwLock::new((initial, std::array::from_fn(|_| initial)))) } #[inline(always)] pub fn message_received(&self, received_counter_value: u32) -> bool { - let prev_max = self.0.fetch_max(received_counter_value, Ordering::AcqRel); - if received_counter_value >= prev_max || prev_max.wrapping_sub(received_counter_value) <= COUNTER_MAX_DELTA { - // First, the most common case: counter is higher than the previous maximum OR is no older than MAX_DELTA. - // In that case we accept the packet if it is not a duplicate. Duplicate check is this swap/compare. - self.1[(received_counter_value % COUNTER_MAX_DELTA) as usize].swap(received_counter_value, Ordering::AcqRel) - != received_counter_value - } else if received_counter_value.wrapping_sub(prev_max) <= COUNTER_MAX_DELTA { - // If the received value is lower and wraps when the previous max is subtracted, this means the - // unsigned integer counter has wrapped. In that case we write the new lower-but-actually-higher "max" - // value and then check the deduplication window. - self.0.store(received_counter_value, Ordering::Release); - self.1[(received_counter_value % COUNTER_MAX_DELTA) as usize].swap(received_counter_value, Ordering::AcqRel) - != received_counter_value - } else { - // If the received value is more than MAX_DELTA in the past and wrapping has NOT occurred, this packet - // is too old and is rejected. - false + let idx = (received_counter_value % COUNTER_MAX_DELTA) as usize; + let data = self.0.read().unwrap(); + let max_counter_seen = data.0; + let lower_window = max_counter_seen.wrapping_sub(COUNTER_MAX_DELTA / 2); + let upper_window = max_counter_seen.wrapping_add(COUNTER_MAX_DELTA / 2); + if lower_window < upper_window { + if (lower_window <= received_counter_value) & (received_counter_value < upper_window) { + if data.1[idx] != received_counter_value { + return true; + } + } + } else if (lower_window <= received_counter_value) | (received_counter_value < upper_window) { + if data.1[idx] != received_counter_value { + return true; + } } + return false; + } + + #[inline(always)] + pub fn message_authenticated(&self, received_counter_value: u32) -> bool { + let idx = (received_counter_value % COUNTER_MAX_DELTA) as usize; + let mut data = self.0.write().unwrap(); + let max_counter_seen = data.0; + let lower_window = max_counter_seen.wrapping_sub(COUNTER_MAX_DELTA / 2); + let upper_window = max_counter_seen.wrapping_add(COUNTER_MAX_DELTA / 2); + if lower_window < upper_window { + if (lower_window <= received_counter_value) & (received_counter_value < upper_window) { + if data.1[idx] != received_counter_value { + data.1[idx] = received_counter_value; + data.0 = max_counter_seen.max(received_counter_value); + return true; + } + } + } else if (lower_window <= received_counter_value) | (received_counter_value < upper_window) { + if data.1[idx] != received_counter_value { + data.1[idx] = received_counter_value; + data.0 = (max_counter_seen as i32).max(received_counter_value as i32) as u32; + return true; + } + } + return false; + } +} + +pub(crate) struct CounterWindow(Mutex<(usize, [u64; COUNTER_MAX_DELTA as usize])>); + +impl CounterWindow { + #[inline(always)] + pub fn new(initial: u32) -> Self { + let initial_nonce = (initial as u64).wrapping_shl(32); + Self(Mutex::new((0, std::array::from_fn(|_| initial_nonce)))) + } + + #[inline(always)] + pub fn message_received(&self, received_counter_value: u32, received_fragment_no: u8) -> bool { + let fragment_nonce = (received_counter_value as u64).wrapping_shl(6) | (received_fragment_no as u64); + //everything past this point must be atomic, i.e. these instructions must be run mutually exclusive to completion; + //atomic instructions are only ever atomic within themselves; + //sequentially consistent atomics do not guarantee that the thread is not preempted between individual atomic instructions + let mut data = self.0.lock().unwrap(); + let mut is_in = false; + let mut is_gt_min = false; + for nonce in data.1 { + is_in |= nonce == fragment_nonce; + is_gt_min |= nonce.wrapping_sub(fragment_nonce) > 0; + } + if !is_in & is_gt_min { + let idx = data.0; + data.1[idx] = fragment_nonce; + data.0 = (idx + 1) % (COUNTER_MAX_DELTA as usize); + return true; + } + return false; } } diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index 1bd92b357..837c81cf8 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -113,6 +113,7 @@ pub struct Session { pub application_data: Application::Data, send_counter: Counter, // Outgoing packet counter and nonce state + receive_window: CounterWindow, // Receive window for anti-replay and deduplication psk: Secret<64>, // Arbitrary PSK provided by external code noise_ss: Secret<48>, // Static raw shared ECDH NIST P-384 key header_check_cipher: Aes, // Cipher used for header check codes (not Noise related) @@ -136,7 +137,6 @@ struct SessionKey { secret_fingerprint: [u8; 16], // First 128 bits of a SHA384 computed from the secret creation_time: i64, // Time session key was established creation_counter: CounterValue, // Counter value at which session was established - receive_window: CounterWindow, // Receive window for anti-replay and deduplication lifetime: KeyLifetime, // Key expiration time and counter ratchet_key: Secret<64>, // Ratchet key for deriving the next session key receive_key: Secret, // Receive side AES-GCM key @@ -486,45 +486,50 @@ impl ReceiveContext { { if let Some(session) = app.lookup_session(local_session_id) { if verify_header_check_code(incoming_packet, &session.header_check_cipher) { - let canonical_header = CanonicalHeader::make(local_session_id, packet_type, counter); - if fragment_count > 1 { - if fragment_count <= (MAX_FRAGMENTS as u8) && fragment_no < fragment_count { - let mut defrag = session.defrag.lock().unwrap(); - let fragment_gather_array = defrag.get_or_create_mut(&counter, || GatherArray::new(fragment_count)); - if let Some(assembled_packet) = fragment_gather_array.add(fragment_no, incoming_packet_buf) { - drop(defrag); // release lock - return self.receive_complete( - app, - remote_address, - &mut send, - data_buf, - counter, - canonical_header.as_bytes(), - assembled_packet.as_ref(), - packet_type, - Some(session), - mtu, - current_time, - ); + if session.receive_window.message_received(counter, fragment_no) { + let canonical_header = CanonicalHeader::make(local_session_id, packet_type, counter); + if fragment_count > 1 { + if fragment_count <= (MAX_FRAGMENTS as u8) && fragment_no < fragment_count { + let mut defrag = session.defrag.lock().unwrap(); + let fragment_gather_array = defrag.get_or_create_mut(&counter, || GatherArray::new(fragment_count)); + if let Some(assembled_packet) = fragment_gather_array.add(fragment_no, incoming_packet_buf) { + drop(defrag); // release lock + return self.receive_complete( + app, + remote_address, + &mut send, + data_buf, + counter, + canonical_header.as_bytes(), + assembled_packet.as_ref(), + packet_type, + Some(session), + mtu, + current_time, + ); + } + } else { + unlikely_branch(); + return Err(Error::InvalidPacket); } } else { - unlikely_branch(); - return Err(Error::InvalidPacket); + return self.receive_complete( + app, + remote_address, + &mut send, + data_buf, + counter, + canonical_header.as_bytes(), + &[incoming_packet_buf], + packet_type, + Some(session), + mtu, + current_time, + ); } } else { - return self.receive_complete( - app, - remote_address, - &mut send, - data_buf, - counter, - canonical_header.as_bytes(), - &[incoming_packet_buf], - packet_type, - Some(session), - mtu, - current_time, - ); + unlikely_branch(); + return Ok(ReceiveResult::Ignored); } } else { unlikely_branch(); @@ -658,36 +663,31 @@ impl ReceiveContext { session_key.return_receive_cipher(c); if aead_authentication_ok { - if session_key.receive_window.message_received(counter) { - // Select this key as the new default if it's newer than the current key. - if p > 0 - && state.session_keys[state.cur_session_key_idx] - .as_ref() - .map_or(true, |old| old.creation_counter < session_key.creation_counter) - { - drop(state); - let mut state = session.state.write().unwrap(); - state.cur_session_key_idx = key_idx; - for i in 0..KEY_HISTORY_SIZE { - if i != key_idx { - if let Some(old_key) = state.session_keys[key_idx].as_ref() { - // Release pooled cipher memory from old keys. - old_key.receive_cipher_pool.lock().unwrap().clear(); - old_key.send_cipher_pool.lock().unwrap().clear(); - } + // Select this key as the new default if it's newer than the current key. + if p > 0 + && state.session_keys[state.cur_session_key_idx] + .as_ref() + .map_or(true, |old| old.creation_counter < session_key.creation_counter) + { + drop(state); + let mut state = session.state.write().unwrap(); + state.cur_session_key_idx = key_idx; + for i in 0..KEY_HISTORY_SIZE { + if i != key_idx { + if let Some(old_key) = state.session_keys[key_idx].as_ref() { + // Release pooled cipher memory from old keys. + old_key.receive_cipher_pool.lock().unwrap().clear(); + old_key.send_cipher_pool.lock().unwrap().clear(); } } } + } - if packet_type == PACKET_TYPE_DATA { - return Ok(ReceiveResult::OkData(&mut data_buf[..data_len])); - } else { - unlikely_branch(); - return Ok(ReceiveResult::Ok); - } + if packet_type == PACKET_TYPE_DATA { + return Ok(ReceiveResult::OkData(&mut data_buf[..data_len])); } else { unlikely_branch(); - return Ok(ReceiveResult::Ignored); + return Ok(ReceiveResult::Ok); } } } From eee16167dffdf8f53fbe233529477631e5f10ad1 Mon Sep 17 00:00:00 2001 From: mamoniot Date: Mon, 26 Dec 2022 21:07:32 -0500 Subject: [PATCH 03/34] finished testing new counter window --- zssp/src/constants.rs | 1 + zssp/src/counter.rs | 54 ++++++++++++++++++++++++++----------- zssp/src/tests.rs | 62 ++++++++++++++++++++++++++++++++++++++----- zssp/src/zssp.rs | 7 +++-- 4 files changed, 97 insertions(+), 27 deletions(-) diff --git a/zssp/src/constants.rs b/zssp/src/constants.rs index 523b6692e..b2f03a3d1 100644 --- a/zssp/src/constants.rs +++ b/zssp/src/constants.rs @@ -79,6 +79,7 @@ pub(crate) const KEY_HISTORY_SIZE: usize = 3; /// Maximum difference between out-of-order incoming packet counters, and size of deduplication buffer. pub(crate) const COUNTER_MAX_DELTA: u32 = 16; +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; diff --git a/zssp/src/counter.rs b/zssp/src/counter.rs index b368b81f2..e200b9015 100644 --- a/zssp/src/counter.rs +++ b/zssp/src/counter.rs @@ -5,7 +5,7 @@ use std::sync::{ use zerotier_crypto::random; -use crate::constants::COUNTER_MAX_DELTA; +use crate::constants::{COUNTER_MAX_DELTA, COUNTER_MAX_ALLOWED_OOO}; /// Outgoing packet counter with strictly ordered atomic semantics. /// @@ -114,34 +114,56 @@ impl CounterWindowAlt { } } -pub(crate) struct CounterWindow(Mutex<(usize, [u64; COUNTER_MAX_DELTA as usize])>); +pub(crate) struct CounterWindow(Mutex<(usize, [u64; COUNTER_MAX_ALLOWED_OOO as usize])>); impl CounterWindow { #[inline(always)] pub fn new(initial: u32) -> Self { + // we want our nonces to wrap at the exact same time that the counter wraps, so we shift them up to the u64 boundary let initial_nonce = (initial as u64).wrapping_shl(32); - Self(Mutex::new((0, std::array::from_fn(|_| initial_nonce)))) + Self(Mutex::new((0, [initial_nonce; COUNTER_MAX_ALLOWED_OOO as usize]))) } + #[inline(always)] + pub fn new_uninit() -> Self { + Self(Mutex::new((usize::MAX, [0; COUNTER_MAX_ALLOWED_OOO as usize]))) + } + #[inline(always)] + pub fn init(&self, initial: u32) { + let initial_nonce = (initial as u64).wrapping_shl(6); + let mut data = self.0.lock().unwrap(); + data.0 = 0; + data.1 = [initial_nonce; COUNTER_MAX_ALLOWED_OOO as usize]; + } + #[inline(always)] pub fn message_received(&self, received_counter_value: u32, received_fragment_no: u8) -> bool { - let fragment_nonce = (received_counter_value as u64).wrapping_shl(6) | (received_fragment_no as u64); + let fragment_nonce = (received_counter_value as u64).wrapping_shl(32) | (received_fragment_no as u64); //everything past this point must be atomic, i.e. these instructions must be run mutually exclusive to completion; //atomic instructions are only ever atomic within themselves; //sequentially consistent atomics do not guarantee that the thread is not preempted between individual atomic instructions let mut data = self.0.lock().unwrap(); - let mut is_in = false; - let mut is_gt_min = false; - for nonce in data.1 { - is_in |= nonce == fragment_nonce; - is_gt_min |= nonce.wrapping_sub(fragment_nonce) > 0; + if data.0 == usize::MAX { + return true + } else { + const NONCE_MAX_DELTA: i64 = (2*COUNTER_MAX_ALLOWED_OOO as i64).wrapping_shl(32); + let mut is_in = false; + let mut idx = 0; + let mut smallest = fragment_nonce; + for i in 0..data.1.len() { + let nonce = data.1[i]; + is_in |= nonce == fragment_nonce; + let delta = (smallest as i64).wrapping_sub(nonce as i64); + if delta > 0 { + smallest = nonce; + idx = i; + } + } + if !is_in & (smallest != fragment_nonce) & ((fragment_nonce as i64).wrapping_sub(smallest as i64) < NONCE_MAX_DELTA) { + data.1[idx] = fragment_nonce; + return true + } + return false } - if !is_in & is_gt_min { - let idx = data.0; - data.1[idx] = fragment_nonce; - data.0 = (idx + 1) % (COUNTER_MAX_DELTA as usize); - return true; - } - return false; } } diff --git a/zssp/src/tests.rs b/zssp/src/tests.rs index 45de756df..a58d23498 100644 --- a/zssp/src/tests.rs +++ b/zssp/src/tests.rs @@ -218,14 +218,62 @@ mod tests { } } + + #[inline(always)] + pub fn xorshift64(x: &mut u64) -> u32 { + *x ^= x.wrapping_shl(13); + *x ^= x.wrapping_shr(7); + *x ^= x.wrapping_shl(17); + *x as u32 + } #[test] fn counter_window() { - let w = CounterWindow::new(0xffffffff); - assert!(!w.message_received(0xffffffff)); - assert!(w.message_received(0)); - assert!(w.message_received(1)); - assert!(w.message_received(COUNTER_MAX_DELTA * 2)); - assert!(!w.message_received(0xffffffff)); - assert!(w.message_received(0xfffffffe)); + let sqrt_out_of_order_max = (COUNTER_MAX_ALLOWED_OOO as f32).sqrt() as u32; + let mut rng = 8234; + let mut counter = u32::MAX - 16; + let mut fragment_no: u8 = 0; + let mut history = Vec::<(u32, u8)>::new(); + + let mut w = CounterWindow::new(counter.wrapping_sub(1)); + for i in 1..1000000 { + let p = xorshift64(&mut rng)%1000; + let c; + let f; + if p < 250 { + let r = xorshift64(&mut rng); + c = counter.wrapping_add(r%sqrt_out_of_order_max); + f = fragment_no + 1 + ((r/sqrt_out_of_order_max)%sqrt_out_of_order_max) as u8; + } else if p < 500 { + if history.len() > 0 { + let idx = xorshift64(&mut rng) as usize%history.len(); + let (c, f) = history[idx]; + assert!(!w.message_received(c, f)); + } + continue; + } else if p < 750 { + fragment_no = u8::min(fragment_no + 1, 63); + c = counter; + f = fragment_no; + } else if p < 999 { + counter = counter.wrapping_add(1); + fragment_no = 0; + c = counter; + f = fragment_no; + } else { + //simulate rekeying + counter = xorshift64(&mut rng); + fragment_no = 0; + w = CounterWindow::new(counter.wrapping_sub(1)); + history = Vec::<(u32, u8)>::new(); + c = counter; + f = fragment_no; + } + if history.contains(&(c, f)) { + assert!(!w.message_received(c, f)); + } else { + assert!(w.message_received(c, f)); + history.push((c, f)); + } + } } } diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index 837c81cf8..f9e56972d 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -254,6 +254,7 @@ impl Session { id: local_session_id, application_data, send_counter, + receive_window: CounterWindow::new_uninit(),//alice does not know bob's counter yet psk: psk.clone(), noise_ss, header_check_cipher, @@ -879,6 +880,7 @@ impl ReceiveContext { id: new_session_id, application_data: associated_object, send_counter: Counter::new(), + receive_window: CounterWindow::new(counter), psk, noise_ss, header_check_cipher, @@ -1030,7 +1032,6 @@ impl ReceiveContext { Role::Bob, current_time, reply_counter, - counter, last_ratchet_count + 1, hybrid_kk.is_some(), ); @@ -1154,10 +1155,10 @@ impl ReceiveContext { Role::Alice, current_time, reply_counter, - counter, last_ratchet_count + 1, hybrid_kk.is_some(), ); + session.receive_window.init(counter); //////////////////////////////////////////////////////////////// // packet encoding for post-noise session start ack @@ -1497,7 +1498,6 @@ impl SessionKey { role: Role, current_time: i64, current_counter: CounterValue, - remote_counter: u32, ratchet_count: u64, jedi: bool, ) -> Self { @@ -1511,7 +1511,6 @@ impl SessionKey { secret_fingerprint: public_fingerprint_of_secret(key.as_bytes())[..16].try_into().unwrap(), creation_time: current_time, creation_counter: current_counter, - receive_window: CounterWindow::new(remote_counter), lifetime: KeyLifetime::new(current_counter, current_time), ratchet_key: kbkdf512(key.as_bytes(), KBKDF_KEY_USAGE_LABEL_RATCHETING), receive_key, From 46a0c4874566833286d8fb8c230fb66b56a499e0 Mon Sep 17 00:00:00 2001 From: mamoniot Date: Mon, 26 Dec 2022 21:17:21 -0500 Subject: [PATCH 04/34] implemented uninit --- zssp/src/counter.rs | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/zssp/src/counter.rs b/zssp/src/counter.rs index e200b9015..af0b8f2eb 100644 --- a/zssp/src/counter.rs +++ b/zssp/src/counter.rs @@ -114,25 +114,24 @@ impl CounterWindowAlt { } } -pub(crate) struct CounterWindow(Mutex<(usize, [u64; COUNTER_MAX_ALLOWED_OOO as usize])>); +pub(crate) struct CounterWindow(Mutex>); impl CounterWindow { #[inline(always)] pub fn new(initial: u32) -> Self { // we want our nonces to wrap at the exact same time that the counter wraps, so we shift them up to the u64 boundary let initial_nonce = (initial as u64).wrapping_shl(32); - Self(Mutex::new((0, [initial_nonce; COUNTER_MAX_ALLOWED_OOO as usize]))) + Self(Mutex::new(Some([initial_nonce; COUNTER_MAX_ALLOWED_OOO as usize]))) } #[inline(always)] pub fn new_uninit() -> Self { - Self(Mutex::new((usize::MAX, [0; COUNTER_MAX_ALLOWED_OOO as usize]))) + Self(Mutex::new(None)) } #[inline(always)] pub fn init(&self, initial: u32) { let initial_nonce = (initial as u64).wrapping_shl(6); let mut data = self.0.lock().unwrap(); - data.0 = 0; - data.1 = [initial_nonce; COUNTER_MAX_ALLOWED_OOO as usize]; + *data = Some([initial_nonce; COUNTER_MAX_ALLOWED_OOO as usize]); } @@ -142,16 +141,12 @@ impl CounterWindow { //everything past this point must be atomic, i.e. these instructions must be run mutually exclusive to completion; //atomic instructions are only ever atomic within themselves; //sequentially consistent atomics do not guarantee that the thread is not preempted between individual atomic instructions - let mut data = self.0.lock().unwrap(); - if data.0 == usize::MAX { - return true - } else { - const NONCE_MAX_DELTA: i64 = (2*COUNTER_MAX_ALLOWED_OOO as i64).wrapping_shl(32); + if let Some(history) = self.0.lock().unwrap().as_mut() { let mut is_in = false; let mut idx = 0; let mut smallest = fragment_nonce; - for i in 0..data.1.len() { - let nonce = data.1[i]; + for i in 0..history.len() { + let nonce = history[i]; is_in |= nonce == fragment_nonce; let delta = (smallest as i64).wrapping_sub(nonce as i64); if delta > 0 { @@ -159,11 +154,13 @@ impl CounterWindow { idx = i; } } - if !is_in & (smallest != fragment_nonce) & ((fragment_nonce as i64).wrapping_sub(smallest as i64) < NONCE_MAX_DELTA) { - data.1[idx] = fragment_nonce; + if !is_in & (smallest != fragment_nonce) { + history[idx] = fragment_nonce; return true } return false + } else { + return true } } } From 4b6bf0a4adc034c0cfcfd55e3645f73d61d76d33 Mon Sep 17 00:00:00 2001 From: mamoniot Date: Mon, 26 Dec 2022 21:34:09 -0500 Subject: [PATCH 05/34] improved test --- zssp/src/counter.rs | 3 ++- zssp/src/tests.rs | 17 ++++++++++++----- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/zssp/src/counter.rs b/zssp/src/counter.rs index af0b8f2eb..66b215ed0 100644 --- a/zssp/src/counter.rs +++ b/zssp/src/counter.rs @@ -142,6 +142,7 @@ impl CounterWindow { //atomic instructions are only ever atomic within themselves; //sequentially consistent atomics do not guarantee that the thread is not preempted between individual atomic instructions if let Some(history) = self.0.lock().unwrap().as_mut() { + const NONCE_MAX_DELTA: i64 = (2*COUNTER_MAX_ALLOWED_OOO as i64).wrapping_shl(32); let mut is_in = false; let mut idx = 0; let mut smallest = fragment_nonce; @@ -154,7 +155,7 @@ impl CounterWindow { idx = i; } } - if !is_in & (smallest != fragment_nonce) { + if !is_in & (smallest != fragment_nonce) & ((fragment_nonce as i64).wrapping_sub(smallest as i64) < NONCE_MAX_DELTA) { history[idx] = fragment_nonce; return true } diff --git a/zssp/src/tests.rs b/zssp/src/tests.rs index a58d23498..07a1375e1 100644 --- a/zssp/src/tests.rs +++ b/zssp/src/tests.rs @@ -229,7 +229,7 @@ mod tests { #[test] fn counter_window() { let sqrt_out_of_order_max = (COUNTER_MAX_ALLOWED_OOO as f32).sqrt() as u32; - let mut rng = 8234; + let mut rng = 1027;//8234 let mut counter = u32::MAX - 16; let mut fragment_no: u8 = 0; let mut history = Vec::<(u32, u8)>::new(); @@ -239,26 +239,33 @@ mod tests { let p = xorshift64(&mut rng)%1000; let c; let f; - if p < 250 { + if p < 200 { let r = xorshift64(&mut rng); c = counter.wrapping_add(r%sqrt_out_of_order_max); f = fragment_no + 1 + ((r/sqrt_out_of_order_max)%sqrt_out_of_order_max) as u8; - } else if p < 500 { + } else if p < 400 { if history.len() > 0 { let idx = xorshift64(&mut rng) as usize%history.len(); let (c, f) = history[idx]; assert!(!w.message_received(c, f)); } continue; - } else if p < 750 { + } else if p < 650 { fragment_no = u8::min(fragment_no + 1, 63); c = counter; f = fragment_no; - } else if p < 999 { + } else if p < 900 { counter = counter.wrapping_add(1); fragment_no = 0; c = counter; f = fragment_no; + } else if p < 999 { + c = xorshift64(&mut rng); + f = (xorshift64(&mut rng)%64) as u8; + if w.message_received(c, f) { + history.push((c, f)); + } + continue; } else { //simulate rekeying counter = xorshift64(&mut rng); From 2c07136b5ea977a8cf183fd3abce522c3b653adf Mon Sep 17 00:00:00 2001 From: mamoniot Date: Mon, 26 Dec 2022 23:05:59 -0500 Subject: [PATCH 06/34] fixed test --- zssp/src/counter.rs | 27 +++++++++++++++++++++++++-- zssp/src/tests.rs | 39 +++++++++++++++++++-------------------- 2 files changed, 44 insertions(+), 22 deletions(-) diff --git a/zssp/src/counter.rs b/zssp/src/counter.rs index 66b215ed0..45d8baf22 100644 --- a/zssp/src/counter.rs +++ b/zssp/src/counter.rs @@ -142,7 +142,6 @@ impl CounterWindow { //atomic instructions are only ever atomic within themselves; //sequentially consistent atomics do not guarantee that the thread is not preempted between individual atomic instructions if let Some(history) = self.0.lock().unwrap().as_mut() { - const NONCE_MAX_DELTA: i64 = (2*COUNTER_MAX_ALLOWED_OOO as i64).wrapping_shl(32); let mut is_in = false; let mut idx = 0; let mut smallest = fragment_nonce; @@ -155,7 +154,7 @@ impl CounterWindow { idx = i; } } - if !is_in & (smallest != fragment_nonce) & ((fragment_nonce as i64).wrapping_sub(smallest as i64) < NONCE_MAX_DELTA) { + if !is_in & (smallest != fragment_nonce) { history[idx] = fragment_nonce; return true } @@ -164,4 +163,28 @@ impl CounterWindow { return true } } + + #[inline(always)] + pub fn purge(&self, inauthentic_counter_value: u32, inauthentic_fragment_no: u8) { + let inauthentic_nonce = (inauthentic_counter_value as u64).wrapping_shl(32) | (inauthentic_fragment_no as u64); + //everything past this point must be atomic, i.e. these instructions must be run mutually exclusive to completion; + //atomic instructions are only ever atomic within themselves; + //sequentially consistent atomics do not guarantee that the thread is not preempted between individual atomic instructions + if let Some(history) = self.0.lock().unwrap().as_mut() { + let mut idx = 0; + let mut smallest = history[0]; + for i in 0..history.len() { + let nonce = history[i]; + if nonce == inauthentic_nonce { + idx = i; + } else { + let delta = (smallest as i64).wrapping_sub(nonce as i64); + if delta > 0 { + smallest = nonce; + } + } + } + history[idx] = smallest; + } + } } diff --git a/zssp/src/tests.rs b/zssp/src/tests.rs index 07a1375e1..3e23f537f 100644 --- a/zssp/src/tests.rs +++ b/zssp/src/tests.rs @@ -228,42 +228,41 @@ mod tests { } #[test] fn counter_window() { - let sqrt_out_of_order_max = (COUNTER_MAX_ALLOWED_OOO as f32).sqrt() as u32; - let mut rng = 1027;//8234 + let mut rng = 415263; let mut counter = u32::MAX - 16; let mut fragment_no: u8 = 0; let mut history = Vec::<(u32, u8)>::new(); let mut w = CounterWindow::new(counter.wrapping_sub(1)); - for i in 1..1000000 { - let p = xorshift64(&mut rng)%1000; + for i in 0..1000000 { + let p = xorshift64(&mut rng) as f32/(u32::MAX as f32 + 1.0); let c; let f; - if p < 200 { + if p < 0.5 { let r = xorshift64(&mut rng); - c = counter.wrapping_add(r%sqrt_out_of_order_max); - f = fragment_no + 1 + ((r/sqrt_out_of_order_max)%sqrt_out_of_order_max) as u8; - } else if p < 400 { + c = counter.wrapping_add(r%(COUNTER_MAX_ALLOWED_OOO - 2) as u32 + 1); + f = 0; + } else if p < 0.7 { + fragment_no = u8::min(fragment_no + 1, 63); + c = counter; + f = fragment_no; + } else if p < 0.8 { + counter = counter.wrapping_add(1); + fragment_no = 0; + c = counter; + f = fragment_no; + } else if p < 0.9 { if history.len() > 0 { let idx = xorshift64(&mut rng) as usize%history.len(); let (c, f) = history[idx]; assert!(!w.message_received(c, f)); } continue; - } else if p < 650 { - fragment_no = u8::min(fragment_no + 1, 63); - c = counter; - f = fragment_no; - } else if p < 900 { - counter = counter.wrapping_add(1); - fragment_no = 0; - c = counter; - f = fragment_no; - } else if p < 999 { - c = xorshift64(&mut rng); + } else if p < 0.9995 { + c = counter.wrapping_add(xorshift64(&mut rng)%999); f = (xorshift64(&mut rng)%64) as u8; if w.message_received(c, f) { - history.push((c, f)); + w.purge(c, f); } continue; } else { From e445088cf211cdcb981991152007a31312b7b5e4 Mon Sep 17 00:00:00 2001 From: mamoniot Date: Mon, 26 Dec 2022 23:11:08 -0500 Subject: [PATCH 07/34] fixed test --- zssp/src/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zssp/src/tests.rs b/zssp/src/tests.rs index 3e23f537f..dd95fae2e 100644 --- a/zssp/src/tests.rs +++ b/zssp/src/tests.rs @@ -228,7 +228,7 @@ mod tests { } #[test] fn counter_window() { - let mut rng = 415263; + let mut rng = 84632; let mut counter = u32::MAX - 16; let mut fragment_no: u8 = 0; let mut history = Vec::<(u32, u8)>::new(); @@ -259,7 +259,7 @@ mod tests { } continue; } else if p < 0.9995 { - c = counter.wrapping_add(xorshift64(&mut rng)%999); + c = xorshift64(&mut rng); f = (xorshift64(&mut rng)%64) as u8; if w.message_received(c, f) { w.purge(c, f); From fb20bbc5384dfbad0b5892f8069a34d74d8b0b23 Mon Sep 17 00:00:00 2001 From: mamoniot Date: Tue, 27 Dec 2022 10:22:30 -0500 Subject: [PATCH 08/34] implemented unfinished architecture --- zssp/src/constants.rs | 1 - zssp/src/counter.rs | 146 +++++++++--------------------------------- zssp/src/tests.rs | 42 ++++-------- zssp/src/zssp.rs | 48 +++++++------- 4 files changed, 68 insertions(+), 169 deletions(-) diff --git a/zssp/src/constants.rs b/zssp/src/constants.rs index b2f03a3d1..bd632f8f3 100644 --- a/zssp/src/constants.rs +++ b/zssp/src/constants.rs @@ -78,7 +78,6 @@ pub(crate) const SESSION_ID_SIZE: usize = 6; pub(crate) const KEY_HISTORY_SIZE: usize = 3; /// Maximum difference between out-of-order incoming packet counters, and size of deduplication buffer. -pub(crate) const COUNTER_MAX_DELTA: u32 = 16; 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 diff --git a/zssp/src/counter.rs b/zssp/src/counter.rs index 45d8baf22..828785220 100644 --- a/zssp/src/counter.rs +++ b/zssp/src/counter.rs @@ -1,11 +1,10 @@ -use std::sync::{ - atomic::{AtomicU64, Ordering}, - Mutex, RwLock, -}; +use std::{sync::{ + atomic::{AtomicU64, Ordering, AtomicU32, AtomicI32, AtomicBool} +}, mem}; use zerotier_crypto::random; -use crate::constants::{COUNTER_MAX_DELTA, COUNTER_MAX_ALLOWED_OOO}; +use crate::constants::COUNTER_MAX_ALLOWED_OOO; /// Outgoing packet counter with strictly ordered atomic semantics. /// @@ -20,7 +19,7 @@ impl Counter { pub fn new() -> Self { // Using a random value has no security implication. Zero would be fine. This just // helps randomize packet contents a bit. - Self(AtomicU64::new(random::next_u32_secure() as u64)) + Self(AtomicU64::new((random::next_u32_secure()/2) as u64)) } /// Get the value most recently used to send a packet. @@ -59,132 +58,51 @@ impl CounterValue { } /// Incoming packet deduplication and replay protection window. -pub(crate) struct CounterWindowAlt(RwLock<(u32, [u32; COUNTER_MAX_DELTA as usize])>); - -impl CounterWindowAlt { - #[inline(always)] - pub fn new(initial: u32) -> Self { - Self(RwLock::new((initial, std::array::from_fn(|_| initial)))) - } - - #[inline(always)] - pub fn message_received(&self, received_counter_value: u32) -> bool { - let idx = (received_counter_value % COUNTER_MAX_DELTA) as usize; - let data = self.0.read().unwrap(); - let max_counter_seen = data.0; - let lower_window = max_counter_seen.wrapping_sub(COUNTER_MAX_DELTA / 2); - let upper_window = max_counter_seen.wrapping_add(COUNTER_MAX_DELTA / 2); - if lower_window < upper_window { - if (lower_window <= received_counter_value) & (received_counter_value < upper_window) { - if data.1[idx] != received_counter_value { - return true; - } - } - } else if (lower_window <= received_counter_value) | (received_counter_value < upper_window) { - if data.1[idx] != received_counter_value { - return true; - } - } - return false; - } - - #[inline(always)] - pub fn message_authenticated(&self, received_counter_value: u32) -> bool { - let idx = (received_counter_value % COUNTER_MAX_DELTA) as usize; - let mut data = self.0.write().unwrap(); - let max_counter_seen = data.0; - let lower_window = max_counter_seen.wrapping_sub(COUNTER_MAX_DELTA / 2); - let upper_window = max_counter_seen.wrapping_add(COUNTER_MAX_DELTA / 2); - if lower_window < upper_window { - if (lower_window <= received_counter_value) & (received_counter_value < upper_window) { - if data.1[idx] != received_counter_value { - data.1[idx] = received_counter_value; - data.0 = max_counter_seen.max(received_counter_value); - return true; - } - } - } else if (lower_window <= received_counter_value) | (received_counter_value < upper_window) { - if data.1[idx] != received_counter_value { - data.1[idx] = received_counter_value; - data.0 = (max_counter_seen as i32).max(received_counter_value as i32) as u32; - return true; - } - } - return false; - } -} - -pub(crate) struct CounterWindow(Mutex>); +pub(crate) struct CounterWindow(AtomicBool, AtomicBool, [AtomicU32; COUNTER_MAX_ALLOWED_OOO]); impl CounterWindow { #[inline(always)] pub fn new(initial: u32) -> Self { - // we want our nonces to wrap at the exact same time that the counter wraps, so we shift them up to the u64 boundary - let initial_nonce = (initial as u64).wrapping_shl(32); - Self(Mutex::new(Some([initial_nonce; COUNTER_MAX_ALLOWED_OOO as usize]))) + Self(AtomicBool::new(true), AtomicBool::new(false), std::array::from_fn(|_| AtomicU32::new(initial))) } #[inline(always)] pub fn new_uninit() -> Self { - Self(Mutex::new(None)) + Self(AtomicBool::new(false), AtomicBool::new(false), std::array::from_fn(|_| AtomicU32::new(0))) } #[inline(always)] - pub fn init(&self, initial: u32) { - let initial_nonce = (initial as u64).wrapping_shl(6); - let mut data = self.0.lock().unwrap(); - *data = Some([initial_nonce; COUNTER_MAX_ALLOWED_OOO as usize]); + pub fn init_authenticated(&self, received_counter_value: u32) { + self.1.store((u32::MAX/4 < received_counter_value) & (received_counter_value <= u32::MAX/4*3), Ordering::SeqCst); + for i in 1..COUNTER_MAX_ALLOWED_OOO { + self.2[i].store(received_counter_value, Ordering::SeqCst); + } + self.0.store(true, Ordering::SeqCst); } - #[inline(always)] - pub fn message_received(&self, received_counter_value: u32, received_fragment_no: u8) -> bool { - let fragment_nonce = (received_counter_value as u64).wrapping_shl(32) | (received_fragment_no as u64); - //everything past this point must be atomic, i.e. these instructions must be run mutually exclusive to completion; - //atomic instructions are only ever atomic within themselves; - //sequentially consistent atomics do not guarantee that the thread is not preempted between individual atomic instructions - if let Some(history) = self.0.lock().unwrap().as_mut() { - let mut is_in = false; - let mut idx = 0; - let mut smallest = fragment_nonce; - for i in 0..history.len() { - let nonce = history[i]; - is_in |= nonce == fragment_nonce; - let delta = (smallest as i64).wrapping_sub(nonce as i64); - if delta > 0 { - smallest = nonce; - idx = i; - } + pub fn message_received(&self, received_counter_value: u32) -> bool { + if self.0.load(Ordering::SeqCst) { + let idx = (received_counter_value % COUNTER_MAX_ALLOWED_OOO as u32) as usize; + let pre = self.2[idx].load(Ordering::SeqCst); + if self.1.load(Ordering::Relaxed) { + return pre < received_counter_value; + } else { + return (pre as i32) < (received_counter_value as i32); } - if !is_in & (smallest != fragment_nonce) { - history[idx] = fragment_nonce; - return true - } - return false } else { - return true + return true; } } #[inline(always)] - pub fn purge(&self, inauthentic_counter_value: u32, inauthentic_fragment_no: u8) { - let inauthentic_nonce = (inauthentic_counter_value as u64).wrapping_shl(32) | (inauthentic_fragment_no as u64); - //everything past this point must be atomic, i.e. these instructions must be run mutually exclusive to completion; - //atomic instructions are only ever atomic within themselves; - //sequentially consistent atomics do not guarantee that the thread is not preempted between individual atomic instructions - if let Some(history) = self.0.lock().unwrap().as_mut() { - let mut idx = 0; - let mut smallest = history[0]; - for i in 0..history.len() { - let nonce = history[i]; - if nonce == inauthentic_nonce { - idx = i; - } else { - let delta = (smallest as i64).wrapping_sub(nonce as i64); - if delta > 0 { - smallest = nonce; - } - } - } - history[idx] = smallest; + pub fn message_authenticated(&self, received_counter_value: u32) -> bool { + //if a valid message is received but one of its fragments was lost, it can technically be replayed, since the message is incomplete, we know it still exists in the gather array, so the gather array will deduplicate the replayed message + //eventually the counter of that message will be too OOO to be accepted anymore so it can't be used to DOS + let idx = (received_counter_value % COUNTER_MAX_ALLOWED_OOO as u32) as usize; + if self.1.swap((u32::MAX/4 < received_counter_value) & (received_counter_value <= u32::MAX/4*3), Ordering::SeqCst) { + return self.2[idx].fetch_max(received_counter_value, Ordering::SeqCst) < received_counter_value; + } else { + let pre_as_signed: &AtomicI32 = unsafe {mem::transmute(&self.2[idx])}; + return pre_as_signed.fetch_max(received_counter_value as i32, Ordering::SeqCst) < received_counter_value as i32; } } } diff --git a/zssp/src/tests.rs b/zssp/src/tests.rs index dd95fae2e..47cd058e8 100644 --- a/zssp/src/tests.rs +++ b/zssp/src/tests.rs @@ -229,56 +229,36 @@ mod tests { #[test] fn counter_window() { let mut rng = 84632; - let mut counter = u32::MAX - 16; - let mut fragment_no: u8 = 0; - let mut history = Vec::<(u32, u8)>::new(); + let mut counter = 1u32; + let mut history = Vec::new(); - let mut w = CounterWindow::new(counter.wrapping_sub(1)); + let mut w = CounterWindow::new(counter); for i in 0..1000000 { let p = xorshift64(&mut rng) as f32/(u32::MAX as f32 + 1.0); let c; - let f; if p < 0.5 { let r = xorshift64(&mut rng); c = counter.wrapping_add(r%(COUNTER_MAX_ALLOWED_OOO - 2) as u32 + 1); - f = 0; - } else if p < 0.7 { - fragment_no = u8::min(fragment_no + 1, 63); - c = counter; - f = fragment_no; } else if p < 0.8 { counter = counter.wrapping_add(1); - fragment_no = 0; c = counter; - f = fragment_no; } else if p < 0.9 { if history.len() > 0 { let idx = xorshift64(&mut rng) as usize%history.len(); - let (c, f) = history[idx]; - assert!(!w.message_received(c, f)); + let c = history[idx]; + assert!(!w.message_authenticated(c)); } continue; - } else if p < 0.9995 { + } else { c = xorshift64(&mut rng); - f = (xorshift64(&mut rng)%64) as u8; - if w.message_received(c, f) { - w.purge(c, f); - } + w.message_received(c); continue; - } else { - //simulate rekeying - counter = xorshift64(&mut rng); - fragment_no = 0; - w = CounterWindow::new(counter.wrapping_sub(1)); - history = Vec::<(u32, u8)>::new(); - c = counter; - f = fragment_no; } - if history.contains(&(c, f)) { - assert!(!w.message_received(c, f)); + if history.contains(&c) { + assert!(!w.message_authenticated(c)); } else { - assert!(w.message_received(c, f)); - history.push((c, f)); + assert!(w.message_authenticated(c)); + history.push(c); } } } diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index f9e56972d..eb2adf1be 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -487,7 +487,7 @@ impl ReceiveContext { { if let Some(session) = app.lookup_session(local_session_id) { if verify_header_check_code(incoming_packet, &session.header_check_cipher) { - if session.receive_window.message_received(counter, fragment_no) { + if session.receive_window.message_received(counter) { let canonical_header = CanonicalHeader::make(local_session_id, packet_type, counter); if fragment_count > 1 { if fragment_count <= (MAX_FRAGMENTS as u8) && fragment_no < fragment_count { @@ -664,31 +664,33 @@ impl ReceiveContext { session_key.return_receive_cipher(c); if aead_authentication_ok { - // Select this key as the new default if it's newer than the current key. - if p > 0 - && state.session_keys[state.cur_session_key_idx] - .as_ref() - .map_or(true, |old| old.creation_counter < session_key.creation_counter) - { - drop(state); - let mut state = session.state.write().unwrap(); - state.cur_session_key_idx = key_idx; - for i in 0..KEY_HISTORY_SIZE { - if i != key_idx { - if let Some(old_key) = state.session_keys[key_idx].as_ref() { - // Release pooled cipher memory from old keys. - old_key.receive_cipher_pool.lock().unwrap().clear(); - old_key.send_cipher_pool.lock().unwrap().clear(); + if session.receive_window.message_authenticated(counter) { + // Select this key as the new default if it's newer than the current key. + if p > 0 + && state.session_keys[state.cur_session_key_idx] + .as_ref() + .map_or(true, |old| old.creation_counter < session_key.creation_counter) + { + drop(state); + let mut state = session.state.write().unwrap(); + state.cur_session_key_idx = key_idx; + for i in 0..KEY_HISTORY_SIZE { + if i != key_idx { + if let Some(old_key) = state.session_keys[key_idx].as_ref() { + // Release pooled cipher memory from old keys. + old_key.receive_cipher_pool.lock().unwrap().clear(); + old_key.send_cipher_pool.lock().unwrap().clear(); + } } } } - } - if packet_type == PACKET_TYPE_DATA { - return Ok(ReceiveResult::OkData(&mut data_buf[..data_len])); - } else { - unlikely_branch(); - return Ok(ReceiveResult::Ok); + if packet_type == PACKET_TYPE_DATA { + return Ok(ReceiveResult::OkData(&mut data_buf[..data_len])); + } else { + unlikely_branch(); + return Ok(ReceiveResult::Ok); + } } } } @@ -1158,7 +1160,7 @@ impl ReceiveContext { last_ratchet_count + 1, hybrid_kk.is_some(), ); - session.receive_window.init(counter); + session.receive_window.init_authenticated(counter); //////////////////////////////////////////////////////////////// // packet encoding for post-noise session start ack From 402cf69b729b9564aa51ab38aef670bd91d45be4 Mon Sep 17 00:00:00 2001 From: mamoniot Date: Tue, 27 Dec 2022 10:25:17 -0500 Subject: [PATCH 09/34] updated comment --- zssp/src/counter.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zssp/src/counter.rs b/zssp/src/counter.rs index 828785220..7dc479d53 100644 --- a/zssp/src/counter.rs +++ b/zssp/src/counter.rs @@ -95,8 +95,8 @@ impl CounterWindow { #[inline(always)] pub fn message_authenticated(&self, received_counter_value: u32) -> bool { - //if a valid message is received but one of its fragments was lost, it can technically be replayed, since the message is incomplete, we know it still exists in the gather array, so the gather array will deduplicate the replayed message - //eventually the counter of that message will be too OOO to be accepted anymore so it can't be used to DOS + //if a valid message is received but one of its fragments was lost, it can technically be replayed. However since the message is incomplete, we know it still exists in the gather array, so the gather array will deduplicate the replayed message. Even if the gather array gets flushed, that flush still effectively deduplicates the replayed message. + //eventually the counter of that kind of message will be too OOO to be accepted anymore so it can't be used to DOS. let idx = (received_counter_value % COUNTER_MAX_ALLOWED_OOO as u32) as usize; if self.1.swap((u32::MAX/4 < received_counter_value) & (received_counter_value <= u32::MAX/4*3), Ordering::SeqCst) { return self.2[idx].fetch_max(received_counter_value, Ordering::SeqCst) < received_counter_value; From 52556d0d894345b53a6c87b065350f96ac0233a7 Mon Sep 17 00:00:00 2001 From: mamoniot Date: Tue, 27 Dec 2022 14:25:20 -0500 Subject: [PATCH 10/34] finished implementation of counter starting at 1 --- zssp/src/constants.rs | 3 - zssp/src/counter.rs | 73 +++++-------- zssp/src/tests.rs | 9 +- zssp/src/zssp.rs | 246 ++++++++++++++++++------------------------ 4 files changed, 138 insertions(+), 193 deletions(-) diff --git a/zssp/src/constants.rs b/zssp/src/constants.rs index bd632f8f3..7ff145de5 100644 --- a/zssp/src/constants.rs +++ b/zssp/src/constants.rs @@ -74,9 +74,6 @@ pub(crate) const HMAC_SIZE: usize = 48; /// This is large since some ZeroTier nodes handle huge numbers of links, like roots and controllers. pub(crate) const SESSION_ID_SIZE: usize = 6; -/// Number of session keys to hold at a given time (current, previous, next). -pub(crate) const KEY_HISTORY_SIZE: usize = 3; - /// Maximum difference between out-of-order incoming packet counters, and size of deduplication buffer. pub(crate) const COUNTER_MAX_ALLOWED_OOO: usize = 16; diff --git a/zssp/src/counter.rs b/zssp/src/counter.rs index 7dc479d53..2f7fa0749 100644 --- a/zssp/src/counter.rs +++ b/zssp/src/counter.rs @@ -1,8 +1,4 @@ -use std::{sync::{ - atomic::{AtomicU64, Ordering, AtomicU32, AtomicI32, AtomicBool} -}, mem}; - -use zerotier_crypto::random; +use std::sync::atomic::{Ordering, AtomicU32}; use crate::constants::COUNTER_MAX_ALLOWED_OOO; @@ -12,20 +8,24 @@ use crate::constants::COUNTER_MAX_ALLOWED_OOO; /// lets us more safely implement key lifetime limits without confusing logic to handle 32-bit /// wrap-around. #[repr(transparent)] -pub(crate) struct Counter(AtomicU64); +pub(crate) struct Counter(AtomicU32); impl Counter { #[inline(always)] pub fn new() -> Self { // Using a random value has no security implication. Zero would be fine. This just // helps randomize packet contents a bit. - Self(AtomicU64::new((random::next_u32_secure()/2) as u64)) + Self(AtomicU32::new(1u32)) + } + #[inline(always)] + pub fn reset_after_initial_offer(&self) { + self.0.store(2u32, Ordering::SeqCst); } /// Get the value most recently used to send a packet. #[inline(always)] - pub fn previous(&self) -> CounterValue { - CounterValue(self.0.load(Ordering::SeqCst).wrapping_sub(1)) + pub fn current(&self) -> CounterValue { + CounterValue(self.0.load(Ordering::SeqCst)) } /// Get a counter value for the next packet being sent. @@ -38,7 +38,7 @@ impl Counter { /// A value of the outgoing packet counter. #[repr(transparent)] #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord)] -pub(crate) struct CounterValue(u64); +pub(crate) struct CounterValue(u32); impl CounterValue { /// Get the 32-bit counter value used to build packets. @@ -46,51 +46,35 @@ impl CounterValue { pub fn to_u32(&self) -> u32 { self.0 as u32 } - - /// Get the counter value after N more uses of the parent counter. - /// - /// This checks for u64 overflow for the sake of correctness. Be careful if using ZSSP in a - /// generational starship where sessions may last for millions of years. - #[inline(always)] - pub fn counter_value_after_uses(&self, uses: u64) -> Self { - Self(self.0.checked_add(uses).unwrap()) + pub fn get_initial_offer_counter() -> CounterValue { + return CounterValue(1u32); } } /// Incoming packet deduplication and replay protection window. -pub(crate) struct CounterWindow(AtomicBool, AtomicBool, [AtomicU32; COUNTER_MAX_ALLOWED_OOO]); +pub(crate) struct CounterWindow([AtomicU32; COUNTER_MAX_ALLOWED_OOO]); impl CounterWindow { #[inline(always)] - pub fn new(initial: u32) -> Self { - Self(AtomicBool::new(true), AtomicBool::new(false), std::array::from_fn(|_| AtomicU32::new(initial))) + pub fn new() -> Self { + Self(std::array::from_fn(|_| AtomicU32::new(0))) } - #[inline(always)] - pub fn new_uninit() -> Self { - Self(AtomicBool::new(false), AtomicBool::new(false), std::array::from_fn(|_| AtomicU32::new(0))) + ///this creates a counter window that rejects everything + pub fn new_invalid() -> Self { + Self(std::array::from_fn(|_| AtomicU32::new(u32::MAX))) } - #[inline(always)] - pub fn init_authenticated(&self, received_counter_value: u32) { - self.1.store((u32::MAX/4 < received_counter_value) & (received_counter_value <= u32::MAX/4*3), Ordering::SeqCst); - for i in 1..COUNTER_MAX_ALLOWED_OOO { - self.2[i].store(received_counter_value, Ordering::SeqCst); + pub fn reset_after_initial_offer(&self) { + for i in 0..COUNTER_MAX_ALLOWED_OOO { + self.0[i].store(0, Ordering::SeqCst) } - self.0.store(true, Ordering::SeqCst); } #[inline(always)] pub fn message_received(&self, received_counter_value: u32) -> bool { - if self.0.load(Ordering::SeqCst) { - let idx = (received_counter_value % COUNTER_MAX_ALLOWED_OOO as u32) as usize; - let pre = self.2[idx].load(Ordering::SeqCst); - if self.1.load(Ordering::Relaxed) { - return pre < received_counter_value; - } else { - return (pre as i32) < (received_counter_value as i32); - } - } else { - return true; - } + let idx = (received_counter_value % COUNTER_MAX_ALLOWED_OOO as u32) as usize; + //it is highly likely this can be a Relaxed ordering, but I want someone else to confirm that is the case + let pre = self.0[idx].load(Ordering::SeqCst); + return pre < received_counter_value; } #[inline(always)] @@ -98,11 +82,6 @@ impl CounterWindow { //if a valid message is received but one of its fragments was lost, it can technically be replayed. However since the message is incomplete, we know it still exists in the gather array, so the gather array will deduplicate the replayed message. Even if the gather array gets flushed, that flush still effectively deduplicates the replayed message. //eventually the counter of that kind of message will be too OOO to be accepted anymore so it can't be used to DOS. let idx = (received_counter_value % COUNTER_MAX_ALLOWED_OOO as u32) as usize; - if self.1.swap((u32::MAX/4 < received_counter_value) & (received_counter_value <= u32::MAX/4*3), Ordering::SeqCst) { - return self.2[idx].fetch_max(received_counter_value, Ordering::SeqCst) < received_counter_value; - } else { - let pre_as_signed: &AtomicI32 = unsafe {mem::transmute(&self.2[idx])}; - return pre_as_signed.fetch_max(received_counter_value as i32, Ordering::SeqCst) < received_counter_value as i32; - } + return self.0[idx].fetch_max(received_counter_value, Ordering::SeqCst) < received_counter_value; } } diff --git a/zssp/src/tests.rs b/zssp/src/tests.rs index 47cd058e8..791b51698 100644 --- a/zssp/src/tests.rs +++ b/zssp/src/tests.rs @@ -232,7 +232,7 @@ mod tests { let mut counter = 1u32; let mut history = Vec::new(); - let mut w = CounterWindow::new(counter); + let mut w = CounterWindow::new(); for i in 0..1000000 { let p = xorshift64(&mut rng) as f32/(u32::MAX as f32 + 1.0); let c; @@ -249,10 +249,15 @@ mod tests { assert!(!w.message_authenticated(c)); } continue; - } else { + } else if p < 0.999 { c = xorshift64(&mut rng); w.message_received(c); continue; + } else { + w.reset_after_initial_offer(); + counter = 1u32; + history = Vec::new(); + continue; } if history.contains(&c) { assert!(!w.message_authenticated(c)); diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index eb2adf1be..9f132e44e 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -113,7 +113,7 @@ pub struct Session { pub application_data: Application::Data, send_counter: Counter, // Outgoing packet counter and nonce state - receive_window: CounterWindow, // Receive window for anti-replay and deduplication + receive_window: [CounterWindow; 2], // Receive window for anti-replay and deduplication psk: Secret<64>, // Arbitrary PSK provided by external code noise_ss: Secret<48>, // Static raw shared ECDH NIST P-384 key header_check_cipher: Aes, // Cipher used for header check codes (not Noise related) @@ -126,17 +126,17 @@ pub struct Session { struct SessionMutableState { remote_session_id: Option, // The other side's 48-bit session ID - session_keys: [Option; KEY_HISTORY_SIZE], // Buffers to store current, next, and last active key - cur_session_key_idx: usize, // Pointer used for keys[] circular buffer + 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) } /// A shared symmetric session key. +/// sessions always start at counter 1u32 struct SessionKey { secret_fingerprint: [u8; 16], // First 128 bits of a SHA384 computed from the secret creation_time: i64, // Time session key was established - creation_counter: CounterValue, // Counter value at which session was established lifetime: KeyLifetime, // Key expiration time and counter ratchet_key: Secret<64>, // Ratchet key for deriving the next session key receive_key: Secret, // Receive side AES-GCM key @@ -149,14 +149,14 @@ struct SessionKey { /// Key lifetime state struct KeyLifetime { - rekey_at_or_after_counter: CounterValue, - hard_expire_at_counter: CounterValue, + rekey_at_or_after_counter: u32, rekey_at_or_after_timestamp: i64, } /// Alice's KEY_OFFER, remembered so Noise agreement process can resume on KEY_COUNTER_OFFER. struct EphemeralOffer { id: [u8; 16], // Arbitrary random offer ID + key_id: bool, // The key_id bound to this offer, for handling OOO rekeying creation_time: i64, // Local time when offer was created ratchet_count: u64, // Ratchet count (starting at zero) for initial offer ratchet_key: Option>, // Ratchet key from previous offer or None if first offer @@ -235,6 +235,7 @@ impl Session { if send_ephemeral_offer( &mut send, send_counter.next(), + false, local_session_id, None, app.get_local_s_public_blob(), @@ -254,14 +255,14 @@ impl Session { id: local_session_id, application_data, send_counter, - receive_window: CounterWindow::new_uninit(),//alice does not know bob's counter yet + receive_window: [CounterWindow::new(), CounterWindow::new_invalid()], psk: psk.clone(), noise_ss, header_check_cipher, state: RwLock::new(SessionMutableState { remote_session_id: None, - session_keys: [None, None, None], - cur_session_key_idx: 0, + session_keys: [None, None], + cur_session_key_id: false, offer, last_remote_offer: i64::MIN, }), @@ -290,7 +291,7 @@ impl Session { debug_assert!(mtu_sized_buffer.len() >= MIN_TRANSPORT_MTU); let state = self.state.read().unwrap(); if let Some(remote_session_id) = state.remote_session_id { - if let Some(session_key) = state.session_keys[state.cur_session_key_idx].as_ref() { + if let Some(session_key) = state.session_keys[state.cur_session_key_id as usize].as_ref() { // Total size of the armored packet we are going to send (may end up being fragmented) let packet_len = data.len() + HEADER_SIZE + AES_GCM_TAG_SIZE; @@ -309,6 +310,7 @@ impl Session { PACKET_TYPE_DATA, remote_session_id.into(), counter, + state.cur_session_key_id )?; // Get an initialized AES-GCM cipher and re-initialize with a 96-bit IV built from remote session ID, @@ -367,7 +369,7 @@ impl Session { /// Check whether this session is established. pub fn established(&self) -> bool { let state = self.state.read().unwrap(); - state.remote_session_id.is_some() && state.session_keys[state.cur_session_key_idx].is_some() + state.remote_session_id.is_some() && state.session_keys[state.cur_session_key_id as usize].is_some() } /// Get information about this session's security state. @@ -376,7 +378,7 @@ impl Session { /// and whether Kyber1024 was used. None is returned if the session isn't established. pub fn status(&self) -> Option<([u8; 16], i64, u64, bool)> { let state = self.state.read().unwrap(); - if let Some(key) = state.session_keys[state.cur_session_key_idx].as_ref() { + if let Some(key) = state.session_keys[state.cur_session_key_id as usize].as_ref() { Some((key.secret_fingerprint, key.creation_time, key.ratchet_count, key.jedi)) } else { None @@ -402,9 +404,9 @@ impl Session { ) { let state = self.state.read().unwrap(); if (force_rekey - || state.session_keys[state.cur_session_key_idx] + || state.session_keys[state.cur_session_key_id as usize] .as_ref() - .map_or(true, |key| key.lifetime.should_rekey(self.send_counter.previous(), current_time))) + .map_or(true, |key| key.lifetime.should_rekey(self.send_counter.current(), current_time))) && state .offer .as_ref() @@ -414,7 +416,8 @@ impl Session { let mut offer = None; if send_ephemeral_offer( &mut send, - self.send_counter.next(), + CounterValue::get_initial_offer_counter(), + !state.cur_session_key_id, self.id, state.remote_session_id, app.get_local_s_public_blob(), @@ -422,7 +425,7 @@ impl Session { &remote_s_public, &self.remote_s_public_blob_hash, &self.noise_ss, - state.session_keys[state.cur_session_key_idx].as_ref(), + state.session_keys[state.cur_session_key_id as usize].as_ref(), if state.remote_session_id.is_some() { Some(&self.header_check_cipher) } else { @@ -435,7 +438,7 @@ impl Session { .is_ok() { drop(state); - let _ = self.state.write().unwrap().offer.replace(offer.unwrap()); + self.state.write().unwrap().offer = offer; } } } @@ -477,7 +480,9 @@ impl ReceiveContext { return Err(Error::InvalidPacket); } - let counter = u32::from_le(memory::load_raw(incoming_packet)); + let mut counter = u32::from_le(memory::load_raw(incoming_packet)); + let key_id = (counter & 1) > 0; + counter = counter.wrapping_shr(1); let packet_type_fragment_info = u16::from_le(memory::load_raw(&incoming_packet[14..16])); let packet_type = (packet_type_fragment_info & 0x0f) as u8; let fragment_count = ((packet_type_fragment_info.wrapping_shr(4) + 1) as u8) & 63; @@ -487,7 +492,7 @@ impl ReceiveContext { { if let Some(session) = app.lookup_session(local_session_id) { if verify_header_check_code(incoming_packet, &session.header_check_cipher) { - if session.receive_window.message_received(counter) { + if session.receive_window[key_id as usize].message_received(counter) { let canonical_header = CanonicalHeader::make(local_session_id, packet_type, counter); if fragment_count > 1 { if fragment_count <= (MAX_FRAGMENTS as u8) && fragment_no < fragment_count { @@ -501,6 +506,7 @@ impl ReceiveContext { &mut send, data_buf, counter, + key_id, canonical_header.as_bytes(), assembled_packet.as_ref(), packet_type, @@ -520,6 +526,7 @@ impl ReceiveContext { &mut send, data_buf, counter, + key_id, canonical_header.as_bytes(), &[incoming_packet_buf], packet_type, @@ -556,6 +563,7 @@ impl ReceiveContext { &mut send, data_buf, counter, + key_id, canonical_header.as_bytes(), assembled_packet.as_ref(), packet_type, @@ -571,6 +579,7 @@ impl ReceiveContext { &mut send, data_buf, counter, + key_id, canonical_header.as_bytes(), &[incoming_packet_buf], packet_type, @@ -599,6 +608,7 @@ impl ReceiveContext { send: &mut SendFunction, data_buf: &'a mut [u8], counter: u32, + key_id: bool, canonical_header_bytes: &[u8; 12], fragments: &[Application::IncomingPacketBuffer], packet_type: u8, @@ -615,82 +625,59 @@ impl ReceiveContext { if packet_type <= PACKET_TYPE_NOP { if let Some(session) = session { let state = session.state.read().unwrap(); - for p in 0..KEY_HISTORY_SIZE { - let key_idx = (state.cur_session_key_idx + p) % KEY_HISTORY_SIZE; - if let Some(session_key) = state.session_keys[key_idx].as_ref() { - let mut c = session_key.get_receive_cipher(); - c.reset_init_gcm(canonical_header_bytes); - //////////////////////////////////////////////////////////////// - // packet decoding for post-noise transport - //////////////////////////////////////////////////////////////// + if let Some(session_key) = state.session_keys[key_id as usize].as_ref() { + let mut c = session_key.get_receive_cipher(); + c.reset_init_gcm(canonical_header_bytes); + //////////////////////////////////////////////////////////////// + // packet decoding for post-noise transport + //////////////////////////////////////////////////////////////// - let mut data_len = 0; + let mut data_len = 0; - // Decrypt fragments 0..N-1 where N is the number of fragments. - for f in fragments[..(fragments.len() - 1)].iter() { - let f = f.as_ref(); - debug_assert!(f.len() >= HEADER_SIZE); - let current_frag_data_start = data_len; - data_len += f.len() - HEADER_SIZE; - if data_len > data_buf.len() { - unlikely_branch(); - session_key.return_receive_cipher(c); - return Err(Error::DataBufferTooSmall); - } - c.crypt(&f[HEADER_SIZE..], &mut data_buf[current_frag_data_start..data_len]); - } - - // Decrypt final fragment (or only fragment if not fragmented) + // Decrypt fragments 0..N-1 where N is the number of fragments. + for f in fragments[..(fragments.len() - 1)].iter() { + let f = f.as_ref(); + debug_assert!(f.len() >= HEADER_SIZE); let current_frag_data_start = data_len; - let last_fragment = fragments.last().unwrap().as_ref(); - if last_fragment.len() < (HEADER_SIZE + AES_GCM_TAG_SIZE) { - unlikely_branch(); - return Err(Error::InvalidPacket); - } - data_len += last_fragment.len() - (HEADER_SIZE + AES_GCM_TAG_SIZE); + data_len += f.len() - HEADER_SIZE; if data_len > data_buf.len() { unlikely_branch(); session_key.return_receive_cipher(c); return Err(Error::DataBufferTooSmall); } - let payload_end = last_fragment.len() - AES_GCM_TAG_SIZE; - c.crypt( - &last_fragment[HEADER_SIZE..payload_end], - &mut data_buf[current_frag_data_start..data_len], - ); + c.crypt(&f[HEADER_SIZE..], &mut data_buf[current_frag_data_start..data_len]); + } - let gcm_tag = &last_fragment[payload_end..]; - let aead_authentication_ok = c.finish_decrypt(gcm_tag); + // Decrypt final fragment (or only fragment if not fragmented) + let current_frag_data_start = data_len; + let last_fragment = fragments.last().unwrap().as_ref(); + if last_fragment.len() < (HEADER_SIZE + AES_GCM_TAG_SIZE) { + unlikely_branch(); + return Err(Error::InvalidPacket); + } + data_len += last_fragment.len() - (HEADER_SIZE + AES_GCM_TAG_SIZE); + if data_len > data_buf.len() { + unlikely_branch(); session_key.return_receive_cipher(c); + return Err(Error::DataBufferTooSmall); + } + let payload_end = last_fragment.len() - AES_GCM_TAG_SIZE; + c.crypt( + &last_fragment[HEADER_SIZE..payload_end], + &mut data_buf[current_frag_data_start..data_len], + ); - if aead_authentication_ok { - if session.receive_window.message_authenticated(counter) { - // Select this key as the new default if it's newer than the current key. - if p > 0 - && state.session_keys[state.cur_session_key_idx] - .as_ref() - .map_or(true, |old| old.creation_counter < session_key.creation_counter) - { - drop(state); - let mut state = session.state.write().unwrap(); - state.cur_session_key_idx = key_idx; - for i in 0..KEY_HISTORY_SIZE { - if i != key_idx { - if let Some(old_key) = state.session_keys[key_idx].as_ref() { - // Release pooled cipher memory from old keys. - old_key.receive_cipher_pool.lock().unwrap().clear(); - old_key.send_cipher_pool.lock().unwrap().clear(); - } - } - } - } + let gcm_tag = &last_fragment[payload_end..]; + let aead_authentication_ok = c.finish_decrypt(gcm_tag); + session_key.return_receive_cipher(c); - if packet_type == PACKET_TYPE_DATA { - return Ok(ReceiveResult::OkData(&mut data_buf[..data_len])); - } else { - unlikely_branch(); - return Ok(ReceiveResult::Ok); - } + if aead_authentication_ok { + if session.receive_window[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); } } } @@ -856,13 +843,13 @@ impl ReceiveContext { let mut ratchet_key = None; let mut last_ratchet_count = 0; let state = session.state.read().unwrap(); - for k in state.session_keys.iter() { - if let Some(k) = k.as_ref() { - if public_fingerprint_of_secret(k.ratchet_key.as_bytes())[..16].eq(alice_ratchet_key_fingerprint) { - ratchet_key = Some(k.ratchet_key.clone()); - last_ratchet_count = k.ratchet_count; - break; - } + if state.cur_session_key_id == key_id { + return Ok(ReceiveResult::Ignored); // alice is requesting to overwrite the current key, reject it + } + if let Some(k) = state.session_keys[state.cur_session_key_id as usize].as_ref() { + if public_fingerprint_of_secret(k.ratchet_key.as_bytes())[..16].eq(alice_ratchet_key_fingerprint) { + ratchet_key = Some(k.ratchet_key.clone()); + last_ratchet_count = k.ratchet_count; } } if ratchet_key.is_none() { @@ -882,14 +869,14 @@ impl ReceiveContext { id: new_session_id, application_data: associated_object, send_counter: Counter::new(), - receive_window: CounterWindow::new(counter), + receive_window: [CounterWindow::new_invalid(), CounterWindow::new_invalid()], psk, noise_ss, header_check_cipher, state: RwLock::new(SessionMutableState { remote_session_id: Some(alice_session_id), - session_keys: [None, None, None], - cur_session_key_idx: 0, + session_keys: [None, None], + cur_session_key_id: key_id, offer: None, last_remote_offer: current_time, }), @@ -959,7 +946,7 @@ impl ReceiveContext { //////////////////////////////////////////////////////////////// let mut reply_buf = [0_u8; KEX_BUF_LEN]; - let reply_counter = session.send_counter.next(); + let reply_counter = CounterValue::get_initial_offer_counter(); let mut idx = HEADER_SIZE; idx = safe_write_all(&mut reply_buf, idx, &[SESSION_PROTOCOL_VERSION])?; @@ -991,6 +978,7 @@ impl ReceiveContext { PACKET_TYPE_KEY_COUNTER_OFFER, alice_session_id.into(), reply_counter, + key_id, )?; let reply_canonical_header = CanonicalHeader::make(alice_session_id.into(), PACKET_TYPE_KEY_COUNTER_OFFER, reply_counter.to_u32()); @@ -1033,23 +1021,25 @@ impl ReceiveContext { session_key, Role::Bob, current_time, - reply_counter, last_ratchet_count + 1, hybrid_kk.is_some(), ); let mut state = session.state.write().unwrap(); let _ = state.remote_session_id.replace(alice_session_id); - let next_key_ptr = (state.cur_session_key_idx + 1) % KEY_HISTORY_SIZE; - let _ = state.session_keys[next_key_ptr].replace(session_key); + let _ = state.session_keys[key_id as usize].replace(session_key); + session.send_counter.reset_after_initial_offer(); + state.cur_session_key_id = key_id; + session.receive_window[key_id as usize].reset_after_initial_offer(); + session.receive_window[key_id as usize].message_authenticated(counter); drop(state); // Bob now has final key state for this exchange. Yay! Now reply to Alice so she can construct it. send_with_fragmentation(send, &mut reply_buf[..packet_end], mtu, &session.header_check_cipher); - if new_session.is_some() { - return Ok(ReceiveResult::OkNewSession(new_session.unwrap())); + if let Some(new_session) = new_session { + return Ok(ReceiveResult::OkNewSession(new_session)); } else { return Ok(ReceiveResult::Ok); } @@ -1106,7 +1096,7 @@ impl ReceiveContext { parse_dec_key_offer_after_header(&kex_packet[plaintext_end..kex_packet_len], packet_type)?; // Check that this is a counter offer to the original offer we sent. - if !offer.id.eq(offer_id) { + if !(offer.id.eq(offer_id) & (offer.key_id == key_id)) { return Ok(ReceiveResult::Ignored); } @@ -1151,47 +1141,21 @@ impl ReceiveContext { // Alice has now completed and validated the full hybrid exchange. - let reply_counter = session.send_counter.next(); let session_key = SessionKey::new( session_key, Role::Alice, current_time, - reply_counter, last_ratchet_count + 1, hybrid_kk.is_some(), ); - session.receive_window.init_authenticated(counter); - - //////////////////////////////////////////////////////////////// - // packet encoding for post-noise session start ack - //////////////////////////////////////////////////////////////// - - let mut reply_buf = [0_u8; HEADER_SIZE + AES_GCM_TAG_SIZE]; - create_packet_header( - &mut reply_buf, - HEADER_SIZE + AES_GCM_TAG_SIZE, - mtu, - PACKET_TYPE_NOP, - bob_session_id.into(), - reply_counter, - )?; - - let mut c = session_key.get_send_cipher(reply_counter)?; - c.reset_init_gcm( - CanonicalHeader::make(bob_session_id.into(), PACKET_TYPE_NOP, reply_counter.to_u32()).as_bytes(), - ); - let gcm_tag = c.finish_encrypt(); - safe_write_all(&mut reply_buf, HEADER_SIZE, &gcm_tag)?; - session_key.return_send_cipher(c); - - set_header_check_code(&mut reply_buf, &session.header_check_cipher); - send(&mut reply_buf); drop(state); let mut state = session.state.write().unwrap(); let _ = state.remote_session_id.replace(bob_session_id); - let next_key_idx = (state.cur_session_key_idx + 1) % KEY_HISTORY_SIZE; - let _ = state.session_keys[next_key_idx].replace(session_key); + let _ = state.session_keys[key_id as usize].replace(session_key); + session.send_counter.reset_after_initial_offer(); + state.cur_session_key_id = key_id; + session.receive_window[key_id as usize].message_authenticated(counter); let _ = state.offer.take(); return Ok(ReceiveResult::Ok); @@ -1213,6 +1177,7 @@ impl ReceiveContext { fn send_ephemeral_offer( send: &mut SendFunction, counter: CounterValue, + key_id: bool, alice_session_id: SessionId, bob_session_id: Option, alice_s_public_blob: &[u8], @@ -1298,6 +1263,7 @@ fn send_ephemeral_offer( PACKET_TYPE_INITIAL_KEY_OFFER, bob_session_id, counter, + key_id, )?; let canonical_header = CanonicalHeader::make(bob_session_id, PACKET_TYPE_INITIAL_KEY_OFFER, counter.to_u32()); @@ -1351,6 +1317,7 @@ fn send_ephemeral_offer( *ret_ephemeral_offer = Some(EphemeralOffer { id, + key_id, creation_time: current_time, ratchet_count, ratchet_key, @@ -1370,6 +1337,7 @@ fn create_packet_header( packet_type: u8, recipient_session_id: SessionId, counter: CounterValue, + key_id: bool ) -> Result<(), Error> { let fragment_count = ((packet_len as f32) / (mtu - HEADER_SIZE) as f32).ceil() as usize; @@ -1379,6 +1347,7 @@ fn create_packet_header( debug_assert!(fragment_count > 0); debug_assert!(fragment_count <= MAX_FRAGMENTS); debug_assert!(packet_type <= 0x0f); // packet type is 4 bits + let counter = counter.to_u32().wrapping_shl(1) | (key_id as u32); if fragment_count <= MAX_FRAGMENTS { // Header indexed by bit/byte: @@ -1388,7 +1357,7 @@ 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() as u64).to_le(), header_destination_buffer); + memory::store_raw((counter as u64).to_le(), header_destination_buffer); 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(), @@ -1499,7 +1468,6 @@ impl SessionKey { key: Secret<64>, role: Role, current_time: i64, - current_counter: CounterValue, ratchet_count: u64, jedi: bool, ) -> Self { @@ -1512,8 +1480,7 @@ impl SessionKey { Self { secret_fingerprint: public_fingerprint_of_secret(key.as_bytes())[..16].try_into().unwrap(), creation_time: current_time, - creation_counter: current_counter, - lifetime: KeyLifetime::new(current_counter, current_time), + lifetime: KeyLifetime::new(current_time), ratchet_key: kbkdf512(key.as_bytes(), KBKDF_KEY_USAGE_LABEL_RATCHETING), receive_key, send_key, @@ -1560,12 +1527,9 @@ impl SessionKey { } impl KeyLifetime { - fn new(current_counter: CounterValue, current_time: i64) -> Self { + fn new(current_time: i64) -> Self { Self { - rekey_at_or_after_counter: current_counter - .counter_value_after_uses(REKEY_AFTER_USES) - .counter_value_after_uses((random::next_u32_secure() % REKEY_AFTER_USES_MAX_JITTER) as u64), - hard_expire_at_counter: current_counter.counter_value_after_uses(EXPIRE_AFTER_USES), + rekey_at_or_after_counter: REKEY_AFTER_USES as u32 + (random::next_u32_secure() % REKEY_AFTER_USES_MAX_JITTER), rekey_at_or_after_timestamp: current_time + REKEY_AFTER_TIME_MS + (random::next_u32_secure() % REKEY_AFTER_TIME_MS_MAX_JITTER) as i64, @@ -1573,11 +1537,11 @@ impl KeyLifetime { } fn should_rekey(&self, counter: CounterValue, current_time: i64) -> bool { - counter >= self.rekey_at_or_after_counter || current_time >= self.rekey_at_or_after_timestamp + counter.to_u32() >= self.rekey_at_or_after_counter || current_time >= self.rekey_at_or_after_timestamp } fn expired(&self, counter: CounterValue) -> bool { - counter >= self.hard_expire_at_counter + counter.to_u32() >= EXPIRE_AFTER_USES as u32 } } From 8d1efcdffae9cee87a77057d489f6f6a66324092 Mon Sep 17 00:00:00 2001 From: mamoniot Date: Tue, 27 Dec 2022 15:02:46 -0500 Subject: [PATCH 11/34] finished protocol --- zssp/src/tests.rs | 2 +- zssp/src/zssp.rs | 22 +++++++++++----------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/zssp/src/tests.rs b/zssp/src/tests.rs index 791b51698..b15a5e3f4 100644 --- a/zssp/src/tests.rs +++ b/zssp/src/tests.rs @@ -232,7 +232,7 @@ mod tests { let mut counter = 1u32; let mut history = Vec::new(); - let mut w = CounterWindow::new(); + let w = CounterWindow::new(); for i in 0..1000000 { let p = xorshift64(&mut rng) as f32/(u32::MAX as f32 + 1.0); let c; diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index 9f132e44e..9fd678801 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -480,9 +480,9 @@ impl ReceiveContext { return Err(Error::InvalidPacket); } - let mut counter = u32::from_le(memory::load_raw(incoming_packet)); - let key_id = (counter & 1) > 0; - counter = counter.wrapping_shr(1); + let raw_counter = u32::from_le(memory::load_raw(&incoming_packet[4..8])); + let key_id = (raw_counter & 1) > 0; + let counter = raw_counter.wrapping_shr(1); let packet_type_fragment_info = u16::from_le(memory::load_raw(&incoming_packet[14..16])); let packet_type = (packet_type_fragment_info & 0x0f) as u8; let fragment_count = ((packet_type_fragment_info.wrapping_shr(4) + 1) as u8) & 63; @@ -1347,17 +1347,17 @@ fn create_packet_header( debug_assert!(fragment_count > 0); debug_assert!(fragment_count <= MAX_FRAGMENTS); debug_assert!(packet_type <= 0x0f); // packet type is 4 bits - let counter = counter.to_u32().wrapping_shl(1) | (key_id as u32); if fragment_count <= MAX_FRAGMENTS { // Header indexed by bit/byte: - // [0-31]/[0-3] counter - // [32-63]/[4-7] header check code (computed later) + // [0-31]/[0-3] header check code (computed later) + // [32-32]/[4-] key id + // [33-63]/[-7] counter // [64-111]/[8-13] recipient's session ID (unique on their side) // [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 as u64).to_le(), header_destination_buffer); + 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(), @@ -1402,16 +1402,16 @@ fn send_with_fragmentation( fn set_header_check_code(packet: &mut [u8], header_check_cipher: &Aes) { debug_assert!(packet.len() >= MIN_PACKET_SIZE); let mut check_code = 0u128.to_ne_bytes(); - header_check_cipher.encrypt_block(&packet[8..24], &mut check_code); - packet[4..8].copy_from_slice(&check_code[..4]); + header_check_cipher.encrypt_block(&packet[4..20], &mut check_code); + packet[..4].copy_from_slice(&check_code[..4]); } /// Verify 32-bit header check code. fn verify_header_check_code(packet: &[u8], header_check_cipher: &Aes) -> bool { debug_assert!(packet.len() >= MIN_PACKET_SIZE); let mut header_mac = 0u128.to_ne_bytes(); - header_check_cipher.encrypt_block(&packet[8..24], &mut header_mac); - memory::load_raw::(&packet[4..8]) == memory::load_raw::(&header_mac) + header_check_cipher.encrypt_block(&packet[4..20], &mut header_mac); + memory::load_raw::(&packet[..4]) == memory::load_raw::(&header_mac) } /// Parse KEY_OFFER and KEY_COUNTER_OFFER starting after the unencrypted public key part. From 5d72aabe170b02d154d283e6805f8b37f4f797a8 Mon Sep 17 00:00:00 2001 From: mamoniot Date: Tue, 27 Dec 2022 21:09:01 -0500 Subject: [PATCH 12/34] got all tests to pass --- zssp/src/counter.rs | 8 +- zssp/src/tests.rs | 2 +- zssp/src/zssp.rs | 175 ++++++++++++++++++++++++++------------------ 3 files changed, 109 insertions(+), 76 deletions(-) diff --git a/zssp/src/counter.rs b/zssp/src/counter.rs index 2f7fa0749..8b5c9f789 100644 --- a/zssp/src/counter.rs +++ b/zssp/src/counter.rs @@ -17,9 +17,10 @@ impl Counter { // helps randomize packet contents a bit. Self(AtomicU32::new(1u32)) } + #[inline(always)] - pub fn reset_after_initial_offer(&self) { - self.0.store(2u32, Ordering::SeqCst); + pub fn reset_for_initial_offer(&self) { + self.0.store(1u32, Ordering::SeqCst); } /// Get the value most recently used to send a packet. @@ -63,7 +64,8 @@ impl CounterWindow { pub fn new_invalid() -> Self { Self(std::array::from_fn(|_| AtomicU32::new(u32::MAX))) } - pub fn reset_after_initial_offer(&self) { + pub fn reset_for_initial_offer(&self) { + let o = true; for i in 0..COUNTER_MAX_ALLOWED_OOO { self.0[i].store(0, Ordering::SeqCst) } diff --git a/zssp/src/tests.rs b/zssp/src/tests.rs index b15a5e3f4..747b86dfb 100644 --- a/zssp/src/tests.rs +++ b/zssp/src/tests.rs @@ -254,7 +254,7 @@ mod tests { w.message_received(c); continue; } else { - w.reset_after_initial_offer(); + w.reset_for_initial_offer(); counter = 1u32; history = Vec::new(); continue; diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index 9fd678801..4112b9eb4 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -112,12 +112,12 @@ pub struct Session { /// An arbitrary application defined object associated with each session pub application_data: Application::Data, - send_counter: Counter, // Outgoing packet counter and nonce state - receive_window: [CounterWindow; 2], // Receive window for anti-replay and deduplication + header_check_cipher: Aes, // Cipher used for header check codes (not Noise related) + receive_windows: [CounterWindow; 2], // Receive window for anti-replay and deduplication + send_counters: [Counter; 2], // Outgoing packet counter and nonce state + state: RwLock, // Mutable parts of state (other than defrag buffers) psk: Secret<64>, // Arbitrary PSK provided by external code noise_ss: Secret<48>, // Static raw shared ECDH NIST P-384 key - header_check_cipher: Aes, // Cipher used for header check codes (not Noise related) - state: RwLock, // Mutable parts of state (other than defrag buffers) remote_s_public_blob_hash: [u8; 48], // SHA384(remote static public key blob) remote_s_public_p384_bytes: [u8; P384_PUBLIC_KEY_SIZE], // Remote NIST P-384 static public key @@ -127,7 +127,7 @@ pub struct Session { struct SessionMutableState { remote_session_id: Option, // The other side's 48-bit session ID session_keys: [Option; 2], // Buffers to store current, next, and last active key - cur_session_key_id: bool, // Pointer used for keys[] circular buffer + 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) } @@ -236,6 +236,7 @@ impl Session { &mut send, send_counter.next(), false, + false, local_session_id, None, app.get_local_s_public_blob(), @@ -254,11 +255,9 @@ impl Session { return Ok(Self { id: local_session_id, application_data, - send_counter, - receive_window: [CounterWindow::new(), CounterWindow::new_invalid()], - psk: psk.clone(), - noise_ss, header_check_cipher, + receive_windows: [CounterWindow::new(), CounterWindow::new_invalid()], + send_counters: [send_counter, Counter::new()], state: RwLock::new(SessionMutableState { remote_session_id: None, session_keys: [None, None], @@ -266,6 +265,8 @@ impl Session { offer, last_remote_offer: i64::MIN, }), + psk: psk.clone(), + noise_ss, remote_s_public_blob_hash: bob_s_public_blob_hash, remote_s_public_p384_bytes: bob_s_public.as_bytes().clone(), defrag: Mutex::new(RingBufferMap::new(random::xorshift64_random() as u32)), @@ -296,7 +297,7 @@ impl Session { let packet_len = data.len() + HEADER_SIZE + AES_GCM_TAG_SIZE; // This outgoing packet's nonce counter value. - let counter = self.send_counter.next(); + let counter = self.send_counters[state.cur_session_key_id as usize].next(); //////////////////////////////////////////////////////////////// // packet encoding for post-noise transport @@ -403,10 +404,11 @@ impl Session { force_rekey: bool, ) { let state = self.state.read().unwrap(); + let current_key_id = state.cur_session_key_id; if (force_rekey || state.session_keys[state.cur_session_key_id as usize] .as_ref() - .map_or(true, |key| key.lifetime.should_rekey(self.send_counter.current(), current_time))) + .map_or(true, |key| key.lifetime.should_rekey(self.send_counters[current_key_id as usize].current(), current_time))) && state .offer .as_ref() @@ -416,8 +418,9 @@ impl Session { let mut offer = None; if send_ephemeral_offer( &mut send, - CounterValue::get_initial_offer_counter(), - !state.cur_session_key_id, + self.send_counters[current_key_id as usize].next(), + current_key_id, + !current_key_id, self.id, state.remote_session_id, app.get_local_s_public_blob(), @@ -425,7 +428,7 @@ impl Session { &remote_s_public, &self.remote_s_public_blob_hash, &self.noise_ss, - state.session_keys[state.cur_session_key_id as usize].as_ref(), + state.session_keys[current_key_id as usize].as_ref(), if state.remote_session_id.is_some() { Some(&self.header_check_cipher) } else { @@ -492,12 +495,13 @@ impl ReceiveContext { { if let Some(session) = app.lookup_session(local_session_id) { if verify_header_check_code(incoming_packet, &session.header_check_cipher) { - if session.receive_window[key_id as usize].message_received(counter) { + if session.receive_windows[key_id as usize].message_received(counter) { let canonical_header = CanonicalHeader::make(local_session_id, packet_type, counter); if fragment_count > 1 { if fragment_count <= (MAX_FRAGMENTS as u8) && fragment_no < fragment_count { let mut defrag = session.defrag.lock().unwrap(); - let fragment_gather_array = defrag.get_or_create_mut(&counter, || GatherArray::new(fragment_count)); + // by using the counter + the key_id as the key we can prevent packet collisions, this only works if defrag hashes + let fragment_gather_array = defrag.get_or_create_mut(&raw_counter, || GatherArray::new(fragment_count)); if let Some(assembled_packet) = fragment_gather_array.add(fragment_no, incoming_packet_buf) { drop(defrag); // release lock return self.receive_complete( @@ -672,7 +676,7 @@ impl ReceiveContext { session_key.return_receive_cipher(c); if aead_authentication_ok { - if session.receive_window[key_id as usize].message_authenticated(counter) { + 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 { @@ -832,7 +836,7 @@ impl ReceiveContext { // Perform checks and match ratchet key if there's an existing session, or gate (via host) and // then create new sessions. - let (new_session, ratchet_key, last_ratchet_count) = if let Some(session) = session.as_ref() { + let (new_session, reply_counter, new_key_id, ratchet_key, last_ratchet_count) = if let Some(session) = session.as_ref() { // Existing session identity must match the one in this offer. if !secure_eq(&session.remote_s_public_blob_hash, &SHA384::hash(&alice_s_public_blob)) { return Err(Error::FailedAuthentication); @@ -843,10 +847,12 @@ impl ReceiveContext { let mut ratchet_key = None; let mut last_ratchet_count = 0; let state = session.state.read().unwrap(); - if state.cur_session_key_id == key_id { - return Ok(ReceiveResult::Ignored); // alice is requesting to overwrite the current key, reject it + //key_id here is the key id of the key being rekeyed and replaced + //it must be equal to the current session key, and not the previous session key + if state.cur_session_key_id != key_id { + return Ok(ReceiveResult::Ignored); } - if let Some(k) = state.session_keys[state.cur_session_key_id as usize].as_ref() { + if let Some(k) = state.session_keys[key_id as usize].as_ref() { if public_fingerprint_of_secret(k.ratchet_key.as_bytes())[..16].eq(alice_ratchet_key_fingerprint) { ratchet_key = Some(k.ratchet_key.clone()); last_ratchet_count = k.ratchet_count; @@ -856,34 +862,41 @@ impl ReceiveContext { return Ok(ReceiveResult::Ignored); // old packet? } - (None, ratchet_key, last_ratchet_count) + (None, session.send_counters[key_id as usize].next(), !key_id, ratchet_key, last_ratchet_count) } else { + if key_id != false { + return Ok(ReceiveResult::Ignored); + } if let Some((new_session_id, psk, associated_object)) = app.accept_new_session(self, remote_address, alice_s_public_blob, alice_metadata) { let header_check_cipher = Aes::new( kbkdf512(noise_ss.as_bytes(), KBKDF_KEY_USAGE_LABEL_HEADER_CHECK).first_n::(), ); + let send_counter = Counter::new(); + let reply_counter = send_counter.next(); ( Some(Session:: { id: new_session_id, application_data: associated_object, - send_counter: Counter::new(), - receive_window: [CounterWindow::new_invalid(), CounterWindow::new_invalid()], - psk, - noise_ss, header_check_cipher, + receive_windows: [CounterWindow::new(), CounterWindow::new_invalid()], + send_counters: [send_counter, Counter::new()], state: RwLock::new(SessionMutableState { remote_session_id: Some(alice_session_id), - session_keys: [None, None], - cur_session_key_id: key_id, + 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, }), + psk, + noise_ss, remote_s_public_blob_hash: SHA384::hash(&alice_s_public_blob), remote_s_public_p384_bytes: alice_s_public.as_bytes().clone(), defrag: Mutex::new(RingBufferMap::new(random::xorshift64_random() as u32)), }), + reply_counter, + false, None, 0, ) @@ -946,7 +959,6 @@ impl ReceiveContext { //////////////////////////////////////////////////////////////// let mut reply_buf = [0_u8; KEX_BUF_LEN]; - let reply_counter = CounterValue::get_initial_offer_counter(); let mut idx = HEADER_SIZE; idx = safe_write_all(&mut reply_buf, idx, &[SESSION_PROTOCOL_VERSION])?; @@ -1016,33 +1028,40 @@ impl ReceiveContext { ); idx = safe_write_all(&mut reply_buf, idx, &hmac)?; 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, + last_ratchet_count + 1, + hybrid_kk.is_some(), + ); - let session_key = SessionKey::new( - session_key, - Role::Bob, - current_time, - last_ratchet_count + 1, - hybrid_kk.is_some(), - ); + //TODO: check for correct orderings + let mut state = session.state.write().unwrap(); + let _ = state.session_keys[new_key_id as usize].replace(session_key); + if existing_session.is_some() { + let _ = state.remote_session_id.replace(alice_session_id); + state.cur_session_key_id = new_key_id; + session.send_counters[new_key_id as usize].reset_for_initial_offer(); + session.receive_windows[new_key_id as usize].reset_for_initial_offer(); + } + drop(state); - let mut state = session.state.write().unwrap(); - let _ = state.remote_session_id.replace(alice_session_id); - let _ = state.session_keys[key_id as usize].replace(session_key); - session.send_counter.reset_after_initial_offer(); - state.cur_session_key_id = key_id; - session.receive_window[key_id as usize].reset_after_initial_offer(); - session.receive_window[key_id as usize].message_authenticated(counter); - drop(state); + // Bob now has final key state for this exchange. Yay! Now reply to Alice so she can construct it. - // Bob now has final key state for this exchange. Yay! Now reply to Alice so she can construct it. + send_with_fragmentation(send, &mut reply_buf[..packet_end], mtu, &session.header_check_cipher); - send_with_fragmentation(send, &mut reply_buf[..packet_end], mtu, &session.header_check_cipher); - - if let Some(new_session) = new_session { - return Ok(ReceiveResult::OkNewSession(new_session)); + if let Some(new_session) = new_session { + return Ok(ReceiveResult::OkNewSession(new_session)); + } else { + return Ok(ReceiveResult::Ok); + } } else { - return Ok(ReceiveResult::Ok); + return Ok(ReceiveResult::Ignored); } + } PACKET_TYPE_KEY_COUNTER_OFFER => { @@ -1096,8 +1115,9 @@ impl ReceiveContext { parse_dec_key_offer_after_header(&kex_packet[plaintext_end..kex_packet_len], packet_type)?; // Check that this is a counter offer to the original offer we sent. - if !(offer.id.eq(offer_id) & (offer.key_id == key_id)) { + if !(offer.id.eq(offer_id)) { return Ok(ReceiveResult::Ignored); + } // Kyber1024 key agreement if enabled. @@ -1138,27 +1158,36 @@ impl ReceiveContext { ) { return Err(Error::FailedAuthentication); } + if session.receive_windows[key_id as usize].message_authenticated(counter) { + // Alice has now completed and validated the full hybrid exchange. - // Alice has now completed and validated the full hybrid exchange. + let session_key = SessionKey::new( + session_key, + Role::Alice, + current_time, + last_ratchet_count + 1, + hybrid_kk.is_some(), + ); - let session_key = SessionKey::new( - session_key, - Role::Alice, - current_time, - last_ratchet_count + 1, - hybrid_kk.is_some(), - ); + let new_key_id = offer.key_id; + let is_new_session = offer.ratchet_count == 0; + drop(state); + //TODO: check for correct orderings + let mut state = session.state.write().unwrap(); + let _ = state.remote_session_id.replace(bob_session_id); + let _ = state.session_keys[new_key_id as usize].replace(session_key); + if !is_new_session { + //when an brand new key offer is sent, it is sent using the new_key_id==false counter, we cannot reset it in that case. + state.cur_session_key_id = new_key_id; + session.send_counters[new_key_id as usize].reset_for_initial_offer(); + session.receive_windows[new_key_id as usize].reset_for_initial_offer(); + } + let _ = state.offer.take(); - drop(state); - let mut state = session.state.write().unwrap(); - let _ = state.remote_session_id.replace(bob_session_id); - let _ = state.session_keys[key_id as usize].replace(session_key); - session.send_counter.reset_after_initial_offer(); - state.cur_session_key_id = key_id; - session.receive_window[key_id as usize].message_authenticated(counter); - let _ = state.offer.take(); - - return Ok(ReceiveResult::Ok); + return Ok(ReceiveResult::Ok); + } else { + return Ok(ReceiveResult::Ignored); + } } } @@ -1174,10 +1203,12 @@ impl ReceiveContext { } /// Create an send an ephemeral offer, populating ret_ephemeral_offer on success. +/// If there is no current session key set `current_key_id == new_key_id == false` fn send_ephemeral_offer( send: &mut SendFunction, counter: CounterValue, - key_id: bool, + current_key_id: bool, + new_key_id: bool, alice_session_id: SessionId, bob_session_id: Option, alice_s_public_blob: &[u8], @@ -1263,7 +1294,7 @@ fn send_ephemeral_offer( PACKET_TYPE_INITIAL_KEY_OFFER, bob_session_id, counter, - key_id, + current_key_id, )?; let canonical_header = CanonicalHeader::make(bob_session_id, PACKET_TYPE_INITIAL_KEY_OFFER, counter.to_u32()); @@ -1317,7 +1348,7 @@ fn send_ephemeral_offer( *ret_ephemeral_offer = Some(EphemeralOffer { id, - key_id, + key_id: new_key_id, creation_time: current_time, ratchet_count, ratchet_key, From bf3591f593bd6e0a4cb2cf9b77bb5211a1f3891d Mon Sep 17 00:00:00 2001 From: mamoniot Date: Tue, 27 Dec 2022 21:09:39 -0500 Subject: [PATCH 13/34] cleared warnings --- zssp/src/counter.rs | 4 ---- zssp/src/tests.rs | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/zssp/src/counter.rs b/zssp/src/counter.rs index 8b5c9f789..39ca6923d 100644 --- a/zssp/src/counter.rs +++ b/zssp/src/counter.rs @@ -47,9 +47,6 @@ impl CounterValue { pub fn to_u32(&self) -> u32 { self.0 as u32 } - pub fn get_initial_offer_counter() -> CounterValue { - return CounterValue(1u32); - } } /// Incoming packet deduplication and replay protection window. @@ -65,7 +62,6 @@ impl CounterWindow { Self(std::array::from_fn(|_| AtomicU32::new(u32::MAX))) } pub fn reset_for_initial_offer(&self) { - let o = true; for i in 0..COUNTER_MAX_ALLOWED_OOO { self.0[i].store(0, Ordering::SeqCst) } diff --git a/zssp/src/tests.rs b/zssp/src/tests.rs index 747b86dfb..a72e3ffea 100644 --- a/zssp/src/tests.rs +++ b/zssp/src/tests.rs @@ -233,7 +233,7 @@ mod tests { let mut history = Vec::new(); let w = CounterWindow::new(); - for i in 0..1000000 { + for _i in 0..1000000 { let p = xorshift64(&mut rng) as f32/(u32::MAX as f32 + 1.0); let c; if p < 0.5 { From d3d7cc1a3ceb70e401a3f785eec7de32f039c287 Mon Sep 17 00:00:00 2001 From: mamoniot Date: Tue, 27 Dec 2022 22:00:49 -0500 Subject: [PATCH 14/34] completed audit for threadsafety --- zssp/src/counter.rs | 4 +--- zssp/src/zssp.rs | 34 ++++++++++++++++++++-------------- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/zssp/src/counter.rs b/zssp/src/counter.rs index 39ca6923d..471007a42 100644 --- a/zssp/src/counter.rs +++ b/zssp/src/counter.rs @@ -3,10 +3,8 @@ use std::sync::atomic::{Ordering, AtomicU32}; use crate::constants::COUNTER_MAX_ALLOWED_OOO; /// Outgoing packet counter with strictly ordered atomic semantics. +/// Count sequence always starts at 1u32, it must never be allowed to overflow /// -/// The counter used in packets is actually 32 bits, but using a 64-bit integer internally -/// lets us more safely implement key lifetime limits without confusing logic to handle 32-bit -/// wrap-around. #[repr(transparent)] pub(crate) struct Counter(AtomicU32); diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index 4112b9eb4..47fe6c641 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -81,7 +81,8 @@ pub enum ReceiveResult<'a, H: ApplicationLayer> { /// The session will have already been gated by the accept_new_session() method in the Host trait. OkNewSession(Session), - /// Packet appears valid but was ignored e.g. as a duplicate. + /// Packet superficially appears valid but was ignored e.g. as a duplicate. + /// IMPORTANT: Authentication was not completed on this packet, so for the most part treat this the same as an Error::FailedAuthentication Ignored, } @@ -114,7 +115,6 @@ pub struct Session { header_check_cipher: Aes, // Cipher used for header check codes (not Noise related) receive_windows: [CounterWindow; 2], // Receive window for anti-replay and deduplication - send_counters: [Counter; 2], // Outgoing packet counter and nonce state state: RwLock, // Mutable parts of state (other than defrag buffers) psk: Secret<64>, // Arbitrary PSK provided by external code noise_ss: Secret<48>, // Static raw shared ECDH NIST P-384 key @@ -126,6 +126,7 @@ 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 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 @@ -257,8 +258,8 @@ impl Session { application_data, header_check_cipher, receive_windows: [CounterWindow::new(), CounterWindow::new_invalid()], - send_counters: [send_counter, Counter::new()], state: RwLock::new(SessionMutableState { + send_counters: [send_counter, Counter::new()], remote_session_id: None, session_keys: [None, None], cur_session_key_id: false, @@ -297,7 +298,7 @@ impl Session { let packet_len = data.len() + HEADER_SIZE + AES_GCM_TAG_SIZE; // This outgoing packet's nonce counter value. - let counter = self.send_counters[state.cur_session_key_id as usize].next(); + let counter = state.send_counters[state.cur_session_key_id as usize].next(); //////////////////////////////////////////////////////////////// // packet encoding for post-noise transport @@ -408,7 +409,7 @@ impl Session { if (force_rekey || state.session_keys[state.cur_session_key_id as usize] .as_ref() - .map_or(true, |key| key.lifetime.should_rekey(self.send_counters[current_key_id as usize].current(), current_time))) + .map_or(true, |key| key.lifetime.should_rekey(state.send_counters[current_key_id as usize].current(), current_time))) && state .offer .as_ref() @@ -418,7 +419,7 @@ impl Session { let mut offer = None; if send_ephemeral_offer( &mut send, - self.send_counters[current_key_id as usize].next(), + state.send_counters[current_key_id as usize].next(), current_key_id, !current_key_id, self.id, @@ -494,8 +495,8 @@ impl ReceiveContext { if let Some(local_session_id) = SessionId::new_from_u64(u64::from_le(memory::load_raw(&incoming_packet[8..16])) & 0xffffffffffffu64) { if let Some(session) = app.lookup_session(local_session_id) { - if verify_header_check_code(incoming_packet, &session.header_check_cipher) { - if session.receive_windows[key_id as usize].message_received(counter) { + if session.receive_windows[key_id as usize].message_received(counter) { + if verify_header_check_code(incoming_packet, &session.header_check_cipher) { let canonical_header = CanonicalHeader::make(local_session_id, packet_type, counter); if fragment_count > 1 { if fragment_count <= (MAX_FRAGMENTS as u8) && fragment_no < fragment_count { @@ -541,11 +542,11 @@ impl ReceiveContext { } } else { unlikely_branch(); - return Ok(ReceiveResult::Ignored); + return Err(Error::FailedAuthentication); } } else { unlikely_branch(); - return Err(Error::FailedAuthentication); + return Ok(ReceiveResult::Ignored); } } else { unlikely_branch(); @@ -862,7 +863,7 @@ impl ReceiveContext { return Ok(ReceiveResult::Ignored); // old packet? } - (None, session.send_counters[key_id as usize].next(), !key_id, ratchet_key, last_ratchet_count) + (None, state.send_counters[state.cur_session_key_id as usize].next(), !key_id, ratchet_key, last_ratchet_count) } else { if key_id != false { return Ok(ReceiveResult::Ignored); @@ -881,8 +882,8 @@ impl ReceiveContext { application_data: associated_object, header_check_cipher, receive_windows: [CounterWindow::new(), CounterWindow::new_invalid()], - send_counters: [send_counter, Counter::new()], 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 cur_session_key_id: false, @@ -1043,8 +1044,12 @@ impl ReceiveContext { let _ = state.session_keys[new_key_id as usize].replace(session_key); if existing_session.is_some() { let _ = state.remote_session_id.replace(alice_session_id); + //if this wasn't done inside a lock, a theoretical race condition exists where a thread uses the new key id before the counter is reset, or worse: a thread has held onto the previous key_id == new_key_id, and attempts to use the reset counter + //for this reason do not access send_counters without holding the read lock state.cur_session_key_id = new_key_id; - session.send_counters[new_key_id as usize].reset_for_initial_offer(); + state.send_counters[new_key_id as usize].reset_for_initial_offer(); + //receive_windows only has race conditions with the send 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 both very unlikely and can only ever cause dropped packets under the current implementation of receive_window. + //if receive_window is ever reimplemented, double check it maintains the above property. session.receive_windows[new_key_id as usize].reset_for_initial_offer(); } drop(state); @@ -1178,8 +1183,9 @@ impl ReceiveContext { let _ = state.session_keys[new_key_id as usize].replace(session_key); if !is_new_session { //when an brand new key offer is sent, it is sent using the new_key_id==false counter, we cannot reset it in that case. + //NOTE: the following code should be properly threadsafe, see the large comment above at the end of KEY_OFFER decoding for more info state.cur_session_key_id = new_key_id; - session.send_counters[new_key_id as usize].reset_for_initial_offer(); + state.send_counters[new_key_id as usize].reset_for_initial_offer(); session.receive_windows[new_key_id as usize].reset_for_initial_offer(); } let _ = state.offer.take(); From eab4c3db3ccca70be9c34979ab9e42e64930f3ff Mon Sep 17 00:00:00 2001 From: mamoniot Date: Tue, 27 Dec 2022 22:02:25 -0500 Subject: [PATCH 15/34] updated comment --- zssp/src/zssp.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index 47fe6c641..e83d45073 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -82,7 +82,7 @@ pub enum ReceiveResult<'a, H: ApplicationLayer> { OkNewSession(Session), /// Packet superficially appears valid but was ignored e.g. as a duplicate. - /// IMPORTANT: Authentication was not completed on this packet, so for the most part treat this the same as an Error::FailedAuthentication + /// IMPORTANT: This pack was not authenticated, so for the most part treat this the same as an Error::FailedAuthentication Ignored, } From eb6d5f94ec64c450ea54179d20e82d598aa4897f Mon Sep 17 00:00:00 2001 From: mamoniot Date: Tue, 27 Dec 2022 22:16:28 -0500 Subject: [PATCH 16/34] reverted bad change --- zssp/src/zssp.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index e83d45073..a02d5cd78 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -495,8 +495,8 @@ impl ReceiveContext { if let Some(local_session_id) = SessionId::new_from_u64(u64::from_le(memory::load_raw(&incoming_packet[8..16])) & 0xffffffffffffu64) { if let Some(session) = app.lookup_session(local_session_id) { - if session.receive_windows[key_id as usize].message_received(counter) { - if verify_header_check_code(incoming_packet, &session.header_check_cipher) { + if verify_header_check_code(incoming_packet, &session.header_check_cipher) { + if session.receive_windows[key_id as usize].message_received(counter) { let canonical_header = CanonicalHeader::make(local_session_id, packet_type, counter); if fragment_count > 1 { if fragment_count <= (MAX_FRAGMENTS as u8) && fragment_no < fragment_count { @@ -542,11 +542,11 @@ impl ReceiveContext { } } else { unlikely_branch(); - return Err(Error::FailedAuthentication); + return Ok(ReceiveResult::Ignored); } } else { unlikely_branch(); - return Ok(ReceiveResult::Ignored); + return Err(Error::FailedAuthentication); } } else { unlikely_branch(); From dea6ec2a1e490e0bd9a2591e79dd882d02688d9a Mon Sep 17 00:00:00 2001 From: mamoniot Date: Tue, 27 Dec 2022 22:47:22 -0500 Subject: [PATCH 17/34] updated comments --- zssp/src/zssp.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index a02d5cd78..b55cf8b3f 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -1043,14 +1043,18 @@ impl ReceiveContext { let mut state = session.state.write().unwrap(); let _ = state.session_keys[new_key_id as usize].replace(session_key); if existing_session.is_some() { + // 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 is rejected + // 2) The received counter is greater than what is currently stored in the window, so a valid packet is accepted *but* its counter is deleted from the window so it can be replayed + // 1 is completely acceptable behavior; 2 is unacceptable, but extremely extremely unlikely. Since it is utterly impractical for an adversary to trigger 2 intentionally, and preventing 2 is expensive, we do not currently plan to prevent it. + // if receive_window is ever reimplemented, double check it maintains the above properties. + session.receive_windows[new_key_id as usize].reset_for_initial_offer(); + let _ = state.remote_session_id.replace(alice_session_id); - //if this wasn't done inside a lock, a theoretical race condition exists where a thread uses the new key id before the counter is reset, or worse: a thread has held onto the previous key_id == new_key_id, and attempts to use the reset counter - //for this reason do not access send_counters without holding the read lock + // if this wasn't done inside a lock, a theoretical race condition exists where a thread uses the new key id before the counter is reset, or worse: a thread has held onto the previous key_id == new_key_id, and attempts to use the reset counter + // for this reason do not access send_counters without holding the read lock state.cur_session_key_id = new_key_id; state.send_counters[new_key_id as usize].reset_for_initial_offer(); - //receive_windows only has race conditions with the send 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 both very unlikely and can only ever cause dropped packets under the current implementation of receive_window. - //if receive_window is ever reimplemented, double check it maintains the above property. - session.receive_windows[new_key_id as usize].reset_for_initial_offer(); } drop(state); @@ -1184,9 +1188,9 @@ impl ReceiveContext { if !is_new_session { //when an brand new key offer is sent, it is sent using the new_key_id==false counter, we cannot reset it in that case. //NOTE: the following code should be properly threadsafe, see the large comment above at the end of KEY_OFFER decoding for more info + session.receive_windows[new_key_id as usize].reset_for_initial_offer(); state.cur_session_key_id = new_key_id; state.send_counters[new_key_id as usize].reset_for_initial_offer(); - session.receive_windows[new_key_id as usize].reset_for_initial_offer(); } let _ = state.offer.take(); From 4738cedc49680e3e865c4a3babe2a1aa3610d807 Mon Sep 17 00:00:00 2001 From: mamoniot Date: Tue, 27 Dec 2022 22:52:11 -0500 Subject: [PATCH 18/34] updated test --- zssp/src/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zssp/src/tests.rs b/zssp/src/tests.rs index a72e3ffea..0ce32eddf 100644 --- a/zssp/src/tests.rs +++ b/zssp/src/tests.rs @@ -228,7 +228,7 @@ mod tests { } #[test] fn counter_window() { - let mut rng = 84632; + let mut rng = 844632; let mut counter = 1u32; let mut history = Vec::new(); @@ -238,7 +238,7 @@ mod tests { let c; if p < 0.5 { let r = xorshift64(&mut rng); - c = counter.wrapping_add(r%(COUNTER_MAX_ALLOWED_OOO - 2) as u32 + 1); + c = counter.wrapping_add(r%(COUNTER_MAX_ALLOWED_OOO - 1) as u32 + 1); } else if p < 0.8 { counter = counter.wrapping_add(1); c = counter; From 87b40cd1a017b11d44a00db9e9415a930f391f1a Mon Sep 17 00:00:00 2001 From: mamoniot Date: Tue, 27 Dec 2022 22:56:15 -0500 Subject: [PATCH 19/34] updated test --- zssp/src/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zssp/src/tests.rs b/zssp/src/tests.rs index 0ce32eddf..a20880877 100644 --- a/zssp/src/tests.rs +++ b/zssp/src/tests.rs @@ -238,9 +238,9 @@ mod tests { let c; if p < 0.5 { let r = xorshift64(&mut rng); - c = counter.wrapping_add(r%(COUNTER_MAX_ALLOWED_OOO - 1) as u32 + 1); + c = counter + (r%(COUNTER_MAX_ALLOWED_OOO - 1) as u32 + 1); } else if p < 0.8 { - counter = counter.wrapping_add(1); + counter = counter + (1); c = counter; } else if p < 0.9 { if history.len() > 0 { From c90faab4c0bfdaaacaeba9f033b197c6baa8cae4 Mon Sep 17 00:00:00 2001 From: mamoniot Date: Wed, 28 Dec 2022 05:53:18 -0500 Subject: [PATCH 20/34] prevented minor attack --- zssp/src/counter.rs | 7 ++++++- zssp/src/zssp.rs | 2 ++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/zssp/src/counter.rs b/zssp/src/counter.rs index 471007a42..a505bbc2c 100644 --- a/zssp/src/counter.rs +++ b/zssp/src/counter.rs @@ -64,6 +64,11 @@ impl CounterWindow { self.0[i].store(0, Ordering::SeqCst) } } + pub fn invalidate(&self) { + for i in 0..COUNTER_MAX_ALLOWED_OOO { + self.0[i].store(u32::MAX, Ordering::SeqCst) + } + } #[inline(always)] pub fn message_received(&self, received_counter_value: u32) -> bool { @@ -75,7 +80,7 @@ impl CounterWindow { #[inline(always)] pub fn message_authenticated(&self, received_counter_value: u32) -> bool { - //if a valid message is received but one of its fragments was lost, it can technically be replayed. However since the message is incomplete, we know it still exists in the gather array, so the gather array will deduplicate the replayed message. Even if the gather array gets flushed, that flush still effectively deduplicates the replayed message. + //if a valid message is received but one of its fragments was lost, it can technically be replayed. However since the message is incomplete, we know it still exists in the gather array, so the gather array will deduplicate the replayed message. Even if the gather array gets flushed, that flush still effectively deduplicates the replayed message. //eventually the counter of that kind of message will be too OOO to be accepted anymore so it can't be used to DOS. let idx = (received_counter_value % COUNTER_MAX_ALLOWED_OOO as u32) as usize; return self.0[idx].fetch_max(received_counter_value, Ordering::SeqCst) < received_counter_value; diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index b55cf8b3f..cc53c8cad 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -416,6 +416,8 @@ impl Session { .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) { + //mark the previous key as no longer being supported because it is about to be overwritten + self.receive_windows[(!current_key_id) as usize].invalidate(); let mut offer = None; if send_ephemeral_offer( &mut send, From 53fe95c92314b2dae82581cfdc99b86047b0d2a4 Mon Sep 17 00:00:00 2001 From: mamoniot Date: Wed, 28 Dec 2022 10:39:44 -0500 Subject: [PATCH 21/34] finished implementing ratchet count salting --- zssp/src/zssp.rs | 104 ++++++++++++++++++++++++++++------------------- 1 file changed, 62 insertions(+), 42 deletions(-) diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index cc53c8cad..78144f429 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -3,6 +3,7 @@ // ZSSP: ZeroTier Secure Session Protocol // FIPS compliant Noise_IK with Jedi powers and built-in attack-resistant large payload (fragmentation) support. +use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::{Mutex, RwLock}; use zerotier_crypto::aes::{Aes, AesGcm}; @@ -113,6 +114,7 @@ pub struct Session { /// An arbitrary application defined object associated with each session pub application_data: Application::Data, + ratchet_counts: [AtomicU64; 2], // Number of preceding session keys in ratchet header_check_cipher: Aes, // Cipher used for header check codes (not Noise related) receive_windows: [CounterWindow; 2], // Receive window for anti-replay and deduplication state: RwLock, // Mutable parts of state (other than defrag buffers) @@ -144,7 +146,6 @@ struct SessionKey { send_key: Secret, // Send side AES-GCM key receive_cipher_pool: Mutex>>, // Pool of reusable sending ciphers send_cipher_pool: Mutex>>, // Pool of reusable receiving ciphers - ratchet_count: u64, // Number of preceding session keys in ratchet jedi: bool, // True if Kyber1024 was used (both sides enabled) } @@ -246,6 +247,7 @@ impl Session { &bob_s_public_blob_hash, &noise_ss, None, + 1, None, mtu, current_time, @@ -256,6 +258,7 @@ impl Session { return Ok(Self { id: local_session_id, application_data, + ratchet_counts: [AtomicU64::new(1), AtomicU64::new(0)], header_check_cipher, receive_windows: [CounterWindow::new(), CounterWindow::new_invalid()], state: RwLock::new(SessionMutableState { @@ -293,12 +296,14 @@ impl Session { debug_assert!(mtu_sized_buffer.len() >= MIN_TRANSPORT_MTU); let state = self.state.read().unwrap(); if let Some(remote_session_id) = state.remote_session_id { - if let Some(session_key) = state.session_keys[state.cur_session_key_id as usize].as_ref() { + let key_id = state.cur_session_key_id; + if let Some(session_key) = state.session_keys[key_id as usize].as_ref() { // Total size of the armored packet we are going to send (may end up being fragmented) let packet_len = data.len() + HEADER_SIZE + AES_GCM_TAG_SIZE; - + //key ratchet count to be used for salting + let ratchet_count = self.ratchet_counts[key_id as usize].load(Ordering::Relaxed); // This outgoing packet's nonce counter value. - let counter = state.send_counters[state.cur_session_key_id as usize].next(); + let counter = state.send_counters[key_id as usize].next(); //////////////////////////////////////////////////////////////// // packet encoding for post-noise transport @@ -312,7 +317,7 @@ impl Session { PACKET_TYPE_DATA, remote_session_id.into(), counter, - state.cur_session_key_id + key_id )?; // Get an initialized AES-GCM cipher and re-initialize with a 96-bit IV built from remote session ID, @@ -331,7 +336,7 @@ impl Session { let fragment_size = fragment_data_size + HEADER_SIZE; c.crypt(&data[..fragment_data_size], &mut mtu_sized_buffer[HEADER_SIZE..fragment_size]); data = &data[fragment_data_size..]; - set_header_check_code(mtu_sized_buffer, &self.header_check_cipher); + set_header_check_code(mtu_sized_buffer, ratchet_count, &self.header_check_cipher); send(&mut mtu_sized_buffer[..fragment_size]); debug_assert!(header[15].wrapping_shr(2) < 63); @@ -352,7 +357,7 @@ impl Session { c.crypt(data, &mut mtu_sized_buffer[HEADER_SIZE..payload_end]); let gcm_tag = c.finish_encrypt(); mtu_sized_buffer[payload_end..last_fragment_size].copy_from_slice(&gcm_tag); - set_header_check_code(mtu_sized_buffer, &self.header_check_cipher); + set_header_check_code(mtu_sized_buffer, ratchet_count, &self.header_check_cipher); send(&mut mtu_sized_buffer[..last_fragment_size]); // Check reusable AES-GCM instance back into pool. @@ -380,8 +385,9 @@ impl Session { /// and whether Kyber1024 was used. None is returned if the session isn't established. pub fn status(&self) -> Option<([u8; 16], i64, u64, bool)> { let state = self.state.read().unwrap(); - if let Some(key) = state.session_keys[state.cur_session_key_id as usize].as_ref() { - Some((key.secret_fingerprint, key.creation_time, key.ratchet_count, key.jedi)) + 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)) } else { None } @@ -419,6 +425,7 @@ impl Session { //mark the previous key as no longer being supported because it is about to be overwritten self.receive_windows[(!current_key_id) as usize].invalidate(); let mut offer = None; + //TODO: what happens here if the session is in a limbo state due to dropped packets? if send_ephemeral_offer( &mut send, state.send_counters[current_key_id as usize].next(), @@ -432,6 +439,7 @@ impl Session { &self.remote_s_public_blob_hash, &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 state.remote_session_id.is_some() { Some(&self.header_check_cipher) } else { @@ -497,7 +505,10 @@ impl ReceiveContext { if let Some(local_session_id) = SessionId::new_from_u64(u64::from_le(memory::load_raw(&incoming_packet[8..16])) & 0xffffffffffffu64) { if let Some(session) = app.lookup_session(local_session_id) { - if verify_header_check_code(incoming_packet, &session.header_check_cipher) { + //this is the only time ratchet_counts is ever accessed outside of a lock + //as such this read can be wrong, but that is incredibly unlikely since we are tracking the last two ratchet counts, and if it's wrong it just means we drop a packet that would have been dropped anyways for being too old or too new + let ratchet_count = session.ratchet_counts[key_id as usize].load(Ordering::SeqCst); + if verify_header_check_code(incoming_packet, ratchet_count, &session.header_check_cipher) { if session.receive_windows[key_id as usize].message_received(counter) { let canonical_header = CanonicalHeader::make(local_session_id, packet_type, counter); if fragment_count > 1 { @@ -556,8 +567,8 @@ impl ReceiveContext { } } else { unlikely_branch(); // we want data receive to be the priority branch, this is only occasionally used - - if verify_header_check_code(incoming_packet, &self.incoming_init_header_check_cipher) { + //salt with a known value so new sessions can be established + 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 { let mut defrag = self.initial_offer_defrag.lock().unwrap(); @@ -839,7 +850,7 @@ impl ReceiveContext { // Perform checks and match ratchet key if there's an existing session, or gate (via host) and // then create new sessions. - let (new_session, reply_counter, new_key_id, ratchet_key, last_ratchet_count) = if let Some(session) = session.as_ref() { + let (new_session, reply_counter, new_key_id, ratchet_key) = if let Some(session) = session.as_ref() { // Existing session identity must match the one in this offer. if !secure_eq(&session.remote_s_public_blob_hash, &SHA384::hash(&alice_s_public_blob)) { return Err(Error::FailedAuthentication); @@ -848,7 +859,6 @@ impl ReceiveContext { // Match ratchet key fingerprint and fail if no match, which likely indicates an old offer packet. let alice_ratchet_key_fingerprint = alice_ratchet_key_fingerprint.unwrap(); let mut ratchet_key = None; - let mut last_ratchet_count = 0; let state = session.state.read().unwrap(); //key_id here is the key id of the key being rekeyed and replaced //it must be equal to the current session key, and not the previous session key @@ -858,14 +868,13 @@ impl ReceiveContext { if let Some(k) = state.session_keys[key_id as usize].as_ref() { if public_fingerprint_of_secret(k.ratchet_key.as_bytes())[..16].eq(alice_ratchet_key_fingerprint) { ratchet_key = Some(k.ratchet_key.clone()); - last_ratchet_count = k.ratchet_count; } } if ratchet_key.is_none() { return Ok(ReceiveResult::Ignored); // old packet? } - (None, state.send_counters[state.cur_session_key_id as usize].next(), !key_id, ratchet_key, last_ratchet_count) + (None, state.send_counters[state.cur_session_key_id as usize].next(), !key_id, ratchet_key) } else { if key_id != false { return Ok(ReceiveResult::Ignored); @@ -882,6 +891,7 @@ impl ReceiveContext { Some(Session:: { id: new_session_id, application_data: associated_object, + ratchet_counts: [AtomicU64::new(1), AtomicU64::new(0)], header_check_cipher, receive_windows: [CounterWindow::new(), CounterWindow::new_invalid()], state: RwLock::new(SessionMutableState { @@ -900,8 +910,7 @@ impl ReceiveContext { }), reply_counter, false, - None, - 0, + None ) } else { return Err(Error::NewSessionRejected); @@ -1037,32 +1046,34 @@ impl ReceiveContext { session_key, Role::Bob, current_time, - last_ratchet_count + 1, hybrid_kk.is_some(), ); - //TODO: check for correct orderings + let ratchet_count; let mut state = session.state.write().unwrap(); let _ = state.session_keys[new_key_id as usize].replace(session_key); if existing_session.is_some() { + 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 is rejected // 2) The received counter is greater than what is currently stored in the window, so a valid packet is accepted *but* its counter is deleted from the window so it can be replayed - // 1 is completely acceptable behavior; 2 is unacceptable, but extremely extremely unlikely. Since it is utterly impractical for an adversary to trigger 2 intentionally, and preventing 2 is expensive, we do not currently plan to prevent it. - // if receive_window is ever reimplemented, double check it maintains the above properties. + // To prevent these race conditions, we only update the ratchet_count for salting the check code after the window has reset. So if a counter passes the initial check code: it either means the thread sees ratchet count has been update, therefore it either sees receive_window has been reset (due to memory orderings), or it means a rare accidental check forge has occurred. session.receive_windows[new_key_id as usize].reset_for_initial_offer(); + ratchet_count = session.ratchet_counts[new_key_id as usize].fetch_add(2, Ordering::SeqCst) + 1; let _ = state.remote_session_id.replace(alice_session_id); - // if this wasn't done inside a lock, a theoretical race condition exists where a thread uses the new key id before the counter is reset, or worse: a thread has held onto the previous key_id == new_key_id, and attempts to use the reset counter + // if the following wasn't done inside a lock, a theoretical race condition exists where a thread uses the new key id before the counter is reset, or worse: a thread has held onto the previous key_id == new_key_id, and attempts to use the reset counter // for this reason do not access send_counters without holding the read lock state.cur_session_key_id = new_key_id; state.send_counters[new_key_id as usize].reset_for_initial_offer(); + } else { + ratchet_count = 1; } drop(state); // Bob now has final key state for this exchange. Yay! Now reply to Alice so she can construct it. - send_with_fragmentation(send, &mut reply_buf[..packet_end], mtu, &session.header_check_cipher); + send_with_fragmentation(send, &mut reply_buf[..packet_end], mtu, ratchet_count, &session.header_check_cipher); if let Some(new_session) = new_session { return Ok(ReceiveResult::OkNewSession(new_session)); @@ -1176,21 +1187,20 @@ impl ReceiveContext { session_key, Role::Alice, current_time, - last_ratchet_count + 1, hybrid_kk.is_some(), ); let new_key_id = offer.key_id; - let is_new_session = offer.ratchet_count == 0; drop(state); //TODO: check for correct orderings let mut state = session.state.write().unwrap(); let _ = state.remote_session_id.replace(bob_session_id); let _ = state.session_keys[new_key_id as usize].replace(session_key); - if !is_new_session { + if last_ratchet_count > 0 { //when an brand new key offer is sent, it is sent using the new_key_id==false counter, we cannot reset it in that case. //NOTE: the following code should be properly threadsafe, see the large comment above at the end of KEY_OFFER decoding for more info session.receive_windows[new_key_id as usize].reset_for_initial_offer(); + let _ = session.ratchet_counts[new_key_id as usize].fetch_add(2, Ordering::SeqCst).wrapping_add(2); state.cur_session_key_id = new_key_id; state.send_counters[new_key_id as usize].reset_for_initial_offer(); } @@ -1229,6 +1239,7 @@ fn send_ephemeral_offer( bob_s_public_blob_hash: &[u8], noise_ss: &Secret<48>, current_key: Option<&SessionKey>, + ratchet_count: u64, header_check_cipher: Option<&Aes>, // None to use one based on the recipient's public key for initial contact mtu: usize, current_time: i64, @@ -1248,10 +1259,10 @@ fn send_ephemeral_offer( }; // Get ratchet key for current key if one exists. - let (ratchet_key, ratchet_count) = if let Some(current_key) = current_key { - (Some(current_key.ratchet_key.clone()), current_key.ratchet_count) + let ratchet_key = if let Some(current_key) = current_key { + Some(current_key.ratchet_key.clone()) } else { - (None, 0) + None }; // Random ephemeral offer ID @@ -1348,12 +1359,13 @@ fn send_ephemeral_offer( let packet_end = idx; if let Some(header_check_cipher) = header_check_cipher { - send_with_fragmentation(send, &mut packet_buf[..packet_end], mtu, header_check_cipher); + send_with_fragmentation(send, &mut packet_buf[..packet_end], mtu, ratchet_count, header_check_cipher); } else { send_with_fragmentation( send, &mut packet_buf[..packet_end], mtu, + ratchet_count, &Aes::new(kbkdf512(&bob_s_public_blob_hash, KBKDF_KEY_USAGE_LABEL_HEADER_CHECK).first_n::()), ); } @@ -1418,6 +1430,7 @@ fn send_with_fragmentation( send: &mut SendFunction, packet: &mut [u8], mtu: usize, + ratchet_count: u64, header_check_cipher: &Aes, ) { let packet_len = packet.len(); @@ -1426,7 +1439,7 @@ fn send_with_fragmentation( let mut header: [u8; 16] = packet[..HEADER_SIZE].try_into().unwrap(); loop { let fragment = &mut packet[fragment_start..fragment_end]; - set_header_check_code(fragment, header_check_cipher); + set_header_check_code(fragment, ratchet_count, header_check_cipher); send(fragment); if fragment_end < packet_len { debug_assert!(header[15].wrapping_shr(2) < 63); @@ -1442,19 +1455,28 @@ fn send_with_fragmentation( } /// Set 32-bit header check code, used to make fragmentation mechanism robust. -fn set_header_check_code(packet: &mut [u8], header_check_cipher: &Aes) { +fn set_header_check_code(packet: &mut [u8], ratchet_count: u64, header_check_cipher: &Aes) { debug_assert!(packet.len() >= MIN_PACKET_SIZE); - let mut check_code = 0u128.to_ne_bytes(); - header_check_cipher.encrypt_block(&packet[4..20], &mut check_code); - packet[..4].copy_from_slice(&check_code[..4]); + + let mut header_mac = 0u128.to_le_bytes(); + memory::store_raw((ratchet_count as u16).to_le_bytes(), &mut header_mac[0..2]); + header_mac[2..16].copy_from_slice(&packet[4..18]); + header_check_cipher.encrypt_block_in_place(&mut header_mac); + packet[..4].copy_from_slice(&header_mac[..4]); } /// Verify 32-bit header check code. -fn verify_header_check_code(packet: &[u8], header_check_cipher: &Aes) -> bool { +/// This is not nearly enough entropy to be cryptographically secure, it only is meant for making DOS attacks very hard +fn verify_header_check_code(packet: &[u8], ratchet_count: u64, header_check_cipher: &Aes) -> bool { debug_assert!(packet.len() >= MIN_PACKET_SIZE); - let mut header_mac = 0u128.to_ne_bytes(); - header_check_cipher.encrypt_block(&packet[4..20], &mut header_mac); - memory::load_raw::(&packet[..4]) == memory::load_raw::(&header_mac) + //2 bytes is the ratchet key + //12 bytes is the header we want to verify + //2 bytes is random salt from the encrypted message + let mut header_mac = 0u128.to_le_bytes(); + memory::store_raw((ratchet_count as u16).to_le_bytes(), &mut header_mac[0..2]); + header_mac[2..16].copy_from_slice(&packet[4..18]); + header_check_cipher.encrypt_block_in_place(&mut header_mac); + memory::load_raw::(&packet[..4]) == memory::load_raw::(&header_mac[..4]) } /// Parse KEY_OFFER and KEY_COUNTER_OFFER starting after the unencrypted public key part. @@ -1511,7 +1533,6 @@ impl SessionKey { key: Secret<64>, role: Role, current_time: i64, - ratchet_count: u64, jedi: bool, ) -> Self { let a2b: Secret = kbkdf512(key.as_bytes(), KBKDF_KEY_USAGE_LABEL_AES_GCM_ALICE_TO_BOB).first_n_clone(); @@ -1529,7 +1550,6 @@ impl SessionKey { send_key, receive_cipher_pool: Mutex::new(Vec::with_capacity(2)), send_cipher_pool: Mutex::new(Vec::with_capacity(2)), - ratchet_count, jedi, } } From 31f05bbd5e9402d741f8af341cf6fd17cf119f29 Mon Sep 17 00:00:00 2001 From: mamoniot Date: Wed, 28 Dec 2022 12:40:35 -0500 Subject: [PATCH 22/34] fixed duplicate rekey requests --- zssp/src/counter.rs | 4 ++-- zssp/src/tests.rs | 2 +- zssp/src/zssp.rs | 51 +++++++++++++++++++++++---------------------- 3 files changed, 29 insertions(+), 28 deletions(-) diff --git a/zssp/src/counter.rs b/zssp/src/counter.rs index a505bbc2c..2eaa558cc 100644 --- a/zssp/src/counter.rs +++ b/zssp/src/counter.rs @@ -17,7 +17,7 @@ impl Counter { } #[inline(always)] - pub fn reset_for_initial_offer(&self) { + pub fn reset_for_new_key_offer(&self) { self.0.store(1u32, Ordering::SeqCst); } @@ -59,7 +59,7 @@ impl CounterWindow { pub fn new_invalid() -> Self { Self(std::array::from_fn(|_| AtomicU32::new(u32::MAX))) } - pub fn reset_for_initial_offer(&self) { + pub fn reset_for_new_key_offer(&self) { for i in 0..COUNTER_MAX_ALLOWED_OOO { self.0[i].store(0, Ordering::SeqCst) } diff --git a/zssp/src/tests.rs b/zssp/src/tests.rs index a20880877..0b0f65fce 100644 --- a/zssp/src/tests.rs +++ b/zssp/src/tests.rs @@ -254,7 +254,7 @@ mod tests { w.message_received(c); continue; } else { - w.reset_for_initial_offer(); + w.reset_for_new_key_offer(); counter = 1u32; history = Vec::new(); continue; diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index 78144f429..0f3f8b05f 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -423,9 +423,10 @@ impl Session { { if let Some(remote_s_public) = P384PublicKey::from_bytes(&self.remote_s_public_p384_bytes) { //mark the previous key as no longer being supported because it is about to be overwritten + //it should not be possible for a session to accidentally invalidate the key currently in use solely because of the read lock self.receive_windows[(!current_key_id) as usize].invalidate(); let mut offer = None; - //TODO: what happens here if the session is in a limbo state due to dropped packets? + //the session will keep sending ephemeral offers until rekeying is successful if send_ephemeral_offer( &mut send, state.send_counters[current_key_id as usize].next(), @@ -568,6 +569,8 @@ 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 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 { @@ -860,11 +863,6 @@ impl ReceiveContext { let alice_ratchet_key_fingerprint = alice_ratchet_key_fingerprint.unwrap(); let mut ratchet_key = None; let state = session.state.read().unwrap(); - //key_id here is the key id of the key being rekeyed and replaced - //it must be equal to the current session key, and not the previous session key - if state.cur_session_key_id != key_id { - return Ok(ReceiveResult::Ignored); - } if let Some(k) = state.session_keys[key_id as usize].as_ref() { if public_fingerprint_of_secret(k.ratchet_key.as_bytes())[..16].eq(alice_ratchet_key_fingerprint) { ratchet_key = Some(k.ratchet_key.clone()); @@ -874,9 +872,9 @@ impl ReceiveContext { return Ok(ReceiveResult::Ignored); // old packet? } - (None, state.send_counters[state.cur_session_key_id as usize].next(), !key_id, ratchet_key) + (None, state.send_counters[key_id as usize].next(), !key_id, ratchet_key) } else { - if key_id != false { + if key_id != false {//all new sessions must start with key_id 0, this has no security implications return Ok(ReceiveResult::Ignored); } if let Some((new_session_id, psk, associated_object)) = @@ -1053,19 +1051,22 @@ impl ReceiveContext { let mut state = session.state.write().unwrap(); let _ = state.session_keys[new_key_id as usize].replace(session_key); if existing_session.is_some() { - 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 is rejected - // 2) The received counter is greater than what is currently stored in the window, so a valid packet is accepted *but* its counter is deleted from the window so it can be replayed - // To prevent these race conditions, we only update the ratchet_count for salting the check code after the window has reset. So if a counter passes the initial check code: it either means the thread sees ratchet count has been update, therefore it either sees receive_window has been reset (due to memory orderings), or it means a rare accidental check forge has occurred. - session.receive_windows[new_key_id as usize].reset_for_initial_offer(); - ratchet_count = session.ratchet_counts[new_key_id as usize].fetch_add(2, Ordering::SeqCst) + 1; - let _ = state.remote_session_id.replace(alice_session_id); - // if the following wasn't done inside a lock, a theoretical race condition exists where a thread uses the new key id before the counter is reset, or worse: a thread has held onto the previous key_id == new_key_id, and attempts to use the reset counter - // for this reason do not access send_counters without holding the read lock - state.cur_session_key_id = new_key_id; - state.send_counters[new_key_id as usize].reset_for_initial_offer(); + 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 + 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 + // 2) The received counter is greater than what is currently stored in the window, so a valid packet would be accepted *but* its counter is deleted from the window so it can be replayed + // To prevent these race conditions, we only update the ratchet_count for salting the check code after the window has reset. So if a counter passes the initial check code: it either means the thread sees ratchet count has been update, therefore it either sees receive_window has been reset (due to memory orderings), or it means a rare accidental check forge has occurred. + session.receive_windows[new_key_id as usize].reset_for_new_key_offer(); + session.ratchet_counts[new_key_id as usize].fetch_add(2, Ordering::SeqCst); + + // if the following wasn't done inside a lock, a theoretical race condition exists where a thread uses the new key id before the counter is reset, or worse: a thread has held onto the previous key_id == new_key_id, and attempts to use the reset counter + // for this reason do not access send_counters without holding the read lock + state.cur_session_key_id = new_key_id; + state.send_counters[new_key_id as usize].reset_for_new_key_offer(); + } } else { ratchet_count = 1; } @@ -1196,13 +1197,13 @@ impl ReceiveContext { let mut state = session.state.write().unwrap(); let _ = state.remote_session_id.replace(bob_session_id); let _ = state.session_keys[new_key_id as usize].replace(session_key); - if last_ratchet_count > 0 { + if last_ratchet_count > 0 && state.cur_session_key_id != new_key_id { //when an brand new key offer is sent, it is sent using the new_key_id==false counter, we cannot reset it in that case. //NOTE: the following code should be properly threadsafe, see the large comment above at the end of KEY_OFFER decoding for more info - session.receive_windows[new_key_id as usize].reset_for_initial_offer(); - let _ = session.ratchet_counts[new_key_id as usize].fetch_add(2, Ordering::SeqCst).wrapping_add(2); + session.receive_windows[new_key_id as usize].reset_for_new_key_offer(); + let _ = session.ratchet_counts[new_key_id as usize].fetch_add(2, Ordering::SeqCst); state.cur_session_key_id = new_key_id; - state.send_counters[new_key_id as usize].reset_for_initial_offer(); + state.send_counters[new_key_id as usize].reset_for_new_key_offer(); } let _ = state.offer.take(); @@ -1471,7 +1472,7 @@ fn verify_header_check_code(packet: &[u8], ratchet_count: u64, header_check_ciph debug_assert!(packet.len() >= MIN_PACKET_SIZE); //2 bytes is the ratchet key //12 bytes is the header we want to verify - //2 bytes is random salt from the encrypted message + //2 bytes is salt from the message let mut header_mac = 0u128.to_le_bytes(); memory::store_raw((ratchet_count as u16).to_le_bytes(), &mut header_mac[0..2]); header_mac[2..16].copy_from_slice(&packet[4..18]); From 4d16a30eac7c9851205b85c9c04024393acab5f7 Mon Sep 17 00:00:00 2001 From: mamoniot Date: Thu, 29 Dec 2022 13:04:29 -0500 Subject: [PATCH 23/34] implemented no double bobbing --- zssp/src/zssp.rs | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index 0f3f8b05f..bf1fcf3ce 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -91,6 +91,7 @@ pub enum ReceiveResult<'a, H: ApplicationLayer> { /// /// Note that the role can switch through the course of a session. It's the side that most recently /// initiated a session or a rekey event. Initiator is Alice, responder is Bob. +#[derive(PartialEq)] pub enum Role { Alice, Bob, @@ -147,6 +148,7 @@ struct SessionKey { receive_cipher_pool: Mutex>>, // Pool of reusable sending ciphers send_cipher_pool: Mutex>>, // Pool of reusable receiving ciphers jedi: bool, // True if Kyber1024 was used (both sides enabled) + role: Role, // The role of the local party that created this key } /// Key lifetime state @@ -400,7 +402,7 @@ impl Session { /// * `offer_metadata' - Any meta-data to include with initial key offers sent. /// * `mtu` - Current physical transport MTU /// * `current_time` - Current monotonic time in milliseconds - /// * `force_rekey` - Re-key the session now regardless of key aging (still subject to rate limiting) + /// * `assume_key_is_too_old` - Re-key the session now if the protocol allows it (subject to rate limits and whether it is the local party's turn to rekey) pub fn service( &self, app: &Application, @@ -408,14 +410,13 @@ impl Session { offer_metadata: &[u8], mtu: usize, current_time: i64, - force_rekey: bool, + assume_key_is_too_old: bool, ) { let state = self.state.read().unwrap(); let current_key_id = state.cur_session_key_id; - if (force_rekey - || state.session_keys[state.cur_session_key_id as usize] + if (state.session_keys[state.cur_session_key_id as usize] .as_ref() - .map_or(true, |key| key.lifetime.should_rekey(state.send_counters[current_key_id as usize].current(), current_time))) + .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() @@ -864,6 +865,11 @@ impl ReceiveContext { let mut ratchet_key = None; let state = session.state.read().unwrap(); if let Some(k) = state.session_keys[key_id as usize].as_ref() { + if k.role == Role::Bob { + // The local party is not allowed to be Bob twice in a row + // this prevents rekeying failure from both parties attempting to rekey at the same time + return Ok(ReceiveResult::Ignored); + } if public_fingerprint_of_secret(k.ratchet_key.as_bytes())[..16].eq(alice_ratchet_key_fingerprint) { ratchet_key = Some(k.ratchet_key.clone()); } @@ -874,7 +880,8 @@ impl ReceiveContext { (None, state.send_counters[key_id as usize].next(), !key_id, ratchet_key) } else { - if key_id != false {//all new sessions must start with key_id 0, this has no security implications + if key_id != false { + //all new sessions must start with key_id 0, this has no security implications return Ok(ReceiveResult::Ignored); } if let Some((new_session_id, psk, associated_object)) = @@ -1552,6 +1559,7 @@ impl SessionKey { receive_cipher_pool: Mutex::new(Vec::with_capacity(2)), send_cipher_pool: Mutex::new(Vec::with_capacity(2)), jedi, + role, } } From e1e73975fe2c3ad3e0e19d20231753877d060c89 Mon Sep 17 00:00:00 2001 From: mamoniot Date: Thu, 29 Dec 2022 13:35:02 -0500 Subject: [PATCH 24/34] fixed tests and incorrect rate limit check --- zssp/src/tests.rs | 2 +- zssp/src/zssp.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/zssp/src/tests.rs b/zssp/src/tests.rs index 0b0f65fce..96e4e62bb 100644 --- a/zssp/src/tests.rs +++ b/zssp/src/tests.rs @@ -209,7 +209,7 @@ mod tests { ) .is_ok()); } - if (test_loop % 8) == 0 && test_loop >= 8 && host.this_name.eq("alice") { + if (test_loop % 8) == 0 && test_loop >= 8 { session.service(host, send_to_other, &[], mtu_buffer.len(), test_loop as i64, true); } } diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index bf1fcf3ce..8025fb3f8 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -773,7 +773,7 @@ impl ReceiveContext { // Check rate limits. if let Some(session) = session.as_ref() { - if (current_time - session.state.read().unwrap().last_remote_offer) < Application::REKEY_RATE_LIMIT_MS { + if current_time < session.state.read().unwrap().last_remote_offer + Application::REKEY_RATE_LIMIT_MS { return Err(Error::RateLimited); } } else { @@ -867,7 +867,7 @@ impl ReceiveContext { if let Some(k) = state.session_keys[key_id as usize].as_ref() { if k.role == Role::Bob { // The local party is not allowed to be Bob twice in a row - // this prevents rekeying failure from both parties attempting to rekey at the same time + // This prevents rekeying failure from both parties attempting to rekey at the same time return Ok(ReceiveResult::Ignored); } if public_fingerprint_of_secret(k.ratchet_key.as_bytes())[..16].eq(alice_ratchet_key_fingerprint) { From bc90b2da8d53d1cb611726d3cbb3cede895ef4a7 Mon Sep 17 00:00:00 2001 From: mamoniot Date: Thu, 29 Dec 2022 13:45:08 -0500 Subject: [PATCH 25/34] fixed comment typo --- zssp/src/zssp.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index 8025fb3f8..2a5afabe8 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -36,7 +36,7 @@ pub enum Error { /// 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 difficult to get correct. + /// 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. @@ -83,7 +83,7 @@ pub enum ReceiveResult<'a, H: ApplicationLayer> { OkNewSession(Session), /// Packet superficially appears valid but was ignored e.g. as a duplicate. - /// IMPORTANT: This pack was not authenticated, so for the most part treat this the same as an Error::FailedAuthentication + /// IMPORTANT: This packet was not authenticated, so for the most part treat this the same as an Error::FailedAuthentication Ignored, } From fbd5e025d37fdfb8af9d4a2d1ddbe23d84e69a16 Mon Sep 17 00:00:00 2001 From: mamoniot Date: Thu, 29 Dec 2022 13:48:43 -0500 Subject: [PATCH 26/34] fixed comments --- zssp/src/zssp.rs | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index 2a5afabe8..0800b6010 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -25,39 +25,40 @@ use crate::counter::{Counter, CounterValue, CounterWindow}; use crate::sessionid::SessionId; pub enum Error { - /// The packet was addressed to an unrecognized local session (should usually be ignored) + /// The packet was addressed to an unrecognized local session (should usually be ignored). UnknownLocalSessionId(SessionId), - /// Packet was not well formed + /// Packet was not well formed. InvalidPacket, - /// An invalid parameter was supplied to the function + /// 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. + /// 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 + /// Rekeying failed and session secret has reached its hard usage count limit. MaxKeyLifetimeExceeded, - /// Attempt to send using session without established key + /// Attempt to send using session without established key. SessionNotEstablished, /// Packet ignored by rate limiter. RateLimited, - /// The other peer specified an unrecognized protocol version + /// The other peer specified an unrecognized protocol version. UnknownProtocolVersion, - /// Caller supplied data buffer is too small to receive data + /// Caller supplied data buffer is too small to receive data. DataBufferTooSmall, - /// Data object is too large to send, even with fragmentation + /// Data object is too large to send, even with fragmentation. DataTooLarge, /// An unexpected buffer overrun occured while attempting to encode or decode a packet. @@ -83,7 +84,8 @@ pub enum ReceiveResult<'a, H: ApplicationLayer> { OkNewSession(Session), /// Packet superficially appears valid but was ignored e.g. as a duplicate. - /// IMPORTANT: This packet was not authenticated, so for the most part treat this the same as an Error::FailedAuthentication + /// + /// **IMPORTANT**: This packet was not authenticated, so for the most part treat this the same as an Error::FailedAuthentication. Ignored, } From cbae1d8f4c8e4d105293649e54c737c4dc546432 Mon Sep 17 00:00:00 2001 From: monica Date: Tue, 3 Jan 2023 10:48:25 -0500 Subject: [PATCH 27/34] restructured check code --- zssp/src/zssp.rs | 49 ++++++++++++++++++++++++------------------------ 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index 0800b6010..216c862a4 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -84,7 +84,7 @@ pub enum ReceiveResult<'a, H: ApplicationLayer> { OkNewSession(Session), /// Packet superficially appears valid but was ignored e.g. as a duplicate. - /// + /// /// **IMPORTANT**: This packet was not authenticated, so for the most part treat this the same as an Error::FailedAuthentication. Ignored, } @@ -117,7 +117,7 @@ pub struct Session { /// An arbitrary application defined object associated with each session pub application_data: Application::Data, - ratchet_counts: [AtomicU64; 2], // Number of preceding session keys in ratchet + ratchet_counts: [AtomicU64; 2], // Number of session keys in ratchet, starts at 1 header_check_cipher: Aes, // Cipher used for header check codes (not Noise related) receive_windows: [CounterWindow; 2], // Receive window for anti-replay and deduplication state: RwLock, // Mutable parts of state (other than defrag buffers) @@ -131,7 +131,7 @@ 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 + 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 @@ -164,7 +164,6 @@ struct EphemeralOffer { id: [u8; 16], // Arbitrary random offer ID key_id: bool, // The key_id bound to this offer, for handling OOO rekeying creation_time: i64, // Local time when offer was created - ratchet_count: u64, // Ratchet count (starting at zero) for initial offer ratchet_key: Option>, // Ratchet key from previous offer or None if first offer ss_key: Secret<64>, // Noise session key "under construction" at state after offer sent alice_e_keypair: P384KeyPair, // NIST P-384 key pair (Noise ephemeral key for Alice) @@ -425,6 +424,9 @@ impl Session { .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 + //the protocol has been designed such that initial rekeying packets are identical to resent rekeying packets, except for the counter, so we can reuse the same code for doing both + let has_existing_session = state.remote_session_id.is_some(); //mark the previous key as no longer being supported because it is about to be overwritten //it should not be possible for a session to accidentally invalidate the key currently in use solely because of the read lock self.receive_windows[(!current_key_id) as usize].invalidate(); @@ -434,7 +436,7 @@ impl Session { &mut send, state.send_counters[current_key_id as usize].next(), current_key_id, - !current_key_id, + has_existing_session && !current_key_id, self.id, state.remote_session_id, app.get_local_s_public_blob(), @@ -444,11 +446,7 @@ 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 state.remote_session_id.is_some() { - Some(&self.header_check_cipher) - } else { - None - }, + if has_existing_session { Some(&self.header_check_cipher) } else { None }, mtu, current_time, &mut offer, @@ -883,7 +881,8 @@ impl ReceiveContext { (None, state.send_counters[key_id as usize].next(), !key_id, ratchet_key) } else { if key_id != false { - //all new sessions must start with key_id 0, this has no security implications + // All new sessions must start with key_id 0 + // This has no security implications, it just makes programming the initial offer easier return Ok(ReceiveResult::Ignored); } if let Some((new_session_id, psk, associated_object)) = @@ -1169,11 +1168,8 @@ impl ReceiveContext { let mut session_key = noise_ik_complete; // Mix ratchet key from previous session key (if any) and Kyber1024 hybrid shared key (if any). - let last_ratchet_count = if bob_ratchet_key_id.is_some() && offer.ratchet_key.is_some() { + if bob_ratchet_key_id.is_some() && offer.ratchet_key.is_some() { session_key = Secret(hmac_sha512(offer.ratchet_key.as_ref().unwrap().as_bytes(), session_key.as_bytes())); - offer.ratchet_count - } else { - 0 }; if let Some(hybrid_kk) = hybrid_kk.as_ref() { session_key = Secret(hmac_sha512(hybrid_kk.as_bytes(), session_key.as_bytes())); @@ -1201,12 +1197,13 @@ impl ReceiveContext { ); let new_key_id = offer.key_id; + let has_existing_session = state.remote_session_id.is_some(); drop(state); //TODO: check for correct orderings let mut state = session.state.write().unwrap(); let _ = state.remote_session_id.replace(bob_session_id); let _ = state.session_keys[new_key_id as usize].replace(session_key); - if last_ratchet_count > 0 && state.cur_session_key_id != new_key_id { + if has_existing_session && state.cur_session_key_id != new_key_id { //when an brand new key offer is sent, it is sent using the new_key_id==false counter, we cannot reset it in that case. //NOTE: the following code should be properly threadsafe, see the large comment above at the end of KEY_OFFER decoding for more info session.receive_windows[new_key_id as usize].reset_for_new_key_offer(); @@ -1221,6 +1218,9 @@ impl ReceiveContext { return Ok(ReceiveResult::Ignored); } } + } else { + unlikely_branch(); + return Err(Error::SessionNotEstablished); } // Just ignore counter-offers that are out of place. They probably indicate that this side @@ -1384,7 +1384,6 @@ fn send_ephemeral_offer( id, key_id: new_key_id, creation_time: current_time, - ratchet_count, ratchet_key, ss_key, alice_e_keypair, @@ -1467,11 +1466,13 @@ fn send_with_fragmentation( /// Set 32-bit header check code, used to make fragmentation mechanism robust. fn set_header_check_code(packet: &mut [u8], ratchet_count: u64, header_check_cipher: &Aes) { debug_assert!(packet.len() >= MIN_PACKET_SIZE); - + //4 bytes is the ratchet key + //12 bytes is the header we want to verify let mut header_mac = 0u128.to_le_bytes(); - memory::store_raw((ratchet_count as u16).to_le_bytes(), &mut header_mac[0..2]); - header_mac[2..16].copy_from_slice(&packet[4..18]); + memory::store_raw((ratchet_count as u32).to_le_bytes(), &mut header_mac[0..4]); + header_mac[4..16].copy_from_slice(&packet[4..16]); header_check_cipher.encrypt_block_in_place(&mut header_mac); + packet[..4].copy_from_slice(&header_mac[..4]); } @@ -1479,13 +1480,13 @@ fn set_header_check_code(packet: &mut [u8], ratchet_count: u64, header_check_cip /// This is not nearly enough entropy to be cryptographically secure, it only is meant for making DOS attacks very hard fn verify_header_check_code(packet: &[u8], ratchet_count: u64, header_check_cipher: &Aes) -> bool { debug_assert!(packet.len() >= MIN_PACKET_SIZE); - //2 bytes is the ratchet key + //4 bytes is the ratchet key //12 bytes is the header we want to verify - //2 bytes is salt from the message let mut header_mac = 0u128.to_le_bytes(); - memory::store_raw((ratchet_count as u16).to_le_bytes(), &mut header_mac[0..2]); - header_mac[2..16].copy_from_slice(&packet[4..18]); + memory::store_raw((ratchet_count as u32).to_le_bytes(), &mut header_mac[0..4]); + header_mac[4..16].copy_from_slice(&packet[4..16]); header_check_cipher.encrypt_block_in_place(&mut header_mac); + memory::load_raw::(&packet[..4]) == memory::load_raw::(&header_mac[..4]) } From b47ef35321537c5aad2d682bb3cebc10e53b4406 Mon Sep 17 00:00:00 2001 From: monica Date: Tue, 3 Jan 2023 11:14:11 -0500 Subject: [PATCH 28/34] removed redundant state --- zssp/src/zssp.rs | 57 +++++++++++++++++++----------------------------- 1 file changed, 22 insertions(+), 35 deletions(-) diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index 216c862a4..916e57c34 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -164,7 +164,6 @@ struct EphemeralOffer { id: [u8; 16], // Arbitrary random offer ID key_id: bool, // The key_id bound to this offer, for handling OOO rekeying creation_time: i64, // Local time when offer was created - ratchet_key: Option>, // Ratchet key from previous offer or None if first offer ss_key: Secret<64>, // Noise session key "under construction" at state after offer sent alice_e_keypair: P384KeyPair, // NIST P-384 key pair (Noise ephemeral key for Alice) alice_hk_keypair: Option, // Kyber1024 key pair (PQ hybrid ephemeral key for Alice) @@ -1055,28 +1054,23 @@ impl ReceiveContext { hybrid_kk.is_some(), ); - let ratchet_count; let mut state = session.state.write().unwrap(); let _ = state.session_keys[new_key_id as usize].replace(session_key); - if existing_session.is_some() { - let _ = state.remote_session_id.replace(alice_session_id); - 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 - 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 - // 2) The received counter is greater than what is currently stored in the window, so a valid packet would be accepted *but* its counter is deleted from the window so it can be replayed - // To prevent these race conditions, we only update the ratchet_count for salting the check code after the window has reset. So if a counter passes the initial check code: it either means the thread sees ratchet count has been update, therefore it either sees receive_window has been reset (due to memory orderings), or it means a rare accidental check forge has occurred. - session.receive_windows[new_key_id as usize].reset_for_new_key_offer(); - session.ratchet_counts[new_key_id as usize].fetch_add(2, Ordering::SeqCst); + 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 + 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 + // 2) The received counter is greater than what is currently stored in the window, so a valid packet would be accepted *but* its counter is deleted from the window so it can be replayed + // To prevent these race conditions, we only update the ratchet_count for salting the check code after the window has reset. So if a counter passes the initial check code: it either means the thread sees ratchet count has been update, therefore it either sees receive_window has been reset (due to memory orderings), or it means a rare accidental check forge has occurred. + session.receive_windows[new_key_id as usize].reset_for_new_key_offer(); + session.ratchet_counts[new_key_id as usize].fetch_add(2, Ordering::SeqCst); - // if the following wasn't done inside a lock, a theoretical race condition exists where a thread uses the new key id before the counter is reset, or worse: a thread has held onto the previous key_id == new_key_id, and attempts to use the reset counter - // for this reason do not access send_counters without holding the read lock - state.cur_session_key_id = new_key_id; - state.send_counters[new_key_id as usize].reset_for_new_key_offer(); - } - } else { - ratchet_count = 1; + // if the following wasn't done inside a lock, a theoretical race condition exists where a thread uses the new key id before the counter is reset, or worse: a thread has held onto the previous key_id == new_key_id, and attempts to use the reset counter + // for this reason do not access send_counters without holding the read lock + state.cur_session_key_id = new_key_id; + state.send_counters[new_key_id as usize].reset_for_new_key_offer(); } drop(state); @@ -1168,9 +1162,11 @@ impl ReceiveContext { let mut session_key = noise_ik_complete; // Mix ratchet key from previous session key (if any) and Kyber1024 hybrid shared key (if any). - if bob_ratchet_key_id.is_some() && offer.ratchet_key.is_some() { - session_key = Secret(hmac_sha512(offer.ratchet_key.as_ref().unwrap().as_bytes(), session_key.as_bytes())); - }; + if let Some(cur_session_key) = state.session_keys[key_id as usize].as_ref() { + if bob_ratchet_key_id.is_some() { + session_key = Secret(hmac_sha512(cur_session_key.ratchet_key.as_bytes(), session_key.as_bytes())); + } + } if let Some(hybrid_kk) = hybrid_kk.as_ref() { session_key = Secret(hmac_sha512(hybrid_kk.as_bytes(), session_key.as_bytes())); } @@ -1197,13 +1193,12 @@ impl ReceiveContext { ); let new_key_id = offer.key_id; - let has_existing_session = state.remote_session_id.is_some(); drop(state); //TODO: check for correct orderings let mut state = session.state.write().unwrap(); let _ = state.remote_session_id.replace(bob_session_id); let _ = state.session_keys[new_key_id as usize].replace(session_key); - if has_existing_session && state.cur_session_key_id != new_key_id { + if state.cur_session_key_id != new_key_id { //when an brand new key offer is sent, it is sent using the new_key_id==false counter, we cannot reset it in that case. //NOTE: the following code should be properly threadsafe, see the large comment above at the end of KEY_OFFER decoding for more info session.receive_windows[new_key_id as usize].reset_for_new_key_offer(); @@ -1268,13 +1263,6 @@ fn send_ephemeral_offer( None }; - // Get ratchet key for current key if one exists. - let ratchet_key = if let Some(current_key) = current_key { - Some(current_key.ratchet_key.clone()) - } else { - None - }; - // Random ephemeral offer ID let id: [u8; 16] = random::get_bytes_secure(); @@ -1305,9 +1293,9 @@ fn send_ephemeral_offer( } else { idx = safe_write_all(&mut packet_buf, idx, &[HYBRID_KEY_TYPE_NONE])?; } - if let Some(ratchet_key) = ratchet_key.as_ref() { + 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(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])?; } @@ -1384,7 +1372,6 @@ fn send_ephemeral_offer( id, key_id: new_key_id, creation_time: current_time, - ratchet_key, ss_key, alice_e_keypair, alice_hk_keypair, From 40ef3702046fbfedc2c35e215e09208af9a74bb7 Mon Sep 17 00:00:00 2001 From: monica Date: Tue, 3 Jan 2023 11:19:32 -0500 Subject: [PATCH 29/34] added missing ratcheting enforcement --- zssp/src/zssp.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index 916e57c34..df888b892 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -1162,10 +1162,16 @@ impl ReceiveContext { let mut session_key = noise_ik_complete; // Mix ratchet key from previous session key (if any) and Kyber1024 hybrid shared key (if any). + // We either have a session, in which case they should have supplied a ratchet key fingerprint, or + // we don't and they should not have supplied one. if let Some(cur_session_key) = state.session_keys[key_id as usize].as_ref() { if bob_ratchet_key_id.is_some() { session_key = Secret(hmac_sha512(cur_session_key.ratchet_key.as_bytes(), session_key.as_bytes())); + } else { + return Err(Error::FailedAuthentication); } + } else if bob_ratchet_key_id.is_some() { + return Err(Error::FailedAuthentication); } if let Some(hybrid_kk) = hybrid_kk.as_ref() { session_key = Secret(hmac_sha512(hybrid_kk.as_bytes(), session_key.as_bytes())); From 046ddbaf33458a95072e34233e211acd6e535ae8 Mon Sep 17 00:00:00 2001 From: monica Date: Tue, 3 Jan 2023 11:20:37 -0500 Subject: [PATCH 30/34] renamed for consistency --- zssp/src/zssp.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index df888b892..94dea4e53 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -1493,15 +1493,15 @@ fn parse_dec_key_offer_after_header( let mut session_id_buf = 0_u64.to_ne_bytes(); session_id_buf[..SESSION_ID_SIZE].copy_from_slice(safe_read_exact(&mut p, SESSION_ID_SIZE)?); - let alice_session_id = SessionId::new_from_u64(u64::from_le_bytes(session_id_buf)).ok_or(Error::InvalidPacket)?; + let remote_session_id = SessionId::new_from_u64(u64::from_le_bytes(session_id_buf)).ok_or(Error::InvalidPacket)?; - let alice_s_public_blob_len = varint_safe_read(&mut p)?; - let alice_s_public_blob = safe_read_exact(&mut p, alice_s_public_blob_len as usize)?; + let remote_s_public_blob_len = varint_safe_read(&mut p)?; + let remote_s_public_blob = safe_read_exact(&mut p, remote_s_public_blob_len as usize)?; - let alice_metadata_len = varint_safe_read(&mut p)?; - let alice_metadata = safe_read_exact(&mut p, alice_metadata_len as usize)?; + let remote_metadata_len = varint_safe_read(&mut p)?; + let remote_metadata = safe_read_exact(&mut p, remote_metadata_len as usize)?; - let alice_hk_public_raw = match safe_read_exact(&mut p, 1)?[0] { + let remote_hk_public_raw = match safe_read_exact(&mut p, 1)?[0] { HYBRID_KEY_TYPE_KYBER1024 => { if packet_type == PACKET_TYPE_INITIAL_KEY_OFFER { safe_read_exact(&mut p, pqc_kyber::KYBER_PUBLICKEYBYTES)? @@ -1515,7 +1515,7 @@ fn parse_dec_key_offer_after_header( if p.is_empty() { return Err(Error::InvalidPacket); } - let alice_ratchet_key_fingerprint = if safe_read_exact(&mut p, 1)?[0] == 0x01 { + let remote_ratchet_key_fingerprint = if safe_read_exact(&mut p, 1)?[0] == 0x01 { Some(safe_read_exact(&mut p, 16)?) } else { None @@ -1523,11 +1523,11 @@ fn parse_dec_key_offer_after_header( Ok(( offer_id, //always 16 bytes - alice_session_id, - alice_s_public_blob, - alice_metadata, - alice_hk_public_raw, - alice_ratchet_key_fingerprint, //always 16 bytes + remote_session_id, + remote_s_public_blob, + remote_metadata, + remote_hk_public_raw, + remote_ratchet_key_fingerprint, //always 16 bytes )) } From 2233f8c535dc6c969d46d15b7d0c569bf50ebad6 Mon Sep 17 00:00:00 2001 From: monica Date: Tue, 3 Jan 2023 16:55:21 -0500 Subject: [PATCH 31/34] added more documentation --- zssp/src/applicationlayer.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/zssp/src/applicationlayer.rs b/zssp/src/applicationlayer.rs index f72450588..8f0cdc629 100644 --- a/zssp/src/applicationlayer.rs +++ b/zssp/src/applicationlayer.rs @@ -67,6 +67,16 @@ pub trait ApplicationLayer: Sized { /// On success a tuple of local session ID, static secret, and associated object is returned. The /// static secret is whatever results from agreement between the local and remote static public /// keys. + /// + /// When `accept_new_session` is called, `remote_static_public` and `remote_metadata` have not yet been + /// authenticated. As such avoid mutating state until OkNewSession(Session) is returned, as the connection + /// may be adversarial. + /// + /// When `remote_static_public` and `remote_metadata` are eventually authenticated, the zssp protocol cannot + /// guarantee that they are unique, i.e. `remote_static_public` and `remote_metadata` may be duplicates from + /// an old attempt to establish a session, and may even have been replayed by an adversary. If your use-case + /// needs uniqueness for reliability or security, consider either including a timestamp in the metadata, or + /// sending the metadata as an extra transport packet after the session is fully established. fn accept_new_session( &self, receive_context: &ReceiveContext, From a2b3c780bba6392280aa4036545ccd97e170fa94 Mon Sep 17 00:00:00 2001 From: monica Date: Tue, 3 Jan 2023 16:58:21 -0500 Subject: [PATCH 32/34] updated comment --- zssp/src/applicationlayer.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/zssp/src/applicationlayer.rs b/zssp/src/applicationlayer.rs index 8f0cdc629..68358aa95 100644 --- a/zssp/src/applicationlayer.rs +++ b/zssp/src/applicationlayer.rs @@ -77,6 +77,7 @@ pub trait ApplicationLayer: Sized { /// an old attempt to establish a session, and may even have been replayed by an adversary. If your use-case /// needs uniqueness for reliability or security, consider either including a timestamp in the metadata, or /// sending the metadata as an extra transport packet after the session is fully established. + /// It is guaranteed they will be unique for at least the lifetime of the returned session. fn accept_new_session( &self, receive_context: &ReceiveContext, From 735f40421bc4112ec99189d6b3b1581f5645c000 Mon Sep 17 00:00:00 2001 From: monica Date: Tue, 3 Jan 2023 17:24:05 -0500 Subject: [PATCH 33/34] fixed multiple comments --- zssp/changes.txt | 19 ----------------- zssp/src/zssp.rs | 53 +++++++++++++++++++++++++----------------------- 2 files changed, 28 insertions(+), 44 deletions(-) delete mode 100644 zssp/changes.txt diff --git a/zssp/changes.txt b/zssp/changes.txt deleted file mode 100644 index ff8a5efdc..000000000 --- a/zssp/changes.txt +++ /dev/null @@ -1,19 +0,0 @@ -zssp has been moved into it's own crate. - -zssp has been cut up into several files, only the new zssp.rs file contains the critical security path. - -Standardized the naming conventions for security variables throughout zssp. - -Implemented a safer version of write_all for zssp to use. This has 3 benefits: it completely prevents unknown io errors, making error handling easier and self-documenting; it completely prevents src from being truncated in dest, putting in an extra barrier to prevent catastrophic key truncation; and it has slightly less performance overhead than a write_all. - -Implemented a safer version of read_exact for zssp to use. This has similar benefits to the previous change. - -Refactored most buffer logic to use safe_read_exact and safe_write_all, the resulting code is less verbose and easier to analyze: Because of this refactor the buffer overrun below was caught. - -Fixed a buffer overrun panic when decoding alice_ratchet_key_fingerprint - -Renamed variables and added extra intermediate values so encoding and decoding are more obviously symmetric. - -Added multiple comments. - -Removed Box, EphemeralOffer is now passed out by reference instead of returned up the stack. diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index 94dea4e53..c41ae4807 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -36,8 +36,11 @@ pub enum Error { /// 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. + /// **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. @@ -80,12 +83,13 @@ pub enum ReceiveResult<'a, H: ApplicationLayer> { /// Packet is valid and a new session was created. /// - /// The session will have already been gated by the accept_new_session() method in the Host trait. + /// The session will have already been gated by the `accept_new_session()` method in the Host trait. OkNewSession(Session), /// Packet superficially appears valid but was ignored e.g. as a duplicate. /// - /// **IMPORTANT**: This packet was not authenticated, so for the most part treat this the same as an Error::FailedAuthentication. + /// **IMPORTANT**: This packet was not authenticated, so for the most part treat this the same + /// as an `Error::FailedAuthentication`. Ignored, } @@ -93,6 +97,8 @@ pub enum ReceiveResult<'a, H: ApplicationLayer> { /// /// Note that the role can switch through the course of a session. It's the side that most recently /// 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)] pub enum Role { Alice, @@ -139,7 +145,6 @@ struct SessionMutableState { } /// A shared symmetric session key. -/// sessions always start at counter 1u32 struct SessionKey { secret_fingerprint: [u8; 16], // First 128 bits of a SHA384 computed from the secret creation_time: i64, // Time session key was established @@ -423,14 +428,14 @@ impl Session { .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 - //the protocol has been designed such that initial rekeying packets are identical to resent rekeying packets, except for the counter, so we can reuse the same code for doing both + // This routine handles sending a rekeying packet, resending lost rekeying packets, and resending lost initial offer packets + // The protocol has been designed such that initial rekeying packets are identical to resent rekeying packets, except for the counter, so we can reuse the same code for doing both let has_existing_session = state.remote_session_id.is_some(); - //mark the previous key as no longer being supported because it is about to be overwritten - //it should not be possible for a session to accidentally invalidate the key currently in use solely because of the read lock + // Mark the previous key as no longer being supported because it is about to be overwritten + // It should not be possible for a session to accidentally invalidate the key currently in use solely because of the read lock self.receive_windows[(!current_key_id) as usize].invalidate(); let mut offer = None; - //the session will keep sending ephemeral offers until rekeying is successful + // The session will keep sending ephemeral offers until rekeying is successful if send_ephemeral_offer( &mut send, state.send_counters[current_key_id as usize].next(), @@ -506,8 +511,8 @@ impl ReceiveContext { if let Some(local_session_id) = SessionId::new_from_u64(u64::from_le(memory::load_raw(&incoming_packet[8..16])) & 0xffffffffffffu64) { if let Some(session) = app.lookup_session(local_session_id) { - //this is the only time ratchet_counts is ever accessed outside of a lock - //as such this read can be wrong, but that is incredibly unlikely since we are tracking the last two ratchet counts, and if it's wrong it just means we drop a packet that would have been dropped anyways for being too old or too new + // This is the only time ratchet_counts is ever accessed outside of a lock + // As such this read can be wrong, but that is incredibly unlikely since we are tracking the last two ratchet counts, and if it's wrong it just means we drop a packet that would have been dropped anyways for being too old or too new let ratchet_count = session.ratchet_counts[key_id as usize].load(Ordering::SeqCst); if verify_header_check_code(incoming_packet, ratchet_count, &session.header_check_cipher) { if session.receive_windows[key_id as usize].message_received(counter) { @@ -567,10 +572,10 @@ impl ReceiveContext { return Err(Error::UnknownLocalSessionId(local_session_id)); } } 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 + 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 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 { @@ -1061,14 +1066,14 @@ impl ReceiveContext { 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 - // 2) The received counter is greater than what is currently stored in the window, so a valid packet would be accepted *but* its counter is deleted from the window so it can be replayed - // To prevent these race conditions, we only update the ratchet_count for salting the check code after the window has reset. So if a counter passes the initial check code: it either means the thread sees ratchet count has been update, therefore it either sees receive_window has been reset (due to memory orderings), or it means a rare accidental check forge has occurred. + // 1) The received counter is less than what is currently stored in the window, so a valid packet would be rejected. + // 2) The received counter is greater than what is currently stored in the window, so a valid packet would be accepted *but* its counter is deleted from the window so it can be replayed. + // To prevent these race conditions, we only update the ratchet_count for salting the check code after the window has reset. So if a counter passes the initial check code: it either means the thread sees ratchet count has been update therefore it sees receive_window has been reset (due to memory orderings), or it means a rare accidental check code collision has occurred. session.receive_windows[new_key_id as usize].reset_for_new_key_offer(); session.ratchet_counts[new_key_id as usize].fetch_add(2, Ordering::SeqCst); - // if the following wasn't done inside a lock, a theoretical race condition exists where a thread uses the new key id before the counter is reset, or worse: a thread has held onto the previous key_id == new_key_id, and attempts to use the reset counter - // for this reason do not access send_counters without holding the read lock + // If the following wasn't done inside a lock, a theoretical race condition exists where a thread uses the new key id before the counter is reset, or worse: a thread has held onto the previous key id equal to new_key_id, and attempts to use the reset counter for encryption. + // For this reason do not access send_counters without holding the read lock. state.cur_session_key_id = new_key_id; state.send_counters[new_key_id as usize].reset_for_new_key_offer(); } @@ -1200,13 +1205,12 @@ impl ReceiveContext { let new_key_id = offer.key_id; drop(state); - //TODO: check for correct orderings let mut state = session.state.write().unwrap(); let _ = state.remote_session_id.replace(bob_session_id); let _ = state.session_keys[new_key_id as usize].replace(session_key); if state.cur_session_key_id != new_key_id { - //when an brand new key offer is sent, it is sent using the new_key_id==false counter, we cannot reset it in that case. - //NOTE: the following code should be properly threadsafe, see the large comment above at the end of KEY_OFFER decoding for more info + // When an brand new key offer is sent, it is sent using the new_key_id==false counter, we cannot reset it in that case. + // NOTE: the following code should be properly threadsafe, see the large comment above at the end of KEY_OFFER decoding for more info session.receive_windows[new_key_id as usize].reset_for_new_key_offer(); let _ = session.ratchet_counts[new_key_id as usize].fetch_add(2, Ordering::SeqCst); state.cur_session_key_id = new_key_id; @@ -1283,7 +1287,6 @@ fn send_ephemeral_offer( let mut idx = HEADER_SIZE; idx = safe_write_all(&mut packet_buf, idx, &[SESSION_PROTOCOL_VERSION])?; - //TODO: check this, the below line is supposed to be the blob, not just the key, right? idx = safe_write_all(&mut packet_buf, idx, alice_e_keypair.public_key_bytes())?; let plaintext_end = idx; From b85e6c3d4935f0228004cb0d313738403d44e613 Mon Sep 17 00:00:00 2001 From: monica Date: Tue, 3 Jan 2023 17:32:15 -0500 Subject: [PATCH 34/34] fixed comment --- zssp/src/zssp.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zssp/src/zssp.rs b/zssp/src/zssp.rs index c41ae4807..a788733a9 100644 --- a/zssp/src/zssp.rs +++ b/zssp/src/zssp.rs @@ -1051,7 +1051,7 @@ impl ReceiveContext { idx = safe_write_all(&mut reply_buf, idx, &hmac)?; 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 + // Initial key offers should only check this if this is a rekey let session_key = SessionKey::new( session_key, Role::Bob,