Improve handling of DTLS 1.3 post-handshake messages. Currently, if we receive a post-handshake message in DTLS 1.3, we treat it as an error. This changes the behavior to ignore the messages and provides an entry point for processing them. Bug: 42290594 Change-Id: I3c4900e4b2d2d0b43033cb7b67f8568cfdfb80ff Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72047 Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: Nick Harper <nharper@chromium.org>
diff --git a/ssl/d1_both.cc b/ssl/d1_both.cc index 016e3a2..80da717 100644 --- a/ssl/d1_both.cc +++ b/ssl/d1_both.cc
@@ -289,6 +289,66 @@ return ssl->d1->incoming_messages[idx].get(); } +bool dtls1_process_handshake_fragments(SSL *ssl, uint8_t *out_alert, + Span<uint8_t> record) { + CBS cbs; + CBS_init(&cbs, record.data(), record.size()); + while (CBS_len(&cbs) > 0) { + // Read a handshake fragment. + struct hm_header_st msg_hdr; + CBS body; + if (!dtls1_parse_fragment(&cbs, &msg_hdr, &body)) { + OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_HANDSHAKE_RECORD); + *out_alert = SSL_AD_DECODE_ERROR; + return false; + } + + const size_t frag_off = msg_hdr.frag_off; + const size_t frag_len = msg_hdr.frag_len; + const size_t msg_len = msg_hdr.msg_len; + if (frag_off > msg_len || frag_off + frag_len < frag_off || + frag_off + frag_len > msg_len || + msg_len > ssl_max_handshake_message_len(ssl)) { + OPENSSL_PUT_ERROR(SSL, SSL_R_EXCESSIVE_MESSAGE_SIZE); + *out_alert = SSL_AD_ILLEGAL_PARAMETER; + return false; + } + + // The encrypted epoch in DTLS has only one handshake message. + if (ssl->d1->r_epoch == 1 && msg_hdr.seq != ssl->d1->handshake_read_seq) { + OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD); + *out_alert = SSL_AD_UNEXPECTED_MESSAGE; + return false; + } + + if (msg_hdr.seq < ssl->d1->handshake_read_seq || + msg_hdr.seq > + (unsigned)ssl->d1->handshake_read_seq + SSL_MAX_HANDSHAKE_FLIGHT) { + // Ignore fragments from the past, or ones too far in the future. + continue; + } + + hm_fragment *frag = dtls1_get_incoming_message(ssl, out_alert, &msg_hdr); + if (frag == NULL) { + return false; + } + assert(frag->msg_len == msg_len); + + if (frag->reassembly == NULL) { + // The message is already assembled. + continue; + } + assert(msg_len > 0); + + // Copy the body into the fragment. + OPENSSL_memcpy(frag->data + DTLS1_HM_HEADER_LENGTH + frag_off, + CBS_data(&body), CBS_len(&body)); + dtls1_hm_fragment_mark(frag, frag_off, frag_off + frag_len); + } + + return true; +} + ssl_open_record_t dtls1_open_handshake(SSL *ssl, size_t *out_consumed, uint8_t *out_alert, Span<uint8_t> in) { uint8_t type; @@ -342,61 +402,9 @@ return ssl_open_record_error; } - CBS cbs; - CBS_init(&cbs, record.data(), record.size()); - while (CBS_len(&cbs) > 0) { - // Read a handshake fragment. - struct hm_header_st msg_hdr; - CBS body; - if (!dtls1_parse_fragment(&cbs, &msg_hdr, &body)) { - OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_HANDSHAKE_RECORD); - *out_alert = SSL_AD_DECODE_ERROR; - return ssl_open_record_error; - } - - const size_t frag_off = msg_hdr.frag_off; - const size_t frag_len = msg_hdr.frag_len; - const size_t msg_len = msg_hdr.msg_len; - if (frag_off > msg_len || frag_off + frag_len < frag_off || - frag_off + frag_len > msg_len || - msg_len > ssl_max_handshake_message_len(ssl)) { - OPENSSL_PUT_ERROR(SSL, SSL_R_EXCESSIVE_MESSAGE_SIZE); - *out_alert = SSL_AD_ILLEGAL_PARAMETER; - return ssl_open_record_error; - } - - // The encrypted epoch in DTLS has only one handshake message. - if (ssl->d1->r_epoch == 1 && msg_hdr.seq != ssl->d1->handshake_read_seq) { - OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD); - *out_alert = SSL_AD_UNEXPECTED_MESSAGE; - return ssl_open_record_error; - } - - if (msg_hdr.seq < ssl->d1->handshake_read_seq || - msg_hdr.seq > - (unsigned)ssl->d1->handshake_read_seq + SSL_MAX_HANDSHAKE_FLIGHT) { - // Ignore fragments from the past, or ones too far in the future. - continue; - } - - hm_fragment *frag = dtls1_get_incoming_message(ssl, out_alert, &msg_hdr); - if (frag == NULL) { - return ssl_open_record_error; - } - assert(frag->msg_len == msg_len); - - if (frag->reassembly == NULL) { - // The message is already assembled. - continue; - } - assert(msg_len > 0); - - // Copy the body into the fragment. - OPENSSL_memcpy(frag->data + DTLS1_HM_HEADER_LENGTH + frag_off, - CBS_data(&body), CBS_len(&body)); - dtls1_hm_fragment_mark(frag, frag_off, frag_off + frag_len); + if (!dtls1_process_handshake_fragments(ssl, out_alert, record)) { + return ssl_open_record_error; } - return ssl_open_record_success; }
diff --git a/ssl/d1_pkt.cc b/ssl/d1_pkt.cc index 9fead5c..13da69a 100644 --- a/ssl/d1_pkt.cc +++ b/ssl/d1_pkt.cc
@@ -140,6 +140,14 @@ } if (type == SSL3_RT_HANDSHAKE) { + // Process handshake fragments for DTLS 1.3 post-handshake messages. + if (ssl_protocol_version(ssl) >= TLS1_3_VERSION) { + if (!dtls1_process_handshake_fragments(ssl, out_alert, record)) { + return ssl_open_record_error; + } + return ssl_open_record_discard; + } + // Parse the first fragment header to determine if this is a pre-CCS or // post-CCS handshake record. DTLS resets handshake message numbers on each // handshake, so renegotiations and retransmissions are ambiguous.
diff --git a/ssl/internal.h b/ssl/internal.h index 092b298..b3b1f67 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -3691,6 +3691,8 @@ bool dtls1_new(SSL *ssl); void dtls1_free(SSL *ssl); +bool dtls1_process_handshake_fragments(SSL *ssl, uint8_t *out_alert, + Span<uint8_t> record); bool dtls1_get_message(const SSL *ssl, SSLMessage *out); ssl_open_record_t dtls1_open_handshake(SSL *ssl, size_t *out_consumed, uint8_t *out_alert, Span<uint8_t> in);
diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go index c019645..8de4b17 100644 --- a/ssl/test/runner/conn.go +++ b/ssl/test/runner/conn.go
@@ -2050,7 +2050,12 @@ if err := c.flushHandshake(); err != nil { return err } - c.useOutTrafficSecret(encryptionApplication, c.out.wireVersion, c.cipherSuite, updateTrafficSecret(c.cipherSuite.hash(), c.wireVersion, c.out.trafficSecret, c.isDTLS)) + if !c.isDTLS { + // TODO(crbug.com/42290594): Properly implement KeyUpdate. Right + // now we only support sending KeyUpdate to test that we drop + // post-HS messages on the floor (instead of erroring). + c.useOutTrafficSecret(encryptionApplication, c.out.wireVersion, c.cipherSuite, updateTrafficSecret(c.cipherSuite.hash(), c.wireVersion, c.out.trafficSecret, c.isDTLS)) + } return nil }
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index e598ac7..72937af 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go
@@ -3322,6 +3322,25 @@ keyUpdateRequest: keyUpdateNotRequested, }, { + protocol: dtls, + name: "KeyUpdate-ToClient-DTLS", + config: Config{ + MaxVersion: VersionTLS13, + }, + sendKeyUpdates: 1, + keyUpdateRequest: keyUpdateNotRequested, + }, + { + protocol: dtls, + testType: serverTest, + name: "KeyUpdate-ToServerDTLS", + config: Config{ + MaxVersion: VersionTLS13, + }, + sendKeyUpdates: 1, + keyUpdateRequest: keyUpdateNotRequested, + }, + { name: "KeyUpdate-FromClient", config: Config{ MaxVersion: VersionTLS13,
diff --git a/ssl/tls13_both.cc b/ssl/tls13_both.cc index 4386a6f..67c7b42 100644 --- a/ssl/tls13_both.cc +++ b/ssl/tls13_both.cc
@@ -664,6 +664,10 @@ } bool tls13_post_handshake(SSL *ssl, const SSLMessage &msg) { + if (SSL_is_dtls(ssl)) { + // TODO(crbug.com/42290594): Process post-handshake messages in DTLS 1.3. + return true; + } if (msg.type == SSL3_MT_KEY_UPDATE) { ssl->s3->key_update_count++; if (ssl->quic_method != nullptr ||