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

rust: handle the case where the leaf cert is a CA #3

Merged
merged 4 commits into from
Sep 7, 2023
Merged

rust: handle the case where the leaf cert is a CA #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9041c7d
rust: check `BasicConstraints` extension when `KeyUsage.keyCertSign`
tetsuo-cpp Sep 4, 2023
22cdb19
rust: handle the case where the leaf cert is a CA
tetsuo-cpp Sep 6, 2023
5f5e7b1
mod: simplify permits_leaf
woodruffw Sep 7, 2023
557f4b2
policy: strengthen EE checks
woodruffw Sep 7, 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
13 changes: 4 additions & 9 deletions src/rust/cryptography-x509-validation/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,18 +126,13 @@ where
}

fn build_chain(&self, leaf: &Certificate<'leaf>) -> Result<Chain<'chain>, ValidationError> {
// Before anything else, check whether the given EE cert
// Before anything else, check whether the given leaf cert
// is well-formed according to our policy (and its underlying
// certificate profile).
//
// This includes a check against the EE cert's SANs.
//
// NOTE: This is correct when the certificate passed in is an EE,
// but there's no reason why a user shouldn't be able to pass in an
// intermediate instead and construct a chain from there. Maybe
// check whether `leaf` is actually an EE and, if not, validate
// it against the policy's CA checks instead?
self.policy.permits_ee(leaf)?;
// In the case that the leaf is an EE, this includes a check
// against the EE cert's SANs.
self.policy.permits_leaf(leaf)?;

// NOTE: We start the chain depth at 1, indicating the EE.
self.build_chain_inner(leaf, 1)
Expand Down
39 changes: 38 additions & 1 deletion 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 @@ -472,6 +472,17 @@ impl<'a, B: CryptoOps> Policy<'a, B> {
}
}

/// Checks whether the given "leaf" certificate is compatible with this policy.
///
/// A "leaf" certificate is just the certificate in the leaf position during
/// path validation, whether it be a CA or EE. As such, `permits_leaf`
/// is logically equivalent to `permits_ee(leaf) || permits_ca(leaf)`.
pub(crate) fn permits_leaf(&self, leaf: &Certificate) -> Result<(), PolicyError> {
// NOTE: Perform `permits_ee` first, since 99% of path validations should have
// an EE certificate in the leaf position.
self.permits_ee(leaf).or_else(|_| self.permits_ca(leaf))
Copy link
Author

Choose a reason for hiding this comment

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

@woodruffw Do you mind if I revert this change back to how it used to be?

        let extensions = leaf.extensions()?;
        if let Some(key_usage) = extensions.get_extension(&KEY_USAGE_OID) {
            let key_usage: KeyUsage = key_usage.value()?;
            if key_usage.key_cert_sign() {
                return self.permits_ca(leaf);
            }
        }
        return self.permits_ee(leaf);

There are two things here:

  • In the case of a CA certificate, we end up doing a bunch of irrelevant permits_ee first. As you point out, it's not the common case so it's not that big a deal, but it's still something.
  • When an EE cert fails validation for whatever reason, it calls permits_ca and then errors out (since it's not a CA). The error message that gets propagated out of the API ends up being a check from permits_ca and can be pretty confusing (it got me while I was debugging).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's fine by me 🙂 -- the combinator form is arguably more idiomatic, but your point about the error being masked is a good one.

If you revert this, would you mind adding a short comment explaining? That way I don't try and get clever and refactor it again.

}

/// Checks whether the given CA certificate is compatible with this policy.
pub(crate) fn permits_ca(&self, cert: &Certificate) -> Result<(), PolicyError> {
self.permits_basic(cert)?;
Expand Down Expand Up @@ -571,7 +582,20 @@ impl<'a, B: CryptoOps> Policy<'a, B> {

let extensions = cert.extensions()?;

// 4.1.2.6 / 4.2.1.6: Subject / Subject Alternative Name
// 5280 4.2.1.3: Key Usage
// It isn't stated explicitly, but an EE is defined to be not a CA,
// so it MUST NOT assert keyCertSign.
if let Some(key_usage) = extensions.get_extension(&KEY_USAGE_OID) {
let key_usage: KeyUsage = key_usage.value()?;

if key_usage.key_cert_sign() {
return Err(PolicyError::Other(
"EE is marked as a CA certificate (keyUsage.keyCertSign)",
));
}
}

// 5280 4.1.2.6 / 4.2.1.6: Subject / Subject Alternative Name
// EE certificates MAY have their subject in either the subject or subjectAltName.
// If the subject is empty, then the subjectAltName MUST be marked critical.
if cert.subject().is_empty() {
Expand All @@ -595,6 +619,19 @@ impl<'a, B: CryptoOps> Policy<'a, B> {
// 5280 4.2.1.5: Policy Mappings
// The RFC is not clear on whether these may appear in EE certificates.

// 5280 4.2.1.9: Basic Constraints
// We refute `KeyUsage.keyCertSign` above, so `BasicConstraints.cA` MUST NOT
// be asserted.
if let Some(basic_constraints) = extensions.get_extension(&BASIC_CONSTRAINTS_OID) {
let basic_constraints: BasicConstraints = basic_constraints.value()?;

if basic_constraints.ca {
return Err(PolicyError::Other(
"EE is marked as a CA certificate (basicConstraints.cA)",
));
}
}

// 5280 4.2.1.10: Name Constraints
// NameConstraints MUST NOT appear in EE certificates.

Expand Down
Loading