Vulnerability: 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.
Vulnerability ID: CVE-2015-1789
CWE: CWE-119
Severity score: 2.9
Project: https://github.com/openssl/openssl
Published: 2015-06-12
File: crypto/x509/x509_vfy.c
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>
Commit author: Emilia Kasper

Vulnerable version (before fix):
 int X509_cmp_time(const ASN1_TIME *ctm, time_t *cmp_time)     ASN1_TIME atm;     long offset;     char buff1[24], buff2[24], *p;     int i, j;      p = buff1;     i = ctm->length;     str = (char *)ctm->data;     if (ctm->type == V_ASN1_UTCTIME) {         if ((i < 11) || (i > 17))             return 0;         memcpy(p, str, 10);         p += 10;         str += 10;     } else {         if (i < 13)             return 0;         memcpy(p, str, 12);         p += 12;         str += 12;     }      if ((*str == 'Z') || (*str == '-') || (*str == '+')) {         *(p++) = '0';         *(p++) = '0';     } else {         *(p++) = *(str++);         *(p++) = *(str++);         /* Skip any fractional seconds... */         if (*str == '.') {             str++;             while ((*str >= '0') && (*str <= '9'))                 str++;         }      }     *(p++) = 'Z';     *(p++) = '\0';      if (*str == 'Z')         offset = 0;     else {         if ((*str != '+') && (*str != '-'))             return 0;         offset = ((str[1] - '0') * 10 + (str[2] - '0')) * 60;         offset += (str[3] - '0') * 10 + (str[4] - '0');         if (*str == '-')

Fixed version (after fix):
 int X509_cmp_time(const ASN1_TIME *ctm, time_t *cmp_time)     ASN1_TIME atm;     long offset;     char buff1[24], buff2[24], *p;     int i, j, remaining;      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      */     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)             return 0;         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)             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++);         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)         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 == '-')

Patch:
patch: @@ -1808,47 +1808,84 @@ int X509_cmp_time(const ASN1_TIME *ctm, time_t *cmp_time)
     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 == '-')
path: crypto/x509/x509_vfy.c