From 006d646e30ba4a3e73e2260077fe624fbd05ec8b Mon Sep 17 00:00:00 2001 From: zkonge Date: Thu, 23 Feb 2023 16:47:02 +0800 Subject: [PATCH] use HandshakeFailure by default in verification Verification error is not always raised by bad certificate, especially in user provided verifier. For example, they may raise HSM connection error or dynamic certificate resolve error. All of them is not about bad certificate. So send BadCertificateAlert is not appropriate. --- bogo/config.json | 6 ++++-- rustls/src/client/tls12.rs | 4 ++-- rustls/src/client/tls13.rs | 4 ++-- rustls/src/conn.rs | 7 +++++-- rustls/tests/server_cert_verifier.rs | 6 +++--- 5 files changed, 16 insertions(+), 11 deletions(-) diff --git a/bogo/config.json b/bogo/config.json index 5464d846..1b76ede6 100644 --- a/bogo/config.json +++ b/bogo/config.json @@ -380,8 +380,10 @@ }, "TestLocalErrorMap": { "SendServerHelloAsHelloRetryRequest": "remote error: error decoding message", - "GarbageCertificate-Server-TLS12": "remote error: access denied", - "GarbageCertificate-Server-TLS13": "remote error: access denied", + "GarbageCertificate-Server-TLS12": "remote error: bad certificate", + "GarbageCertificate-Server-TLS13": "remote error: bad certificate", + "GarbageCertificate-Client-TLS12": "remote error: bad certificate", + "GarbageCertificate-Client-TLS13": "remote error: bad certificate", "Client-VerifyDefault-RSA_PKCS1_SHA1-TLS12": "tls: no common signature algorithms", "Server-VerifyDefault-RSA_PKCS1_SHA1-TLS12": "tls: no common signature algorithms", "Downgrade-TLS10-Client": "tls: no cipher suite supported by both client and server", diff --git a/rustls/src/client/tls12.rs b/rustls/src/client/tls12.rs index 3eb40dc4..fe7eff5c 100644 --- a/rustls/src/client/tls12.rs +++ b/rustls/src/client/tls12.rs @@ -740,7 +740,7 @@ impl State for ExpectServerDone { &st.server_cert.ocsp_response, now, ) - .map_err(|err| conn::send_cert_error_alert(cx.common, err))?; + .map_err(|err| conn::send_cert_verify_error_alert(cx.common, err))?; // 3. // Build up the contents of the signed message. @@ -766,7 +766,7 @@ impl State for ExpectServerDone { st.config .verifier .verify_tls12_signature(&message, &st.server_cert.cert_chain[0], sig) - .map_err(|err| conn::send_cert_error_alert(cx.common, err))? + .map_err(|err| conn::send_cert_verify_error_alert(cx.common, err))? }; cx.common.peer_certificates = Some(st.server_cert.cert_chain); diff --git a/rustls/src/client/tls13.rs b/rustls/src/client/tls13.rs index 14526342..05078957 100644 --- a/rustls/src/client/tls13.rs +++ b/rustls/src/client/tls13.rs @@ -669,7 +669,7 @@ impl State for ExpectCertificateVerify { &self.server_cert.ocsp_response, now, ) - .map_err(|err| conn::send_cert_error_alert(cx.common, err))?; + .map_err(|err| conn::send_cert_verify_error_alert(cx.common, err))?; // 2. Verify their signature on the handshake. let handshake_hash = self.transcript.get_current_hash(); @@ -681,7 +681,7 @@ impl State for ExpectCertificateVerify { &self.server_cert.cert_chain[0], cert_verify, ) - .map_err(|err| conn::send_cert_error_alert(cx.common, err))?; + .map_err(|err| conn::send_cert_verify_error_alert(cx.common, err))?; cx.common.peer_certificates = Some(self.server_cert.cert_chain); self.transcript.add_message(&m); diff --git a/rustls/src/conn.rs b/rustls/src/conn.rs index e9c3525a..44e767ae 100644 --- a/rustls/src/conn.rs +++ b/rustls/src/conn.rs @@ -1433,16 +1433,19 @@ pub trait SideData {} const DEFAULT_RECEIVED_PLAINTEXT_LIMIT: usize = 16 * 1024; const DEFAULT_BUFFER_LIMIT: usize = 64 * 1024; -pub(crate) fn send_cert_error_alert(common: &mut CommonState, err: Error) -> Error { +pub(crate) fn send_cert_verify_error_alert(common: &mut CommonState, err: Error) -> Error { match err { Error::InvalidCertificate(CertificateError::BadEncoding) => { common.send_fatal_alert(AlertDescription::DecodeError); } + Error::InvalidCertificate(_) => { + common.send_fatal_alert(AlertDescription::BadCertificate); + } Error::PeerMisbehaved(_) => { common.send_fatal_alert(AlertDescription::IllegalParameter); } _ => { - common.send_fatal_alert(AlertDescription::BadCertificate); + common.send_fatal_alert(AlertDescription::HandshakeFailure); } }; diff --git a/rustls/tests/server_cert_verifier.rs b/rustls/tests/server_cert_verifier.rs index eab7e835..5a6534ac 100644 --- a/rustls/tests/server_cert_verifier.rs +++ b/rustls/tests/server_cert_verifier.rs @@ -58,7 +58,7 @@ fn client_can_override_certificate_verification_and_reject_certificate() { ErrorFromPeer::Client(Error::InvalidMessage( InvalidMessage::HandshakePayloadTooLarge, )), - ErrorFromPeer::Server(Error::AlertReceived(AlertDescription::BadCertificate)), + ErrorFromPeer::Server(Error::AlertReceived(AlertDescription::HandshakeFailure)), ]), ); } @@ -89,7 +89,7 @@ fn client_can_override_certificate_verification_and_reject_tls12_signatures() { ErrorFromPeer::Client(Error::InvalidMessage( InvalidMessage::HandshakePayloadTooLarge, )), - ErrorFromPeer::Server(Error::AlertReceived(AlertDescription::BadCertificate)), + ErrorFromPeer::Server(Error::AlertReceived(AlertDescription::HandshakeFailure)), ]), ); } @@ -118,7 +118,7 @@ fn client_can_override_certificate_verification_and_reject_tls13_signatures() { ErrorFromPeer::Client(Error::InvalidMessage( InvalidMessage::HandshakePayloadTooLarge, )), - ErrorFromPeer::Server(Error::AlertReceived(AlertDescription::BadCertificate)), + ErrorFromPeer::Server(Error::AlertReceived(AlertDescription::HandshakeFailure)), ]), ); }