Port ssl3_get_client_hello to CBS.

Also fix some DTLS cookie bugs. rcvd_cookie is never referenced after being
saved (and the length isn't saved, so it couldn't be used anyway), and the
cookie verification failed to check the length.

For convenience, add a CBS_mem_equal helper function. Saves a bit of
repetition.

Change-Id: I187137733b069f0ac8d8b1bf151eeb80d388b971
Reviewed-on: https://boringssl-review.googlesource.com/1174
Reviewed-by: Adam Langley <agl@google.com>

10 files changed, 104 insertions, 130 deletions

diff --git a/crypto/bytestring/cbs.c b/crypto/bytestring/cbs.c
index b8461de..3478613 100644
--- a/crypto/bytestring/cbs.c
+++ b/crypto/bytestring/cbs.c
@@ -79,6 +79,12 @@ int CBS_contains_zero_byte(const CBS *cbs) {
   return memchr(cbs->data, 0, cbs->len) != NULL;
 }
 
+int CBS_mem_equal(const CBS *cbs, const uint8_t *data, size_t len) {
+  if (len != cbs->len)
+    return 0;
+  return CRYPTO_memcmp(cbs->data, data, len) == 0;
+}
+
 static int cbs_get_u(CBS *cbs, uint32_t *out, size_t len) {
   uint32_t result = 0;
   size_t i;
diff --git a/include/openssl/bytestring.h b/include/openssl/bytestring.h
index 631acdd..544a5d5 100644
--- a/include/openssl/bytestring.h
+++ b/include/openssl/bytestring.h
@@ -73,6 +73,11 @@ int CBS_strdup(const CBS *cbs, char **out_ptr);
  * a NUL byte and zero otherwise. */
 int CBS_contains_zero_byte(const CBS *cbs);
 
+/* CBS_mem_equal compares the current contents of |cbs| with the |len| bytes
+ * starting at |data|. If they're equal, it returns one, otherwise zero. If the
+ * lengths match, it uses a constant-time comparison. */
+int CBS_mem_equal(const CBS *cbs, const uint8_t *data, size_t len);
+
 /* CBS_get_u8 sets |*out| to the next uint8_t from |cbs| and advances |cbs|. It
  * returns one on success and zero on error. */
 int CBS_get_u8(CBS *cbs, uint8_t *out);
diff --git a/include/openssl/dtls1.h b/include/openssl/dtls1.h
index 488e33d..5871bdb 100644
--- a/include/openssl/dtls1.h
+++ b/include/openssl/dtls1.h
@@ -171,7 +171,6 @@ typedef struct dtls1_state_st
 	{
 	unsigned int send_cookie;
 	unsigned char cookie[DTLS1_COOKIE_LENGTH];
-	unsigned char rcvd_cookie[DTLS1_COOKIE_LENGTH];
 	unsigned int cookie_len;
 
 	/* 
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index 94769ce..81849e1 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -984,8 +984,8 @@ int ssl3_get_server_hello(SSL *s)
 		}
 
 	if (CBS_len(&session_id) != 0 &&
-		CBS_len(&session_id) == s->session->session_id_length &&
-		memcmp(CBS_data(&session_id), s->session->session_id, CBS_len(&session_id)) == 0)
+		CBS_mem_equal(&session_id,
+			s->session->session_id, s->session->session_id_length))
 		{
 		if(s->sid_ctx_length != s->session->sid_ctx_length
 			|| memcmp(s->session->sid_ctx, s->sid_ctx, s->sid_ctx_length))
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index 953e22f..3d05522 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -151,6 +151,7 @@
 #define NETSCAPE_HANG_BUG
 
 #include <stdio.h>
+#include <string.h>
 
 #include <openssl/bn.h>
 #include <openssl/buf.h>
@@ -850,15 +851,14 @@ int ssl3_check_client_hello(SSL *s)
 
 int ssl3_get_client_hello(SSL *s)
 	{
-	int i,j,ok,al=SSL_AD_INTERNAL_ERROR,ret= -1;
-	unsigned int cookie_len;
+	int i,ok,al=SSL_AD_INTERNAL_ERROR,ret= -1;
 	long n;
-	unsigned long id;
-	unsigned char *p,*d;
 	SSL_CIPHER *c;
 	STACK_OF(SSL_CIPHER) *ciphers=NULL;
 	struct ssl_early_callback_ctx early_ctx;
-	CBS cbs;
+	CBS client_hello;
+	uint16_t client_version;
+	CBS client_random, session_id, cipher_suites, compression_methods;
 
 	/* We do this so that we will respond with our native type.
 	 * If we are TLSv1 and we get SSLv3, we will respond with TLSv1,
@@ -886,18 +886,21 @@ int ssl3_get_client_hello(SSL *s)
 		 * contain one, just return since we do not want to
 		 * allocate any memory yet. So check cookie length...
 		 */
-		if (SSL_get_options(s) & SSL_OP_COOKIE_EXCHANGE)
+		if (SSL_IS_DTLS(s) && (SSL_get_options(s) & SSL_OP_COOKIE_EXCHANGE))
 			{
-			unsigned int session_length, cookie_length;
-			p = s->init_msg;
+			CBS session_id;
+			uint8_t cookie_length;
+
+			CBS_init(&client_hello, s->init_msg, n);
+			if (!CBS_skip(&client_hello, 2 + SSL3_RANDOM_SIZE) ||
+				!CBS_get_u8_length_prefixed(&client_hello, &session_id) ||
+				!CBS_get_u8(&client_hello, &cookie_length))
+				{
+				al = SSL_AD_DECODE_ERROR;
+				OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_DECODE_ERROR);
+				goto f_err;
+				}
 
-			if (n < 2 + SSL3_RANDOM_SIZE)
-				return 1;
-			session_length = *(p + 2 + SSL3_RANDOM_SIZE);
-			if (n < 2 + SSL3_RANDOM_SIZE + 1 + session_length)
-				return 1;
-			cookie_length =
-				*(p + 2 + SSL3_RANDOM_SIZE + 1 + session_length);
 			if (cookie_length == 0)
 				return 1;
 			}
@@ -945,12 +948,20 @@ int ssl3_get_client_hello(SSL *s)
 		return -1;
 	}
 
-	d = p = s->init_msg;
+	CBS_init(&client_hello, s->init_msg, n);
+	if (!CBS_get_u16(&client_hello, &client_version) ||
+		!CBS_get_bytes(&client_hello, &client_random, SSL3_RANDOM_SIZE) ||
+		!CBS_get_u8_length_prefixed(&client_hello, &session_id) ||
+		CBS_len(&session_id) > SSL_MAX_SSL_SESSION_ID_LENGTH)
+		{
+		al = SSL_AD_DECODE_ERROR;
+		OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_DECODE_ERROR);
+		goto f_err;
+		}
 
 	/* use version from inside client hello, not from record header
 	 * (may differ: see RFC 2246, Appendix E, second paragraph) */
-	s->client_version=(((int)p[0])<<8)|(int)p[1];
-	p+=2;
+	s->client_version = client_version;
 
 	if (SSL_IS_DTLS(s)  ?	(s->client_version > s->version &&
 				 s->method->version != DTLS_ANY_VERSION)
@@ -967,12 +978,8 @@ int ssl3_get_client_hello(SSL *s)
 		goto f_err;
 		}
 
-	/* load the client random */
-	memcpy(s->s3->client_random,p,SSL3_RANDOM_SIZE);
-	p+=SSL3_RANDOM_SIZE;
-
-	/* get the session-id */
-	j= *(p++);
+	/* Load the client random. */
+	memcpy(s->s3->client_random, CBS_data(&client_random), SSL3_RANDOM_SIZE);
 
 	s->hit=0;
 	/* Versions before 0.9.7 always allow clients to resume sessions in renegotiation.
@@ -1012,36 +1019,33 @@ int ssl3_get_client_hello(SSL *s)
 			}
 		}
 
-	p+=j;
-
 	if (SSL_IS_DTLS(s))
 		{
-		/* cookie stuff */
-		cookie_len = *(p++);
+		CBS cookie;
 
-		/* 
-		 * The ClientHello may contain a cookie even if the
-		 * HelloVerify message has not been sent--make sure that it
-		 * does not cause an overflow.
-		 */
-		if ( cookie_len > sizeof(s->d1->rcvd_cookie))
+		/* TODO(davidben): The length check here is off. Per
+		 * spec, the maximum cookie length is 32. However, the
+		 * DTLS1_COOKIE_LENGTH check is checking against 256,
+		 * not 32 (so it's actually redundant).
+		 * 07a9d1a2c2b735cbc327065000b545deb5e136cf from
+		 * OpenSSL switched this from 32 to 256. */
+		if (!CBS_get_u8_length_prefixed(&client_hello, &cookie) ||
+			CBS_len(&cookie) > DTLS1_COOKIE_LENGTH)
 			{
-			/* too much data */
 			al = SSL_AD_DECODE_ERROR;
-			OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_COOKIE_MISMATCH);
+			OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_DECODE_ERROR);
 			goto f_err;
 			}
 
-		/* verify the cookie if appropriate option is set. */
+		/* Verify the cookie if appropriate option is set. */
 		if ((SSL_get_options(s) & SSL_OP_COOKIE_EXCHANGE) &&
-			cookie_len > 0)
+			CBS_len(&cookie) > 0)
 			{
-			memcpy(s->d1->rcvd_cookie, p, cookie_len);
-
-			if ( s->ctx->app_verify_cookie_cb != NULL)
+			if (s->ctx->app_verify_cookie_cb != NULL)
 				{
-				if ( s->ctx->app_verify_cookie_cb(s, s->d1->rcvd_cookie,
-					cookie_len) == 0)
+				if (s->ctx->app_verify_cookie_cb(s,
+						(unsigned char*)CBS_data(&cookie),
+						CBS_len(&cookie)) == 0)
 					{
 					al=SSL_AD_HANDSHAKE_FAILURE;
 					OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_COOKIE_MISMATCH);
@@ -1049,18 +1053,18 @@ int ssl3_get_client_hello(SSL *s)
 					}
 				/* else cookie verification succeeded */
 				}
-			else if ( memcmp(s->d1->rcvd_cookie, s->d1->cookie, 
-						  s->d1->cookie_len) != 0) /* default verification */
+			else if (!CBS_mem_equal(&cookie, s->d1->cookie, s->d1->cookie_len))
 				{
-					al=SSL_AD_HANDSHAKE_FAILURE;
-					OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_COOKIE_MISMATCH);
-					goto f_err;
+				/* default verification */
+				al=SSL_AD_HANDSHAKE_FAILURE;
+				OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_COOKIE_MISMATCH);
+				goto f_err;
 				}
-			/* Set to -2 so if successful we return 2 */
+			/* Set to -2 so if successful we return 2 and
+			 * don't send HelloVerifyRequest. */
 			ret = -2;
 			}
 
-		p += cookie_len;
 		if (s->method->version == DTLS_ANY_VERSION)
 			{
 			/* Select version to use */
@@ -1094,70 +1098,52 @@ int ssl3_get_client_hello(SSL *s)
 			}
 		}
 
-	n2s(p,i);
-	if ((i == 0) && (j != 0))
+	if (!CBS_get_u16_length_prefixed(&client_hello, &cipher_suites) ||
+		!CBS_get_u8_length_prefixed(&client_hello, &compression_methods) ||
+		CBS_len(&compression_methods) == 0)
 		{
-		/* we need a cipher if we are not resuming a session */
-		al=SSL_AD_ILLEGAL_PARAMETER;
-		OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_NO_CIPHERS_SPECIFIED);
+		al = SSL_AD_DECODE_ERROR;
+		OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_DECODE_ERROR);
 		goto f_err;
 		}
-	if ((p+i) >= (d+n))
+
+	/* TODO(davidben): Per spec, cipher_suites can never be empty
+	 * (specified at the ClientHello structure level). This logic
+	 * allows it to be empty if resuming a session. Can we always
+	 * require non-empty? If a client sends empty cipher_suites
+	 * because it's resuming a session, it could always fail to
+	 * resume a session, so it's unlikely to actually work. */
+	if (CBS_len(&cipher_suites) == 0 && CBS_len(&session_id) != 0)
 		{
-		/* not enough data */
-		al=SSL_AD_DECODE_ERROR;
-		OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_LENGTH_MISMATCH);
+		/* We need a cipher if we are not resuming a session. */
+		al = SSL_AD_ILLEGAL_PARAMETER;
+		OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_NO_CIPHERS_SPECIFIED);
 		goto f_err;
 		}
-	if ((i > 0) && (ssl_bytes_to_cipher_list(s,p,i,&(ciphers))
-		== NULL))
+
+	/* TODO(davidben): Port cipher-list handling to CBS. */
+	if (ssl_bytes_to_cipher_list(s, CBS_data(&cipher_suites),
+			CBS_len(&cipher_suites), &ciphers) == NULL)
 		{
 		goto err;
 		}
-	p+=i;
 
 	/* If it is a hit, check that the cipher is in the list */
-	if ((s->hit) && (i > 0))
+	if (s->hit && CBS_len(&cipher_suites) > 0)
 		{
-		j=0;
-		id=s->session->cipher->id;
+		int found_cipher = 0;
+		unsigned long id = s->session->cipher->id;
 
-#ifdef CIPHER_DEBUG
-		printf("client sent %d ciphers\n",sk_num(ciphers));
-#endif
 		for (i=0; i<sk_SSL_CIPHER_num(ciphers); i++)
 			{
 			c=sk_SSL_CIPHER_value(ciphers,i);
-#ifdef CIPHER_DEBUG
-			printf("client [%2d of %2d]:%s\n",
-				i,sk_num(ciphers),SSL_CIPHER_get_name(c));
-#endif
 			if (c->id == id)
 				{
-				j=1;
+				found_cipher = 1;
 				break;
 				}
 			}
-/* Disabled because it can be used in a ciphersuite downgrade
- * attack: CVE-2010-4180.
- */
-#if 0
-		if (j == 0 && (s->options & SSL_OP_NETSCAPE_REUSE_CIPHER_CHANGE_BUG) && (sk_SSL_CIPHER_num(ciphers) == 1))
-			{
-			/* Special case as client bug workaround: the previously used cipher may
-			 * not be in the current list, the client instead might be trying to
-			 * continue using a cipher that before wasn't chosen due to server
-			 * preferences.  We'll have to reject the connection if the cipher is not
-			 * enabled, though. */
-			c = sk_SSL_CIPHER_value(ciphers, 0);
-			if (sk_SSL_CIPHER_find(SSL_get_ciphers(s), c) >= 0)
-				{
-				s->session->cipher = c;
-				j = 1;
-				}
-			}
-#endif
-		if (j == 0)
+		if (!found_cipher)
 			{
 			/* we need to have the cipher in the cipher
 			 * list if we are asked to reuse it */
@@ -1167,34 +1153,19 @@ int ssl3_get_client_hello(SSL *s)
 			}
 		}
 
-	/* compression */
-	i= *(p++);
-	if ((p+i) > (d+n))
+	/* Only null compression is supported. */
+	if (memchr(CBS_data(&compression_methods), 0,
+			CBS_len(&compression_methods)) == NULL)
 		{
-		/* not enough data */
-		al=SSL_AD_DECODE_ERROR;
-		OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_LENGTH_MISMATCH);
-		goto f_err;
-		}
-	for (j=0; j<i; j++)
-		{
-		if (p[j] == 0) break;
-		}
-
-	p+=i;
-	if (j >= i)
-		{
-		/* no compress */
-		al=SSL_AD_DECODE_ERROR;
+		al = SSL_AD_ILLEGAL_PARAMETER;
 		OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_NO_COMPRESSION_SPECIFIED);
 		goto f_err;
 		}
 
-	CBS_init(&cbs, p, d + n - p);
 	/* TLS extensions*/
 	if (s->version >= SSL3_VERSION)
 		{
-		if (!ssl_parse_clienthello_tlsext(s, &cbs))
+		if (!ssl_parse_clienthello_tlsext(s, &client_hello))
 			{
 			OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_PARSE_TLSEXT);
 			goto err;
@@ -1202,7 +1173,7 @@ int ssl3_get_client_hello(SSL *s)
 		}
 
         /* There should be nothing left over in the record. */
-	if (CBS_len(&cbs) != 0)
+	if (CBS_len(&client_hello) != 0)
 		{
 		/* wrong packet length */
 		al=SSL_AD_DECODE_ERROR;
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 7036cf9..68f4947 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -1572,7 +1572,7 @@ int ssl_cipher_list_to_bytes(SSL *s,STACK_OF(SSL_CIPHER) *sk,unsigned char *p,
 	return(p-q);
 	}
 
-STACK_OF(SSL_CIPHER) *ssl_bytes_to_cipher_list(SSL *s,unsigned char *p,int num,
+STACK_OF(SSL_CIPHER) *ssl_bytes_to_cipher_list(SSL *s, const uint8_t *p,int num,
 					       STACK_OF(SSL_CIPHER) **skp)
 	{
 	const SSL_CIPHER *c;
diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index 537981c..408a4ae 100644
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -969,7 +969,7 @@ int ssl_get_new_session(SSL *s, int session);
 int ssl_get_prev_session(SSL *s, const struct ssl_early_callback_ctx *ctx);
 int ssl_cipher_id_cmp(const void *in_a, const void *in_b);
 int ssl_cipher_ptr_id_cmp(const SSL_CIPHER **ap, const SSL_CIPHER **bp);
-STACK_OF(SSL_CIPHER) *ssl_bytes_to_cipher_list(SSL *s,unsigned char *p,int num,
+STACK_OF(SSL_CIPHER) *ssl_bytes_to_cipher_list(SSL *s, const uint8_t *p,int num,
 					       STACK_OF(SSL_CIPHER) **skp);
 int ssl_cipher_list_to_bytes(SSL *s,STACK_OF(SSL_CIPHER) *sk,unsigned char *p,
                              int (*put_cb)(const SSL_CIPHER *, unsigned char *));
diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c
index 35f9eb6..579627c 100644
--- a/ssl/ssl_sess.c
+++ b/ssl/ssl_sess.c
@@ -1089,6 +1089,7 @@ void SSL_CTX_set_cookie_generate_cb(SSL_CTX *ctx,
 	ctx->app_gen_cookie_cb=cb;
 	}
 
+/* TODO(davidben): |cookie| should be a const pointer. */
 void SSL_CTX_set_cookie_verify_cb(SSL_CTX *ctx,
 	int (*cb)(SSL *ssl, unsigned char *cookie, unsigned int cookie_len))
 	{
diff --git a/ssl/t1_reneg.c b/ssl/t1_reneg.c
index 877a7a6..5d7a240 100644
--- a/ssl/t1_reneg.c
+++ b/ssl/t1_reneg.c
@@ -160,14 +160,7 @@ int ssl_parse_clienthello_renegotiate_ext(SSL *s, CBS *cbs, int *out_alert)
 	    }
 
     /* Check that the extension matches */
-    if (CBS_len(&renegotiated_connection) != s->s3->previous_client_finished_len)
-	    {
-	    OPENSSL_PUT_ERROR(SSL, ssl_parse_clienthello_renegotiate_ext, SSL_R_RENEGOTIATION_MISMATCH);
-	    *out_alert = SSL_AD_HANDSHAKE_FAILURE;
-	    return 0;
-	    }
-    
-    if (memcmp(CBS_data(&renegotiated_connection),
+    if (!CBS_mem_equal(&renegotiated_connection,
 		    s->s3->previous_client_finished,
 		    s->s3->previous_client_finished_len))
 	    {
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index 8f4cbf8..f8a84e6 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -55,9 +55,8 @@ static int select_certificate_callback(const struct ssl_early_callback_ctx *ctx)
     return -1;
   }
 
-  if (CBS_len(&host_name) != strlen(expected_server_name) ||
-      memcmp(expected_server_name,
-             CBS_data(&host_name), CBS_len(&host_name)) != 0) {
+  if (!CBS_mem_equal(&host_name, (const uint8_t*)expected_server_name,
+                     strlen(expected_server_name))) {
     fprintf(stderr, "Server name mismatch.\n");
   }