Always process handshake records in full.
This removes the last place where non-app-data hooks leave anything
uncomsumed in rrec. (There is still a place where non-app-data hooks see
a non-empty rrec an entrance. read_app_data calls into read_handshake.
That'll be fixed in a later patch in this series.)
This should not change behavior, though some error codes may change due
to some processing happening in a slightly different order.
Since we do this in a few places, this adds a BUF_MEM_append with tests.
Change-Id: I9fe1fc0103e47f90e3c9f4acfe638927aecdeff6
Reviewed-on: https://boringssl-review.googlesource.com/21345
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/d1_both.cc b/ssl/d1_both.cc
index 1110e98..3a31a02 100644
--- a/ssl/d1_both.cc
+++ b/ssl/d1_both.cc
@@ -461,7 +461,11 @@
}
}
-int dtls_has_incoming_messages(const SSL *ssl) {
+bool dtls_has_unprocessed_handshake_data(const SSL *ssl) {
+ if (ssl->d1->has_change_cipher_spec) {
+ return true;
+ }
+
size_t current = ssl->d1->handshake_read_seq % SSL_MAX_HANDSHAKE_FLIGHT;
for (size_t i = 0; i < SSL_MAX_HANDSHAKE_FLIGHT; i++) {
// Skip the current message.
@@ -470,10 +474,10 @@
continue;
}
if (ssl->d1->incoming_messages[i] != NULL) {
- return 1;
+ return true;
}
}
- return 0;
+ return false;
}
int dtls1_parse_fragment(CBS *cbs, struct hm_header_st *out_hdr,
diff --git a/ssl/dtls_method.cc b/ssl/dtls_method.cc
index f433428..daaec2d 100644
--- a/ssl/dtls_method.cc
+++ b/ssl/dtls_method.cc
@@ -83,8 +83,8 @@
}
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) || ssl->d1->has_change_cipher_spec) {
+ // Cipher changes are forbidden if the current epoch has leftover data.
+ if (dtls_has_unprocessed_handshake_data(ssl)) {
OPENSSL_PUT_ERROR(SSL, SSL_R_BUFFERED_MESSAGES_ON_CIPHER_CHANGE);
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
return 0;
diff --git a/ssl/internal.h b/ssl/internal.h
index 4c1f9ea..f141923 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1000,9 +1000,13 @@
// dtls_clear_incoming_messages releases all buffered incoming messages.
void dtls_clear_incoming_messages(SSL *ssl);
-// dtls_has_incoming_messages returns one if there are buffered incoming
-// messages ahead of the current message and zero otherwise.
-int dtls_has_incoming_messages(const SSL *ssl);
+// 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);
+
+// dtls_has_unprocessed_handshake_data behaves like
+// |tls_has_unprocessed_handshake_data| for DTLS.
+bool dtls_has_unprocessed_handshake_data(const SSL *ssl);
struct DTLS_OUTGOING_MESSAGE {
uint8_t *data;
@@ -2687,7 +2691,10 @@
int peek);
int ssl3_read_change_cipher_spec(SSL *ssl);
void ssl3_read_close_notify(SSL *ssl);
-int ssl3_read_handshake_bytes(SSL *ssl, uint8_t *buf, int len);
+// ssl3_get_record reads a new input record. On success, it places it in
+// |ssl->s3->rrec| and returns one. Otherwise it returns <= 0 on error or if
+// more data is needed.
+int ssl3_get_record(SSL *ssl);
int ssl3_write_app_data(SSL *ssl, bool *out_needs_handshake, const uint8_t *buf,
int len);
diff --git a/ssl/s3_both.cc b/ssl/s3_both.cc
index 3cc01e9..1c6882e 100644
--- a/ssl/s3_both.cc
+++ b/ssl/s3_both.cc
@@ -274,22 +274,6 @@
return 1;
}
-static int extend_handshake_buffer(SSL *ssl, size_t length) {
- if (!BUF_MEM_reserve(ssl->init_buf, length)) {
- return -1;
- }
- while (ssl->init_buf->length < length) {
- int ret = ssl3_read_handshake_bytes(
- ssl, (uint8_t *)ssl->init_buf->data + ssl->init_buf->length,
- length - ssl->init_buf->length);
- if (ret <= 0) {
- return ret;
- }
- ssl->init_buf->length += (size_t)ret;
- }
- return 1;
-}
-
static int read_v2_client_hello(SSL *ssl) {
// Read the first 5 bytes, the size of the TLS record header. This is
// sufficient to detect a V2ClientHello and ensures that we never read beyond
@@ -438,9 +422,8 @@
return 1;
}
-// TODO(davidben): Remove |out_bytes_needed| and inline into |ssl3_get_message|
-// when the entire record is copied into |init_buf|.
-static bool parse_message(SSL *ssl, SSLMessage *out, size_t *out_bytes_needed) {
+static bool parse_message(const SSL *ssl, SSLMessage *out,
+ size_t *out_bytes_needed) {
if (ssl->init_buf == NULL) {
*out_bytes_needed = 4;
return false;
@@ -481,6 +464,19 @@
return true;
}
+bool tls_has_unprocessed_handshake_data(const SSL *ssl) {
+ size_t msg_len = 0;
+ if (ssl->s3->has_message) {
+ SSLMessage msg;
+ size_t unused;
+ if (parse_message(ssl, &msg, &unused)) {
+ msg_len = CBS_len(&msg.raw);
+ }
+ }
+
+ return ssl->init_buf != NULL && ssl->init_buf->length > msg_len;
+}
+
int ssl3_read_message(SSL *ssl) {
SSLMessage msg;
size_t bytes_needed;
@@ -513,7 +509,40 @@
return ret;
}
- return extend_handshake_buffer(ssl, bytes_needed);
+ SSL3_RECORD *rr = &ssl->s3->rrec;
+ // Get new packet if necessary.
+ if (rr->length == 0) {
+ int ret = ssl3_get_record(ssl);
+ if (ret <= 0) {
+ return ret;
+ }
+ }
+
+ // WatchGuard's TLS 1.3 interference bug is very distinctive: they drop the
+ // ServerHello and send the remaining encrypted application data records
+ // as-is. This manifests as an application data record when we expect
+ // handshake. Report a dedicated error code for this case.
+ if (!ssl->server && rr->type == SSL3_RT_APPLICATION_DATA &&
+ ssl->s3->aead_read_ctx->is_null_cipher()) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_APPLICATION_DATA_INSTEAD_OF_HANDSHAKE);
+ ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
+ return -1;
+ }
+
+ if (rr->type != SSL3_RT_HANDSHAKE) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD);
+ ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
+ return -1;
+ }
+
+ // Append the entire handshake record to the buffer.
+ if (!BUF_MEM_append(ssl->init_buf, rr->data, rr->length)) {
+ return -1;
+ }
+
+ rr->length = 0;
+ ssl_read_buffer_discard(ssl);
+ return 1;
}
void ssl3_next_message(SSL *ssl) {
diff --git a/ssl/s3_pkt.cc b/ssl/s3_pkt.cc
index 890e8b7..b182826 100644
--- a/ssl/s3_pkt.cc
+++ b/ssl/s3_pkt.cc
@@ -126,10 +126,7 @@
static int do_ssl3_write(SSL *ssl, int type, const uint8_t *buf, unsigned len);
-// ssl3_get_record reads a new input record. On success, it places it in
-// |ssl->s3->rrec| and returns one. Otherwise it returns <= 0 on error or if
-// more data is needed.
-static int ssl3_get_record(SSL *ssl) {
+int ssl3_get_record(SSL *ssl) {
again:
switch (ssl->s3->read_shutdown) {
case ssl_shutdown_none:
@@ -462,7 +459,6 @@
int ssl3_read_change_cipher_spec(SSL *ssl) {
SSL3_RECORD *rr = &ssl->s3->rrec;
-
if (rr->length == 0) {
int ret = ssl3_get_record(ssl);
if (ret <= 0) {
@@ -470,7 +466,8 @@
}
}
- if (rr->type != SSL3_RT_CHANGE_CIPHER_SPEC) {
+ if (rr->type != SSL3_RT_CHANGE_CIPHER_SPEC ||
+ tls_has_unprocessed_handshake_data(ssl)) {
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD);
return -1;
@@ -497,43 +494,6 @@
}
}
-int ssl3_read_handshake_bytes(SSL *ssl, uint8_t *buf, int len) {
- SSL3_RECORD *rr = &ssl->s3->rrec;
-
- for (;;) {
- // Get new packet if necessary.
- if (rr->length == 0) {
- int ret = ssl3_get_record(ssl);
- if (ret <= 0) {
- return ret;
- }
- }
-
- // WatchGuard's TLS 1.3 interference bug is very distinctive: they drop the
- // ServerHello and send the remaining encrypted application data records
- // as-is. This manifests as an application data record when we expect
- // handshake. Report a dedicated error code for this case.
- if (!ssl->server && rr->type == SSL3_RT_APPLICATION_DATA &&
- ssl->s3->aead_read_ctx->is_null_cipher()) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_APPLICATION_DATA_INSTEAD_OF_HANDSHAKE);
- ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
- return -1;
- }
-
- if (rr->type != SSL3_RT_HANDSHAKE) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_RECORD);
- ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
- return -1;
- }
-
- if (rr->length != 0) {
- return consume_record(ssl, buf, len, 0 /* consume data */);
- }
-
- // Discard empty records and loop again.
- }
-}
-
int ssl_send_alert(SSL *ssl, int level, int desc) {
// It is illegal to send an alert when we've already sent a closing one.
if (ssl->s3->write_shutdown != ssl_shutdown_none) {
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index 88161ad..c975337 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -924,28 +924,27 @@
}
}
- bool got_handshake = false;
- int ret = ssl->method->read_app_data(ssl, &got_handshake, (uint8_t *)buf,
- num, peek);
- if (ret > 0 || !got_handshake) {
- ssl->s3->key_update_count = 0;
- return ret;
- }
-
- // If we received an interrupt in early read (the end_of_early_data alert),
- // loop again for the handshake to process it.
- if (SSL_in_init(ssl)) {
- continue;
- }
-
+ // Process any buffered post-handshake messages.
SSLMessage msg;
- while (ssl->method->get_message(ssl, &msg)) {
+ if (ssl->method->get_message(ssl, &msg)) {
// Handle the post-handshake message and try again.
if (!ssl_do_post_handshake(ssl, msg)) {
return -1;
}
ssl->method->next_message(ssl);
+ continue; // Loop again. We may have begun a new handshake.
}
+
+ bool got_handshake = false;
+ int ret = ssl->method->read_app_data(ssl, &got_handshake, (uint8_t *)buf,
+ num, peek);
+ if (got_handshake) {
+ continue; // Loop again to process the handshake data.
+ }
+ if (ret > 0) {
+ ssl->s3->key_update_count = 0;
+ }
+ return ret;
}
}
diff --git a/ssl/ssl_transcript.cc b/ssl/ssl_transcript.cc
index 8bddbe2..ab9822f 100644
--- a/ssl/ssl_transcript.cc
+++ b/ssl/ssl_transcript.cc
@@ -212,16 +212,9 @@
bool SSLTranscript::Update(Span<const uint8_t> in) {
// Depending on the state of the handshake, either the handshake buffer may be
// active, the rolling hash, or both.
- if (buffer_) {
- size_t new_len = buffer_->length + in.size();
- if (new_len < in.size()) {
- OPENSSL_PUT_ERROR(SSL, ERR_R_OVERFLOW);
- return false;
- }
- if (!BUF_MEM_grow(buffer_.get(), new_len)) {
- return false;
- }
- OPENSSL_memcpy(buffer_->data + new_len - in.size(), in.data(), in.size());
+ if (buffer_ &&
+ !BUF_MEM_append(buffer_.get(), in.data(), in.size())) {
+ return false;
}
if (EVP_MD_CTX_md(hash_.get()) != NULL) {
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 9ec0d80..a57362d 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2491,7 +2491,7 @@
"-expect-total-renegotiations", "1",
},
shouldFail: true,
- expectedError: ":EXCESSIVE_MESSAGE_SIZE:",
+ expectedError: ":BAD_HELLO_REQUEST:",
},
{
name: "BadHelloRequest-2",
diff --git a/ssl/tls_method.cc b/ssl/tls_method.cc
index 77f9d23..b4d08a0 100644
--- a/ssl/tls_method.cc
+++ b/ssl/tls_method.cc
@@ -74,11 +74,10 @@
assert(!ssl->s3->has_message);
// During the handshake, |init_buf| is retained. Release if it there is no
- // excess in it.
+ // excess in it. There may be excess left if there server sent Finished and
+ // HelloRequest in the same record.
//
- // TODO(davidben): The second check is always true but will not be once we
- // switch to copying the entire handshake record. Replace this comment with an
- // explanation when that happens and a TODO to reject it.
+ // TODO(davidben): SChannel does not support this. Reject this case.
if (ssl->init_buf != NULL && ssl->init_buf->length == 0) {
BUF_MEM_free(ssl->init_buf);
ssl->init_buf = NULL;
@@ -86,8 +85,11 @@
}
static int ssl3_set_read_state(SSL *ssl, UniquePtr<SSLAEADContext> aead_ctx) {
- if (ssl->s3->rrec.length != 0) {
- // There may not be unprocessed record data at a cipher change.
+ // Cipher changes are forbidden if the current epoch has leftover data.
+ //
+ // TODO(davidben): ssl->s3->rrec.length should be impossible now. Remove it
+ // once it is only used for application data.
+ if (ssl->s3->rrec.length != 0 || tls_has_unprocessed_handshake_data(ssl)) {
OPENSSL_PUT_ERROR(SSL, SSL_R_BUFFERED_MESSAGES_ON_CIPHER_CHANGE);
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_UNEXPECTED_MESSAGE);
return 0;
