diff --git a/ssl/dtls_record.cc b/ssl/dtls_record.cc index 2bf1d426b..eccc66b2a 100644 --- a/ssl/dtls_record.cc +++ b/ssl/dtls_record.cc @@ -174,22 +174,11 @@ static void dtls1_bitmap_record(DTLS1_BITMAP *bitmap, } } -enum ssl_open_record_t dtls_open_record(SSL *ssl, uint8_t *out_type, - Span *out, - size_t *out_consumed, - uint8_t *out_alert, Span in) { - *out_consumed = 0; - switch (ssl->s3->read_shutdown) { - case ssl_shutdown_none: - break; - case ssl_shutdown_fatal_alert: - OPENSSL_PUT_ERROR(SSL, SSL_R_PROTOCOL_IS_SHUTDOWN); - *out_alert = 0; - return ssl_open_record_error; - case ssl_shutdown_close_notify: - return ssl_open_record_close_notify; - } - +static enum ssl_open_record_t do_dtls_open_record(SSL *ssl, uint8_t *out_type, + Span *out, + size_t *out_consumed, + uint8_t *out_alert, + Span in) { if (in.empty()) { return ssl_open_record_partial; } @@ -278,6 +267,30 @@ enum ssl_open_record_t dtls_open_record(SSL *ssl, uint8_t *out_type, return ssl_open_record_success; } +enum ssl_open_record_t dtls_open_record(SSL *ssl, uint8_t *out_type, + Span *out, + size_t *out_consumed, + uint8_t *out_alert, Span 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/internal.h b/ssl/internal.h index edbf4eb56..b034b661a 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -2130,7 +2130,7 @@ struct SSL3_BUFFER { enum ssl_shutdown_t { ssl_shutdown_none = 0, ssl_shutdown_close_notify = 1, - ssl_shutdown_fatal_alert = 2, + ssl_shutdown_error = 2, }; struct SSL3_STATE { @@ -2160,6 +2160,10 @@ struct SSL3_STATE { // write_shutdown is the shutdown state for the write half of the connection. enum ssl_shutdown_t write_shutdown; + // read_error, if |read_shutdown| is |ssl_shutdown_error|, is the error for + // the receive half of the connection. + ERR_SAVE_STATE *read_error; + int alert_dispatch; int total_renegotiations; @@ -2858,6 +2862,10 @@ void ssl_ctx_get_current_time(const SSL_CTX *ctx, // ssl_reset_error_state resets state for |SSL_get_error|. void ssl_reset_error_state(SSL *ssl); +// ssl_set_read_error sets |ssl|'s read half into an error state, saving the +// current state of the error queue. +void ssl_set_read_error(SSL* ssl); + } // namespace bssl diff --git a/ssl/s3_lib.cc b/ssl/s3_lib.cc index 3df8e1b3e..4d3cbb1c5 100644 --- a/ssl/s3_lib.cc +++ b/ssl/s3_lib.cc @@ -206,6 +206,7 @@ void ssl3_free(SSL *ssl) { ssl_read_buffer_clear(ssl); ssl_write_buffer_clear(ssl); + ERR_SAVE_STATE_free(ssl->s3->read_error); SSL_SESSION_free(ssl->s3->established_session); ssl_handshake_free(ssl->s3->hs); OPENSSL_free(ssl->s3->next_proto_negotiated); @@ -215,7 +216,6 @@ void ssl3_free(SSL *ssl) { Delete(ssl->s3->aead_write_ctx); BUF_MEM_free(ssl->s3->pending_flight); - OPENSSL_cleanse(ssl->s3, sizeof *ssl->s3); OPENSSL_free(ssl->s3); ssl->s3 = NULL; } diff --git a/ssl/s3_pkt.cc b/ssl/s3_pkt.cc index 71e1a0899..0b3331cf3 100644 --- a/ssl/s3_pkt.cc +++ b/ssl/s3_pkt.cc @@ -497,7 +497,7 @@ int ssl_send_alert(SSL *ssl, int level, int desc) { } else { assert(level == SSL3_AL_FATAL); assert(desc != SSL_AD_CLOSE_NOTIFY); - ssl->s3->write_shutdown = ssl_shutdown_fatal_alert; + ssl->s3->write_shutdown = ssl_shutdown_error; } ssl->s3->alert_dispatch = 1; diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index c975337de..2f5374bec 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -206,6 +206,12 @@ void ssl_reset_error_state(SSL *ssl) { ERR_clear_system_error(); } +void ssl_set_read_error(SSL* ssl) { + ssl->s3->read_shutdown = ssl_shutdown_error; + ERR_SAVE_STATE_free(ssl->s3->read_error); + ssl->s3->read_error = ERR_save_state(); +} + int ssl_can_write(const SSL *ssl) { return !SSL_in_init(ssl) || ssl->s3->hs->can_early_write; } diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 4da109d1d..f475b7367 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -3801,6 +3801,109 @@ TEST_P(SSLVersionTest, StickyErrorHandshake_ParseClientHello) { 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 ctx = CreateContext(); + ASSERT_TRUE(ctx); + UniquePtr 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(record_version >> 8), + static_cast(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_new(client_ctx_.get())); ASSERT_TRUE(ssl); diff --git a/ssl/tls_record.cc b/ssl/tls_record.cc index 44a04d91e..12597a62f 100644 --- a/ssl/tls_record.cc +++ b/ssl/tls_record.cc @@ -187,21 +187,11 @@ size_t ssl_seal_align_prefix_len(const SSL *ssl) { return ret; } -enum ssl_open_record_t tls_open_record(SSL *ssl, uint8_t *out_type, - Span *out, size_t *out_consumed, - uint8_t *out_alert, Span in) { - *out_consumed = 0; - switch (ssl->s3->read_shutdown) { - case ssl_shutdown_none: - break; - case ssl_shutdown_fatal_alert: - OPENSSL_PUT_ERROR(SSL, SSL_R_PROTOCOL_IS_SHUTDOWN); - *out_alert = 0; - return ssl_open_record_error; - case ssl_shutdown_close_notify: - return ssl_open_record_close_notify; - } - +static enum ssl_open_record_t do_tls_open_record(SSL *ssl, uint8_t *out_type, + Span *out, + size_t *out_consumed, + uint8_t *out_alert, + Span in) { CBS cbs = CBS(in); // Decode the record header. @@ -351,6 +341,29 @@ skipped_data: return ssl_open_record_discard; } +enum ssl_open_record_t tls_open_record(SSL *ssl, uint8_t *out_type, + Span *out, size_t *out_consumed, + uint8_t *out_alert, Span 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) { @@ -567,12 +580,8 @@ enum ssl_open_record_t ssl_process_alert(SSL *ssl, uint8_t *out_alert, } if (alert_level == SSL3_AL_FATAL) { - ssl->s3->read_shutdown = ssl_shutdown_fatal_alert; - - char tmp[16]; OPENSSL_PUT_ERROR(SSL, SSL_AD_REASON_OFFSET + alert_descr); - BIO_snprintf(tmp, sizeof(tmp), "%d", alert_descr); - ERR_add_error_data(2, "SSL alert number ", tmp); + ERR_add_error_dataf("SSL alert number %d", alert_descr); *out_alert = 0; // No alert to send back to the peer. return ssl_open_record_error; }