From 87dd91048040edd53b07713447c06ce76cc57d2f Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Fri, 6 Oct 2023 22:38:57 -0700 Subject: [PATCH] Add `verify_is_valid_tls_server_cert_ext`. Add a public API that allows the caller to distinguish the new DoS errors. --- src/budget.rs | 12 ++++++------ src/end_entity.rs | 28 ++++++++++++++++++++++++---- src/error.rs | 46 +++++++++++++++++++--------------------------- src/lib.rs | 2 +- src/verify_cert.rs | 33 +++++++++++---------------------- 5 files changed, 61 insertions(+), 60 deletions(-) diff --git a/src/budget.rs b/src/budget.rs index 49997022..ea73a7dc 100644 --- a/src/budget.rs +++ b/src/budget.rs @@ -13,7 +13,7 @@ // ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. -use crate::error::InternalError; +use crate::ErrorExt; pub(super) struct Budget { signatures: usize, @@ -22,23 +22,23 @@ pub(super) struct Budget { impl Budget { #[inline] - pub fn consume_signature(&mut self) -> Result<(), InternalError> { + pub fn consume_signature(&mut self) -> Result<(), ErrorExt> { checked_sub( &mut self.signatures, - InternalError::MaximumSignatureChecksExceeded, + ErrorExt::MaximumSignatureChecksExceeded, ) } #[inline] - pub fn consume_build_chain_call(&mut self) -> Result<(), InternalError> { + pub fn consume_build_chain_call(&mut self) -> Result<(), ErrorExt> { checked_sub( &mut self.build_chain_calls, - InternalError::MaximumPathBuildCallsExceeded, + ErrorExt::MaximumPathBuildCallsExceeded, ) } } -fn checked_sub(value: &mut usize, underflow_error: InternalError) -> Result<(), InternalError> { +fn checked_sub(value: &mut usize, underflow_error: ErrorExt) -> Result<(), ErrorExt> { *value = value.checked_sub(1).ok_or(underflow_error)?; Ok(()) } diff --git a/src/end_entity.rs b/src/end_entity.rs index 09b01135..6af008d7 100644 --- a/src/end_entity.rs +++ b/src/end_entity.rs @@ -13,7 +13,7 @@ // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. use crate::{ - cert, name, signed_data, verify_cert, DnsNameRef, Error, SignatureAlgorithm, Time, + cert, name, signed_data, verify_cert, DnsNameRef, Error, ErrorExt, SignatureAlgorithm, Time, TlsClientTrustAnchors, TlsServerTrustAnchors, }; @@ -79,6 +79,25 @@ impl<'a> EndEntityCert<'a> { &self.inner } + /// Backward-SemVer-compatible wrapper around `verify_is_valid_tls_server_cert_ext`. + /// + /// Errors that aren't representable as an `Error` are mapped to `Error::UnknownIssuer`. + pub fn verify_is_valid_tls_server_cert( + &self, + supported_sig_algs: &[&SignatureAlgorithm], + trust_anchors: &TlsServerTrustAnchors, + intermediate_certs: &[&[u8]], + time: Time, + ) -> Result<(), Error> { + self.verify_is_valid_tls_server_cert_ext( + supported_sig_algs, + trust_anchors, + intermediate_certs, + time, + ) + .map_err(ErrorExt::into_error_lossy) + } + /// Verifies that the end-entity certificate is valid for use by a TLS /// server. /// @@ -89,13 +108,13 @@ impl<'a> EndEntityCert<'a> { /// intermediate certificates that the server sent in the TLS handshake. /// `time` is the time for which the validation is effective (usually the /// current time). - pub fn verify_is_valid_tls_server_cert( + pub fn verify_is_valid_tls_server_cert_ext( &self, supported_sig_algs: &[&SignatureAlgorithm], &TlsServerTrustAnchors(trust_anchors): &TlsServerTrustAnchors, intermediate_certs: &[&[u8]], time: Time, - ) -> Result<(), Error> { + ) -> Result<(), ErrorExt> { verify_cert::build_chain( verify_cert::EKU_SERVER_AUTH, supported_sig_algs, @@ -120,7 +139,7 @@ impl<'a> EndEntityCert<'a> { /// `cert` is the purported end-entity certificate of the client. `time` is /// the time for which the validation is effective (usually the current /// time). - pub fn verify_is_valid_tls_client_cert( + pub fn verify_is_valid_tls_client_cert_ext( &self, supported_sig_algs: &[&SignatureAlgorithm], &TlsClientTrustAnchors(trust_anchors): &TlsClientTrustAnchors, @@ -135,6 +154,7 @@ impl<'a> EndEntityCert<'a> { &self.inner, time, ) + .map_err(ErrorExt::into_error_lossy) } /// Verifies that the certificate is valid for the given DNS host name. diff --git a/src/error.rs b/src/error.rs index e45f0297..3cb7697f 100644 --- a/src/error.rs +++ b/src/error.rs @@ -107,45 +107,37 @@ impl fmt::Display for Error { #[cfg(feature = "std")] impl ::std::error::Error for Error {} -/// Errors that we cannot report externally since `Error` wasn't declared -/// non-exhaustive, but which we need to differentiate internally (at least -/// for testing). -pub(crate) enum InternalError { +/// An error that occurs during certificate validation or name validation. +/// +/// `ErrorExt` effectively extends `Error` to support reporting new errors. Because `Error` is not +/// declared `#[non_exhaustive]` it could not be directly extended in a backward-compatible way. +#[non_exhaustive] +pub enum ErrorExt { + Error(Error), MaximumSignatureChecksExceeded, /// The maximum number of internal path building calls has been reached. Path complexity is too great. MaximumPathBuildCallsExceeded, } -impl InternalError { - fn is_fatal(&self) -> bool { - matches!( - self, - Self::MaximumSignatureChecksExceeded | Self::MaximumPathBuildCallsExceeded - ) - } -} - -pub(crate) enum ErrorOrInternalError { - Error(Error), - InternalError(InternalError), -} - -impl ErrorOrInternalError { - pub fn is_fatal(&self) -> bool { +impl ErrorExt { + pub(crate) fn is_fatal(&self) -> bool { match self { - ErrorOrInternalError::Error(_) => false, - ErrorOrInternalError::InternalError(e) => e.is_fatal(), + Self::Error(_) => false, + Self::MaximumSignatureChecksExceeded | Self::MaximumPathBuildCallsExceeded => true, } } -} -impl From for ErrorOrInternalError { - fn from(value: InternalError) -> Self { - Self::InternalError(value) + pub(crate) fn into_error_lossy(self) -> Error { + match self { + Self::Error(e) => e, + Self::MaximumSignatureChecksExceeded | Self::MaximumPathBuildCallsExceeded => { + Error::UnknownIssuer + } + } } } -impl From for ErrorOrInternalError { +impl From for ErrorExt { fn from(error: Error) -> Self { Self::Error(error) } diff --git a/src/lib.rs b/src/lib.rs index d42f57d9..40bb9590 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -59,7 +59,7 @@ mod verify_cert; pub use { end_entity::EndEntityCert, - error::Error, + error::{Error, ErrorExt}, name::{DnsNameRef, InvalidDnsNameError}, signed_data::{ SignatureAlgorithm, ECDSA_P256_SHA256, ECDSA_P256_SHA384, ECDSA_P384_SHA256, diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 2c243141..579469e0 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -18,7 +18,7 @@ use crate::{ budget::Budget, cert::{self, Cert, EndEntityOrCa}, der, equal, - error::ErrorOrInternalError, + error::ErrorExt, name, signed_data, time, Error, SignatureAlgorithm, TrustAnchor, }; @@ -29,8 +29,8 @@ pub fn build_chain( intermediate_certs: &[&[u8]], cert: &Cert, time: time::Time, -) -> Result<(), Error> { - let result = build_chain_inner( +) -> Result<(), ErrorExt> { + build_chain_inner( required_eku_if_present, supported_sig_algs, trust_anchors, @@ -39,14 +39,7 @@ pub fn build_chain( time, 0, &mut Budget::default(), - ); - result.map_err(|error| { - match error { - ErrorOrInternalError::Error(e) => e, - // Eat internal errors, - ErrorOrInternalError::InternalError(_) => Error::UnknownIssuer, - } - }) + ) } #[allow(clippy::too_many_arguments)] @@ -59,7 +52,7 @@ fn build_chain_inner( time: time::Time, sub_ca_count: usize, budget: &mut Budget, -) -> Result<(), ErrorOrInternalError> { +) -> Result<(), ErrorExt> { let used_as_ca = used_as_ca(&cert.ee_or_ca); check_issuer_independent_properties( @@ -164,7 +157,7 @@ fn check_signatures( cert_chain: &Cert, trust_anchor_key: untrusted::Input, budget: &mut Budget, -) -> Result<(), ErrorOrInternalError> { +) -> Result<(), ErrorExt> { let mut spki_value = trust_anchor_key; let mut cert = cert_chain; loop { @@ -395,8 +388,8 @@ fn check_eku( fn loop_while_non_fatal_error( values: V, - mut f: impl FnMut(V::Item) -> Result<(), ErrorOrInternalError>, -) -> Result<(), ErrorOrInternalError> + mut f: impl FnMut(V::Item) -> Result<(), ErrorExt>, +) -> Result<(), ErrorExt> where V: IntoIterator, { @@ -424,7 +417,6 @@ mod tests { use core::convert::TryFrom; use super::*; - use crate::error::InternalError; enum ChainTrustAnchor { InChain, @@ -434,7 +426,7 @@ mod tests { fn build_degenerate_chain( intermediate_count: usize, trust_anchor: ChainTrustAnchor, - ) -> ErrorOrInternalError { + ) -> ErrorExt { use crate::ECDSA_P256_SHA256; use crate::{EndEntityCert, Time}; @@ -504,16 +496,13 @@ mod tests { fn test_too_many_signatures() { assert!(matches!( build_degenerate_chain(5, ChainTrustAnchor::NotInChain), - ErrorOrInternalError::InternalError(InternalError::MaximumSignatureChecksExceeded), + ErrorExt::MaximumSignatureChecksExceeded, )); } #[test] fn test_too_many_path_calls() { let result = build_degenerate_chain(10, ChainTrustAnchor::InChain); - assert!(matches!( - result, - ErrorOrInternalError::InternalError(InternalError::MaximumPathBuildCallsExceeded), - )); + assert!(matches!(result, ErrorExt::MaximumPathBuildCallsExceeded,)); } }