From: jsing <jsing@openbsd.org>
Date: Sun, 17 Jul 2022 14:54:10 +0000 (+0000)
Subject: Correct handling of QUIC transport parameters extension.
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=db80cf4eb19af51415bb2f3714765b92864e3924;p=openbsd

Correct handling of QUIC transport parameters extension.

Remove duplicate U16 length prefix, since tlsext_build() already adds this
for us. Condition on SSL_is_quic() rather than TLS version - RFC 9001 is
clear that this extension is only permitted on QUIC transport and an
fatal unsupported extension alert is required if used elsewhere.
Additionally, at the point where extensions are parsed, we do not
necessarily know what TLS version has been negotiated.

ok beck@ tb@
---

diff --git a/lib/libssl/ssl_tlsext.c b/lib/libssl/ssl_tlsext.c
index a7c8f2d61d0..6063991306f 100644
--- a/lib/libssl/ssl_tlsext.c
+++ b/lib/libssl/ssl_tlsext.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_tlsext.c,v 1.120 2022/07/17 14:41:27 jsing Exp $ */
+/* $OpenBSD: ssl_tlsext.c,v 1.121 2022/07/17 14:54:10 jsing Exp $ */
 /*
  * Copyright (c) 2016, 2017, 2019 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2017 Doug Hogan <doug@openbsd.org>
@@ -1950,32 +1950,23 @@ tlsext_psk_server_parse(SSL *s, uint16_t msg_type, CBS *cbs, int *alert)
 }
 
 /*
- * QUIC transport parameters extension.
+ * QUIC transport parameters extension - RFC 9001 section 8.2.
  */
 
 int
 tlsext_quic_transport_parameters_client_needs(SSL *s, uint16_t msg_type)
 {
-	return (s->internal->quic_transport_params_len > 0 &&
-	    s->s3->hs.our_max_tls_version >= TLS1_3_VERSION);
+	return SSL_is_quic(s) && s->internal->quic_transport_params_len > 0;
 }
 
 int
 tlsext_quic_transport_parameters_client_build(SSL *s, uint16_t msg_type,
     CBB *cbb)
 {
-	CBB contents;
-
-	if (!CBB_add_u16_length_prefixed(cbb, &contents))
-		return 0;
-
-	if (!CBB_add_bytes(&contents, s->internal->quic_transport_params,
+	if (!CBB_add_bytes(cbb, s->internal->quic_transport_params,
 	    s->internal->quic_transport_params_len))
 		return 0;
 
-	if (!CBB_flush(cbb))
-		return 0;
-
 	return 1;
 }
 
@@ -1983,20 +1974,16 @@ int
 tlsext_quic_transport_parameters_client_parse(SSL *s, uint16_t msg_type,
     CBS *cbs, int *alert)
 {
-	CBS transport_data;
-
-	/* QUIC requires TLS 1.3. */
-	if (ssl_effective_tls_version(s) < TLS1_3_VERSION) {
+	if (!SSL_is_quic(s)) {
 		*alert = SSL_AD_UNSUPPORTED_EXTENSION;
 		return 0;
 	}
 
-	if (!CBS_get_u16_length_prefixed(cbs, &transport_data))
-		return 0;
-
-	if (!CBS_stow(&transport_data, &s->s3->peer_quic_transport_params,
+	if (!CBS_stow(cbs, &s->s3->peer_quic_transport_params,
 	    &s->s3->peer_quic_transport_params_len))
 		return 0;
+	if (!CBS_skip(cbs, s->s3->peer_quic_transport_params_len))
+		return 0;
 
 	return 1;
 }
@@ -2004,25 +1991,17 @@ tlsext_quic_transport_parameters_client_parse(SSL *s, uint16_t msg_type,
 int
 tlsext_quic_transport_parameters_server_needs(SSL *s, uint16_t msg_type)
 {
-	return s->internal->quic_transport_params_len > 0;
+	return SSL_is_quic(s) && s->internal->quic_transport_params_len > 0;
 }
 
 int
 tlsext_quic_transport_parameters_server_build(SSL *s, uint16_t msg_type,
     CBB *cbb)
 {
-	CBB contents;
-
-	if (!CBB_add_u16_length_prefixed(cbb, &contents))
-		return 0;
-
-	if (!CBB_add_bytes(&contents, s->internal->quic_transport_params,
+	if (!CBB_add_bytes(cbb, s->internal->quic_transport_params,
 	    s->internal->quic_transport_params_len))
 		return 0;
 
-	if (!CBB_flush(cbb))
-		return 0;
-
 	return 1;
 }
 
@@ -2030,27 +2009,16 @@ int
 tlsext_quic_transport_parameters_server_parse(SSL *s, uint16_t msg_type,
     CBS *cbs, int *alert)
 {
-	CBS transport_data;
-
-	/*
-	 * Ignore this extension if we don't have configured quic transport data
-	 * or if we are not TLS 1.3.
-	 */
-	if (s->internal->quic_transport_params_len == 0 ||
-	    ssl_effective_tls_version(s) < TLS1_3_VERSION) {
-		if (!CBS_skip(cbs, CBS_len(cbs))) {
-			*alert = SSL_AD_INTERNAL_ERROR;
-			return 0;
-		}
-		return 1;
-	}
-
-	if (!CBS_get_u16_length_prefixed(cbs, &transport_data))
+	if (!SSL_is_quic(s)) {
+		*alert = SSL_AD_UNSUPPORTED_EXTENSION;
 		return 0;
+	}
 
-	if (!CBS_stow(&transport_data, &s->s3->peer_quic_transport_params,
+	if (!CBS_stow(cbs, &s->s3->peer_quic_transport_params,
 	    &s->s3->peer_quic_transport_params_len))
 		return 0;
+	if (!CBS_skip(cbs, s->s3->peer_quic_transport_params_len))
+		return 0;
 
 	return 1;
 }