[PKI library] abstract CheckOCSPDateValid into general CheckRevocationDateValid function.

CRLs use the same concept of thisUpdate & nextUpdate, split the date
checking function out so that it can be used by both OCSP and CRL
checking.

Bug: 749276
Change-Id: I641db6f92062422ff70697353bd4d435016dc2c4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1628167
Commit-Queue: Matt Mueller <mattm@chromium.org>
Reviewed-by: Eric Roman <eroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664120}

net/BUILD.gn

@@ -220,6 +220,8 @@ component("net") {
     "cert/internal/path_builder.h",
     "cert/internal/revocation_checker.cc",
     "cert/internal/revocation_checker.h",
+    "cert/internal/revocation_util.cc",
+    "cert/internal/revocation_util.h",
     "cert/internal/signature_algorithm.cc",
     "cert/internal/signature_algorithm.h",
     "cert/internal/simple_path_builder_delegate.cc",
@@ -4973,6 +4975,7 @@ test("net_unittests") {
     "cert/internal/path_builder_unittest.cc",
     "cert/internal/path_builder_verify_certificate_chain_unittest.cc",
     "cert/internal/revocation_checker_unittest.cc",
+    "cert/internal/revocation_util_unittest.cc",
     "cert/internal/signature_algorithm_unittest.cc",
     "cert/internal/simple_path_builder_delegate_unittest.cc",
     "cert/internal/test_helpers.cc",

net/cert/internal/ocsp.cc

@@ -13,10 +13,10 @@
 #include "net/cert/internal/cert_errors.h"
 #include "net/cert/internal/extended_key_usage.h"
 #include "net/cert/internal/parsed_certificate.h"
+#include "net/cert/internal/revocation_util.h"
 #include "net/cert/internal/verify_name_match.h"
 #include "net/cert/internal/verify_signed_data.h"
 #include "net/cert/x509_util.h"
-#include "net/der/encode_values.h"
 #include "third_party/boringssl/src/include/openssl/bytestring.h"
 #include "third_party/boringssl/src/include/openssl/digest.h"
 #include "third_party/boringssl/src/include/openssl/mem.h"
@@ -705,7 +705,11 @@ OCSPRevocationStatus GetRevocationStatusForCert(
     // serial numbers. If an OCSP responder provides both an up to date
     // response and an expired response, the up to date response takes
     // precedence (PROVIDED > INVALID_DATE).
-    if (!CheckOCSPDateValid(single_response, verify_time, max_age)) {
+    if (!CheckRevocationDateValid(single_response.this_update,
+                                  single_response.has_next_update
+                                      ? &single_response.next_update
+                                      : nullptr,
+                                  verify_time, max_age)) {
       if (*response_details != OCSPVerifyResult::PROVIDED)
         *response_details = OCSPVerifyResult::INVALID_DATE;
       continue;
@@ -815,30 +819,6 @@ OCSPRevocationStatus CheckOCSP(
   return status;
 }
 
-bool CheckOCSPDateValid(const OCSPSingleResponse& response,
-                        const base::Time& verify_time,
-                        const base::TimeDelta& max_age) {
-  der::GeneralizedTime verify_time_der;
-  if (!der::EncodeTimeAsGeneralizedTime(verify_time, &verify_time_der))
-    return false;
-
-  if (response.this_update > verify_time_der)
-    return false;  // Response is not yet valid.
-
-  if (response.has_next_update && (response.next_update <= verify_time_der))
-    return false;  // Response is no longer valid.
-
-  der::GeneralizedTime earliest_this_update;
-  if (!der::EncodeTimeAsGeneralizedTime(verify_time - max_age,
-                                        &earliest_this_update)) {
-    return false;
-  }
-  if (response.this_update < earliest_this_update)
-    return false;  // Response is too old.
-
-  return true;
-}
-
 bool CreateOCSPRequest(const ParsedCertificate* cert,
                        const ParsedCertificate* issuer,
                        std::vector<uint8_t>* request_der) {

net/cert/internal/ocsp.h

@@ -313,15 +313,6 @@ NET_EXPORT OCSPRevocationStatus CheckOCSP(
     const base::TimeDelta& max_age,
     OCSPVerifyResult::ResponseStatus* response_details) WARN_UNUSED_RESULT;
 
-// Returns true if |response|, a valid OCSP response with a thisUpdate field and
-// potentially a nextUpdate field, is valid at |verify_time| and not older than
-// |max_age|. Expressed differently, returns true if |response.thisUpdate| <=
-// |verify_time| < response.nextUpdate, and |response.thisUpdate| >=
-// |verify_time| - |max_age|.
-NET_EXPORT_PRIVATE bool CheckOCSPDateValid(const OCSPSingleResponse& response,
-                                           const base::Time& verify_time,
-                                           const base::TimeDelta& max_age);
-
 // Creates a DER-encoded OCSPRequest for |cert|. The request is fairly basic:
 //  * No signature
 //  * No requestorName

net/cert/internal/ocsp_unittest.cc

@@ -7,7 +7,6 @@
 #include "base/base64.h"
 #include "base/logging.h"
 #include "base/strings/string_util.h"
-#include "build/build_config.h"
 #include "net/cert/internal/test_helpers.h"
 #include "net/der/encode_values.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -171,133 +170,6 @@ TEST_P(CheckOCSPTest, FromFile) {
             der::Input(&request_data));
 }
 
-TEST(OCSPDateTest, Valid) {
-  OCSPSingleResponse response;
-
-  base::Time now = base::Time::Now();
-  base::Time this_update = now - base::TimeDelta::FromHours(1);
-  ASSERT_TRUE(
-      der::EncodeTimeAsGeneralizedTime(this_update, &response.this_update));
-  response.has_next_update = false;
-  EXPECT_TRUE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));
-
-  base::Time next_update = this_update + base::TimeDelta::FromDays(7);
-  ASSERT_TRUE(
-      der::EncodeTimeAsGeneralizedTime(next_update, &response.next_update));
-  response.has_next_update = true;
-  EXPECT_TRUE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));
-}
-
-TEST(OCSPDateTest, ThisUpdateInTheFuture) {
-  OCSPSingleResponse response;
-
-  base::Time now = base::Time::Now();
-  base::Time this_update = now + base::TimeDelta::FromHours(1);
-  ASSERT_TRUE(
-      der::EncodeTimeAsGeneralizedTime(this_update, &response.this_update));
-  response.has_next_update = false;
-  EXPECT_FALSE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));
-
-  base::Time next_update = this_update + base::TimeDelta::FromDays(7);
-  ASSERT_TRUE(
-      der::EncodeTimeAsGeneralizedTime(next_update, &response.next_update));
-  response.has_next_update = true;
-  EXPECT_FALSE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));
-}
-
-TEST(OCSPDateTest, NextUpdatePassed) {
-  OCSPSingleResponse response;
-
-  base::Time now = base::Time::Now();
-  base::Time this_update = now - base::TimeDelta::FromDays(6);
-  ASSERT_TRUE(
-      der::EncodeTimeAsGeneralizedTime(this_update, &response.this_update));
-  response.has_next_update = false;
-  EXPECT_TRUE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));
-
-  base::Time next_update = now - base::TimeDelta::FromHours(1);
-  ASSERT_TRUE(
-      der::EncodeTimeAsGeneralizedTime(next_update, &response.next_update));
-  response.has_next_update = true;
-  EXPECT_FALSE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));
-}
-
-TEST(OCSPDateTest, NextUpdateBeforeThisUpdate) {
-  OCSPSingleResponse response;
-
-  base::Time now = base::Time::Now();
-  base::Time this_update = now - base::TimeDelta::FromDays(1);
-  ASSERT_TRUE(
-      der::EncodeTimeAsGeneralizedTime(this_update, &response.this_update));
-  response.has_next_update = false;
-  EXPECT_TRUE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));
-
-  base::Time next_update = this_update - base::TimeDelta::FromDays(1);
-  ASSERT_TRUE(
-      der::EncodeTimeAsGeneralizedTime(next_update, &response.next_update));
-  response.has_next_update = true;
-  EXPECT_FALSE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));
-}
-
-TEST(OCSPDateTest, ThisUpdateOlderThanMaxAge) {
-  OCSPSingleResponse response;
-
-  base::Time now = base::Time::Now();
-  base::Time this_update = now - kOCSPAgeOneWeek;
-  ASSERT_TRUE(
-      der::EncodeTimeAsGeneralizedTime(this_update, &response.this_update));
-  response.has_next_update = false;
-  EXPECT_TRUE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));
-
-  base::Time next_update = now + base::TimeDelta::FromHours(1);
-  ASSERT_TRUE(
-      der::EncodeTimeAsGeneralizedTime(next_update, &response.next_update));
-  response.has_next_update = true;
-  EXPECT_TRUE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));
-
-  ASSERT_TRUE(der::EncodeTimeAsGeneralizedTime(
-      this_update - base::TimeDelta::FromSeconds(1), &response.this_update));
-  response.has_next_update = false;
-  EXPECT_FALSE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));
-  response.has_next_update = true;
-  EXPECT_FALSE(CheckOCSPDateValid(response, now, kOCSPAgeOneWeek));
-}
-
-TEST(OCSPDateTest, VerifyTimeFromBeforeWindowsEpoch) {
-  OCSPSingleResponse response;
-  base::Time windows_epoch;
-  base::Time verify_time = windows_epoch - base::TimeDelta::FromDays(1);
-
-  base::Time now = base::Time::Now();
-  base::Time this_update = now - base::TimeDelta::FromHours(1);
-  ASSERT_TRUE(
-      der::EncodeTimeAsGeneralizedTime(this_update, &response.this_update));
-  response.has_next_update = false;
-  EXPECT_FALSE(CheckOCSPDateValid(response, verify_time, kOCSPAgeOneWeek));
-
-  base::Time next_update = this_update + kOCSPAgeOneWeek;
-  ASSERT_TRUE(
-      der::EncodeTimeAsGeneralizedTime(next_update, &response.next_update));
-  response.has_next_update = true;
-  EXPECT_FALSE(CheckOCSPDateValid(response, verify_time, kOCSPAgeOneWeek));
-}
-
-TEST(OCSPDateTest, VerifyTimeMinusAgeFromBeforeWindowsEpoch) {
-  OCSPSingleResponse response;
-  base::Time windows_epoch;
-  base::Time verify_time = windows_epoch + base::TimeDelta::FromDays(1);
-
-  base::Time this_update = windows_epoch;
-  ASSERT_TRUE(
-      der::EncodeTimeAsGeneralizedTime(this_update, &response.this_update));
-  response.has_next_update = false;
-#if defined(OS_WIN)
-  EXPECT_FALSE(CheckOCSPDateValid(response, verify_time, kOCSPAgeOneWeek));
-#else
-  EXPECT_TRUE(CheckOCSPDateValid(response, verify_time, kOCSPAgeOneWeek));
-#endif
-}
-
 base::StringPiece kGetURLTestParams[] = {
     "http://www.example.com/", "http://www.example.com/path/",
     "http://www.example.com/path",

net/cert/internal/revocation_util.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/cert/internal/revocation_util.h"
+
+#include "base/time/time.h"
+#include "net/der/encode_values.h"
+#include "net/der/parse_values.h"
+
+namespace net {
+
+bool CheckRevocationDateValid(const der::GeneralizedTime& this_update,
+                              const der::GeneralizedTime* next_update,
+                              const base::Time& verify_time,
+                              const base::TimeDelta& max_age) {
+  der::GeneralizedTime verify_time_der;
+  if (!der::EncodeTimeAsGeneralizedTime(verify_time, &verify_time_der))
+    return false;
+
+  if (this_update > verify_time_der)
+    return false;  // Response is not yet valid.
+
+  if (next_update && (*next_update <= verify_time_der))
+    return false;  // Response is no longer valid.
+
+  der::GeneralizedTime earliest_this_update;
+  if (!der::EncodeTimeAsGeneralizedTime(verify_time - max_age,
+                                        &earliest_this_update)) {
+    return false;
+  }
+  if (this_update < earliest_this_update)
+    return false;  // Response is too old.
+
+  return true;
+}
+
+}  // namespace net

net/cert/internal/revocation_util.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef NET_CERT_INTERNAL_REVOCATION_UTIL_H_
+#define NET_CERT_INTERNAL_REVOCATION_UTIL_H_
+
+#include "base/optional.h"
+#include "net/base/net_export.h"
+
+namespace base {
+class Time;
+class TimeDelta;
+}  // namespace base
+
+namespace net {
+
+namespace der {
+struct GeneralizedTime;
+}
+
+// Returns true if a revocation status with |this_update| field and potentially
+// a |next_update| field, is valid at |verify_time| and not older than
+// |max_age|.  Expressed differently, returns true if |this_update <=
+// verify_time < next_update|, and |this_update >= verify_time - max_age|.
+NET_EXPORT_PRIVATE bool CheckRevocationDateValid(
+    const der::GeneralizedTime& this_update,
+    const der::GeneralizedTime* next_update,
+    const base::Time& verify_time,
+    const base::TimeDelta& max_age) WARN_UNUSED_RESULT;
+
+}  // namespace net
+
+#endif  // NET_CERT_INTERNAL_REVOCATION_UTIL_H_

net/cert/internal/revocation_util_unittest.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/cert/internal/revocation_util.h"
+
+#include "base/time/time.h"
+#include "build/build_config.h"
+#include "net/der/encode_values.h"
+#include "net/der/parse_values.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+
+namespace {
+
+constexpr base::TimeDelta kOneWeek = base::TimeDelta::FromDays(7);
+
+}  // namespace
+
+TEST(CheckRevocationDateTest, Valid) {
+  base::Time now = base::Time::Now();
+  base::Time this_update = now - base::TimeDelta::FromHours(1);
+  der::GeneralizedTime encoded_this_update;
+  ASSERT_TRUE(
+      der::EncodeTimeAsGeneralizedTime(this_update, &encoded_this_update));
+  EXPECT_TRUE(
+      CheckRevocationDateValid(encoded_this_update, nullptr, now, kOneWeek));
+
+  base::Time next_update = this_update + base::TimeDelta::FromDays(7);
+  der::GeneralizedTime encoded_next_update;
+  ASSERT_TRUE(
+      der::EncodeTimeAsGeneralizedTime(next_update, &encoded_next_update));
+  EXPECT_TRUE(CheckRevocationDateValid(encoded_this_update,
+                                       &encoded_next_update, now, kOneWeek));
+}
+
+TEST(CheckRevocationDateTest, ThisUpdateInTheFuture) {
+  base::Time now = base::Time::Now();
+  base::Time this_update = now + base::TimeDelta::FromHours(1);
+  der::GeneralizedTime encoded_this_update;
+  ASSERT_TRUE(
+      der::EncodeTimeAsGeneralizedTime(this_update, &encoded_this_update));
+  EXPECT_FALSE(
+      CheckRevocationDateValid(encoded_this_update, nullptr, now, kOneWeek));
+
+  base::Time next_update = this_update + base::TimeDelta::FromDays(7);
+  der::GeneralizedTime encoded_next_update;
+  ASSERT_TRUE(
+      der::EncodeTimeAsGeneralizedTime(next_update, &encoded_next_update));
+  EXPECT_FALSE(CheckRevocationDateValid(encoded_this_update,
+                                        &encoded_next_update, now, kOneWeek));
+}
+
+TEST(CheckRevocationDateTest, NextUpdatePassed) {
+  base::Time now = base::Time::Now();
+  base::Time this_update = now - base::TimeDelta::FromDays(6);
+  der::GeneralizedTime encoded_this_update;
+  ASSERT_TRUE(
+      der::EncodeTimeAsGeneralizedTime(this_update, &encoded_this_update));
+  EXPECT_TRUE(
+      CheckRevocationDateValid(encoded_this_update, nullptr, now, kOneWeek));
+
+  base::Time next_update = now - base::TimeDelta::FromHours(1);
+  der::GeneralizedTime encoded_next_update;
+  ASSERT_TRUE(
+      der::EncodeTimeAsGeneralizedTime(next_update, &encoded_next_update));
+  EXPECT_FALSE(CheckRevocationDateValid(encoded_this_update,
+                                        &encoded_next_update, now, kOneWeek));
+}
+
+TEST(CheckRevocationDateTest, NextUpdateBeforeThisUpdate) {
+  base::Time now = base::Time::Now();
+  base::Time this_update = now - base::TimeDelta::FromDays(1);
+  der::GeneralizedTime encoded_this_update;
+  ASSERT_TRUE(
+      der::EncodeTimeAsGeneralizedTime(this_update, &encoded_this_update));
+  EXPECT_TRUE(
+      CheckRevocationDateValid(encoded_this_update, nullptr, now, kOneWeek));
+
+  base::Time next_update = this_update - base::TimeDelta::FromDays(1);
+  der::GeneralizedTime encoded_next_update;
+  ASSERT_TRUE(
+      der::EncodeTimeAsGeneralizedTime(next_update, &encoded_next_update));
+  EXPECT_FALSE(CheckRevocationDateValid(encoded_this_update,
+                                        &encoded_next_update, now, kOneWeek));
+}
+
+TEST(CheckRevocationDateTest, ThisUpdateOlderThanMaxAge) {
+  base::Time now = base::Time::Now();
+  base::Time this_update = now - kOneWeek;
+  der::GeneralizedTime encoded_this_update;
+  ASSERT_TRUE(
+      der::EncodeTimeAsGeneralizedTime(this_update, &encoded_this_update));
+  EXPECT_TRUE(
+      CheckRevocationDateValid(encoded_this_update, nullptr, now, kOneWeek));
+
+  base::Time next_update = now + base::TimeDelta::FromHours(1);
+  der::GeneralizedTime encoded_next_update;
+  ASSERT_TRUE(
+      der::EncodeTimeAsGeneralizedTime(next_update, &encoded_next_update));
+  EXPECT_TRUE(CheckRevocationDateValid(encoded_this_update,
+                                       &encoded_next_update, now, kOneWeek));
+
+  ASSERT_TRUE(der::EncodeTimeAsGeneralizedTime(
+      this_update - base::TimeDelta::FromSeconds(1), &encoded_this_update));
+  EXPECT_FALSE(
+      CheckRevocationDateValid(encoded_this_update, nullptr, now, kOneWeek));
+  EXPECT_FALSE(CheckRevocationDateValid(encoded_this_update,
+                                        &encoded_next_update, now, kOneWeek));
+}
+
+TEST(CheckRevocationDateTest, VerifyTimeFromBeforeWindowsEpoch) {
+  base::Time windows_epoch;
+  base::Time verify_time = windows_epoch - base::TimeDelta::FromDays(1);
+
+  base::Time now = base::Time::Now();
+  base::Time this_update = now - base::TimeDelta::FromHours(1);
+  der::GeneralizedTime encoded_this_update;
+  ASSERT_TRUE(
+      der::EncodeTimeAsGeneralizedTime(this_update, &encoded_this_update));
+  EXPECT_FALSE(CheckRevocationDateValid(encoded_this_update, nullptr,
+                                        verify_time, kOneWeek));
+
+  base::Time next_update = this_update + kOneWeek;
+  der::GeneralizedTime encoded_next_update;
+  ASSERT_TRUE(
+      der::EncodeTimeAsGeneralizedTime(next_update, &encoded_next_update));
+  EXPECT_FALSE(CheckRevocationDateValid(
+      encoded_this_update, &encoded_next_update, verify_time, kOneWeek));
+}
+
+TEST(CheckRevocationDateTest, VerifyTimeMinusAgeFromBeforeWindowsEpoch) {
+  base::Time windows_epoch;
+  base::Time verify_time = windows_epoch + base::TimeDelta::FromDays(1);
+
+  base::Time this_update = windows_epoch;
+  der::GeneralizedTime encoded_this_update;
+  ASSERT_TRUE(
+      der::EncodeTimeAsGeneralizedTime(this_update, &encoded_this_update));
+#if defined(OS_WIN)
+  EXPECT_FALSE(CheckRevocationDateValid(encoded_this_update, nullptr,
+                                        verify_time, kOneWeek));
+#else
+  EXPECT_TRUE(CheckRevocationDateValid(encoded_this_update, nullptr,
+                                       verify_time, kOneWeek));
+#endif
+}
+
+}  // namespace net