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