Only allow supported_versions in a TLSv1.3 ServerHello

As per the latest text in TLSv1.3 draft-26

Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/5604)

diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c
index 0641a253d3a831c7c7bb759ae77ab9904f0b05db..3dc4e8ed942cd8c019c30541987f1ca6141bc9f5 100644 (file)
--- a/ssl/statem/extensions.c
+++ b/ssl/statem/extensions.c
@@ -307,9 +307,8 @@ static const EXTENSION_DEFINITION ext_defs[] = {
     },
     {
         TLSEXT_TYPE_supported_versions,
-        SSL_EXT_CLIENT_HELLO | SSL_EXT_TLS1_2_SERVER_HELLO
-        | SSL_EXT_TLS1_3_SERVER_HELLO | SSL_EXT_TLS1_3_HELLO_RETRY_REQUEST
-        | SSL_EXT_TLS_IMPLEMENTATION_ONLY,
+        SSL_EXT_CLIENT_HELLO | SSL_EXT_TLS1_3_SERVER_HELLO
+        | SSL_EXT_TLS1_3_HELLO_RETRY_REQUEST | SSL_EXT_TLS_IMPLEMENTATION_ONLY,
         NULL,
         /* Processed inline as part of version selection */
         NULL, tls_parse_stoc_supported_versions,

diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c
index 9bf2d1cb38fd2bcb2d2a5c6584977dd0c014511e..bd025d7c023a4dc73c1fc0c9cc5a7f743b762a64 100644 (file)
--- a/ssl/statem/extensions_clnt.c
+++ b/ssl/statem/extensions_clnt.c
@@ -1780,20 +1780,20 @@ int tls_parse_stoc_supported_versions(SSL *s, PACKET *pkt, unsigned int context,
     if (version == TLS1_3_VERSION_DRAFT)
         version = TLS1_3_VERSION;
 
+    /*
+     * The only protocol version we support which is valid in this extension in
+     * a ServerHello is TLSv1.3 therefore we shouldn't be getting anything else.
+     */
+    if (version != TLS1_3_VERSION) {
+        SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER,
+                 SSL_F_TLS_PARSE_STOC_SUPPORTED_VERSIONS,
+                 SSL_R_BAD_PROTOCOL_VERSION_NUMBER);
+        return 0;
+    }
+
     /* We ignore this extension for HRRs except to sanity check it */
-    if (context == SSL_EXT_TLS1_3_HELLO_RETRY_REQUEST) {
-        /*
-         * The only protocol version we support which has an HRR message is
-         * TLSv1.3, therefore we shouldn't be getting an HRR for anything else.
-         */
-        if (version != TLS1_3_VERSION) {
-            SSLfatal(s, SSL_AD_PROTOCOL_VERSION,
-                     SSL_F_TLS_PARSE_STOC_SUPPORTED_VERSIONS,
-                     SSL_R_BAD_HRR_VERSION);
-            return 0;
-        }
+    if (context == SSL_EXT_TLS1_3_HELLO_RETRY_REQUEST)
         return 1;
-    }
 
     /* We just set it here. We validate it in ssl_choose_client_version */
     s->version = version;

diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c
index 425cd80efeacbf3590ffde7ab881abc216069faf..a1f92b076d75709e3f498b5e675e0360fd281073 100644 (file)
--- a/ssl/statem/extensions_srvr.c
+++ b/ssl/statem/extensions_srvr.c
@@ -1572,8 +1572,12 @@ EXT_RETURN tls_construct_stoc_supported_versions(SSL *s, WPACKET *pkt,
                                                  unsigned int context, X509 *x,
                                                  size_t chainidx)
 {
-    if (!SSL_IS_TLS13(s))
-        return EXT_RETURN_NOT_SENT;
+    if (!ossl_assert(SSL_IS_TLS13(s))) {
+        SSLfatal(s, SSL_AD_INTERNAL_ERROR,
+                 SSL_F_TLS_CONSTRUCT_STOC_SUPPORTED_VERSIONS,
+                 ERR_R_INTERNAL_ERROR);
+        return EXT_RETURN_FAIL;
+    }
 
     if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_supported_versions)
             || !WPACKET_start_sub_packet_u16(pkt)