Tolerate early ChangeCipherSpec in DTLS.

This would only come up if the peer didn't pack records together, but
it's free to handle. Notably OpenSSL has a bug where it does not pack
retransmits together.

Change-Id: I0927d768f6b50c62bacdd82bd1c95396ed503cf3
Reviewed-on: https://boringssl-review.googlesource.com/18724
Reviewed-by: David Benjamin <davidben@google.com>

ssl/d1_both.cc

@@ -298,12 +298,10 @@ static hm_fragment *dtls1_get_incoming_message(
   return frag;
 }
 
-/* dtls1_process_handshake_record reads a handshake record and processes it. It
- * returns one if the record was successfully processed and 0 or -1 on error. */
+/* dtls1_process_handshake_record reads a record for the handshake and processes
+ * it. It returns one on success and 0 or -1 on error. */
 static int dtls1_process_handshake_record(SSL *ssl) {
   SSL3_RECORD *rr = &ssl->s3->rrec;
-
-start:
   if (rr->length == 0) {
     int ret = dtls1_get_record(ssl);
     if (ret <= 0) {
@@ -311,27 +309,53 @@ start:
     }
   }
 
-  /* Cross-epoch records are discarded, but we may receive out-of-order
-   * application data between ChangeCipherSpec and Finished or a
-   * ChangeCipherSpec before the appropriate point in the handshake. Those must
-   * be silently discarded.
-   *
-   * However, only allow the out-of-order records in the correct epoch.
-   * Application data must come in the encrypted epoch, and ChangeCipherSpec in
-   * the unencrypted epoch (we never renegotiate). Other cases fall through and
-   * fail with a fatal error. */
-  if ((rr->type == SSL3_RT_APPLICATION_DATA &&
-       !ssl->s3->aead_read_ctx->is_null_cipher()) ||
-      (rr->type == SSL3_RT_CHANGE_CIPHER_SPEC &&
-       ssl->s3->aead_read_ctx->is_null_cipher())) {
-    rr->length = 0;
-    goto start;
-  }
+  switch (rr->type) {
+    case SSL3_RT_APPLICATION_DATA:
+      /* Unencrypted application data records are always illegal. */
+      if (ssl->s3->aead_read_ctx->is_null_cipher()) {
+        ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
+        OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD);
+        return -1;
+      }
+
+      /* Out-of-order application data may be received between ChangeCipherSpec
+       * and finished. Discard it. */
+      rr->length = 0;
+      ssl_read_buffer_discard(ssl);
+      return 1;
 
-  if (rr->type != SSL3_RT_HANDSHAKE) {
-    ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
-    OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD);
-    return -1;
+    case SSL3_RT_CHANGE_CIPHER_SPEC:
+      /* We do not support renegotiation, so encrypted ChangeCipherSpec records
+       * are illegal. */
+      if (!ssl->s3->aead_read_ctx->is_null_cipher()) {
+        ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
+        OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD);
+        return -1;
+      }
+
+      if (rr->length != 1 || rr->data[0] != SSL3_MT_CCS) {
+        OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_CHANGE_CIPHER_SPEC);
+        ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
+        return -1;
+      }
+
+      /* Flag the ChangeCipherSpec for later. */
+      ssl->d1->has_change_cipher_spec = true;
+      ssl_do_msg_callback(ssl, 0 /* read */, SSL3_RT_CHANGE_CIPHER_SPEC,
+                          rr->data, rr->length);
+
+      rr->length = 0;
+      ssl_read_buffer_discard(ssl);
+      return 1;
+
+    case SSL3_RT_HANDSHAKE:
+      /* Break out to main processing. */
+      break;
+
+    default:
+      ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
+      OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD);
+      return -1;
   }
 
   CBS cbs;
@@ -490,6 +514,19 @@ int dtls1_parse_fragment(CBS *cbs, struct hm_header_st *out_hdr,
   return 1;
 }
 
+int dtls1_read_change_cipher_spec(SSL *ssl) {
+  /* Process handshake records until there is a ChangeCipherSpec. */
+  while (!ssl->d1->has_change_cipher_spec) {
+    int ret = dtls1_process_handshake_record(ssl);
+    if (ret <= 0) {
+      return ret;
+    }
+  }
+
+  ssl->d1->has_change_cipher_spec = false;
+  return 1;
+}
+
 
 /* Sending handshake messages. */
 

ssl/d1_pkt.cc

@@ -288,46 +288,6 @@ again:
   return len;
 }
 
-int dtls1_read_change_cipher_spec(SSL *ssl) {
-  SSL3_RECORD *rr = &ssl->s3->rrec;
-
-again:
-  if (rr->length == 0) {
-    int ret = dtls1_get_record(ssl);
-    if (ret <= 0) {
-      return ret;
-    }
-  }
-
-  /* Drop handshake records silently. The epochs match, so this must be a
-   * retransmit of a message we already received. */
-  if (rr->type == SSL3_RT_HANDSHAKE) {
-    rr->length = 0;
-    goto again;
-  }
-
-  /* Other record types are illegal in this epoch. Note all application data
-   * records come in the encrypted epoch. */
-  if (rr->type != SSL3_RT_CHANGE_CIPHER_SPEC) {
-    ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
-    OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD);
-    return -1;
-  }
-
-  if (rr->length != 1 || rr->data[0] != SSL3_MT_CCS) {
-    OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_CHANGE_CIPHER_SPEC);
-    ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
-    return -1;
-  }
-
-  ssl_do_msg_callback(ssl, 0 /* read */, SSL3_RT_CHANGE_CIPHER_SPEC, rr->data,
-                      rr->length);
-
-  rr->length = 0;
-  ssl_read_buffer_discard(ssl);
-  return 1;
-}
-
 void dtls1_read_close_notify(SSL *ssl) {
   /* Bidirectional shutdown doesn't make sense for an unordered transport. DTLS
    * alerts also aren't delivered reliably, so we may even time out because the

ssl/dtls_method.cc

@@ -78,7 +78,7 @@ static void dtls1_received_flight(SSL *ssl) { dtls1_stop_timer(ssl); }
 
 static int dtls1_set_read_state(SSL *ssl, UniquePtr<SSLAEADContext> aead_ctx) {
   /* Cipher changes are illegal when there are buffered incoming messages. */
-  if (dtls_has_incoming_messages(ssl)) {
+  if (dtls_has_incoming_messages(ssl) || ssl->d1->has_change_cipher_spec) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_BUFFERED_MESSAGES_ON_CIPHER_CHANGE);
     ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
     return 0;

ssl/handshake_client.cc

@@ -781,7 +781,7 @@ static int dtls1_get_hello_verify_request(SSL_HANDSHAKE *hs) {
   }
 
   if (ssl->s3->tmp.message_type != DTLS1_MT_HELLO_VERIFY_REQUEST) {
-    ssl->d1->send_cookie = 0;
+    ssl->d1->send_cookie = false;
     ssl->s3->tmp.reuse_message = 1;
     return 1;
   }
@@ -799,7 +799,7 @@ static int dtls1_get_hello_verify_request(SSL_HANDSHAKE *hs) {
   OPENSSL_memcpy(ssl->d1->cookie, CBS_data(&cookie), CBS_len(&cookie));
   ssl->d1->cookie_len = CBS_len(&cookie);
 
-  ssl->d1->send_cookie = 1;
+  ssl->d1->send_cookie = true;
   return 1;
 }
 

ssl/internal.h

@@ -1792,9 +1792,13 @@ struct OPENSSL_timeval {
 };
 
 struct DTLS1_STATE {
-  /* send_cookie is true if we are resending the ClientHello
-   * with a cookie from a HelloVerifyRequest. */
-  unsigned int send_cookie;
+  /* send_cookie is true if we are resending the ClientHello with a cookie from
+   * a HelloVerifyRequest. */
+  bool send_cookie:1;
+
+  /* has_change_cipher_spec is true if we have received a ChangeCipherSpec from
+   * the peer in this epoch. */
+  bool has_change_cipher_spec:1;
 
   uint8_t cookie[DTLS1_COOKIE_LENGTH];
   size_t cookie_len;

ssl/test/runner/common.go

@@ -584,6 +584,11 @@ type ProtocolBugs struct {
 	// may be used to test DTLS's handling of reordered ChangeCipherSpec.
 	StrayChangeCipherSpec bool
 
+	// ReorderChangeCipherSpec causes the ChangeCipherSpec message to be
+	// sent at start of each flight in DTLS. Unlike EarlyChangeCipherSpec,
+	// the cipher change happens at the usual time.
+	ReorderChangeCipherSpec bool
+
 	// FragmentAcrossChangeCipherSpec causes the implementation to fragment
 	// the Finished (or NextProto) message around the ChangeCipherSpec
 	// messages.

ssl/test/runner/conn.go

@@ -1023,10 +1023,8 @@ func (c *Conn) writeRecord(typ recordType, data []byte) (n int, err error) {
 			newData[0] = msgType
 			data = newData
 		}
-	}
 
-	if msgType := c.config.Bugs.SendTrailingMessageData; msgType != 0 {
-		if typ == recordTypeHandshake && data[0] == msgType {
+		if c.config.Bugs.SendTrailingMessageData != 0 && msgType == c.config.Bugs.SendTrailingMessageData {
 			newData := make([]byte, len(data))
 			copy(newData, data)
 
@@ -1060,6 +1058,11 @@ func (c *Conn) writeRecord(typ recordType, data []byte) (n int, err error) {
 		}
 	}
 
+	// Flush buffered data before writing anything.
+	if err := c.flushHandshake(); err != nil {
+		return 0, err
+	}
+
 	return c.doWriteRecord(typ, data)
 }
 

ssl/test/runner/dtls.go

@@ -150,12 +150,29 @@ func (c *Conn) makeFragment(header, data []byte, fragOffset, fragLen int) []byte
 
 func (c *Conn) dtlsWriteRecord(typ recordType, data []byte) (n int, err error) {
 	if typ != recordTypeHandshake {
+		reorder := typ == recordTypeChangeCipherSpec && c.config.Bugs.ReorderChangeCipherSpec
+
+		// Flush pending handshake messages before writing a new record.
+		if !reorder {
+			err = c.dtlsFlushHandshake()
+			if err != nil {
+				return
+			}
+		}
+
 		// Only handshake messages are fragmented.
 		n, err = c.dtlsWriteRawRecord(typ, data)
 		if err != nil {
 			return
 		}
 
+		if reorder {
+			err = c.dtlsFlushHandshake()
+			if err != nil {
+				return
+			}
+		}
+
 		if typ == recordTypeChangeCipherSpec {
 			err = c.out.changeCipherSpec(c.config)
 			if err != nil {

ssl/test/runner/handshake_client.go

@@ -1587,7 +1587,6 @@ func (hs *clientHandshakeState) sendFinished(out []byte, isResume bool) error {
 		c.writeRecord(recordTypeHandshake, postCCSMsgs[0])
 		postCCSMsgs = postCCSMsgs[1:]
 	}
-	c.flushHandshake()
 
 	if !c.config.Bugs.SkipChangeCipherSpec &&
 		c.config.Bugs.EarlyChangeCipherSpec == 0 {
@@ -1614,9 +1613,9 @@ func (hs *clientHandshakeState) sendFinished(out []byte, isResume bool) error {
 		if c.config.Bugs.SendExtraFinished {
 			c.writeRecord(recordTypeHandshake, finished.marshal())
 		}
-
-		c.flushHandshake()
 	}
+
+	c.flushHandshake()
 	return nil
 }
 

ssl/test/runner/handshake_server.go

@@ -1819,7 +1819,6 @@ func (hs *serverHandshakeState) sendFinished(out []byte) error {
 		c.writeRecord(recordTypeHandshake, postCCSBytes)
 		postCCSBytes = nil
 	}
-	c.flushHandshake()
 
 	if !c.config.Bugs.SkipChangeCipherSpec {
 		ccs := []byte{1}
@@ -1842,11 +1841,11 @@ func (hs *serverHandshakeState) sendFinished(out []byte) error {
 		if c.config.Bugs.SendExtraFinished {
 			c.writeRecord(recordTypeHandshake, finished.marshal())
 		}
+	}
 
-		if !c.config.Bugs.PackHelloRequestWithFinished {
-			// Defer flushing until renegotiation.
-			c.flushHandshake()
-		}
+	if !c.config.Bugs.PackHelloRequestWithFinished {
+		// Defer flushing until renegotiation.
+		c.flushHandshake()
 	}
 
 	c.cipherSuite = hs.suite

ssl/test/runner/runner.go

@@ -10040,6 +10040,31 @@ func addChangeCipherSpecTests() {
 		},
 	})
 
+	// Test that reordered ChangeCipherSpecs are tolerated.
+	testCases = append(testCases, testCase{
+		protocol: dtls,
+		name:     "ReorderChangeCipherSpec-DTLS-Client",
+		config: Config{
+			MaxVersion: VersionTLS12,
+			Bugs: ProtocolBugs{
+				ReorderChangeCipherSpec: true,
+			},
+		},
+		resumeSession: true,
+	})
+	testCases = append(testCases, testCase{
+		testType: serverTest,
+		protocol: dtls,
+		name:     "ReorderChangeCipherSpec-DTLS-Server",
+		config: Config{
+			MaxVersion: VersionTLS12,
+			Bugs: ProtocolBugs{
+				ReorderChangeCipherSpec: true,
+			},
+		},
+		resumeSession: true,
+	})
+
 	// Test that the contents of ChangeCipherSpec are checked.
 	testCases = append(testCases, testCase{
 		name: "BadChangeCipherSpec-1",