bug 1020993 - properly handle unknown critical extensions in BackCert::Init r=briansmith
authorDavid Keeler <dkeeler@mozilla.com>
Mon, 09 Jun 2014 13:57:44 -0700
changeset 187810 7950b5746773ee87a44e0e9b0e431d90ada0a861
parent 187809 86f5521e907f82df1fef53f72f8cfb90063d8932
child 187811 88fefd05fb64739808d9185fa3b9e262017c7545
push id26934
push userryanvm@gmail.com
push dateTue, 10 Jun 2014 04:45:09 +0000
treeherdermozilla-central@9dc0ffca10f4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbriansmith
bugs1020993
milestone32.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 1020993 - properly handle unknown critical extensions in BackCert::Init r=briansmith
security/pkix/lib/pkixbuild.cpp
security/pkix/test/gtest/moz.build
security/pkix/test/gtest/pkix_cert_extension_tests.cpp
--- a/security/pkix/lib/pkixbuild.cpp
+++ b/security/pkix/lib/pkixbuild.cpp
@@ -70,45 +70,53 @@ BackCert::Init(const SECItem& certDER)
   const SECItem* dummyEncodedSubjectKeyIdentifier = nullptr;
   const SECItem* dummyEncodedAuthorityKeyIdentifier = nullptr;
   const SECItem* dummyEncodedAuthorityInfoAccess = nullptr;
   const SECItem* dummyEncodedSubjectAltName = nullptr;
 
   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]) {
+    // python DottedOIDToCode.py id-ce 2.5.29
+    static const uint8_t id_ce[] = {
+      0x55, 0x1d
+    };
+
+    // python DottedOIDToCode.py id-pe-authorityInfoAccess 1.3.6.1.5.5.7.1.1
+    static const uint8_t id_pe_authorityInfoAccess[] = {
+      0x2b, 0x06, 0x01, 0x05, 0x05, 0x07, 0x01, 0x01
+    };
+
+    if (ext->id.len == PR_ARRAY_SIZE(id_ce) + 1 &&
+        !memcmp(ext->id.data, id_ce, PR_ARRAY_SIZE(id_ce))) {
+      switch (ext->id.data[ext->id.len - 1]) {
         case 14: out = &dummyEncodedSubjectKeyIdentifier; break; // bug 965136
         case 15: out = &encodedKeyUsage; break;
         case 17: out = &dummyEncodedSubjectAltName; break; // bug 970542
         case 19: out = &encodedBasicConstraints; break;
         case 30: out = &encodedNameConstraints; break;
         case 32: out = &encodedCertificatePolicies; break;
         case 35: out = &dummyEncodedAuthorityKeyIdentifier; break; // bug 965136
         case 37: out = &encodedExtendedKeyUsage; break;
         case 54: out = &encodedInhibitAnyPolicy; break; // Bug 989051
       }
-    } 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]) {
-        // We should remember the value of the encoded AIA extension here, but
-        // since our TrustDomain implementations get the OCSP URI using
-        // CERT_GetOCSPAuthorityInfoAccessLocation, we currently don't need to.
-        case 1: out = &dummyEncodedAuthorityInfoAccess; break;
-      }
-    } else if (ext->critical.data && ext->critical.len > 0) {
-      // The only valid explicit value of the critical flag is TRUE because
-      // it is defined as BOOLEAN DEFAULT FALSE, so we just assume it is true.
+    } else if (ext->id.len == PR_ARRAY_SIZE(id_pe_authorityInfoAccess) &&
+               !memcmp(ext->id.data, id_pe_authorityInfoAccess,
+                       PR_ARRAY_SIZE(id_pe_authorityInfoAccess))) {
+      // We should remember the value of the encoded AIA extension here, but
+      // since our TrustDomain implementations get the OCSP URI using
+      // CERT_GetOCSPAuthorityInfoAccessLocation, we currently don't need to.
+      out = &dummyEncodedAuthorityInfoAccess;
+    }
+
+    // If this is an extension we don't understand and it's marked critical,
+    // we must reject this certificate.
+    // (The only valid explicit value of the critical flag is TRUE because
+    // it is defined as BOOLEAN DEFAULT FALSE, so we just assume it is true.)
+    if (!out && ext->critical.data && ext->critical.len > 0) {
       return Fail(RecoverableError, SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION);
     }
 
     if (out) {
       // This is an extension we understand. Save it in results unless we've
       // already found the extension previously.
       if (*out) {
         // Duplicate extension
--- a/security/pkix/test/gtest/moz.build
+++ b/security/pkix/test/gtest/moz.build
@@ -4,16 +4,17 @@
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 LIBRARY_NAME = 'mozillapkix_gtest'
 
 SOURCES += [
     'nssgtest.cpp',
     'pkix_cert_chain_length_tests.cpp',
+    'pkix_cert_extension_tests.cpp',
     'pkix_ocsp_request_tests.cpp',
     'pkixder_input_tests.cpp',
     'pkixder_pki_types_tests.cpp',
     'pkixder_universal_types_tests.cpp',
 ]
 
 LOCAL_INCLUDES += [
     '../../include',
new file mode 100644
--- /dev/null
+++ b/security/pkix/test/gtest/pkix_cert_extension_tests.cpp
@@ -0,0 +1,319 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
+/* This code is made available to you under your choice of the following sets
+ * of licensing terms:
+ */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+/* Copyright 2013 Mozilla Contributors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "nssgtest.h"
+#include "pkix/pkix.h"
+#include "secerr.h"
+
+using namespace mozilla::pkix;
+using namespace mozilla::pkix::test;
+
+// Creates a self-signed certificate with the given extension.
+static bool
+CreateCert(PLArenaPool* arena, const char* subjectStr,
+           const SECItem* extension,
+           /*out*/ ScopedSECKEYPrivateKey& subjectKey,
+           /*out*/ ScopedCERTCertificate& subjectCert)
+{
+  static long serialNumberValue = 0;
+  ++serialNumberValue;
+  const SECItem* serialNumber(CreateEncodedSerialNumber(arena,
+                                                        serialNumberValue));
+  if (!serialNumber) {
+    return false;
+  }
+  const SECItem* issuerDER(ASCIIToDERName(arena, subjectStr));
+  if (!issuerDER) {
+    return false;
+  }
+  const SECItem* subjectDER(ASCIIToDERName(arena, subjectStr));
+  if (!subjectDER) {
+    return false;
+  }
+
+  const SECItem* extensions[2] = { extension, nullptr };
+
+  SECItem* certDER(CreateEncodedCertificate(arena, v3, SEC_OID_SHA256,
+                                            serialNumber, issuerDER,
+                                            PR_Now() - ONE_DAY,
+                                            PR_Now() + ONE_DAY,
+                                            subjectDER, extensions,
+                                            nullptr, SEC_OID_SHA256,
+                                            subjectKey));
+  if (!certDER) {
+    return false;
+  }
+  subjectCert = CERT_NewTempCertificate(CERT_GetDefaultCertDB(), certDER,
+                                        nullptr, false, true);
+  return subjectCert.get() != nullptr;
+}
+
+class TrustEverythingTrustDomain : public TrustDomain
+{
+private:
+  SECStatus GetCertTrust(EndEntityOrCA,
+                         const CertPolicyId&,
+                         const SECItem& candidateCert,
+                         /*out*/ TrustLevel* trustLevel)
+  {
+    *trustLevel = TrustLevel::TrustAnchor;
+    return SECSuccess;
+  }
+
+  SECStatus FindPotentialIssuers(const SECItem* encodedIssuerName,
+                                 PRTime time,
+                                 /*out*/ ScopedCERTCertList& results)
+  {
+    return SECSuccess;
+  }
+
+  SECStatus VerifySignedData(const CERTSignedData* signedData,
+                             const SECItem& subjectPublicKeyInfo)
+  {
+    return ::mozilla::pkix::VerifySignedData(signedData, subjectPublicKeyInfo,
+                                             nullptr);
+  }
+
+  SECStatus CheckRevocation(EndEntityOrCA, const CERTCertificate*,
+                            /*const*/ CERTCertificate*, PRTime,
+                            /*optional*/ const SECItem*)
+  {
+    return SECSuccess;
+  }
+
+  virtual SECStatus IsChainValid(const CERTCertList*)
+  {
+    return SECSuccess;
+  }
+};
+
+class pkix_cert_extensions: public NSSTest
+{
+public:
+  static void SetUpTestCase()
+  {
+    NSSTest::SetUpTestCase();
+  }
+
+protected:
+  static TrustEverythingTrustDomain trustDomain;
+};
+
+/*static*/ TrustEverythingTrustDomain pkix_cert_extensions::trustDomain;
+
+// Tests that a critical extension not in the id-ce or id-pe arcs (which is
+// thus unknown to us) is detected and that verification fails with the
+// appropriate error.
+TEST_F(pkix_cert_extensions, UnknownCriticalExtension)
+{
+  static const uint8_t unknownCriticalExtensionBytes[] = {
+    0x30, 0x19, // SEQUENCE (length = 25)
+      0x06, 0x12, // OID (length = 18)
+        // 1.3.6.1.4.1.13769.666.666.666.1.500.9.3
+        0x2b, 0x06, 0x01, 0x04, 0x01, 0xeb, 0x49, 0x85, 0x1a,
+        0x85, 0x1a, 0x85, 0x1a, 0x01, 0x83, 0x74, 0x09, 0x03,
+      0x01, 0x01, 0xff, // BOOLEAN (length = 1) TRUE
+      0x04, 0x00 // OCTET STRING (length = 0)
+  };
+  static const SECItem unknownCriticalExtension = {
+    siBuffer,
+    const_cast<unsigned char*>(unknownCriticalExtensionBytes),
+    sizeof(unknownCriticalExtensionBytes)
+  };
+  const char* certCN = "CN=Cert With Unknown Critical Extension";
+  ScopedSECKEYPrivateKey key;
+  ScopedCERTCertificate cert;
+  ASSERT_TRUE(CreateCert(arena.get(), certCN, &unknownCriticalExtension, key,
+                         cert));
+  ScopedCERTCertList results;
+  ASSERT_SECFailure(SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION,
+                    BuildCertChain(trustDomain, cert.get(),
+                                   now, EndEntityOrCA::MustBeEndEntity,
+                                   0, // key usage
+                                   KeyPurposeId::anyExtendedKeyUsage,
+                                   CertPolicyId::anyPolicy,
+                                   nullptr, // stapled OCSP response
+                                   results));
+}
+
+// Tests that a non-critical extension not in the id-ce or id-pe arcs (which is
+// thus unknown to us) verifies successfully.
+TEST_F(pkix_cert_extensions, UnknownNonCriticalExtension)
+{
+  static const uint8_t unknownNonCriticalExtensionBytes[] = {
+    0x30, 0x16, // SEQUENCE (length = 22)
+      0x06, 0x12, // OID (length = 18)
+        // 1.3.6.1.4.1.13769.666.666.666.1.500.9.3
+        0x2b, 0x06, 0x01, 0x04, 0x01, 0xeb, 0x49, 0x85, 0x1a,
+        0x85, 0x1a, 0x85, 0x1a, 0x01, 0x83, 0x74, 0x09, 0x03,
+      0x04, 0x00 // OCTET STRING (length = 0)
+  };
+  static const SECItem unknownNonCriticalExtension = {
+    siBuffer,
+    const_cast<unsigned char*>(unknownNonCriticalExtensionBytes),
+    sizeof(unknownNonCriticalExtensionBytes)
+  };
+  const char* certCN = "CN=Cert With Unknown NonCritical Extension";
+  ScopedSECKEYPrivateKey key;
+  ScopedCERTCertificate cert;
+  ASSERT_TRUE(CreateCert(arena.get(), certCN, &unknownNonCriticalExtension, key,
+                         cert));
+  ScopedCERTCertList results;
+  ASSERT_SECSuccess(BuildCertChain(trustDomain, cert.get(),
+                                   now, EndEntityOrCA::MustBeEndEntity,
+                                   0, // key usage
+                                   KeyPurposeId::anyExtendedKeyUsage,
+                                   CertPolicyId::anyPolicy,
+                                   nullptr, // stapled OCSP response
+                                   results));
+}
+
+// Tests that an incorrect OID for id-pe-authorityInformationAccess
+// (when marked critical) is detected and that verification fails.
+// (Until bug 1020993 was fixed, the code checked for this OID.)
+TEST_F(pkix_cert_extensions, WrongOIDCriticalExtension)
+{
+  static const uint8_t wrongOIDCriticalExtensionBytes[] = {
+    0x30, 0x10, // SEQUENCE (length = 16)
+      0x06, 0x09, // OID (length = 9)
+        // 1.3.6.6.1.5.5.7.1.1 (there is an extra "6" that shouldn't be there)
+        0x2b, 0x06, 0x06, 0x01, 0x05, 0x05, 0x07, 0x01, 0x01,
+      0x01, 0x01, 0xff, // BOOLEAN (length = 1) TRUE
+      0x04, 0x00 // OCTET STRING (length = 0)
+  };
+  static const SECItem wrongOIDCriticalExtension = {
+    siBuffer,
+    const_cast<unsigned char*>(wrongOIDCriticalExtensionBytes),
+    sizeof(wrongOIDCriticalExtensionBytes)
+  };
+  const char* certCN = "CN=Cert With Critical Wrong OID Extension";
+  ScopedSECKEYPrivateKey key;
+  ScopedCERTCertificate cert;
+  ASSERT_TRUE(CreateCert(arena.get(), certCN, &wrongOIDCriticalExtension, key,
+                         cert));
+  ScopedCERTCertList results;
+  ASSERT_SECFailure(SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION,
+                    BuildCertChain(trustDomain, cert.get(),
+                                   now, EndEntityOrCA::MustBeEndEntity,
+                                   0, // key usage
+                                   KeyPurposeId::anyExtendedKeyUsage,
+                                   CertPolicyId::anyPolicy,
+                                   nullptr, // stapled OCSP response
+                                   results));
+}
+
+// Tests that a id-pe-authorityInformationAccess critical extension
+// is detected and that verification succeeds.
+TEST_F(pkix_cert_extensions, CriticalAIAExtension)
+{
+  static const uint8_t criticalAIAExtensionBytes[] = {
+    0x30, 0x0f, // SEQUENCE (length = 15)
+      0x06, 0x08, // OID (length = 8)
+        // 1.3.6.1.5.5.7.1.1
+        0x2b, 0x06, 0x01, 0x05, 0x05, 0x07, 0x01, 0x01,
+      0x01, 0x01, 0xff, // BOOLEAN (length = 1) TRUE
+      0x04, 0x00 // OCTET STRING (length = 0)
+  };
+  static const SECItem criticalAIAExtension = {
+    siBuffer,
+    const_cast<unsigned char*>(criticalAIAExtensionBytes),
+    sizeof(criticalAIAExtensionBytes)
+  };
+  const char* certCN = "CN=Cert With Critical AIA Extension";
+  ScopedSECKEYPrivateKey key;
+  ScopedCERTCertificate cert;
+  ASSERT_TRUE(CreateCert(arena.get(), certCN, &criticalAIAExtension, key,
+                         cert));
+  ScopedCERTCertList results;
+  ASSERT_SECSuccess(BuildCertChain(trustDomain, cert.get(),
+                                   now, EndEntityOrCA::MustBeEndEntity,
+                                   0, // key usage
+                                   KeyPurposeId::anyExtendedKeyUsage,
+                                   CertPolicyId::anyPolicy,
+                                   nullptr, // stapled OCSP response
+                                   results));
+}
+
+// We know about some id-ce extensions (OID arc 2.5.29), but not all of them.
+// Tests that an unknown id-ce extension is detected and that verification
+// fails.
+TEST_F(pkix_cert_extensions, UnknownCriticalCEExtension)
+{
+  static const uint8_t unknownCriticalCEExtensionBytes[] = {
+    0x30, 0x0a, // SEQUENCE (length = 10)
+      0x06, 0x03, // OID (length = 3)
+        0x55, 0x1d, 0x37, // 2.5.29.55
+      0x01, 0x01, 0xff, // BOOLEAN (length = 1) TRUE
+      0x04, 0x00 // OCTET STRING (length = 0)
+  };
+  static const SECItem unknownCriticalCEExtension = {
+    siBuffer,
+    const_cast<unsigned char*>(unknownCriticalCEExtensionBytes),
+    sizeof(unknownCriticalCEExtensionBytes)
+  };
+  const char* certCN = "CN=Cert With Unknown Critical id-ce Extension";
+  ScopedSECKEYPrivateKey key;
+  ScopedCERTCertificate cert;
+  ASSERT_TRUE(CreateCert(arena.get(), certCN, &unknownCriticalCEExtension, key,
+                         cert));
+  ScopedCERTCertList results;
+  ASSERT_SECFailure(SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION,
+                    BuildCertChain(trustDomain, cert.get(),
+                                   now, EndEntityOrCA::MustBeEndEntity,
+                                   0, // key usage
+                                   KeyPurposeId::anyExtendedKeyUsage,
+                                   CertPolicyId::anyPolicy,
+                                   nullptr, // stapled OCSP response
+                                   results));
+}
+
+// Tests that a certificate with a known critical id-ce extension (in this case,
+// OID 2.5.29.54, which is id-ce-inhibitAnyPolicy), verifies successfully.
+TEST_F(pkix_cert_extensions, KnownCriticalCEExtension)
+{
+  static const uint8_t criticalCEExtensionBytes[] = {
+    0x30, 0x0a, // SEQUENCE (length = 10)
+      0x06, 0x03, // OID (length = 3)
+        0x55, 0x1d, 0x36, // 2.5.29.54 (id-ce-inhibitAnyPolicy)
+      0x01, 0x01, 0xff, // BOOLEAN (length = 1) TRUE
+      0x04, 0x00 // OCTET STRING (length = 0)
+  };
+  static const SECItem criticalCEExtension = {
+    siBuffer,
+    const_cast<unsigned char*>(criticalCEExtensionBytes),
+    sizeof(criticalCEExtensionBytes)
+  };
+  const char* certCN = "CN=Cert With Known Critical id-ce Extension";
+  ScopedSECKEYPrivateKey key;
+  ScopedCERTCertificate cert;
+  ASSERT_TRUE(CreateCert(arena.get(), certCN, &criticalCEExtension, key, cert));
+  ScopedCERTCertList results;
+  ASSERT_SECSuccess(BuildCertChain(trustDomain, cert.get(),
+                                   now, EndEntityOrCA::MustBeEndEntity,
+                                   0, // key usage
+                                   KeyPurposeId::anyExtendedKeyUsage,
+                                   CertPolicyId::anyPolicy,
+                                   nullptr, // stapled OCSP response
+                                   results));
+}