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>
diff --git a/ssl/d1_both.cc b/ssl/d1_both.cc
index 50cca83..f019bae 100644
--- a/ssl/d1_both.cc
+++ b/ssl/d1_both.cc
@@ -298,12 +298,10 @@
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 @@
}
}
- /* 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;
+ }
- 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;
+ /* Out-of-order application data may be received between ChangeCipherSpec
+ * and finished. Discard it. */
+ rr->length = 0;
+ ssl_read_buffer_discard(ssl);
+ 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 @@
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. */
diff --git a/ssl/d1_pkt.cc b/ssl/d1_pkt.cc
index 52e8111..a9f2d7c 100644
--- a/ssl/d1_pkt.cc
+++ b/ssl/d1_pkt.cc
@@ -288,46 +288,6 @@
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
diff --git a/ssl/dtls_method.cc b/ssl/dtls_method.cc
index 15c4608..59c771b 100644
--- a/ssl/dtls_method.cc
+++ b/ssl/dtls_method.cc
@@ -78,7 +78,7 @@
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;
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index 742e106..2a3e627 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -781,7 +781,7 @@
}
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 @@
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;
}
diff --git a/ssl/internal.h b/ssl/internal.h
index a9fe951..2dbf063 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1792,9 +1792,13 @@
};
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;
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index a6a521b..b8c2785 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -584,6 +584,11 @@
// 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.
diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go
index 047c3c5..2332e6b 100644
--- a/ssl/test/runner/conn.go
+++ b/ssl/test/runner/conn.go
@@ -1023,10 +1023,8 @@
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 @@
}
}
+ // Flush buffered data before writing anything.
+ if err := c.flushHandshake(); err != nil {
+ return 0, err
+ }
+
return c.doWriteRecord(typ, data)
}
diff --git a/ssl/test/runner/dtls.go b/ssl/test/runner/dtls.go
index 72369d6..42b3a65 100644
--- a/ssl/test/runner/dtls.go
+++ b/ssl/test/runner/dtls.go
@@ -150,12 +150,29 @@
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 {
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go
index 10e841a..33c1b12 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -1587,7 +1587,6 @@
c.writeRecord(recordTypeHandshake, postCCSMsgs[0])
postCCSMsgs = postCCSMsgs[1:]
}
- c.flushHandshake()
if !c.config.Bugs.SkipChangeCipherSpec &&
c.config.Bugs.EarlyChangeCipherSpec == 0 {
@@ -1614,9 +1613,9 @@
if c.config.Bugs.SendExtraFinished {
c.writeRecord(recordTypeHandshake, finished.marshal())
}
-
- c.flushHandshake()
}
+
+ c.flushHandshake()
return nil
}
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index 3e70185..614bb50 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -1819,7 +1819,6 @@
c.writeRecord(recordTypeHandshake, postCCSBytes)
postCCSBytes = nil
}
- c.flushHandshake()
if !c.config.Bugs.SkipChangeCipherSpec {
ccs := []byte{1}
@@ -1842,11 +1841,11 @@
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
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index b5f3766..d53c041 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -10040,6 +10040,31 @@
},
})
+ // 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",