Avoid conversion overflow from struct tm. See discussion in https://boringssl-review.googlesource.com/c/boringssl/+/65967/comment/4b0fb2a6_78bfab3a/ struct tm is defined with tm_ values as all ints. Conversion inside this code is all bounds checked around valid times and can't overflow, but because struct tm uses 1 based months and 1900 based years, we need to modify the input values when converting a struct tm. Rather than do awkward bounds checks, just accept an int64_t on input and we don't have to care. OPENSSL_gmtime_adj gains checks to ensure the cumulative days and seconds values passed in may not overflow. While we are here we also ensure that OPENSSL_gmtime_adj does not modify the output struct tm in the failure case. Also while we are here, just make the offset_seconds value of OPENSSL_gmtime_adj an int64_t - because long was never the correct type. Add tests for all this. Change-Id: I40ac019c4274b5388c97736cf85cede951d8b7ae Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/66047 Commit-Queue: Bob Beck <bbe@google.com> Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com> Auto-Submit: Bob Beck <bbe@google.com>
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc index 76d6889..314ac3d 100644 --- a/crypto/asn1/asn1_test.cc +++ b/crypto/asn1/asn1_test.cc
@@ -1162,8 +1162,8 @@ struct tm tm1, tm2; int days, secs; - OPENSSL_posix_to_tm(0, &tm1); - OPENSSL_posix_to_tm(0, &tm2); + EXPECT_TRUE(OPENSSL_posix_to_tm(0, &tm1)); + EXPECT_TRUE(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)); @@ -2475,6 +2475,87 @@ #endif } +static auto TimeToTuple(const tm &t) { + return std::make_tuple(t.tm_year, t.tm_mon, t.tm_mday, t.tm_hour, t.tm_min, + t.tm_sec); +} + +TEST(ASN1Test, TimeOverflow) { + // Input time is out of range and may overflow internal calculations to shift + // |tm_year| and |tm_mon| to a more normal value. + tm overflow_year = {}; + overflow_year.tm_year = INT_MAX - 1899; + overflow_year.tm_mday = 1; + tm overflow_month = {}; + overflow_month.tm_mon = INT_MAX; + overflow_month.tm_mday = 1; + int64_t posix_u64; + EXPECT_FALSE(OPENSSL_tm_to_posix(&overflow_year, &posix_u64)); + EXPECT_FALSE(OPENSSL_tm_to_posix(&overflow_month, &posix_u64)); + time_t posix; + EXPECT_FALSE(OPENSSL_timegm(&overflow_year, &posix)); + EXPECT_FALSE(OPENSSL_timegm(&overflow_month, &posix)); + EXPECT_FALSE( + OPENSSL_gmtime_adj(&overflow_year, /*offset_day=*/0, /*offset_sec=*/0)); + EXPECT_FALSE( + OPENSSL_gmtime_adj(&overflow_month, /*offset_day=*/0, /*offset_sec=*/0)); + int days, secs; + EXPECT_FALSE( + OPENSSL_gmtime_diff(&days, &secs, &overflow_year, &overflow_year)); + EXPECT_FALSE( + OPENSSL_gmtime_diff(&days, &secs, &overflow_month, &overflow_month)); + + // Input time is in range, but even adding one second puts it out of range. + tm max_time = {}; + max_time.tm_year = 9999 - 1900; + max_time.tm_mon = 12 - 1; + max_time.tm_mday = 31; + max_time.tm_hour = 23; + max_time.tm_min = 59; + max_time.tm_sec = 59; + tm copy = max_time; + EXPECT_TRUE(OPENSSL_gmtime_adj(©, /*offset_day=*/0, /*offset_sec=*/0)); + EXPECT_EQ(TimeToTuple(copy), TimeToTuple(max_time)); + EXPECT_FALSE(OPENSSL_gmtime_adj(©, /*offset_day=*/0, /*offset_sec=*/1)); + + // Likewise for the earliest representable time. + tm min_time = {}; + min_time.tm_year = 0 - 1900; + min_time.tm_mon = 1 - 1; + min_time.tm_mday = 1; + min_time.tm_hour = 0; + min_time.tm_min = 0; + min_time.tm_sec = 0; + copy = min_time; + EXPECT_TRUE(OPENSSL_gmtime_adj(©, /*offset_day=*/0, /*offset_sec=*/0)); + EXPECT_EQ(TimeToTuple(copy), TimeToTuple(min_time)); + EXPECT_FALSE(OPENSSL_gmtime_adj(©, /*offset_day=*/0, /*offset_sec=*/-1)); + + // Test we can offset between the minimum and maximum times. + const int64_t kValidTimeRange = 315569519999; + copy = min_time; + EXPECT_TRUE(OPENSSL_gmtime_adj(©, /*offset_day=*/0, kValidTimeRange)); + EXPECT_EQ(TimeToTuple(copy), TimeToTuple(max_time)); + EXPECT_TRUE(OPENSSL_gmtime_adj(©, /*offset_day=*/0, -kValidTimeRange)); + EXPECT_EQ(TimeToTuple(copy), TimeToTuple(min_time)); + + // The second offset may even exceed kValidTimeRange if it is canceled out by + // offset_day. + EXPECT_TRUE(OPENSSL_gmtime_adj(©, /*offset_day=*/-1, + kValidTimeRange + 24 * 3600)); + EXPECT_EQ(TimeToTuple(copy), TimeToTuple(max_time)); + EXPECT_TRUE(OPENSSL_gmtime_adj(©, /*offset_day=*/1, + -kValidTimeRange - 24 * 3600)); + EXPECT_EQ(TimeToTuple(copy), TimeToTuple(min_time)); + + // Make sure the internal calculations for |OPENSSL_gmtime_adj| stay in + // bounds. + copy = max_time; + EXPECT_FALSE(OPENSSL_gmtime_adj(©, INT_MAX, LONG_MAX)); + copy = min_time; + EXPECT_FALSE(OPENSSL_gmtime_adj(©, INT_MIN, LONG_MIN)); +} + // The ASN.1 macros do not work on Windows shared library builds, where usage of // |OPENSSL_EXPORT| is a bit stricter. #if !defined(OPENSSL_WINDOWS) || !defined(BORINGSSL_SHARED_LIBRARY)
diff --git a/crypto/asn1/internal.h b/crypto/asn1/internal.h index 57e6966..601a3ca 100644 --- a/crypto/asn1/internal.h +++ b/crypto/asn1/internal.h
@@ -87,7 +87,7 @@ // must be in the range of year 0000 to 9999 both before and after the update or // a failure will be returned. OPENSSL_EXPORT int OPENSSL_gmtime_adj(struct tm *tm, int offset_day, - long offset_sec); + int64_t 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
diff --git a/crypto/asn1/posix_time.c b/crypto/asn1/posix_time.c index 2422bd3..ff95cca 100644 --- a/crypto/asn1/posix_time.c +++ b/crypto/asn1/posix_time.c
@@ -26,12 +26,12 @@ #include "internal.h" #define SECS_PER_HOUR (60 * 60) -#define SECS_PER_DAY (24 * SECS_PER_HOUR) +#define SECS_PER_DAY (INT64_C(24) * SECS_PER_HOUR) // Is a year/month/day combination valid, in the range from year 0000 // to 9999? -static int is_valid_date(int year, int month, int day) { +static int is_valid_date(int64_t year, int64_t month, int64_t day) { if (day < 1 || month < 1 || year < 0 || year > 9999) { return 0; } @@ -62,7 +62,7 @@ // Is a time valid? Leap seconds of 60 are not considered valid, as // the POSIX time in seconds does not include them. -static int is_valid_time(int hours, int minutes, int seconds) { +static int is_valid_time(int64_t hours, int64_t minutes, int64_t seconds) { if (hours < 0 || minutes < 0 || seconds < 0 || hours > 23 || minutes > 59 || seconds > 59) { return 0; @@ -70,17 +70,22 @@ return 1; } -// Is a int64 time representing a time within our expected range? -static int is_valid_epoch_time(int64_t time) { - // 0000-01-01 00:00:00 UTC to 9999-12-31 23:59:59 UTC - return (int64_t)-62167219200 <= time && time <= (int64_t)253402300799; +// 0000-01-01 00:00:00 UTC +#define MIN_POSIX_TIME INT64_C(-62167219200) +// 9999-12-31 23:59:59 UTC +#define MAX_POSIX_TIME INT64_C(253402300799) + +// Is an int64 time within our expected range? +static int is_valid_posix_time(int64_t time) { + return MIN_POSIX_TIME <= time && time <= MAX_POSIX_TIME; } // Inspired by algorithms presented in // https://howardhinnant.github.io/date_algorithms.html // (Public Domain) -static int posix_time_from_utc(int year, int month, int day, int hours, - int minutes, int seconds, int64_t *out_time) { +static int posix_time_from_utc(int64_t year, int64_t month, int64_t day, + int64_t hours, int64_t minutes, int64_t seconds, + int64_t *out_time) { if (!is_valid_date(year, month, day) || !is_valid_time(hours, minutes, seconds)) { return 0; @@ -108,7 +113,7 @@ static int utc_from_posix_time(int64_t time, int *out_year, int *out_month, int *out_day, int *out_hours, int *out_minutes, int *out_seconds) { - if (!is_valid_epoch_time(time)) { + if (!is_valid_posix_time(time)) { return 0; } int64_t days = time / SECS_PER_DAY; @@ -143,24 +148,21 @@ } int OPENSSL_tm_to_posix(const struct tm *tm, int64_t *out) { - // Ensure the additions below do not overflow. - if (tm->tm_year > 9999 || tm->tm_mon > 12) { - return 0; - } - - return posix_time_from_utc(tm->tm_year + 1900, tm->tm_mon + 1, tm->tm_mday, - tm->tm_hour, tm->tm_min, tm->tm_sec, out); + return posix_time_from_utc(tm->tm_year + INT64_C(1900), + tm->tm_mon + INT64_C(1), tm->tm_mday, tm->tm_hour, + tm->tm_min, tm->tm_sec, out); } int OPENSSL_posix_to_tm(int64_t time, struct tm *out_tm) { - memset(out_tm, 0, sizeof(struct tm)); - if (!utc_from_posix_time(time, &out_tm->tm_year, &out_tm->tm_mon, - &out_tm->tm_mday, &out_tm->tm_hour, &out_tm->tm_min, - &out_tm->tm_sec)) { + struct tm tmp_tm = {0}; + if (!utc_from_posix_time(time, &tmp_tm.tm_year, &tmp_tm.tm_mon, + &tmp_tm.tm_mday, &tmp_tm.tm_hour, &tmp_tm.tm_min, + &tmp_tm.tm_sec)) { return 0; } - out_tm->tm_year -= 1900; - out_tm->tm_mon -= 1; + tmp_tm.tm_year -= 1900; + tmp_tm.tm_mon -= 1; + *out_tm = tmp_tm; return 1; } @@ -192,43 +194,47 @@ return out_tm; } -int OPENSSL_gmtime_adj(struct tm *tm, int off_day, long offset_sec) { +int OPENSSL_gmtime_adj(struct tm *tm, int offset_day, int64_t offset_sec) { int64_t posix_time; - if (!posix_time_from_utc(tm->tm_year + 1900, tm->tm_mon + 1, tm->tm_mday, - tm->tm_hour, tm->tm_min, tm->tm_sec, &posix_time)) { + if (!OPENSSL_tm_to_posix(tm, &posix_time)) { return 0; } - 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)) { + static_assert(INT_MAX <= INT64_MAX / SECS_PER_DAY, + "day offset in seconds cannot overflow"); + static_assert(MAX_POSIX_TIME <= INT64_MAX - INT_MAX * SECS_PER_DAY, + "addition cannot overflow"); + static_assert(MIN_POSIX_TIME >= INT64_MIN - INT_MIN * SECS_PER_DAY, + "addition cannot underflow"); + posix_time += offset_day * SECS_PER_DAY; + if (posix_time > 0 && offset_sec > INT64_MAX - posix_time) { return 0; } - tm->tm_year -= 1900; - tm->tm_mon -= 1; + if (posix_time < 0 && offset_sec < INT64_MIN - posix_time) { + return 0; + } + posix_time += offset_sec; + + if (!OPENSSL_posix_to_tm(posix_time, tm)) { + return 0; + } return 1; } int OPENSSL_gmtime_diff(int *out_days, int *out_secs, const struct tm *from, const struct tm *to) { - int64_t time_to; - if (!posix_time_from_utc(to->tm_year + 1900, to->tm_mon + 1, to->tm_mday, - to->tm_hour, to->tm_min, to->tm_sec, &time_to)) { + int64_t time_to, time_from; + if (!OPENSSL_tm_to_posix(to, &time_to) || + !OPENSSL_tm_to_posix(from, &time_from)) { return 0; } - int64_t time_from; - if (!posix_time_from_utc(from->tm_year + 1900, from->tm_mon + 1, - from->tm_mday, from->tm_hour, from->tm_min, - from->tm_sec, &time_from)) { - return 0; - } + // Times are in range, so these calculations can not overflow. + static_assert(SECS_PER_DAY <= INT_MAX, "seconds per day does not fit in int"); + static_assert((MAX_POSIX_TIME - MIN_POSIX_TIME) / SECS_PER_DAY <= INT_MAX, + "range of valid POSIX times, in days, does not fit in int"); int64_t timediff = time_to - time_from; int64_t daydiff = timediff / SECS_PER_DAY; timediff %= SECS_PER_DAY; - if (daydiff > INT_MAX || daydiff < INT_MIN) { - return 0; - } *out_secs = (int)timediff; *out_days = (int)daydiff; return 1;