Merge pull request #1832 from zerotier/replay-attack-fixes

redesign of zssp

zssp/changes.txt

@@ -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>, EphemeralOffer is now passed out by reference instead of returned up the stack.

zssp/src/applicationlayer.rs

@@ -67,6 +67,17 @@ 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.
+    /// It is guaranteed they will be unique for at least the lifetime of the returned session.
     fn accept_new_session(
         &self,
         receive_context: &ReceiveContext<Self>,

zssp/src/constants.rs

@@ -74,11 +74,8 @@ 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_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;

zssp/src/counter.rs

@@ -1,28 +1,29 @@
-use std::sync::atomic::{AtomicU32, AtomicU64, Ordering};
+use std::sync::atomic::{Ordering, AtomicU32};
 
-use zerotier_crypto::random;
-
-use crate::constants::COUNTER_MAX_DELTA;
+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(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() as u64))
+        Self(AtomicU32::new(1u32))
+    }
+
+    #[inline(always)]
+    pub fn reset_for_new_key_offer(&self) {
+        self.0.store(1u32, Ordering::SeqCst);
     }
 
     /// Get the value most recently used to send a packet.
     #[inline(always)]
-    pub fn previous(&self) -> CounterValue {
+    pub fn current(&self) -> CounterValue {
         CounterValue(self.0.load(Ordering::SeqCst))
     }
 
@@ -36,7 +37,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.
@@ -44,45 +45,44 @@ 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())
-    }
 }
 
 /// Incoming packet deduplication and replay protection window.
-pub(crate) struct CounterWindow(AtomicU32, [AtomicU32; COUNTER_MAX_DELTA as usize]);
+pub(crate) struct CounterWindow([AtomicU32; COUNTER_MAX_ALLOWED_OOO]);
 
 impl CounterWindow {
     #[inline(always)]
-    pub fn new(initial: u32) -> Self {
-        Self(AtomicU32::new(initial), std::array::from_fn(|_| AtomicU32::new(initial)))
+    pub fn new() -> Self {
+        Self(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)))
+    }
+    pub fn reset_for_new_key_offer(&self) {
+        for i in 0..COUNTER_MAX_ALLOWED_OOO {
+            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 {
-        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_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)]
+    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.
+        //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;
     }
 }

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);
                         }
                     }
@@ -218,14 +218,53 @@ 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 mut rng = 844632;
+        let mut counter = 1u32;
+        let mut history = Vec::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;
+            if p < 0.5 {
+                let r = xorshift64(&mut rng);
+                c = counter + (r%(COUNTER_MAX_ALLOWED_OOO - 1) as u32 + 1);
+            } else if p < 0.8 {
+                counter = counter + (1);
+                c = counter;
+            } else if p < 0.9 {
+                if history.len() > 0 {
+                    let idx = xorshift64(&mut rng) as usize%history.len();
+                    let c = history[idx];
+                    assert!(!w.message_authenticated(c));
+                }
+                continue;
+            } else if p < 0.999 {
+                c = xorshift64(&mut rng);
+                w.message_received(c);
+                continue;
+            } else {
+                w.reset_for_new_key_offer();
+                counter = 1u32;
+                history = Vec::new();
+                continue;
+            }
+            if history.contains(&c) {
+                assert!(!w.message_authenticated(c));
+            } else {
+                assert!(w.message_authenticated(c));
+                history.push(c);
+            }
+        }
     }
 }