Never expose ssl->bbio in the public API.

OpenSSL's bbio logic is kind of crazy. It would be good to eventually do the
buffering in a better way (notably, bbio is fragile, if not outright broken,
for DTLS). In the meantime, this fixes a number of bugs where the existence of
bbio was leaked in the public API and broke things.

- SSL_get_wbio returned the bbio during the handshake. It must always return
  the BIO the consumer configured. In doing so, internal accesses of
  SSL_get_wbio should be switched to ssl->wbio since those want to see bbio.
  For consistency, do the same with rbio.

- The logic in SSL_set_rfd, etc. (which I doubt is quite right since
  SSL_set_bio's lifetime is unclear) would get confused once wbio got wrapped.
  Those want to compare to SSL_get_wbio.

- If SSL_set_bio was called mid-handshake, bbio would get disconnected and lose
  state. It forgets to reattach the bbio afterwards. Unfortunately, Conscrypt
  does this a lot. It just never ended up calling it at a point where the bbio
  would cause problems.

- Make more explicit the invariant that any bbio's which exist are always
  attached. Simplify a few things as part of that.

Change-Id: Ia02d6bdfb9aeb1e3021a8f82dcbd0629f5c7fb8d
Reviewed-on: https://boringssl-review.googlesource.com/8023
Reviewed-by: Kenny Root <kroot@google.com>
Reviewed-by: Adam Langley <agl@google.com>

ssl/ssl_lib.c

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index fd8c04c..86057d1 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -471,14 +471,8 @@
 
   CRYPTO_free_ex_data(&g_ex_data_class_ssl, ssl, &ssl->ex_data);
 
-  if (ssl->bbio != NULL) {
-    /* If the buffering BIO is in place, pop it off */
-    if (ssl->bbio == ssl->wbio) {
-      ssl->wbio = BIO_pop(ssl->wbio);
-    }
-    BIO_free(ssl->bbio);
-    ssl->bbio = NULL;
-  }
+  ssl_free_wbio_buffer(ssl);
+  assert(ssl->bbio == NULL);
 
   int free_wbio = ssl->wbio != ssl->rbio;
   BIO_free_all(ssl->rbio);
@@ -529,10 +523,7 @@
 void SSL_set_bio(SSL *ssl, BIO *rbio, BIO *wbio) {
   /* If the output buffering BIO is still in place, remove it. */
   if (ssl->bbio != NULL) {
-    if (ssl->wbio == ssl->bbio) {
-      ssl->wbio = ssl->wbio->next_bio;
-      ssl->bbio->next_bio = NULL;
-    }
+    ssl->wbio = BIO_pop(ssl->wbio);
   }
 
   if (ssl->rbio != rbio) {
@@ -543,11 +534,23 @@
   }
   ssl->rbio = rbio;
   ssl->wbio = wbio;
+
+  /* Re-attach |bbio| to the new |wbio|. */
+  if (ssl->bbio != NULL) {
+    ssl->wbio = BIO_push(ssl->bbio, ssl->wbio);
+  }
 }
 
 BIO *SSL_get_rbio(const SSL *ssl) { return ssl->rbio; }
 
-BIO *SSL_get_wbio(const SSL *ssl) { return ssl->wbio; }
+BIO *SSL_get_wbio(const SSL *ssl) {
+  if (ssl->bbio != NULL) {
+    /* If |bbio| is active, the true caller-configured BIO is its |next_bio|. */
+    assert(ssl->bbio == ssl->wbio);
+    return ssl->bbio->next_bio;
+  }
+  return ssl->wbio;
+}
 
 int SSL_do_handshake(SSL *ssl) {
   ssl->rwstate = SSL_NOTHING;
@@ -1030,35 +1033,36 @@
 }
 
 int SSL_set_wfd(SSL *ssl, int fd) {
-  if (ssl->rbio == NULL ||
-      BIO_method_type(ssl->rbio) != BIO_TYPE_SOCKET ||
-      BIO_get_fd(ssl->rbio, NULL) != fd) {
+  BIO *rbio = SSL_get_rbio(ssl);
+  if (rbio == NULL || BIO_method_type(rbio) != BIO_TYPE_SOCKET ||
+      BIO_get_fd(rbio, NULL) != fd) {
     BIO *bio = BIO_new(BIO_s_socket());
     if (bio == NULL) {
       OPENSSL_PUT_ERROR(SSL, ERR_R_BUF_LIB);
       return 0;
     }
     BIO_set_fd(bio, fd, BIO_NOCLOSE);
-    SSL_set_bio(ssl, SSL_get_rbio(ssl), bio);
+    SSL_set_bio(ssl, rbio, bio);
   } else {
-    SSL_set_bio(ssl, SSL_get_rbio(ssl), SSL_get_rbio(ssl));
+    SSL_set_bio(ssl, rbio, rbio);
   }
 
   return 1;
 }
 
 int SSL_set_rfd(SSL *ssl, int fd) {
-  if (ssl->wbio == NULL || BIO_method_type(ssl->wbio) != BIO_TYPE_SOCKET ||
-      BIO_get_fd(ssl->wbio, NULL) != fd) {
+  BIO *wbio = SSL_get_wbio(ssl);
+  if (wbio == NULL || BIO_method_type(wbio) != BIO_TYPE_SOCKET ||
+      BIO_get_fd(wbio, NULL) != fd) {
     BIO *bio = BIO_new(BIO_s_socket());
     if (bio == NULL) {
       OPENSSL_PUT_ERROR(SSL, ERR_R_BUF_LIB);
       return 0;
     }
     BIO_set_fd(bio, fd, BIO_NOCLOSE);
-    SSL_set_bio(ssl, bio, SSL_get_wbio(ssl));
+    SSL_set_bio(ssl, bio, wbio);
   } else {
-    SSL_set_bio(ssl, SSL_get_wbio(ssl), SSL_get_wbio(ssl));
+    SSL_set_bio(ssl, wbio, wbio);
   }
   return 1;
 }
@@ -1861,35 +1865,25 @@
 int *SSL_get_server_tmp_key(SSL *ssl, EVP_PKEY **out_key) { return 0; }
 
 int ssl_is_wbio_buffered(const SSL *ssl) {
-  return ssl->bbio != NULL && ssl->wbio == ssl->bbio;
+  return ssl->bbio != NULL;
 }
 
 int ssl_init_wbio_buffer(SSL *ssl) {
-  BIO *bbio;
-
-  if (ssl->bbio == NULL) {
-    bbio = BIO_new(BIO_f_buffer());
-    if (bbio == NULL) {
-      return 0;
-    }
-    ssl->bbio = bbio;
-  } else {
-    bbio = ssl->bbio;
-    if (ssl->bbio == ssl->wbio) {
-      ssl->wbio = BIO_pop(ssl->wbio);
-    }
+  if (ssl->bbio != NULL) {
+    /* Already buffered. */
+    assert(ssl->bbio == ssl->wbio);
+    return 1;
   }
 
-  BIO_reset(bbio);
-  if (!BIO_set_read_buffer_size(bbio, 1)) {
-    OPENSSL_PUT_ERROR(SSL, ERR_R_BUF_LIB);
+  BIO *bbio = BIO_new(BIO_f_buffer());
+  if (bbio == NULL ||
+      !BIO_set_read_buffer_size(bbio, 1)) {
+    BIO_free(bbio);
     return 0;
   }
 
-  if (ssl->wbio != bbio) {
-    ssl->wbio = BIO_push(bbio, ssl->wbio);
-  }
-
+  ssl->bbio = bbio;
+  ssl->wbio = BIO_push(bbio, ssl->wbio);
   return 1;
 }
 
@@ -1898,11 +1892,9 @@
     return;
   }
 
-  if (ssl->bbio == ssl->wbio) {
-    /* remove buffering */
-    ssl->wbio = BIO_pop(ssl->wbio);
-  }
+  assert(ssl->bbio == ssl->wbio);
 
+  ssl->wbio = BIO_pop(ssl->wbio);
   BIO_free(ssl->bbio);
   ssl->bbio = NULL;
 }