From 9be08130ba8dbfdbd63290a8dc6a515b2cfd5dd1 Mon Sep 17 00:00:00 2001 From: Martin Algesten Date: Mon, 15 Jun 2026 21:57:24 +0200 Subject: [PATCH 1/6] Defer DTLS handshake consumption until parse succeeds Co-Authored-By: Codex --- src/dtls12/message/handshake.rs | 61 +++++++++++++++++++++++++++------ src/dtls13/message/handshake.rs | 39 ++++++++++++++------- 2 files changed, 78 insertions(+), 22 deletions(-) diff --git a/src/dtls12/message/handshake.rs b/src/dtls12/message/handshake.rs index ed555c3f..d17f5d19 100644 --- a/src/dtls12/message/handshake.rs +++ b/src/dtls12/message/handshake.rs @@ -13,6 +13,7 @@ use super::HelloVerifyRequest; use super::ServerHello; use super::ServerKeyExchange; use crate::buffer::Buf; +use arrayvec::ArrayVec; use nom::Err; use nom::IResult; use nom::bytes::complete::take; @@ -20,6 +21,8 @@ use nom::error::{Error, ErrorKind}; use nom::number::complete::be_u8; use nom::number::complete::{be_u16, be_u24}; +const MAX_DEFRAGMENT_HANDSHAKES: usize = 50; + #[derive(Debug, PartialEq, Eq, Default, Clone, Copy)] pub struct Header { pub msg_type: MessageType, @@ -154,8 +157,11 @@ impl Handshake { let Body::Fragment(range) = &first_handshake.body else { unreachable!("Non-Fragment body in defragment()") }; + let mut handled = ArrayVec::<&Handshake, MAX_DEFRAGMENT_HANDSHAKES>::new(); + handled + .try_push(first_handshake) + .map_err(|_| crate::InternalError::too_many_records())?; buffer.extend_from_slice(&first_buffer[range.clone()]); - first_handshake.set_handled(); for (handshake, source_buf) in iter { if handshake.header.msg_type != first_handshake.header.msg_type { @@ -166,7 +172,9 @@ impl Handshake { unreachable!("Non-Fragment body in defragment()") }; - handshake.handled.store(true, Ordering::Relaxed); + handled + .try_push(handshake) + .map_err(|_| crate::InternalError::too_many_records())?; buffer.extend_from_slice(&source_buf[range.clone()]); } @@ -176,7 +184,18 @@ impl Handshake { return Err(crate::InternalError::parse_incomplete()); } - // If transcript is provided, write the handshake header + body before parsing + let (rest, body) = Body::parse(buffer, 0, first_handshake.header.msg_type, cipher_suite)?; + + if !rest.is_empty() && first_handshake.header.msg_type == MessageType::Finished { + debug!("Defragmentation failed. Body::parse() did not consume the entire buffer"); + return Err(crate::InternalError::parse_incomplete()); + } + + for handshake in handled { + handshake.set_handled(); + } + + // If transcript is provided, write the handshake header + body after parsing succeeds. if let Some(transcript) = transcript { transcript.push(first_handshake.header.msg_type.as_u8()); transcript.extend_from_slice(&first_handshake.header.length.to_be_bytes()[1..]); @@ -187,13 +206,6 @@ impl Handshake { transcript.extend_from_slice(&buffer[..first_handshake.header.length as usize]); } - let (rest, body) = Body::parse(buffer, 0, first_handshake.header.msg_type, cipher_suite)?; - - if !rest.is_empty() && first_handshake.header.msg_type == MessageType::Finished { - debug!("Defragmentation failed. Body::parse() did not consume the entire buffer"); - return Err(crate::InternalError::parse_incomplete()); - } - let handshake = Handshake { header: Header { msg_type: first_handshake.header.msg_type, @@ -695,4 +707,33 @@ mod tests { assert!(rest.is_empty()); } + + #[test] + fn failed_defragment_parse_does_not_mark_handshake_or_write_transcript() { + let mut body = MESSAGE[12..].to_vec(); + body[38] = 0; + body[39] = 3; + + let handshake = Handshake::new( + MessageType::ClientHello, + body.len() as u32, + 0, + 0, + body.len() as u32, + Body::Fragment(0..body.len()), + ); + + let mut defragmented_buffer = Buf::new(); + let mut transcript = Buf::new(); + let result = Handshake::defragment( + std::iter::once((&handshake, body.as_slice())), + &mut defragmented_buffer, + None, + Some(&mut transcript), + ); + + assert!(result.is_err()); + assert!(!handshake.is_handled()); + assert!(transcript.is_empty()); + } } diff --git a/src/dtls13/message/handshake.rs b/src/dtls13/message/handshake.rs index 64d3d947..c5053a93 100644 --- a/src/dtls13/message/handshake.rs +++ b/src/dtls13/message/handshake.rs @@ -5,6 +5,7 @@ use std::sync::atomic::{AtomicBool, Ordering}; use super::{Certificate, CertificateVerify, ClientHello, Dtls13CipherSuite}; use super::{EncryptedExtensions, Finished, ServerHello}; use crate::buffer::Buf; +use arrayvec::ArrayVec; use nom::Err; use nom::IResult; use nom::bytes::complete::take; @@ -12,6 +13,8 @@ use nom::error::{Error, ErrorKind}; use nom::number::complete::be_u8; use nom::number::complete::{be_u16, be_u24}; +const MAX_DEFRAGMENT_HANDSHAKES: usize = 50; + #[derive(Debug, PartialEq, Eq, Default, Clone, Copy)] pub struct Header { pub msg_type: MessageType, @@ -165,8 +168,11 @@ impl Handshake { let Body::Fragment(range) = &first_handshake.body else { unreachable!("Non-Fragment body in defragment()") }; + let mut handled = ArrayVec::<&Handshake, MAX_DEFRAGMENT_HANDSHAKES>::new(); + handled + .try_push(first_handshake) + .map_err(|_| crate::InternalError::too_many_records())?; buffer.extend_from_slice(&first_buffer[range.clone()]); - first_handshake.set_handled(); let mut assembled_end = first_handshake.header.fragment_offset + first_handshake.header.fragment_length; @@ -180,7 +186,9 @@ impl Handshake { unreachable!("Non-Fragment body in defragment()") }; - handshake.handled.store(true, Ordering::Relaxed); + handled + .try_push(handshake) + .map_err(|_| crate::InternalError::too_many_records())?; // Handle overlapping fragment data: skip bytes already assembled let frag_start = handshake.header.fragment_offset as usize; @@ -200,15 +208,6 @@ impl Handshake { return Err(crate::InternalError::parse_incomplete()); } - // If transcript is provided, write the TLS 1.3-style header + body. - // Per RFC 9147 Section 5.2, the transcript uses msg_type(1) + length(3) - // WITHOUT the DTLS-specific message_seq, fragment_offset, fragment_length. - if let Some(transcript) = transcript { - transcript.push(first_handshake.header.msg_type.as_u8()); - transcript.extend_from_slice(&first_handshake.header.length.to_be_bytes()[1..]); - transcript.extend_from_slice(&buffer[..first_handshake.header.length as usize]); - } - let (rest, body) = if allow_unknown_client_hello_suites { Body::parse_allow_unknown_client_hello_suites( buffer, @@ -225,6 +224,19 @@ impl Handshake { return Err(crate::InternalError::parse_incomplete()); } + for handshake in handled { + handshake.set_handled(); + } + + // If transcript is provided, write the TLS 1.3-style header + body after parsing succeeds. + // Per RFC 9147 Section 5.2, the transcript uses msg_type(1) + length(3) + // WITHOUT the DTLS-specific message_seq, fragment_offset, fragment_length. + if let Some(transcript) = transcript { + transcript.push(first_handshake.header.msg_type.as_u8()); + transcript.extend_from_slice(&first_handshake.header.length.to_be_bytes()[1..]); + transcript.extend_from_slice(&buffer[..first_handshake.header.length as usize]); + } + let handshake = Handshake { header: Header { msg_type: first_handshake.header.msg_type, @@ -620,16 +632,19 @@ mod tests { ); let mut buffer = Buf::new(); + let mut transcript = Buf::new(); let result = Handshake::defragment( std::iter::once((&handshake, source.as_slice())), &mut buffer, None, - None, + Some(&mut transcript), ); assert!( result.is_err(), "KeyUpdate bodies with trailing bytes must be rejected" ); + assert!(!handshake.is_handled()); + assert!(transcript.is_empty()); } } From af9302fa192a5cda0d4e722688326de4cd0a65f2 Mon Sep 17 00:00:00 2001 From: Martin Algesten Date: Sun, 21 Jun 2026 13:14:35 +0200 Subject: [PATCH 2/6] Document handshake extension validation boundary Co-Authored-By: Codex --- src/dtls12/message/handshake.rs | 6 ++++++ src/dtls13/message/handshake.rs | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/src/dtls12/message/handshake.rs b/src/dtls12/message/handshake.rs index d17f5d19..bc44352e 100644 --- a/src/dtls12/message/handshake.rs +++ b/src/dtls12/message/handshake.rs @@ -191,6 +191,12 @@ impl Handshake { return Err(crate::InternalError::parse_incomplete()); } + // Intentional boundary: Body::parse validates the handshake body shape and + // extension envelopes, but known extension payloads remain validated by the + // client/server state handlers. A transiently corrupted UDP datagram whose + // extension payload fails later may therefore have been consumed here; that + // recovery edge is accepted to keep this path parser-only and avoid the + // broader transaction/rollback machinery. for handshake in handled { handshake.set_handled(); } diff --git a/src/dtls13/message/handshake.rs b/src/dtls13/message/handshake.rs index c5053a93..89967251 100644 --- a/src/dtls13/message/handshake.rs +++ b/src/dtls13/message/handshake.rs @@ -224,6 +224,12 @@ impl Handshake { return Err(crate::InternalError::parse_incomplete()); } + // Intentional boundary: Body::parse validates the handshake body shape and + // extension envelopes, but known extension payloads remain validated by the + // client/server state handlers. A transiently corrupted UDP datagram whose + // extension payload fails later may therefore have been consumed here; that + // recovery edge is accepted to keep this path parser-only and avoid the + // broader transaction/rollback machinery. for handshake in handled { handshake.set_handled(); } From 82d070327a06ae00edcf931f16c05e7049ac0649 Mon Sep 17 00:00:00 2001 From: Martin Algesten Date: Sun, 21 Jun 2026 13:17:19 +0200 Subject: [PATCH 3/6] Reject trailing bytes in known handshake bodies Co-Authored-By: Codex --- src/dtls12/message/handshake.rs | 37 ++++++++++++++++++++++++++++++++- src/dtls13/message/handshake.rs | 37 ++++++++++++++++++++++++++++++++- 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/src/dtls12/message/handshake.rs b/src/dtls12/message/handshake.rs index bc44352e..da03591b 100644 --- a/src/dtls12/message/handshake.rs +++ b/src/dtls12/message/handshake.rs @@ -186,7 +186,12 @@ impl Handshake { let (rest, body) = Body::parse(buffer, 0, first_handshake.header.msg_type, cipher_suite)?; - if !rest.is_empty() && first_handshake.header.msg_type == MessageType::Finished { + if !rest.is_empty() + && first_handshake + .header + .msg_type + .rejects_trailing_body_bytes() + { debug!("Defragmentation failed. Body::parse() did not consume the entire buffer"); return Err(crate::InternalError::parse_incomplete()); } @@ -336,6 +341,10 @@ impl MessageType { !matches!(*self, Self(0..=4 | 11..=16 | 20)) } + const fn rejects_trailing_body_bytes(&self) -> bool { + !self.is_unknown() + } + pub fn parse(input: &[u8]) -> IResult<&[u8], MessageType> { let (input, byte) = be_u8(input)?; Ok((input, Self::from_u8(byte))) @@ -742,4 +751,30 @@ mod tests { assert!(!handshake.is_handled()); assert!(transcript.is_empty()); } + + #[test] + fn known_body_rejects_trailing_bytes() { + let body = [0]; + let handshake = Handshake::new( + MessageType::HelloRequest, + body.len() as u32, + 0, + 0, + body.len() as u32, + Body::Fragment(0..body.len()), + ); + + let mut defragmented_buffer = Buf::new(); + let mut transcript = Buf::new(); + let result = Handshake::defragment( + std::iter::once((&handshake, body.as_slice())), + &mut defragmented_buffer, + None, + Some(&mut transcript), + ); + + assert!(result.is_err()); + assert!(!handshake.is_handled()); + assert!(transcript.is_empty()); + } } diff --git a/src/dtls13/message/handshake.rs b/src/dtls13/message/handshake.rs index 89967251..271f4b21 100644 --- a/src/dtls13/message/handshake.rs +++ b/src/dtls13/message/handshake.rs @@ -219,7 +219,12 @@ impl Handshake { Body::parse(buffer, 0, first_handshake.header.msg_type, cipher_suite)? }; - if !rest.is_empty() && first_handshake.header.msg_type == MessageType::Finished { + if !rest.is_empty() + && first_handshake + .header + .msg_type + .rejects_trailing_body_bytes() + { debug!("Defragmentation failed. Body::parse() did not consume the entire buffer"); return Err(crate::InternalError::parse_incomplete()); } @@ -313,6 +318,10 @@ impl MessageType { !matches!(*self, Self(1..=2 | 8 | 11 | 13 | 15 | 20 | 24)) } + const fn rejects_trailing_body_bytes(&self) -> bool { + !self.is_unknown() + } + pub fn parse(input: &[u8]) -> IResult<&[u8], MessageType> { let (input, byte) = be_u8(input)?; Ok((input, Self::from_u8(byte))) @@ -653,4 +662,30 @@ mod tests { assert!(!handshake.is_handled()); assert!(transcript.is_empty()); } + + #[test] + fn known_body_rejects_trailing_bytes() { + let source = [0, 0, 0]; + let handshake = Handshake::new( + MessageType::EncryptedExtensions, + source.len() as u32, + 0, + 0, + source.len() as u32, + Body::Fragment(0..source.len()), + ); + + let mut buffer = Buf::new(); + let mut transcript = Buf::new(); + let result = Handshake::defragment( + std::iter::once((&handshake, source.as_slice())), + &mut buffer, + None, + Some(&mut transcript), + ); + + assert!(result.is_err()); + assert!(!handshake.is_handled()); + assert!(transcript.is_empty()); + } } From f448720826bbf629544b0ede292d1267ac2a405b Mon Sep 17 00:00:00 2001 From: Martin Algesten Date: Sun, 21 Jun 2026 13:24:26 +0200 Subject: [PATCH 4/6] Discard failed handshake defragment candidates Co-Authored-By: Codex --- src/dtls12/message/handshake.rs | 26 +++++++++++++++++++------- src/dtls13/message/handshake.rs | 33 +++++++++++++++++++++++++-------- 2 files changed, 44 insertions(+), 15 deletions(-) diff --git a/src/dtls12/message/handshake.rs b/src/dtls12/message/handshake.rs index da03591b..0081c287 100644 --- a/src/dtls12/message/handshake.rs +++ b/src/dtls12/message/handshake.rs @@ -184,7 +184,14 @@ impl Handshake { return Err(crate::InternalError::parse_incomplete()); } - let (rest, body) = Body::parse(buffer, 0, first_handshake.header.msg_type, cipher_suite)?; + let (rest, body) = + match Body::parse(buffer, 0, first_handshake.header.msg_type, cipher_suite) { + Ok(parsed) => parsed, + Err(err) => { + mark_handled(handled); + return Err(err.into()); + } + }; if !rest.is_empty() && first_handshake @@ -193,6 +200,7 @@ impl Handshake { .rejects_trailing_body_bytes() { debug!("Defragmentation failed. Body::parse() did not consume the entire buffer"); + mark_handled(handled); return Err(crate::InternalError::parse_incomplete()); } @@ -202,9 +210,7 @@ impl Handshake { // extension payload fails later may therefore have been consumed here; that // recovery edge is accepted to keep this path parser-only and avoid the // broader transaction/rollback machinery. - for handshake in handled { - handshake.set_handled(); - } + mark_handled(handled); // If transcript is provided, write the handshake header + body after parsing succeeds. if let Some(transcript) = transcript { @@ -310,6 +316,12 @@ impl Handshake { } } +fn mark_handled(handled: ArrayVec<&Handshake, MAX_DEFRAGMENT_HANDSHAKES>) { + for handshake in handled { + handshake.set_handled(); + } +} + #[repr(transparent)] #[derive(Clone, Copy, Default, PartialEq, Eq, Hash)] pub struct MessageType(u8); @@ -724,7 +736,7 @@ mod tests { } #[test] - fn failed_defragment_parse_does_not_mark_handshake_or_write_transcript() { + fn failed_defragment_parse_discards_candidate_without_writing_transcript() { let mut body = MESSAGE[12..].to_vec(); body[38] = 0; body[39] = 3; @@ -748,7 +760,7 @@ mod tests { ); assert!(result.is_err()); - assert!(!handshake.is_handled()); + assert!(handshake.is_handled()); assert!(transcript.is_empty()); } @@ -774,7 +786,7 @@ mod tests { ); assert!(result.is_err()); - assert!(!handshake.is_handled()); + assert!(handshake.is_handled()); assert!(transcript.is_empty()); } } diff --git a/src/dtls13/message/handshake.rs b/src/dtls13/message/handshake.rs index 271f4b21..ac284a7a 100644 --- a/src/dtls13/message/handshake.rs +++ b/src/dtls13/message/handshake.rs @@ -209,14 +209,26 @@ impl Handshake { } let (rest, body) = if allow_unknown_client_hello_suites { - Body::parse_allow_unknown_client_hello_suites( + match Body::parse_allow_unknown_client_hello_suites( buffer, 0, first_handshake.header.msg_type, cipher_suite, - )? + ) { + Ok(parsed) => parsed, + Err(err) => { + mark_handled(handled); + return Err(err.into()); + } + } } else { - Body::parse(buffer, 0, first_handshake.header.msg_type, cipher_suite)? + match Body::parse(buffer, 0, first_handshake.header.msg_type, cipher_suite) { + Ok(parsed) => parsed, + Err(err) => { + mark_handled(handled); + return Err(err.into()); + } + } }; if !rest.is_empty() @@ -226,6 +238,7 @@ impl Handshake { .rejects_trailing_body_bytes() { debug!("Defragmentation failed. Body::parse() did not consume the entire buffer"); + mark_handled(handled); return Err(crate::InternalError::parse_incomplete()); } @@ -235,9 +248,7 @@ impl Handshake { // extension payload fails later may therefore have been consumed here; that // recovery edge is accepted to keep this path parser-only and avoid the // broader transaction/rollback machinery. - for handshake in handled { - handshake.set_handled(); - } + mark_handled(handled); // If transcript is provided, write the TLS 1.3-style header + body after parsing succeeds. // Per RFC 9147 Section 5.2, the transcript uses msg_type(1) + length(3) @@ -291,6 +302,12 @@ impl Handshake { } } +fn mark_handled(handled: ArrayVec<&Handshake, MAX_DEFRAGMENT_HANDSHAKES>) { + for handshake in handled { + handshake.set_handled(); + } +} + #[repr(transparent)] #[derive(Clone, Copy, Default, PartialEq, Eq, Hash)] pub struct MessageType(u8); @@ -659,7 +676,7 @@ mod tests { result.is_err(), "KeyUpdate bodies with trailing bytes must be rejected" ); - assert!(!handshake.is_handled()); + assert!(handshake.is_handled()); assert!(transcript.is_empty()); } @@ -685,7 +702,7 @@ mod tests { ); assert!(result.is_err()); - assert!(!handshake.is_handled()); + assert!(handshake.is_handled()); assert!(transcript.is_empty()); } } From c0ead3ce03dfd8f64817794745e9092cc0c4c836 Mon Sep 17 00:00:00 2001 From: Martin Algesten Date: Sun, 21 Jun 2026 13:28:46 +0200 Subject: [PATCH 5/6] Document defragmentation fragment cap Co-Authored-By: Codex --- src/dtls12/message/handshake.rs | 4 ++++ src/dtls13/message/handshake.rs | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/src/dtls12/message/handshake.rs b/src/dtls12/message/handshake.rs index 0081c287..ae6a2725 100644 --- a/src/dtls12/message/handshake.rs +++ b/src/dtls12/message/handshake.rs @@ -21,6 +21,10 @@ use nom::error::{Error, ErrorKind}; use nom::number::complete::be_u8; use nom::number::complete::{be_u16, be_u24}; +// Defensive stack cap over flattened handshake fragments selected for one +// defragmentation attempt. This intentionally does not mirror the receive +// queue's record-count cap; exceeding 50 fragments for one handshake implies +// pathologically tiny records and is treated as invalid input. const MAX_DEFRAGMENT_HANDSHAKES: usize = 50; #[derive(Debug, PartialEq, Eq, Default, Clone, Copy)] diff --git a/src/dtls13/message/handshake.rs b/src/dtls13/message/handshake.rs index ac284a7a..1ddfb1cc 100644 --- a/src/dtls13/message/handshake.rs +++ b/src/dtls13/message/handshake.rs @@ -13,6 +13,10 @@ use nom::error::{Error, ErrorKind}; use nom::number::complete::be_u8; use nom::number::complete::{be_u16, be_u24}; +// Defensive stack cap over flattened handshake fragments selected for one +// defragmentation attempt. This intentionally does not mirror the receive +// queue's record-count cap; exceeding 50 fragments for one handshake implies +// pathologically tiny records and is treated as invalid input. const MAX_DEFRAGMENT_HANDSHAKES: usize = 50; #[derive(Debug, PartialEq, Eq, Default, Clone, Copy)] From 5a2db0c15b2f9c1d1688ba2fab7963a4000fdc5d Mon Sep 17 00:00:00 2001 From: Martin Algesten Date: Sun, 21 Jun 2026 15:11:50 +0200 Subject: [PATCH 6/6] docs: add changelog entry for handshake parse fix Co-Authored-By: Codex --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6dba45b6..b35cc0e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ # Unreleased * Fix DTLS 1.3 RFC 9147 conformance issues #147 + * Reject malformed fragmented DTLS handshakes before consuming fragments #144 * Represent DTLS wire-code identifiers as compact newtypes (breaking) #137 * Make public errors structured and fatal-only (breaking) #134