Skip to content

Commit

Permalink
Add verify_is_valid_tls_server_cert_ext.
Browse files Browse the repository at this point in the history
Add a public API that allows the caller to distinguish the new DoS
errors.
  • Loading branch information
briansmith committed Oct 8, 2023
1 parent cef4925 commit 87dd910
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 60 deletions.
12 changes: 6 additions & 6 deletions src/budget.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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(())
}
Expand Down
28 changes: 24 additions & 4 deletions src/end_entity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down Expand Up @@ -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.
///
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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.
Expand Down
46 changes: 19 additions & 27 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<InternalError> 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<Error> for ErrorOrInternalError {
impl From<Error> for ErrorExt {
fn from(error: Error) -> Self {
Self::Error(error)
}
Expand Down
2 changes: 1 addition & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
33 changes: 11 additions & 22 deletions src/verify_cert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand All @@ -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,
Expand All @@ -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)]
Expand All @@ -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(
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -395,8 +388,8 @@ fn check_eku(

fn loop_while_non_fatal_error<V>(
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,
{
Expand Down Expand Up @@ -424,7 +417,6 @@ mod tests {
use core::convert::TryFrom;

use super::*;
use crate::error::InternalError;

enum ChainTrustAnchor {
InChain,
Expand All @@ -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};

Expand Down Expand Up @@ -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,));
}
}

0 comments on commit 87dd910

Please sign in to comment.