Bug 942515 - Show Untrusted Connection Error for SHA-1-based SSL certificates with notBefore >= 2016-01-01 r=keeler

9 files changed, 161 insertions, 71 deletions

diff --git a/lib/mozpkix/include/pkix/pkixtypes.h b/lib/mozpkix/include/pkix/pkixtypes.h
index e68e6a6f6..1ea9535a1 100644
--- a/lib/mozpkix/include/pkix/pkixtypes.h
+++ b/lib/mozpkix/include/pkix/pkixtypes.h
@@ -278,7 +278,8 @@ public:
   // 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) = 0;
+                                               EndEntityOrCA endEntityOrCA,
+                                               Time notBefore) = 0;
 
   // Check that the RSA public key size is acceptable.
   //
diff --git a/lib/mozpkix/lib/pkixbuild.cpp b/lib/mozpkix/lib/pkixbuild.cpp
index 793378dc4..cfaac2335 100644
--- a/lib/mozpkix/lib/pkixbuild.cpp
+++ b/lib/mozpkix/lib/pkixbuild.cpp
@@ -226,9 +226,9 @@ PathBuildingStep::Check(Input potentialIssuerDER,
                   subject.GetSerialNumber());
     Time notBefore(Time::uninitialized);
     Time notAfter(Time::uninitialized);
-    // This should never fail. If we're here, we've already checked that the
-    // given time is in the certificate's validity period.
-    rv = CheckValidity(subject.GetValidity(), time, &notBefore, &notAfter);
+    // 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;
     }
diff --git a/lib/mozpkix/lib/pkixcheck.cpp b/lib/mozpkix/lib/pkixcheck.cpp
index 7533b02d0..79d001a91 100644
--- a/lib/mozpkix/lib/pkixcheck.cpp
+++ b/lib/mozpkix/lib/pkixcheck.cpp
@@ -35,6 +35,7 @@ namespace mozilla { namespace pkix {
 Result
 CheckSignatureAlgorithm(TrustDomain& trustDomain,
                         EndEntityOrCA endEntityOrCA,
+                        Time notBefore,
                         const der::SignedDataWithSignature& signedData,
                         Input signatureValue)
 {
@@ -91,7 +92,8 @@ CheckSignatureAlgorithm(TrustDomain& trustDomain,
   // more generally it short-circuits any path building with them (which, of
   // course, is even slower).
 
-  rv = trustDomain.CheckSignatureDigestAlgorithm(digestAlg, endEntityOrCA);
+  rv = trustDomain.CheckSignatureDigestAlgorithm(digestAlg, endEntityOrCA,
+                                                 notBefore);
   if (rv != Success) {
     return rv;
   }
@@ -125,7 +127,7 @@ CheckSignatureAlgorithm(TrustDomain& trustDomain,
 // 4.1.2.5 Validity
 
 Result
-CheckValidity(Input encodedValidity, Time time,
+ParseValidity(Input encodedValidity,
               /*optional out*/ Time* notBeforeOut,
               /*optional out*/ Time* notAfterOut)
 {
@@ -148,6 +150,19 @@ CheckValidity(Input encodedValidity, Time time,
     return Result::ERROR_INVALID_DER_TIME;
   }
 
+  if (notBeforeOut) {
+    *notBeforeOut = notBefore;
+  }
+  if (notAfterOut) {
+    *notAfterOut = notAfter;
+  }
+
+  return Success;
+}
+
+Result
+CheckValidity(Time time, Time notBefore, Time notAfter)
+{
   if (time < notBefore) {
     return Result::ERROR_NOT_YET_VALID_CERTIFICATE;
   }
@@ -156,12 +171,6 @@ CheckValidity(Input encodedValidity, Time time,
     return Result::ERROR_EXPIRED_CERTIFICATE;
   }
 
-  if (notBeforeOut) {
-    *notBeforeOut = notBefore;
-  }
-  if (notAfterOut) {
-    *notAfterOut = notAfter;
-  }
   return Success;
 }
 
@@ -844,6 +853,17 @@ CheckIssuerIndependentProperties(TrustDomain& trustDomain,
     return rv;
   }
 
+  // IMPORTANT: We parse the validity interval here, so that we can use the
+  // notBefore and notAfter values in checks for things that might be deprecated
+  // over time. However, we must not fail for semantic errors until the end of
+  // this method, in order to preserve error ranking.
+  Time notBefore(Time::uninitialized);
+  Time notAfter(Time::uninitialized);
+  rv = ParseValidity(cert.GetValidity(), &notBefore, &notAfter);
+  if (rv != Success) {
+    return rv;
+  }
+
   if (trustLevel == TrustLevel::TrustAnchor &&
       endEntityOrCA == EndEntityOrCA::MustBeEndEntity &&
       requiredEKUIfPresent == KeyPurposeId::id_kp_OCSPSigning) {
@@ -856,7 +876,7 @@ CheckIssuerIndependentProperties(TrustDomain& trustDomain,
 
   switch (trustLevel) {
     case TrustLevel::InheritsTrust:
-      rv = CheckSignatureAlgorithm(trustDomain, endEntityOrCA,
+      rv = CheckSignatureAlgorithm(trustDomain, endEntityOrCA, notBefore,
                                    cert.GetSignedData(), cert.GetSignature());
       if (rv != Success) {
         return rv;
@@ -945,11 +965,9 @@ CheckIssuerIndependentProperties(TrustDomain& trustDomain,
   // 4.2.1.14. Inhibit anyPolicy is implicitly supported; see the documentation
   //           about policy enforcement in pkix.h.
 
-  // IMPORTANT: This check must come after the other checks in order for error
-  // ranking to work correctly.
-  Time notBefore(Time::uninitialized);
-  Time notAfter(Time::uninitialized);
-  rv = CheckValidity(cert.GetValidity(), time, &notBefore, &notAfter);
+  // IMPORTANT: Even though we parse validity above, we wait until this point to
+  // check it, so that error ranking works correctly.
+  rv = CheckValidity(time, notBefore, notAfter);
   if (rv != Success) {
     return rv;
   }
diff --git a/lib/mozpkix/lib/pkixcheck.h b/lib/mozpkix/lib/pkixcheck.h
index b27be96a9..72b463d27 100644
--- a/lib/mozpkix/lib/pkixcheck.h
+++ b/lib/mozpkix/lib/pkixcheck.h
@@ -45,9 +45,15 @@ Result CheckNameConstraints(Input encodedNameConstraints,
                             const BackCert& firstChild,
                             KeyPurposeId requiredEKUIfPresent);
 
-Result CheckValidity(Input encodedValidity, Time time,
+// ParseValidity and CheckValidity are usually used together.  First you parse
+// the dates from the DER Validity sequence, then you compare them to the time
+// at which you are validating.  They are separate so that the notBefore and
+// notAfter times can be used for other things before they are checked against
+// the time of validation.
+Result ParseValidity(Input encodedValidity,
                      /*optional out*/ Time* notBeforeOut = nullptr,
                      /*optional out*/ Time* notAfterOut = nullptr);
+Result CheckValidity(Time time, Time notBefore, Time notAfter);
 
 } } // namespace mozilla::pkix
 
diff --git a/lib/mozpkix/test/gtest/moz.build b/lib/mozpkix/test/gtest/moz.build
index 51952fb6d..41fc55074 100644
--- a/lib/mozpkix/test/gtest/moz.build
+++ b/lib/mozpkix/test/gtest/moz.build
@@ -11,6 +11,7 @@ SOURCES += [
     'pkixcheck_CheckKeyUsage_tests.cpp',
     'pkixcheck_CheckSignatureAlgorithm_tests.cpp',
     'pkixcheck_CheckValidity_tests.cpp',
+    'pkixcheck_ParseValidity_tests.cpp',
 
     # The naming conventions are described in ./README.txt.
 
diff --git a/lib/mozpkix/test/gtest/pkixcheck_CheckSignatureAlgorithm_tests.cpp b/lib/mozpkix/test/gtest/pkixcheck_CheckSignatureAlgorithm_tests.cpp
index e21c5ac70..7ab9d095a 100644
--- a/lib/mozpkix/test/gtest/pkixcheck_CheckSignatureAlgorithm_tests.cpp
+++ b/lib/mozpkix/test/gtest/pkixcheck_CheckSignatureAlgorithm_tests.cpp
@@ -32,6 +32,7 @@ namespace mozilla { namespace pkix {
 
 extern Result CheckSignatureAlgorithm(
                 TrustDomain& trustDomain, EndEntityOrCA endEntityOrCA,
+                Time notBefore,
                 const der::SignedDataWithSignature& signedData,
                 Input signatureValue);
 
@@ -203,7 +204,8 @@ public:
   {
   }
 
-  Result CheckSignatureDigestAlgorithm(DigestAlgorithm, EndEntityOrCA) override
+  Result CheckSignatureDigestAlgorithm(DigestAlgorithm, EndEntityOrCA, Time)
+    override
   {
     checkedDigestAlgorithm = true;
     return Success;
@@ -226,6 +228,7 @@ public:
 
 TEST_P(pkixcheck_CheckSignatureAlgorithm, CheckSignatureAlgorithm)
 {
+  const Time now(Now());
   const CheckSignatureAlgorithmTestParams& params(GetParam());
 
   Input signatureValueInput;
@@ -248,7 +251,7 @@ TEST_P(pkixcheck_CheckSignatureAlgorithm, CheckSignatureAlgorithm)
 
   ASSERT_EQ(params.expectedResult,
             CheckSignatureAlgorithm(trustDomain, EndEntityOrCA::MustBeEndEntity,
-                                    signedData, signatureValueInput));
+                                    now, signedData, signatureValueInput));
   ASSERT_EQ(params.expectedResult == Success,
             trustDomain.checkedDigestAlgorithm);
   ASSERT_EQ(params.expectedResult == Success,
diff --git a/lib/mozpkix/test/gtest/pkixcheck_CheckValidity_tests.cpp b/lib/mozpkix/test/gtest/pkixcheck_CheckValidity_tests.cpp
index 99b76a32d..a77a2f47c 100644
--- a/lib/mozpkix/test/gtest/pkixcheck_CheckValidity_tests.cpp
+++ b/lib/mozpkix/test/gtest/pkixcheck_CheckValidity_tests.cpp
@@ -56,36 +56,6 @@ static const Time FUTURE_TIME(YMDHMS(2025, 12, 31, 12, 23, 56));
 
 class pkixcheck_CheckValidity : public ::testing::Test { };
 
-TEST_F(pkixcheck_CheckValidity, BothEmptyNull)
-{
-  static const uint8_t DER[] = {
-    0x17/*UTCTime*/, 0/*length*/,
-    0x17/*UTCTime*/, 0/*length*/,
-  };
-  static const Input validity(DER);
-  ASSERT_EQ(Result::ERROR_INVALID_DER_TIME, CheckValidity(validity, NOW));
-}
-
-TEST_F(pkixcheck_CheckValidity, NotBeforeEmptyNull)
-{
-  static const uint8_t DER[] = {
-    0x17/*UTCTime*/, 0x00/*length*/,
-    NEWER_UTCTIME
-  };
-  static const Input validity(DER);
-  ASSERT_EQ(Result::ERROR_INVALID_DER_TIME, CheckValidity(validity, NOW));
-}
-
-TEST_F(pkixcheck_CheckValidity, NotAfterEmptyNull)
-{
-  static const uint8_t DER[] = {
-    NEWER_UTCTIME,
-    0x17/*UTCTime*/, 0x00/*length*/,
-  };
-  static const Input validity(DER);
-  ASSERT_EQ(Result::ERROR_INVALID_DER_TIME, CheckValidity(validity, NOW));
-}
-
 static const uint8_t OLDER_UTCTIME_NEWER_UTCTIME_DATA[] = {
   OLDER_UTCTIME,
   NEWER_UTCTIME,
@@ -95,7 +65,10 @@ OLDER_UTCTIME_NEWER_UTCTIME(OLDER_UTCTIME_NEWER_UTCTIME_DATA);
 
 TEST_F(pkixcheck_CheckValidity, Valid_UTCTIME_UTCTIME)
 {
-  ASSERT_EQ(Success, CheckValidity(OLDER_UTCTIME_NEWER_UTCTIME, NOW));
+  static Time notBefore(Time::uninitialized);
+  static Time notAfter(Time::uninitialized);
+  ASSERT_EQ(Success, ParseValidity(OLDER_UTCTIME_NEWER_UTCTIME, &notBefore, &notAfter));
+  ASSERT_EQ(Success, CheckValidity(NOW, notBefore, notAfter));
 }
 
 TEST_F(pkixcheck_CheckValidity, Valid_GENERALIZEDTIME_GENERALIZEDTIME)
@@ -105,7 +78,10 @@ TEST_F(pkixcheck_CheckValidity, Valid_GENERALIZEDTIME_GENERALIZEDTIME)
     NEWER_GENERALIZEDTIME,
   };
   static const Input validity(DER);
-  ASSERT_EQ(Success, CheckValidity(validity, NOW));
+  static Time notBefore(Time::uninitialized);
+  static Time notAfter(Time::uninitialized);
+  ASSERT_EQ(Success, ParseValidity(validity, &notBefore, &notAfter));
+  ASSERT_EQ(Success, CheckValidity(NOW, notBefore, notAfter));
 }
 
 TEST_F(pkixcheck_CheckValidity, Valid_GENERALIZEDTIME_UTCTIME)
@@ -115,7 +91,10 @@ TEST_F(pkixcheck_CheckValidity, Valid_GENERALIZEDTIME_UTCTIME)
     NEWER_UTCTIME,
   };
   static const Input validity(DER);
-  ASSERT_EQ(Success, CheckValidity(validity, NOW));
+  static Time notBefore(Time::uninitialized);
+  static Time notAfter(Time::uninitialized);
+  ASSERT_EQ(Success, ParseValidity(validity, &notBefore, &notAfter));
+  ASSERT_EQ(Success, CheckValidity(NOW, notBefore, notAfter));
 }
 
 TEST_F(pkixcheck_CheckValidity, Valid_UTCTIME_GENERALIZEDTIME)
@@ -125,27 +104,24 @@ TEST_F(pkixcheck_CheckValidity, Valid_UTCTIME_GENERALIZEDTIME)
     NEWER_GENERALIZEDTIME,
   };
   static const Input validity(DER);
-  ASSERT_EQ(Success, CheckValidity(validity, NOW));
+  static Time notBefore(Time::uninitialized);
+  static Time notAfter(Time::uninitialized);
+  ASSERT_EQ(Success, ParseValidity(validity, &notBefore, &notAfter));
+  ASSERT_EQ(Success, CheckValidity(NOW, notBefore, notAfter));
 }
 
 TEST_F(pkixcheck_CheckValidity, InvalidBeforeNotBefore)
 {
-  ASSERT_EQ(Result::ERROR_NOT_YET_VALID_CERTIFICATE,
-            CheckValidity(OLDER_UTCTIME_NEWER_UTCTIME, PAST_TIME));
+  static Time notBefore(Time::uninitialized);
+  static Time notAfter(Time::uninitialized);
+  ASSERT_EQ(Success, ParseValidity(OLDER_UTCTIME_NEWER_UTCTIME, &notBefore, &notAfter));
+  ASSERT_EQ(Result::ERROR_NOT_YET_VALID_CERTIFICATE, CheckValidity(PAST_TIME, notBefore, notAfter));
 }
 
 TEST_F(pkixcheck_CheckValidity, InvalidAfterNotAfter)
 {
-  ASSERT_EQ(Result::ERROR_EXPIRED_CERTIFICATE,
-            CheckValidity(OLDER_UTCTIME_NEWER_UTCTIME, FUTURE_TIME));
-}
-
-TEST_F(pkixcheck_CheckValidity, InvalidNotAfterBeforeNotBefore)
-{
-  static const uint8_t DER[] = {
-    NEWER_UTCTIME,
-    OLDER_UTCTIME,
-  };
-  static const Input validity(DER);
-  ASSERT_EQ(Result::ERROR_INVALID_DER_TIME, CheckValidity(validity, NOW));
+  static Time notBefore(Time::uninitialized);
+  static Time notAfter(Time::uninitialized);
+  ASSERT_EQ(Success, ParseValidity(OLDER_UTCTIME_NEWER_UTCTIME, &notBefore, &notAfter));
+  ASSERT_EQ(Result::ERROR_EXPIRED_CERTIFICATE, CheckValidity(FUTURE_TIME, notBefore, notAfter));
 }
diff --git a/lib/mozpkix/test/gtest/pkixcheck_ParseValidity_tests.cpp b/lib/mozpkix/test/gtest/pkixcheck_ParseValidity_tests.cpp
new file mode 100644
index 000000000..5206ce14f
--- /dev/null
+++ b/lib/mozpkix/test/gtest/pkixcheck_ParseValidity_tests.cpp
@@ -0,0 +1,83 @@
+/* -*- 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 2014 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 "pkixcheck.h"
+#include "pkixgtest.h"
+
+using namespace mozilla::pkix;
+using namespace mozilla::pkix::test;
+
+#define OLDER_UTCTIME \
+  0x17, 13,                               /* tag, length */ \
+  '9', '9', '0', '1', '0', '1',           /* (19)99-01-01 */ \
+  '0', '0', '0', '0', '0', '0', 'Z'       /* 00:00:00Z */
+
+#define NEWER_UTCTIME \
+  0x17, 13,                               /* tag, length */ \
+  '2', '1', '0', '1', '0', '1',           /* 2021-01-01 */ \
+  '0', '0', '0', '0', '0', '0', 'Z'       /* 00:00:00Z */
+
+static const Time FUTURE_TIME(YMDHMS(2025, 12, 31, 12, 23, 56));
+
+class pkixcheck_ParseValidity : public ::testing::Test { };
+
+TEST_F(pkixcheck_ParseValidity, BothEmptyNull)
+{
+  static const uint8_t DER[] = {
+    0x17/*UTCTime*/, 0/*length*/,
+    0x17/*UTCTime*/, 0/*length*/,
+  };
+  static const Input validity(DER);
+  ASSERT_EQ(Result::ERROR_INVALID_DER_TIME, ParseValidity(validity));
+}
+
+TEST_F(pkixcheck_ParseValidity, NotBeforeEmptyNull)
+{
+  static const uint8_t DER[] = {
+    0x17/*UTCTime*/, 0x00/*length*/,
+    NEWER_UTCTIME
+  };
+  static const Input validity(DER);
+  ASSERT_EQ(Result::ERROR_INVALID_DER_TIME, ParseValidity(validity));
+}
+
+TEST_F(pkixcheck_ParseValidity, NotAfterEmptyNull)
+{
+  static const uint8_t DER[] = {
+    NEWER_UTCTIME,
+    0x17/*UTCTime*/, 0x00/*length*/,
+  };
+  static const Input validity(DER);
+  ASSERT_EQ(Result::ERROR_INVALID_DER_TIME, ParseValidity(validity));
+}
+
+TEST_F(pkixcheck_ParseValidity, InvalidNotAfterBeforeNotBefore)
+{
+  static const uint8_t DER[] = {
+    NEWER_UTCTIME,
+    OLDER_UTCTIME,
+  };
+  static const Input validity(DER);
+  ASSERT_EQ(Result::ERROR_INVALID_DER_TIME, ParseValidity(validity));
+}
diff --git a/lib/mozpkix/test/gtest/pkixgtest.h b/lib/mozpkix/test/gtest/pkixgtest.h
index b271872df..a6fb9dfa3 100644
--- a/lib/mozpkix/test/gtest/pkixgtest.h
+++ b/lib/mozpkix/test/gtest/pkixgtest.h
@@ -126,7 +126,8 @@ public:
   }
 
   Result CheckSignatureDigestAlgorithm(DigestAlgorithm,
-                                       EndEntityOrCA) override
+                                       EndEntityOrCA,
+                                       Time) override
   {
     ADD_FAILURE();
     return NotReached("CheckSignatureDigestAlgorithm should not be called",
@@ -179,7 +180,8 @@ class DefaultCryptoTrustDomain : public EverythingFailsByDefaultTrustDomain
     return TestDigestBuf(item, digestAlg, digestBuf, digestBufLen);
   }
 
-  Result CheckSignatureDigestAlgorithm(DigestAlgorithm, EndEntityOrCA) override
+  Result CheckSignatureDigestAlgorithm(DigestAlgorithm, EndEntityOrCA, Time)
+                                       override
   {
     return Success;
   }