Make X509 time validation stricter.

Copy of OpenSSL change
80770da39e.

This additionally fixes some bugs which causes time validation to
fail when the current time and certificate timestamp are near the
2050 UTCTime/GeneralizedTime cut-off.

Update-Note: Some invalid X.509 timestamps will be newly rejected.

Change-Id: Ie131c61b6840c85bed974101f0a3188e7649059b
Reviewed-on: https://boringssl-review.googlesource.com/29125
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

crypto/CMakeLists.txt

@@ -265,6 +265,7 @@ add_executable(
   test/file_test_gtest.cc
   thread_test.cc
   x509/x509_test.cc
+  x509/x509_time_test.cc
   x509v3/tab_test.cc
   x509v3/v3name_test.cc
 

crypto/x509/x509_time_test.cc

@@ -0,0 +1,240 @@
+/*
+ * Copyright 2017 The OpenSSL Project Authors. All Rights Reserved.
+ *
+ * Licensed under the OpenSSL license (the "License").  You may not use
+ * this file except in compliance with the License.  You can obtain a copy
+ * in the file LICENSE in the source distribution or at
+ * https://www.openssl.org/source/license.html
+ */
+
+// Tests for X509 time functions.
+
+#include <openssl/x509.h>
+
+#include <string.h>
+#include <time.h>
+
+#include <gtest/gtest.h>
+#include <openssl/asn1.h>
+
+struct TestData {
+  const char *data;
+  int type;
+  time_t cmp_time;
+  // -1 if asn1_time <= cmp_time, 1 if asn1_time > cmp_time, 0 if error.
+  int expected;
+};
+
+static TestData kX509CmpTests[] = {
+    {
+        "20170217180154Z",
+        V_ASN1_GENERALIZEDTIME,
+        // The same in seconds since epoch.
+        1487354514,
+        -1,
+    },
+    {
+        "20170217180154Z",
+        V_ASN1_GENERALIZEDTIME,
+        // One second more.
+        1487354515,
+        -1,
+    },
+    {
+        "20170217180154Z",
+        V_ASN1_GENERALIZEDTIME,
+        // One second less.
+        1487354513,
+        1,
+    },
+    // Same as UTC time.
+    {
+        "170217180154Z",
+        V_ASN1_UTCTIME,
+        // The same in seconds since epoch.
+        1487354514,
+        -1,
+    },
+    {
+        "170217180154Z",
+        V_ASN1_UTCTIME,
+        // One second more.
+        1487354515,
+        -1,
+    },
+    {
+        "170217180154Z",
+        V_ASN1_UTCTIME,
+        // One second less.
+        1487354513,
+        1,
+    },
+    // UTCTime from the 20th century.
+    {
+        "990217180154Z",
+        V_ASN1_UTCTIME,
+        // The same in seconds since epoch.
+        919274514,
+        -1,
+    },
+    {
+        "990217180154Z",
+        V_ASN1_UTCTIME,
+        // One second more.
+        919274515,
+        -1,
+    },
+    {
+        "990217180154Z",
+        V_ASN1_UTCTIME,
+        // One second less.
+        919274513,
+        1,
+    },
+    // Various invalid formats.
+    {
+        // No trailing Z.
+        "20170217180154",
+        V_ASN1_GENERALIZEDTIME,
+        0,
+        0,
+    },
+    {
+        // No trailing Z, UTCTime.
+        "170217180154",
+        V_ASN1_UTCTIME,
+        0,
+        0,
+    },
+    {
+        // No seconds.
+        "201702171801Z",
+        V_ASN1_GENERALIZEDTIME,
+        0,
+        0,
+    },
+    {
+        // No seconds, UTCTime.
+        "1702171801Z",
+        V_ASN1_UTCTIME,
+        0,
+        0,
+    },
+    {
+        // Fractional seconds.
+        "20170217180154.001Z",
+        V_ASN1_GENERALIZEDTIME,
+        0,
+        0,
+    },
+    {
+        // Fractional seconds, UTCTime.
+        "170217180154.001Z",
+        V_ASN1_UTCTIME,
+        0,
+        0,
+    },
+    {
+        // Timezone offset.
+        "20170217180154+0100",
+        V_ASN1_GENERALIZEDTIME,
+        0,
+        0,
+    },
+    {
+        // Timezone offset, UTCTime.
+        "170217180154+0100",
+        V_ASN1_UTCTIME,
+        0,
+        0,
+    },
+    {
+        // Extra digits.
+        "2017021718015400Z",
+        V_ASN1_GENERALIZEDTIME,
+        0,
+        0,
+    },
+    {
+        // Extra digits, UTCTime.
+        "17021718015400Z",
+        V_ASN1_UTCTIME,
+        0,
+        0,
+    },
+    {
+        // Non-digits.
+        "2017021718015aZ",
+        V_ASN1_GENERALIZEDTIME,
+        0,
+        0,
+    },
+    {
+        // Non-digits, UTCTime.
+        "17021718015aZ",
+        V_ASN1_UTCTIME,
+        0,
+        0,
+    },
+    {
+        // Trailing garbage.
+        "20170217180154Zlongtrailinggarbage",
+        V_ASN1_GENERALIZEDTIME,
+        0,
+        0,
+    },
+    {
+        // Trailing garbage, UTCTime.
+        "170217180154Zlongtrailinggarbage",
+        V_ASN1_UTCTIME,
+        0,
+        0,
+    },
+    {
+        // Swapped type.
+        "20170217180154Z",
+        V_ASN1_UTCTIME,
+        0,
+        0,
+    },
+    {
+        // Swapped type.
+        "170217180154Z",
+        V_ASN1_GENERALIZEDTIME,
+        0,
+        0,
+    },
+    {
+        // Bad type.
+        "20170217180154Z",
+        V_ASN1_OCTET_STRING,
+        0,
+        0,
+    },
+};
+
+TEST(X509TimeTest, TestCmpTime) {
+  for (auto &test : kX509CmpTests) {
+    SCOPED_TRACE(test.data);
+
+    ASN1_TIME t;
+
+    memset(&t, 0, sizeof(t));
+    t.type = test.type;
+    t.data = (unsigned char*) test.data;
+    t.length = strlen(test.data);
+
+    EXPECT_EQ(test.expected,
+              X509_cmp_time(&t, &test.cmp_time));
+  }
+}
+
+TEST(X509TimeTest, TestCmpTimeCurrent) {
+  time_t now = time(NULL);
+  // Pick a day earlier and later, relative to any system clock.
+  bssl::UniquePtr<ASN1_TIME> asn1_before(ASN1_TIME_adj(NULL, now, -1, 0));
+  bssl::UniquePtr<ASN1_TIME> asn1_after(ASN1_TIME_adj(NULL, now, 1, 0));
+
+  ASSERT_EQ(-1, X509_cmp_time(asn1_before.get(), NULL));
+  ASSERT_EQ(1, X509_cmp_time(asn1_after.get(), NULL));
+}

crypto/x509/x509_vfy.c

@@ -54,6 +54,7 @@
  * copied and put under another distribution licence
  * [including the GNU Public Licence.] */
 
+#include <ctype.h>
 #include <string.h>
 #include <time.h>
 
@@ -1839,122 +1840,67 @@ int X509_cmp_current_time(const ASN1_TIME *ctm)
 
 int X509_cmp_time(const ASN1_TIME *ctm, time_t *cmp_time)
 {
-    char *str;
-    ASN1_TIME atm;
-    long offset;
-    char buff1[24], buff2[24], *p;
-    int i, j, remaining;
+    static const size_t utctime_length = sizeof("YYMMDDHHMMSSZ") - 1;
+    static const size_t generalizedtime_length = sizeof("YYYYMMDDHHMMSSZ") - 1;
+    ASN1_TIME *asn1_cmp_time = NULL;
+    int i, day, sec, ret = 0;
 
-    p = buff1;
-    remaining = ctm->length;
-    str = (char *)ctm->data;
     /*
-     * Note that the following (historical) code allows much more slack in
-     * the time format than RFC5280. In RFC5280, the representation is fixed:
-     * UTCTime: YYMMDDHHMMSSZ GeneralizedTime: YYYYMMDDHHMMSSZ
+     * Note that ASN.1 allows much more slack in the time format than RFC5280.
+     * In RFC5280, the representation is fixed:
+     * UTCTime: YYMMDDHHMMSSZ
+     * GeneralizedTime: YYYYMMDDHHMMSSZ
+     *
+     * We do NOT currently enforce the following RFC 5280 requirement:
+     * "CAs conforming to this profile MUST always encode certificate
+     *  validity dates through the year 2049 as UTCTime; certificate validity
+     *  dates in 2050 or later MUST be encoded as GeneralizedTime."
      */
-    if (ctm->type == V_ASN1_UTCTIME) {
-        /* YYMMDDHHMM[SS]Z or YYMMDDHHMM[SS](+-)hhmm */
-        int min_length = sizeof("YYMMDDHHMMZ") - 1;
-        int max_length = sizeof("YYMMDDHHMMSS+hhmm") - 1;
-        if (remaining < min_length || remaining > max_length)
+    switch (ctm->type) {
+    case V_ASN1_UTCTIME:
+        if (ctm->length != (int)(utctime_length))
             return 0;
-        OPENSSL_memcpy(p, str, 10);
-        p += 10;
-        str += 10;
-        remaining -= 10;
-    } else {
-        /*
-         * YYYYMMDDHHMM[SS[.fff]]Z or YYYYMMDDHHMM[SS[.f[f[f]]]](+-)hhmm
-         */
-        int min_length = sizeof("YYYYMMDDHHMMZ") - 1;
-        int max_length = sizeof("YYYYMMDDHHMMSS.fff+hhmm") - 1;
-        if (remaining < min_length || remaining > max_length)
+        break;
+    case V_ASN1_GENERALIZEDTIME:
+        if (ctm->length != (int)(generalizedtime_length))
             return 0;
-        OPENSSL_memcpy(p, str, 12);
-        p += 12;
-        str += 12;
-        remaining -= 12;
+        break;
+    default:
+        return 0;
     }
 
-    if ((*str == 'Z') || (*str == '-') || (*str == '+')) {
-        *(p++) = '0';
-        *(p++) = '0';
-    } else {
-        /* SS (seconds) */
-        if (remaining < 2)
+    /**
+     * Verify the format: the ASN.1 functions we use below allow a more
+     * flexible format than what's mandated by RFC 5280.
+     * Digit and date ranges will be verified in the conversion methods.
+     */
+    for (i = 0; i < ctm->length - 1; i++) {
+        if (!isdigit(ctm->data[i]))
             return 0;
-        *(p++) = *(str++);
-        *(p++) = *(str++);
-        remaining -= 2;
-        /*
-         * Skip any (up to three) fractional seconds... TODO(emilia): in
-         * RFC5280, fractional seconds are forbidden. Can we just kill them
-         * altogether?
-         */
-        if (remaining && *str == '.') {
-            str++;
-            remaining--;
-            for (i = 0; i < 3 && remaining; i++, str++, remaining--) {
-                if (*str < '0' || *str > '9')
-                    break;
-            }
-        }
-
     }
-    *(p++) = 'Z';
-    *(p++) = '\0';
-
-    /* We now need either a terminating 'Z' or an offset. */
-    if (!remaining)
+    if (ctm->data[ctm->length - 1] != 'Z')
         return 0;
-    if (*str == 'Z') {
-        if (remaining != 1)
-            return 0;
-        offset = 0;
-    } else {
-        /* (+-)HHMM */
-        if ((*str != '+') && (*str != '-'))
-            return 0;
-        /*
-         * Historical behaviour: the (+-)hhmm offset is forbidden in RFC5280.
-         */
-        if (remaining != 5)
-            return 0;
-        if (str[1] < '0' || str[1] > '9' || str[2] < '0' || str[2] > '9' ||
-            str[3] < '0' || str[3] > '9' || str[4] < '0' || str[4] > '9')
-            return 0;
-        offset = ((str[1] - '0') * 10 + (str[2] - '0')) * 60;
-        offset += (str[3] - '0') * 10 + (str[4] - '0');
-        if (*str == '-')
-            offset = -offset;
-    }
-    atm.type = ctm->type;
-    atm.flags = 0;
-    atm.length = sizeof(buff2);
-    atm.data = (unsigned char *)buff2;
 
-    if (X509_time_adj(&atm, offset * 60, cmp_time) == NULL)
-        return 0;
+    /*
+     * There is ASN1_UTCTIME_cmp_time_t but no
+     * ASN1_GENERALIZEDTIME_cmp_time_t or ASN1_TIME_cmp_time_t,
+     * so we go through ASN.1
+     */
+    asn1_cmp_time = X509_time_adj(NULL, 0, cmp_time);
+    if (asn1_cmp_time == NULL)
+        goto err;
+    if (!ASN1_TIME_diff(&day, &sec, ctm, asn1_cmp_time))
+        goto err;
 
-    if (ctm->type == V_ASN1_UTCTIME) {
-        i = (buff1[0] - '0') * 10 + (buff1[1] - '0');
-        if (i < 50)
-            i += 100;           /* cf. RFC 2459 */
-        j = (buff2[0] - '0') * 10 + (buff2[1] - '0');
-        if (j < 50)
-            j += 100;
-
-        if (i < j)
-            return -1;
-        if (i > j)
-            return 1;
-    }
-    i = strcmp(buff1, buff2);
-    if (i == 0)                 /* wait a second then return younger :-) */
-        return -1;
-    else
-        return i;
+    /*
+     * X509_cmp_time comparison is <=.
+     * The return value 0 is reserved for errors.
+     */
+    ret = (day >= 0 && sec >= 0) ? -1 : 1;
+
+ err:
+    ASN1_TIME_free(asn1_cmp_time);
+    return ret;
 }
 
 ASN1_TIME *X509_gmtime_adj(ASN1_TIME *s, long adj)