Add CheckOCSPDateValid() to net/cert/internal

net/cert/internal/parse_ocsp_unittest.cc

Index: net/cert/internal/parse_ocsp_unittest.cc
diff --git a/net/cert/internal/parse_ocsp_unittest.cc b/net/cert/internal/parse_ocsp_unittest.cc
index c0fc061b5e43243f39d156444df42a61f38915e5..0e30799c32babe04d08167d8532ad42065f16d16 100644
--- a/net/cert/internal/parse_ocsp_unittest.cc
+++ b/net/cert/internal/parse_ocsp_unittest.cc
@@ -8,6 +8,7 @@
 #include "base/logging.h"
 #include "net/cert/internal/test_helpers.h"
 #include "net/cert/x509_certificate.h"
+#include "net/der/encode_values.h"
 #include "net/test/test_data_directory.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -15,6 +16,12 @@ namespace net {
 
 namespace {
 
+const base::TimeDelta kOCSPAgeOneWeek = base::TimeDelta::FromDays(7);
+
+const base::Time kWindowsEpoch =
+    base::Time::UnixEpoch() -
+    base::TimeDelta::FromSeconds(INT64_C(11644473600));

[Ryan Sleevi, 2016/07/08 02:21:02]

No need for the INT64_C now that we're C++11 (which added the support for int64
constants w/o the size specifier)

That said, base::Time(0) is equivalent to the Windows Epoch (it's part of
base::Time's API contract -
https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=7 ) so this should
be unnecessary

[dadrian, 2016/07/08 22:25:42]

Done.

+
 std::string GetFilePath(const std::string& file_name) {
   return std::string("net/data/parse_ocsp_unittest/") + file_name;
 }
@@ -182,4 +189,129 @@ TEST(ParseOCSPTest, OCSPMissingResponse) {
   ASSERT_EQ(PARSE_OCSP_SINGLE_RESPONSE, ParseOCSP("missing_response.pem"));
 }
 
+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 verify_time = kWindowsEpoch - base::TimeDelta::FromDays(1);

[Ryan Sleevi, 2016/07/08 02:21:02]

I think my concerns about Windows Epoch may have been confused. That is, I don't
ever expect we'll send a junk verify_time in to these routines (and if we do,
always failing is a perfectly reasonable response). My concern about Windows
Epochs was related to this_update and next_update fields, since those are the
'untrusted' data that comes from CAs.

That is, imagine a CA is compromised and issues an OCSP response that, for
whatever reason, says 1600 (or 2039 on 32-bit, but let's use Windows as the
example)

My hope and expectation is that we reject it.

|verify_time| I'm not worried about (because we should always be programatically
checking that, and so DCHECK'ing on junk is fine, and base::Time::Now()
shouldn't give us junk), and while the max age has the theoretical risk of
causing overflow, that's also a developer screw up.

[dadrian, 2016/07/08 18:04:46]

Since GeneralizedTime handles times from before the Windows epoch, and we
convert _to_ generalized time, this is the only possible error condition we can
force here. This test just checks that we don't accidentally fail open.

+
+  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 verify_time = kWindowsEpoch + base::TimeDelta::FromDays(1);
+
+  base::Time this_update = kWindowsEpoch;
+  ASSERT_TRUE(
+      der::EncodeTimeAsGeneralizedTime(this_update, &response.this_update));
+  response.has_next_update = false;
+#ifdef OS_WIN
+  EXPECT_FALSE(CheckOCSPDateValid(response, verify_time, kOCSPAgeOneWeek));
+#else
+  EXPECT_TRUE(CheckOCSPDateValid(response, verify_time, kOCSPAgeOneWeek));
+#endif
+}
+
 }  // namespace net