kris/boringssl
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Align with OpenSSL on SSL_set_bio behavior.

Browse Source 
SSL_set_bio is a nightmare.

In f715c42322, 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>
This commit is contained in:
David Benjamin 2016-08-01 13:39:41 -04:00 committed by CQ bot account: commit-bot@chromium.org
parent e76cdde77d
commit 4501bd5118
3 changed files with 110 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

29
include/openssl/ssl.h
View File

@ -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);

37
ssl/ssl_lib.c
View File

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

62
ssl/ssl_test.cc
View File

@ -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);