Bug 921895: Check extended key usage in insanity::pkix, r=keeler, r=cviecco, a=sledru
authorBrian Smith <brian@briansmith.org>
Sat, 08 Feb 2014 15:00:32 -0800
changeset 182826 b528490f256045a3a5fd9398cc977c8e1f3e733c
parent 182825 d61793abbcbef4dcaab9c4863e2db69346635318
child 182827 0adaa7bc58d6c7d8c51be16a144b4424dd3d1c8e
push id3343
push userffxbld
push dateMon, 17 Mar 2014 21:55:32 +0000
treeherdermozilla-beta@2f7d3415f79f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler, cviecco, sledru
bugs921895
milestone29.0a2
Bug 921895: Check extended key usage in insanity::pkix, r=keeler, r=cviecco, a=sledru
security/insanity/include/insanity/pkix.h
security/insanity/lib/pkixbuild.cpp
security/insanity/lib/pkixcheck.cpp
security/insanity/lib/pkixcheck.h
security/insanity/lib/pkixutil.h
--- a/security/insanity/include/insanity/pkix.h
+++ b/security/insanity/include/insanity/pkix.h
@@ -22,16 +22,17 @@
 #include "prtime.h"
 
 namespace insanity { namespace pkix {
 
 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.
 // (EC)DSA parameter inheritance is not supported.
 SECStatus VerifySignedData(const CERTSignedData* sd,
                            const CERTCertificate* cert,
                            void* pkcs11PinArg);
 
--- a/security/insanity/lib/pkixbuild.cpp
+++ b/security/insanity/lib/pkixbuild.cpp
@@ -48,16 +48,17 @@ BackCert::Init()
     if (ext->id.len == 3 &&
         ext->id.data[0] == 0x55 && ext->id.data[1] == 0x1d) {
       // { id-ce x }
       switch (ext->id.data[2]) {
         case 14: out = &dummyEncodedSubjectKeyIdentifier; break; // bug 965136
         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 &&
                ext->id.data[2] == 0x06 && ext->id.data[3] == 0x01 &&
                ext->id.data[4] == 0x05 && ext->id.data[5] == 0x05 &&
                ext->id.data[6] == 0x07 && ext->id.data[7] == 0x01) {
       // { id-pe x }
       switch (ext->id.data[8]) {
@@ -86,25 +87,27 @@ BackCert::Init()
   return Success;
 }
 
 static Result BuildForward(TrustDomain& trustDomain,
                            BackCert& subject,
                            PRTime time,
                            EndEntityOrCA endEntityOrCA,
                            KeyUsages requiredKeyUsagesIfPresent,
+                           SECOidTag requiredEKUIfPresent,
                            unsigned int subCACount,
                            /*out*/ ScopedCERTCertList& results);
 
 // The code that executes in the inner loop of BuildForward
 static Result
 BuildForwardInner(TrustDomain& trustDomain,
                   BackCert& subject,
                   PRTime time,
                   EndEntityOrCA endEntityOrCA,
+                  SECOidTag requiredEKUIfPresent,
                   CERTCertificate* potentialIssuerCertToDup,
                   unsigned int subCACount,
                   ScopedCERTCertList& results)
 {
   PORT_Assert(potentialIssuerCertToDup);
 
   BackCert potentialIssuer(potentialIssuerCertToDup, &subject);
   Result rv = potentialIssuer.Init();
@@ -137,17 +140,17 @@ BuildForwardInner(TrustDomain& trustDoma
   unsigned int newSubCACount = subCACount;
   if (endEntityOrCA == MustBeCA) {
     newSubCACount = subCACount + 1;
   } else {
     PR_ASSERT(newSubCACount == 0);
   }
 
   rv = BuildForward(trustDomain, potentialIssuer, time, MustBeCA,
-                    KU_KEY_CERT_SIGN,
+                    KU_KEY_CERT_SIGN, requiredEKUIfPresent,
                     newSubCACount, results);
   if (rv != Success) {
     return rv;
   }
 
   if (trustDomain.VerifySignedData(&subject.GetNSSCert()->signatureWrap,
                                    potentialIssuer.GetNSSCert()) != SECSuccess) {
     return MapSECStatus(SECFailure);
@@ -158,16 +161,17 @@ BuildForwardInner(TrustDomain& trustDoma
 
 // Caller must check for expiration before calling this function
 static Result
 BuildForward(TrustDomain& trustDomain,
              BackCert& subject,
              PRTime time,
              EndEntityOrCA endEntityOrCA,
              KeyUsages requiredKeyUsagesIfPresent,
+             SECOidTag requiredEKUIfPresent,
              unsigned int subCACount,
              /*out*/ ScopedCERTCertList& results)
 {
   // Avoid stack overflows and poor performance by limiting cert length.
   // XXX: 6 is not enough for chains.sh anypolicywithlevel.cfg tests
   static const size_t MAX_DEPTH = 8;
   if (subCACount >= MAX_DEPTH - 1) {
     return RecoverableError;
@@ -186,17 +190,17 @@ BuildForward(TrustDomain& trustDomain,
   if (trustLevel != TrustDomain::TrustAnchor &&
       trustLevel != TrustDomain::InheritsTrust) {
     // The TrustDomain returned a trust level that we weren't expecting.
     return Fail(FatalError, PR_INVALID_STATE_ERROR);
   }
 
   rv = CheckExtensions(subject, endEntityOrCA,
                        trustLevel == TrustDomain::TrustAnchor,
-                       requiredKeyUsagesIfPresent,
+                       requiredKeyUsagesIfPresent, requiredEKUIfPresent,
                        subCACount);
   if (rv != Success) {
     return rv;
   }
 
   if (trustLevel == TrustDomain::TrustAnchor) {
     // End of the recursion. Create the result list and add the trust anchor to
     // it.
@@ -218,16 +222,17 @@ BuildForward(TrustDomain& trustDomain,
   PORT_Assert(candidates.get());
   if (!candidates) {
     return Fail(RecoverableError, SEC_ERROR_UNKNOWN_ISSUER);
   }
 
   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
       return subject.PrependNSSCertToList(results.get());
     }
     if (rv != RecoverableError) {
       return rv;
     }
@@ -236,16 +241,17 @@ BuildForward(TrustDomain& trustDomain,
   return Fail(RecoverableError, SEC_ERROR_UNKNOWN_ISSUER);
 }
 
 SECStatus
 BuildCertChain(TrustDomain& trustDomain,
                CERTCertificate* certToDup,
                PRTime time,
                /*optional*/ KeyUsages requiredKeyUsagesIfPresent,
+               /*optional*/ SECOidTag requiredEKUIfPresent,
                /*out*/ ScopedCERTCertList& results)
 {
   PORT_Assert(certToDup);
 
   if (!certToDup) {
     PR_SetError(SEC_ERROR_INVALID_ARGS, 0);
     return SECFailure;
   }
@@ -255,17 +261,17 @@ BuildCertChain(TrustDomain& trustDomain,
 
   BackCert ee(certToDup, nullptr);
   Result rv = ee.Init();
   if (rv != Success) {
     return SECFailure;
   }
 
   rv = BuildForward(trustDomain, ee, time, MustBeEndEntity,
-                    requiredKeyUsagesIfPresent,
+                    requiredKeyUsagesIfPresent, requiredEKUIfPresent,
                     0, results);
   if (rv != Success) {
     results = nullptr;
     return SECFailure;
   }
 
   // Build the cert chain even if the cert is expired, because we would
   // rather report the untrusted issuer error than the expired error.
--- a/security/insanity/lib/pkixcheck.cpp
+++ b/security/insanity/lib/pkixcheck.cpp
@@ -144,22 +144,27 @@ CheckBasicConstraints(const BackCert& ce
       }
     }
   }
 
   if (endEntityOrCA == MustBeEndEntity) {
     // 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;
   }
 
   PORT_Assert(endEntityOrCA == MustBeCA);
 
   // End-entity certificates are not allowed to act as CA certs.
@@ -172,25 +177,107 @@ CheckBasicConstraints(const BackCert& ce
            static_cast<unsigned int>(basicConstraints.pathLenConstraint)) {
       return Fail(RecoverableError, SEC_ERROR_PATH_LEN_CONSTRAINT_INVALID);
     }
   }
 
   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<CERTOidSequence, CERT_DestroyOidSequence>
+      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
 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
 
   PLArenaPool* arena = cert.GetArena();
   if (!arena) {
     return FatalError;
   }
 
@@ -216,15 +303,21 @@ CheckExtensions(BackCert& cert,
   rv = CheckBasicConstraints(cert, endEntityOrCA, isTrustAnchor, subCACount);
   if (rv != Success) {
     return rv;
   }
 
   // 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
 
   return Success;
 }
 
 } } // namespace insanity::pkix
--- a/security/insanity/lib/pkixcheck.h
+++ b/security/insanity/lib/pkixcheck.h
@@ -24,13 +24,14 @@
 namespace insanity { namespace pkix {
 
 Result CheckTimes(const CERTCertificate* cert, PRTime time);
 
 Result CheckExtensions(BackCert& certExt,
                        EndEntityOrCA endEntityOrCA,
                        bool isTrustAnchor,
                        KeyUsages requiredKeyUsagesIfPresent,
+                       SECOidTag requiredEKUIfPresent,
                        unsigned int depth);
 
 } } // namespace insanity::pkix
 
 #endif // insanity__pkixcheck_h
--- a/security/insanity/lib/pkixutil.h
+++ b/security/insanity/lib/pkixutil.h
@@ -79,25 +79,27 @@ MapSECStatus(SECStatus srv)
 // so that we can parse the extension block once and then process the
 // extensions in an order that may be different than they appear in the cert.
 class BackCert
 {
 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)
   {
   }
 
   Result Init();
 
   const SECItem* encodedBasicConstraints;
+  const SECItem* encodedExtendedKeyUsage;
   const SECItem* encodedKeyUsage;
 
   BackCert* const childCert;
 
   const CERTCertificate* GetNSSCert() const { return nssCert; }
 
   // This is the only place where we should be dealing with non-const
   // CERTCertificates.