diff --git a/neqo-transport/src/crypto.rs b/neqo-transport/src/crypto.rs index 68c9ab3411..2552852966 100644 --- a/neqo-transport/src/crypto.rs +++ b/neqo-transport/src/crypto.rs @@ -36,7 +36,7 @@ use crate::{ recovery::RecoveryToken, recv_stream::RxStreamOrderer, send_stream::TxBuffer, - shuffle::find_sni, + sni::find_sni, stats::FrameStats, tparams::{TpZeroRttChecker, TransportParameters, TransportParametersHandler}, tracking::PacketNumberSpace, @@ -1554,9 +1554,8 @@ impl CryptoStreams { (left, _) = left.split_at(limit - right.len()); } else { // Both chunks are too long to fit into one packet. Just send a part of each. - let half_limit = limit / 2; - (left, _) = left.split_at(half_limit); - (right, _) = right.split_at(half_limit); + (left, _) = left.split_at(limit / 2); + (right, _) = right.split_at(limit / 2); } ((left_offset, left), (right_offset, right)) } @@ -1564,41 +1563,43 @@ impl CryptoStreams { let Some(cs) = self.get_mut(space) else { return; }; - let Some((offset, data)) = cs.tx.next_bytes() else { - return; - }; - let written = if sni_slicing { - 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); - - // Truncate the chunks so we can fit them into roughly evenly-filled packets. - let packets_needed = data.len().div_ceil(builder.limit()); - let limit = data.len() / packets_needed; - let ((left_offset, left), (right_offset, right)) = - limit_chunks((offset, left), (offset + mid as u64, right), limit); - ( - write_chunk(right_offset, right, builder), - write_chunk(left_offset, left, builder), - ) + while let Some((offset, data)) = cs.tx.next_bytes() { + let written = if sni_slicing && 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); + + // Truncate the chunks so we can fit them into roughly evenly-filled packets. + let packets_needed = data.len().div_ceil(builder.limit()); + let limit = data.len() / packets_needed; + let ((left_offset, left), (right_offset, right)) = + limit_chunks((offset, left), (offset + mid as u64, right), limit); + ( + write_chunk(right_offset, right, builder), + write_chunk(left_offset, left, builder), + ) + } else { + // No SNI found, write the entire data. + (write_chunk(offset, data, builder), None) + } } else { - // No SNI found, write the entire data. + // SNI slicing disabled or data not at offset 0, write the entire data. (write_chunk(offset, data, builder), None) - } - } else { - // SNI slicing disabled, write the entire data. - (write_chunk(offset, data, builder), None) - }; + }; - match written { - (None, None) => (), - (None, Some((offset, len))) | (Some((offset, len)), None) => { - mark_as_sent(cs, space, tokens, offset, len, stats); - } - (Some((offset1, len1)), Some((offset2, len2))) => { - mark_as_sent(cs, space, tokens, offset1, len1, stats); - mark_as_sent(cs, space, tokens, offset2, len2, stats); + match written { + (None, None) => break, + (None, Some((offset, len))) | (Some((offset, len)), None) => { + mark_as_sent(cs, space, tokens, offset, len, stats); + } + (Some((offset1, len1)), Some((offset2, len2))) => { + mark_as_sent(cs, space, tokens, offset1, len1, stats); + mark_as_sent(cs, space, tokens, offset2, len2, stats); + // We only end up in this arm if we successfully sliced above. In that case, + // don't try and fit more crypto data into this packet. + break; + } } } } diff --git a/neqo-transport/src/lib.rs b/neqo-transport/src/lib.rs index 0ba8d77bcc..fc925ca1b3 100644 --- a/neqo-transport/src/lib.rs +++ b/neqo-transport/src/lib.rs @@ -44,7 +44,7 @@ pub mod send_stream; mod send_stream; mod sender; pub mod server; -mod shuffle; +mod sni; mod stats; pub mod stream_id; pub mod streams; @@ -69,7 +69,7 @@ pub use self::{ quic_datagrams::DatagramTracking, recv_stream::{RecvStreamStats, RECV_BUFFER_SIZE}, send_stream::{SendStreamStats, SEND_BUFFER_SIZE}, - shuffle::find_sni, + sni::find_sni, stats::Stats, stream_id::{StreamId, StreamType}, version::Version, diff --git a/neqo-transport/src/shuffle.rs b/neqo-transport/src/sni.rs similarity index 86% rename from neqo-transport/src/shuffle.rs rename to neqo-transport/src/sni.rs index fd81190c27..7b2773315f 100644 --- a/neqo-transport/src/shuffle.rs +++ b/neqo-transport/src/sni.rs @@ -47,7 +47,10 @@ pub fn find_sni(buf: &[u8]) -> Option> { let ext_len: u16 = dec.decode_uint()?; if ext_type == 0 { // SNI! - let sni_len: u16 = dec.decode_uint()?; + let sni_len = dec.decode_uint::()?; + if sni_len < 3 { + return None; + } skip(&mut dec, 3)?; // Skip name_type and host_name length let start = dec.offset(); let end = start + usize::from(sni_len) - 3; @@ -107,10 +110,8 @@ mod tests { // 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_eq!(&buf[len - 2..len], &[0x01, 0xcb], "check extensions length"); + buf[len - 2..len].copy_from_slice(&[0x00, 0x00]); // Set extensions length to 0 assert!(super::find_sni(&buf).is_none()); } @@ -121,6 +122,16 @@ mod tests { assert!(super::find_sni(truncated).is_none()); } + #[test] + fn find_sni_invalid_sni_length() { + // ClientHello with an SNI extension with an invalid length + let mut buf = Vec::from(BUF_WITH_SNI); + let len = buf.len(); + assert_eq!(&buf[len - 23..len - 21], &[0x00, 0x0c], "check SNI length"); + buf[len - 23..len - 21].copy_from_slice(&[0x00, 0x02]); // Set Server Name List length to 2 + assert!(super::find_sni(&buf).is_none()); + } + #[test] fn find_sni_no_ci() { // Not a ClientHello (msg_type != 1) diff --git a/neqo-transport/tests/sni.rs b/neqo-transport/tests/sni.rs new file mode 100644 index 0000000000..916c031b02 --- /dev/null +++ b/neqo-transport/tests/sni.rs @@ -0,0 +1,44 @@ +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use neqo_transport::ConnectionParameters; +use test_fixture::{default_client, default_server, new_client, now}; + +#[test] +fn sni_no_slicing_at_nonzero_offset() { + let mut client = default_client(); + let mut server = default_server(); + let mut now = now(); + + // This packet will have two CRPYTO frames [y..end] and [0..x], where x < y. + let ch1 = client.process_output(now).dgram(); + assert_eq!(client.stats().frame_tx.crypto, 2); + // This packet will have one CRYPTO frame [x..y]. + let _ch2 = client.process_output(now).dgram(); + assert_eq!(client.stats().frame_tx.crypto, 3); + // We are dropping the second packet and only deliver the first. + let ack = server.process(ch1, now).dgram(); + // Client will now RTX the second packet. + now += client.process(ack, now).callback(); + // Make sure it only has one CRYPTO frame. + _ = client.process_output(now).dgram(); + assert_eq!(client.stats().frame_tx.crypto, 4); +} + +#[test] +fn sni_no_slicing_at_nonzero_offset_no_mlkem() { + let mut client = new_client(ConnectionParameters::default().mlkem(false)); + let mut now = now(); + + // This packet will have two CRPYTO frames [x..end] and [0..x]. + _ = client.process_output(now).dgram(); + assert_eq!(client.stats().frame_tx.crypto, 2); + // Client will now RTX the packet. + now += client.process_output(now).callback(); + // Make sure it has two CRYPTO frames. + _ = client.process_output(now).dgram(); + assert_eq!(client.stats().frame_tx.crypto, 4); +} diff --git a/qns/Dockerfile b/qns/Dockerfile index f44f96cf8d..5719ab36b8 100644 --- a/qns/Dockerfile +++ b/qns/Dockerfile @@ -1,4 +1,4 @@ -FROM lukemathwalker/cargo-chef@sha256:66b16e2261871e522c3c8b2bc2da4222843e03b3581f4f2a98ac8fc59fa3d3ae AS chef +FROM lukemathwalker/cargo-chef@sha256:2c9b2693a474b223a20f1e2bf850816d51848920bd34794fdf7c09afcaaa0fa6 AS chef WORKDIR /app