Skip to content

Commit

Permalink
Fix race condition between 0-RTT and Incoming
Browse files Browse the repository at this point in the history
Closes quinn-rs#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.
  • Loading branch information
gretchenfrage committed Apr 14, 2024
1 parent 82a67db commit f6a8c5b
Show file tree
Hide file tree
Showing 5 changed files with 166 additions and 53 deletions.
12 changes: 6 additions & 6 deletions quinn-proto/src/connection/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ use crate::{
packet::{Header, InitialHeader, InitialPacket, LongType, Packet, PartialDecode, SpaceId},
range_set::ArrayRangeSet,
shared::{
ConnectionEvent, ConnectionEventInner, ConnectionId, EcnCodepoint, EndpointEvent,
EndpointEventInner,
ConnectionEvent, ConnectionEventInner, ConnectionId, DatagramConnectionEvent, EcnCodepoint,
EndpointEvent, EndpointEventInner,
},
token::ResetToken,
transport_parameters::TransportParameters,
Expand Down Expand Up @@ -989,13 +989,13 @@ impl Connection {
pub fn handle_event(&mut self, event: ConnectionEvent) {
use self::ConnectionEventInner::*;
match event.0 {
Datagram {
Datagram(DatagramConnectionEvent {
now,
remote,
ecn,
first_decode,
remaining,
} => {
}) => {
// If this packet could initiate a migration and we're a client or a server that
// forbids migration, drop the datagram. This could be relaxed to heuristically
// permit NAT-rebinding-like migration.
Expand Down Expand Up @@ -3346,11 +3346,11 @@ impl Connection {
#[cfg(test)]
pub(crate) fn decode_packet(&self, event: &ConnectionEvent) -> Option<Vec<u8>> {
let (first_decode, remaining) = match &event.0 {
ConnectionEventInner::Datagram {
ConnectionEventInner::Datagram(DatagramConnectionEvent {
first_decode,
remaining,
..
} => (first_decode, remaining),
}) => (first_decode, remaining),
_ => return None,
};

Expand Down
Loading

0 comments on commit f6a8c5b

Please sign in to comment.