Skip to content

Commit f6a8c5b

Browse files
committed
Fix race condition between 0-RTT and Incoming
Closes #1820 The fix: - Endpoint now maintains a slab with an entry for each pending Incoming to buffer received data. - ConnectionIndex now maps initial DCID to that slab key immediately upon construction of Incoming. - If Incoming is accepted, association is overridden with association with ConnectionHandle, and all buffered datagrams are fed to newly constructed Connection. - If Incoming is refused/retried/ignored, or accepting errors, association and slab entry are cleaned up to prevent memory leak. Additional considerations: - The Incoming::ignore operation can no longer be implemented as just dropping it. To help prevent incorrect API usage, proto::Incoming is modified to log a warning if it is dropped without being passed to Endpoint::accept/refuse/retry/ignore. - To help protect against memory exhaustion attacks, per-Incoming buffered data is limited to twice the receive window or 10 KB, which- ever is larger. Excessive packets silently dropped. - Does this introduce a new vulnerability to an attack in which an attacker could spam a server with 0-RTT packets with the same connection ID as it observed a client attempting to initiate a 0-RTT connection to the server? I do think so. Is this a severe problem? Here's two reasons I don't think so: 1. The default receive window is set to max value, so this won't actually kick in unless the user is already hardening against adverse conditions. 2. It is already possible for an on-path attacker to distrupt a connection handshake if 0.5-RTT data is being used, so this probably doesn't actually expand the set of situations in which it's vulnerable to this kind of vulnerability. Could this be avoided? Possibly by introducing additional state to the buffering state to validate whether these packets are validly encrypted for the associated connection? However, that may risk making these operations costly enough that they start to defeat the DDOS-resistance abilities of the Incoming API.
1 parent 82a67db commit f6a8c5b

5 files changed

Lines changed: 166 additions & 53 deletions

File tree

quinn-proto/src/connection/mod.rs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,8 @@ use crate::{
2525
packet::{Header, InitialHeader, InitialPacket, LongType, Packet, PartialDecode, SpaceId},
2626
range_set::ArrayRangeSet,
2727
shared::{
28-
ConnectionEvent, ConnectionEventInner, ConnectionId, EcnCodepoint, EndpointEvent,
29-
EndpointEventInner,
28+
ConnectionEvent, ConnectionEventInner, ConnectionId, DatagramConnectionEvent, EcnCodepoint,
29+
EndpointEvent, EndpointEventInner,
3030
},
3131
token::ResetToken,
3232
transport_parameters::TransportParameters,
@@ -989,13 +989,13 @@ impl Connection {
989989
pub fn handle_event(&mut self, event: ConnectionEvent) {
990990
use self::ConnectionEventInner::*;
991991
match event.0 {
992-
Datagram {
992+
Datagram(DatagramConnectionEvent {
993993
now,
994994
remote,
995995
ecn,
996996
first_decode,
997997
remaining,
998-
} => {
998+
}) => {
999999
// If this packet could initiate a migration and we're a client or a server that
10001000
// forbids migration, drop the datagram. This could be relaxed to heuristically
10011001
// permit NAT-rebinding-like migration.
@@ -3346,11 +3346,11 @@ impl Connection {
33463346
#[cfg(test)]
33473347
pub(crate) fn decode_packet(&self, event: &ConnectionEvent) -> Option<Vec<u8>> {
33483348
let (first_decode, remaining) = match &event.0 {
3349-
ConnectionEventInner::Datagram {
3349+
ConnectionEventInner::Datagram(DatagramConnectionEvent {
33503350
first_decode,
33513351
remaining,
33523352
..
3353-
} => (first_decode, remaining),
3353+
}) => (first_decode, remaining),
33543354
_ => return None,
33553355
};
33563356

0 commit comments

Comments
 (0)