Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

backport rustls/webpki path complexity budget #277

Merged
merged 3 commits into from
Sep 30, 2023
Merged

backport rustls/webpki path complexity budget #277

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f827e02
Track signature limit using `Budget` type
ctz Sep 5, 2023
75322f4
Add comment indicating source of signature budget
ctz Sep 5, 2023
95b6b20
verify_cert: apply path building budget
cpu Sep 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 106 additions & 34 deletions src/verify_cert.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
// ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
// OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.

use core::default::Default;

use crate::{
cert::{self, Cert, EndEntityOrCa},
der, name, signed_data, time, Error, SignatureAlgorithm, TrustAnchor,
Expand All @@ -33,7 +35,7 @@ pub fn build_chain(
cert,
time,
0,
&mut 0,
&mut Budget::default(),
);
result.map_err(|error| {
match error {
Expand All @@ -49,6 +51,8 @@ pub fn build_chain(
/// for testing).
enum InternalError {
MaximumSignatureChecksExceeded,
/// The maximum number of internal path building calls has been reached. Path complexity is too great.
MaximumPathBuildCallsExceeded,
}

enum ErrorOrInternalError {
Expand All @@ -60,7 +64,8 @@ impl ErrorOrInternalError {
fn is_fatal(&self) -> bool {
match self {
ErrorOrInternalError::Error(_) => false,
ErrorOrInternalError::InternalError(InternalError::MaximumSignatureChecksExceeded) => {
ErrorOrInternalError::InternalError(InternalError::MaximumSignatureChecksExceeded)
| ErrorOrInternalError::InternalError(InternalError::MaximumPathBuildCallsExceeded) => {
true
}
}
Expand All @@ -79,6 +84,7 @@ impl From<Error> for ErrorOrInternalError {
}
}

#[allow(clippy::too_many_arguments)]
fn build_chain_inner(
required_eku_if_present: KeyPurposeId,
supported_sig_algs: &[&SignatureAlgorithm],
Expand All @@ -87,7 +93,7 @@ fn build_chain_inner(
cert: &Cert,
time: time::Time,
sub_ca_count: usize,
signatures: &mut usize,
budget: &mut Budget,
) -> Result<(), ErrorOrInternalError> {
let used_as_ca = used_as_ca(&cert.ee_or_ca);

Expand Down Expand Up @@ -132,7 +138,7 @@ fn build_chain_inner(

// TODO: check_distrust(trust_anchor_subject, trust_anchor_spki)?;

check_signatures(supported_sig_algs, cert, trust_anchor_spki, signatures)?;
check_signatures(supported_sig_algs, cert, trust_anchor_spki, budget)?;

Ok(())
}) {
Expand Down Expand Up @@ -182,6 +188,7 @@ fn build_chain_inner(
UsedAsCa::Yes => sub_ca_count + 1,
};

budget.consume_build_chain_call()?;
build_chain_inner(
required_eku_if_present,
supported_sig_algs,
Expand All @@ -190,7 +197,7 @@ fn build_chain_inner(
&potential_issuer,
time,
next_sub_ca_count,
signatures,
budget,
)
})
}
Expand All @@ -199,16 +206,12 @@ fn check_signatures(
supported_sig_algs: &[&SignatureAlgorithm],
cert_chain: &Cert,
trust_anchor_key: untrusted::Input,
signatures: &mut usize,
budget: &mut Budget,
) -> Result<(), ErrorOrInternalError> {
let mut spki_value = trust_anchor_key;
let mut cert = cert_chain;
loop {
*signatures += 1;
if *signatures > 100 {
return Err(InternalError::MaximumSignatureChecksExceeded.into());
}

budget.consume_signature()?;
signed_data::verify_signed_data(supported_sig_algs, spki_value, &cert.signed_data)?;

// TODO: check revocation
Expand All @@ -227,6 +230,47 @@ fn check_signatures(
Ok(())
}

struct Budget {
signatures: usize,
build_chain_calls: usize,
}

impl Budget {
#[inline]
fn consume_signature(&mut self) -> Result<(), InternalError> {
self.signatures = self
.signatures
.checked_sub(1)
.ok_or(InternalError::MaximumSignatureChecksExceeded)?;
Ok(())
}

#[inline]
fn consume_build_chain_call(&mut self) -> Result<(), InternalError> {
self.build_chain_calls = self
.build_chain_calls
.checked_sub(1)
.ok_or(InternalError::MaximumPathBuildCallsExceeded)?;
Ok(())
}
}

impl Default for Budget {
fn default() -> Self {
Self {
// This limit is taken from the remediation for golang CVE-2018-16875. However,
// note that golang subsequently implemented AKID matching due to this limit
// being hit in real applications (see <https://github.com/spiffe/spire/issues/1004>).
// So this may actually be too aggressive.
signatures: 100,

// This limit is taken from NSS libmozpkix, see:
// <https://github.com/nss-dev/nss/blob/bb4a1d38dd9e92923525ac6b5ed0288479f3f3fc/lib/mozpkix/lib/pkixbuild.cpp#L381-L393>
build_chain_calls: 200_000,
}
}
}

fn check_issuer_independent_properties(
cert: &Cert,
time: time::Time,
Expand Down Expand Up @@ -427,25 +471,35 @@ where
Err(Error::UnknownIssuer.into())
}

#[cfg(feature = "alloc")]
#[cfg(test)]
mod tests {
use alloc::string::ToString;
use alloc::vec::Vec;
use core::convert::TryFrom;

#[test]
#[cfg(feature = "alloc")]
fn test_too_many_signatures() {
use super::*;
use super::*;
use crate::verify_cert::{Budget, InternalError};

enum ChainTrustAnchor {
InChain,
NotInChain,
}

fn build_degenerate_chain(
intermediate_count: usize,
trust_anchor: ChainTrustAnchor,
) -> ErrorOrInternalError {
use crate::ECDSA_P256_SHA256;
use crate::{EndEntityCert, Time};
use alloc::{string::ToString, vec::Vec};
use core::convert::TryFrom;

let alg = &rcgen::PKCS_ECDSA_P256_SHA256;

let make_issuer = || {
let make_issuer = |org_name| {
let mut ca_params = rcgen::CertificateParams::new(Vec::new());
ca_params
.distinguished_name
.push(rcgen::DnType::OrganizationName, "Bogus Subject");
.push(rcgen::DnType::OrganizationName, org_name);
ca_params.is_ca = rcgen::IsCa::Ca(rcgen::BasicConstraints::Unconstrained);
ca_params.key_usages = vec![
rcgen::KeyUsagePurpose::KeyCertSign,
Expand All @@ -456,13 +510,17 @@ mod tests {
rcgen::Certificate::from_params(ca_params).unwrap()
};

let ca_cert = make_issuer();
let ca_cert = make_issuer("Bogus Subject");
let ca_cert_der = ca_cert.serialize_der().unwrap();

let mut intermediates = Vec::with_capacity(101);
let mut intermediates = Vec::with_capacity(intermediate_count);
if let ChainTrustAnchor::InChain = trust_anchor {
intermediates.push(ca_cert_der.to_vec());
}

let mut issuer = ca_cert;
for _ in 0..101 {
let intermediate = make_issuer();
for _ in 0..intermediate_count {
let intermediate = make_issuer("Bogus Subject");
let intermediate_der = intermediate.serialize_der_with_signer(&issuer).unwrap();
intermediates.push(intermediate_der);
issuer = intermediate;
Expand All @@ -474,29 +532,43 @@ mod tests {
let ee_cert = rcgen::Certificate::from_params(ee_params).unwrap();
let ee_cert_der = ee_cert.serialize_der_with_signer(&issuer).unwrap();

let anchors = &[TrustAnchor::try_from_cert_der(&ca_cert_der).unwrap()];
let trust_anchor = match trust_anchor {
ChainTrustAnchor::InChain => make_issuer("Bogus Trust Anchor").serialize_der().unwrap(),
ChainTrustAnchor::NotInChain => ca_cert_der.clone(),
};

let anchors = &[TrustAnchor::try_from_cert_der(&trust_anchor).unwrap()];
let time = Time::from_seconds_since_unix_epoch(0x1fed_f00d);
let cert = EndEntityCert::try_from(&ee_cert_der[..]).unwrap();
let intermediates_der: Vec<&[u8]> = intermediates.iter().map(|x| x.as_ref()).collect();
let intermediate_certs: &[&[u8]] = intermediates_der.as_ref();
let intermediate_certs = intermediates.iter().map(|x| x.as_ref()).collect::<Vec<_>>();

// TODO: Use `build_chain` when `Error` is made non-exhaustive.
let result = build_chain_inner(
build_chain_inner(
EKU_SERVER_AUTH,
&[&ECDSA_P256_SHA256],
anchors,
intermediate_certs,
&intermediate_certs,
cert.inner(),
time,
0,
&mut 0,
);
&mut Budget::default(),
)
.unwrap_err()
}

#[test]
fn test_too_many_signatures() {
assert!(matches!(
build_degenerate_chain(5, ChainTrustAnchor::NotInChain),
ErrorOrInternalError::InternalError(InternalError::MaximumSignatureChecksExceeded),
));
}

#[test]
fn test_too_many_path_calls() {
let result = build_degenerate_chain(10, ChainTrustAnchor::InChain);
assert!(matches!(
result,
Err(ErrorOrInternalError::InternalError(
InternalError::MaximumSignatureChecksExceeded
))
ErrorOrInternalError::InternalError(InternalError::MaximumPathBuildCallsExceeded),
));
}
}
Loading