From: Olivier Houchard <cognet@ci0.org>
Date: Sat, 27 Jan 2024 21:58:29 +0000 (+0100)
Subject: BUG/MAJOR: ssl_sock: Always clear retry flags in read/write functions
X-Git-Tag: v3.0-dev3~148
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1ad19917213fac57ee37e581b0ef137e36c6309d;p=thirdparty%2Fhaproxy.git

BUG/MAJOR: ssl_sock: Always clear retry flags in read/write functions

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.
---

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index cd200283e9..92e78e80b0 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;
 }