rustls / webpki

WebPKI X.509 Certificate Validation in Rust
https://docs.rs/rustls-webpki/latest/webpki/
Other
99 stars 51 forks source link

Unable to use CRL without "issuing distribution point" #228

Closed jasperpatterson closed 3 months ago

jasperpatterson commented 9 months ago

I'm running into a problem getting my CRL to be recognized as authoritative for a certificate.

The certificate itself has a "distribution point" listed on it, however the given CRL does not use the "issuing distribution point" extension, which appears to be optional in RFC 5280. I couldn't find anything in the spec which specifically relates these two fields as a requirement to be present when the other is. As another data point, I do not run into this issue with PKIjs.

In practice, this affects people using AWS Private CA, a managed PKI offering which includes the issuance of X.509 certificates, and hosting of CRLs. The certificates issued by Private CA contain a "distribution point" while the CRLs do not implement the "issuing distribution point" extension.

To be clear, I'm hitting the return cert_dps.is_none() check in this code, returning false:

pub(crate) fn authoritative(&self, path: &PathNode<'_>) -> bool {
    // In all cases we require that the authoritative CRL have the same issuer
    // as the certificate. Recall we do not support indirect CRLs.
    if self.issuer() != path.cert.issuer() {
        return false;
    }

    let crl_idp = match (
        path.cert.crl_distribution_points(),
        self.issuing_distribution_point(),
    ) {
        // If the certificate has no CRL distribution points, and the CRL has no issuing distribution point,
        // then we can consider this CRL authoritative based on the issuer matching.
        (cert_dps, None) => return cert_dps.is_none(),

        // If the CRL has an issuing distribution point, parse it so we can consider its scope
        // and compare against the cert CRL distribution points, if present.
        (_, Some(crl_idp)) => {
            match IssuingDistributionPoint::from_der(untrusted::Input::from(crl_idp)) {
                Ok(crl_idp) => crl_idp,
                Err(_) => return false, // Note: shouldn't happen - we verify IDP at CRL-load.
            }
        }
    };

    crl_idp.authoritative_for(path)
}
cpu commented 9 months ago

Thanks for filing an issue. Have you spoken to Amazon about why their CRLs do not include an IDP extension? Is this something a user can configure, or a baked-in constraint of the product?

For broader context this was discussed fairly thoroughly in https://github.com/rustls/webpki/issues/121

jasperpatterson commented 9 months ago

I haven't yet reached out to AWS to ask about it. However I know it is not user-configurable, the CRL is fully managed.

jasperpatterson commented 7 months ago

I reached out to AWS about this and they provided the following response:

The only required CRL extensions are the Authority Key Identifier (AKI) and CRL number. The IDP extension is not required for non-partitioned CRLs. You mentioned associating certificates to a CRL. This is only required for partitioned CRLs. A non-partitioned CRL is authoritative for all certificates issued by a given CA. When ACM PCA service implement partitioned CRL support, internal team mentioned they will add the IDP extension to CRLs.

cpu commented 7 months ago

Thanks for passing on their reply. I'll have to find time to page back in more context here. I agree the IDP extension is not required per 5280, but when the end entity certificates have a CRL distribution point, wouldn't you want the CRL to have an IDP that identifies it as the same CRL mentioned in the cert CRL DP?

What is the proposed change in behaviour you want to see in webpki (keeping in mind the constraints discussed in #121)?

jasperpatterson commented 7 months ago

I'm not sure I fully understand the constraints from #121 but I suppose the proposal here would be treating the CRL authoritative based solely on the issuer, if the CRL is non-partitioned.

cpu commented 7 months ago

Relaxing the check so that a CRL without an IDP ext. is considered authoritative for a cert with a CRL DP ext. iff the CRL issuer matches the cert issuer might be reasonable, but I think we avoided that out of wariness of substitution attacks. Admitedly the specifics are out of my mental cache.

@jsha Tagging you here as well since you raised the initial concerns in #121 that resulted in the stricter matching logic. Any thoughts on this case? I'm having a hard time justifying our approach based on 5280 alone, but it also seems like relaxing our logic could be dangerous.

eeror commented 4 months ago

@cpu This may be a place where providing solid, safe defaults could lead the user into making choices that are less safe.

Since @jasperpatterson already mentioned Amazon: most (if not all) certificates issued by AWS have a chain from an Amazon Trust Services root (mostly number 1 used for 2048-bit RSA) through one out of four (per root) intermediate certificates. Currently webpki refuses to check the CRLs for those intermediate certificates.

Now, I'd say that if we had a reliable out-of-band confirmation about a certificate being revoked by its issuer, it'd be safer to treat the certificate as revoked. A signed CRL without the extension in question definitely seems to be a reliable out-of-band confirmation regardless of where we got it from.

Furthermore, unless they use RevocationCheckDepth::EndEntity, a user will also be deprived of knowledge about any end entity certificates being revoked as the chain doesn't validate. Now, technically, it is true that we don't know the revocation status for a certificate if we don't know the revocation status of the issuer of the corresponding CRL. Currently, however, Error::UnknownRevocationStatus means at least two totally different things. It can mean that there is no information about the revocation status, but it can also mean that we do have information about a revocation but we simply don't find it reliable enough.

In both of these situations, I would find it plausible (and, depending on the situation, reasonable!) that the user of the library might want to not trust a certificate if there was any trace, even if potentially untrustworthy, of it ever being revoked, but cautiously proceed if there was no trace of it. Note that if they are manually providing the CRLs, they may have varying levels of certainty about whether the set of CRLs is complete or not.

Currently, they don't really get to choose. They may need to proceed if revocation data is not available, but this may lead them to also ignore potentially real revocations just because we don't trust the source for one reason or the other.

cpu commented 4 months ago

I said previously I was open to relaxing the behaviour:

Relaxing the check so that a CRL without an IDP ext. is considered authoritative for a cert with a CRL DP ext. iff the CRL issuer matches the cert issuer might be reasonable

ISTM a CRL without an issuing distribution point isn't partitioned and so the substitution attack discussed in the MDSP post linked to from #121 is of less relevance. I also don't think we have a spec justification for making the CRL IDP ext mandatory on the basis of the cert including the CRL DP ext.

However, I'd prefer not to regress on concerns from #121 trying to accommodate one particular CA. I would still like input from another maintainer (and particularly @jsha, as the originator of #121) before thinking about starting or reviewing a PR with a code change.

eeror commented 4 months ago

@cpu Thank you! Understood. Just wanted to reignite the discussion here. Additionally, I tried to make a point that while this restriction can make some users safer, it can also make some users less safe. (Perhaps a valid reason to provide a configuration option?) In the end, even if AWS is the only CA we know about that does that, a change would accommodate not the CA, but the users having to use that CA. :) I hope @jsha will have time to give some thought to this issue.

eeror commented 4 months ago

Also, while discussing substitution attacks and the like: one way to get out of situations like these would be to be more lenient if and only if it leads to reporting a revocation. This way, the user would have more information to base their further actions on.

jsha commented 4 months ago

Thanks for your patience while I found time to come back to this issue. I agree it makes sense to support CRLs without the IDP extension.

I think the crux of the issue is: according to RFC 5280, CRLs can have a "scope". Given a CRL with no IDP extension, it's impossible to know what the scope of that CRL is. You need out of band information.

In practice, I think most CRLs with no IDP extension have a scope of "everything". And that's such a common configuration that it's often taken for granted. For instance, AWS' documentation for CRLs from their private CA doesn't mention scope at all.

So should rustls-webpki assume that, in the absence of CRLDP, the scope is "all certs from the issuer"? I think that's pretty reasonable. My main concern in #121 was that (a) sharded CRLs could be swapped by an attacker and (b) user code passing the wrong set of CRLs would go unnoticed.

In #138 @cpu wrote:

As of this commit we consider a CRL authoritative for a certificate when:

  • The certificate issuer matches the CRL issuer and,
  • The certificate has no CRL distribution points, and the CRL has no issuing distribution point extension.
  • Or, the certificate has no CRL distribution points, but the the CRL has an issuing distribution point extension with a scope that includes the certificate.
  • Or, the certificate has CRL distribution points, and the CRL has an issuing distribution point extension with a scope that includes the certificate, and at least one distribution point full name is a URI type general name that can also be found in the CRL issuing distribution point full name general name sequence.

We could add:

We are still protected by the requirement that the "certificate issuer matches the CRL issuer".

cpu commented 3 months ago

This issue is fixed in main. Thanks everyone!

I expect we'll cut a 0.102.7 release with the fix after #275 is merged.