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 <nhorman@openssl.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(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 51278e73dbc04904286e6f285d7ec661f1bd6a27..29a4d002265fd979534c254fcb098d050d342536 100644 (file)
--- a/crypto/asn1/a_time_posix.c
+++ b/crypto/asn1/a_time_posix.c
@@ -21,6 +21,7 @@
 #include <openssl/asn1.h>
 #include <openssl/posix_time.h>
 
+#include <crypto/x509.h>
 #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 649bd32405cc3412b32444a0073c440e2d9f0b11..55105207093b6a5597fd4f369b158f80d3a436d9 100644 (file)
--- 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 2e99fff4d09634216d5ee5857e1154588f004c83..378c170e91d4eccb38915e8ca083ff363f7dd1fe 100644 (file)
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -24,6 +24,7 @@
 #include <openssl/x509v3.h>
 #include <openssl/ocsp.h>
 #include <openssl/objects.h>
+#include <openssl/posix_time.h>
 #include <openssl/core_names.h>
 #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), &notbefore_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), &notafter_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), &notbefore_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), &notafter_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 f03184efb3133d7dcf115d192cc49a53c77374f7..346bfd73ff7148da2ded0aab6ec29c364d9ed7eb 100644 (file)
--- 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 0ed3316fd265e55a9ee1dc9ee6f0a1faf94ac5e7..e334559a7008899fa85fd5bcd1f172bc182ee163 100644 (file)
--- 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 fb47567f58db0728932b23527961382bd82db728..7920966b342eaf2b96c0f43601c28aa721fc22c3 100644 (file)
--- 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 4500fd40a02b4b7855349feacd810fed7b54b238..e36c3a6aeba89c6ced9a8b4ed38051164c964cf9 100644 (file)
--- 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 c09351e21002eb24976b30204ff9cc5d937b4b3b..4f5f821e3d0ecc52972bbfcea7153af0ff1ec24e 100644 (file)
--- 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;
 }