From ac512395c562fbdfd0e2a44d5fe8da153b2f3f34 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Wed, 10 Apr 2024 15:36:09 +0300 Subject: [PATCH 1/5] fix: Make sure the primary path exists at various steps in the handshake Fixes #1448 --- neqo-transport/src/connection/mod.rs | 20 +++++----- neqo-transport/tests/server.rs | 60 ++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 9 deletions(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 8522507a69..1cc602b808 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -46,7 +46,7 @@ use crate::{ quic_datagrams::{DatagramTracking, QuicDatagrams}, recovery::{LossRecovery, RecoveryToken, SendProfile}, recv_stream::RecvStreamStats, - rtt::GRANULARITY, + rtt::{GRANULARITY, INITIAL_RTT}, send_stream::SendStream, stats::{Stats, StatsCell}, stream_id::StreamType, @@ -610,11 +610,11 @@ impl Connection { /// a value of this approximate order. Don't use this for loss recovery, /// only use it where a more precise value is not important. fn pto(&self) -> Duration { - self.paths - .primary() - .borrow() - .rtt() - .pto(PacketNumberSpace::ApplicationData) + if let Some(path) = self.paths.primary_fallible() { + path.borrow().rtt().pto(PacketNumberSpace::ApplicationData) + } else { + 3 * INITIAL_RTT + } } fn create_resumption_token(&mut self, now: Instant) { @@ -962,9 +962,11 @@ impl Connection { let res = self.crypto.states.check_key_update(now); self.absorb_error(now, res); - let lost = self.loss_recovery.timeout(&self.paths.primary(), now); - self.handle_lost_packets(&lost); - qlog::packets_lost(&mut self.qlog, &lost); + if let Some(path) = self.paths.primary_fallible() { + let lost = self.loss_recovery.timeout(&path, now); + self.handle_lost_packets(&lost); + qlog::packets_lost(&mut self.qlog, &lost); + } if self.release_resumption_token_timer.is_some() { self.create_resumption_token(now); diff --git a/neqo-transport/tests/server.rs b/neqo-transport/tests/server.rs index 7388e0fee7..88c6ff21c2 100644 --- a/neqo-transport/tests/server.rs +++ b/neqo-transport/tests/server.rs @@ -477,6 +477,66 @@ fn bad_client_initial() { assert_eq!(res, Output::None); } +#[test] +fn bad_client_initial_connection_close() { + let mut client = default_client(); + let mut server = default_server(); + + let dgram = client.process(None, now()).dgram().expect("a datagram"); + let (header, d_cid, s_cid, payload) = decode_initial_header(&dgram, Role::Client); + let (aead, hp) = initial_aead_and_hp(d_cid, Role::Client); + let (_, pn) = remove_header_protection(&hp, header, payload); + + // let mut payload_enc = Encoder::from(plaintext); + let mut payload_enc = Encoder::with_capacity(1200); + payload_enc.encode(&[0x1c, 0x01, 0x00, 0x00]); // Add a CONNECTION_CLOSE frame. + + // Make a new header with a 1 byte packet number length. + let mut header_enc = Encoder::new(); + header_enc + .encode_byte(0xc0) // Initial with 1 byte packet number. + .encode_uint(4, Version::default().wire_version()) + .encode_vec(1, d_cid) + .encode_vec(1, s_cid) + .encode_vvec(&[]) + .encode_varint(u64::try_from(payload_enc.len() + aead.expansion() + 1).unwrap()) + .encode_byte(u8::try_from(pn).unwrap()); + + let mut ciphertext = header_enc.as_ref().to_vec(); + ciphertext.resize(header_enc.len() + payload_enc.len() + aead.expansion(), 0); + let v = aead + .encrypt( + pn, + header_enc.as_ref(), + payload_enc.as_ref(), + &mut ciphertext[header_enc.len()..], + ) + .unwrap(); + assert_eq!(header_enc.len() + v.len(), ciphertext.len()); + // Pad with zero to get up to 1200. + ciphertext.resize(1200, 0); + + apply_header_protection( + &hp, + &mut ciphertext, + (header_enc.len() - 1)..header_enc.len(), + ); + let bad_dgram = Datagram::new( + dgram.source(), + dgram.destination(), + dgram.tos(), + dgram.ttl(), + ciphertext, + ); + + // The server should ignore this and go to Draining. + let mut now = now(); + let response = server.process(Some(&bad_dgram), now); + now += response.callback(); + let response = server.process(None, now); + assert_eq!(response, Output::None); +} + #[test] fn version_negotiation_ignored() { let mut server = default_server(); From 2165718720ca6a8d6b50b03d608df04e088276d8 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Wed, 10 Apr 2024 17:56:30 +0300 Subject: [PATCH 2/5] Tweak --- neqo-transport/src/connection/mod.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 1cc602b808..5a301eff3f 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -46,7 +46,7 @@ use crate::{ quic_datagrams::{DatagramTracking, QuicDatagrams}, recovery::{LossRecovery, RecoveryToken, SendProfile}, recv_stream::RecvStreamStats, - rtt::{GRANULARITY, INITIAL_RTT}, + rtt::{RttEstimate, GRANULARITY}, send_stream::SendStream, stats::{Stats, StatsCell}, stream_id::StreamType, @@ -610,11 +610,10 @@ impl Connection { /// a value of this approximate order. Don't use this for loss recovery, /// only use it where a more precise value is not important. fn pto(&self) -> Duration { - if let Some(path) = self.paths.primary_fallible() { - path.borrow().rtt().pto(PacketNumberSpace::ApplicationData) - } else { - 3 * INITIAL_RTT - } + self.paths.primary_fallible().map_or( + RttEstimate::default().pto(PacketNumberSpace::ApplicationData), + |p| p.borrow().rtt().pto(PacketNumberSpace::ApplicationData), + ) } fn create_resumption_token(&mut self, now: Instant) { From d4bd6d8eac48675b615ab132bde98f4681db8adf Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Wed, 10 Apr 2024 18:00:47 +0300 Subject: [PATCH 3/5] If I understand things correctly, `map_or_else` may be better here --- neqo-transport/src/connection/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 5a301eff3f..60436267e5 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -610,8 +610,8 @@ impl Connection { /// a value of this approximate order. Don't use this for loss recovery, /// only use it where a more precise value is not important. fn pto(&self) -> Duration { - self.paths.primary_fallible().map_or( - RttEstimate::default().pto(PacketNumberSpace::ApplicationData), + self.paths.primary_fallible().map_or_else( + || RttEstimate::default().pto(PacketNumberSpace::ApplicationData), |p| p.borrow().rtt().pto(PacketNumberSpace::ApplicationData), ) } From d1103d0d9768b49422b27170a48a9388751fe91d Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Thu, 11 Apr 2024 14:10:35 +0300 Subject: [PATCH 4/5] Another one --- neqo-transport/src/connection/mod.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 60436267e5..f4e2847609 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -2862,8 +2862,11 @@ impl Connection { { qdebug!([self], "Rx ACK space={}, ranges={:?}", space, ack_ranges); + let Some(path) = self.paths.primary_fallible() else { + return; + }; let (acked_packets, lost_packets) = self.loss_recovery.on_ack_received( - &self.paths.primary(), + &path, space, largest_acknowledged, ack_ranges, From 94080497a6ea1f8d3de86a3131d4839591b136b1 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Fri, 12 Apr 2024 11:21:14 +0300 Subject: [PATCH 5/5] Update server.rs Co-authored-by: Max Inden Signed-off-by: Lars Eggert --- neqo-transport/tests/server.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/neqo-transport/tests/server.rs b/neqo-transport/tests/server.rs index 88c6ff21c2..745ee79520 100644 --- a/neqo-transport/tests/server.rs +++ b/neqo-transport/tests/server.rs @@ -487,7 +487,6 @@ fn bad_client_initial_connection_close() { let (aead, hp) = initial_aead_and_hp(d_cid, Role::Client); let (_, pn) = remove_header_protection(&hp, header, payload); - // let mut payload_enc = Encoder::from(plaintext); let mut payload_enc = Encoder::with_capacity(1200); payload_enc.encode(&[0x1c, 0x01, 0x00, 0x00]); // Add a CONNECTION_CLOSE frame.