Randomize the first packet number

This is mostly the work of @larseggert, I'm just repackaging it.  This
will target his branch with any changes.

This randomizes the starting packet number the client uses for the Initial packet number space.

We don't randomize this on the server, since otherwise we'd need even more changes to the tests to account for that.

Fixes mozilla#2462.
Closes mozilla#2499.

Author: martinthomson

neqo-transport/src/connection/mod.rs

@@ -392,6 +392,7 @@ impl Connection {
             c.conn_params.get_versions().compatible(),
             Role::Client,
             &dcid,
+            c.conn_params.randomize_first_pn_enabled(),
         )?;
         c.original_destination_cid = Some(dcid);
         let path = Path::temporary(
@@ -1336,6 +1337,7 @@ impl Connection {
             self.conn_params.get_versions().compatible(),
             self.role,
             &retry_scid,
+            false, // don't randomize on Retry
         )?;
         self.address_validation = AddressValidationInfo::Retry {
             token: packet.token().to_vec(),
@@ -1523,7 +1525,11 @@ impl Connection {
                 // Record the client's selected CID so that it can be accepted until
                 // the client starts using a real connection ID.
                 let dcid = ConnectionId::from(packet.dcid());
-                self.crypto.states_mut().init_server(version, &dcid)?;
+                self.crypto.states_mut().init_server(
+                    version,
+                    &dcid,
+                    self.conn_params.randomize_first_pn_enabled(),
+                )?;
                 self.original_destination_cid = Some(dcid);
                 self.set_state(State::WaitInitial, now);
 
@@ -3072,7 +3078,8 @@ impl Connection {
                 .original_destination_cid
                 .as_ref()
                 .ok_or(Error::ProtocolViolation)?;
-            self.crypto.states_mut().init_server(version, dcid)?;
+            // No need to randomize the starting packet number; that's already taken care of.
+            self.crypto.states_mut().init_server(version, dcid, false)?;
             version
         };
 

neqo-transport/src/connection/params.rs

@@ -94,6 +94,8 @@ pub struct ConnectionParameters {
     sni_slicing: bool,
     /// Whether to enable mlkem768nistp256-sha256.
     mlkem: bool,
+    /// Whether to randomize the packet number of the first Initial packet.
+    randomize_first_pn: bool,
 }
 
 impl Default for ConnectionParameters {
@@ -125,6 +127,7 @@ impl Default for ConnectionParameters {
             pmtud_iface_mtu: true,
             sni_slicing: true,
             mlkem: true,
+            randomize_first_pn: true,
         }
     }
 }
@@ -437,6 +440,17 @@ impl ConnectionParameters {
         self
     }
 
+    #[must_use]
+    pub const fn randomize_first_pn_enabled(&self) -> bool {
+        self.randomize_first_pn
+    }
+
+    #[must_use]
+    pub const fn randomize_first_pn(mut self, randomize_first_pn: bool) -> Self {
+        self.randomize_first_pn = randomize_first_pn;
+        self
+    }
+
     /// # Errors
     /// When a connection ID cannot be obtained.
     /// # Panics

neqo-transport/src/connection/tests/cc.rs

@@ -58,7 +58,9 @@ enum CongestionSignal {
 }
 
 fn cc_slow_start_to_cong_avoidance_recovery_period(congestion_signal: CongestionSignal) {
-    let mut client = default_client();
+    // This test needs to calculate largest_acked, which is difficult if packet number
+    // randomization is enabled.
+    let mut client = new_client(ConnectionParameters::default().randomize_first_pn(false));
     let mut server = default_server();
     let now = connect_rtt_idle(&mut client, &mut server, DEFAULT_RTT);
 

neqo-transport/src/connection/tests/datagram.rs

@@ -8,10 +8,9 @@ use std::{cell::RefCell, rc::Rc};
 
 use neqo_common::event::Provider as _;
 use static_assertions::const_assert;
-use test_fixture::now;
 
 use super::{
-    assert_error, connect_force_idle, default_client, default_server, new_client, new_server,
+    assert_error, connect_force_idle, default_client, default_server, new_client, new_server, now,
     AT_LEAST_PTO,
 };
 use crate::{

neqo-transport/src/connection/tests/handshake.rs

@@ -1459,7 +1459,8 @@ fn client_initial_pto_matches_custom_initial_rtt() {
 #[test]
 fn server_initial_retransmits_identical() {
     let mut now = now();
-    let mut client = default_client();
+    // We calculate largest_acked, which is difficult with packet number randomization.
+    let mut client = new_client(ConnectionParameters::default().randomize_first_pn(false));
     let mut ci = client.process_output(now).dgram();
     let mut ci2 = client.process_output(now).dgram();
 

neqo-transport/src/connection/tests/resumption.rs

@@ -86,11 +86,12 @@ fn ticket_rtt(rtt: Duration) -> Duration {
     // A simple ACK frame for a single packet with packet number 0.
     const ACK_FRAME_1: &[u8] = &[0x02, 0x00, 0x00, 0x00, 0x00];
 
-    // This test needs to decrypt the CI, so turn off MLKEM.
+    // This test needs to predicts the exact shape of an ACK frame, so disable randomization.
     let mut client = new_client(
         ConnectionParameters::default()
             .versions(Version::Version1, vec![Version::Version1])
-            .mlkem(false),
+            .mlkem(false)
+            .randomize_first_pn(false),
     );
     let mut server = default_server();
     let mut now = now();
@@ -109,7 +110,6 @@ fn ticket_rtt(rtt: Duration) -> Duration {
     // Now decrypt the packet.
     let (aead, hp) = initial_aead_and_hp(&client_dcid, Role::Server);
     let (header, pn) = header_protection::remove(&hp, protected_header, payload);
-    assert_eq!(pn, 0);
     let pn_len = header.len() - protected_header.len();
     let mut buf = vec![0; payload.len()];
     let mut plaintext = aead

neqo-transport/src/crypto.rs

@@ -18,11 +18,11 @@ use enum_map::EnumMap;
 use neqo_common::{hex, hex_snip_middle, qdebug, qinfo, qtrace, Buffer, Encoder, Role};
 pub use neqo_crypto::Epoch;
 use neqo_crypto::{
-    hkdf, hp, Aead, Agent, AntiReplay, Cipher, Error as CryptoError, HandshakeState, PrivateKey,
-    PublicKey, Record, RecordList, ResumptionToken, SymKey, ZeroRttChecker, TLS_AES_128_GCM_SHA256,
-    TLS_AES_256_GCM_SHA384, TLS_CHACHA20_POLY1305_SHA256, TLS_CT_HANDSHAKE, TLS_GRP_EC_SECP256R1,
-    TLS_GRP_EC_SECP384R1, TLS_GRP_EC_SECP521R1, TLS_GRP_EC_X25519, TLS_GRP_KEM_MLKEM768X25519,
-    TLS_VERSION_1_3,
+    hkdf, hp, random, Aead, Agent, AntiReplay, Cipher, Error as CryptoError, HandshakeState,
+    PrivateKey, PublicKey, Record, RecordList, ResumptionToken, SymKey, ZeroRttChecker,
+    TLS_AES_128_GCM_SHA256, TLS_AES_256_GCM_SHA384, TLS_CHACHA20_POLY1305_SHA256, TLS_CT_HANDSHAKE,
+    TLS_GRP_EC_SECP256R1, TLS_GRP_EC_SECP384R1, TLS_GRP_EC_SECP521R1, TLS_GRP_EC_X25519,
+    TLS_GRP_KEM_MLKEM768X25519, TLS_VERSION_1_3,
 };
 
 use crate::{
@@ -477,17 +477,18 @@ impl CryptoDxState {
         epoch: Epoch,
         secret: &SymKey,
         cipher: Cipher,
+        min_pn: packet::Number,
     ) -> Res<Self> {
-        qdebug!("Making {direction:?} {epoch:?} CryptoDxState, v={version:?} cipher={cipher}",);
+        qdebug!("Making {direction:?} {epoch:?} CryptoDxState, v={version:?} cipher={cipher} min_pn={min_pn}",);
         let hplabel = String::from(version.label_prefix()) + "hp";
         Ok(Self {
             version,
             direction,
             epoch: usize::from(epoch),
             aead: Aead::new(TLS_VERSION_1_3, cipher, secret, version.label_prefix())?,
             hpkey: hp::Key::extract(TLS_VERSION_1_3, cipher, secret, &hplabel)?,
-            used_pn: 0..0,
-            min_pn: 0,
+            used_pn: min_pn..min_pn,
+            min_pn,
             invocations: Self::limit(direction, cipher),
             largest_packet_len: INITIAL_LARGEST_PACKET_LEN,
         })
@@ -498,6 +499,7 @@ impl CryptoDxState {
         direction: CryptoDxDirection,
         label: &str,
         dcid: &[u8],
+        min_pn: packet::Number,
     ) -> Res<Self> {
         qtrace!("new_initial {version:?} {}", ConnectionIdRef::from(dcid));
         let salt = version.initial_salt();
@@ -511,7 +513,7 @@ impl CryptoDxState {
 
         let secret = hkdf::expand_label(TLS_VERSION_1_3, cipher, &initial_secret, &[], label)?;
 
-        Self::new(version, direction, Epoch::Initial, &secret, cipher)
+        Self::new(version, direction, Epoch::Initial, &secret, cipher, min_pn)
     }
 
     /// Determine the confidentiality and integrity limits for the cipher.
@@ -732,6 +734,7 @@ impl CryptoDxState {
             CryptoDxDirection::Write,
             "server in",
             CLIENT_CID,
+            0,
         )
         .unwrap()
     }
@@ -794,7 +797,7 @@ impl CryptoDxAppData {
         cipher: Cipher,
     ) -> Res<Self> {
         Ok(Self {
-            dx: CryptoDxState::new(version, dir, Epoch::ApplicationData, secret, cipher)?,
+            dx: CryptoDxState::new(version, dir, Epoch::ApplicationData, secret, cipher, 0)?,
             cipher,
             next_secret: Self::update_secret(cipher, secret)?,
         })
@@ -1022,7 +1025,13 @@ impl CryptoStates {
 
     /// Create the initial crypto state.
     /// Note that the version here can change and that's OK.
-    pub fn init<'v, V>(&mut self, versions: V, role: Role, dcid: &[u8]) -> Res<()>
+    pub fn init<'v, V>(
+        &mut self,
+        versions: V,
+        role: Role,
+        dcid: &[u8],
+        randomize_first_pn: bool,
+    ) -> Res<()>
     where
         V: IntoIterator<Item = &'v Version>,
     {
@@ -1034,15 +1043,27 @@ impl CryptoStates {
             Role::Server => (SERVER_INITIAL_LABEL, CLIENT_INITIAL_LABEL),
         };
 
+        let min_pn = if randomize_first_pn {
+            let r = random::<4>();
+            let pn = packet::Number::from;
+            // A random starting packet number. 5 uniformly random bits.
+            // Then maybe add up to 256 to occasionally nudge into a second byte for
+            // both varint and packet number encodings, heavily weighted to small values.
+            // And a small offset to ensure that the value is always non-zero.
+            (pn(r[0]) & 0x1f) + (pn(r[1]) + pn(r[2]) + pn(r[3])).saturating_sub(512) + 1
+        } else {
+            0
+        };
+
         for v in versions {
             qdebug!(
                 "[{self}] Creating initial cipher state v={v:?}, role={role:?} dcid={}",
                 hex(dcid)
             );
 
             let mut initial = CryptoState {
-                tx: CryptoDxState::new_initial(*v, CryptoDxDirection::Write, write, dcid)?,
-                rx: CryptoDxState::new_initial(*v, CryptoDxDirection::Read, read, dcid)?,
+                tx: CryptoDxState::new_initial(*v, CryptoDxDirection::Write, write, dcid, min_pn)?,
+                rx: CryptoDxState::new_initial(*v, CryptoDxDirection::Read, read, dcid, 0)?,
             };
             if let Some(prev) = &self.initials[*v] {
                 qinfo!(
@@ -1063,9 +1084,14 @@ impl CryptoStates {
     /// This is maybe slightly inefficient in the first case, because we might
     /// not need the send keys if the packet is subsequently discarded, but
     /// the overall effort is small enough to write off.
-    pub fn init_server(&mut self, version: Version, dcid: &[u8]) -> Res<()> {
+    pub fn init_server(
+        &mut self,
+        version: Version,
+        dcid: &[u8],
+        randomize_first_pn: bool,
+    ) -> Res<()> {
         if self.initials[version].is_none() {
-            self.init(&[version], Role::Server, dcid)?;
+            self.init(&[version], Role::Server, dcid, randomize_first_pn)?;
         }
         Ok(())
     }
@@ -1102,6 +1128,7 @@ impl CryptoStates {
             Epoch::ZeroRtt,
             secret,
             cipher,
+            0,
         )?);
         Ok(())
     }
@@ -1143,13 +1170,15 @@ impl CryptoStates {
                 Epoch::Handshake,
                 write_secret,
                 cipher,
+                0,
             )?,
             rx: CryptoDxState::new(
                 version,
                 CryptoDxDirection::Read,
                 Epoch::Handshake,
                 read_secret,
                 cipher,
+                0,
             )?,
         });
         Ok(())

neqo-transport/src/saved.rs

@@ -47,10 +47,10 @@ impl SavedDatagrams {
         let store = self.store(epoch);
 
         if store.len() < MAX_SAVED_DATAGRAMS {
-            qdebug!("saving datagram of {} bytes", d.len());
+            qdebug!("saving {epoch:?} datagram of {} bytes", d.len());
             store.push(SavedDatagram { d, t });
         } else {
-            qinfo!("not saving datagram of {} bytes", d.len());
+            qinfo!("not saving {epoch:?} datagram of {} bytes", d.len());
         }
     }
 

neqo-transport/tests/connection.rs

@@ -85,11 +85,13 @@ fn reorder_server_initial() {
     // A simple ACK frame for a single packet with packet number 0.
     const ACK_FRAME: &[u8] = &[0x02, 0x00, 0x00, 0x00, 0x00];
 
-    // This test needs to decrypt the CI, so turn off MLKEM.
+    // This test predicts the precise format of an ACK frame, so turn off MLKEM
+    // and packet number randomization.
     let mut client = new_client(
         ConnectionParameters::default()
             .versions(Version::Version1, vec![Version::Version1])
-            .mlkem(false),
+            .mlkem(false)
+            .randomize_first_pn(false),
     );
     let mut server = default_server();
 
@@ -324,3 +326,65 @@ fn handshake_mlkem768x25519() {
         neqo_crypto::TLS_GRP_KEM_MLKEM768X25519
     );
 }
+
+#[test]
+fn client_initial_packet_number() {
+    // Check that the initial packet number is randomized (i.e, > 0) if the `randomize_first_pn`
+    // connection parameter is set, and that it is zero when not.
+    for randomize in [true, false] {
+        // This test needs to decrypt the CI, so turn off MLKEM.
+        let mut client = new_client(
+            ConnectionParameters::default()
+                .versions(Version::Version1, vec![Version::Version1])
+                .mlkem(false)
+                .randomize_first_pn(randomize),
+        );
+
+        let client_initial = client.process_output(now());
+        let (protected_header, client_dcid, _, payload) =
+            decode_initial_header(client_initial.as_dgram_ref().unwrap(), Role::Client).unwrap();
+        let (_, hp) = initial_aead_and_hp(client_dcid, Role::Client);
+        let (_, pn) = header_protection::remove(&hp, protected_header, payload);
+        assert!(
+            randomize && pn > 0 || !randomize && pn == 0,
+            "randomize {randomize} = {pn}"
+        );
+    }
+}
+
+#[test]
+fn server_initial_packet_number() {
+    // Check that the initial packet number is randomized (i.e, > 0) if the `randomize_first_pn`
+    // connection parameter is set, and that it is zero when not.
+    for randomize in [true, false] {
+        // This test needs to decrypt the CI, so turn off MLKEM.
+        let mut client = new_client(
+            ConnectionParameters::default()
+                .versions(Version::Version1, vec![Version::Version1])
+                .mlkem(false),
+        );
+        let mut server = new_server(
+            DEFAULT_ALPN,
+            ConnectionParameters::default()
+                .versions(Version::Version1, vec![Version::Version1])
+                .randomize_first_pn(randomize),
+        );
+
+        let client_initial = client.process_output(now()).dgram();
+        let (_protected_header, client_dcid, _scid, _payload) =
+            decode_initial_header(client_initial.as_ref().unwrap(), Role::Client).unwrap();
+
+        let (_, hp) = initial_aead_and_hp(client_dcid, Role::Server);
+
+        let server_initial = server.process(client_initial, now()).dgram();
+        let (protected_header, _dcid, _scid, payload) =
+            decode_initial_header(server_initial.as_ref().unwrap(), Role::Server).unwrap();
+
+        let (_, pn) = header_protection::remove(&hp, protected_header, payload);
+        println!();
+        assert!(
+            randomize && pn > 0 || !randomize && pn == 0,
+            "randomize {randomize} = {pn}"
+        );
+    }
+}