diff --git a/security/insanity/include/insanity/pkix.h b/security/insanity/include/insanity/pkix.h index 0b485ac66f67..6ca7fa3d9cf0 100644 --- a/security/insanity/include/insanity/pkix.h +++ b/security/insanity/include/insanity/pkix.h @@ -27,6 +27,7 @@ SECStatus BuildCertChain(TrustDomain& trustDomain, CERTCertificate* cert, PRTime time, /*optional*/ KeyUsages requiredKeyUsagesIfPresent, + /*optional*/ SECOidTag requiredEKUIfPresent, /*out*/ ScopedCERTCertList& results); // Verify the given signed data using the public key of the given certificate. diff --git a/security/insanity/lib/pkixbuild.cpp b/security/insanity/lib/pkixbuild.cpp index 7e5dbe6ae14b..fe64566da3e8 100644 --- a/security/insanity/lib/pkixbuild.cpp +++ b/security/insanity/lib/pkixbuild.cpp @@ -53,6 +53,7 @@ BackCert::Init() case 15: out = &encodedKeyUsage; break; case 19: out = &encodedBasicConstraints; break; case 35: out = &dummyEncodedAuthorityKeyIdentifier; break; // bug 965136 + case 37: out = &encodedExtendedKeyUsage; break; } } else if (ext->id.len == 9 && ext->id.data[0] == 0x2b && ext->id.data[1] == 0x06 && @@ -91,6 +92,7 @@ static Result BuildForward(TrustDomain& trustDomain, PRTime time, EndEntityOrCA endEntityOrCA, KeyUsages requiredKeyUsagesIfPresent, + SECOidTag requiredEKUIfPresent, unsigned int subCACount, /*out*/ ScopedCERTCertList& results); @@ -100,6 +102,7 @@ BuildForwardInner(TrustDomain& trustDomain, BackCert& subject, PRTime time, EndEntityOrCA endEntityOrCA, + SECOidTag requiredEKUIfPresent, CERTCertificate* potentialIssuerCertToDup, unsigned int subCACount, ScopedCERTCertList& results) @@ -142,7 +145,7 @@ BuildForwardInner(TrustDomain& trustDomain, } rv = BuildForward(trustDomain, potentialIssuer, time, MustBeCA, - KU_KEY_CERT_SIGN, + KU_KEY_CERT_SIGN, requiredEKUIfPresent, newSubCACount, results); if (rv != Success) { return rv; @@ -163,6 +166,7 @@ BuildForward(TrustDomain& trustDomain, PRTime time, EndEntityOrCA endEntityOrCA, KeyUsages requiredKeyUsagesIfPresent, + SECOidTag requiredEKUIfPresent, unsigned int subCACount, /*out*/ ScopedCERTCertList& results) { @@ -191,7 +195,7 @@ BuildForward(TrustDomain& trustDomain, rv = CheckExtensions(subject, endEntityOrCA, trustLevel == TrustDomain::TrustAnchor, - requiredKeyUsagesIfPresent, + requiredKeyUsagesIfPresent, requiredEKUIfPresent, subCACount); if (rv != Success) { return rv; @@ -223,6 +227,7 @@ BuildForward(TrustDomain& trustDomain, for (CERTCertListNode* n = CERT_LIST_HEAD(candidates); !CERT_LIST_END(n, candidates); n = CERT_LIST_NEXT(n)) { rv = BuildForwardInner(trustDomain, subject, time, endEntityOrCA, + requiredEKUIfPresent, n->cert, subCACount, results); if (rv == Success) { // We found a trusted issuer. At this point, we know the cert is valid @@ -241,6 +246,7 @@ BuildCertChain(TrustDomain& trustDomain, CERTCertificate* certToDup, PRTime time, /*optional*/ KeyUsages requiredKeyUsagesIfPresent, + /*optional*/ SECOidTag requiredEKUIfPresent, /*out*/ ScopedCERTCertList& results) { PORT_Assert(certToDup); @@ -260,7 +266,7 @@ BuildCertChain(TrustDomain& trustDomain, } rv = BuildForward(trustDomain, ee, time, MustBeEndEntity, - requiredKeyUsagesIfPresent, + requiredKeyUsagesIfPresent, requiredEKUIfPresent, 0, results); if (rv != Success) { results = nullptr; diff --git a/security/insanity/lib/pkixcheck.cpp b/security/insanity/lib/pkixcheck.cpp index 1d63e1a508fe..142812b3c2a2 100644 --- a/security/insanity/lib/pkixcheck.cpp +++ b/security/insanity/lib/pkixcheck.cpp @@ -149,12 +149,17 @@ CheckBasicConstraints(const BackCert& cert, // CA certificates are not trusted as EE certs. if (basicConstraints.isCA) { - // XXX: We use SEC_ERROR_INADEQUATE_CERT_TYPE here so we can distinguish + // XXX: We use SEC_ERROR_CA_CERT_INVALID here so we can distinguish // this error from other errors, given that NSS does not have a "CA cert // used as end-entity" error code since it doesn't have such a // prohibition. We should add such an error code and stop abusing - // SEC_ERROR_INADEQUATE_CERT_TYPE this way. - return Fail(RecoverableError, SEC_ERROR_INADEQUATE_CERT_TYPE); + // SEC_ERROR_CA_CERT_INVALID this way. + // + // Note, in particular, that this check prevents a delegated OCSP + // response signing certificate with the CA bit from successfully + // validating when we check it from pkixocsp.cpp, which is a good thing. + // + return Fail(RecoverableError, SEC_ERROR_CA_CERT_INVALID); } return Success; @@ -177,6 +182,82 @@ CheckBasicConstraints(const BackCert& cert, return Success; } +// 4.2.1.12. Extended Key Usage (id-ce-extKeyUsage) +// 4.2.1.12. Extended Key Usage (id-ce-extKeyUsage) +Result +CheckExtendedKeyUsage(EndEntityOrCA endEntityOrCA, const SECItem* encodedEKUs, + SECOidTag requiredEKU) +{ + // TODO: Either do not allow anyExtendedKeyUsage to be passed as requiredEKU, + // or require that callers pass anyExtendedKeyUsage instead of + // SEC_OID_UNKNWON and disallow SEC_OID_UNKNWON. + + // XXX: We're using SEC_ERROR_INADEQUATE_CERT_TYPE here so that callers can + // distinguish EKU mismatch from KU mismatch from basic constraints mismatch. + // We should probably add a new error code that is more clear for this type + // of problem. + + bool foundOCSPSigning = false; + + if (encodedEKUs) { + ScopedPtr + seq(CERT_DecodeOidSequence(encodedEKUs)); + if (!seq) { + PR_SetError(SEC_ERROR_INADEQUATE_CERT_TYPE, 0); + return RecoverableError; + } + + bool found = false; + + // XXX: We allow duplicate entries. + for (const SECItem* const* oids = seq->oids; oids && *oids; ++oids) { + SECOidTag oidTag = SECOID_FindOIDTag(*oids); + if (requiredEKU != SEC_OID_UNKNOWN && oidTag == requiredEKU) { + found = true; + } + if (oidTag == SEC_OID_OCSP_RESPONDER) { + foundOCSPSigning = true; + } + } + + // If the EKU extension was included, then the required EKU must be in the + // list. + if (!found) { + PR_SetError(SEC_ERROR_INADEQUATE_CERT_TYPE, 0); + return RecoverableError; + } + } + + // pkixocsp.cpp depends on the following additional checks. + + if (foundOCSPSigning) { + // When validating anything other than an delegated OCSP signing cert, + // reject any cert that also claims to be an OCSP responder, because such + // a cert does not make sense. For example, if an SSL certificate were to + // assert id-kp-OCSPSigning then it could sign OCSP responses for itself, + // if not for this check. + if (requiredEKU != SEC_OID_OCSP_RESPONDER) { + PR_SetError(SEC_ERROR_INADEQUATE_CERT_TYPE, 0); + return RecoverableError; + } + } else if (requiredEKU == SEC_OID_OCSP_RESPONDER && + endEntityOrCA == MustBeEndEntity) { + // http://tools.ietf.org/html/rfc6960#section-4.2.2.2: + // "OCSP signing delegation SHALL be designated by the inclusion of + // id-kp-OCSPSigning in an extended key usage certificate extension + // included in the OCSP response signer's certificate." + // + // id-kp-OCSPSigning is the only EKU that isn't implicitly assumed when the + // EKU extension is missing from an end-entity certificate. However, any CA + // certificate can issue a delegated OCSP response signing certificate, so + // we can't require the EKU be explicitly included for CA certificates. + PR_SetError(SEC_ERROR_INADEQUATE_CERT_TYPE, 0); + return RecoverableError; + } + + return Success; +} + // Checks extensions that apply to both EE and intermediate certs, // except for AIA, CRL, and AKI/SKI, which are handled elsewhere. Result @@ -184,8 +265,14 @@ CheckExtensions(BackCert& cert, EndEntityOrCA endEntityOrCA, bool isTrustAnchor, KeyUsages requiredKeyUsagesIfPresent, + SECOidTag requiredEKUIfPresent, unsigned int subCACount) { + if (endEntityOrCA != MustBeCA && isTrustAnchor) { + PR_NOT_REACHED("only CAs can be trust anchors."); + return Fail(FatalError, PR_INVALID_STATE_ERROR); + } + // 4.2.1.1. Authority Key Identifier dealt with as part of path building // 4.2.1.2. Subject Key Identifier dealt with as part of path building @@ -221,6 +308,12 @@ CheckExtensions(BackCert& cert, // 4.2.1.10. Name Constraints // 4.2.1.11. Policy Constraints // 4.2.1.12. Extended Key Usage + rv = CheckExtendedKeyUsage(endEntityOrCA, cert.encodedExtendedKeyUsage, + requiredEKUIfPresent); + if (rv != Success) { + return rv; + } + // 4.2.1.13. CRL Distribution Points will be dealt with elsewhere // 4.2.1.14. Inhibit anyPolicy diff --git a/security/insanity/lib/pkixcheck.h b/security/insanity/lib/pkixcheck.h index ad6aa34c828b..16983b0f9609 100644 --- a/security/insanity/lib/pkixcheck.h +++ b/security/insanity/lib/pkixcheck.h @@ -29,6 +29,7 @@ Result CheckExtensions(BackCert& certExt, EndEntityOrCA endEntityOrCA, bool isTrustAnchor, KeyUsages requiredKeyUsagesIfPresent, + SECOidTag requiredEKUIfPresent, unsigned int depth); } } // namespace insanity::pkix diff --git a/security/insanity/lib/pkixutil.h b/security/insanity/lib/pkixutil.h index 1b2e1372e189..cf0e816f5052 100644 --- a/security/insanity/lib/pkixutil.h +++ b/security/insanity/lib/pkixutil.h @@ -84,6 +84,7 @@ public: // nssCert and childCert must be valid for the lifetime of BackCert BackCert(CERTCertificate* nssCert, BackCert* childCert) : encodedBasicConstraints(nullptr) + , encodedExtendedKeyUsage(nullptr) , encodedKeyUsage(nullptr) , childCert(childCert) , nssCert(nssCert) @@ -93,6 +94,7 @@ public: Result Init(); const SECItem* encodedBasicConstraints; + const SECItem* encodedExtendedKeyUsage; const SECItem* encodedKeyUsage; BackCert* const childCert;