bug 1057123 - mozilla::pkix: allow end-entity certificates to assert keyCertSign in some cases r=briansmith

security/manager/ssl/tests/unit/test_certificate_usages.js

@@ -27,7 +27,6 @@ function run_test() {
     var ca_name = "ca-" + (i + 1);
     var ca_filename = ca_name + ".der";
     addCertFromFile(certdb, "test_certificate_usages/" + ca_filename, "CTu,CTu,CTu");
-    do_print("ca_name=" + ca_name);
     var cert = certdb.findCertByNickname(null, ca_name);
   }
 
@@ -46,6 +45,7 @@ function run_test() {
   // validated for any other usage.
   var ee_usages = [
     [ basicEndEntityUsages,
+      basicEndEntityUsages,
       basicEndEntityUsages,
       basicEndEntityUsages,
       '',
@@ -56,6 +56,7 @@ function run_test() {
     ],
 
     [ basicEndEntityUsages,
+      basicEndEntityUsages,
       basicEndEntityUsages,
       basicEndEntityUsages,
       '',
@@ -66,6 +67,7 @@ function run_test() {
     ],
 
     [ basicEndEntityUsages,
+      basicEndEntityUsages,
       basicEndEntityUsages,
       basicEndEntityUsages,
       '',
@@ -88,6 +90,7 @@ function run_test() {
       '',
       '',
       '',
+      '',
       ''
      ]
   ];
@@ -100,7 +103,6 @@ function run_test() {
     var usages = {};
     var cert = certdb.findCertByNickname(null, ca_name);
     cert.getUsagesString(true, verified, usages);
-    do_print("usages.value=" + usages.value);
     do_check_eq(ca_usages[i], usages.value);
     if (ca_usages[i].indexOf('SSL CA') != -1) {
       checkCertErrorGeneric(certdb, cert, 0, certificateUsageVerifyCA);
@@ -109,14 +111,12 @@ function run_test() {
     for (var j = 0; j < ee_usages[i].length; j++) {
       var ee_name = "ee-" + (j + 1) + "-" + ca_name;
       var ee_filename = ee_name + ".der";
-      //do_print("ee_filename" + ee_filename);
       addCertFromFile(certdb, "test_certificate_usages/" + ee_filename, ",,");
       var ee_cert;
       ee_cert = certdb.findCertByNickname(null, ee_name);
       var verified = {};
       var usages = {};
       ee_cert.getUsagesString(true, verified, usages);
-      do_print("cert usages.value=" + usages.value);
       do_check_eq(ee_usages[i][j], usages.value);
     }
   }

security/manager/ssl/tests/unit/test_certificate_usages/generate.pl

@@ -24,6 +24,7 @@ my @base_usages=("",
 my @ee_usages=("",
                "digitalSignature,keyEncipherment,dataEncipherment",
                "digitalSignature,nonRepudiation,keyEncipherment,dataEncipherment,keyAgreement",
+               "digitalSignature,keyEncipherment,dataEncipherment,certSigning",
                "certSigning");
 my @eku_usages=("serverAuth,clientAuth,codeSigning,emailProtection,timeStamp,ocspResponder,stepUp,msTrustListSign",
                 "serverAuth,clientAuth",

security/manager/ssl/tests/unit/test_intermediate_basic_usage_constraints.js

@@ -60,12 +60,19 @@ function run_test() {
   // it.
   test_cert_for_usages(["ee-int-no-extensions", "int-no-extensions"], "");
 
-  // a certificate with bsaicConstraints.cA==false is considered an EE.
+  // a certificate with basicConstraints.cA==false is considered an EE.
   test_cert_for_usages(["int-not-a-ca"], ee_usage1);
 
   // int-not-a-ca is an EE (see previous case), so no certs can chain to it.
   test_cert_for_usages(["ee-int-not-a-ca", "int-not-a-ca"], "");
 
+  // a certificate with basicConstraints.cA==false but with the keyCertSign
+  // key usage may not act as a CA (it can act like an end-entity).
+  test_cert_for_usages(["int-cA-FALSE-asserts-keyCertSign"], ee_usage1);
+  test_cert_for_usages(["ee-int-cA-FALSE-asserts-keyCertSign",
+                        "int-cA-FALSE-asserts-keyCertSign"], "");
+
+
   // int-limited-depth has cA==true and a path length constraint of zero.
   test_cert_for_usages(["int-limited-depth"], ca_usage1);
 

security/manager/ssl/tests/unit/test_intermediate_basic_usage_constraints/generate.py

@@ -23,6 +23,7 @@ EE_basic_constraints = "basicConstraints = CA:FALSE\n"
 
 CA_min_ku = "keyUsage = critical, keyCertSign\n"
 CA_bad_ku = "keyUsage = digitalSignature, nonRepudiation, keyEncipherment, dataEncipherment, keyAgreement, cRLSign\n"
+all_ku = "keyUsage = digitalSignature, nonRepudiation, keyEncipherment, dataEncipherment, keyAgreement, keyCertSign, cRLSign\n"
 EE_full_ku ="keyUsage = digitalSignature, nonRepudiation, keyEncipherment, dataEncipherment, keyAgreement\n"
 
 Server_eku = "extendedKeyUsage = critical, serverAuth, clientAuth\n "
@@ -35,13 +36,13 @@ def generate_int_and_ee2(ca_key, ca_cert, suffix, int_ext_text, ee_ext_text):
     int_serial = random.randint(100, 40000000);
     ee_serial = random.randint(100, 40000000);
     [int_key, int_cert] = CertUtils.generate_cert_generic(db,
-                                                        srcdir,
-                                                        int_serial,
-                                                        key_type,
-                                                        int_name,
-                                                        int_ext_text,
-                                                        ca_key,
-                                                        ca_cert);
+                                                          srcdir,
+                                                          int_serial,
+                                                          key_type,
+                                                          int_name,
+                                                          int_ext_text,
+                                                          ca_key,
+                                                          ca_cert);
     [ee_key, ee_cert] = CertUtils.generate_cert_generic(db,
                                                         srcdir,
                                                         ee_serial,
@@ -66,6 +67,8 @@ def generate_certs():
     generate_int_and_ee2(ca_key, ca_cert, "no-extensions", "", ee_ext_text)
     generate_int_and_ee2(ca_key, ca_cert, "not-a-ca", EE_basic_constraints,
                          ee_ext_text)
+    generate_int_and_ee2(ca_key, ca_cert, "cA-FALSE-asserts-keyCertSign",
+                         EE_basic_constraints + all_ku, ee_ext_text)
 
     [int_key, int_cert, a, b] = generate_int_and_ee2(ca_key, ca_cert,
                                                      "limited-depth",

security/pkix/lib/pkixcheck.cpp

@@ -148,14 +148,17 @@ CheckKeyUsage(EndEntityOrCA endEntityOrCA, const Input* encodedKeyUsage,
     }
   }
 
-  if (endEntityOrCA != EndEntityOrCA::MustBeCA) {
-    // RFC 5280 says "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."
-    if ((bits & KeyUsageToBitMask(KeyUsage::keyCertSign)) != 0) {
-      return Result::ERROR_INADEQUATE_KEY_USAGE;
-    }
+  // RFC 5280 says "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."
+  // However, we allow end-entity certificates (i.e. certificates without
+  // basicConstraints.cA set to TRUE) to claim keyCertSign for compatibility
+  // reasons. This does not compromise security because we only allow
+  // certificates with basicConstraints.cA set to TRUE to act as CAs.
+  if (requiredKeyUsageIfPresent == KeyUsage::keyCertSign &&
+      endEntityOrCA != EndEntityOrCA::MustBeCA) {
+    return Result::ERROR_INADEQUATE_KEY_USAGE;
   }
 
   // The padding applies to the last byte, so skip to the last byte.

security/pkix/test/gtest/pkixcheck_CheckKeyUsage_tests.cpp

@@ -195,7 +195,9 @@ TEST_F(pkixcheck_CheckKeyUsage, simpleCases)
   ASSERT_SimpleCase(3, 0x08, KeyUsage::keyAgreement);
 }
 
-// Only CAs are allowed to assert keyCertSign
+// Only CAs are allowed to assert keyCertSign.
+// End-entity certs may assert it along with other key usages if keyCertSign
+// isn't the required key usage. This is for compatibility.
 TEST_F(pkixcheck_CheckKeyUsage, keyCertSign)
 {
   NAMED_SIMPLE_KU(good, 2, 0x04);
@@ -222,6 +224,16 @@ TEST_F(pkixcheck_CheckKeyUsage, keyCertSign)
                            KeyUsage::keyCertSign));
   ASSERT_BAD(CheckKeyUsage(EndEntityOrCA::MustBeCA, &twoByteNotGood,
                            KeyUsage::keyCertSign));
+
+  // If an end-entity certificate does assert keyCertSign, this is allowed
+  // as long as that isn't the required key usage.
+  NAMED_SIMPLE_KU(digitalSignatureAndKeyCertSign, 2, 0x84);
+  ASSERT_EQ(Success, CheckKeyUsage(EndEntityOrCA::MustBeEndEntity,
+                                   &digitalSignatureAndKeyCertSign,
+                                   KeyUsage::digitalSignature));
+  ASSERT_BAD(CheckKeyUsage(EndEntityOrCA::MustBeEndEntity,
+                           &digitalSignatureAndKeyCertSign,
+                           KeyUsage::keyCertSign));
 }
 
 TEST_F(pkixcheck_CheckKeyUsage, unusedBitNotZero)