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>

crypto/bio/bio.c

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;
 }

include/openssl/bio.h

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);
 

ssl/bio_ssl.cc

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;
     }
 

ssl/ssl_test.cc

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