From 2e7dc7cd6886b8006386e9f37e1defef66cbab55 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Fri, 17 Jun 2016 13:59:59 +0100 Subject: [PATCH] Never expose ssl->bbio in the public API. This is adapted from BoringSSL commit 2f87112b963. 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, some internal accesses of SSL_get_wbio should be switched to ssl->wbio since those want to see bbio. - 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. RT#4572 Reviewed-by: Richard Levitte --- ssl/ssl_lib.c | 112 +++++++++++++++++++-------------------- ssl/statem/statem_dtls.c | 6 +-- 2 files changed, 58 insertions(+), 60 deletions(-) diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index bf63a6c4b8..4288c6fbbc 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -977,14 +977,8 @@ void SSL_free(SSL *s) dane_final(&s->dane); CRYPTO_free_ex_data(CRYPTO_EX_INDEX_SSL, s, &s->ex_data); - if (s->bbio != NULL) { - /* If the buffering BIO is in place, pop it off */ - if (s->bbio == s->wbio) { - s->wbio = BIO_pop(s->wbio); - } - BIO_free(s->bbio); - s->bbio = NULL; - } + ssl_free_wbio_buffer(s); + if (s->wbio != s->rbio) BIO_free_all(s->wbio); BIO_free_all(s->rbio); @@ -1061,15 +1055,16 @@ void SSL_set_wbio(SSL *s, BIO *wbio) /* * If the output buffering BIO is still in place, remove it */ - if (s->bbio != NULL) { - if (s->wbio == s->bbio) { - s->wbio = BIO_next(s->wbio); - BIO_set_next(s->bbio, NULL); - } - } + if (s->bbio != NULL) + s->wbio = BIO_pop(s->wbio); + if (s->wbio != wbio && s->rbio != s->wbio) BIO_free_all(s->wbio); s->wbio = wbio; + + /* Re-attach |bbio| to the new |wbio|. */ + if (s->bbio != NULL) + s->wbio = BIO_push(s->bbio, s->wbio); } void SSL_set_bio(SSL *s, BIO *rbio, BIO *wbio) @@ -1080,17 +1075,24 @@ void SSL_set_bio(SSL *s, BIO *rbio, BIO *wbio) BIO *SSL_get_rbio(const SSL *s) { - return (s->rbio); + return s->rbio; } BIO *SSL_get_wbio(const SSL *s) { - return (s->wbio); + if (s->bbio != NULL) { + /* + * If |bbio| is active, the true caller-configured BIO is its + * |next_bio|. + */ + return BIO_next(s->bbio); + } + return s->wbio; } int SSL_get_fd(const SSL *s) { - return (SSL_get_rfd(s)); + return SSL_get_rfd(s); } int SSL_get_rfd(const SSL *s) @@ -1138,46 +1140,43 @@ int SSL_set_fd(SSL *s, int fd) int SSL_set_wfd(SSL *s, int fd) { - int ret = 0; - BIO *bio = NULL; + BIO *rbio = SSL_get_rbio(s); - if ((s->rbio == NULL) || (BIO_method_type(s->rbio) != BIO_TYPE_SOCKET) - || ((int)BIO_get_fd(s->rbio, NULL) != fd)) { - bio = BIO_new(BIO_s_socket()); + if (rbio == NULL || BIO_method_type(rbio) != BIO_TYPE_SOCKET + || (int)BIO_get_fd(rbio, NULL) != fd) { + BIO *bio = BIO_new(BIO_s_socket()); if (bio == NULL) { SSLerr(SSL_F_SSL_SET_WFD, ERR_R_BUF_LIB); - goto err; + return 0; } BIO_set_fd(bio, fd, BIO_NOCLOSE); - SSL_set_bio(s, SSL_get_rbio(s), bio); - } else - SSL_set_bio(s, SSL_get_rbio(s), SSL_get_rbio(s)); - ret = 1; - err: - return (ret); + SSL_set_wbio(s, bio); + } else { + SSL_set_wbio(s, rbio); + } + return 1; } int SSL_set_rfd(SSL *s, int fd) { - int ret = 0; - BIO *bio = NULL; + BIO *wbio = SSL_get_wbio(s); - if ((s->wbio == NULL) || (BIO_method_type(s->wbio) != BIO_TYPE_SOCKET) - || ((int)BIO_get_fd(s->wbio, NULL) != fd)) { - bio = BIO_new(BIO_s_socket()); + if (wbio == NULL || BIO_method_type(wbio) != BIO_TYPE_SOCKET + || ((int)BIO_get_fd(wbio, NULL) != fd)) { + BIO *bio = BIO_new(BIO_s_socket()); if (bio == NULL) { SSLerr(SSL_F_SSL_SET_RFD, ERR_R_BUF_LIB); - goto err; + return 0; } BIO_set_fd(bio, fd, BIO_NOCLOSE); - SSL_set_bio(s, bio, SSL_get_wbio(s)); - } else - SSL_set_bio(s, SSL_get_wbio(s), SSL_get_wbio(s)); - ret = 1; - err: - return (ret); + SSL_set_rbio(s, bio); + } else { + SSL_set_rbio(s, wbio); + } + + return 1; } #endif @@ -2923,7 +2922,11 @@ int SSL_get_error(const SSL *s, int i) } if (SSL_want_write(s)) { - bio = SSL_get_wbio(s); + /* + * Access wbio directly - in order to use the buffered bio if + * present + */ + bio = s->wbio; if (BIO_should_write(bio)) return (SSL_ERROR_WANT_WRITE); else if (BIO_should_read(bio)) @@ -3266,21 +3269,19 @@ int ssl_init_wbio_buffer(SSL *s) { BIO *bbio; - if (s->bbio == NULL) { - bbio = BIO_new(BIO_f_buffer()); - if (bbio == NULL) - return 0; - s->bbio = bbio; - s->wbio = BIO_push(bbio, s->wbio); - } else { - bbio = s->bbio; - (void)BIO_reset(bbio); + if (s->bbio != NULL) { + /* Already buffered. */ + return 1; } - if (!BIO_set_read_buffer_size(bbio, 1)) { + bbio = BIO_new(BIO_f_buffer()); + if (bbio == NULL || !BIO_set_read_buffer_size(bbio, 1)) { + BIO_free(bbio); SSLerr(SSL_F_SSL_INIT_WBIO_BUFFER, ERR_R_BUF_LIB); return 0; } + s->bbio = bbio; + s->wbio = BIO_push(bbio, s->wbio); return 1; } @@ -3291,11 +3292,8 @@ void ssl_free_wbio_buffer(SSL *s) if (s->bbio == NULL) return; - if (s->bbio == s->wbio) { - /* remove buffering */ - s->wbio = BIO_pop(s->wbio); - assert(s->wbio != NULL); - } + s->wbio = BIO_pop(s->wbio); + assert(s->wbio != NULL); BIO_free(s->bbio); s->bbio = NULL; } diff --git a/ssl/statem/statem_dtls.c b/ssl/statem/statem_dtls.c index 5929113b30..31ae1cbc8f 100644 --- a/ssl/statem/statem_dtls.c +++ b/ssl/statem/statem_dtls.c @@ -182,7 +182,7 @@ int dtls1_do_write(SSL *s, int type) } } - used_len = BIO_wpending(SSL_get_wbio(s)) + DTLS1_RT_HEADER_LENGTH + used_len = BIO_wpending(s->wbio) + DTLS1_RT_HEADER_LENGTH + mac_size + blocksize; if (s->d1->mtu > used_len) curr_mtu = s->d1->mtu - used_len; @@ -193,7 +193,7 @@ int dtls1_do_write(SSL *s, int type) /* * grr.. we could get an error if MTU picked was wrong */ - ret = BIO_flush(SSL_get_wbio(s)); + ret = BIO_flush(s->wbio); if (ret <= 0) { s->rwstate = SSL_WRITING; return ret; @@ -1120,7 +1120,7 @@ dtls1_retransmit_message(SSL *s, unsigned short seq, int *found) s->d1->retransmitting = 0; - (void)BIO_flush(SSL_get_wbio(s)); + (void)BIO_flush(s->wbio); return ret; } -- 2.25.1