Save and restore errors when ignoring ssl_send_alert result.
Out-of-band errors, the UNIX gift that keeps on giving...
We almost always ignore the result of ssl_send_alert, treating it as
largely a "best effort". Sending an alert is the only place in the TLS
stack where we call back to user code with state in the error queue. (If
we've put something in the error queue, it means we are in the process
of failing an operation.) That user code may mess up state by, say,
calling ERR_clear_error.
In particular, if the underlying BIO is implemented with SSL_write, as
in TLS tunneled over an HTTPS proxy, the call to SSL_write will
ERR_clear_error and clobber our error state. (SSL_write must
ERR_clear_error so that SSL_get_error works. This is one of the few
places we are sensitive to clearing the error queue.)
Split ssl_send_alert into a low-level ssl_send_alert_impl (for the two
places we do honor the return value) an ssl_send_alert wrapper which
saves and restores the error queue across the call, more explicitly
ignoring the return value.
This is intended as a minimal fix to https://crbug.com/959305, in case
we need to merge it to a release branch. As a follow-up, I plan to
rework the handshake state machine so it never calls ssl_send_alert,
instead returning the alert as part of the error. This is the last bit
of I/O still in the handshake. (We have the out_alert calling
convention, but I'm thinking it's worth a small sum type where the error
branch has an alert so we don't forget to supply one everywhere.
Update-Note: This changes our behavior when sending an alert fails.
Bug: chromium:959305
Change-Id: I24033205ad0f7ebd1797964489e4d46414a3e7ec
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/35904
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/internal.h b/ssl/internal.h
index 2a173c1..c7f6a5e 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -2677,7 +2677,8 @@
void ssl_update_cache(SSL_HANDSHAKE *hs, int mode);
-int ssl_send_alert(SSL *ssl, int level, int desc);
+void ssl_send_alert(SSL *ssl, int level, int desc);
+int ssl_send_alert_impl(SSL *ssl, int level, int desc);
bool ssl3_get_message(const SSL *ssl, SSLMessage *out);
ssl_open_record_t ssl3_open_handshake(SSL *ssl, size_t *out_consumed,
uint8_t *out_alert, Span<uint8_t> in);
diff --git a/ssl/s3_pkt.cc b/ssl/s3_pkt.cc
index abc6798..67bfd63 100644
--- a/ssl/s3_pkt.cc
+++ b/ssl/s3_pkt.cc
@@ -118,6 +118,7 @@
#include <openssl/mem.h>
#include <openssl/rand.h>
+#include "../crypto/err/internal.h"
#include "../crypto/internal.h"
#include "internal.h"
@@ -381,7 +382,24 @@
return ssl_open_record_success;
}
-int ssl_send_alert(SSL *ssl, int level, int desc) {
+void ssl_send_alert(SSL *ssl, int level, int desc) {
+ // This function is called in response to a fatal error from the peer. Ignore
+ // any failures writing the alert and report only the original error. In
+ // particular, if the transport uses |SSL_write|, our existing error will be
+ // clobbered so we must save and restore the error queue. See
+ // https://crbug.com/959305.
+ //
+ // TODO(davidben): Return the alert out of the handshake, rather than calling
+ // this function internally everywhere.
+ //
+ // TODO(davidben): This does not allow retrying if the alert hit EAGAIN. See
+ // https://crbug.com/boringssl/130.
+ UniquePtr<ERR_SAVE_STATE> err_state(ERR_save_state());
+ ssl_send_alert_impl(ssl, level, desc);
+ ERR_restore_state(err_state.get());
+}
+
+int ssl_send_alert_impl(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) {
OPENSSL_PUT_ERROR(SSL, SSL_R_PROTOCOL_IS_SHUTDOWN);
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index 95177e0..f27f2b8 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -1195,7 +1195,7 @@
if (ssl->s3->write_shutdown != ssl_shutdown_close_notify) {
// Send a close_notify.
- if (ssl_send_alert(ssl, SSL3_AL_WARNING, SSL_AD_CLOSE_NOTIFY) <= 0) {
+ if (ssl_send_alert_impl(ssl, SSL3_AL_WARNING, SSL_AD_CLOSE_NOTIFY) <= 0) {
return -1;
}
} else if (ssl->s3->alert_dispatch) {
@@ -1242,7 +1242,7 @@
return ssl->method->dispatch_alert(ssl);
}
- return ssl_send_alert(ssl, SSL3_AL_FATAL, alert);
+ return ssl_send_alert_impl(ssl, SSL3_AL_FATAL, alert);
}
int SSL_set_quic_transport_params(SSL *ssl, const uint8_t *params,
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index f06b1d9..6f180c7 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -5350,5 +5350,94 @@
EXPECT_EQ(1, BORINGSSL_enum_c_type_test());
}
+TEST_P(SSLVersionTest, DoubleSSLError) {
+ // Connect the inner SSL connections.
+ ASSERT_TRUE(Connect());
+
+ // Make a pair of |BIO|s which wrap |client_| and |server_|.
+ UniquePtr<BIO_METHOD> bio_method(BIO_meth_new(0, nullptr));
+ ASSERT_TRUE(bio_method);
+ ASSERT_TRUE(BIO_meth_set_read(
+ bio_method.get(), [](BIO *bio, char *out, int len) -> int {
+ SSL *ssl = static_cast<SSL *>(BIO_get_data(bio));
+ int ret = SSL_read(ssl, out, len);
+ int ssl_ret = SSL_get_error(ssl, ret);
+ if (ssl_ret == SSL_ERROR_WANT_READ) {
+ BIO_set_retry_read(bio);
+ }
+ return ret;
+ }));
+ ASSERT_TRUE(BIO_meth_set_write(
+ bio_method.get(), [](BIO *bio, const char *in, int len) -> int {
+ SSL *ssl = static_cast<SSL *>(BIO_get_data(bio));
+ int ret = SSL_write(ssl, in, len);
+ int ssl_ret = SSL_get_error(ssl, ret);
+ if (ssl_ret == SSL_ERROR_WANT_WRITE) {
+ BIO_set_retry_write(bio);
+ }
+ return ret;
+ }));
+ ASSERT_TRUE(BIO_meth_set_ctrl(
+ bio_method.get(), [](BIO *bio, int cmd, long larg, void *parg) -> long {
+ // |SSL| objects require |BIO_flush| support.
+ if (cmd == BIO_CTRL_FLUSH) {
+ return 1;
+ }
+ return 0;
+ }));
+
+ UniquePtr<BIO> client_bio(BIO_new(bio_method.get()));
+ ASSERT_TRUE(client_bio);
+ BIO_set_data(client_bio.get(), client_.get());
+ BIO_set_init(client_bio.get(), 1);
+
+ UniquePtr<BIO> server_bio(BIO_new(bio_method.get()));
+ ASSERT_TRUE(server_bio);
+ BIO_set_data(server_bio.get(), server_.get());
+ BIO_set_init(server_bio.get(), 1);
+
+ // Wrap the inner connections in another layer of SSL.
+ UniquePtr<SSL> client_outer(SSL_new(client_ctx_.get()));
+ ASSERT_TRUE(client_outer);
+ SSL_set_connect_state(client_outer.get());
+ SSL_set_bio(client_outer.get(), client_bio.get(), client_bio.get());
+ client_bio.release(); // |SSL_set_bio| takes ownership.
+
+ UniquePtr<SSL> server_outer(SSL_new(server_ctx_.get()));
+ ASSERT_TRUE(server_outer);
+ SSL_set_accept_state(server_outer.get());
+ SSL_set_bio(server_outer.get(), server_bio.get(), server_bio.get());
+ server_bio.release(); // |SSL_set_bio| takes ownership.
+
+ // Configure |client_outer| to reject the server certificate.
+ SSL_set_custom_verify(
+ client_outer.get(), SSL_VERIFY_PEER,
+ [](SSL *ssl, uint8_t *out_alert) -> ssl_verify_result_t {
+ return ssl_verify_invalid;
+ });
+
+ for (;;) {
+ int client_ret = SSL_do_handshake(client_outer.get());
+ int client_err = SSL_get_error(client_outer.get(), client_ret);
+ if (client_err != SSL_ERROR_WANT_READ &&
+ client_err != SSL_ERROR_WANT_WRITE) {
+ // The client handshake should terminate on a certificate verification
+ // error.
+ EXPECT_EQ(SSL_ERROR_SSL, client_err);
+ uint32_t err = ERR_peek_error();
+ EXPECT_EQ(ERR_LIB_SSL, ERR_GET_LIB(err));
+ EXPECT_EQ(SSL_R_CERTIFICATE_VERIFY_FAILED, ERR_GET_REASON(err));
+ break;
+ }
+
+ // Run the server handshake and continue.
+ int server_ret = SSL_do_handshake(server_outer.get());
+ int server_err = SSL_get_error(server_outer.get(), server_ret);
+ ASSERT_TRUE(server_err == SSL_ERROR_NONE ||
+ server_err == SSL_ERROR_WANT_READ ||
+ server_err == SSL_ERROR_WANT_WRITE);
+ }
+}
+
} // namespace
BSSL_NAMESPACE_END
