Change CCS_OK to EXPECT_CCS.

Now that the flag is set accurately, use it to enforce that the handshake and
CCS synchronization. If EXPECT_CCS is set, enforce that:

(a) No handshake records may be received before ChangeCipherSpec.

(b) There is no pending handshake data at the point EXPECT_CCS is set.

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

include/openssl/ssl.h

@@ -2522,6 +2522,7 @@ void ERR_load_SSL_strings(void);
 #define SSL_F_tls1_change_cipher_state_aead 279
 #define SSL_F_tls1_aead_ctx_init 280
 #define SSL_F_tls1_check_duplicate_extensions 281
+#define SSL_F_ssl3_expect_change_cipher_spec 282
 #define SSL_R_UNABLE_TO_FIND_ECDH_PARAMETERS 100
 #define SSL_R_DECRYPTION_FAILED_OR_BAD_RECORD_MAC 101
 #define SSL_R_INVALID_NULL_CMD_NAME 102
@@ -2832,6 +2833,8 @@ void ERR_load_SSL_strings(void);
 #define SSL_R_CLIENTHELLO_PARSE_FAILED 437
 #define SSL_R_CONNECTION_REJECTED 438
 #define SSL_R_DECODE_ERROR 439
+#define SSL_R_UNPROCESSED_HANDSHAKE_DATA 440
+#define SSL_R_HANDSHAKE_RECORD_BEFORE_CCS 441
 #define SSL_R_SSLV3_ALERT_UNEXPECTED_MESSAGE 1010
 #define SSL_R_SSLV3_ALERT_BAD_RECORD_MAC 1020
 #define SSL_R_TLSV1_ALERT_DECRYPTION_FAILED 1021

include/openssl/ssl3.h

@@ -343,7 +343,9 @@ 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
-#define SSL3_FLAGS_CCS_OK			0x0080
+/* TODO(davidben): This flag can probably be merged into s3->change_cipher_spec
+ * to something tri-state. (Normal / Expect CCS / Between CCS and Finished). */
+#define SSL3_FLAGS_EXPECT_CCS			0x0080
  
 /* SSL3_FLAGS_SGC_RESTART_DONE is set when we
  * restart a handshake because of MS SGC and so prevents us

ssl/s3_both.c

@@ -245,7 +245,9 @@ int ssl3_get_finished(SSL *s, int a, int b)
 
 	if (!ok) return((int)n);
 
-	/* If this occurs, we have missed a message */
+	/* If this occurs, we have missed a message.
+	 * TODO(davidben): Is this check now redundant with
+	 * SSL3_FLAGS_EXPECT_CCS? */
 	if (!s->s3->change_cipher_spec)
 		{
 		al=SSL_AD_UNEXPECTED_MESSAGE;

ssl/s3_clnt.c

@@ -517,7 +517,11 @@ int ssl3_connect(SSL *s)
 		case SSL3_ST_CR_CHANGE:
 			/* At this point, the next message must be entirely
 			 * behind a ChangeCipherSpec. */
-			s->s3->flags |= SSL3_FLAGS_CCS_OK;
+			if (!ssl3_expect_change_cipher_spec(s))
+				{
+				ret = -1;
+				goto end;
+				}
 			s->state = SSL3_ST_CR_FINISHED_A;
 			break;
 

ssl/s3_pkt.c

@@ -901,6 +901,22 @@ int ssl3_write_pending(SSL *s, int type, const unsigned char *buf,
 		}
 	}
 
+/* ssl3_expect_change_cipher_spec informs the record layer that a
+ * ChangeCipherSpec record is required at this point. If a Handshake record is
+ * received before ChangeCipherSpec, the connection will fail. Moreover, if
+ * there are unprocessed handshake bytes, the handshake will also fail and the
+ * function returns zero. Otherwise, the function returns one. */
+int ssl3_expect_change_cipher_spec(SSL *s)
+	{
+	if (s->s3->handshake_fragment_len > 0 || s->s3->tmp.reuse_message)
+		{
+		OPENSSL_PUT_ERROR(SSL, ssl3_expect_change_cipher_spec, SSL_R_UNPROCESSED_HANDSHAKE_DATA);
+		return 0;
+		}
+	s->s3->flags |= SSL3_FLAGS_EXPECT_CCS;
+	return 1;
+	}
+
 /* Return up to 'len' payload bytes received in 'type' records.
  * 'type' is one of the following:
  *
@@ -1007,6 +1023,15 @@ start:
 		goto f_err;
 		}
 
+	/* If we are expecting a ChangeCipherSpec, it is illegal to receive a
+	 * Handshake record. */
+	if (rr->type == SSL3_RT_HANDSHAKE && (s->s3->flags & SSL3_FLAGS_EXPECT_CCS))
+		{
+		al = SSL_AD_UNEXPECTED_MESSAGE;
+		OPENSSL_PUT_ERROR(SSL, ssl3_read_bytes, SSL_R_HANDSHAKE_RECORD_BEFORE_CCS);
+		goto f_err;
+		}
+
 	/* If the other end has shut down, throw anything we read away
 	 * (even in 'peek' mode) */
 	if (s->shutdown & SSL_RECEIVED_SHUTDOWN)
@@ -1016,7 +1041,6 @@ start:
 		return(0);
 		}
 
-
 	if (type == rr->type) /* SSL3_RT_APPLICATION_DATA or SSL3_RT_HANDSHAKE */
 		{
 		/* make sure that we are not getting application data when we
@@ -1274,14 +1298,14 @@ start:
 			goto f_err;
 			}
 
-		if (!(s->s3->flags & SSL3_FLAGS_CCS_OK))
+		if (!(s->s3->flags & SSL3_FLAGS_EXPECT_CCS))
 			{
 			al=SSL_AD_UNEXPECTED_MESSAGE;
 			OPENSSL_PUT_ERROR(SSL, ssl3_read_bytes, SSL_R_CCS_RECEIVED_EARLY);
 			goto f_err;
 			}
 
-		s->s3->flags &= ~SSL3_FLAGS_CCS_OK;
+		s->s3->flags &= ~SSL3_FLAGS_EXPECT_CCS;
 
 		rr->length=0;
 

ssl/s3_srvr.c

@@ -573,7 +573,11 @@ int ssl3_accept(SSL *s)
 
 			/* At this point, the next message must be entirely
 			 * behind a ChangeCipherSpec. */
-			s->s3->flags |= SSL3_FLAGS_CCS_OK;
+			if (!ssl3_expect_change_cipher_spec(s))
+				{
+				ret = -1;
+				goto end;
+				}
 			if (next_proto_neg)
 				s->state = SSL3_ST_SR_NEXT_PROTO_A;
 			else if (channel_id)
@@ -3023,7 +3027,9 @@ int ssl3_get_next_proto(SSL *s)
 
 	/* s->state doesn't reflect whether ChangeCipherSpec has been received
 	 * in this handshake, but s->s3->change_cipher_spec does (will be reset
-	 * by ssl3_get_finished). */
+	 * by ssl3_get_finished).
+	 * TODO(davidben): Is this check now redundant with
+	 * SSL3_FLAGS_EXPECT_CCS? */
 	if (!s->s3->change_cipher_spec)
 		{
 		OPENSSL_PUT_ERROR(SSL, ssl3_get_next_proto, SSL_R_GOT_NEXT_PROTO_BEFORE_A_CCS);
@@ -3096,7 +3102,9 @@ int ssl3_get_channel_id(SSL *s)
 
 	/* s->state doesn't reflect whether ChangeCipherSpec has been received
 	 * in this handshake, but s->s3->change_cipher_spec does (will be reset
-	 * by ssl3_get_finished). */
+	 * by ssl3_get_finished).
+	 * TODO(davidben): Is this check now redundant with
+	 * SSL3_FLAGS_EXPECT_CCS? */
 	if (!s->s3->change_cipher_spec)
 		{
 		OPENSSL_PUT_ERROR(SSL, ssl3_get_channel_id, SSL_R_GOT_CHANNEL_ID_BEFORE_A_CCS);

ssl/ssl_error.c

@@ -108,6 +108,7 @@ const ERR_STRING_DATA SSL_error_string_data[] = {
   {ERR_PACK(ERR_LIB_SSL, SSL_F_ssl3_ctx_ctrl, 0), "ssl3_ctx_ctrl"},
   {ERR_PACK(ERR_LIB_SSL, SSL_F_ssl3_digest_cached_records, 0), "ssl3_digest_cached_records"},
   {ERR_PACK(ERR_LIB_SSL, SSL_F_ssl3_do_change_cipher_spec, 0), "ssl3_do_change_cipher_spec"},
+  {ERR_PACK(ERR_LIB_SSL, SSL_F_ssl3_expect_change_cipher_spec, 0), "ssl3_expect_change_cipher_spec"},
   {ERR_PACK(ERR_LIB_SSL, SSL_F_ssl3_generate_key_block, 0), "ssl3_generate_key_block"},
   {ERR_PACK(ERR_LIB_SSL, SSL_F_ssl3_get_cert_status, 0), "ssl3_get_cert_status"},
   {ERR_PACK(ERR_LIB_SSL, SSL_F_ssl3_get_cert_verify, 0), "ssl3_get_cert_verify"},
@@ -298,6 +299,7 @@ const ERR_STRING_DATA SSL_error_string_data[] = {
   {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_GOT_CHANNEL_ID_BEFORE_A_CCS), "GOT_CHANNEL_ID_BEFORE_A_CCS"},
   {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_GOT_NEXT_PROTO_BEFORE_A_CCS), "GOT_NEXT_PROTO_BEFORE_A_CCS"},
   {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_GOT_NEXT_PROTO_WITHOUT_EXTENSION), "GOT_NEXT_PROTO_WITHOUT_EXTENSION"},
+  {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_HANDSHAKE_RECORD_BEFORE_CCS), "HANDSHAKE_RECORD_BEFORE_CCS"},
   {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_HTTPS_PROXY_REQUEST), "HTTPS_PROXY_REQUEST"},
   {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_HTTP_REQUEST), "HTTP_REQUEST"},
   {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_ILLEGAL_PADDING), "ILLEGAL_PADDING"},
@@ -513,6 +515,7 @@ const ERR_STRING_DATA SSL_error_string_data[] = {
   {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_UNKNOWN_SSL_VERSION), "UNKNOWN_SSL_VERSION"},
   {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_UNKNOWN_STATE), "UNKNOWN_STATE"},
   {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_UNKNOWN_SUPPLEMENTAL_DATA_TYPE), "UNKNOWN_SUPPLEMENTAL_DATA_TYPE"},
+  {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_UNPROCESSED_HANDSHAKE_DATA), "UNPROCESSED_HANDSHAKE_DATA"},
   {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_UNSAFE_LEGACY_RENEGOTIATION_DISABLED), "UNSAFE_LEGACY_RENEGOTIATION_DISABLED"},
   {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_UNSUPPORTED_CIPHER), "UNSUPPORTED_CIPHER"},
   {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_UNSUPPORTED_COMPRESSION_ALGORITHM), "UNSUPPORTED_COMPRESSION_ALGORITHM"},

ssl/ssl_locl.h

@@ -947,6 +947,7 @@ const SSL_CIPHER *ssl3_get_cipher(unsigned int u);
 int ssl3_renegotiate(SSL *ssl); 
 int ssl3_renegotiate_check(SSL *ssl); 
 int ssl3_dispatch_alert(SSL *s);
+int ssl3_expect_change_cipher_spec(SSL *s);
 int ssl3_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek);
 int ssl3_write_bytes(SSL *s, int type, const void *buf, int len);
 int ssl3_final_finish_mac(SSL *s, const char *sender, int slen,unsigned char *p);

ssl/test/runner/common.go

@@ -375,6 +375,11 @@ type ProtocolBugs struct {
 	// and 1.0.1 modes, respectively.
 	EarlyChangeCipherSpec int
 
+	// FragmentAcrossChangeCipherSpec causes the implementation to fragment
+	// the Finished (or NextProto) message around the ChangeCipherSpec
+	// messages.
+	FragmentAcrossChangeCipherSpec bool
+
 	// SkipNewSessionTicket causes the server to skip sending the
 	// NewSessionTicket message despite promising to in ServerHello.
 	SkipNewSessionTicket bool

ssl/test/runner/handshake_client.go

@@ -584,10 +584,7 @@ func (hs *clientHandshakeState) readSessionTicket() error {
 func (hs *clientHandshakeState) sendFinished() error {
 	c := hs.c
 
-	if !c.config.Bugs.SkipChangeCipherSpec &&
-		c.config.Bugs.EarlyChangeCipherSpec == 0 {
-		c.writeRecord(recordTypeChangeCipherSpec, []byte{1})
-	}
+	var postCCSBytes []byte
 	if hs.serverHello.nextProtoNeg {
 		nextProto := new(nextProtoMsg)
 		proto, fallback := mutualProtocol(c.config.NextProtos, hs.serverHello.nextProtos)
@@ -595,8 +592,9 @@ func (hs *clientHandshakeState) sendFinished() error {
 		c.clientProtocol = proto
 		c.clientProtocolFallback = fallback
 
-		hs.finishedHash.Write(nextProto.marshal())
-		c.writeRecord(recordTypeHandshake, nextProto.marshal())
+		nextProtoBytes := nextProto.marshal()
+		hs.finishedHash.Write(nextProtoBytes)
+		postCCSBytes = append(postCCSBytes, nextProtoBytes...)
 	}
 
 	finished := new(finishedMsg)
@@ -605,8 +603,21 @@ func (hs *clientHandshakeState) sendFinished() error {
 	} else {
 		finished.verifyData = hs.finishedHash.clientSum(hs.masterSecret)
 	}
-	hs.finishedHash.Write(finished.marshal())
-	c.writeRecord(recordTypeHandshake, finished.marshal())
+	finishedBytes := finished.marshal()
+	hs.finishedHash.Write(finishedBytes)
+	postCCSBytes = append(postCCSBytes, finishedBytes...)
+
+	if c.config.Bugs.FragmentAcrossChangeCipherSpec {
+		c.writeRecord(recordTypeHandshake, postCCSBytes[:5])
+		postCCSBytes = postCCSBytes[5:]
+	}
+
+	if !c.config.Bugs.SkipChangeCipherSpec &&
+		c.config.Bugs.EarlyChangeCipherSpec == 0 {
+		c.writeRecord(recordTypeChangeCipherSpec, []byte{1})
+	}
+
+	c.writeRecord(recordTypeHandshake, postCCSBytes)
 	return nil
 }
 

ssl/test/runner/handshake_server.go

@@ -598,14 +598,21 @@ func (hs *serverHandshakeState) sendSessionTicket() error {
 func (hs *serverHandshakeState) sendFinished() error {
 	c := hs.c
 
+	finished := new(finishedMsg)
+	finished.verifyData = hs.finishedHash.serverSum(hs.masterSecret)
+	postCCSBytes := finished.marshal()
+	hs.finishedHash.Write(postCCSBytes)
+
+	if c.config.Bugs.FragmentAcrossChangeCipherSpec {
+		c.writeRecord(recordTypeHandshake, postCCSBytes[:5])
+		postCCSBytes = postCCSBytes[5:]
+	}
+
 	if !c.config.Bugs.SkipChangeCipherSpec {
 		c.writeRecord(recordTypeChangeCipherSpec, []byte{1})
 	}
 
-	finished := new(finishedMsg)
-	finished.verifyData = hs.finishedHash.serverSum(hs.masterSecret)
-	hs.finishedHash.Write(finished.marshal())
-	c.writeRecord(recordTypeHandshake, finished.marshal())
+	c.writeRecord(recordTypeHandshake, postCCSBytes)
 
 	c.cipherSuite = hs.suite.id
 

ssl/test/runner/runner.go

@@ -234,7 +234,7 @@ var testCases = []testCase{
 			},
 		},
 		shouldFail:    true,
-		expectedError: ":GOT_A_FIN_BEFORE_A_CCS:",
+		expectedError: ":HANDSHAKE_RECORD_BEFORE_CCS:",
 	},
 	{
 		testType: serverTest,
@@ -245,7 +245,7 @@ var testCases = []testCase{
 			},
 		},
 		shouldFail:    true,
-		expectedError: ":GOT_A_FIN_BEFORE_A_CCS:",
+		expectedError: ":HANDSHAKE_RECORD_BEFORE_CCS:",
 	},
 	{
 		testType: serverTest,
@@ -260,7 +260,43 @@ var testCases = []testCase{
 			"-advertise-npn", "\x03foo\x03bar\x03baz",
 		},
 		shouldFail:    true,
-		expectedError: ":GOT_NEXT_PROTO_BEFORE_A_CCS:",
+		expectedError: ":HANDSHAKE_RECORD_BEFORE_CCS:",
+	},
+	{
+		name: "FragmentAcrossChangeCipherSpec-Client",
+		config: Config{
+			Bugs: ProtocolBugs{
+				FragmentAcrossChangeCipherSpec: true,
+			},
+		},
+		shouldFail:    true,
+		expectedError: ":HANDSHAKE_RECORD_BEFORE_CCS:",
+	},
+	{
+		testType: serverTest,
+		name:     "FragmentAcrossChangeCipherSpec-Server",
+		config: Config{
+			Bugs: ProtocolBugs{
+				FragmentAcrossChangeCipherSpec: true,
+			},
+		},
+		shouldFail:    true,
+		expectedError: ":HANDSHAKE_RECORD_BEFORE_CCS:",
+	},
+	{
+		testType: serverTest,
+		name:     "FragmentAcrossChangeCipherSpec-Server-NPN",
+		config: Config{
+			NextProtos: []string{"bar"},
+			Bugs: ProtocolBugs{
+				FragmentAcrossChangeCipherSpec: true,
+			},
+		},
+		flags: []string{
+			"-advertise-npn", "\x03foo\x03bar\x03baz",
+		},
+		shouldFail:    true,
+		expectedError: ":HANDSHAKE_RECORD_BEFORE_CCS:",
 	},
 	{
 		testType: serverTest,