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

Replay the entire error queue on ssl_hs_error.

Browse Source 
This is analogous to the Go stack's handshakeErr field. Since it's quite
common for callers to run two I/O operations in parallel[*] like
SSL_read and SSL_write (or SSL_read and SSL_do_handshake for client
0-RTT). Accordingly, the new handshake state machine jams itself up on
handshake error, but to fully work with such callers, we should also
replay the error state.

This doesn't yet catch all cases (there are some parts of the read flow
which need to be fixed). Those will be resolved in later changes.

[*] Not actually in parallel, of course, but logically in parallel on a
non-blocking socket.

Bug: 206
Change-Id: I5a4d37a258b9e3fc555b732938b0528b839650f8
Reviewed-on: https://boringssl-review.googlesource.com/21285
Commit-Queue: David Benjamin <davidben@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-01 22:35:10 -04:00 committed by CQ bot account: commit-bot@chromium.org
parent b25a8999be
commit e52f4c4642
3 changed files with 47 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
ssl/handshake.cc
View File

@ -427,7 +427,7 @@ int ssl_run_handshake(SSL_HANDSHAKE *hs, bool *out_early_return) {
// Resolve the operation the handshake was waiting on. // Resolve the operation the handshake was waiting on.
switch (hs->wait) { switch (hs->wait) {
case ssl_hs_error: case ssl_hs_error:
OPENSSL_PUT_ERROR(SSL, SSL_R_SSL_HANDSHAKE_FAILURE); ERR_restore_state(hs->error.get());
return -1; return -1;
case ssl_hs_flush: { case ssl_hs_flush: {
@ -531,8 +531,7 @@ int ssl_run_handshake(SSL_HANDSHAKE *hs, bool *out_early_return) {
// Run the state machine again. // Run the state machine again.
hs->wait = ssl->do_handshake(hs); hs->wait = ssl->do_handshake(hs);
if (hs->wait == ssl_hs_error) { if (hs->wait == ssl_hs_error) {
// Don't loop around to avoid a stray |SSL_R_SSL_HANDSHAKE_FAILURE| the hs->error.reset(ERR_save_state());
// first time around.
return -1; return -1;
} }
if (hs->wait == ssl_hs_ok) { if (hs->wait == ssl_hs_ok) {

4
ssl/internal.h
View File

@ -158,6 +158,7 @@
#include <openssl/span.h> #include <openssl/span.h>
#include <openssl/stack.h> #include <openssl/stack.h>
#include "../crypto/err/internal.h"
#include "../crypto/internal.h" #include "../crypto/internal.h"
@ -1292,6 +1293,9 @@ struct SSL_HANDSHAKE {
// TLS 1.3. // TLS 1.3.
uint16_t retry_group = 0; uint16_t retry_group = 0;
// error, if |wait| is |ssl_hs_error|, is the error the handshake failed on.
UniquePtr<ERR_SAVE_STATE> error;
// key_share is the current key exchange instance. // key_share is the current key exchange instance.
UniquePtr<SSLKeyShare> key_share; UniquePtr<SSLKeyShare> key_share;

41
ssl/ssl_test.cc
View File

@ -3760,6 +3760,47 @@ TEST_P(SSLVersionTest, SessionVersion) {
EXPECT_EQ(version(), SSL_SESSION_get_protocol_version(session.get())); 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()));
}
// TODO(davidben): Convert this file to GTest properly. // TODO(davidben): Convert this file to GTest properly.
TEST(SSLTest, AllTests) { TEST(SSLTest, AllTests) {
if (!TestSSL_SESSIONEncoding(kOpenSSLSession) || if (!TestSSL_SESSIONEncoding(kOpenSSLSession) ||