Reimplement ASN1_TIME_print with the new parser. No sense in keeping two around. This does cause the functions to reject some previously accepted invalid inputs. These were intentionally accepted by https://boringssl-review.googlesource.com/c/boringssl/+/13082 for an old version of M2Crypto, but I belive we no longer need to be compatible with that. Update-Note: ASN1_TIME_print, ASN1_UTCTIME_print, and ASN1_GENERALIZEDTIME_print will no longer accept various invalid inputs. Change-Id: I4606d0b39585a19eb4b984ac809706e497a3f799 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53090 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Bob Beck <bbe@google.com>
diff --git a/crypto/asn1/a_strex.c b/crypto/asn1/a_strex.c index 52d979e..e6aacf0 100644 --- a/crypto/asn1/a_strex.c +++ b/crypto/asn1/a_strex.c
@@ -60,8 +60,10 @@ #include <ctype.h> #include <inttypes.h> #include <string.h> +#include <time.h> #include <openssl/bio.h> +#include <openssl/bytestring.h> #include <openssl/mem.h> #include "internal.h" @@ -464,7 +466,7 @@ if (tm->type == V_ASN1_GENERALIZEDTIME) { return ASN1_GENERALIZEDTIME_print(bp, tm); } - BIO_write(bp, "Bad time value", 14); + BIO_puts(bp, "Bad time value"); return 0; } @@ -472,136 +474,29 @@ "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"}; int ASN1_GENERALIZEDTIME_print(BIO *bp, const ASN1_GENERALIZEDTIME *tm) { - char *v; - int gmt = 0; - int i; - int y = 0, M = 0, d = 0, h = 0, m = 0, s = 0; - char *f = NULL; - int f_len = 0; - - i = tm->length; - v = (char *)tm->data; - - if (i < 12) { - goto err; - } - if (v[i - 1] == 'Z') { - gmt = 1; - } - for (i = 0; i < 12; i++) { - if ((v[i] > '9') || (v[i] < '0')) { - goto err; - } - } - y = (v[0] - '0') * 1000 + (v[1] - '0') * 100 + (v[2] - '0') * 10 + - (v[3] - '0'); - M = (v[4] - '0') * 10 + (v[5] - '0'); - if ((M > 12) || (M < 1)) { - goto err; - } - d = (v[6] - '0') * 10 + (v[7] - '0'); - h = (v[8] - '0') * 10 + (v[9] - '0'); - m = (v[10] - '0') * 10 + (v[11] - '0'); - if (tm->length >= 14 && (v[12] >= '0') && (v[12] <= '9') && (v[13] >= '0') && - (v[13] <= '9')) { - s = (v[12] - '0') * 10 + (v[13] - '0'); - // Check for fractions of seconds. - if (tm->length >= 15 && v[14] == '.') { - int l = tm->length; - f = &v[14]; // The decimal point. - f_len = 1; - while (14 + f_len < l && f[f_len] >= '0' && f[f_len] <= '9') { - ++f_len; - } - } - } - - if (BIO_printf(bp, "%s %2d %02d:%02d:%02d%.*s %d%s", mon[M - 1], d, h, m, s, - f_len, f, y, (gmt) ? " GMT" : "") <= 0) { - return 0; - } else { - return 1; - } -err: - BIO_write(bp, "Bad time value", 14); - return 0; -} - -// consume_two_digits is a helper function for ASN1_UTCTIME_print. If |*v|, -// assumed to be |*len| bytes long, has two leading digits, updates |*out| with -// their value, updates |v| and |len|, and returns one. Otherwise, returns -// zero. -static int consume_two_digits(int *out, const char **v, int *len) { - if (*len < 2 || !isdigit((unsigned char)((*v)[0])) || - !isdigit((unsigned char)((*v)[1]))) { - return 0; - } - *out = ((*v)[0] - '0') * 10 + ((*v)[1] - '0'); - *len -= 2; - *v += 2; - return 1; -} - -// consume_zulu_timezone is a helper function for ASN1_UTCTIME_print. If |*v|, -// assumed to be |*len| bytes long, starts with "Z" then it updates |*v| and -// |*len| and returns one. Otherwise returns zero. -static int consume_zulu_timezone(const char **v, int *len) { - if (*len == 0 || (*v)[0] != 'Z') { + CBS cbs; + CBS_init(&cbs, tm->data, tm->length); + struct tm utc; + if (!CBS_parse_generalized_time(&cbs, &utc, /*allow_timezone_offset=*/0)) { + BIO_puts(bp, "Bad time value"); return 0; } - *len -= 1; - *v += 1; - return 1; + return BIO_printf(bp, "%s %2d %02d:%02d:%02d %d GMT", mon[utc.tm_mon], + utc.tm_mday, utc.tm_hour, utc.tm_min, utc.tm_sec, + utc.tm_year + 1900) > 0; } int ASN1_UTCTIME_print(BIO *bp, const ASN1_UTCTIME *tm) { - const char *v = (const char *)tm->data; - int len = tm->length; - int Y = 0, M = 0, D = 0, h = 0, m = 0, s = 0; - - // YYMMDDhhmm are required to be present. - if (!consume_two_digits(&Y, &v, &len) || !consume_two_digits(&M, &v, &len) || - !consume_two_digits(&D, &v, &len) || !consume_two_digits(&h, &v, &len) || - !consume_two_digits(&m, &v, &len)) { - goto err; - } - // https://tools.ietf.org/html/rfc5280, section 4.1.2.5.1, requires seconds - // to be present, but historically this code has forgiven its absence. - consume_two_digits(&s, &v, &len); - - // https://tools.ietf.org/html/rfc5280, section 4.1.2.5.1, specifies this - // interpretation of the year. - if (Y < 50) { - Y += 2000; - } else { - Y += 1900; - } - if (M > 12 || M == 0) { - goto err; - } - if (D > 31 || D == 0) { - goto err; - } - if (h > 23 || m > 59 || s > 60) { - goto err; + CBS cbs; + CBS_init(&cbs, tm->data, tm->length); + struct tm utc; + if (!CBS_parse_utc_time(&cbs, &utc, /*allow_timezone_offset=*/0)) { + BIO_puts(bp, "Bad time value"); + return 0; } - // https://tools.ietf.org/html/rfc5280, section 4.1.2.5.1, requires the "Z" - // to be present, but historically this code has forgiven its absence. - const int is_gmt = consume_zulu_timezone(&v, &len); - - // https://tools.ietf.org/html/rfc5280, section 4.1.2.5.1, does not permit - // the specification of timezones using the +hhmm / -hhmm syntax, which is - // the only other thing that might legitimately be found at the end. - if (len) { - goto err; - } - - return BIO_printf(bp, "%s %2d %02d:%02d:%02d %d%s", mon[M - 1], D, h, m, s, Y, - is_gmt ? " GMT" : "") > 0; - -err: - BIO_write(bp, "Bad time value", 14); - return 0; + return BIO_printf(bp, "%s %2d %02d:%02d:%02d %d GMT", mon[utc.tm_mon], + utc.tm_mday, utc.tm_hour, utc.tm_min, utc.tm_sec, + utc.tm_year + 1900) > 0; }
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc index c4f47bb..442c1ea 100644 --- a/crypto/asn1/asn1_test.cc +++ b/crypto/asn1/asn1_test.cc
@@ -23,6 +23,7 @@ #include <openssl/asn1.h> #include <openssl/asn1t.h> +#include <openssl/bio.h> #include <openssl/bytestring.h> #include <openssl/err.h> #include <openssl/mem.h> @@ -926,28 +927,46 @@ return day == 0 && sec ==0; } +static std::string PrintStringToBIO(const ASN1_STRING *str, + int (*print_func)(BIO *, + const ASN1_STRING *)) { + const uint8_t *data; + size_t len; + bssl::UniquePtr<BIO> bio(BIO_new(BIO_s_mem())); + if (!bio || // + !print_func(bio.get(), str) || + !BIO_mem_contents(bio.get(), &data, &len)) { + ADD_FAILURE() << "Could not print to BIO"; + return ""; + } + return std::string(data, data + len); +} + TEST(ASN1Test, SetTime) { static const struct { time_t time; const char *generalized; const char *utc; + const char *printed; } kTests[] = { - {-631152001, "19491231235959Z", nullptr}, - {-631152000, "19500101000000Z", "500101000000Z"}, - {0, "19700101000000Z", "700101000000Z"}, - {981173106, "20010203040506Z", "010203040506Z"}, - {951804000, "20000229060000Z", "000229060000Z"}, + {-631152001, "19491231235959Z", nullptr, "Dec 31 23:59:59 1949 GMT"}, + {-631152000, "19500101000000Z", "500101000000Z", + "Jan 1 00:00:00 1950 GMT"}, + {0, "19700101000000Z", "700101000000Z", "Jan 1 00:00:00 1970 GMT"}, + {981173106, "20010203040506Z", "010203040506Z", "Feb 3 04:05:06 2001 GMT"}, + {951804000, "20000229060000Z", "000229060000Z", "Feb 29 06:00:00 2000 GMT"}, #if defined(OPENSSL_64_BIT) // TODO(https://crbug.com/boringssl/416): These cases overflow 32-bit // |time_t| and do not consistently work on 32-bit platforms. For now, // disable the tests on 32-bit. Re-enable them once the bug is fixed. - {2524607999, "20491231235959Z", "491231235959Z"}, - {2524608000, "20500101000000Z", nullptr}, + {2524607999, "20491231235959Z", "491231235959Z", + "Dec 31 23:59:59 2049 GMT"}, + {2524608000, "20500101000000Z", nullptr, "Jan 1 00:00:00 2050 GMT"}, // Test boundary conditions. - {-62167219200, "00000101000000Z", nullptr}, - {-62167219201, nullptr, nullptr}, - {253402300799, "99991231235959Z", nullptr}, - {253402300800, nullptr, nullptr}, + {-62167219200, "00000101000000Z", nullptr, "Jan 1 00:00:00 0 GMT"}, + {-62167219201, nullptr, nullptr, nullptr}, + {253402300799, "99991231235959Z", nullptr, "Dec 31 23:59:59 9999 GMT"}, + {253402300800, nullptr, nullptr, nullptr}, #endif }; for (const auto &t : kTests) { @@ -966,6 +985,8 @@ EXPECT_EQ(V_ASN1_UTCTIME, ASN1_STRING_type(utc.get())); EXPECT_EQ(t.utc, ASN1StringToStdString(utc.get())); EXPECT_TRUE(ASN1Time_check_time_t(utc.get(), t.time)); + EXPECT_EQ(PrintStringToBIO(utc.get(), &ASN1_UTCTIME_print), t.printed); + EXPECT_EQ(PrintStringToBIO(utc.get(), &ASN1_TIME_print), t.printed); } else { EXPECT_FALSE(utc); } @@ -977,6 +998,11 @@ EXPECT_EQ(V_ASN1_GENERALIZEDTIME, ASN1_STRING_type(generalized.get())); EXPECT_EQ(t.generalized, ASN1StringToStdString(generalized.get())); EXPECT_TRUE(ASN1Time_check_time_t(generalized.get(), t.time)); + EXPECT_EQ( + PrintStringToBIO(generalized.get(), &ASN1_GENERALIZEDTIME_print), + t.printed); + EXPECT_EQ(PrintStringToBIO(generalized.get(), &ASN1_TIME_print), + t.printed); } else { EXPECT_FALSE(generalized); }
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index ce70ae3..fe4a775 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc
@@ -2129,10 +2129,10 @@ {"000000000000Z", "Bad time value"}, {"999999999999Z", "Bad time value"}, - // Missing components. Not legal RFC 5280, but permitted. - {"090303125425", "Mar 3 12:54:25 2009"}, - {"9003031254", "Mar 3 12:54:00 1990"}, - {"9003031254Z", "Mar 3 12:54:00 1990 GMT"}, + // Missing components. + {"090303125425", "Bad time value"}, + {"9003031254", "Bad time value"}, + {"9003031254Z", "Bad time value"}, // GENERALIZEDTIME confused for UTCTIME. {"20090303125425Z", "Bad time value"},
diff --git a/include/openssl/bytestring.h b/include/openssl/bytestring.h index 846ab24..1d47f60 100644 --- a/include/openssl/bytestring.h +++ b/include/openssl/bytestring.h
@@ -362,16 +362,17 @@ // corresponding time in UTC. This function does not compute |out_tm->tm_wday| // or |out_tm->tm_yday|. OPENSSL_EXPORT int CBS_parse_generalized_time(const CBS *cbs, struct tm *out_tm, - int allow_timezeone_offset); + int allow_timezone_offset); // CBS_parse_utc_time returns one if |cbs| is a valid DER-encoded, ASN.1 // UTCTime body within the limitations imposed by RFC 5280, or zero otherwise. // If |allow_timezone_offset| is non-zero, four-digit timezone offsets, which // would not be allowed by DER, are permitted. On success, if |out_tm| is // non-NULL, |*out_tm| will be zeroed, and then set to the corresponding time -// in UTC. This function does not compute |out_tm->tm_wday| or |out_tm->tm_yday|. +// in UTC. This function does not compute |out_tm->tm_wday| or +// |out_tm->tm_yday|. OPENSSL_EXPORT int CBS_parse_utc_time(const CBS *cbs, struct tm *out_tm, - int allow_timezeone_offset); + int allow_timezone_offset); // CRYPTO ByteBuilder. //