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

rust: Use extension policy mechanism to check for unaccounted critical extensions #6

Merged
merged 2 commits into from
Oct 24, 2023
Merged

rust: Use extension policy mechanism to check for unaccounted critical extensions #6

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4a1be91
rust: Use extension policy mechanism to check for unaccounted critica…
tetsuo-cpp Oct 24, 2023
9916348
validation/policy: slightly more efficient critical matching
woodruffw Oct 24, 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
79 changes: 26 additions & 53 deletions src/rust/cryptography-x509-validation/src/policy/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,14 +117,6 @@ pub static WEBPKI_PERMITTED_ALGORITHMS: Lazy<HashSet<&AlgorithmIdentifier<'_>>>
])
});

const RFC5280_CRITICAL_CA_EXTENSIONS: &[asn1::ObjectIdentifier] =
&[BASIC_CONSTRAINTS_OID, KEY_USAGE_OID, NAME_CONSTRAINTS_OID];
const RFC5280_CRITICAL_EE_EXTENSIONS: &[asn1::ObjectIdentifier] = &[
BASIC_CONSTRAINTS_OID,
SUBJECT_ALTERNATIVE_NAME_OID,
KEY_USAGE_OID,
];

#[derive(Debug, PartialEq, Eq)]
pub enum PolicyError {
Malformed(asn1::ParseError),
Expand Down Expand Up @@ -222,9 +214,6 @@ pub struct Policy<'a, B: CryptoOps> {
/// If `None`, all signature algorithms are permitted.
pub permitted_algorithms: Option<HashSet<AlgorithmIdentifier<'a>>>,

pub critical_ca_extensions: HashSet<ObjectIdentifier>,
pub critical_ee_extensions: HashSet<ObjectIdentifier>,

common_extension_policies: Vec<ExtensionPolicy<B>>,
ca_extension_policies: Vec<ExtensionPolicy<B>>,
ee_extension_policies: Vec<ExtensionPolicy<B>>,
Expand All @@ -247,8 +236,6 @@ impl<'a, B: CryptoOps> Policy<'a, B> {
.cloned()
.collect(),
),
critical_ca_extensions: RFC5280_CRITICAL_CA_EXTENSIONS.iter().cloned().collect(),
critical_ee_extensions: RFC5280_CRITICAL_EE_EXTENSIONS.iter().cloned().collect(),
common_extension_policies: Vec::from([
// 5280 4.2.1.8: Subject Directory Attributes
ExtensionPolicy::maybe_present(
Expand Down Expand Up @@ -369,6 +356,28 @@ impl<'a, B: CryptoOps> Policy<'a, B> {
}
}

// Check that all critical extensions in this certificate are accounted for.
let critical_extensions = extensions
.iter()
.filter(|e| e.critical)
.map(|e| e.extn_id)
.collect::<HashSet<_>>();
let checked_extensions = self
.common_extension_policies
.iter()
.chain(self.ca_extension_policies.iter())
.chain(self.ee_extension_policies.iter())
.map(|p| p.oid.clone())
.collect::<HashSet<_>>();
let unchecked_extensions = critical_extensions
.difference(&checked_extensions)
.collect::<Vec<_>>();

if !unchecked_extensions.is_empty() {
// TODO: Render the OIDs here.
return Err("certificate contains unaccounted-for critical extensions".into());
}
Comment on lines +359 to +379
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is slightly more verbose, but should be more efficient than the previous critical extension sweep.


Ok(())
}

Expand Down Expand Up @@ -466,16 +475,6 @@ impl<'a, B: CryptoOps> Policy<'a, B> {

// TODO: Policy-level checks for EKUs, algorthms, etc.

// Finally, check whether every critical extension in this CA
// certificate is accounted for.
for ext in extensions.iter() {
if ext.critical && !self.critical_ca_extensions.contains(&ext.extn_id) {
return Err(PolicyError::Other(
"CA certificate contains unaccounted critical extension",
));
}
}

Ok(())
}

Expand Down Expand Up @@ -505,16 +504,6 @@ impl<'a, B: CryptoOps> Policy<'a, B> {

// TODO: Policy-level checks here for KUs, algorithms, etc.

// Finally, check whether every critical extension in this EE certificate
// is accounted for.
for ext in extensions.iter() {
if ext.critical && !self.critical_ee_extensions.contains(&ext.extn_id) {
return Err(PolicyError::Other(
"EE certificate contains unaccounted critical extensions",
));
}
}

Ok(())
}

Expand Down Expand Up @@ -583,15 +572,14 @@ mod tests {
};

use crate::{
ops::tests::NullOps,
policy::{Subject, RFC5280_CRITICAL_CA_EXTENSIONS, RFC5280_CRITICAL_EE_EXTENSIONS},
policy::Subject,
types::{DNSName, IPAddress},
};

use super::{
Policy, ECDSA_SHA256, ECDSA_SHA384, ECDSA_SHA512, RSASSA_PKCS1V15_SHA256,
RSASSA_PKCS1V15_SHA384, RSASSA_PKCS1V15_SHA512, RSASSA_PSS_SHA256, RSASSA_PSS_SHA384,
RSASSA_PSS_SHA512, WEBPKI_PERMITTED_ALGORITHMS,
ECDSA_SHA256, ECDSA_SHA384, ECDSA_SHA512, RSASSA_PKCS1V15_SHA256, RSASSA_PKCS1V15_SHA384,
RSASSA_PKCS1V15_SHA512, RSASSA_PSS_SHA256, RSASSA_PSS_SHA384, RSASSA_PSS_SHA512,
WEBPKI_PERMITTED_ALGORITHMS,
};

#[test]
Expand Down Expand Up @@ -669,21 +657,6 @@ mod tests {
}
}

#[test]
fn test_policy_critical_extensions() {
let time = asn1::DateTime::new(2023, 9, 12, 1, 1, 1).unwrap();
let policy = Policy::new(NullOps {}, None, time);

assert_eq!(
policy.critical_ca_extensions,
RFC5280_CRITICAL_CA_EXTENSIONS.iter().cloned().collect()
);
assert_eq!(
policy.critical_ee_extensions,
RFC5280_CRITICAL_EE_EXTENSIONS.iter().cloned().collect()
);
}

#[test]
fn test_subject_from_impls() {
assert!(matches!(
Expand Down