kris/boringssl
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Always process handshake records in full.

Browse Source 
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>
This commit is contained in:
David Benjamin 2017-10-06 18:26:36 -04:00 committed by CQ bot account: commit-bot@chromium.org
parent f66e88228a
commit 40e94701dc
13 changed files with 214 additions and 104 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
crypto/CMakeLists.txt
View File

@ -220,6 +220,7 @@ add_executable(
asn1/asn1_test.cc
base64/base64_test.cc
buf/buf_test.cc
bio/bio_test.cc
bytestring/bytestring_test.cc
chacha/chacha_test.cc

14
crypto/buf/buf.c
View File

@ -131,6 +131,20 @@ size_t BUF_MEM_grow_clean(BUF_MEM *buf, size_t len) {
return BUF_MEM_grow(buf, len);
}
int BUF_MEM_append(BUF_MEM *buf, const void *in, size_t len) {
size_t new_len = buf->length + len;
if (new_len < len) {
OPENSSL_PUT_ERROR(BUF, ERR_R_OVERFLOW);
return 0;
}
if (!BUF_MEM_reserve(buf, new_len)) {
return 0;
}
OPENSSL_memcpy(buf->data + buf->length, in, len);
buf->length = new_len;
return 1;
}
char *BUF_strdup(const char *str) {
if (str == NULL) {
return NULL;

97
crypto/buf/buf_test.cc Normal file
View File

@ -0,0 +1,97 @@
/* Copyright (c) 2017, Google Inc.
*
* Permission to use, copy, modify, and/or distribute this software for any
* purpose with or without fee is hereby granted, provided that the above
* copyright notice and this permission notice appear in all copies.
*
* THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
* WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
* MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY
* SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
* WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION
* OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
* CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */
#include <openssl/buf.h>
#include <string.h>
#include <string>
#include <gtest/gtest.h>
TEST(BufTest, Basic) {
bssl::UniquePtr<BUF_MEM> buf(BUF_MEM_new());
ASSERT_TRUE(buf);
EXPECT_EQ(0u, buf->length);
// Use BUF_MEM_reserve to increase buf->max.
ASSERT_TRUE(BUF_MEM_reserve(buf.get(), 200));
EXPECT_GE(buf->max, 200u);
EXPECT_EQ(0u, buf->length);
// BUF_MEM_reserve with a smaller cap is a no-op.
size_t old_max = buf->max;
ASSERT_TRUE(BUF_MEM_reserve(buf.get(), 100));
EXPECT_EQ(old_max, buf->max);
EXPECT_EQ(0u, buf->length);
// BUF_MEM_grow can increase the length without reallocating.
ASSERT_EQ(100u, BUF_MEM_grow(buf.get(), 100));
EXPECT_EQ(100u, buf->length);
EXPECT_EQ(old_max, buf->max);
memset(buf->data, 'A', buf->length);
// If BUF_MEM_reserve reallocates, it preserves the contents.
ASSERT_TRUE(BUF_MEM_reserve(buf.get(), old_max + 1));
ASSERT_GE(buf->max, old_max + 1);
EXPECT_EQ(100u, buf->length);
for (size_t i = 0; i < 100; i++) {
EXPECT_EQ('A', buf->data[i]);
}
// BUF_MEM_grow should zero everything beyond buf->length.
memset(buf->data, 'B', buf->max);
ASSERT_EQ(150u, BUF_MEM_grow(buf.get(), 150));
EXPECT_EQ(150u, buf->length);
for (size_t i = 0; i < 100; i++) {
EXPECT_EQ('B', buf->data[i]);
}
for (size_t i = 100; i < 150; i++) {
EXPECT_EQ(0, buf->data[i]);
}
// BUF_MEM_grow can rellocate if necessary.
size_t new_len = buf->max + 1;
ASSERT_EQ(new_len, BUF_MEM_grow(buf.get(), new_len));
EXPECT_GE(buf->max, new_len);
EXPECT_EQ(new_len, buf->length);
for (size_t i = 0; i < 100; i++) {
EXPECT_EQ('B', buf->data[i]);
}
for (size_t i = 100; i < new_len; i++) {
EXPECT_EQ(0, buf->data[i]);
}
// BUF_MEM_grow can shink.
ASSERT_EQ(50u, BUF_MEM_grow(buf.get(), 50));
EXPECT_EQ(50u, buf->length);
for (size_t i = 0; i < 50; i++) {
EXPECT_EQ('B', buf->data[i]);
}
}
TEST(BufTest, Append) {
bssl::UniquePtr<BUF_MEM> buf(BUF_MEM_new());
ASSERT_TRUE(buf);
ASSERT_TRUE(BUF_MEM_append(buf.get(), nullptr, 0));
ASSERT_TRUE(BUF_MEM_append(buf.get(), "hello ", 6));
ASSERT_TRUE(BUF_MEM_append(buf.get(), nullptr, 0));
ASSERT_TRUE(BUF_MEM_append(buf.get(), "world", 5));
std::string str(128, 'A');
ASSERT_TRUE(BUF_MEM_append(buf.get(), str.data(), str.size()));
EXPECT_EQ("hello world" + str, std::string(buf->data, buf->length));
}

4
include/openssl/buf.h
View File

@ -93,6 +93,10 @@ OPENSSL_EXPORT size_t BUF_MEM_grow(BUF_MEM *buf, size_t len);
// allocated memory on free.
OPENSSL_EXPORT size_t BUF_MEM_grow_clean(BUF_MEM *buf, size_t len);
// BUF_MEM_append appends |in| to |buf|. It returns one on success and zero on
// error.
OPENSSL_EXPORT int BUF_MEM_append(BUF_MEM *buf, const void *in, size_t len);
// BUF_strdup returns an allocated, duplicate of |str|.
OPENSSL_EXPORT char *BUF_strdup(const char *str);

10
ssl/d1_both.cc
View File

@ -461,7 +461,11 @@ void dtls_clear_incoming_messages(SSL *ssl) {
}
}
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 @@ int dtls_has_incoming_messages(const SSL *ssl) {
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,

4
ssl/dtls_method.cc
View File

@ -83,8 +83,8 @@ static void dtls1_on_handshake_complete(SSL *ssl) {
}
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;

15
ssl/internal.h
View File

@ -1000,9 +1000,13 @@ size_t ssl_max_handshake_message_len(const SSL *ssl);
// 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 ssl3_read_app_data(SSL *ssl, bool *out_got_handshake, uint8_t *buf, int len,
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);

69
ssl/s3_both.cc
View File

@ -274,22 +274,6 @@ int ssl3_flush_flight(SSL *ssl) {
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 @@ static int read_v2_client_hello(SSL *ssl) {
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 @@ bool ssl3_get_message(SSL *ssl, SSLMessage *out) {
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 @@ int ssl3_read_message(SSL *ssl) {
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) {

46
ssl/s3_pkt.cc
View File

@ -126,10 +126,7 @@ namespace bssl {
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_app_data(SSL *ssl, bool *out_got_handshake, uint8_t *buf, int len,
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 @@ int ssl3_read_change_cipher_spec(SSL *ssl) {
}
}
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 @@ void ssl3_read_close_notify(SSL *ssl) {
}
}
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) {

29
ssl/ssl_lib.cc
View File

@ -924,28 +924,27 @@ static int ssl_read_impl(SSL *ssl, void *buf, int num, int peek) {
}
}
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;
}
}

13
ssl/ssl_transcript.cc
View File

@ -212,16 +212,9 @@ const EVP_MD *SSLTranscript::Digest() const {
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) {

2
ssl/test/runner/runner.go
View File

@ -2491,7 +2491,7 @@ read alert 1 0
"-expect-total-renegotiations", "1",
},
shouldFail: true,
expectedError: ":EXCESSIVE_MESSAGE_SIZE:",
expectedError: ":BAD_HELLO_REQUEST:",
},
{
name: "BadHelloRequest-2",

14
ssl/tls_method.cc
View File

@ -74,11 +74,10 @@ static void ssl3_on_handshake_complete(SSL *ssl) {
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 void ssl3_on_handshake_complete(SSL *ssl) {
}
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;