Bug 921893: Verify key usage extension in insanity::pkix, r=keeler, r=cviecco, a=sledru
authorBrian Smith <brian@briansmith.org>
Tue, 28 Jan 2014 23:20:11 -0800
changeset 182825 d61793abbcbef4dcaab9c4863e2db69346635318
parent 182824 ac6d341c6d172be14cfb8531e5665bdbacb59c35
child 182826 b528490f256045a3a5fd9398cc977c8e1f3e733c
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
bugs921893
milestone29.0a2
Bug 921893: Verify key usage extension in insanity::pkix, r=keeler, r=cviecco, a=sledru
security/insanity/lib/pkixbuild.cpp
security/insanity/lib/pkixcheck.cpp
security/insanity/lib/pkixcheck.h
--- a/security/insanity/lib/pkixbuild.cpp
+++ b/security/insanity/lib/pkixbuild.cpp
@@ -45,16 +45,17 @@ BackCert::Init()
   for (const CERTCertExtension* ext = *exts; ext; ext = *++exts) {
     const SECItem** out = nullptr;
 
     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
       }
     } 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) {
@@ -84,16 +85,17 @@ BackCert::Init()
 
   return Success;
 }
 
 static Result BuildForward(TrustDomain& trustDomain,
                            BackCert& subject,
                            PRTime time,
                            EndEntityOrCA endEntityOrCA,
+                           KeyUsages requiredKeyUsagesIfPresent,
                            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,
@@ -135,16 +137,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,
                     newSubCACount, results);
   if (rv != Success) {
     return rv;
   }
 
   if (trustDomain.VerifySignedData(&subject.GetNSSCert()->signatureWrap,
                                    potentialIssuer.GetNSSCert()) != SECSuccess) {
     return MapSECStatus(SECFailure);
@@ -154,16 +157,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,
              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;
@@ -182,16 +186,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,
                        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.
@@ -230,16 +235,17 @@ BuildForward(TrustDomain& trustDomain,
 
   return Fail(RecoverableError, SEC_ERROR_UNKNOWN_ISSUER);
 }
 
 SECStatus
 BuildCertChain(TrustDomain& trustDomain,
                CERTCertificate* certToDup,
                PRTime time,
+               /*optional*/ KeyUsages requiredKeyUsagesIfPresent,
                /*out*/ ScopedCERTCertList& results)
 {
   PORT_Assert(certToDup);
 
   if (!certToDup) {
     PR_SetError(SEC_ERROR_INVALID_ARGS, 0);
     return SECFailure;
   }
@@ -249,16 +255,17 @@ BuildCertChain(TrustDomain& trustDomain,
 
   BackCert ee(certToDup, nullptr);
   Result rv = ee.Init();
   if (rv != Success) {
     return SECFailure;
   }
 
   rv = BuildForward(trustDomain, ee, time, MustBeEndEntity,
+                    requiredKeyUsagesIfPresent,
                     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
@@ -31,16 +31,77 @@ CheckTimes(const CERTCertificate* cert, 
   SECCertTimeValidity validity = CERT_CheckCertValidTimes(cert, time, false);
   if (validity != secCertTimeValid) {
     return Fail(RecoverableError, SEC_ERROR_EXPIRED_CERTIFICATE);
   }
 
   return Success;
 }
 
+// 4.2.1.3. Key Usage (id-ce-keyUsage)
+// Modeled after GetKeyUsage in certdb.c
+Result
+CheckKeyUsage(EndEntityOrCA endEntityOrCA,
+              bool isTrustAnchor,
+              const SECItem* encodedKeyUsage,
+              KeyUsages requiredKeyUsagesIfPresent,
+              PLArenaPool* arena)
+{
+  if (!encodedKeyUsage) {
+    // TODO: Reject certificates that are being used to verify certificate
+    // signatures unless the certificate is a trust anchor, to reduce the
+    // chances of an end-entity certificate being abused as a CA certificate.
+    // if (endEntityOrCA == MustBeCA && !isTrustAnchor) {
+    //   return Fail(RecoverableError, SEC_ERROR_INADEQUATE_KEY_USAGE);
+    // }
+    //
+    // TODO: Users may configure arbitrary certificates as trust anchors, not
+    // just roots. We should only allow a certificate without a key usage to be
+    // used as a CA when it is self-issued and self-signed.
+    return Success;
+  }
+
+  SECItem tmpItem;
+  Result rv = MapSECStatus(SEC_QuickDERDecodeItem(arena, &tmpItem,
+                              SEC_ASN1_GET(SEC_BitStringTemplate),
+                              encodedKeyUsage));
+  if (rv != Success) {
+    return rv;
+  }
+
+  // TODO XXX: Why is tmpItem.len > 1?
+
+  KeyUsages allowedKeyUsages = tmpItem.data[0];
+  if ((allowedKeyUsages & requiredKeyUsagesIfPresent)
+        != requiredKeyUsagesIfPresent) {
+    return Fail(RecoverableError, SEC_ERROR_INADEQUATE_KEY_USAGE);
+  }
+
+  if (endEntityOrCA == MustBeCA) {
+   // "If the keyUsage extension is present, then the subject public key
+   //  MUST NOT be used to verify signatures on certificates or CRLs unless
+   //  the corresponding keyCertSign or cRLSign bit is set."
+   if ((allowedKeyUsages & KU_KEY_CERT_SIGN) == 0) {
+      return Fail(RecoverableError, SEC_ERROR_INADEQUATE_KEY_USAGE);
+    }
+  } else {
+    // "The keyCertSign bit is asserted when the subject public key is
+    //  used for verifying signatures on public key certificates.  If the
+    //  keyCertSign bit is asserted, then the cA bit in the basic
+    //  constraints extension (Section 4.2.1.9) MUST also be asserted."
+    // TODO XXX: commented out to match classic NSS behavior.
+    //if ((allowedKeyUsages & KU_KEY_CERT_SIGN) != 0) {
+    //  // XXX: better error code.
+    //  return Fail(RecoverableError, SEC_ERROR_INADEQUATE_CERT_TYPE);
+    //}
+  }
+
+  return Success;
+}
+
 // RFC5280 4.2.1.9. Basic Constraints (id-ce-basicConstraints)
 Result
 CheckBasicConstraints(const BackCert& cert,
                       EndEntityOrCA endEntityOrCA,
                       bool isTrustAnchor,
                       unsigned int subCACount)
 {
   CERTBasicConstraints basicConstraints;
@@ -117,29 +178,37 @@ CheckBasicConstraints(const BackCert& ce
 }
 
 // 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,
                 unsigned int subCACount)
 {
   // 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;
   }
 
   Result rv;
 
   // 4.2.1.3. Key Usage
+
+  rv = CheckKeyUsage(endEntityOrCA, isTrustAnchor, cert.encodedKeyUsage,
+                     requiredKeyUsagesIfPresent, arena);
+  if (rv != Success) {
+    return rv;
+  }
+
   // 4.2.1.4. Certificate Policies
   // 4.2.1.5. Policy Mappings are rejected in BackCert::Init()
   // 4.2.1.6. Subject Alternative Name dealt with elsewhere
   // 4.2.1.7. Issuer Alternative Name is not something that needs checking
   // 4.2.1.8. Subject Directory Attributes is not something that needs checking
 
   // 4.2.1.9. Basic Constraints. We check basic constraints before other
   // properties that take endEntityOrCA so that those checks can use
--- a/security/insanity/lib/pkixcheck.h
+++ b/security/insanity/lib/pkixcheck.h
@@ -23,13 +23,14 @@
 
 namespace insanity { namespace pkix {
 
 Result CheckTimes(const CERTCertificate* cert, PRTime time);
 
 Result CheckExtensions(BackCert& certExt,
                        EndEntityOrCA endEntityOrCA,
                        bool isTrustAnchor,
+                       KeyUsages requiredKeyUsagesIfPresent,
                        unsigned int depth);
 
 } } // namespace insanity::pkix
 
 #endif // insanity__pkixcheck_h