From ac2287e6c19ced3b3cb31d8fc1a2afb7ad75aa18 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Mon, 12 Nov 2018 14:53:19 +0100 Subject: [PATCH] Implement new-style variable length packet number encoding --- quinn-proto/src/connection.rs | 44 +++++++----- quinn-proto/src/endpoint.rs | 28 +++----- quinn-proto/src/packet.rs | 123 ++++++++++++++++++++++++---------- 3 files changed, 124 insertions(+), 71 deletions(-) diff --git a/quinn-proto/src/connection.rs b/quinn-proto/src/connection.rs index d42b6fe6a..d4d818b97 100644 --- a/quinn-proto/src/connection.rs +++ b/quinn-proto/src/connection.rs @@ -5,7 +5,6 @@ use std::{cmp, io, mem}; use bytes::{Buf, Bytes, BytesMut}; use fnv::{FnvHashMap, FnvHashSet}; -use rand::distributions::Distribution; use slog::Logger; use coding::{BufExt, BufMutExt}; @@ -346,7 +345,6 @@ impl Connection { loc_cid: ConnectionId, rem_cid: ConnectionId, remote: SocketAddrV6, - initial_packet_number: u64, client_config: Option, tls: TlsSession, ctx: &mut Context, @@ -427,7 +425,7 @@ impl Connection { largest_sent_before_rto: 0, time_of_last_sent_retransmittable_packet: 0, time_of_last_sent_handshake_packet: 0, - largest_sent_packet: initial_packet_number.overflowing_sub(1).0, + largest_sent_packet: 0, largest_acked_packet: 0, sent_packets: BTreeMap::new(), @@ -1203,13 +1201,17 @@ impl Connection { ctx.incoming_handshakes -= 1; } let n = self.get_tx_number(); + debug_assert!(n < 64); // handshake_close doesn't have the connection state + // to decide on packet number encoding length; since this + // is about closing the handshake, it seems reasonable to + // assume that the packet number will fit in one byte. ctx.io.push_back(Io::Transmit { destination: remote, packet: handshake_close( &self.handshake_crypto, &self.rem_cid, &self.loc_cid, - n as u32, + n as u8, state.reason.clone(), state.alert.as_ref().map(|x| &x[..]), ), @@ -1250,7 +1252,7 @@ impl Connection { // Received Retry as a server debug!(self.log, "received retry from client"); Err(TransportError::PROTOCOL_VIOLATION.into()) - } else if state.clienthello_packet.unwrap() as u64 > number { + } else if state.clienthello_packet.unwrap() > number { // Retry corresponds to an outdated Initial; must be a duplicate, so // ignore it Ok(State::Handshake(state)) @@ -1268,7 +1270,6 @@ impl Connection { self.loc_cid, rem_cid, remote, - ctx.initial_packet_number.sample(&mut ctx.rng), self.client_config.clone(), tls, ctx, @@ -1904,21 +1905,21 @@ impl Connection { { if let State::Handshake(ref mut state) = self.state.as_mut().unwrap() { if state.clienthello_packet.is_none() { - state.clienthello_packet = Some(number as u32); + state.clienthello_packet = Some(number); } } Header::Initial { src_cid: self.loc_cid, dst_cid: self.rem_cid, token: vec![], // TODO: determine what's needed here - number: number as u32, + number: PacketNumber::new(number, self.largest_acked_packet), } } else { Header::Long { ty: LongType::Handshake, src_cid: self.loc_cid, dst_cid: self.rem_cid, - number: number as u32, + number: PacketNumber::new(number, self.largest_acked_packet), } }; ( @@ -2148,7 +2149,11 @@ impl Connection { } } if !crypto.is_1rtt() { - set_payload_length(&mut buf, header_len as usize); + let pn_len = match header { + Header::Initial { number, .. } | Header::Long { number, .. } => number.len(), + _ => panic!("invalid header for packet payload length"), + }; + set_payload_length(&mut buf, header_len as usize, pn_len); } crypto.encrypt(number, &mut buf, header_len as usize); (number, acks, ack_only, crypto.is_initial()) @@ -2519,7 +2524,7 @@ impl Connection { (key_phase, number) } Header::Initial { number, .. } | Header::Long { number, .. } if handshake => { - (false, PacketNumber::U32(number)) + (false, number) } _ => { return Err(None); @@ -2677,20 +2682,23 @@ pub fn handshake_close( crypto: &Crypto, remote_id: &ConnectionId, local_id: &ConnectionId, - packet_number: u32, + packet_number: u8, reason: R, tls_alert: Option<&[u8]>, ) -> Box<[u8]> where R: Into, { - let mut buf = Vec::::new(); - Header::Long { + let number = PacketNumber::U8(packet_number); + let header = Header::Long { ty: LongType::Handshake, dst_cid: *remote_id, src_cid: *local_id, - number: packet_number, - }.encode(&mut buf); + number, + }; + + let mut buf = Vec::::new(); + header.encode(&mut buf); let header_len = buf.len(); let max_len = MIN_MTU - header_len as u16 - AEAD_TAG_SIZE as u16; match reason.into() { @@ -2707,7 +2715,7 @@ where }.encode(false, &mut buf); } } - set_payload_length(&mut buf, header_len); + set_payload_length(&mut buf, header_len, number.len()); crypto.encrypt(packet_number as u64, &mut buf, header_len); buf.into() } @@ -2855,7 +2863,7 @@ pub mod state { pub struct Handshake { /// The number of the packet that first contained the latest version of the TLS /// ClientHello. Present iff we're the client. - pub clienthello_packet: Option, + pub clienthello_packet: Option, pub rem_cid_set: bool, pub token: Option, } diff --git a/quinn-proto/src/endpoint.rs b/quinn-proto/src/endpoint.rs index d2647beaf..abfa3a480 100644 --- a/quinn-proto/src/endpoint.rs +++ b/quinn-proto/src/endpoint.rs @@ -5,8 +5,7 @@ use std::{cmp, io}; use bytes::{Bytes, BytesMut}; use fnv::{FnvHashMap, FnvHashSet}; -use rand::distributions::Distribution; -use rand::{distributions, rngs::OsRng, Rng, RngCore}; +use rand::{rngs::OsRng, Rng, RngCore}; use ring::digest; use ring::hmac::SigningKey; use slab::Slab; @@ -18,7 +17,10 @@ use connection::{ ReadError, State, WriteError, }; use crypto::{self, reset_token_for, ConnectError, Crypto, ServerConfig}; -use packet::{ConnectionId, Header, Packet, PacketDecodeError, PacketNumber, PartialDecode}; +use packet::{ + ConnectionId, Header, Packet, PacketDecodeError, PacketNumber, PartialDecode, + PACKET_NUMBER_32_MASK, +}; use { Directionality, Side, StreamId, TransportError, MAX_CID_SIZE, MIN_CID_SIZE, MIN_INITIAL_SIZE, RESET_TOKEN_SIZE, VERSION, @@ -153,16 +155,9 @@ pub struct Context { pub incoming_handshakes: usize, pub dirty_conns: FnvHashSet, pub readable_conns: FnvHashSet, - pub initial_packet_number: distributions::Uniform, pub listen_keys: Option, } -impl Context { - fn gen_initial_packet_num(&mut self) -> u32 { - self.initial_packet_number.sample(&mut self.rng) as u32 - } -} - /// Information that should be preserved between restarts for server endpoints. /// /// Keeping this around allows better behavior by clients that communicated with a previous @@ -229,7 +224,6 @@ impl Endpoint { config, io: VecDeque::new(), // session_ticket_buffer, - initial_packet_number: distributions::Uniform::from(0..2u64.pow(32) - 1024), events: VecDeque::new(), dirty_conns: FnvHashSet::default(), readable_conns: FnvHashSet::default(), @@ -418,9 +412,10 @@ impl Endpoint { ).saturating_sub(RESET_TOKEN_SIZE), ); buf.reserve_exact(header_len + padding + RESET_TOKEN_SIZE); + let number = self.ctx.rng.gen::() & PACKET_NUMBER_32_MASK | 0x4000; Header::Short { dst_cid: ConnectionId::random(&mut self.ctx.rng, MAX_CID_SIZE), - number: PacketNumber::U8(self.ctx.rng.gen()), + number: PacketNumber::U32(number), key_phase: false, }.encode(&mut buf); { @@ -485,7 +480,6 @@ impl Endpoint { client_config: Option, ) -> ConnectionHandle { debug_assert!(!local_id.is_empty()); - let packet_num = self.ctx.gen_initial_packet_num(); let conn = { let entry = self.connections.vacant_entry(); let conn = ConnectionHandle(entry.key()); @@ -497,7 +491,6 @@ impl Endpoint { local_id, remote_id, remote, - packet_num.into(), client_config, tls, &mut self.ctx, @@ -527,6 +520,7 @@ impl Endpoint { } => (src_cid, dst_cid, number), _ => panic!("non-initial packet in handle_initial()"), }; + let packet_number = packet_number.expand(0); if crypto .decrypt(packet_number as u64, &header_data, &mut payload) @@ -541,14 +535,13 @@ impl Endpoint { == self.ctx.config.accept_buffer as usize { debug!(self.log, "rejecting connection due to full accept buffer"); - let n = self.ctx.gen_initial_packet_num(); self.ctx.io.push_back(Io::Transmit { destination: remote, packet: handshake_close( &crypto, &src_cid, &loc_cid, - n, + 0, TransportError::SERVER_BUSY, None, ), @@ -567,14 +560,13 @@ impl Endpoint { Ok(()) => {} Err(e) => { debug!(self.log, "handshake failed"; "reason" => %e); - let n = self.ctx.gen_initial_packet_num(); self.ctx.io.push_back(Io::Transmit { destination: remote, packet: handshake_close( &crypto, &src_cid, &loc_cid, - n, + 0, TransportError::TLS_HANDSHAKE_FAILED, None, ), diff --git a/quinn-proto/src/packet.rs b/quinn-proto/src/packet.rs index 7b68c99de..045baa63b 100644 --- a/quinn-proto/src/packet.rs +++ b/quinn-proto/src/packet.rs @@ -4,7 +4,7 @@ use bytes::{BigEndian, Buf, BufMut, ByteOrder, Bytes, BytesMut}; use rand::Rng; use slog; -use coding::{self, BufExt, BufMutExt}; +use coding::{self, BufExt, BufMutExt, Codec}; use {MAX_CID_SIZE, MIN_CID_SIZE, VERSION}; // Due to packet number encryption, it is impossible to fully decode a header @@ -64,14 +64,7 @@ impl PartialDecode { let (payload_len, header, allow_coalesced) = match invariant_header { InvariantHeader::Short { first, dst_cid } => { let key_phase = first & KEY_PHASE_BIT != 0; - let number = match first & 0b11 { - 0x0 => PacketNumber::U8(buf.get()?), - 0x1 => PacketNumber::U16(buf.get()?), - 0x2 => PacketNumber::U32(buf.get()?), - _ => { - return Err(PacketDecodeError::InvalidHeader("unknown packet type")); - } - }; + let number = PacketNumber::decode(&mut buf)?; ( buf.remaining(), Header::Short { @@ -124,7 +117,7 @@ impl PartialDecode { buf.copy_to_slice(&mut token); let len = buf.get_var()?; - let number = buf.get()?; + let number = PacketNumber::decode(&mut buf)?; ( len as usize, Header::Initial { @@ -138,7 +131,7 @@ impl PartialDecode { } PacketType::Long(ty) => { let len = buf.get_var()?; - let number = buf.get()?; + let number = PacketNumber::decode(&mut buf)?; ( len as usize, Header::Long { @@ -190,13 +183,13 @@ pub enum Header { src_cid: ConnectionId, dst_cid: ConnectionId, token: Vec, - number: u32, + number: PacketNumber, }, Long { ty: LongType, src_cid: ConnectionId, dst_cid: ConnectionId, - number: u32, + number: PacketNumber, }, Retry { src_cid: ConnectionId, @@ -231,7 +224,7 @@ impl Header { w.write_var(token.len() as u64); w.put_slice(token); w.write::(0); // Placeholder for payload length; see `set_payload_length` - w.write(number); + number.encode(w); } Long { ty, @@ -243,7 +236,7 @@ impl Header { w.write(VERSION); Self::encode_cids(w, dst_cid, src_cid); w.write::(0); // Placeholder for payload length; see `set_payload_length` - w.write(number); + number.encode(w); } Retry { ref src_cid, @@ -261,8 +254,7 @@ impl Header { number, key_phase, } => { - let ty = number.ty() | 0x30 | if key_phase { KEY_PHASE_BIT } else { 0 }; - w.write(ty); + w.write(0x30 | if key_phase { KEY_PHASE_BIT } else { 0 }); w.put_slice(dst_cid); number.encode(w); } @@ -369,7 +361,7 @@ impl InvariantHeader { } // An encoded packet number -#[derive(Debug, Copy, Clone)] +#[derive(Debug, Copy, Clone, Eq, PartialEq)] pub enum PacketNumber { U8(u8), U16(u16), @@ -378,9 +370,6 @@ pub enum PacketNumber { impl PacketNumber { pub fn new(n: u64, largest_acked: u64) -> Self { - if largest_acked == 0 { - return PacketNumber::U32(n as u32); - } let range = (n - largest_acked) / 2; if range < 1 << 8 { PacketNumber::U8(n as u8) @@ -393,21 +382,60 @@ impl PacketNumber { } } - fn ty(self) -> u8 { + pub fn len(self) -> usize { use self::PacketNumber::*; match self { - U8(_) => 0x00, - U16(_) => 0x01, - U32(_) => 0x02, + U8(_) => 1, + U16(_) => 2, + U32(_) => 4, } } pub fn encode(self, w: &mut W) { use self::PacketNumber::*; match self { - U8(x) => w.write(x), - U16(x) => w.write(x), - U32(x) => w.write(x), + U8(x) => { + debug_assert!(x < 128); + w.write(x) + } + U16(x) => { + debug_assert!(x >= 128); + debug_assert!(x < 16384); + w.write(x | 0x8000) + } + U32(x) => { + debug_assert!(x >= 16384); + debug_assert!(x < 1073741824); + w.write(x | 0xc0000000) + } + } + } + + pub fn decode(r: &mut R) -> Result { + use self::PacketNumber::*; + if r.remaining() < 1 { + return Err(coding::UnexpectedEnd.into()); + } + + let first = r.bytes()[0]; + let len = if first < 0x80 { + 1 + } else if first < 0xc0 { + 2 + } else { + 4 + }; + if r.remaining() < len { + return Err(coding::UnexpectedEnd.into()); + } + + match len { + 1 => Ok(U8(r.get()?)), + 2 => Ok(U16(u16::decode(r)? & PACKET_NUMBER_16_MASK)), + 4 => Ok(U32(u32::decode(r)? & PACKET_NUMBER_32_MASK)), + _ => Err(PacketDecodeError::InvalidHeader( + "unable to decode packet number", + )), } } @@ -415,11 +443,7 @@ impl PacketNumber { use self::PacketNumber::*; let t = prev + 1; // Compute missing bits that minimize the difference from expected - let d = match self { - U8(_) => 1 << 8, - U16(_) => 1 << 16, - U32(_) => 1 << 32, - }; + let d = 1 << (8 * self.len()); let x = match self { U8(x) => x as u64, U16(x) => x as u64, @@ -595,12 +619,41 @@ impl slog::Value for ConnectionId { } } -pub fn set_payload_length(packet: &mut [u8], header_len: usize) { +pub fn set_payload_length(packet: &mut [u8], header_len: usize, pn_len: usize) { let len = packet.len() - header_len + AEAD_TAG_SIZE; assert!(len < 2usize.pow(14)); // Fits in reserved space - BigEndian::write_u16(&mut packet[header_len - 6..], len as u16 | 0b01 << 14); + BigEndian::write_u16( + &mut packet[header_len - pn_len - 2..], + len as u16 | 0b01 << 14, + ); } pub const AEAD_TAG_SIZE: usize = 16; +pub const PACKET_NUMBER_16_MASK: u16 = 0x3fff; +pub const PACKET_NUMBER_32_MASK: u32 = 0x3fffffff; + const LONG_HEADER_FORM: u8 = 0x80; const KEY_PHASE_BIT: u8 = 0x40; + +#[cfg(test)] +mod tests { + use super::PacketNumber; + use std::io; + + fn check_pn(typed: PacketNumber, encoded: &[u8]) { + let mut buf = Vec::new(); + typed.encode(&mut buf); + assert_eq!(&buf[..encoded.len()], encoded); + let decoded = PacketNumber::decode(&mut io::Cursor::new(&buf)).unwrap(); + assert_eq!(typed, decoded); + } + + #[test] + fn roundtrip_packet_numbers() { + check_pn(PacketNumber::U8(127), &[0x7f]); + check_pn(PacketNumber::U16(128), &[0x80, 0x80]); + check_pn(PacketNumber::U16(16383), &[0xbf, 0xff]); + check_pn(PacketNumber::U32(16384), &[0xc0, 0x00, 0x40, 0x00]); + check_pn(PacketNumber::U32(1073741823), &[0xff, 0xff, 0xff, 0xff]); + } +}