As reported by David Ramos, most consumer of ssl_get_message() perform late

bounds check, after reading the 2-, 3- or 4-byte size of the next chunk to
process. But the size fields themselves are not checked for being entirely
contained in the buffer.

Since reading past your bounds is bad practice, and may not possible if you
are using a secure memory allocator, we need to add the necessary bounds check,
at the expense of some readability.

As a bonus, a wrong size GOST session key will now trigger an error instead of
a printf to stderr and it being handled as if it had the correct size.

Creating this diff made my eyes bleed (in the real sense); reviewing it
made guenther@'s and beck@'s eyes bleed too (in the literal sense).

ok guenther@ beck@

1 files changed, 65 insertions, 41 deletions

diff --git a/lib/libssl/s3_srvr.c b/lib/libssl/s3_srvr.c
index 66a4552237d..89325b7be90 100644
--- a/lib/libssl/s3_srvr.c
+++ b/lib/libssl/s3_srvr.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: s3_srvr.c,v 1.74 2014/07/11 15:18:52 miod Exp $ */
+/* $OpenBSD: s3_srvr.c,v 1.75 2014/07/11 22:57:25 miod Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -894,18 +894,17 @@ ssl3_get_client_hello(SSL *s)
 		s->state = SSL3_ST_SR_CLNT_HELLO_B;
 	}
 	s->first_packet = 1;
-	n = s->method->ssl_get_message(s,
-	SSL3_ST_SR_CLNT_HELLO_B,
-	SSL3_ST_SR_CLNT_HELLO_C,
-	SSL3_MT_CLIENT_HELLO,
-	SSL3_RT_MAX_PLAIN_LENGTH,
-	&ok);
+	n = s->method->ssl_get_message(s, SSL3_ST_SR_CLNT_HELLO_B,
+	    SSL3_ST_SR_CLNT_HELLO_C, SSL3_MT_CLIENT_HELLO,
+	    SSL3_RT_MAX_PLAIN_LENGTH, &ok);
 
 	if (!ok)
 		return ((int)n);
 	s->first_packet = 0;
-	d = p=(unsigned char *)s->init_msg;
+	d = p = (unsigned char *)s->init_msg;
 
+	if (2 > n)
+		goto truncated;
 	/*
 	 * Use version from inside client hello, not from record header.
 	 * (may differ: see RFC 2246, Appendix E, second paragraph)
@@ -944,12 +943,17 @@ ssl3_get_client_hello(SSL *s)
 			return (1);
 	}
 
+	if (p + SSL3_RANDOM_SIZE + 1 - d > n)
+		goto truncated;
+
 	/* load the client random */
 	memcpy(s->s3->client_random, p, SSL3_RANDOM_SIZE);
 	p += SSL3_RANDOM_SIZE;
 
 	/* get the session-id */
 	j= *(p++);
+	if (p + j - d > n)
+		goto truncated;
 
 	s->hit = 0;
 	/*
@@ -988,6 +992,8 @@ ssl3_get_client_hello(SSL *s)
 
 	if (SSL_IS_DTLS(s)) {
 		/* cookie stuff */
+		if (p + 1 - d > n)
+			goto truncated;
 		cookie_len = *(p++);
 
 		/*
@@ -1003,6 +1009,9 @@ ssl3_get_client_hello(SSL *s)
 			goto f_err;
 		}
 
+		if (p + cookie_len - d > n)
+			goto truncated;
+
 		/* verify the cookie if appropriate option is set. */
 		if ((SSL_get_options(s) & SSL_OP_COOKIE_EXCHANGE) &&
 		    cookie_len > 0) {
@@ -1032,6 +1041,8 @@ ssl3_get_client_hello(SSL *s)
 		p += cookie_len;
 	}
 
+	if (p + 2 - d > n)
+		goto truncated;
 	n2s(p, i);
 	if ((i == 0) && (j != 0)) {
 		/* we need a cipher if we are not resuming a session */
@@ -1040,13 +1051,8 @@ ssl3_get_client_hello(SSL *s)
 		    SSL_R_NO_CIPHERS_SPECIFIED);
 		goto f_err;
 	}
-	if ((p + i) >= (d + n)) {
-		/* not enough data */
-		al = SSL_AD_DECODE_ERROR;
-		SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO,
-		    SSL_R_LENGTH_MISMATCH);
-		goto f_err;
-	}
+	if (p + i - d > n)
+		goto truncated;
 	if ((i > 0) &&
 	    (ssl_bytes_to_cipher_list(s, p, i, &(ciphers)) == NULL)) {
 		goto err;
@@ -1078,14 +1084,11 @@ ssl3_get_client_hello(SSL *s)
 	}
 
 	/* compression */
+	if (p + 1 - d > n)
+		goto truncated;
 	i= *(p++);
-	if ((p + i) > (d + n)) {
-		/* not enough data */
-		al = SSL_AD_DECODE_ERROR;
-		SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO,
-		    SSL_R_LENGTH_MISMATCH);
-		goto f_err;
-	}
+	if (p + i - d > n)
+		goto truncated;
 	for (j = 0; j < i; j++) {
 		if (p[j] == 0)
 			break;
@@ -1247,6 +1250,9 @@ ssl3_get_client_hello(SSL *s)
 	if (ret < 0)
 		ret = 1;
 	if (0) {
+truncated:
+		al = SSL_AD_DECODE_ERROR;
+		SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_BAD_PACKET_LENGTH);
 f_err:
 		ssl3_send_alert(s, SSL3_AL_FATAL, al);
 	}
@@ -1847,7 +1853,7 @@ ssl3_get_client_key_exchange(SSL *s)
 	int i, al, ok;
 	long n;
 	unsigned long alg_k;
-	unsigned char *p;
+	unsigned char *d, *p;
 	RSA *rsa = NULL;
 	EVP_PKEY *pkey = NULL;
 	BIGNUM *pub = NULL;
@@ -1863,7 +1869,7 @@ ssl3_get_client_key_exchange(SSL *s)
 	    SSL3_ST_SR_KEY_EXCH_B, SSL3_MT_CLIENT_KEY_EXCHANGE, 2048, &ok);
 	if (!ok)
 		return ((int)n);
-	p = (unsigned char *)s->init_msg;
+	d = p = (unsigned char *)s->init_msg;
 
 	alg_k = s->s3->tmp.new_cipher->algorithm_mkey;
 
@@ -1897,6 +1903,8 @@ ssl3_get_client_key_exchange(SSL *s)
 
 		/* TLS and [incidentally] DTLS{0xFEFF} */
 		if (s->version > SSL3_VERSION && s->version != DTLS1_BAD_VER) {
+			if (2 > n)
+				goto truncated;
 			n2s(p, i);
 			if (n != i + 2) {
 				if (!(s->options & SSL_OP_TLS_D5_BUG)) {
@@ -1919,6 +1927,8 @@ ssl3_get_client_key_exchange(SSL *s)
 			/* SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE,SSL_R_BAD_RSA_DECRYPT); */
 		}
 
+		if (p + 2 - d > n)	/* needed in the SSL3 case */
+			goto truncated;
 		if ((al == -1) && !((p[0] == (s->client_version >> 8)) &&
 		    (p[1] == (s->client_version & 0xff)))) {
 			/*
@@ -1975,6 +1985,8 @@ ssl3_get_client_key_exchange(SSL *s)
 		OPENSSL_cleanse(p, i);
 	} else
 	if (alg_k & (SSL_kEDH|SSL_kDHr|SSL_kDHd)) {
+		if (2 > n)
+			goto truncated;
 		n2s(p, i);
 		if (n != i + 2) {
 			if (!(s->options & SSL_OP_SSLEAY_080_CLIENT_DH_BUG)) {
@@ -2206,6 +2218,8 @@ ssl3_get_client_key_exchange(SSL *s)
 			    client_pub_pkey) <= 0)
 				ERR_clear_error();
 		}
+		if (2 > n)
+			goto truncated;
 		/* Decrypt session key */
 		if (ASN1_get_object((const unsigned char **)&p, &Tlen, &Ttag,
 		    &Tclass, n) != V_ASN1_CONSTRUCTED ||
@@ -2242,11 +2256,14 @@ gerr:
 	} else {
 		al = SSL_AD_HANDSHAKE_FAILURE;
 		SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE,
-		SSL_R_UNKNOWN_CIPHER_TYPE);
+		    SSL_R_UNKNOWN_CIPHER_TYPE);
 		goto f_err;
 	}
 
 	return (1);
+truncated:
+	al = SSL_AD_DECODE_ERROR;
+	SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, SSL_R_BAD_PACKET_LENGTH);
 f_err:
 	ssl3_send_alert(s, SSL3_AL_FATAL, al);
 err:
@@ -2338,6 +2355,8 @@ ssl3_get_cert_verify(SSL *s)
 				al = SSL_AD_INTERNAL_ERROR;
 				goto f_err;
 			}
+			if (2 > n)
+				goto truncated;
 			/* Check key type is consistent with signature */
 			if (sigalg != (int)p[1]) {
 				SSLerr(SSL_F_SSL3_GET_CERT_VERIFY,
@@ -2355,14 +2374,12 @@ ssl3_get_cert_verify(SSL *s)
 			p += 2;
 			n -= 2;
 		}
+		if (2 > n)
+			goto truncated;
 		n2s(p, i);
 		n -= 2;
-		if (i > n) {
-			SSLerr(SSL_F_SSL3_GET_CERT_VERIFY,
-			    SSL_R_LENGTH_MISMATCH);
-			al = SSL_AD_DECODE_ERROR;
-			goto f_err;
-		}
+		if (i > n)
+			goto truncated;
 	}
 	j = EVP_PKEY_size(pkey);
 	if ((i > j) || (n > j) || (n <= 0)) {
@@ -2445,7 +2462,10 @@ ssl3_get_cert_verify(SSL *s)
 		EVP_PKEY_CTX *pctx = EVP_PKEY_CTX_new(pkey, NULL);
 		EVP_PKEY_verify_init(pctx);
 		if (i != 64) {
-			fprintf(stderr, "GOST signature length is %d", i);
+			SSLerr(SSL_F_SSL3_GET_CERT_VERIFY,
+			    SSL_R_WRONG_SIGNATURE_SIZE);
+			al = SSL_AD_DECODE_ERROR;
+			goto f_err;
 		}
 		for (idx = 0; idx < 64; idx++) {
 			signature[63 - idx] = p[idx];
@@ -2469,6 +2489,9 @@ ssl3_get_cert_verify(SSL *s)
 
 	ret = 1;
 	if (0) {
+truncated:
+		al = SSL_AD_DECODE_ERROR;
+		SSLerr(SSL_F_SSL3_GET_CERT_VERIFY, SSL_R_BAD_PACKET_LENGTH);
 f_err:
 		ssl3_send_alert(s, SSL3_AL_FATAL, al);
 	}
@@ -2490,7 +2513,6 @@ ssl3_get_client_certificate(SSL *s)
 	X509 *x = NULL;
 	unsigned long l, nc, llen, n;
 	const unsigned char *p, *q;
-	unsigned char *d;
 	STACK_OF(X509) *sk = NULL;
 
 	n = s->method->ssl_get_message(s, SSL3_ST_SR_CERT_A, SSL3_ST_SR_CERT_B,
@@ -2528,7 +2550,7 @@ ssl3_get_client_certificate(SSL *s)
 		    SSL_R_WRONG_MESSAGE_TYPE);
 		goto f_err;
 	}
-	p = d = (unsigned char *)s->init_msg;
+	p = (const unsigned char *)s->init_msg;
 
 	if ((sk = sk_X509_new_null()) == NULL) {
 		SSLerr(SSL_F_SSL3_GET_CLIENT_CERTIFICATE,
@@ -2536,16 +2558,14 @@ ssl3_get_client_certificate(SSL *s)
 		goto err;
 	}
 
+	if (3 > n)
+		goto truncated;
 	n2l3(p, llen);
-	if (llen + 3 != n) {
-		al = SSL_AD_DECODE_ERROR;
-		SSLerr(SSL_F_SSL3_GET_CLIENT_CERTIFICATE,
-		    SSL_R_LENGTH_MISMATCH);
-		goto f_err;
-	}
+	if (llen + 3 != n)
+		goto truncated;
 	for (nc = 0; nc < llen;) {
 		n2l3(p, l);
-		if ((l + nc + 3) > llen) {
+		if (l + nc + 3 > llen) {
 			al = SSL_AD_DECODE_ERROR;
 			SSLerr(SSL_F_SSL3_GET_CLIENT_CERTIFICATE,
 			    SSL_R_CERT_LENGTH_MISMATCH);
@@ -2635,6 +2655,10 @@ ssl3_get_client_certificate(SSL *s)
 
 	ret = 1;
 	if (0) {
+truncated:
+		al = SSL_AD_DECODE_ERROR;
+		SSLerr(SSL_F_SSL3_GET_CLIENT_CERTIFICATE,
+		    SSL_R_BAD_PACKET_LENGTH);
 f_err:
 		ssl3_send_alert(s, SSL3_AL_FATAL, al);
 	}