Don't call ssl3_read_message from ssl3_read_app_data.
With this change, it should now always be the case that rr->length is
zero on entry to ssl3_read_message. This will let us detach everything
but application data from rr. This pushes some init_buf invariants down
into tls_open_record so we don't need to maintain them everywhere.
Change-Id: I206747434e0a9603eea7d19664734fd16fa2de8e
Reviewed-on: https://boringssl-review.googlesource.com/21524
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
diff --git a/ssl/internal.h b/ssl/internal.h
index 577b382..60e69f9 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1001,6 +1001,10 @@
// dtls_clear_incoming_messages releases all buffered incoming messages.
void dtls_clear_incoming_messages(SSL *ssl);
+// tls_can_accept_handshake_data returns whether |ssl| is able to accept more
+// data into handshake buffer.
+bool tls_can_accept_handshake_data(const SSL *ssl, uint8_t *out_alert);
+
// tls_has_unprocessed_handshake_data returns whether there is buffered
// handshake data that has not been consumed by |get_message|.
bool tls_has_unprocessed_handshake_data(const SSL *ssl);
diff --git a/ssl/s3_both.cc b/ssl/s3_both.cc
index 376018d..7fc843e 100644
--- a/ssl/s3_both.cc
+++ b/ssl/s3_both.cc
@@ -464,6 +464,26 @@
return true;
}
+bool tls_can_accept_handshake_data(const SSL *ssl, uint8_t *out_alert) {
+ // If there is a complete message, the caller must have consumed it first.
+ SSLMessage msg;
+ size_t bytes_needed;
+ if (parse_message(ssl, &msg, &bytes_needed)) {
+ OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
+ *out_alert = SSL_AD_INTERNAL_ERROR;
+ return false;
+ }
+
+ // Enforce the limit so the peer cannot force us to buffer 16MB.
+ if (bytes_needed > 4 + ssl_max_handshake_message_len(ssl)) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_EXCESSIVE_MESSAGE_SIZE);
+ *out_alert = SSL_AD_ILLEGAL_PARAMETER;
+ return false;
+ }
+
+ return true;
+}
+
bool tls_has_unprocessed_handshake_data(const SSL *ssl) {
size_t msg_len = 0;
if (ssl->s3->has_message) {
@@ -478,20 +498,6 @@
}
int ssl3_read_message(SSL *ssl) {
- SSLMessage msg;
- size_t bytes_needed;
- if (parse_message(ssl, &msg, &bytes_needed)) {
- OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
- return -1;
- }
-
- // Enforce the limit so the peer cannot force us to buffer 16MB.
- if (bytes_needed > 4 + ssl_max_handshake_message_len(ssl)) {
- ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
- OPENSSL_PUT_ERROR(SSL, SSL_R_EXCESSIVE_MESSAGE_SIZE);
- return -1;
- }
-
// Re-create the handshake buffer if needed.
if (ssl->init_buf == NULL) {
ssl->init_buf = BUF_MEM_new();
diff --git a/ssl/s3_pkt.cc b/ssl/s3_pkt.cc
index 0b3331c..2718fde 100644
--- a/ssl/s3_pkt.cc
+++ b/ssl/s3_pkt.cc
@@ -365,22 +365,20 @@
SSL3_RECORD *rr = &ssl->s3->rrec;
for (;;) {
- // A previous iteration may have read a partial handshake message. Do not
- // allow more app data in that case.
- int has_hs_data = ssl->init_buf != NULL && ssl->init_buf->length > 0;
-
// Get new packet if necessary.
- if (rr->length == 0 && !has_hs_data) {
+ if (rr->length == 0) {
int ret = ssl3_get_record(ssl);
if (ret <= 0) {
return ret;
}
}
- if (has_hs_data || rr->type == SSL3_RT_HANDSHAKE) {
+ const bool is_early_data_read = ssl->server && SSL_in_early_data(ssl);
+
+ if (rr->type == SSL3_RT_HANDSHAKE) {
// If reading 0-RTT data, reject handshake data. 0-RTT data is terminated
// by an alert.
- if (SSL_in_init(ssl)) {
+ if (is_early_data_read) {
OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD);
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
return -1;
@@ -395,20 +393,19 @@
return -1;
}
- // Parse post-handshake handshake messages.
- int ret = ssl3_read_message(ssl);
- if (ret <= 0) {
- return ret;
+ if (ssl->init_buf == NULL) {
+ ssl->init_buf = BUF_MEM_new();
+ }
+ if (ssl->init_buf == NULL ||
+ !BUF_MEM_append(ssl->init_buf, rr->data, rr->length)) {
+ return -1;
}
*out_got_handshake = true;
+ rr->length = 0;
+ ssl_read_buffer_discard(ssl);
return -1;
}
- const int is_early_data_read = ssl->server &&
- ssl->s3->hs != NULL &&
- ssl->s3->hs->can_early_read &&
- ssl_protocol_version(ssl) >= TLS1_3_VERSION;
-
// Handle the end_of_early_data alert.
if (rr->type == SSL3_RT_ALERT &&
rr->length == 2 &&
@@ -457,8 +454,7 @@
}
}
- if (rr->type != SSL3_RT_CHANGE_CIPHER_SPEC ||
- tls_has_unprocessed_handshake_data(ssl)) {
+ if (rr->type != SSL3_RT_CHANGE_CIPHER_SPEC) {
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD);
return -1;
diff --git a/ssl/tls_record.cc b/ssl/tls_record.cc
index 12597a6..2a28859 100644
--- a/ssl/tls_record.cc
+++ b/ssl/tls_record.cc
@@ -192,6 +192,12 @@
size_t *out_consumed,
uint8_t *out_alert,
Span<uint8_t> in) {
+ // If there is an unprocessed handshake message or we are already buffering
+ // too much, stop before decrypting another handshake record.
+ if (!tls_can_accept_handshake_data(ssl, out_alert)) {
+ return ssl_open_record_error;
+ }
+
CBS cbs = CBS(in);
// Decode the record header.
@@ -321,6 +327,14 @@
return ssl_process_alert(ssl, out_alert, *out);
}
+ // Handshake messages may not interleave with any other record type.
+ if (type != SSL3_RT_HANDSHAKE &&
+ tls_has_unprocessed_handshake_data(ssl)) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD);
+ *out_alert = SSL_AD_UNEXPECTED_MESSAGE;
+ return ssl_open_record_error;
+ }
+
ssl->s3->warning_alert_count = 0;
*out_type = type;
