send_client_hello: don't override version after HRR is received

When a Hello Retry Request is received, do not set our (transient)
version to TLS1.2 on the second client hello. That's because both
peers have already negotiated TLS1.3.

This addresses issue with peers which may send a changecipherspec
message at this stage, which is now allowed when our version is
set to be TLS1.2. Introduced test suite using openssl and resumption
using HRR which reproduces the issue.

Signed-off-by: Nikos Mavrogiannopoulos <nmav@redhat.com>

2 files changed, 44 insertions, 12 deletions

diff --git a/lib/handshake.c b/lib/handshake.c
index 5feaed99fd..d0c0f9dc97 100644
--- a/lib/handshake.c
+++ b/lib/handshake.c
@@ -2049,13 +2049,15 @@ static int send_client_hello(gnutls_session_t session, int again)
 			goto cleanup;
 		}
 
-		/* Set the version we advertized as maximum 
-		 * (RSA uses it).
-		 */
-		set_adv_version(session, hver->major, hver->minor);
-		if (_gnutls_set_current_version(session, hver->id) < 0) {
-			ret = gnutls_assert_val(GNUTLS_E_UNSUPPORTED_VERSION_PACKET);
-			goto cleanup;
+		/* if we are replying to an HRR the version is already negotiated */
+		if (!(session->internals.hsk_flags & HSK_HRR_RECEIVED) || !get_version(session)) {
+			/* Set the version we advertized as maximum
+			 * (RSA uses it). */
+			set_adv_version(session, hver->major, hver->minor);
+			if (_gnutls_set_current_version(session, hver->id) < 0) {
+				ret = gnutls_assert_val(GNUTLS_E_UNSUPPORTED_VERSION_PACKET);
+				goto cleanup;
+			}
 		}
 
 		if (session->internals.priorities->min_record_version != 0) {
diff --git a/tests/suite/testcompat-tls13-openssl.sh b/tests/suite/testcompat-tls13-openssl.sh
index 957aa5fe05..b28aad5bf0 100755
--- a/tests/suite/testcompat-tls13-openssl.sh
+++ b/tests/suite/testcompat-tls13-openssl.sh
@@ -185,7 +185,6 @@ run_client_suite() {
 	PID=$!
 	wait_server ${PID}
 
-#	${VALGRIND} "${CLI}" ${DEBUG} -p "${PORT}" 127.0.0.1 --priority "NORMAL:-VERS-ALL:+VERS-TLS1.3${ADD}" --x509cafile "${CA_CERT}" </dev/null >>${OUTPUT} || \
 	${VALGRIND} "${CLI}" ${DEBUG} -p "${PORT}" 127.0.0.1 --priority "NORMAL:-VERS-ALL:+VERS-TLS1.3${ADD}" --insecure </dev/null >>${OUTPUT} || \
 		fail ${PID} "Failed"
 
@@ -198,7 +197,6 @@ run_client_suite() {
 	PID=$!
 	wait_server ${PID}
 
-#	${VALGRIND} "${CLI}" ${DEBUG} -p "${PORT}" 127.0.0.1 --priority "NORMAL:-VERS-ALL:+VERS-TLS1.3${ADD}" --x509cafile "${CA_CERT}" </dev/null >>${OUTPUT} || \
 	${VALGRIND} "${CLI}" ${DEBUG} -p "${PORT}" 127.0.0.1 --priority "NORMAL:-VERS-ALL:+VERS-TLS1.3${ADD}" --insecure </dev/null >>${OUTPUT} || \
 		fail ${PID} "Failed"
 
@@ -213,14 +211,28 @@ run_client_suite() {
 	PID=$!
 	wait_server ${PID}
 
-	# ${VALGRIND} "${CLI}" ${DEBUG} -p "${PORT}" 127.0.0.1 --priority "NORMAL:-VERS-ALL:+VERS-TLS1.3:+GROUP-ALL${ADD}" --x509cafile "${CA_CERT}" --inline-commands | tee "${testdir}/client.out" >> ${OUTPUT}
 	${VALGRIND} "${CLI}" ${DEBUG} -p "${PORT}" 127.0.0.1 --priority "NORMAL:-VERS-ALL:+VERS-TLS1.3:+GROUP-ALL${ADD}" --insecure --inline-commands <<< $(echo -e "^resume^\nGET / HTTP/1.0\r\n\r\n")| tee "${testdir}/client.out" >> ${OUTPUT}
 	grep '^\*\*\* This is a resumed session' "${testdir}/client.out" || \
 		fail ${PID} "Failed"
 
 	kill ${PID}
 	wait
-	rm -rf "$testdir"
+
+	# Try resumption with HRR
+	echo_cmd "${PREFIX}Checking TLS 1.3 with resumption and HRR..."
+	eval "${GETPORT}"
+	launch_bare_server $$ s_server -quiet -www -accept "${PORT}" -groups 'X25519:P-256' -keyform pem -certform pem ${OPENSSL_DH_PARAMS_OPT} -key "${RSA_KEY}" -cert "${RSA_CERT}" -CAfile "${CA_CERT}"
+	PID=$!
+	wait_server ${PID}
+
+	${VALGRIND} "${CLI}" ${DEBUG} -p "${PORT}" 127.0.0.1 --priority "NORMAL:-VERS-ALL:+VERS-TLS1.3:-GROUP-ALL:+GROUP-FFDHE2048:+GROUP-SECP256R1${ADD}" --single-key-share --insecure --inline-commands <<< $(echo -e "^resume^\nGET / HTTP/1.0\r\n\r\n")| tee "${testdir}/client.out" >> ${OUTPUT}
+	grep '^\*\*\* This is a resumed session' "${testdir}/client.out" || \
+		fail ${PID} "Failed"
+
+	kill ${PID}
+	wait
+
+	rm -rf "${testdir}"
 
 }
 
@@ -448,7 +460,25 @@ _EOF_
 
 	kill ${PID}
 	wait
-	rm -rf "$testdir"
+
+	echo_cmd "${PREFIX}Checking TLS 1.3 with resumption and HRR..."
+	eval "${GETPORT}"
+	launch_server $$ --priority "NORMAL:-VERS-ALL:+VERS-TLS1.3:-CIPHER-ALL:+AES-256-GCM:-GROUP-ALL:+GROUP-SECP384R1${ADD}" --x509certfile "${RSA_CERT}" --x509keyfile "${RSA_KEY}" --x509cafile "${CA_CERT}"  >>${OUTPUT} 2>&1
+	PID=$!
+	wait_server ${PID}
+
+	{ echo a; sleep 1; } | \
+	${OPENSSL_CLI} s_client -host localhost -port "${PORT}" -curves 'X25519:P-256:X448:P-521:P-384' -CAfile "${CA_CERT}" -sess_out "${testdir}/sess-hrr.pem" 2>&1 | grep "\:error\:" && \
+		fail ${PID} "Failed"
+	${OPENSSL_CLI} s_client -host localhost -port "${PORT}" -curves 'X25519:P-256:X448:P-521:P-384' -CAfile "${CA_CERT}" -sess_in "${testdir}/sess-hrr.pem" </dev/null 2>&1 > "${testdir}/server.out"
+	grep "\:error\:" "${testdir}/server.out" && \
+		fail ${PID} "Failed"
+	grep "^Reused, TLSv1.3" "${testdir}/server.out" || \
+		fail ${PID} "Failed"
+
+	kill ${PID}
+	wait
+	rm -rf "${testdir}"
 
 }