Merge pull request #10811 from kareem-wolfssl/gh10746

Correct alert type for missing supported_versions in HRR and avoid sending duplicate protocol_version alerts.

Author: SparkiDev

src/internal.c

@@ -33167,7 +33167,17 @@ static void MakePSKPreMasterSecret(Arrays* arrays, byte use_psk_key)
 #ifdef WOLFSSL_TLS13
         if (IsAtLeastTLSv1_3(pv)) {
             byte type = server_hello;
-            return DoTls13ServerHello(ssl, input, inOutIdx, helloSz, &type);
+            ret = DoTls13ServerHello(ssl, input, inOutIdx, helloSz, &type);
+            if (ret != 0 &&
+                    ssl->alert_history.last_tx.level != alert_fatal) {
+                int alertType = TranslateErrorToAlert(ret);
+                if (alertType != invalid_alert) {
+                    if (SendAlert(ssl, alert_fatal, alertType) ==
+                            WC_NO_ERR_TRACE(SOCKET_ERROR_E))
+                        ret = SOCKET_ERROR_E;
+                }
+            }
+            return ret;
         }
 #endif
 

src/tls13.c

@@ -5483,7 +5483,6 @@ int DoTls13ServerHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx,
 
     if (args->pv.major != ssl->version.major ||
         args->pv.minor != tls12minor) {
-        SendAlert(ssl, alert_fatal, wolfssl_alert_protocol_version);
         WOLFSSL_ERROR_VERBOSE(VERSION_ERROR);
         return VERSION_ERROR;
     }
@@ -5567,39 +5566,42 @@ int DoTls13ServerHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx,
     }
 
     if ((args->idx - args->begin) + OPAQUE16_LEN > helloSz) {
-        if (!ssl->options.downgrade) {
-            /* Fewer than OPAQUE16_LEN bytes remain after the compression
-             * method, so there is no complete extensions length field. */
-            if ((args->idx - args->begin) < helloSz) {
-                /* A partial extensions length field is genuinely malformed:
-                 * report it as a decode error. */
-                WOLFSSL_MSG("Truncated extensions length in ServerHello");
-                return BUFFER_ERROR;
-            }
-            /* No extensions field at all, so the server is not offering TLS 1.3
-             * (no supported_versions extension - see RFC 8446 4.2.1) but TLS 1.2
-             * or below. This is a well-formed message, so a TLS 1.3-only client
-             * (downgrade disabled) must reject it as a version mismatch, not as
-             * a malformed message. Returning VERSION_ERROR makes the caller send
-             * a protocol_version alert (RFC 8446 6.2) rather than decode_error. */
-            WOLFSSL_MSG("Server offered TLS 1.2 (no supported_versions ext) "
-                        "but downgrade not allowed");
-            WOLFSSL_ERROR_VERBOSE(VERSION_ERROR);
-            return VERSION_ERROR;
+        /* Fewer than OPAQUE16_LEN bytes remain after the compression method, so
+         * there is no complete extensions length field. */
+        if ((args->idx - args->begin) < helloSz) {
+            /* A partial extensions length field is genuinely malformed: report
+             * it as a decode error regardless of message type. */
+            WOLFSSL_MSG("Truncated extensions length in ServerHello");
+            return BUFFER_ERROR;
         }
 
+        /* No extensions field at all. */
         if (args->extMsgType == hello_retry_request) {
             /* The sentinel Random (RFC 8446 4.1.3) identifies this as a TLS 1.3
              * HelloRetryRequest, which MUST carry supported_versions
-             * (4.1.4/4.2.1). Reaching this downgrade branch means it does not,
-             * so the HRR is malformed. Reject it before DoServerHello would
-             * reinterpret the sentinel as a plain TLS 1.2 ServerHello.Random.
-             * The sentinel has already identified this as an HRR, so report it
-             * uniformly as an invalid HRR; INVALID_PARAMETER maps to the
-             * illegal_parameter alert. */
+             * (4.2.1/9.2). Its complete absence is a missing mandatory
+             * extension, so - consistently with the extensions-present case
+             * handled later - report it as missing_extension (via
+             * INCOMPLETE_DATA), regardless of whether a downgrade would
+             * otherwise be allowed. Reject here before DoServerHello would
+             * reinterpret the sentinel as a plain TLS 1.2 ServerHello.Random. */
             WOLFSSL_MSG("HelloRetryRequest with no supported_versions");
-            WOLFSSL_ERROR_VERBOSE(INVALID_PARAMETER);
-            return INVALID_PARAMETER;
+            WOLFSSL_ERROR_VERBOSE(INCOMPLETE_DATA);
+            return INCOMPLETE_DATA;
+        }
+
+        if (!ssl->options.downgrade) {
+            /* A plain ServerHello with no extensions is not offering TLS 1.3
+             * (no supported_versions extension - see RFC 8446 4.2.1) but TLS
+             * 1.2 or below. This is a well-formed message, so a TLS 1.3-only
+             * client (downgrade disabled) must reject it as a version mismatch,
+             * not as a malformed message. Returning VERSION_ERROR makes the
+             * caller send a protocol_version alert (RFC 8446 6.2) rather than
+             * decode_error. */
+            WOLFSSL_MSG("Server offered TLS 1.2 (no supported_versions ext) "
+                        "but downgrade not allowed");
+            WOLFSSL_ERROR_VERBOSE(VERSION_ERROR);
+            return VERSION_ERROR;
         }
 #ifndef WOLFSSL_NO_TLS12
         /* Force client hello version 1.2 to work for static RSA. */
@@ -5620,14 +5622,14 @@ int DoTls13ServerHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx,
 #endif
         ssl->options.haveEMS = 0;
         if (args->pv.minor < ssl->options.minDowngrade) {
-            SendAlert(ssl, alert_fatal, wolfssl_alert_protocol_version);
+            WOLFSSL_ERROR_VERBOSE(VERSION_ERROR);
             return VERSION_ERROR;
         }
 #ifndef WOLFSSL_NO_TLS12
         ssl->options.tls1_3 = 0;
         return DoServerHello(ssl, input, inOutIdx, helloSz);
 #else
-        SendAlert(ssl, alert_fatal, wolfssl_alert_protocol_version);
+        WOLFSSL_ERROR_VERBOSE(VERSION_ERROR);
         return VERSION_ERROR;
 #endif
     }
@@ -5649,28 +5651,27 @@ int DoTls13ServerHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx,
             return ret;
         }
         if (!foundVersion) {
+            /* RFC 8446 4.1.4: "The server's extensions MUST contain
+             * 'supported_versions'." (also 9.2: "supported_versions" is
+             * REQUIRED for all ... HelloRetryRequest messages). The HRR random
+             * unambiguously identifies a TLS 1.3 server, so its absence is not
+             * a downgrade attempt but a missing mandatory extension, which per
+             * the "missing_extension" alert definition must be reported as
+             * such. Return INCOMPLETE_DATA (which maps to a missing_extension
+             * alert) and let the caller emit the alert via
+             * TranslateErrorToAlert(). */
+            if (*extMsgType == hello_retry_request) {
+                WOLFSSL_MSG("HelloRetryRequest missing supported_versions "
+                            "extension");
+                WOLFSSL_ERROR_VERBOSE(INCOMPLETE_DATA);
+                return INCOMPLETE_DATA;
+            }
             if (!ssl->options.downgrade) {
                 WOLFSSL_MSG("Server trying to downgrade to version less than "
                             "TLS v1.3");
-                SendAlert(ssl, alert_fatal, wolfssl_alert_protocol_version);
                 WOLFSSL_ERROR_VERBOSE(VERSION_ERROR);
                 return VERSION_ERROR;
             }
-
-            if (args->extMsgType == hello_retry_request) {
-                /* The HelloRetryRequest sentinel Random is reserved for TLS 1.3
-                 * (RFC 8446 4.1.3), but supported_versions (which an HRR MUST
-                 * carry, 4.1.4/4.2.1) is absent, so the message is malformed.
-                 * Reject before the downgrade reparses the remaining extensions
-                 * as a TLS 1.2 server_hello, where a recognized-but-not-
-                 * permitted extension (e.g. server_name) would wrongly yield an
-                 * unsupported_extension alert. illegal_parameter (via
-                 * EXT_NOT_ALLOWED) is chosen over missing_extension per the
-                 * RFC 8446 4.2 rule for such extensions. */
-                WOLFSSL_MSG("HelloRetryRequest without supported_versions");
-                WOLFSSL_ERROR_VERBOSE(EXT_NOT_ALLOWED);
-                return EXT_NOT_ALLOWED;
-            }
 #if defined(OPENSSL_EXTRA) || defined(HAVE_WEBSERVER) || \
     defined(WOLFSSL_WPAS_SMALL)
             /* Check if client has disabled TLS 1.2 */
@@ -5686,14 +5687,12 @@ int DoTls13ServerHello(WOLFSSL* ssl, const byte* input, word32* inOutIdx,
 
             if (!ssl->options.dtls &&
                 args->pv.minor < ssl->options.minDowngrade) {
-                SendAlert(ssl, alert_fatal, wolfssl_alert_protocol_version);
                 WOLFSSL_ERROR_VERBOSE(VERSION_ERROR);
                 return VERSION_ERROR;
             }
 
             if (ssl->options.dtls &&
                 args->pv.minor > ssl->options.minDowngrade) {
-                SendAlert(ssl, alert_fatal, wolfssl_alert_protocol_version);
                 WOLFSSL_ERROR_VERBOSE(VERSION_ERROR);
                 return VERSION_ERROR;
             }

tests/api/test_tls13.c

@@ -6354,8 +6354,9 @@ int test_tls13_warning_alert_is_fatal(void)
 /* Regression test for a malformed HelloRetryRequest sent to a downgrade capable
  * (TLS 1.2 and TLS 1.3) client. Both crafted messages bear the HelloRetryRequest
  * sentinel Random but omit supported_versions, which an HRR MUST carry, so the
- * client must reject them with illegal_parameter (RFC 8446 Sec. 4.1.4/4.2)
- * rather than downgrading to TLS 1.2 and losing the HRR context:
+ * client must reject them with missing_extension (RFC 8446 Sec. 4.1.4/4.2.1/9.2
+ * - a missing mandatory extension) rather than downgrading to TLS 1.2 and losing
+ * the HRR context:
  *   1. has-extensions form: carries a server_name extension (recognized but not
  *      permitted in an HRR). Before the fix the downgrade reparsed it as a plain
  *      server_hello, where server_name is allowed, and the client wrongly sent
@@ -6446,16 +6447,18 @@ int test_tls13_hrr_recognized_ext_downgrade(void)
     ExpectIntEQ(test_memio_inject_message(&test_ctx, 1,
         (const char *)hrr_sni_no_sv, sizeof(hrr_sni_no_sv)), 0);
 
-    /* Handshake must fail with EXT_NOT_ALLOWED (server_name is not permitted in
-     * a HelloRetryRequest), not UNSUPPORTED_EXTENSION. */
+    /* Handshake must fail because the mandatory supported_versions extension
+     * is absent from the HelloRetryRequest. The HRR is rejected before the
+     * downgrade reparses the remaining (server_name) extension, so the result
+     * is the missing mandatory extension (INCOMPLETE_DATA). */
     ExpectIntEQ(wolfSSL_connect(ssl_c), -1);
     ExpectIntEQ(wolfSSL_get_error(ssl_c, -1),
-        WC_NO_ERR_TRACE(EXT_NOT_ALLOWED));
+        WC_NO_ERR_TRACE(INCOMPLETE_DATA));
 
-    /* RFC 8446 Sec. 4.2: the client MUST abort with illegal_parameter (47),
-     * not unsupported_extension (110). */
+    /* RFC 8446 Sec. 4.1.4/4.2: a mandatory-but-absent extension MUST abort with
+     * a missing_extension (109) alert. */
     ExpectIntEQ(wolfSSL_get_alert_history(ssl_c, &h), WOLFSSL_SUCCESS);
-    ExpectIntEQ(h.last_tx.code, illegal_parameter);
+    ExpectIntEQ(h.last_tx.code, missing_extension);
     ExpectIntEQ(h.last_tx.level, alert_fatal);
 
     wolfSSL_free(ssl_c);
@@ -6464,7 +6467,7 @@ int test_tls13_hrr_recognized_ext_downgrade(void)
     ctx_c = NULL;
 
     /* Scenario 2: HelloRetryRequest sentinel with no extensions field at all.
-     * Must also be rejected with illegal_parameter rather than downgraded. */
+     * Must also be rejected with missing_extension rather than downgraded. */
     XMEMSET(&test_ctx, 0, sizeof(test_ctx));
     XMEMSET(&h, 0, sizeof(h));
 
@@ -6474,14 +6477,15 @@ int test_tls13_hrr_recognized_ext_downgrade(void)
     ExpectIntEQ(test_memio_inject_message(&test_ctx, 1,
         (const char *)hrr_no_ext, sizeof(hrr_no_ext)), 0);
 
-    /* No offending extension is present here, so the client reports the message
-     * as an invalid HRR; the wire alert is still illegal_parameter. */
+    /* An HRR with no extensions field at all is still missing the mandatory
+     * supported_versions extension, so - consistently with scenario 1 - the
+     * client aborts with missing_extension (RFC 8446 4.2.1/9.2). */
     ExpectIntEQ(wolfSSL_connect(ssl_c), -1);
     ExpectIntEQ(wolfSSL_get_error(ssl_c, -1),
-        WC_NO_ERR_TRACE(INVALID_PARAMETER));
+        WC_NO_ERR_TRACE(INCOMPLETE_DATA));
 
     ExpectIntEQ(wolfSSL_get_alert_history(ssl_c, &h), WOLFSSL_SUCCESS);
-    ExpectIntEQ(h.last_tx.code, illegal_parameter);
+    ExpectIntEQ(h.last_tx.code, missing_extension);
     ExpectIntEQ(h.last_tx.level, alert_fatal);
 
     wolfSSL_free(ssl_c);