Replace s->first_packet with a s->s3->have_version bit.

first_packet is a temporary connection-global flag set for the duration of some
call and then queried from other code. This kind of logic is too difficult to
reason through. It also incorrectly treats renegotiate ClientHellos as
pre-version-negotiation records. This eliminates the need to query
enc_write_ctx (which wasn't EVP_AEAD-aware anyway).

Instead, take a leaf from Go TLS's book and add a have_version bit. This is
placed on s->s3 as it is connection state; s->s3 automatically gets reset on
SSL_clear while s doesn't.

This new flag will also be used to determine whether to do the V2ClientHello
sniff when the version-locked methods merge into SSLv23_method. It will also
replace needing to condition s->method against a dummy DTLS_ANY_VERSION value
to determine whether DTLS version negotiation has happened yet.

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

include/openssl/ssl.h

@@ -1300,7 +1300,6 @@ struct ssl_st
 	unsigned long options; /* protocol behaviour */
 	unsigned long mode; /* API behaviour */
 	long max_cert_list;
-	int first_packet;
 	int client_version;	/* what was passed, used for
 				 * SSLv3/TLS rollback check */
 	unsigned int max_send_fragment;

include/openssl/ssl3.h

@@ -365,6 +365,10 @@ typedef struct ssl3_state_st
 	/* The value of 'extra' when the buffers were initialized */
 	int init_extra;
 
+	/* have_version is true if the connection's final version is
+	 * known. Otherwise the version has not been negotiated yet. */
+	char have_version;
+
 	SSL3_BUFFER rbuf;	/* read IO goes into here */
 	SSL3_BUFFER wbuf;	/* write IO goes into here */
 

ssl/d1_clnt.c

@@ -552,7 +552,6 @@ static int dtls1_get_hello_verify(SSL *s)
 	CBS hello_verify_request, cookie;
 	uint16_t server_version;
 
-	s->first_packet = 1;
 	n=s->method->ssl_get_message(s,
 		DTLS1_ST_CR_HELLO_VERIFY_REQUEST_A,
 		DTLS1_ST_CR_HELLO_VERIFY_REQUEST_B,
@@ -561,7 +560,6 @@ static int dtls1_get_hello_verify(SSL *s)
 		20000,
 		SSL_GET_MESSAGE_HASH_MESSAGE,
 		&ok);
-	s->first_packet = 0;
 
 	if (!ok) return((int)n);
 

ssl/d1_pkt.c

@@ -583,7 +583,7 @@ again:
 		n2s(p,rr->length);
 
 		/* Lets check version */
-		if (!s->first_packet)
+		if (s->s3->have_version)
 			{
 			if (version != s->version)
 				{

ssl/s3_clnt.c

@@ -777,11 +777,6 @@ int ssl3_get_server_hello(SSL *s)
 	uint8_t compression_method;
 	unsigned long mask_ssl;
 
-	/* DTLS_ANY_VERSION does not sniff the version ahead of time,
-	 * so disable the version check. */
-	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,
@@ -790,9 +785,6 @@ int ssl3_get_server_hello(SSL *s)
 		SSL_GET_MESSAGE_HASH_MESSAGE,
 		&ok);
 
-	if (SSL_IS_DTLS(s))
-		s->first_packet = 0;
-
 	if (!ok) return((int)n);
 
 	CBS_init(&server_hello, s->init_msg, n);
@@ -837,6 +829,12 @@ int ssl3_get_server_hello(SSL *s)
 		goto f_err;
 		}
 
+	/* At this point, the connection's version is known and s->version is
+	 * fixed. Begin enforcing the record-layer version. Note: SSLv23_method
+	 * currently determines its version sooner, but it will later be moved
+	 * to this point. */
+	s->s3->have_version = 1;
+
 	/* Copy over the server random. */
 	memcpy(s->s3->server_random, CBS_data(&server_random), SSL3_RANDOM_SIZE);
 

ssl/s3_pkt.c

@@ -341,12 +341,12 @@ fprintf(stderr, "Record type=%d, Length=%d\n", rr->type, rr->length);
 #endif
 
 		/* Lets check version */
-		if (!s->first_packet)
+		if (s->s3->have_version)
 			{
 			if (version != s->version)
 				{
 				OPENSSL_PUT_ERROR(SSL, ssl3_get_record, SSL_R_WRONG_VERSION_NUMBER);
-                                if ((s->version & 0xFF00) == (version & 0xFF00) && !s->enc_write_ctx && !s->write_hash)
+                                if ((s->version & 0xFF00) == (version & 0xFF00))
                                 	/* Send back error using their minor version number :-) */
 					s->version = (unsigned short)version;
 				al=SSL_AD_PROTOCOL_VERSION;

ssl/s3_srvr.c

@@ -715,7 +715,6 @@ int ssl3_get_client_hello(SSL *s)
 	switch (s->state) {
 	case SSL3_ST_SR_CLNT_HELLO_A:
 	case SSL3_ST_SR_CLNT_HELLO_B:
-		s->first_packet=1;
 		n=s->method->ssl_get_message(s,
 			SSL3_ST_SR_CLNT_HELLO_A,
 			SSL3_ST_SR_CLNT_HELLO_B,
@@ -725,7 +724,6 @@ int ssl3_get_client_hello(SSL *s)
 			&ok);
 
 		if (!ok) return((int)n);
-		s->first_packet=0;
 
 		/* If we require cookies and this ClientHello doesn't
 		 * contain one, just return since we do not want to
@@ -814,7 +812,7 @@ int ssl3_get_client_hello(SSL *s)
 		{
 		OPENSSL_PUT_ERROR(SSL, ssl3_get_client_hello, SSL_R_WRONG_VERSION_NUMBER);
 		if ((s->client_version>>8) == SSL3_VERSION_MAJOR &&
-			!s->enc_write_ctx && !s->write_hash)
+			!s->s3->have_version)
 			{
 			/* similar to ssl3_get_record, send alert using remote version number */
 			s->version = s->client_version;
@@ -890,6 +888,12 @@ int ssl3_get_client_hello(SSL *s)
 			}
 		}
 
+	/* At this point, the connection's version is known and s->version is
+	 * fixed. Begin enforcing the record-layer version. Note: SSLv23_method
+	 * currently determines its version sooner, but it will later be moved
+	 * to this point. */
+	s->s3->have_version = 1;
+
 	s->hit=0;
 	/* Versions before 0.9.7 always allow clients to resume sessions in renegotiation.
 	 * 0.9.7 and later allow this by default, but optionally ignore resumption requests

ssl/ssl_lib.c

@@ -243,8 +243,6 @@ int SSL_clear(SSL *s)
 	ssl_clear_hash_ctx(&s->read_hash);
 	ssl_clear_hash_ctx(&s->write_hash);
 
-	s->first_packet=0;
-
 	s->method->ssl_clear(s);
 	return(1);
 	}