From 04e62715db684d83bffac53793ff4cfac51e047a Mon Sep 17 00:00:00 2001 From: Rich Salz Date: Sun, 11 Jun 2017 16:36:07 -0400 Subject: [PATCH] Introduce ASN1_TIME_set_string_X509 API Make funcs to deal with non-null-term'd string in both asn1_generalizedtime_to_tm() and asn1_utctime_to_tm(). Fixes issue #3444. This one is used to enforce strict format (RFC 5280) check and to convert GeneralizedTime to UTCTime. apps/ca has been changed to use the new API. Test cases and documentation are updated/added Signed-off-by: Paul Yang Reviewed-by: Kurt Roeckx Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/3566) --- apps/apps.c | 4 +- apps/ca.c | 4 +- crypto/asn1/a_gentm.c | 57 ++++++++++++--- crypto/asn1/a_time.c | 60 +++++++++++++++- crypto/asn1/a_utctm.c | 43 ++++++++--- doc/man3/ASN1_TIME_set.pod | 19 +++-- include/openssl/asn1.h | 3 + test/time_offset_test.c | 1 + test/x509_time_test.c | 143 +++++++++++++++++++++++++++++++++++++ util/libcrypto.num | 1 + 10 files changed, 308 insertions(+), 27 deletions(-) diff --git a/apps/apps.c b/apps/apps.c index 1a1b7ec309..15cf4a76f0 100644 --- a/apps/apps.c +++ b/apps/apps.c @@ -2662,14 +2662,14 @@ int set_cert_times(X509 *x, const char *startdate, const char *enddate, if (X509_gmtime_adj(X509_getm_notBefore(x), 0) == NULL) return 0; } else { - if (!ASN1_TIME_set_string(X509_getm_notBefore(x), startdate)) + if (!ASN1_TIME_set_string_X509(X509_getm_notBefore(x), startdate)) return 0; } if (enddate == NULL) { if (X509_time_adj_ex(X509_getm_notAfter(x), days, 0, NULL) == NULL) return 0; - } else if (!ASN1_TIME_set_string(X509_getm_notAfter(x), enddate)) { + } else if (!ASN1_TIME_set_string_X509(X509_getm_notAfter(x), enddate)) { return 0; } return 1; diff --git a/apps/ca.c b/apps/ca.c index 289fd48e17..7e6e1031b0 100644 --- a/apps/ca.c +++ b/apps/ca.c @@ -805,7 +805,7 @@ end_of_options: if (startdate == NULL) ERR_clear_error(); } - if (startdate && !ASN1_TIME_set_string(NULL, startdate)) { + if (startdate && !ASN1_TIME_set_string_X509(NULL, startdate)) { BIO_printf(bio_err, "start date is invalid, it should be YYMMDDHHMMSSZ or YYYYMMDDHHMMSSZ\n"); goto end; @@ -818,7 +818,7 @@ end_of_options: if (enddate == NULL) ERR_clear_error(); } - if (enddate && !ASN1_TIME_set_string(NULL, enddate)) { + if (enddate && !ASN1_TIME_set_string_X509(NULL, enddate)) { BIO_printf(bio_err, "end date is invalid, it should be YYMMDDHHMMSSZ or YYYYMMDDHHMMSSZ\n"); goto end; diff --git a/crypto/asn1/a_gentm.c b/crypto/asn1/a_gentm.c index ff1b695475..bd90d05114 100644 --- a/crypto/asn1/a_gentm.c +++ b/crypto/asn1/a_gentm.c @@ -22,7 +22,7 @@ int asn1_generalizedtime_to_tm(struct tm *tm, const ASN1_GENERALIZEDTIME *d) static const int min[9] = { 0, 0, 1, 1, 0, 0, 0, 0, 0 }; static const int max[9] = { 99, 99, 12, 31, 23, 59, 59, 12, 59 }; char *a; - int n, i, l, o; + int n, i, l, o, min_l = 13, strict = 0; if (d->type != V_ASN1_GENERALIZEDTIME) return (0); @@ -34,10 +34,26 @@ int asn1_generalizedtime_to_tm(struct tm *tm, const ASN1_GENERALIZEDTIME *d) * as YYYY. This stuff treats everything as a two digit field so make * first two fields 00 to 99 */ - if (l < 13) + + /* + * ASN1_STRING_FLAG_X509_TIME is used to enforce RFC 5280 + * time string format, in which: + * + * 1. "seconds" is a 'MUST' + * 2. "Zulu" timezone is a 'MUST' + * 3. "+|-" is not allowed to indicate a time zone + * 4. fractional seconds are not allowed in GeneralizedTime + */ + + if (d->flags & ASN1_STRING_FLAG_X509_TIME) { + min_l = 15; + strict = 1; + } + + if (l < min_l) goto err; for (i = 0; i < 7; i++) { - if ((i == 6) && ((a[o] == 'Z') || (a[o] == '+') || (a[o] == '-'))) { + if (!strict && (i == 6) && ((a[o] == 'Z') || (a[o] == '+') || (a[o] == '-'))) { i++; if (tm) tm->tm_sec = 0; @@ -46,13 +62,15 @@ int asn1_generalizedtime_to_tm(struct tm *tm, const ASN1_GENERALIZEDTIME *d) if ((a[o] < '0') || (a[o] > '9')) goto err; n = a[o] - '0'; - if (++o > l) + /* incomplete 2-digital number */ + if (++o == l) goto err; if ((a[o] < '0') || (a[o] > '9')) goto err; n = (n * 10) + a[o] - '0'; - if (++o > l) + /* no more bytes to read, but we haven't seen time-zone yet */ + if (++o == l) goto err; if ((n < min[i]) || (n > max[i])) @@ -88,22 +106,39 @@ int asn1_generalizedtime_to_tm(struct tm *tm, const ASN1_GENERALIZEDTIME *d) * digits. */ if (a[o] == '.') { - if (++o > l) + if (strict) + /* RFC 5280 forbids fractional seconds */ + goto err; + if (++o == l) goto err; i = o; - while ((a[o] >= '0') && (a[o] <= '9') && (o <= l)) + while ((o < l) && (a[o] >= '0') && (a[o] <= '9')) o++; /* Must have at least one digit after decimal point */ if (i == o) goto err; + /* no more bytes to read, but we haven't seen time-zone yet */ + if (o == l) + goto err; } - if (a[o] == 'Z') + /* + * 'o' will never point to '\0' at this point, the only chance + * 'o' can point th '\0' is either the subsequent if or the first + * else if is true. + */ + if (a[o] == 'Z') { o++; - else if ((a[o] == '+') || (a[o] == '-')) { + } else if (!strict && ((a[o] == '+') || (a[o] == '-'))) { int offsign = a[o] == '-' ? 1 : -1, offset = 0; o++; - if (o + 4 > l) + /* + * if not equal, no need to do subsequent checks + * since the following for-loop will add 'o' by 4 + * and the final return statement will check if 'l' + * and 'o' are equal. + */ + if (o + 4 != l) goto err; for (i = 7; i < 9; i++) { if ((a[o] < '0') || (a[o] > '9')) @@ -146,6 +181,8 @@ int ASN1_GENERALIZEDTIME_set_string(ASN1_GENERALIZEDTIME *s, const char *str) t.type = V_ASN1_GENERALIZEDTIME; t.length = strlen(str); t.data = (unsigned char *)str; + t.flags = 0; + if (ASN1_GENERALIZEDTIME_check(&t)) { if (s != NULL) { if (!ASN1_STRING_set((ASN1_STRING *)s, str, t.length)) diff --git a/crypto/asn1/a_time.c b/crypto/asn1/a_time.c index 27f9bc6808..12b1ff5890 100644 --- a/crypto/asn1/a_time.c +++ b/crypto/asn1/a_time.c @@ -107,7 +107,6 @@ ASN1_GENERALIZEDTIME *ASN1_TIME_to_generalizedtime(const ASN1_TIME *t, return NULL; } - int ASN1_TIME_set_string(ASN1_TIME *s, const char *str) { ASN1_TIME t; @@ -130,6 +129,65 @@ int ASN1_TIME_set_string(ASN1_TIME *s, const char *str) return 1; } +int ASN1_TIME_set_string_X509(ASN1_TIME *s, const char *str) +{ + ASN1_TIME t; + struct tm tm; + int rv = 0; + + t.length = strlen(str); + t.data = (unsigned char *)str; + t.flags = ASN1_STRING_FLAG_X509_TIME; + + t.type = V_ASN1_UTCTIME; + + if (!ASN1_TIME_check(&t)) { + t.type = V_ASN1_GENERALIZEDTIME; + if (!ASN1_TIME_check(&t)) + goto out; + } + + /* + * Per RFC 5280 (section 4.1.2.5.), the valid input time + * strings should be encoded with the following rules: + * + * 1. UTC: YYMMDDHHMMSSZ, if YY < 50 (20YY) --> UTC: YYMMDDHHMMSSZ + * 2. UTC: YYMMDDHHMMSSZ, if YY >= 50 (19YY) --> UTC: YYMMDDHHMMSSZ + * 3. G'd: YYYYMMDDHHMMSSZ, if YYYY >= 2050 --> G'd: YYYYMMDDHHMMSSZ + * 4. G'd: YYYYMMDDHHMMSSZ, if YYYY < 2050 --> UTC: YYMMDDHHMMSSZ + * + * Only strings of the 4th rule should be reformatted, but since a + * UTC can only present [1950, 2050), so if the given time string + * is less than 1950 (e.g. 19230419000000Z), we do nothing... + */ + + if (s != NULL && t.type == V_ASN1_GENERALIZEDTIME) { + if (!asn1_generalizedtime_to_tm(&tm, &t)) + goto out; + if (tm.tm_year >= 50 && tm.tm_year < 150) { + t.length -= 2; + /* + * it's OK to let original t.data go since that's assigned + * to a piece of memory allocated outside of this function. + * new t.data would be freed after ASN1_STRING_copy is done. + */ + t.data = OPENSSL_zalloc(t.length + 1); + if (t.data == NULL) + goto out; + memcpy(t.data, str + 2, t.length); + t.type = V_ASN1_UTCTIME; + } + } + + if (s == NULL || ASN1_STRING_copy((ASN1_STRING *)s, (ASN1_STRING *)&t)) + rv = 1; + + if (t.data != (unsigned char *)str) + OPENSSL_free(t.data); +out: + return rv; +} + int ASN1_TIME_to_tm(const ASN1_TIME *s, struct tm *tm) { if (s == NULL) { diff --git a/crypto/asn1/a_utctm.c b/crypto/asn1/a_utctm.c index 9797aa8a1e..ee98e4b489 100644 --- a/crypto/asn1/a_utctm.c +++ b/crypto/asn1/a_utctm.c @@ -18,7 +18,7 @@ int asn1_utctime_to_tm(struct tm *tm, const ASN1_UTCTIME *d) static const int min[8] = { 0, 1, 1, 0, 0, 0, 0, 0 }; static const int max[8] = { 99, 12, 31, 23, 59, 59, 12, 59 }; char *a; - int n, i, l, o; + int n, i, l, o, min_l = 11, strict = 0; if (d->type != V_ASN1_UTCTIME) return (0); @@ -26,10 +26,24 @@ int asn1_utctime_to_tm(struct tm *tm, const ASN1_UTCTIME *d) a = (char *)d->data; o = 0; - if (l < 11) + /* + * ASN1_STRING_FLAG_X509_TIME is used to enforce RFC 5280 + * time string format, in which: + * + * 1. "seconds" is a 'MUST' + * 2. "Zulu" timezone is a 'MUST' + * 3. "+|-" is not allowed to indicate a time zone + */ + + if (d->flags & ASN1_STRING_FLAG_X509_TIME) { + min_l = 13; + strict = 1; + } + + if (l < min_l) goto err; for (i = 0; i < 6; i++) { - if ((i == 5) && ((a[o] == 'Z') || (a[o] == '+') || (a[o] == '-'))) { + if (!strict && (i == 5) && ((a[o] == 'Z') || (a[o] == '+') || (a[o] == '-'))) { i++; if (tm) tm->tm_sec = 0; @@ -38,13 +52,15 @@ int asn1_utctime_to_tm(struct tm *tm, const ASN1_UTCTIME *d) if ((a[o] < '0') || (a[o] > '9')) goto err; n = a[o] - '0'; - if (++o > l) + /* incomplete 2-digital number */ + if (++o == l) goto err; if ((a[o] < '0') || (a[o] > '9')) goto err; n = (n * 10) + a[o] - '0'; - if (++o > l) + /* no more bytes to read, but we haven't seen time-zone yet */ + if (++o == l) goto err; if ((n < min[i]) || (n > max[i])) @@ -72,12 +88,18 @@ int asn1_utctime_to_tm(struct tm *tm, const ASN1_UTCTIME *d) } } } - if (a[o] == 'Z') + + /* + * 'o' will never point to '\0' at this point, the only chance + * 'o' can point th '\0' is either the subsequent if or the first + * else if is true. + */ + if (a[o] == 'Z') { o++; - else if ((a[o] == '+') || (a[o] == '-')) { + } else if (!strict && ((a[o] == '+') || (a[o] == '-'))) { int offsign = a[o] == '-' ? 1 : -1, offset = 0; o++; - if (o + 4 > l) + if (o + 4 != l) goto err; for (i = 6; i < 8; i++) { if ((a[o] < '0') || (a[o] > '9')) @@ -99,6 +121,9 @@ int asn1_utctime_to_tm(struct tm *tm, const ASN1_UTCTIME *d) } if (offset && !OPENSSL_gmtime_adj(tm, 0, offset * offsign)) return 0; + } else { + /* not Z, or not +/- in non-strict mode */ + return 0; } return o == l; err: @@ -117,6 +142,8 @@ int ASN1_UTCTIME_set_string(ASN1_UTCTIME *s, const char *str) t.type = V_ASN1_UTCTIME; t.length = strlen(str); t.data = (unsigned char *)str; + t.flags = 0; + if (ASN1_UTCTIME_check(&t)) { if (s != NULL) { if (!ASN1_STRING_set((ASN1_STRING *)s, str, t.length)) diff --git a/doc/man3/ASN1_TIME_set.pod b/doc/man3/ASN1_TIME_set.pod index 95bc06dc38..5f041a575c 100644 --- a/doc/man3/ASN1_TIME_set.pod +++ b/doc/man3/ASN1_TIME_set.pod @@ -2,7 +2,8 @@ =head1 NAME -ASN1_TIME_set, ASN1_TIME_adj, ASN1_TIME_check, ASN1_TIME_set_string, +ASN1_TIME_set, ASN1_TIME_adj, ASN1_TIME_check, +ASN1_TIME_set_string, ASN1_TIME_set_string_X509, ASN1_TIME_print, ASN1_TIME_to_tm, ASN1_TIME_diff - ASN.1 Time functions =head1 SYNOPSIS @@ -11,6 +12,7 @@ ASN1_TIME_print, ASN1_TIME_to_tm, ASN1_TIME_diff - ASN.1 Time functions ASN1_TIME *ASN1_TIME_adj(ASN1_TIME *s, time_t t, int offset_day, long offset_sec); int ASN1_TIME_set_string(ASN1_TIME *s, const char *str); + int ASN1_TIME_set_string_X509(ASN1_TIME *s, const char *str); int ASN1_TIME_check(const ASN1_TIME *t); int ASN1_TIME_print(BIO *b, const ASN1_TIME *s); int ASN1_TIME_to_tm(const ASN1_TIME *s, struct tm *tm); @@ -33,7 +35,15 @@ and returned. ASN1_TIME_set_string() sets ASN1_TIME structure B to the time represented by string B which must be in appropriate ASN.1 time -format (for example YYMMDDHHMMSSZ or YYYYMMDDHHMMSSZ). +format (for example YYMMDDHHMMSSZ or YYYYMMDDHHMMSSZ). If B is NULL +this function performs a format check on B only. + +ASN1_TIME_set_string_X509() sets ASN1_TIME structure B to the time +represented by string B which must be in appropriate time format +that RFC 5280 requires, which means it only allows YYMMDDHHMMSSZ and +YYYYMMDDHHMMSSZ (leap second is rejected), all other ASN.1 time format +are not allowed. If B is NULL this function performs a format check +on B only. ASN1_TIME_check() checks the syntax of ASN1_TIME structure B. @@ -122,8 +132,8 @@ Determine if one time is later or sooner than the current time: ASN1_TIME_set() and ASN1_TIME_adj() return a pointer to an ASN1_TIME structure or NULL if an error occurred. -ASN1_TIME_set_string() returns 1 if the time value is successfully set and -0 otherwise. +ASN1_TIME_set_string() and ASN1_TIME_set_string_X509() return 1 if the time +value is successfully set and 0 otherwise. ASN1_TIME_check() returns 1 if the structure is syntactically correct and 0 otherwise. @@ -140,6 +150,7 @@ pass ASN1_TIME structure has invalid syntax for example. =head1 HISTORY The ASN1_TIME_to_tm() function was added in OpenSSL 1.1.1. +The ASN1_TIME_set_string_X509() function was added in OpenSSL 1.1.1. =head1 COPYRIGHT diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h index ea24799690..60409926ad 100644 --- a/include/openssl/asn1.h +++ b/include/openssl/asn1.h @@ -141,6 +141,8 @@ DEFINE_STACK_OF(X509_ALGOR) # define ASN1_STRING_FLAG_MSTRING 0x040 /* String is embedded and only content should be freed */ # define ASN1_STRING_FLAG_EMBED 0x080 +/* String should be parsed in RFC 5280's time format */ +# define ASN1_STRING_FLAG_X509_TIME 0x100 /* This is the base type that holds just about everything :-) */ struct asn1_string_st { int length; @@ -628,6 +630,7 @@ int ASN1_TIME_check(const ASN1_TIME *t); ASN1_GENERALIZEDTIME *ASN1_TIME_to_generalizedtime(const ASN1_TIME *t, ASN1_GENERALIZEDTIME **out); int ASN1_TIME_set_string(ASN1_TIME *s, const char *str); +int ASN1_TIME_set_string_X509(ASN1_TIME *s, const char *str); int ASN1_TIME_to_tm(const ASN1_TIME *s, struct tm *tm); int i2a_ASN1_INTEGER(BIO *bp, const ASN1_INTEGER *a); diff --git a/test/time_offset_test.c b/test/time_offset_test.c index 7b4bec92c5..6660aee5dc 100644 --- a/test/time_offset_test.c +++ b/test/time_offset_test.c @@ -74,6 +74,7 @@ static int test_offset(int idx) at.data = (unsigned char*)testdata->data; at.length = strlen(testdata->data); at.type = testdata->type; + at.flags = 0; if (!TEST_true(ASN1_TIME_diff(&day, &sec, &the_asn1_time, &at))) { TEST_info("ASN1_TIME_diff() failed for %s\n", at.data); diff --git a/test/x509_time_test.c b/test/x509_time_test.c index b73e6554b9..6812805847 100644 --- a/test/x509_time_test.c +++ b/test/x509_time_test.c @@ -25,6 +25,99 @@ typedef struct { int expected; } TESTDATA; +typedef struct { + const char *data; + /* 0 for check-only mode, 1 for set-string mode */ + int set_string; + /* 0 for error, 1 if succeed */ + int expected; + /* + * The following 2 fields are ignored if set_string field is set to '0' + * (in check only mode). + * + * But they can still be ignored explicitly in set-string mode by: + * setting -1 to expected_type and setting NULL to expected_string. + * + * It's useful in a case of set-string mode but the expected result + * is a 'parsing error'. + */ + int expected_type; + const char *expected_string; +} TESTDATA_FORMAT; + +/* + * Actually, the "loose" mode has been tested in + * those time-compare-cases, so we may not test it again. + */ +static TESTDATA_FORMAT x509_format_tests[] = { + /* GeneralizedTime */ + { + /* good format, check only */ + "20170217180105Z", 0, 1, -1, NULL, + }, + { + /* SS is missing, check only */ + "201702171801Z", 0, 0, -1, NULL, + }, + { + /* fractional seconds, check only */ + "20170217180105.001Z", 0, 0, -1, NULL, + }, + { + /* time zone, check only */ + "20170217180105+0800", 0, 0, -1, NULL, + }, + { + /* SS is missing, set string */ + "201702171801Z", 1, 0, -1, NULL, + }, + { + /* fractional seconds, set string */ + "20170217180105.001Z", 1, 0, -1, NULL, + }, + { + /* time zone, set string */ + "20170217180105+0800", 1, 0, -1, NULL, + }, + { + /* good format, check returned 'turned' string */ + "20170217180154Z", 1, 1, V_ASN1_UTCTIME, "170217180154Z", + }, + { + /* good format, check returned string */ + "20510217180154Z", 1, 1, V_ASN1_GENERALIZEDTIME, "20510217180154Z", + }, + { + /* good format but out of UTC range, check returned string */ + "19230419180154Z", 1, 1, V_ASN1_GENERALIZEDTIME, "19230419180154Z", + }, + /* UTC */ + { + /* SS is missing, check only */ + "1702171801Z", 0, 0, -1, NULL, + }, + { + /* time zone, check only */ + "170217180154+0800", 0, 0, -1, NULL, + }, + { + /* SS is missing, set string */ + "1702171801Z", 1, 0, -1, NULL, + }, + { + /* time zone, set string */ + "170217180154+0800", 1, 0, -1, NULL, + }, + { + /* 2017, good format, check returned string */ + "170217180154Z", 1, 1, V_ASN1_UTCTIME, "170217180154Z", + }, + { + /* 1998, good format, check returned string */ + "981223180154Z", 1, 1, V_ASN1_UTCTIME, "981223180154Z", + }, +}; + static TESTDATA x509_cmp_tests[] = { { "20170217180154Z", V_ASN1_GENERALIZEDTIME, @@ -153,6 +246,7 @@ static int test_x509_cmp_time(int idx) t.type = x509_cmp_tests[idx].type; t.data = (unsigned char*)(x509_cmp_tests[idx].data); t.length = strlen(x509_cmp_tests[idx].data); + t.flags = 0; result = X509_cmp_time(&t, &x509_cmp_tests[idx].cmp_time); if (!TEST_int_eq(result, x509_cmp_tests[idx].expected)) { @@ -187,8 +281,57 @@ static int test_x509_cmp_time_current() return failed == 0; } +static int test_x509_time(int idx) +{ + ASN1_TIME *t = NULL; + int result, rv = 0; + + if (x509_format_tests[idx].set_string) { + /* set-string mode */ + t = ASN1_TIME_new(); + if (t == NULL) { + TEST_info("test_x509_time(%d) failed: internal error\n", idx); + return 0; + } + } + + result = ASN1_TIME_set_string_X509(t, x509_format_tests[idx].data); + /* time string parsing result is always checked against what's expected */ + if (!TEST_int_eq(result, x509_format_tests[idx].expected)) { + TEST_info("test_x509_time(%d) failed: expected %d, got %d\n", + idx, x509_format_tests[idx].expected, result); + goto out; + } + + /* if t is not NULL but expected_type is ignored(-1), it is an 'OK' case */ + if (t != NULL && x509_format_tests[idx].expected_type != -1) { + if (!TEST_int_eq(t->type, x509_format_tests[idx].expected_type)) { + TEST_info("test_x509_time(%d) failed: expected_type %d, got %d\n", + idx, x509_format_tests[idx].expected_type, t->type); + goto out; + } + } + + /* if t is not NULL but expected_string is NULL, it is an 'OK' case too */ + if (t != NULL && x509_format_tests[idx].expected_string) { + if (!TEST_str_eq((const char *)t->data, + x509_format_tests[idx].expected_string)) { + TEST_info("test_x509_time(%d) failed: expected_string %s, got %s\n", + idx, x509_format_tests[idx].expected_string, t->data); + goto out; + } + } + + rv = 1; +out: + if (t != NULL) + ASN1_TIME_free(t); + return rv; +} + void register_tests() { ADD_TEST(test_x509_cmp_time_current); ADD_ALL_TESTS(test_x509_cmp_time, OSSL_NELEM(x509_cmp_tests)); + ADD_ALL_TESTS(test_x509_time, OSSL_NELEM(x509_format_tests)); } diff --git a/util/libcrypto.num b/util/libcrypto.num index 37cb2c2f76..13bd9505b0 100644 --- a/util/libcrypto.num +++ b/util/libcrypto.num @@ -4298,3 +4298,4 @@ UI_dup_user_data 4240 1_1_1 EXIST::FUNCTION:UI UI_method_get_data_destructor 4241 1_1_1 EXIST::FUNCTION:UI ERR_load_strings_const 4242 1_1_1 EXIST::FUNCTION: ASN1_TIME_to_tm 4243 1_1_1 EXIST::FUNCTION: +ASN1_TIME_set_string_X509 4244 1_1_1 EXIST::FUNCTION: -- 2.25.1