Ensure SSL3_FLAGS_CCS_OK (or d1->change_cipher_spec_ok for DTLS) is reset

once the ChangeCipherSpec message is received. Previously, the server would
set the flag once at SSL3_ST_SR_CERT_VRFY and again at SSL3_ST_SR_FINISHED.
This would allow a second CCS to arrive and would corrupt the server state.

(Because the first CCS would latch the correct keys and subsequent CCS
messages would have to be encrypted, a MitM attacker cannot exploit this,
though.)

Thanks to Joeri de Ruiter for reporting this issue.

Reviewed-by: Matt Caswell <matt@openssl.org>
(cherry picked from commit e94a6c0ede623960728415b68650a595e48f5a43)

8 files changed, 90 insertions, 20 deletions

diff --git a/CHANGES b/CHANGES
index b59c95cfcf..a26cb3993a 100644
--- a/CHANGES
+++ b/CHANGES
@@ -43,6 +43,11 @@
      (CVE-2014-3566)
      [Adam Langley, Bodo Moeller]
 
+   *) Tighten handling of the ChangeCipherSpec (CCS) message: reject
+      early CCS messages during renegotiation. (Note that because
+      renegotiation is encrypted, this early CCS was not exploitable.)
+      [Emilia Käsper]
+
    *) Tighten client-side session ticket handling during renegotiation:
       ensure that the client only accepts a session ticket if the server sends
       the extension anew in the ServerHello. Previously, a TLS client would
@@ -376,6 +381,11 @@
 
  Changes between 1.0.1j and 1.0.1k [xx XXX xxxx]
 
+   *) Tighten handling of the ChangeCipherSpec (CCS) message: reject
+      early CCS messages during renegotiation. (Note that because
+      renegotiation is encrypted, this early CCS was not exploitable.)
+      [Emilia Käsper]
+
    *) Tighten client-side session ticket handling during renegotiation:
       ensure that the client only accepts a session ticket if the server sends
       the extension anew in the ServerHello. Previously, a TLS client would
diff --git a/ssl/d1_clnt.c b/ssl/d1_clnt.c
index 171d144586..ea36ea448a 100644
--- a/ssl/d1_clnt.c
+++ b/ssl/d1_clnt.c
@@ -267,6 +267,9 @@ int dtls1_connect(SSL *s)
 			memset(s->s3->client_random,0,sizeof(s->s3->client_random));
 			s->d1->send_cookie = 0;
 			s->hit = 0;
+			s->d1->change_cipher_spec_ok = 0;
+			/* Should have been reset by ssl3_get_finished, too. */
+			s->s3->change_cipher_spec = 0;
 			break;
 
 #ifndef OPENSSL_NO_SCTP
@@ -510,7 +513,6 @@ int dtls1_connect(SSL *s)
 				else
 #endif
 					s->state=SSL3_ST_CW_CHANGE_A;
-				s->s3->change_cipher_spec=0;
 				}
 
 			s->init_num=0;
@@ -531,7 +533,6 @@ int dtls1_connect(SSL *s)
 #endif
 				s->state=SSL3_ST_CW_CHANGE_A;
 			s->init_num=0;
-			s->s3->change_cipher_spec=0;
 			break;
 
 		case SSL3_ST_CW_CHANGE_A:
diff --git a/ssl/d1_srvr.c b/ssl/d1_srvr.c
index a6e7d053ca..084118375f 100644
--- a/ssl/d1_srvr.c
+++ b/ssl/d1_srvr.c
@@ -264,6 +264,9 @@ int dtls1_accept(SSL *s)
 				}
 
 			s->init_num=0;
+			s->d1->change_cipher_spec_ok = 0;
+			/* Should have been reset by ssl3_get_finished, too. */
+			s->s3->change_cipher_spec = 0;
 
 			if (s->state != SSL_ST_RENEGOTIATE)
 				{
@@ -694,8 +697,14 @@ int dtls1_accept(SSL *s)
 
 		case SSL3_ST_SR_CERT_VRFY_A:
 		case SSL3_ST_SR_CERT_VRFY_B:
-
-			s->d1->change_cipher_spec_ok = 1;
+			/*
+			 * This *should* be the first time we enable CCS, but be
+			 * extra careful about surrounding code changes. We need
+			 * to set this here because we don't know if we're
+			 * expecting a CertificateVerify or not.
+			 */
+			if (!s->s3->change_cipher_spec)
+				s->d1->change_cipher_spec_ok = 1;
 			/* we should decide if we expected this one */
 			ret=ssl3_get_cert_verify(s);
 			if (ret <= 0) goto end;
@@ -711,7 +720,18 @@ int dtls1_accept(SSL *s)
 
 		case SSL3_ST_SR_FINISHED_A:
 		case SSL3_ST_SR_FINISHED_B:
-			s->d1->change_cipher_spec_ok = 1;
+			/*
+			 * Enable CCS for resumed handshakes.
+			 * In a full handshake, we end up here through
+			 * SSL3_ST_SR_CERT_VRFY_B, so change_cipher_spec_ok was
+			 * already set. Receiving a CCS clears the flag, so make
+			 * sure not to re-enable it to ban duplicates.
+			 * s->s3->change_cipher_spec is set when a CCS is
+			 * processed in d1_pkt.c, and remains set until
+			 * the client's Finished message is read.
+			 */
+			if (!s->s3->change_cipher_spec)
+				s->d1->change_cipher_spec_ok = 1;
 			ret=ssl3_get_finished(s,SSL3_ST_SR_FINISHED_A,
 				SSL3_ST_SR_FINISHED_B);
 			if (ret <= 0) goto end;
diff --git a/ssl/dtls1.h b/ssl/dtls1.h
index 5cb79f1dac..af86f60fb5 100644
--- a/ssl/dtls1.h
+++ b/ssl/dtls1.h
@@ -256,6 +256,10 @@ typedef struct dtls1_state_st
 	unsigned int handshake_fragment_len;
 
 	unsigned int retransmitting;
+	/*
+	 * Set when the handshake is ready to process peer's ChangeCipherSpec message.
+	 * Cleared after the message has been processed.
+	 */
 	unsigned int change_cipher_spec_ok;
 
 #ifndef OPENSSL_NO_SCTP
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index f8d76781f2..c9b5ee1831 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -273,6 +273,9 @@ int ssl3_connect(SSL *s)
 			s->state=SSL3_ST_CW_CLNT_HELLO_A;
 			s->ctx->stats.sess_connect++;
 			s->init_num=0;
+			s->s3->flags &= ~SSL3_FLAGS_CCS_OK;
+			/* Should have been reset by ssl3_get_finished, too. */
+			s->s3->change_cipher_spec = 0;
 			break;
 
 		case SSL3_ST_CW_CLNT_HELLO_A:
@@ -421,12 +424,10 @@ int ssl3_connect(SSL *s)
 			else
 				{
 				s->state=SSL3_ST_CW_CHANGE_A;
-				s->s3->change_cipher_spec=0;
 				}
 			if (s->s3->flags & TLS1_FLAGS_SKIP_CERT_VERIFY)
 				{
 				s->state=SSL3_ST_CW_CHANGE_A;
-				s->s3->change_cipher_spec=0;
 				}
 
 			s->init_num=0;
@@ -438,7 +439,6 @@ int ssl3_connect(SSL *s)
 			if (ret <= 0) goto end;
 			s->state=SSL3_ST_CW_CHANGE_A;
 			s->init_num=0;
-			s->s3->change_cipher_spec=0;
 			break;
 
 		case SSL3_ST_CW_CHANGE_A:
@@ -498,7 +498,6 @@ int ssl3_connect(SSL *s)
 				s->method->ssl3_enc->client_finished_label,
 				s->method->ssl3_enc->client_finished_label_len);
 			if (ret <= 0) goto end;
-			s->s3->flags |= SSL3_FLAGS_CCS_OK;
 			s->state=SSL3_ST_CW_FLUSH;
 
 			/* clear flags */
@@ -547,7 +546,6 @@ int ssl3_connect(SSL *s)
 
 		case SSL3_ST_CR_FINISHED_A:
 		case SSL3_ST_CR_FINISHED_B:
-
 			s->s3->flags |= SSL3_FLAGS_CCS_OK;
 			ret=ssl3_get_finished(s,SSL3_ST_CR_FINISHED_A,
 				SSL3_ST_CR_FINISHED_B);
@@ -986,7 +984,6 @@ int ssl3_get_server_hello(SSL *s)
 			s->session->cipher = pref_cipher ?
 				pref_cipher : ssl_get_cipher_by_char(s, p+j);
 			s->hit = 1;
-			s->s3->flags |= SSL3_FLAGS_CCS_OK;
 			}
 		}
 #endif /* OPENSSL_NO_TLSEXT */
@@ -1002,7 +999,6 @@ int ssl3_get_server_hello(SSL *s)
 		SSLerr(SSL_F_SSL3_GET_SERVER_HELLO,SSL_R_ATTEMPT_TO_REUSE_SESSION_IN_DIFFERENT_CONTEXT);
 		goto f_err;
 		}
-	    s->s3->flags |= SSL3_FLAGS_CCS_OK;
 	    s->hit=1;
 	    }
 	/* a miss or crap from the other end */
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index 4914838847..6f82d3ceb4 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -301,6 +301,9 @@ int ssl3_accept(SSL *s)
 			s->init_num=0;
 			s->s3->flags &= ~SSL3_FLAGS_SGC_RESTART_DONE;
 			s->s3->flags &= ~TLS1_FLAGS_SKIP_CERT_VERIFY;
+			s->s3->flags &= ~SSL3_FLAGS_CCS_OK;
+			/* Should have been reset by ssl3_get_finished, too. */
+			s->s3->change_cipher_spec = 0;
 
 			if (s->state != SSL_ST_RENEGOTIATE)
 				{
@@ -676,8 +679,14 @@ int ssl3_accept(SSL *s)
 
 		case SSL3_ST_SR_CERT_VRFY_A:
 		case SSL3_ST_SR_CERT_VRFY_B:
-
-			s->s3->flags |= SSL3_FLAGS_CCS_OK;
+			/*
+			 * This *should* be the first time we enable CCS, but be
+			 * extra careful about surrounding code changes. We need
+			 * to set this here because we don't know if we're
+			 * expecting a CertificateVerify or not.
+			 */
+			if (!s->s3->change_cipher_spec)
+				s->s3->flags |= SSL3_FLAGS_CCS_OK;
 			/* we should decide if we expected this one */
 			ret=ssl3_get_cert_verify(s);
 			if (ret <= 0) goto end;
@@ -696,6 +705,19 @@ int ssl3_accept(SSL *s)
 #if !defined(OPENSSL_NO_TLSEXT) && !defined(OPENSSL_NO_NEXTPROTONEG)
 		case SSL3_ST_SR_NEXT_PROTO_A:
 		case SSL3_ST_SR_NEXT_PROTO_B:
+			/*
+			 * Enable CCS for resumed handshakes with NPN.
+			 * In a full handshake with NPN, we end up here through
+			 * SSL3_ST_SR_CERT_VRFY_B, where SSL3_FLAGS_CCS_OK was
+			 * already set. Receiving a CCS clears the flag, so make
+			 * sure not to re-enable it to ban duplicates.
+			 * s->s3->change_cipher_spec is set when a CCS is
+			 * processed in s3_pkt.c, and remains set until
+			 * the client's Finished message is read.
+			 */
+			if (!s->s3->change_cipher_spec)
+				s->s3->flags |= SSL3_FLAGS_CCS_OK;
+
 			ret=ssl3_get_next_proto(s);
 			if (ret <= 0) goto end;
 			s->init_num = 0;
@@ -705,7 +727,18 @@ int ssl3_accept(SSL *s)
 
 		case SSL3_ST_SR_FINISHED_A:
 		case SSL3_ST_SR_FINISHED_B:
-			s->s3->flags |= SSL3_FLAGS_CCS_OK;
+			/*
+			 * Enable CCS for resumed handshakes without NPN.
+			 * In a full handshake, we end up here through
+			 * SSL3_ST_SR_CERT_VRFY_B, where SSL3_FLAGS_CCS_OK was
+			 * already set. Receiving a CCS clears the flag, so make
+			 * sure not to re-enable it to ban duplicates.
+			 * s->s3->change_cipher_spec is set when a CCS is
+			 * processed in s3_pkt.c, and remains set until
+			 * the client's Finished message is read.
+			 */
+			if (!s->s3->change_cipher_spec)
+				s->s3->flags |= SSL3_FLAGS_CCS_OK;
 			ret=ssl3_get_finished(s,SSL3_ST_SR_FINISHED_A,
 				SSL3_ST_SR_FINISHED_B);
 			if (ret <= 0) goto end;
@@ -777,7 +810,6 @@ int ssl3_accept(SSL *s)
 #else
 				if (s->s3->next_proto_neg_seen)
 					{
-					s->s3->flags |= SSL3_FLAGS_CCS_OK;
 					s->s3->tmp.next_state=SSL3_ST_SR_NEXT_PROTO_A;
 					}
 				else
diff --git a/ssl/ssl3.h b/ssl/ssl3.h
index 274e6773c8..36320ffed0 100644
--- a/ssl/ssl3.h
+++ b/ssl/ssl3.h
@@ -429,8 +429,12 @@ typedef struct ssl3_buffer_st
 #define TLS1_FLAGS_TLS_PADDING_BUG		0x0008
 #define TLS1_FLAGS_SKIP_CERT_VERIFY		0x0010
 #define TLS1_FLAGS_KEEP_HANDSHAKE		0x0020
+/*
+ * Set when the handshake is ready to process peer's ChangeCipherSpec message.
+ * Cleared after the message has been processed.
+ */
 #define SSL3_FLAGS_CCS_OK			0x0080
- 
+
 /* SSL3_FLAGS_SGC_RESTART_DONE is set when we
  * restart a handshake because of MS SGC and so prevents us
  * from restarting the handshake in a loop. It's reset on a
@@ -492,8 +496,11 @@ typedef struct ssl3_state_st
 	 * and freed and MD_CTX-es for all required digests are stored in
 	 * this array */
 	EVP_MD_CTX **handshake_dgst;
-	/* this is set whenerver we see a change_cipher_spec message
-	 * come in when we are not looking for one */
+	/*
+	 * Set whenever an expected ChangeCipherSpec message is processed.
+	 * Unset when the peer's Finished message is received.
+	 * Unexpected ChangeCipherSpec messages trigger a fatal alert.
+	 */
 	int change_cipher_spec;
 
 	int warn_alert;
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 160ce7628a..8e802a2e3f 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -2560,7 +2560,7 @@ static int ssl_scan_serverhello_tlsext(SSL *s, unsigned char **p, unsigned char
 #ifndef OPENSSL_NO_NEXTPROTONEG
 	s->s3->next_proto_neg_seen = 0;
 #endif
-        s->tlsext_ticket_expected = 0;
+	s->tlsext_ticket_expected = 0;
 
 	if (s->s3->alpn_selected)
 		{