Require handshake flights end at record boundaries.
The TLS handshake runs over a sequence of messages, serialized onto a
stream, which is then packetized into records, with no correlation to
message boundaries.
TLS messages may span records, so a TLS implementation will buffer up
excess data in a record for the next message. If not checked, that next
message may a round-trip or even a key change later. Carrying data
across a key change has security consequences, so we reject any excess
data across key changes (see ChangeCipherSpec synchronization tests and
(d)tls_set_read_state). However, we do not currently check it across
network round trips that do not change keys.
For instance, a TLS 1.2 client may pack part of ClientKeyExchange (the
first byte, at least, is deterministic) into the ClientHello record,
before even receiving ServerHello. Most TLS implementations will accept
this.
However, the handback logic does *not* serialize excess data in hs_buf.
There shouldn't be any, but if the peer is doing strange things as
above, that data will get silently dropped. The way TLS 1.3 0-RTT
handback logic works (the key isn't installed until after handback),
this data is even silently dropped though there is a key change.
To keep all our behavior consistent, check for unprocessed handshake
data at the end of each flight and reject it. Add a bunch of tests.
Update-Note: If the peer packs data across handshake flights, or packs
HelloRequest into the same record as Finished, this will now be an
error. (The former is pathologically odd behavior. The latter is also
rejected by Schannel and also odd.)
Change-Id: I9412bbdea09ee7fdcfeb78d3456329505a190641
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/39987
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc
index 8bb3339..f48d1da 100644
--- a/ssl/tls13_client.cc
+++ b/ssl/tls13_client.cc
@@ -183,6 +183,13 @@
return ssl_hs_error;
}
+ // HelloRetryRequest should be the end of the flight.
+ if (ssl->method->has_unprocessed_handshake_data(ssl)) {
+ ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
+ OPENSSL_PUT_ERROR(SSL, SSL_R_EXCESS_HANDSHAKE_DATA);
+ return ssl_hs_error;
+ }
+
ssl->method->next_message(ssl);
ssl->s3->used_hello_retry_request = true;
hs->tls13_state = state_send_second_client_hello;
@@ -622,6 +629,13 @@
return ssl_hs_error;
}
+ // Finished should be the end of the flight.
+ if (ssl->method->has_unprocessed_handshake_data(ssl)) {
+ ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
+ OPENSSL_PUT_ERROR(SSL, SSL_R_EXCESS_HANDSHAKE_DATA);
+ return ssl_hs_error;
+ }
+
ssl->method->next_message(ssl);
hs->tls13_state = state_send_end_of_early_data;
return ssl_hs_ok;
