From 42d638b11636aa43e2e5bc912096d13b4ccc24e0 Mon Sep 17 00:00:00 2001 From: Ronen Ulanovsky Date: Sun, 24 May 2026 11:28:08 +0300 Subject: [PATCH] dtls: split local invalid state errors --- CHANGELOG.md | 1 + src/dtls12/client.rs | 92 +++++++++++++++++++++++++++++++++++++------- src/dtls12/engine.rs | 22 +++++------ src/dtls12/server.rs | 18 ++++----- src/dtls13/client.rs | 64 +++++++++++++++++++++++++++--- src/dtls13/engine.rs | 4 +- src/dtls13/server.rs | 8 ++-- src/error.rs | 3 ++ 8 files changed, 166 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 51748c28..6b1421da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ # Unreleased + * Split local DTLS invalid-state errors from peer-input errors #126 * Reject malformed DTLS 1.3 ClientHello extension vectors #133 * Reject malformed DTLS 1.3 KeyUpdate bodies #131 * Reject malformed DTLS 1.3 ACK record-number vectors #130 diff --git a/src/dtls12/client.rs b/src/dtls12/client.rs index 81f031f4..6f4d99c2 100644 --- a/src/dtls12/client.rs +++ b/src/dtls12/client.rs @@ -552,7 +552,7 @@ impl State { }; if certificate.certificate_list.is_empty() { - return Err(Error::UnexpectedMessage( + return Err(Error::CertificateError( "No server certificate received".to_string(), )); } @@ -585,7 +585,7 @@ impl State { let cipher_suite = client .engine .cipher_suite() - .ok_or_else(|| Error::UnexpectedMessage("No cipher suite selected".to_string()))?; + .ok_or_else(|| Error::InvalidState("No cipher suite selected".to_string()))?; if cipher_suite.is_psk() { self.await_server_key_exchange_psk(client) @@ -669,7 +669,7 @@ impl State { let cipher_suite = client .engine .cipher_suite() - .ok_or_else(|| Error::UnexpectedMessage("No cipher suite selected".to_string()))?; + .ok_or_else(|| Error::InvalidState("No cipher suite selected".to_string()))?; // Ensure the server's (hash, signature) pair was offered by the client let offered = SignatureAndHashAlgorithm::supported().contains(&signature_algorithm); @@ -843,7 +843,7 @@ impl State { let cipher_suite = client .engine .cipher_suite() - .ok_or_else(|| Error::UnexpectedMessage("No cipher suite selected".to_string()))?; + .ok_or_else(|| Error::InvalidState("No cipher suite selected".to_string()))?; if cipher_suite.is_psk() { // PSK: no certificates involved @@ -905,7 +905,7 @@ impl State { let cipher_suite = client .engine .cipher_suite() - .ok_or_else(|| Error::UnexpectedMessage("No cipher suite selected".to_string()))?; + .ok_or_else(|| Error::InvalidState("No cipher suite selected".to_string()))?; let suite_hash = cipher_suite.hash_algorithm(); let mut buf = Buf::new(); @@ -949,15 +949,13 @@ impl State { fn derive_keys(client: &mut Client) -> Result<(), Error> { trace!("Deriving keys"); let Some(cipher_suite) = client.engine.cipher_suite() else { - return Err(Error::UnexpectedMessage( - "No cipher suite selected".to_string(), - )); + return Err(Error::InvalidState("No cipher suite selected".to_string())); }; trace!("Using cipher suite for key derivation: {:?}", cipher_suite); let Some(server_random) = &client.server_random else { - return Err(Error::UnexpectedMessage( + return Err(Error::InvalidState( "No server random available".to_string(), )); }; @@ -978,7 +976,7 @@ impl State { // Use the captured session hash from when ServerHelloDone was received let session_hash = client.captured_session_hash.as_ref().ok_or_else(|| { - Error::CryptoError( + Error::InvalidState( "Extended Master Secret negotiated but session hash not captured".to_string(), ) })?; @@ -1256,9 +1254,7 @@ fn handshake_create_certificate(body: &mut Buf, engine: &mut Engine) -> Result<( fn handshake_create_client_key_exchange(body: &mut Buf, engine: &mut Engine) -> Result<(), Error> { // Just check that a cipher suite exists without binding to unused variable let Some(cipher_suite) = engine.cipher_suite() else { - return Err(Error::UnexpectedMessage( - "No cipher suite selected".to_string(), - )); + return Err(Error::InvalidState("No cipher suite selected".to_string())); }; let key_exchange_algorithm = cipher_suite.as_key_exchange_algorithm(); @@ -1384,3 +1380,73 @@ impl LocalEvent { } } } + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use super::*; + + fn psk_client() -> Client { + let engine = Engine::new(Arc::new(Config::default()), AuthMode::Psk); + Client::new_with_engine(engine, Instant::now()) + } + + fn epoch0_handshake_packet(msg_type: MessageType, message_seq: u16, body: &[u8]) -> Vec { + let handshake_len = 12 + body.len(); + let mut packet = Vec::new(); + packet.push(ContentType::Handshake.as_u8()); + packet.extend_from_slice(&[0xfe, 0xfd]); + packet.extend_from_slice(&0u16.to_be_bytes()); + packet.extend_from_slice(&0u64.to_be_bytes()[2..]); + packet.extend_from_slice(&(handshake_len as u16).to_be_bytes()); + packet.push(msg_type.as_u8()); + packet.extend_from_slice(&(body.len() as u32).to_be_bytes()[1..]); + packet.extend_from_slice(&message_seq.to_be_bytes()); + packet.extend_from_slice(&0u32.to_be_bytes()[1..]); + packet.extend_from_slice(&(body.len() as u32).to_be_bytes()[1..]); + packet.extend_from_slice(body); + packet + } + + #[test] + fn empty_server_certificate_is_certificate_error() { + let mut client = psk_client(); + client + .engine + .parse_packet(&epoch0_handshake_packet( + MessageType::Certificate, + 0, + &[0, 0, 0], + )) + .expect("queue empty Certificate"); + + let err = State::AwaitCertificate + .await_certificate(&mut client) + .expect_err("empty server Certificate should fail"); + + assert!(matches!(err, Error::CertificateError(_))); + } + + #[test] + fn derive_keys_without_cipher_suite_is_invalid_state() { + let mut client = psk_client(); + + let err = State::derive_keys(&mut client) + .expect_err("derive_keys requires negotiated cipher suite"); + + assert!(matches!(err, Error::InvalidState(_))); + } + + #[test] + fn derive_keys_without_server_random_is_invalid_state() { + let mut client = psk_client(); + client + .engine + .set_cipher_suite(Dtls12CipherSuite::PSK_AES128_CCM_8); + + let err = State::derive_keys(&mut client).expect_err("derive_keys requires server random"); + + assert!(matches!(err, Error::InvalidState(_))); + } +} diff --git a/src/dtls12/engine.rs b/src/dtls12/engine.rs index fc48e51f..82a3d960 100644 --- a/src/dtls12/engine.rs +++ b/src/dtls12/engine.rs @@ -728,14 +728,14 @@ impl Engine { where F: FnOnce(&mut Buf), { - let maybe_suite = - if epoch >= 1 { - Some(self.cipher_suite().ok_or_else(|| { - Error::UnexpectedMessage("No cipher suite selected".to_string()) - })?) - } else { - None - }; + let maybe_suite = if epoch >= 1 { + Some( + self.cipher_suite() + .ok_or_else(|| Error::InvalidState("No cipher suite selected".to_string()))?, + ) + } else { + None + }; // Prepare the plaintext fragment let mut fragment = self.buffers_free.pop(); @@ -923,7 +923,7 @@ impl Engine { // explicit_nonce + tag). Used to size fragments to fit the MTU. let protection_overhead = if epoch >= 1 { self.cipher_suite() - .ok_or_else(|| Error::UnexpectedMessage("No cipher suite selected".to_string()))?; + .ok_or_else(|| Error::InvalidState("No cipher suite selected".to_string()))?; self.min_protected_fragment_len() } else { 0 @@ -1190,9 +1190,7 @@ impl Engine { pub fn generate_verify_data(&mut self, is_client: bool) -> Result<[u8; 12], Error> { let Some(suite) = self.cipher_suite() else { - return Err(Error::UnexpectedMessage( - "No cipher suite selected".to_string(), - )); + return Err(Error::InvalidState("No cipher suite selected".to_string())); }; let algorithm = suite.hash_algorithm(); let mut handshake_hash = self.buffers_free.pop(); diff --git a/src/dtls12/server.rs b/src/dtls12/server.rs index b28029e0..52d9e91c 100644 --- a/src/dtls12/server.rs +++ b/src/dtls12/server.rs @@ -506,7 +506,7 @@ impl State { let cs = server .engine .cipher_suite() - .ok_or_else(|| Error::UnexpectedMessage("No cipher suite selected".to_string()))?; + .ok_or_else(|| Error::InvalidState("No cipher suite selected".to_string()))?; // PSK suites skip Certificate if cs.is_psk() { @@ -532,7 +532,7 @@ impl State { let cs = server .engine .cipher_suite() - .ok_or_else(|| Error::UnexpectedMessage("No cipher suite selected".to_string()))?; + .ok_or_else(|| Error::InvalidState("No cipher suite selected".to_string()))?; if cs.is_psk() { return self.send_server_key_exchange_psk(server); @@ -540,7 +540,7 @@ impl State { let client_random = server .client_random - .ok_or_else(|| Error::UnexpectedMessage("No client random".to_string()))?; + .ok_or_else(|| Error::InvalidState("No client random".to_string()))?; // unwrap: is ok because we set the random in handle_timeout let server_random = server.random.unwrap(); @@ -668,7 +668,7 @@ impl State { let cs = server .engine .cipher_suite() - .ok_or_else(|| Error::UnexpectedMessage("No cipher suite selected".to_string()))?; + .ok_or_else(|| Error::InvalidState("No cipher suite selected".to_string()))?; // PSK: no client certificates if cs.is_psk() { @@ -747,7 +747,7 @@ impl State { let suite = server .engine .cipher_suite() - .ok_or_else(|| Error::UnexpectedMessage("No cipher suite selected".to_string()))?; + .ok_or_else(|| Error::InvalidState("No cipher suite selected".to_string()))?; if suite.is_psk() { // Extract PSK identity range before dropping handshake @@ -839,7 +839,7 @@ impl State { }; let session_hash = server.captured_session_hash.as_ref().ok_or_else(|| { - Error::CryptoError( + Error::InvalidState( "Extended Master Secret negotiated but session hash not captured".to_string(), ) })?; @@ -1189,7 +1189,7 @@ fn handshake_create_server_hello( let cs = engine .cipher_suite() - .ok_or_else(|| Error::UnexpectedMessage("No cipher suite".to_string()))?; + .ok_or_else(|| Error::InvalidState("No cipher suite".to_string()))?; let srtp_pid = negotiated_srtp_profile.map(|p| match p { SrtpProfile::AEAD_AES_256_GCM => SrtpProfileId::SRTP_AEAD_AES_256_GCM, @@ -1220,9 +1220,7 @@ fn handshake_create_server_key_exchange( algorithm: SignatureAndHashAlgorithm, ) -> Result<(), Error> { let Some(cipher_suite) = engine.cipher_suite() else { - return Err(Error::UnexpectedMessage( - "No cipher suite selected".to_string(), - )); + return Err(Error::InvalidState("No cipher suite selected".to_string())); }; let key_exchange_algorithm = cipher_suite.as_key_exchange_algorithm(); diff --git a/src/dtls13/client.rs b/src/dtls13/client.rs index 2570ebab..4e93c8d7 100644 --- a/src/dtls13/client.rs +++ b/src/dtls13/client.rs @@ -615,7 +615,7 @@ impl State { let key_exchange = client .active_key_exchange .take() - .ok_or_else(|| Error::CryptoError("No active key exchange".to_string()))?; + .ok_or_else(|| Error::InvalidState("No active key exchange".to_string()))?; if key_exchange.group() != server_group { return Err(Error::SecurityError(format!( @@ -751,7 +751,7 @@ impl State { } if certificate.certificate_list.is_empty() { - return Err(Error::UnexpectedMessage( + return Err(Error::CertificateError( "No server certificate received".to_string(), )); } @@ -864,7 +864,7 @@ impl State { let server_hs_secret = client .server_hs_traffic_secret .as_ref() - .ok_or_else(|| Error::CryptoError("No server handshake traffic secret".to_string()))?; + .ok_or_else(|| Error::InvalidState("No server handshake traffic secret".to_string()))?; let expected_verify_data = client.engine.compute_verify_data(server_hs_secret)?; let maybe = client @@ -903,7 +903,7 @@ impl State { // Derive application secrets from handshake secret + transcript through server Finished let handshake_secret = client.handshake_secret.as_ref().ok_or_else(|| { - Error::CryptoError("No handshake secret for application key derivation".to_string()) + Error::InvalidState("No handshake secret for application key derivation".to_string()) })?; let (c_ap_traffic, s_ap_traffic) = @@ -997,7 +997,7 @@ impl State { } let client_hs_secret = client.client_hs_traffic_secret.as_ref().ok_or_else(|| { - Error::CryptoError("No client handshake traffic secret for Finished".to_string()) + Error::InvalidState("No client handshake traffic secret for Finished".to_string()) })?; let mut client_hs_secret_copy = Buf::new(); client_hs_secret_copy.extend_from_slice(client_hs_secret); @@ -1526,3 +1526,57 @@ impl LocalEvent { } } } + +#[cfg(all(test, feature = "rcgen"))] +mod tests { + use std::sync::Arc; + + use super::*; + use crate::Config; + use crate::certificate::generate_self_signed_certificate; + + fn client() -> Client { + let cert = generate_self_signed_certificate().expect("generate cert"); + let engine = Engine::new(Arc::new(Config::default()), cert); + Client::new_with_engine(engine, Instant::now()) + } + + fn epoch0_handshake_packet(msg_type: MessageType, message_seq: u16, body: &[u8]) -> Vec { + let handshake_len = 12 + body.len(); + let mut packet = Vec::new(); + packet.push(ContentType::Handshake.as_u8()); + packet.extend_from_slice(&[0xfe, 0xfd]); + packet.extend_from_slice(&0u16.to_be_bytes()); + packet.extend_from_slice(&0u64.to_be_bytes()[2..]); + packet.extend_from_slice(&(handshake_len as u16).to_be_bytes()); + packet.push(msg_type.as_u8()); + packet.extend_from_slice(&(body.len() as u32).to_be_bytes()[1..]); + packet.extend_from_slice(&message_seq.to_be_bytes()); + packet.extend_from_slice(&0u32.to_be_bytes()[1..]); + packet.extend_from_slice(&(body.len() as u32).to_be_bytes()[1..]); + packet.extend_from_slice(body); + packet + } + + #[test] + fn empty_server_certificate_is_certificate_error() { + let mut client = client(); + client.engine.advance_peer_handshake_seq(); // ServerHello + client.engine.advance_peer_handshake_seq(); // EncryptedExtensions + + client + .engine + .parse_packet(&epoch0_handshake_packet( + MessageType::Certificate, + 2, + &[0, 0, 0, 0], + )) + .expect("queue empty Certificate"); + + let err = State::AwaitCertificate + .await_certificate(&mut client) + .expect_err("empty server Certificate should fail"); + + assert!(matches!(err, Error::CertificateError(_))); + } +} diff --git a/src/dtls13/engine.rs b/src/dtls13/engine.rs index 5401693c..39ef14cc 100644 --- a/src/dtls13/engine.rs +++ b/src/dtls13/engine.rs @@ -1821,7 +1821,7 @@ impl Engine { /// Rotate send keys: move current app send keys → prev, derive new ones. fn update_send_keys(&mut self) -> Result<(), Error> { let current_keys = self.app_send_keys.take().ok_or_else(|| { - Error::CryptoError("No current app send keys for KeyUpdate".to_string()) + Error::InvalidState("No current app send keys for KeyUpdate".to_string()) })?; let next_secret = self.derive_next_traffic_secret(¤t_keys.traffic_secret)?; @@ -1852,7 +1852,7 @@ impl Engine { pub fn update_recv_keys(&mut self) -> Result { // Find the latest recv epoch entry let latest = self.app_recv_keys.last().ok_or_else(|| { - Error::CryptoError("No current app recv keys for KeyUpdate".to_string()) + Error::InvalidState("No current app recv keys for KeyUpdate".to_string()) })?; let next_secret = self.derive_next_traffic_secret(&latest.keys.traffic_secret)?; diff --git a/src/dtls13/server.rs b/src/dtls13/server.rs index 222b6764..58689c87 100644 --- a/src/dtls13/server.rs +++ b/src/dtls13/server.rs @@ -788,7 +788,7 @@ impl State { // Derive handshake secrets let shared_secret = server.shared_secret.take().ok_or_else(|| { - Error::CryptoError("No shared secret for handshake key derivation".to_string()) + Error::InvalidState("No shared secret for handshake key derivation".to_string()) })?; let (c_hs_traffic, s_hs_traffic, handshake_secret) = @@ -878,7 +878,7 @@ impl State { trace!("Sending server Finished message"); let server_hs_secret = server.server_hs_traffic_secret.as_ref().ok_or_else(|| { - Error::CryptoError("No server handshake traffic secret for Finished".to_string()) + Error::InvalidState("No server handshake traffic secret for Finished".to_string()) })?; let mut server_hs_secret_copy = Buf::new(); server_hs_secret_copy.extend_from_slice(server_hs_secret); @@ -894,7 +894,7 @@ impl State { // Derive application secrets from handshake secret + transcript through server Finished let handshake_secret = server.handshake_secret.as_ref().ok_or_else(|| { - Error::CryptoError("No handshake secret for application key derivation".to_string()) + Error::InvalidState("No handshake secret for application key derivation".to_string()) })?; let (c_ap_traffic, s_ap_traffic) = @@ -1053,7 +1053,7 @@ impl State { let client_hs_secret = server .client_hs_traffic_secret .as_ref() - .ok_or_else(|| Error::CryptoError("No client handshake traffic secret".to_string()))?; + .ok_or_else(|| Error::InvalidState("No client handshake traffic secret".to_string()))?; let expected_verify_data = server.engine.compute_verify_data(client_hs_secret)?; let maybe = server diff --git a/src/error.rs b/src/error.rs index 37b21e27..35a0582f 100644 --- a/src/error.rs +++ b/src/error.rs @@ -10,6 +10,8 @@ pub enum Error { ParseError(nom::error::ErrorKind), /// Unexpected DTLS message UnexpectedMessage(String), + /// Local state was missing data required for the requested operation + InvalidState(String), /// Cryptographic operation failed CryptoError(String), /// Certificate validation failed @@ -72,6 +74,7 @@ impl std::fmt::Display for Error { Error::ParseIncomplete => write!(f, "parse incomplete"), Error::ParseError(kind) => write!(f, "parse error: {:?}", kind), Error::UnexpectedMessage(msg) => write!(f, "unexpected message: {}", msg), + Error::InvalidState(msg) => write!(f, "invalid state: {}", msg), Error::CryptoError(msg) => write!(f, "crypto error: {}", msg), Error::CertificateError(msg) => write!(f, "certificate error: {}", msg), Error::SecurityError(msg) => write!(f, "security error: {}", msg),