Fix for CVE-2015-1789.

X509_cmp_time does not properly check the length of the ASN1_TIME string
and can read a few bytes out of bounds. In addition, X509_cmp_time
accepts an arbitrary number of fractional seconds in the time string.

An attacker can use this to craft malformed certificates and CRLs of
various sizes and potentially cause a segmentation fault, resulting in a
DoS on applications that verify certificates or CRLs. TLS clients that
verify CRLs are affected. TLS clients and servers with client
authentication enabled may be affected if they use custom verification
callbacks.

This change cherry-picks the following changes from BoringSSL:

d87021d2 – Fix length checks in X509_cmp_time to avoid out-of-bounds reads.

Change-Id: Ia7d0c5d889f61a3c4be6ea79a5ab41f67bc3c65c

1 files changed, 47 insertions, 7 deletions

diff --git a/src/crypto/x509/x509_vfy.c b/src/crypto/x509/x509_vfy.c
index 2ba9c84..f53f279 100644
--- a/src/crypto/x509/x509_vfy.c
+++ b/src/crypto/x509/x509_vfy.c
@@ -1829,49 +1829,89 @@ 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';
 
+	/* 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 == '-')