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>

1 files changed, 58 insertions, 19 deletions

diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index 12d71f5..07dc6a9 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -1674,54 +1674,93 @@ 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)) return 0;
+                /* 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) return 0;
+                /* 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')
-		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 == '-')
-			offset= -offset;
-		}
+        /* 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 == '-')
+                    offset= -offset;
+        }
 	atm.type=ctm->type;
 	atm.flags = 0;
 	atm.length=sizeof(buff2);