Bug 921893: Verify key usage extension in insanity::pkix, r=keeler, r=cviecco

--HG--
extra : rebase_source : 1ea7626ed36cd92cdbf2aea5533e983fc0f58a44

security/insanity/lib/pkixbuild.cpp

@@ -50,6 +50,7 @@ BackCert::Init()
       // { 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
       }
@@ -89,6 +90,7 @@ static Result BuildForward(TrustDomain& trustDomain,
                            BackCert& subject,
                            PRTime time,
                            EndEntityOrCA endEntityOrCA,
+                           KeyUsages requiredKeyUsagesIfPresent,
                            unsigned int subCACount,
                            /*out*/ ScopedCERTCertList& results);
 
@@ -140,6 +142,7 @@ BuildForwardInner(TrustDomain& trustDomain,
   }
 
   rv = BuildForward(trustDomain, potentialIssuer, time, MustBeCA,
+                    KU_KEY_CERT_SIGN,
                     newSubCACount, results);
   if (rv != Success) {
     return rv;
@@ -159,6 +162,7 @@ BuildForward(TrustDomain& trustDomain,
              BackCert& subject,
              PRTime time,
              EndEntityOrCA endEntityOrCA,
+             KeyUsages requiredKeyUsagesIfPresent,
              unsigned int subCACount,
              /*out*/ ScopedCERTCertList& results)
 {
@@ -187,6 +191,7 @@ BuildForward(TrustDomain& trustDomain,
 
   rv = CheckExtensions(subject, endEntityOrCA,
                        trustLevel == TrustDomain::TrustAnchor,
+                       requiredKeyUsagesIfPresent,
                        subCACount);
   if (rv != Success) {
     return rv;
@@ -235,6 +240,7 @@ SECStatus
 BuildCertChain(TrustDomain& trustDomain,
                CERTCertificate* certToDup,
                PRTime time,
+               /*optional*/ KeyUsages requiredKeyUsagesIfPresent,
                /*out*/ ScopedCERTCertList& results)
 {
   PORT_Assert(certToDup);
@@ -254,6 +260,7 @@ BuildCertChain(TrustDomain& trustDomain,
   }
 
   rv = BuildForward(trustDomain, ee, time, MustBeEndEntity,
+                    requiredKeyUsagesIfPresent,
                     0, results);
   if (rv != Success) {
     results = nullptr;

security/insanity/lib/pkixcheck.cpp

@@ -36,6 +36,67 @@ CheckTimes(const CERTCertificate* cert, PRTime time)
   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,
@@ -122,6 +183,7 @@ 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
@@ -135,6 +197,13 @@ CheckExtensions(BackCert& cert,
   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

security/insanity/lib/pkixcheck.h

@@ -28,6 +28,7 @@ Result CheckTimes(const CERTCertificate* cert, PRTime time);
 Result CheckExtensions(BackCert& certExt,
                        EndEntityOrCA endEntityOrCA,
                        bool isTrustAnchor,
+                       KeyUsages requiredKeyUsagesIfPresent,
                        unsigned int depth);
 
 } } // namespace insanity::pkix