CVE-2015-1789: Improper Restriction of Operations within the Bounds of a Memory Buffer (CWE-119)
value: The X509_cmp_time function in crypto/x509/x509_vfy.c in OpenSSL before 0.9.8zg, 1.0.0 before 1.0.0s, 1.0.1 before 1.0.1n, and 1.0.2 before 1.0.2b allows remote attackers to cause a denial of service (out-of-bounds read and application crash) via a crafted length field in ASN1_TIME data, as demonstrated by an attack against a server that supports client authentication with a custom verification callback.
CWE description: The product performs operations on a memory buffer, but it reads from or writes to a memory location outside the buffer's intended boundary. This may result in read or write operations on unexpected memory locations that could be linked to other variables, data structures, or internal program data.
CVSS v3 base score: 7.5
Published: 2015-06-12T19:59Z
Repository: https://github.com/openssl/openssl
Language: C
Files changed: crypto/x509/x509_vfy.c
Commit date: 2015-06-11 11:06:30 +0100
Commit message: Fix length checks in X509_cmp_time to avoid out-of-bounds reads.

Also tighten X509_cmp_time to reject more than three fractional
seconds in the time; and to reject trailing garbage after the offset.

CVE-2015-1789

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>

Diff: diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index 7e00436ec0..a920ea70b1 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -1806,51 +1806,88 @@ 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;
+    int i, j, remaining;
 
     p = buff1;
-    i = ctm->length;
+    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
+     */
     if (ctm->type == V_ASN1_UTCTIME) {
-        if ((i < 11) || (i > 17))
+        /* 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)
             return 0;
         memcpy(p, str, 10);
         p += 10;
         str += 10;
+        remaining -= 10;
     } else {
-        if (i < 13)
+        /* 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)
             return 0;
         memcpy(p, str, 12);
         p += 12;
         str += 12;
+        remaining -= 12;
     }
 
     if ((*str == 'Z') || (*str == '-') || (*str == '+')) {
         *(p++) = '0';
         *(p++) = '0';
     } else {
+        /* SS (seconds) */
+        if (remaining < 2)
+            return 0;
         *(p++) = *(str++);
         *(p++) = *(str++);
-        /* Skip any fractional seconds... */
-        if (*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++;
-            while ((*str >= '0') && (*str <= '9'))
-                str++;
+            remaining--;
+            for (i = 0; i < 3 && remaining; i++, str++, remaining--) {
+                if (*str < '0' || *str > '9')
+                    break;
+            }
         }
 
     }
     *(p++) = 'Z';
     *(p++) = '\0';
 
-    if (*str == 'Z')
+    /* We now need either a terminating 'Z' or an offset. */
+    if (!remaining)
+        return 0;
+    if (*str == 'Z') {
+        if (remaining != 1)
+            return 0;
         offset = 0;
-    else {
+    } 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;
     }

Vulnerable code:     int i, j;
    i = ctm->length;
        if ((i < 11) || (i > 17))
        if (i < 13)
        /* Skip any fractional seconds... */
        if (*str == '.') {
            while ((*str >= '0') && (*str <= '9'))
                str++;
    if (*str == 'Z')
    else {
Fixed code:     int i, j, remaining;
    remaining = ctm->length;
    /*
     * 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
     */
        /* 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)
        remaining -= 10;
        /* 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)
        remaining -= 12;
        /* SS (seconds) */
        if (remaining < 2)
            return 0;
        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 == '.') {
            remaining--;
            for (i = 0; i < 3 && remaining; i++, str++, remaining--) {
                if (*str < '0' || *str > '9')
                    break;
            }
    /* We now need either a terminating 'Z' or an offset. */
    if (!remaining)
        return 0;
    if (*str == 'Z') {
        if (remaining != 1)
            return 0;
    } else {
        /* (+-)HHMM */
        /* 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;