From 1dc0b5ed1c25a803b68a8c8b89eaefe0dca78bc6 Mon Sep 17 00:00:00 2001 From: Benjamin Saunders Date: Sat, 27 Apr 2019 16:24:40 -0700 Subject: [PATCH] Allow connection migration to be disabled Allows undemanding applications to reduce their attack surface area. --- quinn-proto/src/connection.rs | 21 ++++++++++++++------- quinn-proto/src/endpoint.rs | 13 +++++++++++-- quinn-proto/src/transport_error.rs | 2 +- quinn-proto/src/transport_parameters.rs | 4 +++- 4 files changed, 29 insertions(+), 11 deletions(-) diff --git a/quinn-proto/src/connection.rs b/quinn-proto/src/connection.rs index 8cce7d4c9..c15033ee2 100644 --- a/quinn-proto/src/connection.rs +++ b/quinn-proto/src/connection.rs @@ -28,8 +28,9 @@ use crate::spaces::{CryptoSpace, PacketSpace, Retransmits, SentPacket}; use crate::stream::{self, FinishError, ReadError, Streams, WriteError}; use crate::transport_parameters::{self, TransportParameters}; use crate::{ - frame, Directionality, EndpointConfig, Frame, Side, StreamId, Transmit, TransportError, - MAX_STREAM_COUNT, MIN_INITIAL_SIZE, MIN_MTU, RESET_TOKEN_SIZE, TIMER_GRANULARITY, + frame, Directionality, EndpointConfig, Frame, ServerConfig, Side, StreamId, Transmit, + TransportError, MAX_STREAM_COUNT, MIN_INITIAL_SIZE, MIN_MTU, RESET_TOKEN_SIZE, + TIMER_GRANULARITY, }; /// Protocol state and logic for a single QUIC connection @@ -41,6 +42,7 @@ use crate::{ pub struct Connection { log: Logger, endpoint_config: Arc, + server_config: Option>, config: Arc, rng: OsRng, tls: TlsSession, @@ -156,6 +158,7 @@ impl Connection { pub(crate) fn new( log: Logger, endpoint_config: Arc, + server_config: Option>, config: Arc, init_cid: ConnectionId, loc_cid: ConnectionId, @@ -184,6 +187,7 @@ impl Connection { let mut this = Self { log, endpoint_config, + server_config, rng, tls, handshake_cid: loc_cid, @@ -1282,7 +1286,7 @@ impl Connection { .crypto .start_session( &client_opts.server_name, - &TransportParameters::new(&self.config), + &TransportParameters::new(&self.config, None), ) .unwrap(); self.discard_space(SpaceId::Initial); // Make sure we clean up after any retransmitted Initials @@ -1832,10 +1836,13 @@ impl Connection { } if remote != self.remote && !is_probing_packet { - debug_assert!( - self.side.is_server(), - "packets from unknown remote should be dropped by clients" - ); + let server_config = self + .server_config + .as_ref() + .expect("packets from unknown remote should be dropped by clients"); + if !server_config.migration { + return Err(TransportError::INVALID_MIGRATION("")); + } self.migrate(now, remote); // Break linkability, if possible if let Some(cid) = self.rem_cids.pop() { diff --git a/quinn-proto/src/endpoint.rs b/quinn-proto/src/endpoint.rs index 8fb8bef9f..cd817f006 100644 --- a/quinn-proto/src/endpoint.rs +++ b/quinn-proto/src/endpoint.rs @@ -401,7 +401,7 @@ impl Endpoint { config, server_name, } => { - let params = TransportParameters::new(&config.transport); + let params = TransportParameters::new(&config.transport, None); ( config.crypto.start_session(&server_name, ¶ms)?, Some(ClientOpts { @@ -416,7 +416,7 @@ impl Endpoint { } ConnectionOpts::Server { orig_dst_cid } => { let config = self.server_config.as_ref().unwrap(); - let params = TransportParameters::new(&config.transport); + let params = TransportParameters::new(&config.transport, Some(config)); let server_params = TransportParameters { stateless_reset_token: Some(reset_token_for(&self.config.reset_key, &loc_cid)), original_connection_id: orig_dst_cid, @@ -437,6 +437,7 @@ impl Endpoint { let conn = Connection::new( log, Arc::clone(&self.config), + self.server_config.as_ref().map(Arc::clone), transport_config, init_cid, loc_cid, @@ -729,6 +730,12 @@ pub struct ServerConfig { /// /// Accepting a connection removes it from the buffer, so this does not need to be large. pub accept_buffer: u32, + + /// Whether to allow clients to migrate to new addresses + /// + /// Improves behavior for clients that move between different internet connections or suffer NAT + /// rebinding. Enabled by default. + pub migration: bool, } impl Default for ServerConfig { @@ -747,6 +754,8 @@ impl Default for ServerConfig { retry_token_lifetime: 15_000_000, accept_buffer: 1024, + + migration: true, } } } diff --git a/quinn-proto/src/transport_error.rs b/quinn-proto/src/transport_error.rs index d4d52b21f..9e4a9b89b 100644 --- a/quinn-proto/src/transport_error.rs +++ b/quinn-proto/src/transport_error.rs @@ -162,5 +162,5 @@ errors! { TRANSPORT_PARAMETER_ERROR(0x8) "received transport parameters that were badly formatted, included an invalid value, was absent even though it is mandatory, was present though it is forbidden, or is otherwise in error"; VERSION_NEGOTIATION_ERROR(0x9) "received transport parameters that contained version negotiation parameters that disagreed with the version negotiation that was performed, constituting a potential version downgrade attack"; PROTOCOL_VIOLATION(0xA) "detected an error with protocol compliance that was not covered by more specific error codes"; - INVALID_MIGRATION(0xC) "received a PATH_RESPONSE frame that did not correspond to any PATH_CHALLENGE frame that it previously sent"; + INVALID_MIGRATION(0xC) "migrated to a different address when the endpoint had disabled migration"; } diff --git a/quinn-proto/src/transport_parameters.rs b/quinn-proto/src/transport_parameters.rs index aaed5c671..30c7cd949 100644 --- a/quinn-proto/src/transport_parameters.rs +++ b/quinn-proto/src/transport_parameters.rs @@ -4,6 +4,7 @@ use bytes::{Buf, BufMut}; use err_derive::Error; use crate::coding::{BufExt, BufMutExt, UnexpectedEnd}; +use crate::endpoint::ServerConfig; use crate::shared::{ConnectionId, ResetToken}; use crate::{ varint, Side, TransportConfig, TransportError, MAX_CID_SIZE, MIN_CID_SIZE, RESET_TOKEN_SIZE, @@ -69,7 +70,7 @@ macro_rules! make_struct { apply_params!(make_struct); impl TransportParameters { - pub fn new(config: &TransportConfig) -> Self { + pub fn new(config: &TransportConfig, server_config: Option<&ServerConfig>) -> Self { TransportParameters { initial_max_streams_bidi: config.stream_window_bidi, initial_max_streams_uni: config.stream_window_uni, @@ -79,6 +80,7 @@ impl TransportParameters { initial_max_stream_data_uni: config.stream_receive_window, idle_timeout: config.idle_timeout, max_ack_delay: 0, // Unimplemented + disable_migration: server_config.map_or(false, |c| !c.migration), ..Self::default() } }