diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 6fd5b3b5..8a1eb4ce 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -240,12 +240,33 @@ OPENSSL_EXPORT int SSL_is_server(SSL *ssl); * In DTLS, if |rbio| is blocking, it must handle * |BIO_CTRL_DGRAM_SET_NEXT_TIMEOUT| control requests to set read timeouts. * - * If |rbio| (respectively, |wbio|) is the same as the currently configured - * |BIO| for reading (respectively, writing), that side is left untouched and is - * not freed. Using this behavior and calling this function if |ssl| already has - * |BIO|s configured is deprecated. */ + * If |rbio| is the same as the currently configured |BIO| for reading, that + * side is left untouched and is not freed. + * + * If |wbio| is the same as the currently configured |BIO| for writing AND |ssl| + * is not currently configured to read from and write to the same |BIO|, that + * side is left untouched and is not freed. This asymmetry is present for + * historical reasons. + * + * Due to the very complex historical behavior of this function, calling this + * function if |ssl| already has |BIO|s configured is deprecated. Prefer + * |SSL_set0_rbio| and |SSL_set0_wbio| instead. */ OPENSSL_EXPORT void SSL_set_bio(SSL *ssl, BIO *rbio, BIO *wbio); +/* SSL_set0_rbio configures |ssl| to write to |rbio|. It takes ownership of + * |rbio|. + * + * Note that, although this function and |SSL_set0_wbio| may be called on the + * same |BIO|, each call takes a reference. Use |BIO_up_ref| to balance this. */ +OPENSSL_EXPORT void SSL_set0_rbio(SSL *ssl, BIO *rbio); + +/* SSL_set0_wbio configures |ssl| to write to |wbio|. It takes ownership of + * |wbio|. + * + * Note that, although this function and |SSL_set0_rbio| may be called on the + * same |BIO|, each call takes a reference. Use |BIO_up_ref| to balance this. */ +OPENSSL_EXPORT void SSL_set0_wbio(SSL *ssl, BIO *wbio); + /* SSL_get_rbio returns the |BIO| that |ssl| reads from. */ OPENSSL_EXPORT BIO *SSL_get_rbio(const SSL *ssl); diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 21699c4f..294f95dc 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -528,12 +528,12 @@ void SSL_set_accept_state(SSL *ssl) { ssl->handshake_func = ssl3_accept; } -static void ssl_set_rbio(SSL *ssl, BIO *rbio) { +void SSL_set0_rbio(SSL *ssl, BIO *rbio) { BIO_free_all(ssl->rbio); ssl->rbio = rbio; } -static void ssl_set_wbio(SSL *ssl, BIO *wbio) { +void SSL_set0_wbio(SSL *ssl, BIO *wbio) { /* If the output buffering BIO is still in place, remove it. */ if (ssl->bbio != NULL) { ssl->wbio = BIO_pop(ssl->wbio); @@ -552,25 +552,34 @@ void SSL_set_bio(SSL *ssl, BIO *rbio, BIO *wbio) { /* For historical reasons, this function has many different cases in ownership * handling. */ + /* If nothing has changed, do nothing */ + if (rbio == SSL_get_rbio(ssl) && wbio == SSL_get_wbio(ssl)) { + return; + } + /* If the two arguments are equal, one fewer reference is granted than * taken. */ if (rbio != NULL && rbio == wbio) { BIO_up_ref(rbio); } - /* If at most one of rbio or wbio is changed, only adopt one reference. */ + /* If only the wbio is changed, adopt only one reference. */ if (rbio == SSL_get_rbio(ssl)) { - ssl_set_wbio(ssl, wbio); + SSL_set0_wbio(ssl, wbio); return; } - if (wbio == SSL_get_wbio(ssl)) { - ssl_set_rbio(ssl, rbio); + + /* There is an asymmetry here for historical reasons. If only the rbio is + * changed AND the rbio and wbio were originally different, then we only adopt + * one reference. */ + if (wbio == SSL_get_wbio(ssl) && SSL_get_rbio(ssl) != SSL_get_wbio(ssl)) { + SSL_set0_rbio(ssl, rbio); return; } /* Otherwise, adopt both references. */ - ssl_set_rbio(ssl, rbio); - ssl_set_wbio(ssl, wbio); + SSL_set0_rbio(ssl, rbio); + SSL_set0_wbio(ssl, wbio); } BIO *SSL_get_rbio(const SSL *ssl) { return ssl->rbio; } @@ -1158,9 +1167,11 @@ int SSL_set_wfd(SSL *ssl, int fd) { return 0; } BIO_set_fd(bio, fd, BIO_NOCLOSE); - SSL_set_bio(ssl, rbio, bio); + SSL_set0_wbio(ssl, bio); } else { - SSL_set_bio(ssl, rbio, rbio); + /* Copy the rbio over to the wbio. */ + BIO_up_ref(rbio); + SSL_set0_wbio(ssl, rbio); } return 1; @@ -1176,9 +1187,11 @@ int SSL_set_rfd(SSL *ssl, int fd) { return 0; } BIO_set_fd(bio, fd, BIO_NOCLOSE); - SSL_set_bio(ssl, bio, wbio); + SSL_set0_rbio(ssl, bio); } else { - SSL_set_bio(ssl, wbio, wbio); + /* Copy the wbio over to the rbio. */ + BIO_up_ref(wbio); + SSL_set0_rbio(ssl, wbio); } return 1; } diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 5dd689b5..86e79422 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -1278,9 +1278,9 @@ static bool TestSessionDuplication() { SSL_SESSION *session0 = SSL_get_session(client.get()); ScopedSSL_SESSION session1(SSL_SESSION_dup(session0, 1)); if (!session1) { - return false; + return false; } - + uint8_t *s0_bytes, *s1_bytes; size_t s0_len, s1_len; @@ -1397,6 +1397,63 @@ static bool TestSetFD() { return true; } +static bool TestSetBIO() { + ScopedSSL_CTX ctx(SSL_CTX_new(TLS_method())); + if (!ctx) { + return false; + } + + ScopedSSL ssl(SSL_new(ctx.get())); + ScopedBIO bio1(BIO_new(BIO_s_mem())), bio2(BIO_new(BIO_s_mem())), + bio3(BIO_new(BIO_s_mem())); + if (!ssl || !bio1 || !bio2 || !bio3) { + return false; + } + + // SSL_set_bio takes one reference when the parameters are the same. + BIO_up_ref(bio1.get()); + SSL_set_bio(ssl.get(), bio1.get(), bio1.get()); + + // Repeating the call does nothing. + SSL_set_bio(ssl.get(), bio1.get(), bio1.get()); + + // It takes one reference each when the parameters are different. + BIO_up_ref(bio2.get()); + BIO_up_ref(bio3.get()); + SSL_set_bio(ssl.get(), bio2.get(), bio3.get()); + + // Repeating the call does nothing. + SSL_set_bio(ssl.get(), bio2.get(), bio3.get()); + + // It takes one reference when changing only wbio. + BIO_up_ref(bio1.get()); + SSL_set_bio(ssl.get(), bio2.get(), bio1.get()); + + // It takes one reference when changing only rbio and the two are different. + BIO_up_ref(bio3.get()); + SSL_set_bio(ssl.get(), bio3.get(), bio1.get()); + + // If setting wbio to rbio, it takes no additional references. + SSL_set_bio(ssl.get(), bio3.get(), bio3.get()); + + // From there, wbio may be switched to something else. + BIO_up_ref(bio1.get()); + SSL_set_bio(ssl.get(), bio3.get(), bio1.get()); + + // If setting rbio to wbio, it takes no additional references. + SSL_set_bio(ssl.get(), bio1.get(), bio1.get()); + + // From there, rbio may be switched to something else, but, for historical + // reasons, it takes a reference to both parameters. + BIO_up_ref(bio1.get()); + BIO_up_ref(bio2.get()); + SSL_set_bio(ssl.get(), bio2.get(), bio1.get()); + + // ASAN builds will implicitly test that the internal |BIO| reference-counting + // is correct. + return true; +} + static uint16_t kVersions[] = { SSL3_VERSION, TLS1_VERSION, TLS1_1_VERSION, TLS1_2_VERSION, TLS1_3_VERSION, }; @@ -1546,6 +1603,7 @@ int main() { !TestOneSidedShutdown() || !TestSessionDuplication() || !TestSetFD() || + !TestSetBIO() || !TestGetPeerCertificate() || !TestRetainOnlySHA256OfCerts()) { ERR_print_errors_fp(stderr);