diff --git a/neqo-transport/src/crypto.rs b/neqo-transport/src/crypto.rs index 5bf52c1b45..ff76f7d06b 100644 --- a/neqo-transport/src/crypto.rs +++ b/neqo-transport/src/crypto.rs @@ -1562,9 +1562,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)) } @@ -1572,41 +1571,44 @@ 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 { + qdebug!("XXX SNI slicing enabled"); + 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, 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/shuffle.rs b/neqo-transport/src/shuffle.rs index fd81190c27..88e3722570 100644 --- a/neqo-transport/src/shuffle.rs +++ b/neqo-transport/src/shuffle.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; @@ -68,6 +71,8 @@ pub fn find_sni(buf: &[u8]) -> Option> { #[cfg(test)] mod tests { + use test_fixture::{now, default_client, default_server}; + const BUF_WITH_SNI: &[u8] = &[ 0x01, // msg_type == 1 (ClientHello) 0x00, 0x01, 0xfc, // length (arbitrary) @@ -107,10 +112,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 +124,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) @@ -134,4 +147,19 @@ mod tests { let buf = [1; 1]; assert!(super::find_sni(&buf).is_none()); } + + #[test] + fn sni_no_slicing_at_nonzero_offset() { + let mut client = default_client(); + let mut server = default_server(); + let mut now = now(); + + let ch1 = client.process_output(now).dgram(); + let _ch2 = client.process_output(now).dgram(); + let ack = server.process(ch1, now).dgram(); + now += client.process(ack, now).callback(); + let stats = client.stats().frame_tx; + _ = client.process_output(now).dgram(); + assert_eq!(stats.crypto + 1, client.stats().frame_tx.crypto); + } }