From: Bob Beck Date: Wed, 22 Oct 2025 03:34:56 +0000 (-0600) Subject: Simplify x509 time checking X-Git-Tag: 4.0-PRE-CLANG-FORMAT-WEBKIT~195 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=64f4ff7816df396306e83ac14c88ead4c17ef0a9;p=thirdparty%2Fopenssl.git Simplify x509 time checking This changes x509 verification to use int64 values of epoch seconds internally instead of time_t. While time values from a system will still come from/to a platform dependant time_t which could be range constrained, we can simplify this to convert the certificate time to a posix time and then just do a normal comparison of the int64_t values. This removes the need to do further computation to compare values which potentially do not cover the range of certificate times, and makes the internal functions a bit more readable. This also modifies the tests to ensure the full range of times are tested, without depending on time_t, and adds tests for checking CRL expiry, which were lacking before. Reviewed-by: Neil Horman Reviewed-by: Paul Dale Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/28987) --- diff --git a/crypto/asn1/a_time_posix.c b/crypto/asn1/a_time_posix.c index 51278e73dbc..29a4d002265 100644 --- a/crypto/asn1/a_time_posix.c +++ b/crypto/asn1/a_time_posix.c @@ -21,6 +21,7 @@ #include #include +#include #include "asn1_local.h" #define SECS_PER_HOUR (int64_t)(60 * 60) @@ -275,3 +276,16 @@ int OPENSSL_gmtime_diff(int *out_days, int *out_secs, const struct tm *from, return 1; } + +int ossl_posix_to_asn1_time(int64_t posix_time, ASN1_TIME **out_time) +{ + struct tm ts; + + if (!OPENSSL_posix_to_tm(posix_time, &ts)) + return 0; + + if ((*out_time = ossl_asn1_time_from_tm(*out_time, &ts, V_ASN1_UNDEF)) == NULL) + return 0; + + return 1; +} diff --git a/crypto/x509/x509_local.h b/crypto/x509/x509_local.h index 649bd32405c..55105207093 100644 --- a/crypto/x509/x509_local.h +++ b/crypto/x509/x509_local.h @@ -21,7 +21,7 @@ struct X509_VERIFY_PARAM_st { char *name; - time_t check_time; /* Time to use */ + int64_t check_time; /* Time to use */ uint32_t inh_flags; /* Inheritance flags */ unsigned long flags; /* Various verify flags */ int purpose; /* purpose to check untrusted certificates */ diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index 2e99fff4d09..378c170e91d 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -24,6 +24,7 @@ #include #include #include +#include #include #include "internal/dane.h" #include "crypto/x509.h" @@ -959,7 +960,7 @@ static int check_id(X509_STORE_CTX *ctx) /* * Returns 1 is an ASN1 time is valid for an RFC5280 certificate, 0 otherwise */ -static int validate_certifiate_time(const ASN1_TIME *ctm) +static int validate_certificate_time(const ASN1_TIME *ctm) { static const size_t utctime_length = sizeof("YYMMDDHHMMSSZ") - 1; static const size_t generalizedtime_length = sizeof("YYYYMMDDHHMMSSZ") - 1; @@ -1009,25 +1010,20 @@ static int validate_certifiate_time(const ASN1_TIME *ctm) return 1; } -/* - * Compare a certificate time to a time_t. - * returns 0 if either the certificate time or time_t were invalid on not - * representable. Otherwise returns 1 and stores the comparison result - * (-1, 0, or 1) in *out_comparison. - */ -static int x509_cmp_time_internal(const ASN1_TIME *ctm, const time_t *cmp_time, - int *out_comparison) +/* Validate and convert certificate time to a posix time */ +static int certificate_time_to_posix(const ASN1_TIME *ctm, int64_t *out_time) { - time_t t = cmp_time == NULL ? time(NULL) : *cmp_time; - int comparison; + struct tm stm; - if (!validate_certifiate_time(ctm)) + if (!validate_certificate_time(ctm)) return 0; - if ((comparison = ASN1_TIME_cmp_time_t(ctm, t)) == -2) + if (!ASN1_TIME_to_tm(ctm, &stm)) + return 0; + + if (!OPENSSL_tm_to_posix(&stm, out_time)) return 0; - *out_comparison = comparison; return 1; } @@ -1410,48 +1406,55 @@ static int check_cert_crl(X509_STORE_CTX *ctx) return ok; } +/* + * returns 1 and sets verification time if time should be checked. + * returns 0 if time should not be checked. + */ +static int get_verification_time(const X509_VERIFY_PARAM *vpm, + int64_t *verification_time) +{ + if (vpm != NULL && (vpm->flags & X509_V_FLAG_USE_CHECK_TIME) != 0) + *verification_time = vpm->check_time; + else + *verification_time = (int64_t)time(NULL); + + return vpm == NULL || (vpm->flags & X509_V_FLAG_NO_CHECK_TIME) == 0; +} + /* Check CRL times against values in X509_STORE_CTX */ -static int check_crl_time(X509_STORE_CTX *ctx, X509_CRL *crl, int notify) +int ossl_x509_check_crl_time(X509_STORE_CTX *ctx, X509_CRL *crl, int notify) { - time_t *ptime; - int i, comparison; + int64_t verification_time, last_update, next_update; + int err; - if ((ctx->param->flags & X509_V_FLAG_USE_CHECK_TIME) != 0) - ptime = &ctx->param->check_time; - else if ((ctx->param->flags & X509_V_FLAG_NO_CHECK_TIME) != 0) + if (!get_verification_time(ctx->param, &verification_time)) return 1; - else - ptime = NULL; + if (notify) ctx->current_crl = crl; - i = x509_cmp_time_internal(X509_CRL_get0_lastUpdate(crl), ptime, &comparison); - if (i == 0) { - if (!notify) - return 0; - if (!verify_cb_crl(ctx, X509_V_ERR_ERROR_IN_CRL_LAST_UPDATE_FIELD)) - return 0; - } - - if (comparison > 0) { - if (!notify) + if (!certificate_time_to_posix(X509_CRL_get0_lastUpdate(crl), + &last_update)) { + err = X509_V_ERR_ERROR_IN_CRL_LAST_UPDATE_FIELD; + if (!notify || !verify_cb_crl(ctx, err)) return 0; - if (!verify_cb_crl(ctx, X509_V_ERR_CRL_NOT_YET_VALID)) + } else if (verification_time < last_update) { + err = X509_V_ERR_CRL_NOT_YET_VALID; + if (!notify || !verify_cb_crl(ctx, err)) return 0; } if (X509_CRL_get0_nextUpdate(crl)) { - i = x509_cmp_time_internal(X509_CRL_get0_nextUpdate(crl), ptime, &comparison); - - if (i == 0) { - if (!notify) - return 0; - if (!verify_cb_crl(ctx, X509_V_ERR_ERROR_IN_CRL_NEXT_UPDATE_FIELD)) + if (!certificate_time_to_posix(X509_CRL_get0_nextUpdate(crl), + &next_update)) { + err = X509_V_ERR_ERROR_IN_CRL_NEXT_UPDATE_FIELD; + if (!notify || !verify_cb_crl(ctx, err)) return 0; - } - /* Ignore expiration of base CRL is delta is valid */ - if (comparison < 0 && (ctx->current_crl_score & CRL_SCORE_TIME_DELTA) == 0) { - if (!notify || !verify_cb_crl(ctx, X509_V_ERR_CRL_HAS_EXPIRED)) + } else if (verification_time > next_update + /* Ignore expiration of base CRL is delta is valid */ + && (ctx->current_crl_score & CRL_SCORE_TIME_DELTA) == 0) { + err = X509_V_ERR_CRL_HAS_EXPIRED; + if (!notify || !verify_cb_crl(ctx, err)) return 0; } } @@ -1598,7 +1601,7 @@ static void get_delta_sk(X509_STORE_CTX *ctx, X509_CRL **dcrl, int *pscore, *dcrl = delta; - if (check_crl_time(ctx, delta, 0)) + if (ossl_x509_check_crl_time(ctx, delta, 0)) *pscore |= CRL_SCORE_TIME_DELTA; return; @@ -1649,7 +1652,7 @@ static int get_crl_score(X509_STORE_CTX *ctx, X509 **pissuer, crl_score |= CRL_SCORE_NOCRITICAL; /* Check expiration */ - if (check_crl_time(ctx, crl, 0)) + if (ossl_x509_check_crl_time(ctx, crl, 0)) crl_score |= CRL_SCORE_TIME; /* Check authority key ID and locate certificate issuer */ @@ -1991,7 +1994,7 @@ static int check_crl(X509_STORE_CTX *ctx, X509_CRL *crl) } if ((ctx->current_crl_score & CRL_SCORE_TIME) == 0 && - !check_crl_time(ctx, crl, 1)) + !ossl_x509_check_crl_time(ctx, crl, 1)) return 0; /* Attempt to get issuer certificate public key */ @@ -2123,28 +2126,6 @@ static int check_policy(X509_STORE_CTX *ctx) return -1; } -/*- - * Check an ASN1_time against X509 verify parameter time. - * - * Return 1 on success, 0 otherwise. - */ -int ossl_x509_compare_asn1_time(const X509_VERIFY_PARAM *vpm, - const ASN1_TIME *asn1_time, int *comparison) -{ - const time_t now = time(NULL); - const time_t *check_time = NULL; - - if (vpm == NULL) { - check_time = &now; - } else if ((vpm->flags & X509_V_FLAG_USE_CHECK_TIME) != 0) { - check_time = &vpm->check_time; - } else if ((vpm->flags & X509_V_FLAG_NO_CHECK_TIME) != 0) { - *comparison = 0; - return 1; - } - return x509_cmp_time_internal(asn1_time, check_time, comparison); -} - /*- * Check certificate validity times. * @@ -2153,34 +2134,41 @@ int ossl_x509_compare_asn1_time(const X509_VERIFY_PARAM *vpm, int ossl_x509_check_certificate_times(const X509_VERIFY_PARAM *vpm, X509 *x, int *error) { - int err = 0, ret = 0; - int comparison; - const ASN1_TIME *notafter; + int ret = 0, err = 0; + int64_t notafter_seconds, notbefore_seconds, verification_time; + + if (!get_verification_time(vpm, &verification_time)) { + ret = 1; + goto done; + } - if (!ossl_x509_compare_asn1_time(vpm, X509_get0_notBefore(x), &comparison)) { + if (!certificate_time_to_posix(X509_get0_notBefore(x), ¬before_seconds)) { err = X509_V_ERR_ERROR_IN_CERT_NOT_BEFORE_FIELD; goto done; } - if (comparison > 0) { + + if (verification_time < notbefore_seconds) { err = X509_V_ERR_CERT_NOT_YET_VALID; goto done; } + + if (!certificate_time_to_posix(X509_get0_notAfter(x), ¬after_seconds)) { + err = X509_V_ERR_ERROR_IN_CERT_NOT_AFTER_FIELD; + goto done; + } + /* * RFC 5280 4.1.2.5: * To indicate that a certificate has no well-defined expiration date, * the notAfter SHOULD be assigned the GeneralizedTime value of - * 99991231235959Z. + * 99991231235959Z. This is INT64_C(253402300799) in epoch seconds. */ - notafter = X509_get0_notAfter(x); - if (notafter->length == 15 - && memcmp(ASN1_STRING_get0_data(notafter), "99991231235959Z", 15) == 0) - return 1; - - if (!ossl_x509_compare_asn1_time(vpm, notafter, &comparison)) { - err = X509_V_ERR_ERROR_IN_CERT_NOT_AFTER_FIELD; + if (notafter_seconds == INT64_C(253402300799)) { + ret = 1; goto done; } - if (comparison < 0) { + + if (verification_time > notafter_seconds) { err = X509_V_ERR_CERT_HAS_EXPIRED; goto done; } @@ -2205,35 +2193,41 @@ done: int ossl_x509_check_cert_time(X509_STORE_CTX *ctx, X509 *x, int depth) { const X509_VERIFY_PARAM *vpm = ctx->param; - int i, comparison; - const ASN1_TIME *notafter; - - i = ossl_x509_compare_asn1_time(vpm, X509_get0_notBefore(x), &comparison); - if (i == 0 && depth < 0) - return 0; - if (comparison > 0 && depth < 0) - return 0; - CB_FAIL_IF(i == 0, ctx, x, depth, X509_V_ERR_ERROR_IN_CERT_NOT_BEFORE_FIELD); - CB_FAIL_IF(comparison > 0, ctx, x, depth, X509_V_ERR_CERT_NOT_YET_VALID); + int64_t notafter_seconds, notbefore_seconds, verification_time; + int err; - /* - * RFC 5280 4.1.2.5: - * To indicate that a certificate has no well-defined expiration date, - * the notAfter SHOULD be assigned the GeneralizedTime value of - * 99991231235959Z. - */ - notafter = X509_get0_notAfter(x); - if (notafter->length == 15 - && memcmp(ASN1_STRING_get0_data(notafter), "99991231235959Z", 15) == 0) + if (!get_verification_time(vpm, &verification_time)) return 1; - i = ossl_x509_compare_asn1_time(vpm, notafter, &comparison); - if (i == 0 && depth < 0) - return 0; - if (comparison < 0 && depth < 0) - return 0; - CB_FAIL_IF(i == 0, ctx, x, depth, X509_V_ERR_ERROR_IN_CERT_NOT_AFTER_FIELD); - CB_FAIL_IF(comparison < 0, ctx, x, depth, X509_V_ERR_CERT_HAS_EXPIRED); + if (!certificate_time_to_posix(X509_get0_notBefore(x), ¬before_seconds)) { + err = X509_V_ERR_ERROR_IN_CERT_NOT_BEFORE_FIELD; + if (depth < 0 || verify_cb_cert(ctx, x, depth, err) == 0) + return 0; + } else if (verification_time < notbefore_seconds) { + err = X509_V_ERR_CERT_NOT_YET_VALID; + if (depth < 0 || verify_cb_cert(ctx, x, depth, err) == 0) + return 0; + } + + if (!certificate_time_to_posix(X509_get0_notAfter(x), ¬after_seconds)) { + err = X509_V_ERR_ERROR_IN_CERT_NOT_AFTER_FIELD; + if (depth < 0 || verify_cb_cert(ctx, x, depth, err) == 0) + return 0; + } else { + /* + * RFC 5280 4.1.2.5: + * To indicate that a certificate has no well-defined expiration date, + * the notAfter SHOULD be assigned the GeneralizedTime value of + * 99991231235959Z. This is INT64_C(253402300799) in epoch seconds. + */ + if (notafter_seconds == INT64_C(253402300799)) + return 1; + if (verification_time > notafter_seconds) { + err = X509_V_ERR_CERT_HAS_EXPIRED; + if (depth < 0 || verify_cb_cert(ctx, x, depth, err) == 0) + return 0; + } + } return 1; } @@ -2368,16 +2362,17 @@ int X509_cmp_current_time(const ASN1_TIME *ctm) /* returns 0 on error, otherwise 1 if ctm > cmp_time, else -1 */ int X509_cmp_time(const ASN1_TIME *ctm, time_t *cmp_time) { - int comparison; + int64_t cert_time, posix_time = cmp_time == NULL ? (int64_t)time(NULL) + : (int64_t)*cmp_time; - if (!x509_cmp_time_internal(ctm, cmp_time, &comparison)) + if (!certificate_time_to_posix(ctm, &cert_time)) return 0; - /* It's tradition, that makes it OK. Hyrum's law bites forever */ - if (comparison == 0) - comparison = -1; + if (cert_time > posix_time) + return 1; - return comparison; + /* It's tradition, that makes it OK. Hyrum's law bites forever */ + return -1; } /* diff --git a/crypto/x509/x509_vpm.c b/crypto/x509/x509_vpm.c index f03184efb31..346bfd73ff7 100644 --- a/crypto/x509/x509_vpm.c +++ b/crypto/x509/x509_vpm.c @@ -324,10 +324,17 @@ void X509_VERIFY_PARAM_set_auth_level(X509_VERIFY_PARAM *param, int auth_level) time_t X509_VERIFY_PARAM_get_time(const X509_VERIFY_PARAM *param) { - return param->check_time; + /* This will be in the time_t range, because the only setter uses time_t */ + return (time_t)param->check_time; } void X509_VERIFY_PARAM_set_time(X509_VERIFY_PARAM *param, time_t t) +{ + param->check_time = (int64_t)t; + param->flags |= X509_V_FLAG_USE_CHECK_TIME; +} + +void ossl_x509_verify_param_set_time_posix(X509_VERIFY_PARAM *param, int64_t t) { param->check_time = t; param->flags |= X509_V_FLAG_USE_CHECK_TIME; diff --git a/include/crypto/x509.h b/include/crypto/x509.h index 0ed3316fd26..e334559a700 100644 --- a/include/crypto/x509.h +++ b/include/crypto/x509.h @@ -389,7 +389,7 @@ STACK_OF(X509_ATTRIBUTE) *ossl_x509at_add1_attr_by_txt(STACK_OF(X509_ATTRIBUTE) int type, const unsigned char *bytes, int len); - + int ossl_print_attribute_value(BIO *out, int obj_nid, const ASN1_TYPE *av, @@ -403,5 +403,8 @@ int ossl_x509_check_certificate_times(const X509_VERIFY_PARAM *vpm, X509 *x, int *error); /* No error callback if depth < 0 */ int ossl_x509_check_cert_time(X509_STORE_CTX *ctx, X509 *x, int depth); +int ossl_x509_check_crl_time(X509_STORE_CTX *ctx, X509_CRL *crl, int notify); +int ossl_posix_to_asn1_time(int64_t posix_time, ASN1_TIME **out_time); +void ossl_x509_verify_param_set_time_posix(X509_VERIFY_PARAM *param, int64_t t); #endif /* OSSL_CRYPTO_X509_H */ diff --git a/test/crltest.c b/test/crltest.c index fb47567f58d..7920966b342 100644 --- a/test/crltest.c +++ b/test/crltest.c @@ -405,7 +405,14 @@ static int test_basic_crl(void) X509_V_FLAG_CRL_CHECK, PARAM_TIME), X509_V_OK) && TEST_int_eq(verify(test_leaf, test_root, make_CRL_stack(basic_crl, revoked_crl), - X509_V_FLAG_CRL_CHECK, PARAM_TIME), X509_V_ERR_CERT_REVOKED); + X509_V_FLAG_CRL_CHECK, PARAM_TIME), X509_V_ERR_CERT_REVOKED) + && TEST_int_eq(verify(test_leaf, test_root, + make_CRL_stack(basic_crl, revoked_crl), + X509_V_FLAG_CRL_CHECK, PARAM_TIME2), X509_V_ERR_CRL_HAS_EXPIRED) + && TEST_int_eq(verify(test_leaf, test_root, + make_CRL_stack(basic_crl, revoked_crl), + X509_V_FLAG_CRL_CHECK, 0), X509_V_ERR_CRL_NOT_YET_VALID); + if (r) { X509_CRL_get0_signature(basic_crl, NULL, &alg); tbsalg = X509_CRL_get0_tbs_sigalg(basic_crl); diff --git a/test/x509_internal_test.c b/test/x509_internal_test.c index 4500fd40a02..e36c3a6aeba 100644 --- a/test/x509_internal_test.c +++ b/test/x509_internal_test.c @@ -266,43 +266,22 @@ static CERT_TEST_DATA cert_test_data[] = { /* Returns 0 for success, 1 if failed */ static int test_a_time(X509_STORE_CTX *ctx, X509 *x509, + X509_CRL *crl, const int64_t test_time, int64_t notBefore, int64_t notAfter, - int64_t lower_limit, int64_t upper_limit, const char *file, const int line) { - int expected_value, error, expected_error; + int expected_value, expected_crl_value, error, expected_error; X509_VERIFY_PARAM *vpm; - /* Skip tests out of time_t range */ - if (test_time < lower_limit || test_time > upper_limit) - return 0; - - /* - * XXX beck This block below is a hack. The current comparison - * routines needlessly convert the time_t value to a struct - * tm to compare it to the asn1_string converted to a struct tm. - * OPENSSL_gmtime() does this, but fails on large time_t values. - * Once we remove this conversion we should be able to compare - * against the full range of time_t. but for the moment we need - * to skip this test if OPENSSL_gmtime() fails. - */ - { - const time_t t = (const time_t) test_time; - struct tm tm; - - if (OPENSSL_gmtime(&t, &tm) == NULL) { - TEST_info("%s:%d - OPENSSL_gmtime can't handle time of %lld, " - "skipping test.", - file, line, (long long) test_time); - return 0; - } - } - expected_value = notBefore <= test_time; if (expected_value) expected_value = notAfter == MAX_CERT_TIME || notAfter >= test_time; + expected_crl_value = notBefore <= test_time; + if (expected_crl_value) + expected_crl_value = notAfter >= test_time; + if (notBefore > test_time) expected_error = X509_V_ERR_CERT_NOT_YET_VALID; else if (notAfter < test_time && notAfter != MAX_CERT_TIME) @@ -311,7 +290,7 @@ static int test_a_time(X509_STORE_CTX *ctx, X509 *x509, expected_error = 0; vpm = X509_STORE_CTX_get0_param(ctx); - X509_VERIFY_PARAM_set_time(vpm, test_time); + ossl_x509_verify_param_set_time_posix(vpm, test_time); if (ossl_x509_check_cert_time(ctx, x509, 0) != expected_value) { TEST_info("%s:%d - ossl_X509_check_cert_time %s unexpectedly when " "verifying notBefore %lld, notAfter %lld at time %lld\n", @@ -321,6 +300,15 @@ static int test_a_time(X509_STORE_CTX *ctx, X509 *x509, (long long)test_time); return 1; } + if (ossl_x509_check_crl_time(ctx, crl, 0) != expected_crl_value) { + TEST_info("%s:%d - ossl_X509_check_crl_time %s unexpectedly when " + "verifying lastUpdate %lld, nextUpdate %lld at time %lld\n", + file, line, + expected_value ? "failed" : "succeeded", + (long long)notBefore, (long long)notAfter, + (long long)test_time); + return 1; + } error = 0; if (ossl_x509_check_certificate_times(vpm, x509, &error) != expected_value) { TEST_info("%s:%d - ossl_X509_check_certificate_times %s unexpectedly " @@ -344,175 +332,152 @@ static int test_a_time(X509_STORE_CTX *ctx, X509 *x509, return 0; } -static int do_x509_time_tests(CERT_TEST_DATA *tests, size_t ntests, - int64_t lower_limit, int64_t upper_limit) +static int do_x509_time_tests(CERT_TEST_DATA *tests, size_t ntests) { int ret = 0; int failures = 0; X509 *x509 = NULL; + X509_CRL *crl = NULL; X509_STORE_CTX *ctx = NULL; X509_VERIFY_PARAM *vpm = NULL; ASN1_TIME *nb = NULL, *na = NULL; size_t i; - if ((x509 = X509_new()) == NULL) { - TEST_info("Malloc posral se do postele."); + if (!TEST_ptr(x509 = X509_new())) { + TEST_info("Malloc failed"); + goto err; + } + if (!TEST_ptr(crl = X509_CRL_new())) { + TEST_info("Malloc failed"); goto err; } - if ((ctx = X509_STORE_CTX_new()) == NULL) { - TEST_info("Malloc posral se do postele."); + if (!TEST_ptr(ctx = X509_STORE_CTX_new())) { + TEST_info("Malloc failed"); goto err; } X509_STORE_CTX_init(ctx, NULL, NULL, NULL); - if ((vpm = X509_VERIFY_PARAM_new()) == NULL) { - TEST_info("Malloc posral se do postele."); + if (!TEST_ptr(vpm = X509_VERIFY_PARAM_new())) { + TEST_info("Malloc failed"); goto err; } X509_STORE_CTX_set0_param(ctx, vpm); - if ((nb = ASN1_TIME_new()) == NULL) { - TEST_info("Malloc posral se do postele."); + if (!TEST_ptr(nb = ASN1_TIME_new())) { + TEST_info("Malloc failed"); goto err; } - if ((na = ASN1_TIME_new()) == NULL) { - TEST_info("Malloc posral se do postele."); + if (!TEST_ptr(na = ASN1_TIME_new())) { + TEST_info("Malloc failed"); goto err; } for (i = 0; i < ntests; i++) { int64_t test_time; - /* Skip this test if any cert values are out of time_t range */ - if (tests[i].NotBefore < lower_limit || tests[i].NotBefore > upper_limit) - continue; - if (tests[i].NotAfter < lower_limit || tests[i].NotAfter > upper_limit) - continue; - /* - * XXX beck This block below is a hack. The current comparison - * routines needlessly convert the time_t value to a struct - * tm to compare it to the asn1_string converted to a struct tm. - * OPENSSL_gmtime() does this, but fails on large time_t values. - * Once we remove this conversion we should be able to compare - * against the full range of time_t. but for the moment we need - * to skip this test if OPENSSL_gmtime() fails. - */ - { - const time_t t = (const time_t) tests[i].NotBefore; - const time_t t2 = (const time_t) tests[i].NotAfter; - struct tm tm; - - if (OPENSSL_gmtime(&t, &tm) == NULL) { - TEST_info("OPENSSL_gmtime can't handle notBefore time of %lld, skipping test", - (long long) tests[i].NotBefore); - continue; - } - if (OPENSSL_gmtime(&t2, &tm) == NULL) { - TEST_info("OPENSSL_gmtime can't handle notAfter time of %lld, skipping test", - (long long) tests[i].NotAfter); - continue; - } - } - - if (ASN1_TIME_adj(nb, (time_t)tests[i].NotBefore, 0, 0) == NULL) { + if (!TEST_true(ossl_posix_to_asn1_time(tests[i].NotBefore, &nb))) { TEST_info("Could not create NotBefore for time %lld\n", (long long) tests[i].NotBefore); goto err; } - if (ASN1_TIME_adj(na, (time_t)tests[i].NotAfter, 0, 0) == NULL) { + if (!TEST_true(ossl_posix_to_asn1_time(tests[i].NotAfter, &na))) { TEST_info("Could not create NotAfter for time %lld\n", (long long) tests[i].NotBefore); goto err; } /* Forcibly jam the times into the X509 */ - if (!X509_set1_notBefore(x509, nb)) { - TEST_info("X509_set1_notBefore failed"); + if (!TEST_true(X509_set1_notBefore(x509, nb))) goto err; - } - if (!X509_set1_notAfter(x509, na)) { - TEST_info("X509_set1_notBefore failed"); + + if (!TEST_true(X509_set1_notAfter(x509, na))) + TEST_info("X509_set1_notAftere failed"); + + /* Forcibly jam the times into the CRL */ + if (!TEST_true(X509_CRL_set1_lastUpdate(crl, nb))) + goto err; + + if (!TEST_true(X509_CRL_set1_nextUpdate(crl, na))) goto err; - } /* Test boundaries of NotBefore */ test_time = tests[i].NotBefore - 1; - failures += test_a_time(ctx, x509, test_time, tests[i].NotBefore, - tests[i].NotAfter, lower_limit, upper_limit, + failures += test_a_time(ctx, x509, crl, test_time, tests[i].NotBefore, + tests[i].NotAfter, __FILE__, __LINE__); test_time = tests[i].NotBefore; - failures += test_a_time(ctx, x509, test_time, tests[i].NotBefore, - tests[i].NotAfter, lower_limit, upper_limit, + failures += test_a_time(ctx, x509, crl, test_time, tests[i].NotBefore, + tests[i].NotAfter, __FILE__, __LINE__); test_time = tests[i].NotBefore + 1; - failures += test_a_time(ctx, x509, test_time, tests[i].NotBefore, - tests[i].NotAfter, lower_limit, upper_limit, + failures += test_a_time(ctx, x509, crl, test_time, tests[i].NotBefore, + tests[i].NotAfter, __FILE__, __LINE__); /* Test boundaries of NotAfter */ test_time = tests[i].NotAfter - 1; - failures += test_a_time(ctx, x509, test_time, tests[i].NotBefore, - tests[i].NotAfter, lower_limit, upper_limit, + failures += test_a_time(ctx, x509, crl, test_time, tests[i].NotBefore, + tests[i].NotAfter, __FILE__, __LINE__); test_time = tests[i].NotAfter; - failures += test_a_time(ctx, x509, test_time, tests[i].NotBefore, - tests[i].NotAfter, lower_limit, upper_limit, + failures += test_a_time(ctx, x509, crl, test_time, tests[i].NotBefore, + tests[i].NotAfter, __FILE__, __LINE__); test_time = tests[i].NotAfter + 1; - failures += test_a_time(ctx, x509, test_time, tests[i].NotBefore, - tests[i].NotAfter, lower_limit, upper_limit, + failures += test_a_time(ctx, x509, crl, test_time, tests[i].NotBefore, + tests[i].NotAfter, __FILE__, __LINE__); test_time = 1442939232; - failures += test_a_time(ctx, x509, test_time, tests[i].NotBefore, - tests[i].NotAfter, lower_limit, upper_limit, + failures += test_a_time(ctx, x509, crl, test_time, tests[i].NotBefore, + tests[i].NotAfter, __FILE__, __LINE__); test_time = 1443004020; - failures += test_a_time(ctx, x509, test_time, tests[i].NotBefore, - tests[i].NotAfter, lower_limit, upper_limit, + failures += test_a_time(ctx, x509, crl, test_time, tests[i].NotBefore, + tests[i].NotAfter, __FILE__, __LINE__); test_time = MIN_UTC_TIME; - failures += test_a_time(ctx, x509, test_time, tests[i].NotBefore, - tests[i].NotAfter, lower_limit, upper_limit, + failures += test_a_time(ctx, x509, crl, test_time, tests[i].NotBefore, + tests[i].NotAfter, __FILE__, __LINE__); test_time = MIN_UTC_TIME - 1; - failures += test_a_time(ctx, x509, test_time, tests[i].NotBefore, - tests[i].NotAfter, lower_limit, upper_limit, + failures += test_a_time(ctx, x509, crl, test_time, tests[i].NotBefore, + tests[i].NotAfter, __FILE__, __LINE__); test_time = MAX_UTC_TIME; - failures += test_a_time(ctx, x509, test_time, tests[i].NotBefore, - tests[i].NotAfter, lower_limit, upper_limit, + failures += test_a_time(ctx, x509, crl, test_time, tests[i].NotBefore, + tests[i].NotAfter, __FILE__, __LINE__); test_time = MAX_UTC_TIME + 1; - failures += test_a_time(ctx, x509, test_time, tests[i].NotBefore, - tests[i].NotAfter, lower_limit, upper_limit, + failures += test_a_time(ctx, x509, crl, test_time, tests[i].NotBefore, + tests[i].NotAfter, __FILE__, __LINE__); /* Test integer value boundaries */ test_time = INT64_MIN; - failures += test_a_time(ctx, x509, test_time, tests[i].NotBefore, - tests[i].NotAfter, lower_limit, upper_limit, + failures += test_a_time(ctx, x509, crl, test_time, tests[i].NotBefore, + tests[i].NotAfter, __FILE__, __LINE__); test_time = INT32_MIN; - failures += test_a_time(ctx, x509, test_time, tests[i].NotBefore, - tests[i].NotAfter, lower_limit, upper_limit, + failures += test_a_time(ctx, x509, crl, test_time, tests[i].NotBefore, + tests[i].NotAfter, __FILE__, __LINE__); test_time = -1; - failures += test_a_time(ctx, x509, test_time, tests[i].NotBefore, - tests[i].NotAfter, lower_limit, upper_limit, + failures += test_a_time(ctx, x509, crl, test_time, tests[i].NotBefore, + tests[i].NotAfter, __FILE__, __LINE__); test_time = 0; - failures += test_a_time(ctx, x509, test_time, tests[i].NotBefore, - tests[i].NotAfter, lower_limit, upper_limit, + failures += test_a_time(ctx, x509, crl, test_time, tests[i].NotBefore, + tests[i].NotAfter, __FILE__, __LINE__); test_time = 1; - failures += test_a_time(ctx, x509, test_time, tests[i].NotBefore, - tests[i].NotAfter, lower_limit, upper_limit, + failures += test_a_time(ctx, x509, crl, test_time, tests[i].NotBefore, + tests[i].NotAfter, __FILE__, __LINE__); test_time = INT32_MAX; - failures += test_a_time(ctx, x509, test_time, tests[i].NotBefore, - tests[i].NotAfter, lower_limit, upper_limit, + failures += test_a_time(ctx, x509, crl, test_time, tests[i].NotBefore, + tests[i].NotAfter, __FILE__, __LINE__); test_time = UINT32_MAX; - failures += test_a_time(ctx, x509, test_time, tests[i].NotBefore, - tests[i].NotAfter, lower_limit, upper_limit, + failures += test_a_time(ctx, x509, crl, test_time, tests[i].NotBefore, + tests[i].NotAfter, __FILE__, __LINE__); test_time = INT64_MAX; - failures += test_a_time(ctx, x509, test_time, tests[i].NotBefore, - tests[i].NotAfter, lower_limit, upper_limit, + failures += test_a_time(ctx, x509, crl, test_time, tests[i].NotBefore, + tests[i].NotAfter, __FILE__, __LINE__); } @@ -521,6 +486,7 @@ static int do_x509_time_tests(CERT_TEST_DATA *tests, size_t ntests, err: X509_STORE_CTX_free(ctx); X509_free(x509); + X509_CRL_free(crl); ASN1_STRING_free(nb); ASN1_STRING_free(na); return ret; @@ -528,32 +494,8 @@ err: static int tests_X509_check_time(void) { - /* - * time_t sanity checks. We need these until we can evaluate cert - * time without depending on platform time_t. Then all this - * unpleasantness to decide to not run unit tests that exceed the - * range of a platform time_t can go away. - */ - time_t test_time_t = -1; - int time_t_is_unsigned = (test_time_t > 0); - int time_t_is_64_bit = (sizeof(time_t) == sizeof(int64_t)); - int time_t_is_32_bit = (sizeof(time_t) == sizeof(int32_t)); - - OPENSSL_assert(time_t_is_32_bit || time_t_is_64_bit); - OPENSSL_assert(!time_t_is_unsigned || time_t_is_32_bit); - - if (time_t_is_32_bit) { - if (time_t_is_unsigned) { - return do_x509_time_tests(cert_test_data, sizeof(cert_test_data) - / sizeof(CERT_TEST_DATA), INT32_MIN, - INT32_MAX); - } else { - return do_x509_time_tests(cert_test_data, sizeof(cert_test_data) - / sizeof(CERT_TEST_DATA), 0, UINT32_MAX); - } - } return do_x509_time_tests(cert_test_data, sizeof(cert_test_data) - / sizeof(CERT_TEST_DATA), INT64_MIN, INT64_MAX); + / sizeof(CERT_TEST_DATA)); } int setup_tests(void) diff --git a/test/x509_time_test.c b/test/x509_time_test.c index c09351e2100..4f5f821e3d0 100644 --- a/test/x509_time_test.c +++ b/test/x509_time_test.c @@ -277,22 +277,29 @@ static int test_x509_cmp_time_current(void) { time_t now = time(NULL); /* Pick a day earlier and later, relative to any system clock. */ - ASN1_TIME *asn1_before = NULL, *asn1_after = NULL; + ASN1_TIME *asn1_before = NULL, *asn1_after = NULL, *asn1_now = NULL; int cmp_result, failed = 0; asn1_before = ASN1_TIME_adj(NULL, now, -1, 0); asn1_after = ASN1_TIME_adj(NULL, now, 1, 0); + asn1_now = ASN1_TIME_adj(NULL, now, 0, 0); - cmp_result = X509_cmp_time(asn1_before, NULL); + /* X509_cmp_time is expected to return -1 for equal */ + cmp_result = X509_cmp_time(asn1_now, &now); if (!TEST_int_eq(cmp_result, -1)) failed = 1; - cmp_result = X509_cmp_time(asn1_after, NULL); + cmp_result = X509_cmp_time(asn1_before, &now); + if (!TEST_int_eq(cmp_result, -1)) + failed = 1; + + cmp_result = X509_cmp_time(asn1_after, &now); if (!TEST_int_eq(cmp_result, 1)) failed = 1; ASN1_TIME_free(asn1_before); ASN1_TIME_free(asn1_after); + ASN1_TIME_free(asn1_now); return failed == 0; }