Simplify HelloVerifyRequest processing.

Rather than switching the order of the ServerHello and HelloVerifyRequest
states and processing each twice, have the states follow the protocol order.
HelloVerifyRequest reading is optional and ServerHello is strict. Use the
send_cookie bit to determine whether we're expecting a cookie or not.

Fix the dtls1_stop_timer call in these states to consistently hit the end of a
server flight; the previous flight should not be cleared from the retransmit
buffer until the entire next flight is received. That said, OpenSSL doesn't
appear to implement the part where, on receipt of the previous peer flight, the
buffered flight is retransmitted. (With the exception of a SSL3_MT_FINISHED
special-case in dtls1_read_bytes.) So if the peer is also OpenSSL, this doesn't
do anything.

Also fix the DTLS test which wasn't actually asserting that the ClientHello
matched.

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

include/openssl/dtls1.h

@@ -171,7 +171,10 @@ typedef struct hm_fragment_st
 
 typedef struct dtls1_state_st
 	{
+	/* send_cookie is true if we are resending the ClientHello
+	 * with a cookie from a HelloVerifyRequest. */
 	unsigned int send_cookie;
+
 	uint8_t cookie[DTLS1_COOKIE_LENGTH];
 	size_t cookie_len;
 

ssl/d1_clnt.c

@@ -232,8 +232,6 @@ int dtls1_connect(SSL *s)
 			s->state=SSL3_ST_CW_CLNT_HELLO_A;
 			s->ctx->stats.sess_connect++;
 			s->init_num=0;
-			/* mark client_random uninitialized */
-			memset(s->s3->client_random,0,sizeof(s->s3->client_random));
 			s->d1->send_cookie = 0;
 			s->hit = 0;
 			break;
@@ -256,7 +254,7 @@ int dtls1_connect(SSL *s)
 				s->s3->tmp.next_state=SSL3_ST_CR_SRVR_HELLO_A;
 				}
 			else
-				s->state=SSL3_ST_CR_SRVR_HELLO_A;
+				s->state=DTLS1_ST_CR_HELLO_VERIFY_REQUEST_A;
 
 			s->init_num=0;
 				/* turn on buffering for the next lot of output */
@@ -265,41 +263,46 @@ int dtls1_connect(SSL *s)
 
 			break;
 
-		case SSL3_ST_CR_SRVR_HELLO_A:
-		case SSL3_ST_CR_SRVR_HELLO_B:
-			ret=ssl3_get_server_hello(s);
-			if (ret <= 0) goto end;
-			else
-				{
-				if (s->hit)
-					{
-					s->state=SSL3_ST_CR_FINISHED_A;
-					if (s->tlsext_ticket_expected)
-						{
-						/* receive renewed session ticket */
-						s->state=SSL3_ST_CR_SESSION_TICKET_A;
-						}
-					}
-				else
-					s->state=DTLS1_ST_CR_HELLO_VERIFY_REQUEST_A;
-				}
-			s->init_num=0;
-			break;
-
 		case DTLS1_ST_CR_HELLO_VERIFY_REQUEST_A:
 		case DTLS1_ST_CR_HELLO_VERIFY_REQUEST_B:
 
 			ret = dtls1_get_hello_verify(s);
 			if ( ret <= 0)
 				goto end;
-			dtls1_stop_timer(s);
-			if ( s->d1->send_cookie) /* start again, with a cookie */
-				s->state=SSL3_ST_CW_CLNT_HELLO_A;
+			if ( s->d1->send_cookie)
+				{
+				/* start again, with a cookie */
+				dtls1_stop_timer(s);
+				s->state = SSL3_ST_CW_CLNT_HELLO_A;
+				}
 			else
-				s->state = SSL3_ST_CR_CERT_A;
+				{
+				s->state = SSL3_ST_CR_SRVR_HELLO_A;
+				}
 			s->init_num = 0;
 			break;
 
+		case SSL3_ST_CR_SRVR_HELLO_A:
+		case SSL3_ST_CR_SRVR_HELLO_B:
+			ret=ssl3_get_server_hello(s);
+			if (ret <= 0) goto end;
+
+			if (s->hit)
+				{
+				s->state=SSL3_ST_CR_FINISHED_A;
+				if (s->tlsext_ticket_expected)
+					{
+					/* receive renewed session ticket */
+					s->state=SSL3_ST_CR_SESSION_TICKET_A;
+					}
+				}
+			else
+				{
+				s->state=SSL3_ST_CR_CERT_A;
+				}
+			s->init_num=0;
+			break;
+
 		case SSL3_ST_CR_CERT_A:
 		case SSL3_ST_CR_CERT_B:
 			if (ssl_cipher_has_server_public_key(s->s3->tmp.new_cipher))
@@ -594,7 +597,8 @@ static int dtls1_get_hello_verify(SSL *s)
 		DTLS1_ST_CR_HELLO_VERIFY_REQUEST_A,
 		DTLS1_ST_CR_HELLO_VERIFY_REQUEST_B,
 		-1,
-		s->max_cert_list,
+		/* Use the same maximum size as ssl3_get_server_hello. */
+		20000,
 		&ok);
 	s->first_packet = 0;
 
@@ -603,7 +607,7 @@ static int dtls1_get_hello_verify(SSL *s)
 	if (s->s3->tmp.message_type != DTLS1_MT_HELLO_VERIFY_REQUEST)
 		{
 		s->d1->send_cookie = 0;
-		s->s3->tmp.reuse_message=1;
+		s->s3->tmp.reuse_message = 1;
 		return(1);
 		}
 

ssl/s3_clnt.c

@@ -683,27 +683,14 @@ int ssl3_client_hello(SSL *s)
 
 		p=s->s3->client_random;
 
-		/* for DTLS if client_random is initialized, reuse it, we are
-		 * required to use same upon reply to HelloVerify */
-		if (SSL_IS_DTLS(s))
+		/* If resending the ClientHello in DTLS after a
+		 * HelloVerifyRequest, don't renegerate the client_random. The
+		 * random must be reused. */
+		if (!SSL_IS_DTLS(s) || !s->d1->send_cookie)
 			{
-			size_t idx;
-			i = 1;
-			for (idx=0; idx < sizeof(s->s3->client_random); idx++)
-				{
-				if (p[idx])
-					{
-					i = 0;
-					break;
-					}
-				}
-			}
-		else 
-			i = 1;
-
-		if (i)
 			ssl_fill_hello_random(s, 0, p,
 					      sizeof(s->s3->client_random));
+			}
 
 		/* Do the message type and length last.
 		 * Note: the final argument to ssl_add_clienthello_tlsext below
@@ -829,47 +816,16 @@ int ssl3_get_server_hello(SSL *s)
 	CBS server_hello, server_random, session_id;
 	uint16_t server_version, cipher_suite;
 	uint8_t compression_method;
-	/* Hello verify request and/or server hello version may not
-	 * match so set first packet if we're negotiating version.
-	 */
-	if (SSL_IS_DTLS(s))
-		s->first_packet = 1;
 
 	n=s->method->ssl_get_message(s,
 		SSL3_ST_CR_SRVR_HELLO_A,
 		SSL3_ST_CR_SRVR_HELLO_B,
-		-1,
+		SSL3_MT_SERVER_HELLO,
 		20000, /* ?? */
 		&ok);
 
 	if (!ok) return((int)n);
 
-	if (SSL_IS_DTLS(s))
-		{
-		s->first_packet = 0;
-		if ( s->s3->tmp.message_type == DTLS1_MT_HELLO_VERIFY_REQUEST)
-			{
-			if ( s->d1->send_cookie == 0)
-				{
-				s->s3->tmp.reuse_message = 1;
-				return 1;
-				}
-			else /* already sent a cookie */
-				{
-				al=SSL_AD_UNEXPECTED_MESSAGE;
-				OPENSSL_PUT_ERROR(SSL, ssl3_get_server_hello, SSL_R_BAD_MESSAGE_TYPE);
-				goto f_err;
-				}
-			}
-		}
-	
-	if ( s->s3->tmp.message_type != SSL3_MT_SERVER_HELLO)
-		{
-		al=SSL_AD_UNEXPECTED_MESSAGE;
-		OPENSSL_PUT_ERROR(SSL, ssl3_get_server_hello, SSL_R_BAD_MESSAGE_TYPE);
-		goto f_err;
-		}
-
 	CBS_init(&server_hello, s->init_msg, n);
 
 	if (!CBS_get_u16(&server_hello, &server_version) ||

ssl/test/runner/handshake_server.go

@@ -142,9 +142,17 @@ func (hs *serverHandshakeState) readClientHello() (isResume bool, err error) {
 		if !bytes.Equal(newClientHello.cookie, helloVerifyRequest.cookie) {
 			return false, errors.New("dtls: invalid cookie")
 		}
-		// Apart from the cookie, client hello must match.
-		hs.clientHello.cookie = newClientHello.cookie
-		if hs.clientHello.equal(newClientHello) {
+
+		// Apart from the cookie, the two ClientHellos must
+		// match. Note that clientHello.equal compares the
+		// serialization, so we make a copy.
+		oldClientHelloCopy := *hs.clientHello
+		oldClientHelloCopy.raw = nil
+		oldClientHelloCopy.cookie = nil
+		newClientHelloCopy := *newClientHello
+		newClientHelloCopy.raw = nil
+		newClientHelloCopy.cookie = nil
+		if !oldClientHelloCopy.equal(&newClientHelloCopy) {
 			return false, errors.New("dtls: retransmitted ClientHello does not match")
 		}
 		hs.clientHello = newClientHello