Process alerts between ChangeCipherSpec and Finished.

This mostly[*] doesn't matter for TLS since the message would have been
rejected anyway, but, in DTLS, if the peer rejects our Finished, it will send
an encrypted alert. This will then cause it to hang, which isn't very helpful.

I've made the change on both TLS and DTLS so the two protocols don't diverge on
this point. It is true that we're accepting nominally encrypted and
authenticated alerts before Finished, but, prior to ChangeCipherSpec, the
alerts are sent in the clear anyway so an attacker could already inject alerts.
A consumer could only be sensitive to it being post-CCS if it was watching
msg_callback. The only non-debug consumer of msg_callback I've found anywhere
is some hostapd code to detect Heartbeat.

See https://code.google.com/p/webrtc/issues/detail?id=4403 for an instance
where the equivalent behavior in OpenSSL masks an alert.

[*] This does change behavior slightly if the peer sends a warning alert
between CCS and Finished. I believe this is benign as warning alerts are
usually ignored apart from info_callback and msg_callback. The one exception is
a close_notify which is a slightly new state (accepting close_notify during a
handshake seems questionable...), but they're processed pre-CCS too.

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

ssl/d1_pkt.c

@@ -664,10 +664,11 @@ start:
 
   /* |change_cipher_spec is set when we receive a ChangeCipherSpec and reset by
    * ssl3_get_finished. */
-  if (s->s3->change_cipher_spec && rr->type != SSL3_RT_HANDSHAKE) {
-    /* We now have application data between CCS and Finished. Most likely the
-     * packets were reordered on their way, so buffer the application data for
-     * later processing rather than dropping the connection. */
+  if (s->s3->change_cipher_spec && rr->type != SSL3_RT_HANDSHAKE &&
+      rr->type != SSL3_RT_ALERT) {
+    /* We now have an unexpected record between CCS and Finished. Most likely
+     * the packets were reordered on their way, so buffer the application data
+     * for later processing rather than dropping the connection. */
     if (dtls1_buffer_record(s, &(s->d1->buffered_app_data), rr->seq_num) < 0) {
       OPENSSL_PUT_ERROR(SSL, dtls1_read_bytes, ERR_R_INTERNAL_ERROR);
       return -1;

ssl/s3_pkt.c

@@ -815,9 +815,10 @@ start:
 
   /* we now have a packet which can be read and processed */
 
-  if (s->s3->change_cipher_spec /* set when we receive ChangeCipherSpec,
-                                 * reset by ssl3_get_finished */
-      && rr->type != SSL3_RT_HANDSHAKE) {
+  /* |change_cipher_spec is set when we receive a ChangeCipherSpec and reset by
+   * ssl3_get_finished. */
+  if (s->s3->change_cipher_spec && rr->type != SSL3_RT_HANDSHAKE &&
+      rr->type != SSL3_RT_ALERT) {
     al = SSL_AD_UNEXPECTED_MESSAGE;
     OPENSSL_PUT_ERROR(SSL, ssl3_read_bytes,
                       SSL_R_DATA_BETWEEN_CCS_AND_FINISHED);

ssl/test/runner/common.go

@@ -610,6 +610,10 @@ type ProtocolBugs struct {
 	// be sent immediately after ChangeCipherSpec.
 	AppDataAfterChangeCipherSpec []byte
 
+	// AlertAfterChangeCipherSpec, if non-zero, causes an alert to be sent
+	// immediately after ChangeCipherSpec.
+	AlertAfterChangeCipherSpec alert
+
 	// TimeoutSchedule is the schedule of packet drops and simulated
 	// timeouts for before each handshake leg from the peer.
 	TimeoutSchedule []time.Duration

ssl/test/runner/handshake_client.go

@@ -863,6 +863,10 @@ func (hs *clientHandshakeState) sendFinished(isResume bool) error {
 	if c.config.Bugs.AppDataAfterChangeCipherSpec != nil {
 		c.writeRecord(recordTypeApplicationData, c.config.Bugs.AppDataAfterChangeCipherSpec)
 	}
+	if c.config.Bugs.AlertAfterChangeCipherSpec != 0 {
+		c.sendAlert(c.config.Bugs.AlertAfterChangeCipherSpec)
+		return errors.New("tls: simulating post-CCS alert")
+	}
 
 	if !c.config.Bugs.SkipFinished {
 		c.writeRecord(recordTypeHandshake, postCCSBytes)

ssl/test/runner/handshake_server.go

@@ -854,6 +854,10 @@ func (hs *serverHandshakeState) sendFinished() error {
 	if c.config.Bugs.AppDataAfterChangeCipherSpec != nil {
 		c.writeRecord(recordTypeApplicationData, c.config.Bugs.AppDataAfterChangeCipherSpec)
 	}
+	if c.config.Bugs.AlertAfterChangeCipherSpec != 0 {
+		c.sendAlert(c.config.Bugs.AlertAfterChangeCipherSpec)
+		return errors.New("tls: simulating post-CCS alert")
+	}
 
 	if !c.config.Bugs.SkipFinished {
 		c.writeRecord(recordTypeHandshake, postCCSBytes)

ssl/test/runner/runner.go

@@ -690,6 +690,27 @@ var testCases = []testCase{
 			},
 		},
 	},
+	{
+		name: "AlertAfterChangeCipherSpec",
+		config: Config{
+			Bugs: ProtocolBugs{
+				AlertAfterChangeCipherSpec: alertRecordOverflow,
+			},
+		},
+		shouldFail:    true,
+		expectedError: ":TLSV1_ALERT_RECORD_OVERFLOW:",
+	},
+	{
+		protocol: dtls,
+		name:     "AlertAfterChangeCipherSpec-DTLS",
+		config: Config{
+			Bugs: ProtocolBugs{
+				AlertAfterChangeCipherSpec: alertRecordOverflow,
+			},
+		},
+		shouldFail:    true,
+		expectedError: ":TLSV1_ALERT_RECORD_OVERFLOW:",
+	},
 	{
 		protocol: dtls,
 		name:     "ReorderHandshakeFragments-Small-DTLS",