Repository: cloudflare/boringtun (language: rust)

Issue:
Incomplete session lifetime management (key rotation)
WireGuard protocol requirements for per-peer session lifetime based on the whitepaper:

 1. Sessions must no longer be used REJECT-AFTER-TIME (180 seconds). Data must not be sent when a session is expired and received data must be ignored.
 2. If an *initiator* sends data and session age >= REKEY-AFTER-TIME (120 seconds), it should attempt to start a new handshake.
 3. If a *responder* receives data and the session age is >= REJECT-AFTER-TIME - KEEPALIVE-TIMEOUT - REKEY-TIMEOUT (180 - 10 - 5 seconds), it should try to initiate a new handshake once for this event.
 4. After `REJECT-AFTER-TIME * 3`, all existing sessions must be destroyed.
 5. During key rotation, one must be able to decrypt data using the current and previous session (subject to the above REJECT-AFTER-TIME requirement), but previous sessions must be destroyed.

boringtun implements session retrieval and management in src/noise/timers.rs and src/noise/mod.rs.

Sessions are cleared only when:

 * `Tunn::set_static_private`: called when a new private key is set.
 * `Tunn::clear_all`: called when either:
    * The most recent established session is older than `REJECT-AFTER-TIME * 3` (established defined as receipt of an initiator/response message, not the 1.5-RTT data message for the responder!)
    * A new handshake was started by this device more than REKEY-ATTEMPT-TIME ago, but was not answered (handshake timeout).

Sessions are created on receipt of handshake messages in `network_to_tunnel`:
 * On receipt of a valid initiation message. Note that this session cannot be used yet since the `current` index has not yet been updated.
 * On receipt of a valid response handshake message that matches an existing in-progress handshake. Note that this session cannot be used yet since the `current` index has not yet been updated.

Sessions are used in:
 * `time_since_last_handshake` - uses the current session.
 * `tunnel_to_network` - uses the current session. Called when a locally-originated data packet is about to be sent.
 * `receive_data_packet` - uses the session based on the receiver index which is *chosen by the remote peer*.

The "current" session is updated in:
 * `network_to_tunnel` - when an initiator receives a handshake response.
 * `receive_data_packet` - when a data packet for an existing session is received and is successfully decrypted with a valid counter that is not reused.

Problems:
 1. The "current session" can be changed by the remote peer (or attacker) who sends a valid data packet. The "current session" should only be replaced when the a session is newer. Otherwise, consider this attack against the boringtun as initiator (a similar issue applies in the responder role):
    1. A honest boringtun initiator establishes a session with a responder and sends data. An attacker intercepts and withholds some data packets from the responder.
    2. The initiator (or responder) starts a new session due to key rotation and switches over to the new session.
    3. The attacker sends one of the data packets to the initiator, possibly **after REJECT-AFTER-TIME** has passed for the first session.
    4. The initiator switches the session secrets back to the old session, data sent by the initiator will not be protected with newer session secrets.
    5. This basically defeats key rotation and violates Requirement 1.
 2. Requirement 2 seems not fully implemented. If an initiator has sent data at some point, and after REKEY_AFTER_TIME, it should not automatically send an initiation message. Instead it should only trigger a new handshake right after sending a data message (including Keep-Alive). Right now it unconditionally sends an initiation message after REKEY-AFTER-TIME while it should have remained silent.
 3. Requirement 3 seems not fully implemented. `REJECT_AFTER_TIME - KEEPALIVE_TIMEOUT - REKEY_TIMEOUT` must be checked by the responder, but boringtun *only* checks it for the initiator.
 4. Requirement 4 seems satisfied but with one potential issue. As sessions are considered "established" right after receiving the Initiation message, the following attack is possible against a boringtun responder:
    1. Initiator starts handshake by sending an Initiation message. An attacker prevents these messages from arriving, so the initiator repeatedly sends new initiation messages.
    2. The attacker forwards the very *first* initiation message and thus establishes a session.
    3. Assume that the initiator no longer has data to send, so after `REJECT-AFTER-TIME * 3`, all sessions must be cleared.
    4. Just before it expires though, the attacker forwards the second initiation message. As this is not a replay, the boringtun responder assumes that a new session is established.
    5. This process can be repeated by an attacker using the remaining initiation messages.
    6. The result is that the session lifetimes can be prolonged.
5. Requirement 5 is not satisfied because previous sessions are not cleared.
6. (minor observation) `Tunn:tick_handshake_started` appears to be dead code, `Tunn::format_handshake_initiation` calls `self.timer_tick(TimerName::TimeLastHandshakeStarted)` directly.

Summary: Key rotation is not properly implemented, keys should have been cleared but are actual maintained for potentially a very long time. This might defeat forward secrecy when the boringtun client/server is raided since all keys are still in memory.

Note: this is based on a manual code review, I did not actually try to test the actual implementation.

Analysis applies to current master (commit e765e12613e8eb33e39e7fb0676ad5797c02c5f6).


Discussion / hints:

Fix patch:
diff --git a/src/noise/mod.rs b/src/noise/mod.rs
index 0fef3daa..f568916c 100644
--- a/src/noise/mod.rs
+++ b/src/noise/mod.rs
@@ -38,7 +38,7 @@ const IPV6_IP_SZ: usize = 16;
 const IP_LEN_SZ: usize = 2;
 
 const MAX_QUEUE_DEPTH: usize = 256;
-const N_SESSIONS: usize = 4; // number of sessions in the ring, better keep a PoT
+const N_SESSIONS: usize = 8; // number of sessions in the ring, better keep a PoT
 
 #[derive(Debug)]
 pub enum TunnResult<'a> {
@@ -329,12 +329,12 @@ impl Tunn {
             handshake.receive_handshake_initialization(p, dst)?
         };
         // Store new session in ring buffer
-        let index = session.local_index();
-        *self.sessions[index % N_SESSIONS].write() = Some(session);
+        let index = session.local_index() % N_SESSIONS;
+        *self.sessions[index].write() = Some(session);
 
         self.timer_tick(TimerName::TimeLastPacketReceived);
         self.timer_tick(TimerName::TimeLastPacketSent);
-        self.timer_tick_session_established(false); // New session established, we are not the initiator
+        self.timer_tick_session_established(false, index); // New session established, we are not the initiator
 
         self.log(Verbosity::Debug, "Sending handshake_response");
         Ok(TunnResult::WriteToNetwork(packet))
@@ -354,13 +354,13 @@ impl Tunn {
 
         let keepalive_packet = session.format_packet_data(&[], dst);
         // Store new session in ring buffer
-        let index = session.local_index();
-        *self.sessions[index % N_SESSIONS].write() = Some(session);
+        let index = session.local_index() % N_SESSIONS;
+        *self.sessions[index].write() = Some(session);
         // The new session becomes the currently used session
         self.current.store(index, Ordering::SeqCst);
 
         self.timer_tick(TimerName::TimeLastPacketReceived);
-        self.timer_tick_session_established(true); // New session established, we are the initiator
+        self.timer_tick_session_established(true, index); // New session established, we are the initiator
 
         self.log(Verbosity::Debug, "Sending keepalive");
         Ok(TunnResult::WriteToNetwork(keepalive_packet)) // Send a keepalive as a response
@@ -382,24 +382,41 @@ impl Tunn {
         Ok(TunnResult::Done)
     }
 
+    // Update the index of the currently used session, if needed
+    fn set_current_session(&self, new_idx: usize) {
+        let cur_idx = self.current.load(Ordering::Relaxed);
+        if cur_idx == new_idx {
+            // There is nothing to do, already using this session, this is the common case
+            return;
+        }
+        if self.sessions[cur_idx].read().is_none()
+            || self.timers.session_timers[new_idx].time()
+                > self.timers.session_timers[cur_idx].time()
+        {
+            self.current.store(new_idx, Ordering::Relaxed);
+        }
+    }
+
     // Decrypts a data packet, and stores the decapsulated packet in dst.
     fn handle_data<'a>(
         &self,
         packet: PacketData,
         dst: &'a mut [u8],
     ) -> Result<TunnResult<'a>, WireGuardError> {
-        let idx = packet.receiver_idx as usize;
+        let idx = packet.receiver_idx as usize % N_SESSIONS;
         // Get the (probably) right session
-        let lock = self.sessions[idx % N_SESSIONS].read();
-        let session = (*lock).as_ref().ok_or(WireGuardError::NoCurrentSession)?;
-        let packet = session.receive_packet_data(packet, dst)?;
-        // Update current session, the exact session we use is not important as long as it is valid
-        self.current.store(idx, Ordering::Relaxed);
+        let decapsulated_packet = {
+            let lock = self.sessions[idx].read();
+            let session = (*lock).as_ref().ok_or(WireGuardError::NoCurrentSession)?;
+            session.receive_packet_data(packet, dst)?
+        };
+
+        self.set_current_session(idx);
 
         self.timer_tick(TimerName::TimeLastDataPacketReceived);
         self.timer_tick(TimerName::TimeLastPacketReceived);
 
-        Ok(self.validate_decapsulated_packet(packet))
+        Ok(self.validate_decapsulated_packet(decapsulated_packet))
     }
 
     // Formats a new handshake initiation message and store it in dst. If force_resend is true will send
diff --git a/src/noise/session.rs b/src/noise/session.rs
index 5ffa41ac..8c6322f1 100644
--- a/src/noise/session.rs
+++ b/src/noise/session.rs
@@ -9,21 +9,16 @@ use crate::noise::errors::WireGuardError;
 use ring::aead::*;
 use std::sync::atomic::{AtomicUsize, Ordering};
 
-#[cfg(not(target_arch = "arm"))]
 pub struct Session {
     receiving_index: u32,
     sending_index: u32,
+    #[cfg(not(target_arch = "arm"))]
     receiver: OpeningKey,
+    #[cfg(not(target_arch = "arm"))]
     sender: SealingKey,
-    sending_key_counter: AtomicUsize,
-    receiving_key_counter: spin::Mutex<ReceivingKeyCounterValidator>,
-}
-
-#[cfg(target_arch = "arm")]
-pub struct Session {
-    receiving_index: u32,
-    sending_index: u32,
+    #[cfg(target_arch = "arm")]
     receiver: ChaCha20Poly1305,
+    #[cfg(target_arch = "arm")]
     sender: ChaCha20Poly1305,
     sending_key_counter: AtomicUsize,
     receiving_key_counter: spin::Mutex<ReceivingKeyCounterValidator>,
@@ -163,21 +158,16 @@ impl Session {
         receiving_key: [u8; 32],
         sending_key: [u8; 32],
     ) -> Session {
-        #[cfg(not(target_arch = "arm"))]
         return Session {
             receiving_index: local_index,
             sending_index: peer_index,
+            #[cfg(not(target_arch = "arm"))]
             receiver: OpeningKey::new(&CHACHA20_POLY1305, &receiving_key).unwrap(),
+            #[cfg(not(target_arch = "arm"))]
             sender: SealingKey::new(&CHACHA20_POLY1305, &sending_key).unwrap(),
-            sending_key_counter: AtomicUsize::new(0),
-            receiving_key_counter: spin::Mutex::new(Default::default()),
-        };
-
-        #[cfg(target_arch = "arm")]
-        return Session {
-            receiving_index: local_index,
-            sending_index: peer_index,
+            #[cfg(target_arch = "arm")]
             receiver: ChaCha20Poly1305::new_aead(&receiving_key[..]),
+            #[cfg(target_arch = "arm")]
             sender: ChaCha20Poly1305::new_aead(&sending_key[..]),
             sending_key_counter: AtomicUsize::new(0),
             receiving_key_counter: spin::Mutex::new(Default::default()),
diff --git a/src/noise/timers.rs b/src/noise/timers.rs
index 27d21b90..af63245e 100644
--- a/src/noise/timers.rs
+++ b/src/noise/timers.rs
@@ -50,6 +50,7 @@ pub struct Timers {
     is_initiator: AtomicBool, // Is the owner of the timer the initiator or the responder for the last handshake?
     time_started: Instant,    // Start time of the tunnel
     timers: [Timer; TimerName::Top as usize],
+    pub(super) session_timers: [Timer; super::N_SESSIONS],
     want_keepalive: AtomicBool, // Did we receive data without sending anything back?
     want_handshake: AtomicBool, // Did we send data without hearing back?
     persistent_keepalive: AtomicUsize,
@@ -57,11 +58,12 @@ pub struct Timers {
 }
 
 impl Timers {
-    pub fn new(persistent_keepalive: Option<u16>, reset_rr: bool) -> Timers {
+    pub(super) fn new(persistent_keepalive: Option<u16>, reset_rr: bool) -> Timers {
         Timers {
             is_initiator: AtomicBool::new(false),
             time_started: Instant::now(),
             timers: Default::default(),
+            session_timers: Default::default(),
             want_keepalive: Default::default(),
             want_handshake: Default::default(),
             persistent_keepalive: AtomicUsize::new(usize::from(persistent_keepalive.unwrap_or(0))),
@@ -75,7 +77,7 @@ impl Timers {
 
     // We don't really clear the timers, but we set them to the current time to
     // so the reference time frame is the same
-    pub fn clear(&self) {
+    pub(super) fn clear(&self) {
         let now = Instant::now().duration_since(self.time_started);
         for t in &self.timers[..] {
             t.set(now);
@@ -93,7 +95,7 @@ impl Index<TimerName> for Timers {
 }
 
 impl Timer {
-    fn time(&self) -> Duration {
+    pub(super) fn time(&self) -> Duration {
         Duration::from_secs(self.time.load(Ordering::Relaxed) as _)
     }
 
@@ -103,11 +105,7 @@ impl Timer {
 }
 
 impl Tunn {
-    pub fn tick_handshake_started(&self) {
-        self.timers[TimeLastHandshakeStarted].set(self.timers[TimeCurrent].time());
-    }
-
-    pub fn timer_tick(&self, timer_name: TimerName) {
+    pub(super) fn timer_tick(&self, timer_name: TimerName) {
         match timer_name {
             TimeLastPacketReceived => {
                 self.timers.want_keepalive.store(true, Ordering::Relaxed);
@@ -123,8 +121,10 @@ impl Tunn {
         self.timers[timer_name].set(self.timers[TimeCurrent].time());
     }
 
-    pub fn timer_tick_session_established(&self, is_initiator: bool) {
+    pub(super) fn timer_tick_session_established(&self, is_initiator: bool, session_idx: usize) {
         self.timer_tick(TimeSessionEstablished);
+        self.timers.session_timers[session_idx % crate::noise::N_SESSIONS]
+            .set(self.timers[TimeCurrent].time());
         self.timers
             .is_initiator
             .store(is_initiator, Ordering::Relaxed)
@@ -145,6 +145,19 @@ impl Tunn {
         self.timers.clear();
     }
 
+    fn update_session_timers(&self, time_now: Duration) {
+        let timers = &self.timers;
+
+        for (i, t) in timers.session_timers.iter().enumerate() {
+            if time_now - t.time() > REJECT_AFTER_TIME {
+                if (*self.sessions[i].write()).take().is_some() {
+                    self.log(Verbosity::Debug, "SESSION_EXPIRED(REJECT_AFTER_TIME)");
+                }
+                t.set(time_now);
+            }
+        }
+    }
+
     pub fn update_timers<'a>(&self, dst: &'a mut [u8]) -> TunnResult<'a> {
         let mut handshake_initiation_required = false;
         let mut keepalive_required = false;
@@ -161,6 +174,8 @@ impl Tunn {
         let now = time.duration_since(timers.time_started);
         timers[TimeCurrent].set(now);
 
+        self.update_session_timers(now);
+
         // Load timers only once:
         let session_established = timers[TimeSessionEstablished].time();
         let handshake_started = timers[TimeLastHandshakeStarted].time();
@@ -249,7 +264,8 @@ impl Tunn {
                 // If we have sent a packet to a given peer but have not received a
                 // packet after from that peer for (KEEPALIVE + REKEY_TIMEOUT) ms,
                 // we initiate a new handshake.
-                if now - aut_packet_received >= KEEPALIVE_TIMEOUT + REKEY_TIMEOUT
+                if aut_packet_sent > aut_packet_received
+                    && now - aut_packet_sent >= KEEPALIVE_TIMEOUT + REKEY_TIMEOUT
                     && timers.want_handshake.swap(false, Ordering::Relaxed)
                 {
                     self.log(Verbosity::Debug, "HANDSHAKE(KEEPALIVE + REKEY_TIMEOUT)");
@@ -259,7 +275,8 @@ impl Tunn {
                 if !handshake_initiation_required {
                     // If a packet has been received from a given peer, but we have not sent one back
                     // to the given peer in KEEPALIVE ms, we send an empty packet.
-                    if now - aut_packet_sent >= KEEPALIVE_TIMEOUT
+                    if aut_packet_received > aut_packet_sent
+                        && now - aut_packet_received >= KEEPALIVE_TIMEOUT
                         && timers.want_keepalive.swap(false, Ordering::Relaxed)
                     {
                         self.log(Verbosity::Debug, "KEEPALIVE(KEEPALIVE_TIMEOUT)");


Test patch: