KeyUpdate messages are a feature of TLS 1.3 that allows the symmetric
keys of a connection to be periodically rotated. It's
mandatory-to-implement in TLS 1.3, but not mandatory to use. Google
Chrome tried enabling KeyUpdate and promptly broke several sites, at
least some of which are using HAProxy.

The cause is that HAProxy's code to disable TLS renegotiation[1] is
triggering for TLS 1.3 post-handshake messages. But renegotiation has
been removed in TLS 1.3 and post-handshake messages are no longer
abnormal. Thus I'm attaching a patch to only enforce that check when
the version of a TLS connection is <= 1.2.

Since sites that are using HAProxy with OpenSSL 1.1.1 will break when
Chrome reenables KeyUpdate without this change, I'd like to suggest it
as a candidate for backporting to stable branches.

[1] https://github.com/haproxy/haproxy/blob/master/src/ssl_sock.c#L1472


Thank you

AGL

--
Adam Langley a...@imperialviolet.org https://www.imperialviolet.org
From 39a7adab6c2583f7cf4bbe0c888c4131823d6500 Mon Sep 17 00:00:00 2001
From: Adam Langley <agl@imperialviolet.org>
Date: Sun, 20 Jan 2019 12:59:20 -0800
Subject: [PATCH] Ignore post-handshake messages in TLS 1.3 and later.

TLS 1.3 removed renegotiation but has other types of post-handshake
messages including KeyUpdate and post-handshake authentication. The
check for renegotiation in HAProxy is mistakenly triggering for these
messages in TLS 1.3.

Google Chrome plans on using KeyUpdate in the future so, at that time,
HAProxy linked against OpenSSL 1.1.1 will stop working (unless TLS 1.3
is disabled) without this patch.
---
 src/ssl_sock.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 9153843b..854becc7 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -1463,21 +1463,26 @@ out:
 #endif
 
 void ssl_sock_infocbk(const SSL *ssl, int where, int ret)
 {
 	struct connection *conn = SSL_get_ex_data(ssl, ssl_app_data_index);
 	BIO *write_bio;
 	(void)ret; /* shut gcc stupid warning */
 
 	if (where & SSL_CB_HANDSHAKE_START) {
 		/* Disable renegotiation (CVE-2009-3555) */
-		if ((conn->flags & (CO_FL_CONNECTED | CO_FL_EARLY_SSL_HS | CO_FL_EARLY_DATA)) == CO_FL_CONNECTED) {
+		if ((conn->flags & (CO_FL_CONNECTED | CO_FL_EARLY_SSL_HS | CO_FL_EARLY_DATA)) == CO_FL_CONNECTED &&
+		    /* Renegotiation has been removed from TLS 1.3. However,
+		       TLS 1.3 post-handshake messages like KeyUpdate will still
+		       trigger this callback. These must be ignored in TLS 1.3
+		       to avoid false-positives. */
+		    SSL_version(ssl) <= TLS1_2_VERSION) {
 			conn->flags |= CO_FL_ERROR;
 			conn->err_code = CO_ER_SSL_RENEG;
 		}
 	}
 
 	if ((where & SSL_CB_ACCEPT_LOOP) == SSL_CB_ACCEPT_LOOP) {
 		if (!(conn->xprt_st & SSL_SOCK_ST_FL_16K_WBFSIZE)) {
 			/* Long certificate chains optimz
 			   If write and read bios are differents, we
 			   consider that the buffering was activated,
-- 
2.20.1

Reply via email to