Bug 1001188: Set the error code when the max cert chain length limit is exceeded, r=cviecco
authorBrian Smith <brian@briansmith.org>
Sat, 31 May 2014 16:55:54 -0700
changeset 205454 a24472ea29d4554271b97075031f7b5bc131557e
parent 205453 d03cc4b6832c1dd5a2a7703d98e600b582796b21
child 205455 9b0804cee835153ae33bb01d22bff8a0a107f107
push id3741
push userasasaki@mozilla.com
push dateMon, 21 Jul 2014 20:25:18 +0000
treeherdermozilla-beta@4d6f46f5af68 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscviecco
bugs1001188
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 1001188: Set the error code when the max cert chain length limit is exceeded, r=cviecco
security/pkix/lib/pkixbuild.cpp
security/pkix/test/gtest/moz.build
security/pkix/test/gtest/pkix_cert_chain_length_tests.cpp
security/pkix/test/lib/pkixtestutil.cpp
security/pkix/test/lib/pkixtestutil.h
--- a/security/pkix/lib/pkixbuild.cpp
+++ b/security/pkix/lib/pkixbuild.cpp
@@ -201,17 +201,17 @@ BuildForward(TrustDomain& trustDomain,
              /*optional*/ const SECItem* stapledOCSPResponse,
              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;
+    return Fail(RecoverableError, SEC_ERROR_UNKNOWN_ISSUER);
   }
 
   Result rv;
 
   TrustLevel trustLevel;
   // If this is an end-entity and not a trust anchor, we defer reporting
   // any error found here until after attempting to find a valid chain.
   // See the explanation of error prioritization in pkix.h.
--- a/security/pkix/test/gtest/moz.build
+++ b/security/pkix/test/gtest/moz.build
@@ -3,16 +3,17 @@
 # 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/.
 
 LIBRARY_NAME = 'mozillapkix_gtest'
 
 SOURCES += [
     'nssgtest.cpp',
+    'pkix_cert_chain_length_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_chain_length_tests.cpp
@@ -0,0 +1,250 @@
+/* -*- 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 "prinit.h"
+#include "secerr.h"
+
+using namespace mozilla::pkix;
+using namespace mozilla::pkix::test;
+
+static bool
+CreateCert(PLArenaPool* arena, const char* issuerStr,
+           const char* subjectStr, EndEntityOrCA endEntityOrCA,
+           /*optional*/ SECKEYPrivateKey* issuerKey,
+           /*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, issuerStr));
+  if (!issuerDER) {
+    return false;
+  }
+  const SECItem* subjectDER(ASCIIToDERName(arena, subjectStr));
+  if (!subjectDER) {
+    return false;
+  }
+
+  const SECItem* extensions[2] = { nullptr, nullptr };
+  if (endEntityOrCA == EndEntityOrCA::MustBeCA) {
+    extensions[0] =
+      CreateEncodedBasicConstraints(arena, true, nullptr,
+                                    ExtensionCriticality::Critical);
+    if (!extensions[0]) {
+      return false;
+    }
+  }
+
+  SECItem* certDER(CreateEncodedCertificate(arena, v3, SEC_OID_SHA256,
+                                            serialNumber, issuerDER,
+                                            PR_Now() - ONE_DAY,
+                                            PR_Now() + ONE_DAY,
+                                            subjectDER, extensions,
+                                            issuerKey, SEC_OID_SHA256,
+                                            subjectKey));
+  if (!certDER) {
+    return false;
+  }
+  subjectCert = CERT_NewTempCertificate(CERT_GetDefaultCertDB(), certDER,
+                                        nullptr, false, true);
+  return subjectCert.get() != nullptr;
+}
+
+class TestTrustDomain : public TrustDomain
+{
+public:
+  // The "cert chain tail" is a longish chain of certificates that is used by
+  // all of the tests here. We share this chain across all the tests in order
+  // to speed up the tests (generating keypairs for the certs is very slow).
+  bool SetUpCertChainTail()
+  {
+    static char const* const names[] = {
+        "CN=CA1 (Root)", "CN=CA2", "CN=CA3", "CN=CA4", "CN=CA5", "CN=CA6",
+        "CN=CA7"
+    };
+
+    static_assert(PR_ARRAY_SIZE(names) == PR_ARRAY_SIZE(certChainTail),
+                  "mismatch in sizes of names and certChainTail arrays");
+
+    ScopedPLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
+    if (!arena) {
+      return false;
+    }
+
+    for (size_t i = 0; i < PR_ARRAY_SIZE(names); ++i) {
+      const char* issuerName = i == 0 ? names[0]
+                                      : certChainTail[i - 1]->subjectName;
+      if (!CreateCert(arena.get(), issuerName, names[i],
+                      EndEntityOrCA::MustBeCA, leafCAKey.get(), leafCAKey,
+                      certChainTail[i])) {
+        return false;
+      }
+    }
+
+    return true;
+  }
+
+private:
+  SECStatus GetCertTrust(EndEntityOrCA,
+                         const CertPolicyId&,
+                         const CERTCertificate* candidateCert,
+                         /*out*/ TrustLevel* trustLevel)
+  {
+    if (candidateCert == certChainTail[0].get()) {
+      *trustLevel = TrustLevel::TrustAnchor;
+    } else {
+      *trustLevel = TrustLevel::InheritsTrust;
+    }
+    return SECSuccess;
+  }
+
+  SECStatus FindPotentialIssuers(const SECItem* encodedIssuerName,
+                                 PRTime time,
+                                 /*out*/ ScopedCERTCertList& results)
+  {
+    results = CERT_CreateSubjectCertList(nullptr, CERT_GetDefaultCertDB(),
+                                         encodedIssuerName, time, true);
+    return SECSuccess;
+  }
+
+  SECStatus VerifySignedData(const CERTSignedData* signedData,
+                             const CERTCertificate* cert)
+  {
+    return ::mozilla::pkix::VerifySignedData(signedData, cert, nullptr);
+  }
+
+  SECStatus CheckRevocation(EndEntityOrCA, const CERTCertificate*,
+                            /*const*/ CERTCertificate*, PRTime,
+                            /*optional*/ const SECItem*)
+  {
+    return SECSuccess;
+  }
+
+  virtual SECStatus IsChainValid(const CERTCertList*)
+  {
+    return SECSuccess;
+  }
+
+  // We hold references to CERTCertificates in the cert chain tail so that we
+  // CERT_CreateSubjectCertList can find them.
+  ScopedCERTCertificate certChainTail[7];
+
+public:
+  ScopedSECKEYPrivateKey leafCAKey;
+  CERTCertificate* GetLeafeCACert() const
+  {
+    return certChainTail[PR_ARRAY_SIZE(certChainTail) - 1].get();
+  }
+};
+
+class pkix_cert_chain_length : public NSSTest
+{
+public:
+  static void SetUpTestCase()
+  {
+    NSSTest::SetUpTestCase();
+    // Initialize the tail of the cert chains we'll be using once, to make the
+    // tests run faster (generating the keys is slow).
+    if (!trustDomain.SetUpCertChainTail()) {
+      PR_Abort();
+    }
+  }
+
+protected:
+  static TestTrustDomain trustDomain;
+};
+
+/*static*/ TestTrustDomain pkix_cert_chain_length::trustDomain;
+
+TEST_F(pkix_cert_chain_length, MaxAcceptableCertChainLength)
+{
+  ScopedCERTCertList results;
+  ASSERT_SECSuccess(BuildCertChain(trustDomain, trustDomain.GetLeafeCACert(),
+                                   now, EndEntityOrCA::MustBeCA,
+                                   0, // key usage
+                                   KeyPurposeId::id_kp_serverAuth,
+                                   CertPolicyId::anyPolicy,
+                                   nullptr, // stapled OCSP response
+                                   results));
+
+  ScopedSECKEYPrivateKey privateKey;
+  ScopedCERTCertificate cert;
+  ASSERT_TRUE(CreateCert(arena.get(),
+                         trustDomain.GetLeafeCACert()->subjectName,
+                         "CN=Direct End-Entity",
+                         EndEntityOrCA::MustBeEndEntity,
+                         trustDomain.leafCAKey.get(), privateKey, cert));
+  ASSERT_SECSuccess(BuildCertChain(trustDomain, cert.get(), now,
+                                   EndEntityOrCA::MustBeEndEntity,
+                                   0, // key usage
+                                   KeyPurposeId::id_kp_serverAuth,
+                                   CertPolicyId::anyPolicy,
+                                   nullptr, // stapled OCSP response
+                                   results));
+}
+
+TEST_F(pkix_cert_chain_length, BeyondMaxAcceptableCertChainLength)
+{
+  ScopedCERTCertList results;
+
+  ScopedSECKEYPrivateKey caPrivateKey;
+  ScopedCERTCertificate caCert;
+  ASSERT_TRUE(CreateCert(arena.get(),
+                         trustDomain.GetLeafeCACert()->subjectName,
+                         "CN=CA Too Far", EndEntityOrCA::MustBeCA,
+                         trustDomain.leafCAKey.get(),
+                         caPrivateKey, caCert));
+  PR_SetError(0, 0);
+  ASSERT_SECFailure(SEC_ERROR_UNKNOWN_ISSUER,
+                    BuildCertChain(trustDomain, caCert.get(), now,
+                                   EndEntityOrCA::MustBeCA,
+                                   0, // key usage
+                                   KeyPurposeId::id_kp_serverAuth,
+                                   CertPolicyId::anyPolicy,
+                                   nullptr, // stapled OCSP response
+                                   results));
+
+  ScopedSECKEYPrivateKey privateKey;
+  ScopedCERTCertificate cert;
+  ASSERT_TRUE(CreateCert(arena.get(), caCert->subjectName,
+                         "CN=End-Entity Too Far",
+                         EndEntityOrCA::MustBeEndEntity,
+                         caPrivateKey.get(), privateKey, cert));
+  PR_SetError(0, 0);
+  ASSERT_SECFailure(SEC_ERROR_UNKNOWN_ISSUER,
+                    BuildCertChain(trustDomain, cert.get(), now,
+                                   EndEntityOrCA::MustBeEndEntity,
+                                   0, // key usage
+                                   KeyPurposeId::id_kp_serverAuth,
+                                   CertPolicyId::anyPolicy,
+                                   nullptr, // stapled OCSP response
+                                   results));
+}
--- a/security/pkix/test/lib/pkixtestutil.cpp
+++ b/security/pkix/test/lib/pkixtestutil.cpp
@@ -647,43 +647,54 @@ static SECItem* TBSCertificate(PLArenaPo
 SECItem*
 CreateEncodedCertificate(PLArenaPool* arena, long version,
                          SECOidTag signature, const SECItem* serialNumber,
                          const SECItem* issuerNameDER, PRTime notBefore,
                          PRTime notAfter, const SECItem* subjectNameDER,
                          /*optional*/ SECItem const* const* extensions,
                          /*optional*/ SECKEYPrivateKey* issuerPrivateKey,
                          SECOidTag signatureHashAlg,
-                         /*out*/ ScopedSECKEYPrivateKey& privateKey)
+                         /*out*/ ScopedSECKEYPrivateKey& privateKeyResult)
 {
   PR_ASSERT(arena);
   PR_ASSERT(issuerNameDER);
   PR_ASSERT(subjectNameDER);
   if (!arena || !issuerNameDER || !subjectNameDER) {
     PR_SetError(SEC_ERROR_INVALID_ARGS, 0);
     return nullptr;
   }
 
+  // It may be the case that privateKeyResult refers to the
+  // ScopedSECKEYPrivateKey that owns issuerPrivateKey; thus, we can't set
+  // privateKeyResult until after we're done with issuerPrivateKey.
   ScopedSECKEYPublicKey publicKey;
-  if (GenerateKeyPair(publicKey, privateKey) != SECSuccess) {
+  ScopedSECKEYPrivateKey privateKeyTemp;
+  if (GenerateKeyPair(publicKey, privateKeyTemp) != SECSuccess) {
     return nullptr;
   }
 
   SECItem* tbsCertificate(TBSCertificate(arena, version, serialNumber,
                                          signature, issuerNameDER, notBefore,
                                          notAfter, subjectNameDER,
                                          publicKey.get(), extensions));
   if (!tbsCertificate) {
     return nullptr;
   }
 
-  return MaybeLogOutput(SignedData(arena, tbsCertificate,
-                                   issuerPrivateKey ? issuerPrivateKey
-                                                    : privateKey.get(),
-                                   signatureHashAlg, false, nullptr), "cert");
+  SECItem*
+    result(MaybeLogOutput(SignedData(arena, tbsCertificate,
+                                     issuerPrivateKey ? issuerPrivateKey
+                                                      : privateKeyTemp.get(),
+                                     signatureHashAlg, false, nullptr),
+                          "cert"));
+  if (!result) {
+    return nullptr;
+  }
+  privateKeyResult = privateKeyTemp.release();
+  return result;
 }
 
 // TBSCertificate  ::=  SEQUENCE  {
 //      version         [0]  Version DEFAULT v1,
 //      serialNumber         CertificateSerialNumber,
 //      signature            AlgorithmIdentifier,
 //      issuer               Name,
 //      validity             Validity,
@@ -834,39 +845,41 @@ CreateEncodedSerialNumber(PLArenaPool* a
   return Integer(arena, serialNumberValue);
 }
 
 // BasicConstraints ::= SEQUENCE {
 //         cA                      BOOLEAN DEFAULT FALSE,
 //         pathLenConstraint       INTEGER (0..MAX) OPTIONAL }
 SECItem*
 CreateEncodedBasicConstraints(PLArenaPool* arena, bool isCA,
-                              long pathLenConstraintValue,
+                              /*optional*/ long* pathLenConstraintValue,
                               ExtensionCriticality criticality)
 {
   PR_ASSERT(arena);
   if (!arena) {
     PR_SetError(SEC_ERROR_INVALID_ARGS, 0);
     return nullptr;
   }
 
   Output value;
 
   if (isCA) {
     if (value.Add(Boolean(arena, true)) != der::Success) {
       return nullptr;
     }
   }
 
-  SECItem* pathLenConstraint(Integer(arena, pathLenConstraintValue));
-  if (!pathLenConstraint) {
-    return nullptr;
-  }
-  if (value.Add(pathLenConstraint) != der::Success) {
-    return nullptr;
+  if (pathLenConstraintValue) {
+    SECItem* pathLenConstraint(Integer(arena, *pathLenConstraintValue));
+    if (!pathLenConstraint) {
+      return nullptr;
+    }
+    if (value.Add(pathLenConstraint) != der::Success) {
+      return nullptr;
+    }
   }
 
   return Extension(arena, SEC_OID_X509_BASIC_CONSTRAINTS, criticality, value);
 }
 
 // ExtKeyUsageSyntax ::= SEQUENCE SIZE (1..MAX) OF KeyPurposeId
 // KeyPurposeId ::= OBJECT IDENTIFIER
 SECItem*
--- a/security/pkix/test/lib/pkixtestutil.h
+++ b/security/pkix/test/lib/pkixtestutil.h
@@ -94,17 +94,17 @@ SECItem* CreateEncodedCertificate(PLAren
                           /*out*/ ScopedSECKEYPrivateKey& privateKey);
 
 SECItem* CreateEncodedSerialNumber(PLArenaPool* arena, long value);
 
 MOZILLA_PKIX_ENUM_CLASS ExtensionCriticality { NotCritical = 0, Critical = 1 };
 
 // The return value, if non-null, is owned by the arena and MUST NOT be freed.
 SECItem* CreateEncodedBasicConstraints(PLArenaPool* arena, bool isCA,
-                                       long pathLenConstraint,
+                                       /*optional*/ long* pathLenConstraint,
                                        ExtensionCriticality criticality);
 
 // ekus must be non-null and must must point to a SEC_OID_UNKNOWN-terminated
 // array of SECOidTags. If the first item of the array is SEC_OID_UNKNOWN then
 // an empty EKU extension will be encoded.
 //
 // The return value, if non-null, is owned by the arena and MUST NOT be freed.
 SECItem* CreateEncodedEKUExtension(PLArenaPool* arena,