feat: Shuffle the client Initial crypto data (#2228)

* feat: Mix up the Initial crypto data a bit

Look for ranges of `N` or more bytes of graphical ASCII data in `data`.
Create at least one split point for each range, multiple ones each `N`
bytes if the range is long enough. Create data chunks based on those
split points. Shuffle the chunks and return them.

CC @martinthomson @dennisjackson

* Fix tests

* Fixes

* Doc fixes

* More tests and corner-case fixes

* Footgun prevention

* WIP; suggestions from @martinthomson

* Address more code review comments

* Refactor loop

* More from @martinthomson

* Again

* Create roughly five chunks per packet

* Latest suggestion from @martinthomson

* Simpler version from @martinthomson

* Fix

* Update neqo-transport/src/crypto.rs

Co-authored-by: Martin Thomson <mt@lowentropy.net>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Use `Decoder`

* Minimize diff

* Fix comment

* Update neqo-transport/src/crypto.rs

Co-authored-by: Martin Thomson <mt@lowentropy.net>
Signed-off-by: Lars Eggert <lars@eggert.org>

* Final suggestions

---------

Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: Martin Thomson <mt@lowentropy.net>

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

@@ -1199,7 +1199,7 @@ fn client_initial_retransmits_identical() {
         assert_eq!(
             client.stats().frame_tx,
             FrameStats {
-                crypto: i,
+                crypto: 2 * i,
                 ..Default::default()
             }
         );

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

@@ -251,7 +251,7 @@ fn compatible_upgrade_large_initial() {
     assert_eq!(server.version(), Version::Version2);
     // Only handshake padding is "dropped".
     assert_eq!(client.stats().dropped_rx, 1);
-    assert_eq!(server.stats().dropped_rx, 1);
+    assert_eq!(server.stats().dropped_rx, 2);
 }
 
 /// A server that supports versions 1 and 2 might prefer version 1 and that's OK.

neqo-transport/src/crypto.rs

@@ -30,6 +30,7 @@ use crate::{
     recovery::RecoveryToken,
     recv_stream::RxStreamOrderer,
     send_stream::TxBuffer,
+    shuffle::find_sni,
     stats::FrameStats,
     tparams::{TpZeroRttChecker, TransportParameters, TransportParametersHandler},
     tracking::PacketNumberSpace,
@@ -1493,13 +1494,16 @@ impl CryptoStreams {
         tokens: &mut Vec<RecoveryToken>,
         stats: &mut FrameStats,
     ) {
-        let cs = self.get_mut(space).unwrap();
-        if let Some((offset, data)) = cs.tx.next_bytes() {
+        fn write_chunk(
+            offset: u64,
+            data: &[u8],
+            builder: &mut PacketBuilder,
+        ) -> Option<(u64, usize)> {
             let mut header_len = 1 + Encoder::varint_len(offset) + 1;
 
             // Don't bother if there isn't room for the header and some data.
             if builder.remaining() < header_len + 1 {
-                return;
+                return None;
             }
             // Calculate length of data based on the minimum of:
             // - available data
@@ -1512,16 +1516,38 @@ impl CryptoStreams {
             builder.encode_varint(crate::frame::FRAME_TYPE_CRYPTO);
             builder.encode_varint(offset);
             builder.encode_vvec(&data[..length]);
+            Some((offset, length))
+        }
 
-            cs.tx.mark_as_sent(offset, length);
-
-            qdebug!("CRYPTO for {} offset={}, len={}", space, offset, length);
-            tokens.push(RecoveryToken::Crypto(CryptoRecoveryToken {
-                space,
-                offset,
-                length,
-            }));
-            stats.crypto += 1;
+        let cs = self.get_mut(space).unwrap();
+        if let Some((offset, data)) = cs.tx.next_bytes() {
+            let written = if offset == 0 {
+                if let Some(sni) = find_sni(data) {
+                    // Cut the crypto data in two at the midpoint of the SNI and swap the chunks.
+                    let mid = sni.start + (sni.end - sni.start) / 2;
+                    let (left, right) = data.split_at(mid);
+                    [
+                        write_chunk(offset + mid as u64, right, builder),
+                        write_chunk(offset, left, builder),
+                    ]
+                } else {
+                    // No SNI found, write the entire data.
+                    [write_chunk(offset, data, builder), None]
+                }
+            } else {
+                // Not at the start of the crypto stream, write the entire data.
+                [write_chunk(offset, data, builder), None]
+            };
+            for (offset, length) in written.into_iter().flatten() {
+                cs.tx.mark_as_sent(offset, length);
+                qdebug!("CRYPTO for {} offset={}, len={}", space, offset, length);
+                tokens.push(RecoveryToken::Crypto(CryptoRecoveryToken {
+                    space,
+                    offset,
+                    length,
+                }));
+                stats.crypto += 1;
+            }
         }
     }
 }

neqo-transport/src/lib.rs

@@ -46,6 +46,7 @@ pub mod send_stream;
 mod send_stream;
 mod sender;
 pub mod server;
+mod shuffle;
 mod stats;
 pub mod stream_id;
 pub mod streams;
@@ -70,6 +71,7 @@ pub use self::{
     quic_datagrams::DatagramTracking,
     recv_stream::{RecvStreamStats, RECV_BUFFER_SIZE},
     send_stream::{SendStreamStats, SEND_BUFFER_SIZE},
+    shuffle::find_sni,
     stats::Stats,
     stream_id::{StreamId, StreamType},
     version::Version,

neqo-transport/src/shuffle.rs

@@ -0,0 +1,133 @@
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+use std::ops::Range;
+
+use neqo_common::{qtrace, Decoder};
+
+/// Finds the range where the SNI extension lives, or returns `None`.
+#[must_use]
+pub fn find_sni(buf: &[u8]) -> Option<Range<usize>> {
+    #[must_use]
+    fn skip(dec: &mut Decoder, len: usize) -> Option<()> {
+        if len > dec.remaining() {
+            return None;
+        }
+        dec.skip(len);
+        Some(())
+    }
+
+    #[must_use]
+    fn skip_vec<const N: usize>(dec: &mut Decoder) -> Option<()> {
+        let len = dec.decode_uint(N)?.try_into().ok()?;
+        skip(dec, len)
+    }
+
+    let mut dec = Decoder::from(buf);
+
+    // Return if buf is empty or does not contain a ClientHello (first byte == 1)
+    if buf.is_empty() || dec.decode_byte()? != 1 {
+        return None;
+    }
+    skip(&mut dec, 3 + 2 + 32)?; // Skip length, version, random
+    skip_vec::<1>(&mut dec)?; // Skip session_id
+    skip_vec::<2>(&mut dec)?; // Skip cipher_suites
+    skip_vec::<1>(&mut dec)?; // Skip compression_methods
+    skip(&mut dec, 2)?;
+
+    while dec.remaining() >= 4 {
+        let ext_type: u16 = dec.decode_uint(2)?.try_into().ok()?;
+        let ext_len: u16 = dec.decode_uint(2)?.try_into().ok()?;
+        if ext_type == 0 {
+            // SNI!
+            let sni_len: u16 = dec.decode_uint(2)?.try_into().ok()?;
+            skip(&mut dec, 3)?; // Skip name_type and host_name length
+            let start = dec.offset();
+            let end = start + usize::from(sni_len) - 3;
+            if end > dec.offset() + dec.remaining() {
+                return None;
+            }
+            qtrace!(
+                "SNI range {start}..{end}: {:?}",
+                String::from_utf8_lossy(&buf[start..end])
+            );
+            return Some(start..end);
+        }
+        // Skip extension
+        skip(&mut dec, ext_len.into())?;
+    }
+    None
+}
+
+#[cfg(test)]
+mod tests {
+    const BUF_WITH_SNI: &[u8] = &[
+        0x01, // msg_type == 1 (ClientHello)
+        0x00, 0x01, 0xfc, // length (arbitrary)
+        0x03, 0x03, // version (TLS 1.2)
+        0x0e, 0x2d, 0x03, 0x37, 0xd9, 0x14, 0x2b, 0x32, 0x4e, 0xa8, 0xcf, 0x1f, 0xfa, 0x5b, 0x6c,
+        0xeb, 0xdd, 0x10, 0xa6, 0x49, 0x6e, 0xbf, 0xe4, 0x32, 0x3d, 0x0c, 0xe4, 0xbf, 0x90, 0xcf,
+        0x08, 0x42, // random
+        0x00, // session_id length
+        0x00, 0x08, // cipher_suites length
+        0x13, 0x01, 0x13, 0x03, 0x13, 0x02, 0xca, 0xca, // cipher_suites
+        0x01, // compression_methods length
+        0x00, // compression_methods
+        0x01, 0xcb, // extensions length
+        0xff, 0x01, 0x00, 0x01, 0x00, // renegiation_info
+        0x00, 0x2d, 0x00, 0x03, 0x02, 0x01, 0x87, // psk_exchange_modes
+        // SNI extension
+        0x00, 0x00, // Extension type (SNI)
+        0x00, 0x0e, // Extension length
+        0x00, 0x0c, // Server Name List length
+        0x00, // Name type (host_name)
+        0x00, 0x09, // Host name length
+        0x6c, 0x6f, 0x63, 0x61, 0x6c, 0x68, 0x6f, 0x73, 0x74, // server_name: "localhost"
+        0x00, 0x05, 0x00, 0x05, 0x01, 0x00, 0x00, 0x00, 0x00, // status_request
+    ];
+
+    #[test]
+    fn find_sni() {
+        // ClientHello with SNI extension
+        let range = super::find_sni(BUF_WITH_SNI).unwrap();
+        let expected_range = BUF_WITH_SNI.len() - 18..BUF_WITH_SNI.len() - 9;
+        assert_eq!(range, expected_range);
+        assert_eq!(&BUF_WITH_SNI[range], b"localhost");
+    }
+
+    #[test]
+    fn find_sni_no_sni() {
+        // ClientHello without SNI extension
+        let mut buf = Vec::from(&BUF_WITH_SNI[..BUF_WITH_SNI.len() - 39]);
+        let len = buf.len();
+        assert!(buf[len - 2] == 0x01 && buf[len - 1] == 0xcb); // Check extensions length
+                                                               // Set extensions length to 0
+        buf[len - 2] = 0x00;
+        buf[len - 1] = 0x00;
+        assert!(super::find_sni(&buf).is_none());
+    }
+
+    #[test]
+    fn find_sni_invalid_sni() {
+        // ClientHello with an SNI extension truncated somewhere in the hostname
+        let truncated = &BUF_WITH_SNI[..BUF_WITH_SNI.len() - 15];
+        assert!(super::find_sni(truncated).is_none());
+    }
+
+    #[test]
+    fn find_sni_no_ci() {
+        // Not a ClientHello (msg_type != 1)
+        let buf = [0; 1];
+        assert!(super::find_sni(&buf).is_none());
+    }
+
+    #[test]
+    fn find_sni_malformed_ci() {
+        // Buffer starting with `1` but otherwise malformed
+        let buf = [1; 1];
+        assert!(super::find_sni(&buf).is_none());
+    }
+}

test/test.sh

@@ -13,7 +13,7 @@ tmp=$(mktemp -d)
 
 cargo build --bin neqo-client --bin neqo-server
 
-addr=127.0.0.1
+addr=localhost
 port=4433
 path=/20000
 flags="--verbose --verbose --verbose --qlog-dir $tmp --use-old-http --alpn hq-interop --quic-version 1"