Fix crash when flushing an SSL BIO.
OpenSSL synchronizes bio->next_bio and ssl->rbio with a variety of
callbacks, so BIO_copy_next_retry worked. We do not, so attempting to
flush the BIO crashed.
The SSL BIO is a compatibility hack and intentionally much more limited,
so start by just copying things from the right BIO directly. Add a basic
unit test for SSL BIOs. If we need to, we can implement a more complex
synchronization later.
Additionally reject reconfiguring an SSL BIO because that will leak the
object right now.
Change-Id: I724c95ab6f1a3a1aa1889b0483c81ce3bdc534ae
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/43424
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/bio/bio.c b/crypto/bio/bio.c
index 7d97c3e..3d36e28 100644
--- a/crypto/bio/bio.c
+++ b/crypto/bio/bio.c
@@ -262,6 +262,8 @@
int BIO_get_retry_reason(const BIO *bio) { return bio->retry_reason; }
+void BIO_set_retry_reason(BIO *bio, int reason) { bio->retry_reason = reason; }
+
void BIO_clear_flags(BIO *bio, int flags) {
bio->flags &= ~flags;
}
diff --git a/include/openssl/bio.h b/include/openssl/bio.h
index da0dcdf..f25492a 100644
--- a/include/openssl/bio.h
+++ b/include/openssl/bio.h
@@ -199,6 +199,10 @@
// retried. The return value is one of the |BIO_RR_*| values.
OPENSSL_EXPORT int BIO_get_retry_reason(const BIO *bio);
+// BIO_set_retry_reason sets the special I/O operation that needs to be retried
+// to |reason|, which should be one of the |BIO_RR_*| values.
+OPENSSL_EXPORT void BIO_set_retry_reason(BIO *bio, int reason);
+
// BIO_clear_flags ANDs |bio->flags| with the bitwise-complement of |flags|.
OPENSSL_EXPORT void BIO_clear_flags(BIO *bio, int flags);
diff --git a/ssl/bio_ssl.cc b/ssl/bio_ssl.cc
index 61afee5..a249889 100644
--- a/ssl/bio_ssl.cc
+++ b/ssl/bio_ssl.cc
@@ -37,12 +37,12 @@
case SSL_ERROR_WANT_ACCEPT:
BIO_set_retry_special(bio);
- bio->retry_reason = BIO_RR_ACCEPT;
+ BIO_set_retry_reason(bio, BIO_RR_ACCEPT);
break;
case SSL_ERROR_WANT_CONNECT:
BIO_set_retry_special(bio);
- bio->retry_reason = BIO_RR_CONNECT;
+ BIO_set_retry_reason(bio, BIO_RR_CONNECT);
break;
case SSL_ERROR_NONE:
@@ -77,7 +77,7 @@
case SSL_ERROR_WANT_CONNECT:
BIO_set_retry_special(bio);
- bio->retry_reason = BIO_RR_CONNECT;
+ BIO_set_retry_reason(bio, BIO_RR_CONNECT);
break;
case SSL_ERROR_NONE:
@@ -98,6 +98,17 @@
switch (cmd) {
case BIO_C_SET_SSL:
+ if (ssl != NULL) {
+ // OpenSSL allows reusing an SSL BIO with a different SSL object. We do
+ // not support this.
+ OPENSSL_PUT_ERROR(SSL, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
+ return 0;
+ }
+
+ // Note this differs from upstream OpenSSL, which synchronizes
+ // |bio->next_bio| with |ssl|'s rbio here, and on |BIO_CTRL_PUSH|. We call
+ // into the corresponding |BIO| directly. (We can implement the upstream
+ // behavior if it ends up necessary.)
bio->shutdown = num;
bio->ptr = ptr;
bio->init = 1;
@@ -117,9 +128,11 @@
return SSL_pending(ssl);
case BIO_CTRL_FLUSH: {
+ BIO *wbio = SSL_get_wbio(ssl);
BIO_clear_retry_flags(bio);
- long ret = BIO_ctrl(SSL_get_wbio(ssl), cmd, num, ptr);
- BIO_copy_next_retry(bio);
+ long ret = BIO_ctrl(wbio, cmd, num, ptr);
+ BIO_set_flags(bio, BIO_get_retry_flags(wbio));
+ BIO_set_retry_reason(bio, BIO_get_retry_reason(wbio));
return ret;
}
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 451b401..6584686 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -6529,5 +6529,64 @@
sizeof(kTicket)));
}
+TEST(SSLTest, BIO) {
+ bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method()));
+ bssl::UniquePtr<SSL_CTX> server_ctx(SSL_CTX_new(TLS_method()));
+ ASSERT_TRUE(client_ctx);
+ ASSERT_TRUE(server_ctx);
+
+ bssl::UniquePtr<X509> cert = GetTestCertificate();
+ bssl::UniquePtr<EVP_PKEY> key = GetTestKey();
+ ASSERT_TRUE(cert);
+ ASSERT_TRUE(key);
+ ASSERT_TRUE(SSL_CTX_use_certificate(server_ctx.get(), cert.get()));
+ ASSERT_TRUE(SSL_CTX_use_PrivateKey(server_ctx.get(), key.get()));
+
+ for (bool take_ownership : {true, false}) {
+ // For simplicity, get the handshake out of the way first.
+ bssl::UniquePtr<SSL> client, server;
+ ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(),
+ server_ctx.get()));
+
+ // Wrap |client| in an SSL BIO.
+ bssl::UniquePtr<BIO> client_bio(BIO_new(BIO_f_ssl()));
+ ASSERT_TRUE(client_bio);
+ ASSERT_EQ(1, BIO_set_ssl(client_bio.get(), client.get(), take_ownership));
+ if (take_ownership) {
+ client.release();
+ }
+
+ // Flushing the BIO should not crash.
+ EXPECT_EQ(1, BIO_flush(client_bio.get()));
+
+ // Exchange some data.
+ EXPECT_EQ(5, BIO_write(client_bio.get(), "hello", 5));
+ uint8_t buf[5];
+ ASSERT_EQ(5, SSL_read(server.get(), buf, sizeof(buf)));
+ EXPECT_EQ(Bytes("hello"), Bytes(buf));
+
+ EXPECT_EQ(5, SSL_write(server.get(), "world", 5));
+ ASSERT_EQ(5, BIO_read(client_bio.get(), buf, sizeof(buf)));
+ EXPECT_EQ(Bytes("world"), Bytes(buf));
+
+ // |BIO_should_read| should work.
+ EXPECT_EQ(-1, BIO_read(client_bio.get(), buf, sizeof(buf)));
+ EXPECT_TRUE(BIO_should_read(client_bio.get()));
+
+ // Writing data should eventually exceed the buffer size and fail, reporting
+ // |BIO_should_write|.
+ int ret;
+ for (int i = 0; i < 1024; i++) {
+ std::vector<uint8_t> buffer(1024);
+ ret = BIO_write(client_bio.get(), buffer.data(), buffer.size());
+ if (ret <= 0) {
+ break;
+ }
+ }
+ EXPECT_EQ(-1, ret);
+ EXPECT_TRUE(BIO_should_write(client_bio.get()));
+ }
+}
+
} // namespace
BSSL_NAMESPACE_END
