From bc8988913a2098a234726e8bb1480136ba1e481e Mon Sep 17 00:00:00 2001

From: "J.C. Jones" <jc@mozila.com>

Date: Sat, 24 Oct 2020 14:21:28 +0300

Subject: [PATCH] MOZILLA 1666567 land NSS 8ebee3cec9cf UPGRADE_NSS_RELEASE,

 r=kjacobs

2020-09-23  Dana Keeler  <dkeeler@mozilla.com>

	* gtests/mozpkix_gtest/pkixbuild_tests.cpp,

	gtests/mozpkix_gtest/pkixcert_extension_tests.cpp,

	gtests/mozpkix_gtest/pkixcert_signature_algorithm_tests.cpp,

	gtests/mozpkix_gtest/pkixcheck_CheckExtendedKeyUsage_tests.cpp,

	gtests/mozpkix_gtest/pkixcheck_CheckSignatureAlgorithm_tests.cpp,

	gtests/mozpkix_gtest/pkixgtest.h,

	lib/mozpkix/include/pkix/pkixtypes.h, lib/mozpkix/lib/pkixbuild.cpp:

	Bug 1665715 - (2/2) pass encoded signed certificate timestamp

	extension (if present) in CheckRevocation r=jcj

	This will allow Firefox to make decisions based on the earliest

	known time that a certificate exists (with respect to certificate

	transparency) that a CA is unlikely to back-date. In particular,

	this is essential for CRLite. Note that if the SCT signature isn't

	validated, a CA could still make a certificate appear to have

	existed for longer than it really has. However, this change is not

	an attempt to catch malicious CAs. The aim is to avoid false

	positives in CRLite resulting from CAs backdating the notBefore

	field on certificates they issue.

	Depends on D90595

	[8ebee3cec9cf] [tip]

2020-09-18  Dana Keeler  <dkeeler@mozilla.com>

	* gtests/mozpkix_gtest/pkixbuild_tests.cpp,

	gtests/mozpkix_gtest/pkixcert_extension_tests.cpp,

	gtests/mozpkix_gtest/pkixcert_signature_algorithm_tests.cpp,

	gtests/mozpkix_gtest/pkixcheck_CheckExtendedKeyUsage_tests.cpp,

	gtests/mozpkix_gtest/pkixcheck_CheckSignatureAlgorithm_tests.cpp,

	gtests/mozpkix_gtest/pkixgtest.h,

	lib/mozpkix/include/pkix/pkixtypes.h, lib/mozpkix/lib/pkixbuild.cpp:

	Bug 1665715 - (1/2) revert e8f2720c8254 (bug 1593141) because it's

	no longer necessary r=jcj

	Bug 1593141 added the certificate's notBefore field as an argument

	to TrustDomain::CheckRevocation so that Firefox could use it with

	CRLite. However, since CAs can backdate that field, we need to use

	the earliest embedded SCT timestamp instead.

	[c1f4d565ceda]

Differential Revision: https://phabricator.services.mozilla.com/D91211

---

 mozilla/security/nss/TAG-INFO                 |  2 +-

 mozilla/security/nss/coreconf/coreconf.dep    |  1 -

 .../gtests/mozpkix_gtest/pkixbuild_tests.cpp  | 52 +++++++------------

 .../pkixcert_extension_tests.cpp              |  5 +-

 .../pkixcert_signature_algorithm_tests.cpp    |  4 +-

 .../pkixcheck_CheckExtendedKeyUsage_tests.cpp |  4 +-

 ...kixcheck_CheckSignatureAlgorithm_tests.cpp |  3 +-

 .../nss/gtests/mozpkix_gtest/pkixgtest.h      |  3 +-

 .../nss/lib/mozpkix/include/pkix/pkixtypes.h  |  4 +-

 .../nss/lib/mozpkix/lib/pkixbuild.cpp         |  6 +--

 10 files changed, 36 insertions(+), 48 deletions(-)

diff --git a/mozilla/security/nss/TAG-INFO b/mozilla/security/nss/TAG-INFO

index d38ae44a379..cc417133487 100644

--- a/mozilla/security/nss/TAG-INFO

+++ b/mozilla/security/nss/TAG-INFO

@@ -1 +1 @@

-NSS_3_53_1_RTM

\ No newline at end of file

+8ebee3cec9cf

\ No newline at end of file

diff --git a/mozilla/security/nss/coreconf/coreconf.dep b/mozilla/security/nss/coreconf/coreconf.dep

index 590d1bfaeee..5182f75552c 100644

--- a/mozilla/security/nss/coreconf/coreconf.dep

+++ b/mozilla/security/nss/coreconf/coreconf.dep

@@ -3,11 +3,10 @@

  * 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/. */

 /*

  * A dummy header file that is a dependency for all the object files.

  * Used to force a full recompilation of NSS in Mozilla's Tinderbox

  * depend builds.  See comments in rules.mk.

  */

 #error "Do not include this header file."

-

diff --git a/mozilla/security/nss/gtests/mozpkix_gtest/pkixbuild_tests.cpp b/mozilla/security/nss/gtests/mozpkix_gtest/pkixbuild_tests.cpp

index c1c81b3a7c1..c5ac86e62aa 100644

--- a/mozilla/security/nss/gtests/mozpkix_gtest/pkixbuild_tests.cpp

+++ b/mozilla/security/nss/gtests/mozpkix_gtest/pkixbuild_tests.cpp

@@ -145,28 +145,25 @@ private:

     }

     bool keepGoing;

     rv = checker.Check(derCert, nullptr/*additionalNameConstraints*/,

                        keepGoing);

     if (rv != Success) {

       return rv;

     }

     return Success;

   }

-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time,

-                         Time validityBeginning, Duration,

-                         /*optional*/ const Input*, /*optional*/ const Input*)

+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,

+                         /*optional*/ const Input*, /*optional*/ const Input*,

+                         /*optional*/ const Input*)

                          override

   {

-    // All of the certificates in this test for which this is called have a

-    // validity period that begins "one day before now".

-    EXPECT_EQ(TimeFromEpochInSeconds(oneDayBeforeNow), validityBeginning);

     return Success;

   }

   Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override

   {

     return Success;

   }

   std::map<ByteString, ByteString> subjectDERToCertDER;

   ByteString leafCACertDER;

@@ -298,46 +295,44 @@ public:

       return rv;

     }

     return checker.Check(rootCert, nullptr, keepGoing);

   }

   Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override

   {

     return Success;

   }

-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time,

-                         Time validityBeginning, Duration,

-                         /*optional*/ const Input*, /*optional*/ const Input*)

+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,

+                         /*optional*/ const Input*, /*optional*/ const Input*,

+                         /*optional*/ const Input*)

                          override

   {

-    // All of the certificates in this test for which this is called have a

-    // validity period that begins "one day before now".

-    EXPECT_EQ(TimeFromEpochInSeconds(oneDayBeforeNow), validityBeginning);

     return Success;

   }

 private:

   ByteString rootDER;

 };

 // A TrustDomain that explicitly fails if CheckRevocation is called.

 class ExpiredCertTrustDomain final : public SingleRootTrustDomain

 {

 public:

   explicit ExpiredCertTrustDomain(ByteString aRootDER)

     : SingleRootTrustDomain(aRootDER)

   {

   }

-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Time, Duration,

-                         /*optional*/ const Input*, /*optional*/ const Input*)

+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,

+                         /*optional*/ const Input*, /*optional*/ const Input*,

+                         /*optional*/ const Input*)

                          override

   {

     ADD_FAILURE();

     return NotReached("CheckRevocation should not be called",

                       Result::FATAL_ERROR_LIBRARY_FAILURE);

   }

 };

 TEST_F(pkixbuild, NoRevocationCheckingForExpiredCert)

 {

@@ -443,28 +438,25 @@ public:

     Input issuerInput;

     EXPECT_EQ(Success, issuerInput.Init(issuer.data(), issuer.length()));

     bool keepGoing;

     EXPECT_EQ(Success,

               checker.Check(issuerInput, nullptr /*additionalNameConstraints*/,

                             keepGoing));

     EXPECT_EQ(expectedKeepGoing, keepGoing);

     return Success;

   }

-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time,

-                         Time validityBeginning, Duration,

-                         /*optional*/ const Input*, /*optional*/ const Input*)

+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,

+                         /*optional*/ const Input*, /*optional*/ const Input*,

+                         /*optional*/ const Input*)

                          override

   {

-    // All of the certificates in this test for which this is called have a

-    // validity period that begins "one day before now".

-    EXPECT_EQ(TimeFromEpochInSeconds(oneDayBeforeNow), validityBeginning);

     return Success;

   }

   Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override

   {

     return Success;

   }

 private:

   const ByteString issuer;

@@ -670,28 +662,25 @@ private:

     if (keepGoing) {

       rv = CheckCert(intermediateSignedByUntrustedRootCertDER, checker,

                      keepGoing);

       if (rv != Success) {

         return rv;

       }

     }

     return Success;

   }

-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time,

-                         Time validityBeginning, Duration,

+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,

+                         /*optional*/ const Input*,

                          /*optional*/ const Input*,

                          /*optional*/ const Input*) override

   {

-    // All of the certificates in this test for which this is called have a

-    // validity period that begins "one day before now".

-    EXPECT_EQ(TimeFromEpochInSeconds(oneDayBeforeNow), validityBeginning);

     return Success;

   }

   Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override

   {

     return Success;

   }

   std::map<ByteString, ByteString> subjectDERToCertDER;

   ByteString rootCACertDER;

@@ -732,22 +721,22 @@ TEST_F(pkixbuild, BadEmbeddedSCTWithMultiplePaths)

                            KeyPurposeId::id_kp_serverAuth,

                            CertPolicyId::anyPolicy,

                            nullptr/*stapledOCSPResponse*/));

 }

 // Same as a MultiplePathTrustDomain, but the end-entity is revoked.

 class RevokedEndEntityTrustDomain final : public MultiplePathTrustDomain

 {

 public:

   Result CheckRevocation(EndEntityOrCA endEntityOrCA, const CertID&, Time,

-                         Time, Duration, /*optional*/ const Input*,

-                         /*optional*/ const Input*) override

+                         Duration, /*optional*/ const Input*,

+                         /*optional*/ const Input*, /*optional*/ const Input*) override

   {

     if (endEntityOrCA == EndEntityOrCA::MustBeEndEntity) {

       return Result::ERROR_REVOKED_CERTIFICATE;

     }

     return Success;

   }

 };

 TEST_F(pkixbuild, RevokedEndEntityWithMultiplePaths)

 {

@@ -837,28 +826,25 @@ private:

         return rv;

       }

       rv = checker.Check(certInput, nullptr, keepGoing);

       if (rv != Success || !keepGoing) {

         return rv;

       }

     }

     return Success;

   }

-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time,

-                         Time validityBeginning, Duration,

-                         /*optional*/ const Input*, /*optional*/ const Input*)

+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,

+                         /*optional*/ const Input*, /*optional*/ const Input*,

+                         /*optional*/ const Input*)

                          override

   {

-    // All of the certificates in this test for which this is called have a

-    // validity period that begins "one day before now".

-    EXPECT_EQ(TimeFromEpochInSeconds(oneDayBeforeNow), validityBeginning);

     return Success;

   }

   Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override

   {

     return Success;

   }

   std::vector<ByteString> certs;

   ByteString rootCACertDER;

diff --git a/mozilla/security/nss/gtests/mozpkix_gtest/pkixcert_extension_tests.cpp b/mozilla/security/nss/gtests/mozpkix_gtest/pkixcert_extension_tests.cpp

index 71399a26bd4..e2dcc8e0214 100644

--- a/mozilla/security/nss/gtests/mozpkix_gtest/pkixcert_extension_tests.cpp

+++ b/mozilla/security/nss/gtests/mozpkix_gtest/pkixcert_extension_tests.cpp

@@ -63,22 +63,23 @@ CreateCertWithOneExtension(const char* subjectStr, const ByteString& extension)

 class TrustEverythingTrustDomain final : public DefaultCryptoTrustDomain

 {

 private:

   Result GetCertTrust(EndEntityOrCA, const CertPolicyId&, Input,

                       /*out*/ TrustLevel& trustLevel) override

   {

     trustLevel = TrustLevel::TrustAnchor;

     return Success;

   }

-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Time, Duration,

-                         /*optional*/ const Input*, /*optional*/ const Input*)

+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,

+                         /*optional*/ const Input*, /*optional*/ const Input*,

+                         /*optional*/ const Input*)

                          override

   {

     return Success;

   }

   Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override

   {

     return Success;

   }

 };

diff --git a/mozilla/security/nss/gtests/mozpkix_gtest/pkixcert_signature_algorithm_tests.cpp b/mozilla/security/nss/gtests/mozpkix_gtest/pkixcert_signature_algorithm_tests.cpp

index 54e19fc3d26..5719d1045d9 100644

--- a/mozilla/security/nss/gtests/mozpkix_gtest/pkixcert_signature_algorithm_tests.cpp

+++ b/mozilla/security/nss/gtests/mozpkix_gtest/pkixcert_signature_algorithm_tests.cpp

@@ -85,22 +85,22 @@ private:

     }

     Input issuerCert;

     Result rv = issuerCert.Init(issuerDER->data(), issuerDER->length());

     if (rv != Success) {

       return rv;

     }

     bool keepGoing;

     return checker.Check(issuerCert, nullptr, keepGoing);

   }

-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Time, Duration,

-                         const Input*, const Input*) override

+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,

+                         const Input*, const Input*, const Input*) override

   {

     return Success;

   }

   Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override

   {

     return Success;

   }

   ByteString rootDER;

diff --git a/mozilla/security/nss/gtests/mozpkix_gtest/pkixcheck_CheckExtendedKeyUsage_tests.cpp b/mozilla/security/nss/gtests/mozpkix_gtest/pkixcheck_CheckExtendedKeyUsage_tests.cpp

index 9fd1e52f1a7..364be47e652 100644

--- a/mozilla/security/nss/gtests/mozpkix_gtest/pkixcheck_CheckExtendedKeyUsage_tests.cpp

+++ b/mozilla/security/nss/gtests/mozpkix_gtest/pkixcheck_CheckExtendedKeyUsage_tests.cpp

@@ -551,22 +551,22 @@ private:

   {

     Input derCert;

     Result rv = derCert.Init(mIssuerCertDER.data(), mIssuerCertDER.length());

     if (rv != Success) {

       return rv;

     }

     bool keepGoing;

     return checker.Check(derCert, nullptr, keepGoing);

   }

-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Time, Duration,

-                         const Input*, const Input*) override

+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,

+                         const Input*, const Input*, const Input*) override

   {

     return Success;

   }

   Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override

   {

     return Success;

   }

   ByteString mIssuerCertDER;

diff --git a/mozilla/security/nss/gtests/mozpkix_gtest/pkixcheck_CheckSignatureAlgorithm_tests.cpp b/mozilla/security/nss/gtests/mozpkix_gtest/pkixcheck_CheckSignatureAlgorithm_tests.cpp

index e1f35e5b40e..d3a57c3e6f2 100644

--- a/mozilla/security/nss/gtests/mozpkix_gtest/pkixcheck_CheckSignatureAlgorithm_tests.cpp

+++ b/mozilla/security/nss/gtests/mozpkix_gtest/pkixcheck_CheckSignatureAlgorithm_tests.cpp

@@ -295,21 +295,22 @@ public:

     Input issuerInput;

     EXPECT_EQ(Success, issuerInput.Init(issuer.data(), issuer.length()));

     bool keepGoing;

     EXPECT_EQ(Success, checker.Check(issuerInput, nullptr, keepGoing));

     EXPECT_FALSE(keepGoing);

     return Success;

   }

-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Time, Duration,

+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,

+                         /*optional*/ const Input*,

                          /*optional*/ const Input*,

                          /*optional*/ const Input*) override

   {

     return Success;

   }

   Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override

   {

     return Success;

   }

diff --git a/mozilla/security/nss/gtests/mozpkix_gtest/pkixgtest.h b/mozilla/security/nss/gtests/mozpkix_gtest/pkixgtest.h

index 0a203c5e1ea..719b87d54f0 100644

--- a/mozilla/security/nss/gtests/mozpkix_gtest/pkixgtest.h

+++ b/mozilla/security/nss/gtests/mozpkix_gtest/pkixgtest.h

@@ -93,21 +93,22 @@ class EverythingFailsByDefaultTrustDomain : public TrustDomain {

     return NotReached("GetCertTrust should not be called",

                       Result::FATAL_ERROR_LIBRARY_FAILURE);

   }

   Result FindIssuer(Input, IssuerChecker&, Time) override {

     ADD_FAILURE();

     return NotReached("FindIssuer should not be called",

                       Result::FATAL_ERROR_LIBRARY_FAILURE);

   }

-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Time, Duration,

+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,

+                         /*optional*/ const Input*,

                          /*optional*/ const Input*,

                          /*optional*/ const Input*) override {

     ADD_FAILURE();

     return NotReached("CheckRevocation should not be called",

                       Result::FATAL_ERROR_LIBRARY_FAILURE);

   }

   Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override {

     ADD_FAILURE();

     return NotReached("IsChainValid should not be called",

diff --git a/mozilla/security/nss/lib/mozpkix/include/pkix/pkixtypes.h b/mozilla/security/nss/lib/mozpkix/include/pkix/pkixtypes.h

index bfa5c780ac2..6c391681f3a 100644

--- a/mozilla/security/nss/lib/mozpkix/include/pkix/pkixtypes.h

+++ b/mozilla/security/nss/lib/mozpkix/include/pkix/pkixtypes.h

@@ -271,24 +271,24 @@ class TrustDomain {

   // application must not assume anything about the validity of the last

   // certificate chain passed to IsChainValid; especially, it would be very

   // wrong to assume that the certificate chain is valid.

   //

   // certChain.GetDER(0) is the trust anchor.

   virtual Result IsChainValid(const DERArray& certChain, Time time,

                               const CertPolicyId& requiredPolicy) = 0;

   virtual Result CheckRevocation(EndEntityOrCA endEntityOrCA,

                                  const CertID& certID, Time time,

-                                 Time validityBeginning,

                                  Duration validityDuration,

                                  /*optional*/ const Input* stapledOCSPresponse,

-                                 /*optional*/ const Input* aiaExtension) = 0;

+                                 /*optional*/ const Input* aiaExtension,

+                                 /*optional*/ const Input* sctExtension) = 0;

   // Check that the given digest algorithm is acceptable for use in signatures.

   //

   // Return Success if the algorithm is acceptable,

   // Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED if the algorithm is not

   // acceptable, or another error code if another error occurred.

   virtual Result CheckSignatureDigestAlgorithm(DigestAlgorithm digestAlg,

                                                EndEntityOrCA endEntityOrCA,

                                                Time notBefore) = 0;

diff --git a/mozilla/security/nss/lib/mozpkix/lib/pkixbuild.cpp b/mozilla/security/nss/lib/mozpkix/lib/pkixbuild.cpp

index b95907a947b..afe7e2a2477 100644

--- a/mozilla/security/nss/lib/mozpkix/lib/pkixbuild.cpp

+++ b/mozilla/security/nss/lib/mozpkix/lib/pkixbuild.cpp

@@ -245,23 +245,23 @@ PathBuildingStep::Check(Input potentialIssuerDER,

     Time notBefore(Time::uninitialized);

     Time notAfter(Time::uninitialized);

     // This should never fail. If we're here, we've already parsed the validity

     // and checked that the given time is in the certificate's validity period.

     rv = ParseValidity(subject.GetValidity(), &notBefore, &notAfter);

     if (rv != Success) {

       return rv;

     }

     Duration validityDuration(notAfter, notBefore);

     rv = trustDomain.CheckRevocation(subject.endEntityOrCA, certID, time,

-                                     notBefore, validityDuration,

-                                     stapledOCSPResponse,

-                                     subject.GetAuthorityInfoAccess());

+                                     validityDuration, stapledOCSPResponse,

+                                     subject.GetAuthorityInfoAccess(),

+                                     subject.GetSignedCertificateTimestamps());

     if (rv != Success) {

       // Since this is actually a problem with the current subject certificate

       // (rather than the issuer), it doesn't make sense to keep going; all

       // paths through this certificate will fail.

       Result savedRv = RecordResult(rv, keepGoing);

       keepGoing = false;

       return savedRv;

     }

     if (subject.endEntityOrCA == EndEntityOrCA::MustBeEndEntity) {

-- 

2.25.4