From 1ad19917213fac57ee37e581b0ef137e36c6309d Mon Sep 17 00:00:00 2001 From: Olivier Houchard Date: Sat, 27 Jan 2024 22:58:29 +0100 Subject: [PATCH] BUG/MAJOR: ssl_sock: Always clear retry flags in read/write functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It has been found that under some rare error circumstances, SSL_do_handshake() could return with SSL_ERROR_WANT_READ without even trying to call the read function, causing permanent wakeups that prevent the process from sleeping. It was established that this only happens if the retry flags are not systematically cleared in both directions upon any I/O attempt, but, given the lack of documentation on this topic, it is hard to say if this rather strange behavior is expected or not, otherwise why wouldn't the library always clear the flags by itself before proceeding? In addition, this only seems to affect OpenSSL 1.1.0 and above, and does not affect wolfSSL nor aws-lc. A bisection on haproxy showed that this issue was first triggered by commit a8955d57ed ("MEDIUM: ssl: provide our own BIO."), which means that OpenSSL's socket BIO does not have this problem. And this one does always clear the flags before proceeding. So let's just proceed the same way. It was verified that it properly fixes the problem, does not affect other implementations, and doesn't cause any freeze nor spurious wakeups either. Many thanks to Valentín Gutiérrez for providing a network capture showing the incident as well as a reproducer. This is GH issue #2403. This patch needs to be backported to all versions that include the commit above, i.e. as far as 2.0. --- src/ssl_sock.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ssl_sock.c b/src/ssl_sock.c index cd200283e..92e78e80b 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -226,11 +226,11 @@ static int ha_ssl_write(BIO *h, const char *buf, int num) tmpbuf.head = 0; flags = (ctx->xprt_st & SSL_SOCK_SEND_MORE) ? CO_SFL_MSG_MORE : 0; ret = ctx->xprt->snd_buf(ctx->conn, ctx->xprt_ctx, &tmpbuf, num, flags); + BIO_clear_retry_flags(h); if (ret == 0 && !(ctx->conn->flags & (CO_FL_ERROR | CO_FL_SOCK_WR_SH))) { BIO_set_retry_write(h); ret = -1; - } else if (ret == 0) - BIO_clear_retry_flags(h); + } return ret; } @@ -258,11 +258,11 @@ static int ha_ssl_read(BIO *h, char *buf, int size) tmpbuf.data = 0; tmpbuf.head = 0; ret = ctx->xprt->rcv_buf(ctx->conn, ctx->xprt_ctx, &tmpbuf, size, 0); + BIO_clear_retry_flags(h); if (ret == 0 && !(ctx->conn->flags & (CO_FL_ERROR | CO_FL_SOCK_RD_SH))) { BIO_set_retry_read(h); ret = -1; - } else if (ret == 0) - BIO_clear_retry_flags(h); + } return ret; }