diff --git a/CHANGELOG.md b/CHANGELOG.md index 045bba72..d94f86bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ # Unreleased + * Reject empty required client certificates in DTLS 1.2 and 1.3 #170 + # 0.7.5 * Reject internally reordered DTLS 1.2 datagrams #169 diff --git a/src/dtls12/server.rs b/src/dtls12/server.rs index 515a9287..fb22425e 100644 --- a/src/dtls12/server.rs +++ b/src/dtls12/server.rs @@ -5,7 +5,7 @@ // - Client resends ClientHello with cookie // 3. Server sends ServerHello, Certificate, ServerKeyExchange, // CertificateRequest (required), ServerHelloDone -// 4. Client sends Certificate (optional), ClientKeyExchange, +// 4. Client sends Certificate (required when requested), ClientKeyExchange, // CertificateVerify (if client cert), ChangeCipherSpec, Finished // 5. Server verifies Finished, then sends ChangeCipherSpec, Finished // 6. Handshake complete, application data can flow @@ -724,7 +724,19 @@ impl State { drop(maybe); if cert_ranges.is_empty() { - // Client didn't provide a certificate (allowed), skip + // The handshake has no key material yet, so send a fatal alert in epoch 0. + server.engine.abort(); + server + .engine + .create_record(ContentType::Alert, 0, false, |body| { + body.push(2); // fatal + body.push(40); // handshake_failure + })?; + server.state = State::Closed; + return Err(Error::CertificateError( + crate::CertificateError::NoClientCertificateReceived, + ) + .into()); } else { // Store and verify via callback debug!( diff --git a/src/dtls13/server.rs b/src/dtls13/server.rs index eee5c9c1..3701db9a 100644 --- a/src/dtls13/server.rs +++ b/src/dtls13/server.rs @@ -972,13 +972,20 @@ impl State { }; if certificate.certificate_list.is_empty() { - debug!("Client sent empty certificate list"); - drop(maybe); - server.engine.advance_peer_handshake_seq(); - // Client's Certificate means our flight was received; stop retransmitting - server.engine.flight_stop_resend_timers(); - // Empty cert list is acceptable - proceed without client auth verification - return Ok(Self::AwaitFinished); + server.engine.abort(); + // Server Finished was already sent, so the client now reads epoch 3. + let epoch = server.engine.app_send_epoch(); + server + .engine + .create_ciphertext_record(ContentType::Alert, epoch, false, |body| { + body.push(2); // legacy level (ignored in DTLS 1.3) + body.push(116); // certificate_required + })?; + server.state = State::Closed; + return Err(Error::CertificateError( + crate::CertificateError::NoClientCertificateReceived, + ) + .into()); } debug!( diff --git a/src/error.rs b/src/error.rs index c22663f7..527aad0e 100644 --- a/src/error.rs +++ b/src/error.rs @@ -390,6 +390,8 @@ pub enum CryptoOperation { pub enum CertificateError { /// The peer did not send a server certificate. NoServerCertificateReceived, + /// The client sent an empty certificate list when a certificate was required. + NoClientCertificateReceived, /// Client certificate verification was requested with no client certificate. NoClientCertificateForVerification, /// Server certificate verification was requested with no server certificate. @@ -1050,6 +1052,7 @@ impl fmt::Display for CertificateError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Self::NoServerCertificateReceived => write!(f, "no server certificate received"), + Self::NoClientCertificateReceived => write!(f, "no client certificate received"), Self::NoClientCertificateForVerification => { write!(f, "no client certificate for verification") } diff --git a/tests/dtls12/handshake.rs b/tests/dtls12/handshake.rs index a6f19907..1728ac50 100644 --- a/tests/dtls12/handshake.rs +++ b/tests/dtls12/handshake.rs @@ -7,6 +7,81 @@ use dimpl::{Config, Dtls, SrtpProfile}; use crate::common::*; +#[test] +#[cfg(feature = "rcgen")] +fn dtls12_client_certificate_not_required() { + use dimpl::certificate::generate_self_signed_certificate; + + let now = Instant::now(); + let client_cert = generate_self_signed_certificate().expect("gen client cert"); + let server_cert = generate_self_signed_certificate().expect("gen server cert"); + let config = Arc::new( + Config::builder() + .require_client_certificate(false) + .build() + .expect("build config"), + ); + let mut client = Dtls::new_12(Arc::clone(&config), client_cert, now); + client.set_active(true); + let mut server = Dtls::new_12(config, server_cert, now); + complete_dtls12_handshake(&mut client, &mut server, now); +} + +#[test] +#[cfg(feature = "rcgen")] +fn dtls12_empty_client_certificate_is_rejected_when_required() { + use dimpl::certificate::generate_self_signed_certificate; + use dimpl::{CertificateError, Error}; + + let mut now = Instant::now(); + let client_cert = generate_self_signed_certificate().expect("gen client cert"); + let server_cert = generate_self_signed_certificate().expect("gen server cert"); + let config = Arc::new(Config::builder().build().expect("build config")); + let mut client = Dtls::new_12(Arc::clone(&config), client_cert, now); + client.set_active(true); + let mut server = Dtls::new_12(config, server_cert, now); + + for step in 0..60 { + client.handle_timeout(now).expect("client timeout"); + server.handle_timeout(now).expect("server timeout"); + let client_out = drain_outputs(&mut client); + let server_out = drain_outputs(&mut server); + for packet in &client_out.packets { + if packet.len() >= 25 && packet[0] == 22 && packet[13] == CERTIFICATE { + // Replace the client's plaintext Certificate record with an + // empty certificate list, preserving record and handshake sequence numbers. + let mut empty = packet[..25].to_vec(); + empty[11..13].copy_from_slice(&15u16.to_be_bytes()); + empty[14..17].copy_from_slice(&[0, 0, 3]); + empty[22..25].copy_from_slice(&[0, 0, 3]); + empty.extend_from_slice(&[0, 0, 0]); + assert!(matches!( + server.handle_packet(&empty), + Err(Error::CertificateError( + CertificateError::NoClientCertificateReceived + )) + )); + let output = drain_outputs(&mut server); + assert_eq!(output.packets.len(), 1, "one fatal alert packet"); + let alert = &output.packets[0]; + assert_eq!(alert[0], 21, "DTLS Alert record"); + assert_eq!(&alert[3..5], &[0, 0], "pre-key-exchange epoch"); + assert_eq!(&alert[13..], &[2, 40], "fatal handshake_failure"); + assert!(server.is_closed()); + return; + } + server.handle_packet(packet).expect("server packet"); + } + deliver_packets(&server_out.packets, &mut client); + if step % 5 == 4 { + now += Duration::from_secs(2); + } else { + now += Duration::from_millis(50); + } + } + panic!("client did not send Certificate"); +} + #[test] #[cfg(feature = "rcgen")] fn dtls12_cookie_retry_proceeds_to_server_hello() { diff --git a/tests/dtls13/handshake.rs b/tests/dtls13/handshake.rs index 1799bf7f..288dfdac 100644 --- a/tests/dtls13/handshake.rs +++ b/tests/dtls13/handshake.rs @@ -3,10 +3,84 @@ use std::sync::Arc; use std::time::{Duration, Instant}; -use dimpl::{Dtls, SrtpProfile}; +use dimpl::{CertificateError, Config, Dtls, DtlsCertificate, Error, SrtpProfile}; use crate::common::*; +#[test] +#[cfg(feature = "rcgen")] +fn dtls13_empty_client_certificate_is_rejected_when_required() { + use dimpl::certificate::generate_self_signed_certificate; + + let now = Instant::now(); + let client_cert = generate_self_signed_certificate().expect("gen client cert"); + let server_cert = generate_self_signed_certificate().expect("gen server cert"); + let config = Arc::new(Config::builder().build().expect("build config")); + let mut client = Dtls::new_13( + Arc::clone(&config), + DtlsCertificate { + certificate: Vec::new(), + private_key: client_cert.private_key, + }, + now, + ); + client.set_active(true); + let mut server = Dtls::new_13(config, server_cert, now); + + let mut now = now; + for _ in 0..40 { + client.handle_timeout(now).expect("client timeout"); + server.handle_timeout(now).expect("server timeout"); + let client_out = drain_outputs(&mut client); + let server_out = drain_outputs(&mut server); + for packet in &client_out.packets { + match server.handle_packet(packet) { + Err(Error::CertificateError(CertificateError::NoClientCertificateReceived)) => { + let output = drain_outputs(&mut server); + assert_eq!(output.packets.len(), 1, "one fatal alert packet"); + let alert_result = client.handle_packet(&output.packets[0]); + assert!( + matches!( + alert_result, + Err(Error::SecurityError(dimpl::SecurityError::FatalAlert { + description: 116 + })) + ), + "client result: {alert_result:?}" + ); + assert!(server.is_closed()); + return; + } + Err(error) => panic!("unexpected server error: {error}"), + Ok(()) => {} + } + } + deliver_packets(&server_out.packets, &mut client); + now += Duration::from_millis(10); + } + panic!("server accepted an empty client Certificate"); +} + +#[test] +#[cfg(feature = "rcgen")] +fn dtls13_client_certificate_not_required() { + use dimpl::certificate::generate_self_signed_certificate; + + let now = Instant::now(); + let client_cert = generate_self_signed_certificate().expect("gen client cert"); + let server_cert = generate_self_signed_certificate().expect("gen server cert"); + let config = Arc::new( + Config::builder() + .require_client_certificate(false) + .build() + .expect("build config"), + ); + let mut client = Dtls::new_13(Arc::clone(&config), client_cert, now); + client.set_active(true); + let mut server = Dtls::new_13(config, server_cert, now); + complete_dtls13_handshake(&mut client, &mut server, now); +} + #[test] #[cfg(feature = "rcgen")] fn dtls13_basic_handshake() {