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/include/openssl/ssl.h b/include/openssl/ssl.h
index 3e7fad8..bd583e8 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -3863,7 +3863,9 @@
BIO *wbio; /* used by SSL_write */
/* bbio, if non-NULL, is a buffer placed in front of |wbio| to pack handshake
- * messages within one flight into a single |BIO_write|.
+ * messages within one flight into a single |BIO_write|. In this case, |wbio|
+ * and |bbio| are equal and the true caller-configured BIO is
+ * |bbio->next_bio|.
*
* TODO(davidben): This does not work right for DTLS. It assumes the MTU is
* smaller than the buffer size so that the buffer's internal flushing never
diff --git a/ssl/d1_both.c b/ssl/d1_both.c
index 074668c..8d8784b 100644
--- a/ssl/d1_both.c
+++ b/ssl/d1_both.c
@@ -249,12 +249,12 @@
/* TODO(davidben): What is this code doing and do we need it? */
if (ssl->d1->mtu < dtls1_min_mtu() &&
!(SSL_get_options(ssl) & SSL_OP_NO_QUERY_MTU)) {
- long mtu = BIO_ctrl(SSL_get_wbio(ssl), BIO_CTRL_DGRAM_QUERY_MTU, 0, NULL);
+ long mtu = BIO_ctrl(ssl->wbio, BIO_CTRL_DGRAM_QUERY_MTU, 0, NULL);
if (mtu >= 0 && mtu <= (1 << 30) && (unsigned)mtu >= dtls1_min_mtu()) {
ssl->d1->mtu = (unsigned)mtu;
} else {
ssl->d1->mtu = kDefaultMTU;
- BIO_ctrl(SSL_get_wbio(ssl), BIO_CTRL_DGRAM_SET_MTU, ssl->d1->mtu, NULL);
+ BIO_ctrl(ssl->wbio, BIO_CTRL_DGRAM_SET_MTU, ssl->d1->mtu, NULL);
}
}
@@ -274,7 +274,7 @@
}
ret -= overhead;
- size_t pending = BIO_wpending(SSL_get_wbio(ssl));
+ size_t pending = BIO_wpending(ssl->wbio);
if (ret <= pending) {
return 0;
}
@@ -290,7 +290,7 @@
/* During the handshake, wbio is buffered to pack messages together. Flush the
* buffer if the ChangeCipherSpec would not fit in a packet. */
if (dtls1_max_record_size(ssl) == 0) {
- int ret = BIO_flush(SSL_get_wbio(ssl));
+ int ret = BIO_flush(ssl->wbio);
if (ret <= 0) {
ssl->rwstate = SSL_WRITING;
return ret;
@@ -339,13 +339,13 @@
/* During the handshake, wbio is buffered to pack messages together. Flush
* the buffer if there isn't enough room to make progress. */
if (dtls1_max_record_size(ssl) < DTLS1_HM_HEADER_LENGTH + 1) {
- int flush_ret = BIO_flush(SSL_get_wbio(ssl));
+ int flush_ret = BIO_flush(ssl->wbio);
if (flush_ret <= 0) {
ssl->rwstate = SSL_WRITING;
ret = flush_ret;
goto err;
}
- assert(BIO_wpending(SSL_get_wbio(ssl)) == 0);
+ assert(BIO_wpending(ssl->wbio) == 0);
}
size_t todo = dtls1_max_record_size(ssl);
@@ -672,7 +672,7 @@
if (!SSL_in_init(ssl)) {
/* done, no need to send a retransmit */
- BIO_set_flags(SSL_get_rbio(ssl), BIO_FLAGS_READ);
+ BIO_set_flags(ssl->rbio, BIO_FLAGS_READ);
return code;
}
@@ -742,7 +742,7 @@
}
}
- ret = BIO_flush(SSL_get_wbio(ssl));
+ ret = BIO_flush(ssl->wbio);
if (ret <= 0) {
ssl->rwstate = SSL_WRITING;
goto err;
diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c
index dc3c640..c1c9f91 100644
--- a/ssl/d1_lib.c
+++ b/ssl/d1_lib.c
@@ -177,7 +177,7 @@
ssl->d1->next_timeout.tv_sec++;
ssl->d1->next_timeout.tv_usec -= 1000000;
}
- BIO_ctrl(SSL_get_rbio(ssl), BIO_CTRL_DGRAM_SET_NEXT_TIMEOUT, 0,
+ BIO_ctrl(ssl->rbio, BIO_CTRL_DGRAM_SET_NEXT_TIMEOUT, 0,
&ssl->d1->next_timeout);
}
@@ -251,7 +251,7 @@
ssl->d1->num_timeouts = 0;
memset(&ssl->d1->next_timeout, 0, sizeof(struct timeval));
ssl->d1->timeout_duration_ms = ssl->initial_timeout_duration_ms;
- BIO_ctrl(SSL_get_rbio(ssl), BIO_CTRL_DGRAM_SET_NEXT_TIMEOUT, 0,
+ BIO_ctrl(ssl->rbio, BIO_CTRL_DGRAM_SET_NEXT_TIMEOUT, 0,
&ssl->d1->next_timeout);
/* Clear retransmission buffer */
dtls1_clear_record_buffer(ssl);
@@ -263,8 +263,7 @@
/* Reduce MTU after 2 unsuccessful retransmissions */
if (ssl->d1->num_timeouts > DTLS1_MTU_TIMEOUTS &&
!(SSL_get_options(ssl) & SSL_OP_NO_QUERY_MTU)) {
- long mtu = BIO_ctrl(SSL_get_wbio(ssl), BIO_CTRL_DGRAM_GET_FALLBACK_MTU, 0,
- NULL);
+ long mtu = BIO_ctrl(ssl->wbio, BIO_CTRL_DGRAM_GET_FALLBACK_MTU, 0, NULL);
if (mtu >= 0 && mtu <= (1 << 30) && (unsigned)mtu >= dtls1_min_mtu()) {
ssl->d1->mtu = (unsigned)mtu;
}
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;
}
