commit b2536a2c6234496ef609e7c909936bbf828dac6d
author Bob Beck <bbe@google.com> Mon Nov 07 17:16:55 2022 +0000
committer Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Mon Nov 07 22:43:58 2022 +0000
tree 327f0b98e76ed60e99bbfcf270caa71f911f0c5b
parent eeb3333f44cf0a8b0a1018dcfca54c72d8c4a3a3

Fix integer overflow in OPENSSL_gmtime_adj

OpenSSL uses integer parameters for this function, and the
multiplication here ends up being done as an integer.  Since we
support values up to year 9999, it is possible for someone to pass
in a number of days to the "adj" function to adjust a base time far
enough to overflow a 32 bit integer.

Change-Id: Iedfc33d8bf90d70049f99897df1d193fb29805d0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55125
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>

crypto/asn1/asn1_test.cc

diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
index 2f0ec76..7d9688b 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -1047,6 +1047,32 @@
   }
 }
 
+TEST(ASN1Test, AdjTime) {
+  struct tm tm1, tm2;
+  int days, secs;
+
+  OPENSSL_posix_to_tm(0, &tm1);
+  OPENSSL_posix_to_tm(0, &tm2);
+  // Test values that are too large and should be rejected.
+  EXPECT_FALSE(OPENSSL_gmtime_adj(&tm1, INT_MIN, INT_MIN));
+  EXPECT_FALSE(OPENSSL_gmtime_adj(&tm1, INT_MAX, INT_MAX));
+  // Basic functionality.
+  EXPECT_TRUE(OPENSSL_gmtime_adj(&tm2, 1, 1));
+  EXPECT_TRUE(OPENSSL_gmtime_diff(&days, &secs, &tm1, &tm2));
+  EXPECT_EQ(days, 1);
+  EXPECT_EQ(secs, 1);
+  EXPECT_TRUE(OPENSSL_gmtime_diff(&days, &secs, &tm2, &tm1));
+  EXPECT_EQ(days, -1);
+  EXPECT_EQ(secs, -1);
+  // Test a value of days that is very large, but valid.
+  EXPECT_TRUE(OPENSSL_gmtime_adj(&tm2, 2932800, 0));
+  EXPECT_TRUE(OPENSSL_gmtime_diff(&days, &secs, &tm1, &tm2));
+  EXPECT_EQ(days, 2932801);
+  EXPECT_EQ(secs, 1);
+  EXPECT_TRUE(OPENSSL_gmtime_diff(&days, &secs, &tm2, &tm1));
+  EXPECT_EQ(days, -2932801);
+  EXPECT_EQ(secs, -1);
+}
 static std::vector<uint8_t> StringToVector(const std::string &str) {
   return std::vector<uint8_t>(str.begin(), str.end());
 }

crypto/asn1/internal.h

diff --git a/crypto/asn1/internal.h b/crypto/asn1/internal.h
index 882b5fb..4f98a1a 100644
--- a/crypto/asn1/internal.h
+++ b/crypto/asn1/internal.h
@@ -96,7 +96,8 @@
 // |offset_day| days and |offset_sec| seconds. It returns zero on failure. |tm|
 // must be in the range of year 0000 to 9999 both before and after the update or
 // a failure will be returned.
-int OPENSSL_gmtime_adj(struct tm *tm, int offset_day, long offset_sec);
+OPENSSL_EXPORT int OPENSSL_gmtime_adj(struct tm *tm, int offset_day,
+                                      long offset_sec);
 
 // OPENSSL_gmtime_diff calculates the difference between |from| and |to|. It
 // returns one, and outputs the difference as a number of days and seconds in

crypto/asn1/posix_time.c

diff --git a/crypto/asn1/posix_time.c b/crypto/asn1/posix_time.c
index 81fbe83..7dd9e5b 100644
--- a/crypto/asn1/posix_time.c
+++ b/crypto/asn1/posix_time.c
@@ -191,9 +191,10 @@
                            tm->tm_hour, tm->tm_min, tm->tm_sec, &posix_time)) {
     return 0;
   }
-  if (!utc_from_posix_time(posix_time + off_day * SECS_PER_DAY + offset_sec,
-                           &tm->tm_year, &tm->tm_mon, &tm->tm_mday,
-                           &tm->tm_hour, &tm->tm_min, &tm->tm_sec)) {
+  if (!utc_from_posix_time(
+          posix_time + (int64_t)off_day * SECS_PER_DAY + offset_sec,
+          &tm->tm_year, &tm->tm_mon, &tm->tm_mday, &tm->tm_hour, &tm->tm_min,
+          &tm->tm_sec)) {
     return 0;
   }
   tm->tm_year -= 1900;