Make all read errors idempotent.
Now that we've gotten everything, test this by just making bssl_shim run
all errors twice. The manual tests added to ssl_test.cc may now be
removed.
Bug: 206
Change-Id: Iefa0eae83ba59b476e6b6c6f0f921d5d1b72cbfb
Reviewed-on: https://boringssl-review.googlesource.com/21886
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/dtls_record.cc b/ssl/dtls_record.cc
index eccc66b..a746640 100644
--- a/ssl/dtls_record.cc
+++ b/ssl/dtls_record.cc
@@ -174,11 +174,15 @@
}
}
-static enum ssl_open_record_t do_dtls_open_record(SSL *ssl, uint8_t *out_type,
- Span<uint8_t> *out,
- size_t *out_consumed,
- uint8_t *out_alert,
- Span<uint8_t> in) {
+enum ssl_open_record_t dtls_open_record(SSL *ssl, uint8_t *out_type,
+ Span<uint8_t> *out,
+ size_t *out_consumed,
+ uint8_t *out_alert, Span<uint8_t> in) {
+ *out_consumed = 0;
+ if (ssl->s3->read_shutdown == ssl_shutdown_close_notify) {
+ return ssl_open_record_close_notify;
+ }
+
if (in.empty()) {
return ssl_open_record_partial;
}
@@ -267,30 +271,6 @@
return ssl_open_record_success;
}
-enum ssl_open_record_t dtls_open_record(SSL *ssl, uint8_t *out_type,
- Span<uint8_t> *out,
- size_t *out_consumed,
- uint8_t *out_alert, Span<uint8_t> in) {
- *out_consumed = 0;
- switch (ssl->s3->read_shutdown) {
- case ssl_shutdown_none:
- break;
- case ssl_shutdown_error:
- ERR_restore_state(ssl->s3->read_error);
- *out_alert = 0;
- return ssl_open_record_error;
- case ssl_shutdown_close_notify:
- return ssl_open_record_close_notify;
- }
-
- enum ssl_open_record_t ret =
- do_dtls_open_record(ssl, out_type, out, out_consumed, out_alert, in);
- if (ret == ssl_open_record_error) {
- ssl_set_read_error(ssl);
- }
- return ret;
-}
-
static const SSLAEADContext *get_write_aead(const SSL *ssl,
enum dtls1_use_epoch_t use_epoch) {
if (use_epoch == dtls1_use_previous_epoch) {
diff --git a/ssl/handshake.cc b/ssl/handshake.cc
index a318ec3..8531ca4 100644
--- a/ssl/handshake.cc
+++ b/ssl/handshake.cc
@@ -504,11 +504,11 @@
size_t consumed = 0;
ssl_open_record_t ret;
if (hs->wait == ssl_hs_read_change_cipher_spec) {
- ret = ssl->method->open_change_cipher_spec(ssl, &consumed, &alert,
- ssl_read_buffer(ssl));
- } else {
- ret = ssl->method->open_handshake(ssl, &consumed, &alert,
+ ret = ssl_open_change_cipher_spec(ssl, &consumed, &alert,
ssl_read_buffer(ssl));
+ } else {
+ ret =
+ ssl_open_handshake(ssl, &consumed, &alert, ssl_read_buffer(ssl));
}
if (ret == ssl_open_record_error &&
hs->wait == ssl_hs_read_server_hello) {
diff --git a/ssl/internal.h b/ssl/internal.h
index 7801cc5..174445a 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1736,21 +1736,14 @@
bool (*get_message)(SSL *ssl, SSLMessage *out);
// next_message is called to release the current handshake message.
void (*next_message)(SSL *ssl);
- // open_handshake processes a record from |in| for reading a handshake
- // message.
+ // Use the |ssl_open_handshake| wrapper.
ssl_open_record_t (*open_handshake)(SSL *ssl, size_t *out_consumed,
uint8_t *out_alert, Span<uint8_t> in);
- // open_change_cipher_spec processes a record from |in| for reading a
- // ChangeCipherSpec. If an out-of-order record was received in DTLS, it
- // succeeds without consuming input.
+ // Use the |ssl_open_change_cipher_spec| wrapper.
ssl_open_record_t (*open_change_cipher_spec)(SSL *ssl, size_t *out_consumed,
uint8_t *out_alert,
Span<uint8_t> in);
- // open_app_data processes a record from |in| for reading application data.
- // On success, it returns |ssl_open_record_success| and sets |*out| to the
- // input. If it encounters a post-handshake message, it returns
- // |ssl_open_record_discard|. The caller should then retry, after processing
- // any messages received with |get_message|.
+ // Use the |ssl_open_app_data| wrapper.
ssl_open_record_t (*open_app_data)(SSL *ssl, Span<uint8_t> *out,
size_t *out_consumed, uint8_t *out_alert,
Span<uint8_t> in);
@@ -1790,6 +1783,28 @@
bool (*set_write_state)(SSL *ssl, UniquePtr<SSLAEADContext> aead_ctx);
};
+// The following wrappers call |open_*| but handle |read_shutdown| correctly.
+
+// ssl_open_handshake processes a record from |in| for reading a handshake
+// message.
+ssl_open_record_t ssl_open_handshake(SSL *ssl, size_t *out_consumed,
+ uint8_t *out_alert, Span<uint8_t> in);
+
+// ssl_open_change_cipher_spec processes a record from |in| for reading a
+// ChangeCipherSpec.
+ssl_open_record_t ssl_open_change_cipher_spec(SSL *ssl, size_t *out_consumed,
+ uint8_t *out_alert,
+ Span<uint8_t> in);
+
+// ssl_open_app_data processes a record from |in| for reading application data.
+// On success, it returns |ssl_open_record_success| and sets |*out| to the
+// input. If it encounters a post-handshake message, it returns
+// |ssl_open_record_discard|. The caller should then retry, after processing any
+// messages received with |get_message|.
+ssl_open_record_t ssl_open_app_data(SSL *ssl, Span<uint8_t> *out,
+ size_t *out_consumed, uint8_t *out_alert,
+ Span<uint8_t> in);
+
// ssl_crypto_x509_method provides the |SSL_X509_METHOD| functions using
// crypto/x509.
extern const SSL_X509_METHOD ssl_crypto_x509_method;
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index ee7406d..e3f8a88 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -212,6 +212,14 @@
ssl->s3->read_error = ERR_save_state();
}
+static bool check_read_error(const SSL *ssl) {
+ if (ssl->s3->read_shutdown == ssl_shutdown_error) {
+ ERR_restore_state(ssl->s3->read_error);
+ return false;
+ }
+ return true;
+}
+
int ssl_can_write(const SSL *ssl) {
return !SSL_in_init(ssl) || ssl->s3->hs->can_early_write;
}
@@ -220,6 +228,51 @@
return !SSL_in_init(ssl) || ssl->s3->hs->can_early_read;
}
+ssl_open_record_t ssl_open_handshake(SSL *ssl, size_t *out_consumed,
+ uint8_t *out_alert, Span<uint8_t> in) {
+ *out_consumed = 0;
+ if (!check_read_error(ssl)) {
+ *out_alert = 0;
+ return ssl_open_record_error;
+ }
+ auto ret = ssl->method->open_handshake(ssl, out_consumed, out_alert, in);
+ if (ret == ssl_open_record_error) {
+ ssl_set_read_error(ssl);
+ }
+ return ret;
+}
+
+ssl_open_record_t ssl_open_change_cipher_spec(SSL *ssl, size_t *out_consumed,
+ uint8_t *out_alert,
+ Span<uint8_t> in) {
+ *out_consumed = 0;
+ if (!check_read_error(ssl)) {
+ *out_alert = 0;
+ return ssl_open_record_error;
+ }
+ auto ret =
+ ssl->method->open_change_cipher_spec(ssl, out_consumed, out_alert, in);
+ if (ret == ssl_open_record_error) {
+ ssl_set_read_error(ssl);
+ }
+ return ret;
+}
+
+ssl_open_record_t ssl_open_app_data(SSL *ssl, Span<uint8_t> *out,
+ size_t *out_consumed, uint8_t *out_alert,
+ Span<uint8_t> in) {
+ *out_consumed = 0;
+ if (!check_read_error(ssl)) {
+ *out_alert = 0;
+ return ssl_open_record_error;
+ }
+ auto ret = ssl->method->open_app_data(ssl, out, out_consumed, out_alert, in);
+ if (ret == ssl_open_record_error) {
+ ssl_set_read_error(ssl);
+ }
+ return ret;
+}
+
void ssl_cipher_preference_list_free(
struct ssl_cipher_preference_list_st *cipher_list) {
if (cipher_list == NULL) {
@@ -916,6 +969,11 @@
return -1;
}
+ // Replay post-handshake message errors.
+ if (!check_read_error(ssl)) {
+ return -1;
+ }
+
while (ssl->s3->pending_app_data.empty()) {
// Complete the current handshake, if any. False Start will cause
// |SSL_do_handshake| to return mid-handshake, so this may require multiple
@@ -936,6 +994,7 @@
if (ssl->method->get_message(ssl, &msg)) {
// Handle the post-handshake message and try again.
if (!ssl_do_post_handshake(ssl, msg)) {
+ ssl_set_read_error(ssl);
return -1;
}
ssl->method->next_message(ssl);
@@ -944,9 +1003,8 @@
uint8_t alert = SSL_AD_DECODE_ERROR;
size_t consumed = 0;
- auto ret =
- ssl->method->open_app_data(ssl, &ssl->s3->pending_app_data, &consumed,
- &alert, ssl_read_buffer(ssl));
+ auto ret = ssl_open_app_data(ssl, &ssl->s3->pending_app_data, &consumed,
+ &alert, ssl_read_buffer(ssl));
bool retry;
int bio_ret = ssl_handle_open_record(ssl, &retry, ret, consumed, alert);
if (bio_ret <= 0) {
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 434dfdd..5e9cde1 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -3760,150 +3760,6 @@
EXPECT_EQ(version(), SSL_SESSION_get_protocol_version(session.get()));
}
-// Test that a handshake-level errors are sticky.
-TEST_P(SSLVersionTest, StickyErrorHandshake_ParseClientHello) {
- UniquePtr<SSL_CTX> ctx = CreateContext();
- ASSERT_TRUE(ctx);
- UniquePtr<SSL> ssl(SSL_new(ctx.get()));
- ASSERT_TRUE(ssl);
- SSL_set_accept_state(ssl.get());
-
- // Pass in an empty ClientHello.
- if (is_dtls()) {
- static const uint8_t kBadClientHello[] = {
- 0x16, 0xfe, 0xff, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
- 0x00, 0x00, 0x00, 0x0c, 0x01, 0x00, 0x00, 0x00, 0x00,
- 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
- SSL_set0_rbio(ssl.get(),
- BIO_new_mem_buf(kBadClientHello, sizeof(kBadClientHello)));
- } else {
- static const uint8_t kBadClientHello[] = {0x16, 0x03, 0x01, 0x00, 0x04,
- 0x01, 0x00, 0x00, 0x00};
- SSL_set0_rbio(ssl.get(),
- BIO_new_mem_buf(kBadClientHello, sizeof(kBadClientHello)));
- }
-
- SSL_set0_wbio(ssl.get(), BIO_new(BIO_s_mem()));
-
- // The handshake logic should reject the ClientHello.
- int ret = SSL_do_handshake(ssl.get());
- EXPECT_NE(1, ret);
- EXPECT_EQ(SSL_ERROR_SSL, SSL_get_error(ssl.get(), ret));
- EXPECT_EQ(ERR_LIB_SSL, ERR_GET_LIB(ERR_peek_error()));
- EXPECT_EQ(SSL_R_DECODE_ERROR, ERR_GET_REASON(ERR_peek_error()));
- ERR_clear_error();
-
- // If we drive the handshake again, the error is replayed.
- ret = SSL_do_handshake(ssl.get());
- EXPECT_NE(1, ret);
- EXPECT_EQ(SSL_ERROR_SSL, SSL_get_error(ssl.get(), ret));
- EXPECT_EQ(ERR_LIB_SSL, ERR_GET_LIB(ERR_peek_error()));
- EXPECT_EQ(SSL_R_DECODE_ERROR, ERR_GET_REASON(ERR_peek_error()));
-}
-
-// Test that alerts during a handshake are sticky.
-TEST_P(SSLVersionTest, StickyErrorHandshake_Alert) {
- UniquePtr<SSL_CTX> ctx = CreateContext();
- ASSERT_TRUE(ctx);
- UniquePtr<SSL> ssl(SSL_new(ctx.get()));
- ASSERT_TRUE(ssl);
- SSL_set_accept_state(ssl.get());
-
- if (is_dtls()) {
- static const uint8_t kHandshakeFailureDTLS[] = {
- 0x15, 0xfe, 0xff, 0x00, 0x00, 0x00, 0x00, 0x00,
- 0x00, 0x00, 0x00, 0x00, 0x02, 0x02, 0x28};
- SSL_set0_rbio(ssl.get(), BIO_new_mem_buf(kHandshakeFailureDTLS,
- sizeof(kHandshakeFailureDTLS)));
- } else {
- static const uint8_t kHandshakeFailureTLS[] = {0x15, 0x03, 0x01, 0x00,
- 0x02, 0x02, 0x28};
- SSL_set0_rbio(ssl.get(), BIO_new_mem_buf(kHandshakeFailureTLS,
- sizeof(kHandshakeFailureTLS)));
- }
- SSL_set0_wbio(ssl.get(), BIO_new(BIO_s_mem()));
-
- int ret = SSL_do_handshake(ssl.get());
- EXPECT_NE(1, ret);
- EXPECT_EQ(SSL_ERROR_SSL, SSL_get_error(ssl.get(), ret));
- EXPECT_EQ(ERR_LIB_SSL, ERR_GET_LIB(ERR_peek_error()));
- EXPECT_EQ(SSL_R_SSLV3_ALERT_HANDSHAKE_FAILURE,
- ERR_GET_REASON(ERR_peek_error()));
- ERR_clear_error();
-
- // Driving the handshake again does not consume more records.
- ret = SSL_do_handshake(ssl.get());
- EXPECT_NE(1, ret);
- EXPECT_EQ(SSL_ERROR_SSL, SSL_get_error(ssl.get(), ret));
- EXPECT_EQ(ERR_LIB_SSL, ERR_GET_LIB(ERR_peek_error()));
- EXPECT_EQ(SSL_R_SSLV3_ALERT_HANDSHAKE_FAILURE,
- ERR_GET_REASON(ERR_peek_error()));
-}
-
-// Test that a bad record header causes a sticky error.
-TEST_P(SSLVersionTest, StickyErrorRead_BadRecordHeader) {
- // Bad record headers in DTLS are discarded.
- if (is_dtls()) {
- return;
- }
-
- ASSERT_TRUE(Connect());
-
- // Inject a record with invalid version into the stream.
- static const uint8_t kBadRecord[] = {0x16, 0x00, 0x00, 0x00, 0x00};
- SSL_set0_rbio(server_.get(), BIO_new_mem_buf(kBadRecord, sizeof(kBadRecord)));
-
- // The bad header should be rejected.
- char buf[5];
- int ret = SSL_read(server_.get(), buf, sizeof(buf));
- EXPECT_EQ(-1, ret);
- EXPECT_EQ(SSL_ERROR_SSL, SSL_get_error(server_.get(), ret));
- EXPECT_EQ(ERR_LIB_SSL, ERR_GET_LIB(ERR_peek_error()));
- EXPECT_EQ(SSL_R_WRONG_VERSION_NUMBER, ERR_GET_REASON(ERR_peek_error()));
- ERR_clear_error();
-
- // It should continue to be rejected on a retry.
- ret = SSL_read(server_.get(), buf, sizeof(buf));
- EXPECT_EQ(-1, ret);
- EXPECT_EQ(SSL_ERROR_SSL, SSL_get_error(server_.get(), ret));
- EXPECT_EQ(ERR_LIB_SSL, ERR_GET_LIB(ERR_peek_error()));
- EXPECT_EQ(SSL_R_WRONG_VERSION_NUMBER, ERR_GET_REASON(ERR_peek_error()));
-}
-
-// Test that a bad encrypted record causes a sticky error.
-TEST_P(SSLVersionTest, StickyErrorRead_BadCiphertext) {
- // Bad ciphertext in DTLS is discarded.
- if (is_dtls()) {
- return;
- }
- ASSERT_TRUE(Connect());
-
- // Inject a record with invalid version into the stream.
- uint16_t record_version =
- version() >= TLS1_3_VERSION ? TLS1_VERSION : version();
- uint8_t record[] = {SSL3_RT_APPLICATION_DATA,
- static_cast<uint8_t>(record_version >> 8),
- static_cast<uint8_t>(record_version),
- 0x00,
- 0x01,
- 0x42};
- SSL_set0_rbio(server_.get(), BIO_new_mem_buf(record, sizeof(record)));
-
- // The bad record should be rejected.
- char buf[5];
- int ret = SSL_read(server_.get(), buf, sizeof(buf));
- EXPECT_EQ(-1, ret);
- EXPECT_EQ(SSL_ERROR_SSL, SSL_get_error(server_.get(), ret));
- uint32_t err = ERR_get_error();
- ERR_clear_error();
-
- // It should continue to be rejected on a retry with the same error.
- ret = SSL_read(server_.get(), buf, sizeof(buf));
- EXPECT_EQ(-1, ret);
- EXPECT_EQ(SSL_ERROR_SSL, SSL_get_error(server_.get(), ret));
- EXPECT_EQ(err, ERR_peek_error());
-}
-
TEST_P(SSLVersionTest, SSLPending) {
UniquePtr<SSL> ssl(SSL_new(client_ctx_.get()));
ASSERT_TRUE(ssl);
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index 5f83910..1d7b996 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -56,6 +56,7 @@
#include <openssl/ssl.h>
#include <openssl/x509.h>
+#include <functional>
#include <memory>
#include <string>
#include <vector>
@@ -1408,6 +1409,32 @@
}
}
+// CheckIdempotentError runs |func|, an operation on |ssl|, ensuring that
+// errors are idempotent.
+static int CheckIdempotentError(const char *name, SSL *ssl,
+ std::function<int()> func) {
+ int ret = func();
+ int ssl_err = SSL_get_error(ssl, ret);
+ uint32_t err = ERR_peek_error();
+ if (ssl_err == SSL_ERROR_SSL || ssl_err == SSL_ERROR_ZERO_RETURN) {
+ int ret2 = func();
+ int ssl_err2 = SSL_get_error(ssl, ret2);
+ uint32_t err2 = ERR_peek_error();
+ if (ret != ret2 || ssl_err != ssl_err2 || err != err2) {
+ fprintf(stderr, "Repeating %s did not replay the error.\n", name);
+ char buf[256];
+ ERR_error_string_n(err, buf, sizeof(buf));
+ fprintf(stderr, "Wanted: %d %d %s\n", ret, ssl_err, buf);
+ ERR_error_string_n(err2, buf, sizeof(buf));
+ fprintf(stderr, "Got: %d %d %s\n", ret2, ssl_err2, buf);
+ // runner treats exit code 90 as always failing. Otherwise, it may
+ // accidentally consider the result an expected protocol failure.
+ exit(90);
+ }
+ }
+ return ret;
+}
+
// DoRead reads from |ssl|, resolving any asynchronous operations. It returns
// the result value of the final |SSL_read| call.
static int DoRead(SSL *ssl, uint8_t *out, size_t max_out) {
@@ -1421,8 +1448,10 @@
// trigger a retransmit, so disconnect the write quota.
AsyncBioEnforceWriteQuota(test_state->async_bio, false);
}
- ret = config->peek_then_read ? SSL_peek(ssl, out, max_out)
- : SSL_read(ssl, out, max_out);
+ ret = CheckIdempotentError("SSL_peek/SSL_read", ssl, [&]() -> int {
+ return config->peek_then_read ? SSL_peek(ssl, out, max_out)
+ : SSL_read(ssl, out, max_out);
+ });
if (config->async) {
AsyncBioEnforceWriteQuota(test_state->async_bio, true);
}
@@ -2163,7 +2192,9 @@
int ret;
if (!config->implicit_handshake) {
do {
- ret = SSL_do_handshake(ssl);
+ ret = CheckIdempotentError("SSL_do_handshake", ssl, [&]() -> int {
+ return SSL_do_handshake(ssl);
+ });
} while (config->async && RetryAsync(ssl, ret));
if (ret != 1 ||
!CheckHandshakeProperties(ssl, is_resume, config)) {
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 065dee8..e666404 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -1173,13 +1173,15 @@
shimKilledLock.Unlock()
}
- var isValgrindError bool
+ var isValgrindError, mustFail bool
if exitError, ok := childErr.(*exec.ExitError); ok {
switch exitError.Sys().(syscall.WaitStatus).ExitStatus() {
case 88:
return errMoreMallocs
case 89:
return errUnimplemented
+ case 90:
+ mustFail = true
case 99:
isValgrindError = true
}
@@ -1209,7 +1211,7 @@
correctFailure = correctFailure && strings.Contains(localError, test.expectedLocalError)
}
- if failed != test.shouldFail || failed && !correctFailure {
+ if failed != test.shouldFail || failed && !correctFailure || mustFail {
childError := "none"
if childErr != nil {
childError = childErr.Error()
@@ -1223,6 +1225,8 @@
msg = "unexpected success"
case failed && !correctFailure:
msg = "bad error (wanted '" + expectedError + "' / '" + test.expectedLocalError + "')"
+ case mustFail:
+ msg = "test failure"
default:
panic("internal error")
}
diff --git a/ssl/tls_record.cc b/ssl/tls_record.cc
index a85a08f..bc641fa 100644
--- a/ssl/tls_record.cc
+++ b/ssl/tls_record.cc
@@ -187,11 +187,14 @@
return ret;
}
-static enum ssl_open_record_t do_tls_open_record(SSL *ssl, uint8_t *out_type,
- Span<uint8_t> *out,
- size_t *out_consumed,
- uint8_t *out_alert,
- Span<uint8_t> in) {
+enum ssl_open_record_t tls_open_record(SSL *ssl, uint8_t *out_type,
+ Span<uint8_t> *out, size_t *out_consumed,
+ uint8_t *out_alert, Span<uint8_t> in) {
+ *out_consumed = 0;
+ if (ssl->s3->read_shutdown == ssl_shutdown_close_notify) {
+ return ssl_open_record_close_notify;
+ }
+
// 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)) {
@@ -355,29 +358,6 @@
return ssl_open_record_discard;
}
-enum ssl_open_record_t tls_open_record(SSL *ssl, uint8_t *out_type,
- Span<uint8_t> *out, size_t *out_consumed,
- uint8_t *out_alert, Span<uint8_t> in) {
- *out_consumed = 0;
- switch (ssl->s3->read_shutdown) {
- case ssl_shutdown_none:
- break;
- case ssl_shutdown_error:
- ERR_restore_state(ssl->s3->read_error);
- *out_alert = 0;
- return ssl_open_record_error;
- case ssl_shutdown_close_notify:
- return ssl_open_record_close_notify;
- }
-
- enum ssl_open_record_t ret =
- do_tls_open_record(ssl, out_type, out, out_consumed, out_alert, in);
- if (ret == ssl_open_record_error) {
- ssl_set_read_error(ssl);
- }
- return ret;
-}
-
static int do_seal_record(SSL *ssl, uint8_t *out_prefix, uint8_t *out,
uint8_t *out_suffix, uint8_t type, const uint8_t *in,
const size_t in_len) {