Align with OpenSSL on SSL_set_bio behavior. SSL_set_bio is a nightmare. In f715c423224a292d79ba0e3df373c828fbae29f7, we noticed that, among other problems, SSL_set_bio's actual behavior did not match how SSL_set_rfd was calling it due to an asymmetry in the rbio/wbio handling. This resulted in SSL_set_fd/SSL_set_rfd calls to crash. We decided that SSL_set_rfd's believed semantics were definitive and changed SSL_set_bio. Upstream, in 65e2d672548e7c4bcb28f1c5c835362830b1745b, decided that SSL_set_bio's behavior, asymmetry and all, was definitive and that the SSL_set_rfd crash was a bug in SSL_set_rfd. Accordingly, they switched the fd callers to use the side-specific setters, new in 1.1.0. Align with upstream's behavior and add tests for all of SSL_set_bio's insanity. Also export the new side-specific setters in anticipation of wanting to be mostly compatible with OpenSSL 1.1.0. Change-Id: Iceac9508711f79750a3cc2ded081b2bb2cbf54d8 Reviewed-on: https://boringssl-review.googlesource.com/9064 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: Adam Langley <agl@google.com> CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 6fd5b3b..8a1eb4c 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h
@@ -240,12 +240,33 @@ * 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 21699c4..294f95d 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c
@@ -528,12 +528,12 @@ 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 @@ /* 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 @@ 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 @@ 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 5dd689b..86e7942 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc
@@ -1278,9 +1278,9 @@ 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 @@ 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 @@ !TestOneSidedShutdown() || !TestSessionDuplication() || !TestSetFD() || + !TestSetBIO() || !TestGetPeerCertificate() || !TestRetainOnlySHA256OfCerts()) { ERR_print_errors_fp(stderr);