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;