Merge pull request #10299 from Frauschi/pqc_key_share_fix

Fix PQC key exchange with multiple KEM key shares

Author: dgarske

src/internal.c

@@ -35430,16 +35430,14 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx,
     int NamedGroupIsPqc(int group)
     {
         switch (group) {
-        #ifndef WOLFSSL_NO_ML_KEM
+            /* FIPS 204 ML-KEM */
             case WOLFSSL_ML_KEM_512:
             case WOLFSSL_ML_KEM_768:
             case WOLFSSL_ML_KEM_1024:
-        #endif
-        #ifdef WOLFSSL_MLKEM_KYBER
+            /* Kyber Round 3 */
             case WOLFSSL_KYBER_LEVEL1:
             case WOLFSSL_KYBER_LEVEL3:
             case WOLFSSL_KYBER_LEVEL5:
-        #endif
                 return 1;
             default:
                 return 0;
@@ -35449,17 +35447,12 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx,
     /* Returns 1 when the given group is a PQC hybrid group, 0 otherwise. */
     int NamedGroupIsPqcHybrid(int group)
     {
-    #if defined(WOLFSSL_PQC_HYBRIDS) || defined(WOLFSSL_EXTRA_PQC_HYBRIDS) || \
-        defined(WOLFSSL_MLKEM_KYBER)
-
         switch (group) {
-        #ifndef WOLFSSL_NO_ML_KEM
-            #ifdef WOLFSSL_PQC_HYBRIDS
+            /* Standardized hybrids */
             case WOLFSSL_SECP256R1MLKEM768:
             case WOLFSSL_X25519MLKEM768:
             case WOLFSSL_SECP384R1MLKEM1024:
-            #endif /* WOLFSSL_PQC_HYBRIDS */
-            #ifdef WOLFSSL_EXTRA_PQC_HYBRIDS
+            /* Additional experimental hybrids */
             case WOLFSSL_SECP256R1MLKEM512:
             case WOLFSSL_SECP384R1MLKEM768:
             case WOLFSSL_SECP521R1MLKEM1024:
@@ -35470,25 +35463,18 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx,
             case WOLFSSL_P384_ML_KEM_768_OLD:
             case WOLFSSL_P521_ML_KEM_1024_OLD:
             #endif
-            #endif /* WOLFSSL_EXTRA_PQC_HYBRIDS */
-        #endif
-        #ifdef WOLFSSL_MLKEM_KYBER
+            /* Kyber round 3 hybrids */
             case WOLFSSL_P256_KYBER_LEVEL3:
             case WOLFSSL_X25519_KYBER_LEVEL3:
             case WOLFSSL_P256_KYBER_LEVEL1:
             case WOLFSSL_P384_KYBER_LEVEL3:
             case WOLFSSL_P521_KYBER_LEVEL5:
             case WOLFSSL_X25519_KYBER_LEVEL1:
             case WOLFSSL_X448_KYBER_LEVEL3:
-        #endif
                 return 1;
             default:
                 return 0;
         }
-    #else
-        (void)group;
-        return 0;
-    #endif
     }
 #endif /* WOLFSSL_HAVE_MLKEM */
 

src/tls.c

@@ -9082,16 +9082,18 @@ static void TLSX_KeyShare_FreeAll(KeyShareEntry* list, void* heap)
             wc_curve448_free((curve448_key*)current->key);
 #endif
         }
-#ifdef WOLFSSL_HAVE_MLKEM
         else if (WOLFSSL_NAMED_GROUP_IS_PQC(current->group)) {
+#ifdef WOLFSSL_HAVE_MLKEM
             wc_KyberKey_Free((KyberKey*)current->key);
         #ifndef WOLFSSL_TLSX_PQC_MLKEM_STORE_OBJ
             if (current->privKey != NULL) {
                 ForceZero(current->privKey, current->privKeyLen);
             }
         #endif
+#endif
         }
         else if (WOLFSSL_NAMED_GROUP_IS_PQC_HYBRID(current->group)) {
+#ifdef WOLFSSL_HAVE_MLKEM
             int ecc_group = 0;
             findEccPqc(&ecc_group, NULL, NULL, current->group);
 
@@ -9129,8 +9131,8 @@ static void TLSX_KeyShare_FreeAll(KeyShareEntry* list, void* heap)
                 wc_ecc_free((ecc_key*)current->key);
             #endif
             }
-        }
 #endif
+        }
         else {
 #ifdef HAVE_ECC
         #if defined(WC_ECC_NONBLOCK) && defined(WOLFSSL_ASYNC_CRYPT_SW) && \
@@ -10304,42 +10306,16 @@ static int TLSX_KeyShareEntry_Parse(const WOLFSSL* ssl, const byte* input,
         *seenGroupsCnt = i + 1;
     }
 
-#if defined(WOLFSSL_HAVE_MLKEM)
-    if ((WOLFSSL_NAMED_GROUP_IS_PQC(group)
-    #if !defined(WOLFSSL_ASYNC_CRYPT)
-         || WOLFSSL_NAMED_GROUP_IS_PQC_HYBRID(group)
-    #endif
-        ) && ssl->options.side == WOLFSSL_SERVER_END) {
-        /* When handling a key share containing a KEM public key on the server
-         * end, we have to perform the encapsulation immediately in order to
-         * send the resulting ciphertext back to the client in the ServerHello
-         * message. As the public key is not stored and we do not modify it, we
-         * don't have to create a copy of it.
-         * In case of a hybrid key exchange, the ECDH part is also performed
-         * immediately (to not split the generation of the master secret).
-         * Hence, we also don't have to store this public key either.
-         *
-         * When WOLFSSL_ASYNC_CRYPT is enabled, this handling is not possible
-         * for the hybrid case, as the ECC part is performed asynchronously,
-         * requiring the key share data to be stored.
-         */
-        ke = (byte *)&input[offset];
-    } else
-#endif
-    {
-        /* Store a copy in the key share object. */
-        ke = (byte*)XMALLOC(keLen, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
-        if (ke == NULL)
-            return MEMORY_E;
-        XMEMCPY(ke, &input[offset], keLen);
-    }
+    /* Store a copy in the key share object. */
+    ke = (byte*)XMALLOC(keLen, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
+    if (ke == NULL)
+        return MEMORY_E;
+    XMEMCPY(ke, &input[offset], keLen);
 
     /* Populate a key share object in the extension. */
     ret = TLSX_KeyShare_Use(ssl, group, keLen, ke, kse, extensions);
     if (ret != 0) {
-        if (ke != &input[offset]) {
-            XFREE(ke, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
-        }
+        XFREE(ke, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
         return ret;
     }
 
@@ -11066,46 +11042,8 @@ int TLSX_KeyShare_Use(const WOLFSSL* ssl, word16 group, word16 len, byte* data,
             return ret;
     }
 
-
-#if defined(WOLFSSL_HAVE_MLKEM) && !defined(WOLFSSL_MLKEM_NO_ENCAPSULATE)
-    if (ssl->options.side == WOLFSSL_SERVER_END &&
-            WOLFSSL_NAMED_GROUP_IS_PQC(group)) {
-        if (TLSX_IsGroupSupported(group)) {
-            ret = TLSX_KeyShare_HandlePqcKeyServer((WOLFSSL*)ssl,
-                                                   keyShareEntry,
-                                                   data, len,
-                                                   ssl->arrays->preMasterSecret,
-                                                   &ssl->arrays->preMasterSz);
-            if (ret != 0)
-                return ret;
-        }
-        else {
-            XFREE(keyShareEntry->ke, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
-            keyShareEntry->ke = NULL;
-            keyShareEntry->keLen = 0;
-        }
-    }
-    else
-#if !defined(WOLFSSL_ASYNC_CRYPT)
-    if (ssl->options.side == WOLFSSL_SERVER_END &&
-             WOLFSSL_NAMED_GROUP_IS_PQC_HYBRID(group)) {
-        if (TLSX_IsGroupSupported(group)) {
-            ret = TLSX_KeyShare_HandlePqcHybridKeyServer((WOLFSSL*)ssl,
-                                                         keyShareEntry,
-                                                         data, len);
-            if (ret != 0)
-                return ret;
-        }
-        else {
-            XFREE(keyShareEntry->ke, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
-            keyShareEntry->ke = NULL;
-            keyShareEntry->keLen = 0;
-        }
-    }
-    else
-#endif
-#endif
     if (data != NULL) {
+        /* Store the peer data in the key share object. */
         XFREE(keyShareEntry->ke, ssl->heap, DYNAMIC_TYPE_PUBLIC_KEY);
         keyShareEntry->ke = data;
         keyShareEntry->keLen = len;
@@ -11293,7 +11231,10 @@ static int TLSX_KeyShare_GroupRank(const WOLFSSL* ssl, int group)
     byte numGroups;
 
     if (ssl->numGroups == 0) {
-        return 0;
+        /* If the user didn't specify a group list with a preferred order,
+         * use the internal preferred group list. */
+        groups = preferredGroup;
+        numGroups = PREFERRED_GROUP_SZ;
     }
     else {
         groups = ssl->group;
@@ -11590,9 +11531,7 @@ int TLSX_KeyShare_Choose(const WOLFSSL *ssl, TLSX* extensions,
 
     /* Use server's preference order. */
     for (clientKSE = list; clientKSE != NULL; clientKSE = clientKSE->next) {
-        if ((clientKSE->ke == NULL) &&
-            (!WOLFSSL_NAMED_GROUP_IS_PQC(clientKSE->group)) &&
-            (!WOLFSSL_NAMED_GROUP_IS_PQC_HYBRID(clientKSE->group)))
+        if (clientKSE->ke == NULL)
             continue;
 
 #ifdef WOLFSSL_SM2
@@ -11681,26 +11620,17 @@ int TLSX_KeyShare_Setup(WOLFSSL *ssl, KeyShareEntry* clientKSE)
         return ret;
 
     if (clientKSE->key == NULL) {
-#ifdef WOLFSSL_HAVE_MLKEM
-        if (WOLFSSL_NAMED_GROUP_IS_PQC(clientKSE->group)
-    #if !defined(WOLFSSL_ASYNC_CRYPT)
-            || WOLFSSL_NAMED_GROUP_IS_PQC_HYBRID(clientKSE->group)
-    #endif
-        ) {
-            /* Going to need the public key (AKA ciphertext). */
-            serverKSE->pubKey = clientKSE->pubKey;
-            clientKSE->pubKey = NULL;
-            serverKSE->pubKeyLen = clientKSE->pubKeyLen;
-            clientKSE->pubKeyLen = 0;
+#if defined(WOLFSSL_HAVE_MLKEM) && !defined(WOLFSSL_MLKEM_NO_ENCAPSULATE)
+        if (WOLFSSL_NAMED_GROUP_IS_PQC(clientKSE->group)) {
+            ret = TLSX_KeyShare_HandlePqcKeyServer(ssl, serverKSE,
+                    clientKSE->ke, clientKSE->keLen,
+                    ssl->arrays->preMasterSecret, &ssl->arrays->preMasterSz);
         }
-        else
-    #if defined(WOLFSSL_ASYNC_CRYPT)
-        if (WOLFSSL_NAMED_GROUP_IS_PQC_HYBRID(clientKSE->group)) {
+        else if (WOLFSSL_NAMED_GROUP_IS_PQC_HYBRID(clientKSE->group)) {
             ret = TLSX_KeyShare_HandlePqcHybridKeyServer(ssl, serverKSE,
                     clientKSE->ke, clientKSE->keLen);
         }
         else
-    #endif
 #endif
         {
             ret = TLSX_KeyShare_GenKey(ssl, serverKSE);

tests/api/test_tls13.c

@@ -2667,6 +2667,101 @@ int test_tls13_pq_groups(void)
     return EXPECT_RESULT();
 }
 
+/* Regression test handling multiple PQC key shares in the ClientHello.
+ *
+ * Previously, the server eagerly ran KEM encapsulation on every PQC/hybrid
+ * key_share entry while parsing the ClientHello, clobbering
+ * ssl->arrays->preMasterSecret with whichever entry was parsed last.
+ * When the ClientHello offers both SecP384R1_MLKEM1024 and pure
+ * ML_KEM_1024, the resulting handshake either produces keys that the
+ * client cannot decrypt (hybrid chosen, pure-ML-KEM secret written) or
+ * trips a BUFFER_E inside the second encapsulation. Either ordering
+ * causes the handshake to fail.
+ *
+ * The test runs a memio TLS 1.3 handshake for both orderings and
+ * expects the handshake to complete successfully with the hybrid group
+ * selected (higher server preference rank). */
+#if defined(WOLFSSL_TLS13) && defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) && \
+    defined(WOLFSSL_HAVE_MLKEM) && defined(WOLFSSL_PQC_HYBRIDS) && \
+    !defined(WOLFSSL_TLS_NO_MLKEM_STANDALONE) && \
+    !defined(WOLFSSL_NO_ML_KEM_1024) && \
+    !defined(WOLFSSL_MLKEM_NO_ENCAPSULATE) && \
+    !defined(WOLFSSL_MLKEM_NO_DECAPSULATE) && \
+    !defined(WOLFSSL_MLKEM_NO_MAKE_KEY) && \
+    defined(HAVE_ECC) && (defined(HAVE_ECC384) || defined(HAVE_ALL_CURVES)) && \
+    ECC_MIN_KEY_SZ <= 384 && \
+    !defined(NO_WOLFSSL_CLIENT) && !defined(NO_WOLFSSL_SERVER)
+#define TEST_TLS13_MULTI_PQC_KEY_SHARE_ENABLED
+
+/* Run one TLS 1.3 memio handshake where the client offers both
+ * WOLFSSL_SECP384R1MLKEM1024 and WOLFSSL_ML_KEM_1024 in the key_share
+ * extension, in the order dictated by `hybridFirst`. */
+static int test_tls13_multi_pqc_key_share_once(int hybridFirst)
+{
+    EXPECT_DECLS;
+    WOLFSSL_CTX *ctx_c = NULL;
+    WOLFSSL_CTX *ctx_s = NULL;
+    WOLFSSL *ssl_c = NULL;
+    WOLFSSL *ssl_s = NULL;
+    struct test_memio_ctx test_ctx;
+
+    XMEMSET(&test_ctx, 0, sizeof(test_ctx));
+    ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s,
+        wolfTLSv1_3_client_method, wolfTLSv1_3_server_method), 0);
+
+    wolfSSL_set_verify(ssl_c, WOLFSSL_VERIFY_NONE, NULL);
+    wolfSSL_set_verify(ssl_s, WOLFSSL_VERIFY_NONE, NULL);
+
+    /* Force the client to include both PQC key shares in the ClientHello
+     * by calling UseKeyShare twice. The order of the UseKeyShare calls
+     * determines the order of the entries in the key_share extension. */
+    if (hybridFirst) {
+        ExpectIntEQ(wolfSSL_UseKeyShare(ssl_c, WOLFSSL_SECP384R1MLKEM1024),
+            WOLFSSL_SUCCESS);
+        ExpectIntEQ(wolfSSL_UseKeyShare(ssl_c, WOLFSSL_ML_KEM_1024),
+            WOLFSSL_SUCCESS);
+    }
+    else {
+        ExpectIntEQ(wolfSSL_UseKeyShare(ssl_c, WOLFSSL_ML_KEM_1024),
+            WOLFSSL_SUCCESS);
+        ExpectIntEQ(wolfSSL_UseKeyShare(ssl_c, WOLFSSL_SECP384R1MLKEM1024),
+            WOLFSSL_SUCCESS);
+    }
+
+    ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
+
+    /* The server ranks SecP384R1_MLKEM1024 higher than ML_KEM_1024, so
+     * the hybrid group must be selected regardless of client ordering. */
+    ExpectStrEQ(wolfSSL_get_curve_name(ssl_s), "SecP384r1MLKEM1024");
+    ExpectStrEQ(wolfSSL_get_curve_name(ssl_c), "SecP384r1MLKEM1024");
+
+    wolfSSL_free(ssl_c);
+    wolfSSL_free(ssl_s);
+    wolfSSL_CTX_free(ctx_c);
+    wolfSSL_CTX_free(ctx_s);
+    return EXPECT_RESULT();
+}
+#endif /* TEST_TLS13_MULTI_PQC_KEY_SHARE_ENABLED */
+
+int test_tls13_multi_pqc_key_share(void)
+{
+    EXPECT_DECLS;
+#ifdef TEST_TLS13_MULTI_PQC_KEY_SHARE_ENABLED
+    /* Hybrid first, then pure ML-KEM: pre-fix the server selected the
+     * hybrid but had overwritten preMasterSecret with the pure-KEM
+     * result, producing 32-byte KE Secret instead of 80 and causing the
+     * client to fail to decrypt the server's first encrypted record. */
+    ExpectIntEQ(test_tls13_multi_pqc_key_share_once(1), TEST_SUCCESS);
+
+    /* Pure ML-KEM first, then hybrid: pre-fix the server tripped
+     * BUFFER_E inside the second encapsulation because preMasterSz was
+     * left at 32 from the first call, and the hybrid handler then
+     * overflowed the preMasterSecret buffer. */
+    ExpectIntEQ(test_tls13_multi_pqc_key_share_once(0), TEST_SUCCESS);
+#endif
+    return EXPECT_RESULT();
+}
+
 #if defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES) &&                           \
     defined(WOLFSSL_EARLY_DATA) && defined(HAVE_SESSION_TICKET)
 static int test_tls13_read_until_write_ok(WOLFSSL* ssl, void* buf, int bufLen)

tests/api/test_tls13.h

@@ -29,6 +29,7 @@ int test_tls13_cipher_suites(void);
 int test_tls13_bad_psk_binder(void);
 int test_tls13_rpk_handshake(void);
 int test_tls13_pq_groups(void);
+int test_tls13_multi_pqc_key_share(void);
 int test_tls13_early_data(void);
 int test_tls13_same_ch(void);
 int test_tls13_hrr_different_cs(void);
@@ -68,6 +69,7 @@ int test_tls13_cert_with_extern_psk_sh_confirms_resumption(void);
     TEST_DECL_GROUP("tls13", test_tls13_bad_psk_binder),        \
     TEST_DECL_GROUP("tls13", test_tls13_rpk_handshake),         \
     TEST_DECL_GROUP("tls13", test_tls13_pq_groups),             \
+    TEST_DECL_GROUP("tls13", test_tls13_multi_pqc_key_share),   \
     TEST_DECL_GROUP("tls13", test_tls13_early_data),            \
     TEST_DECL_GROUP("tls13", test_tls13_same_ch),               \
     TEST_DECL_GROUP("tls13", test_tls13_hrr_different_cs),      \

wolfssl/ssl.h

@@ -4779,9 +4779,8 @@ enum {
     WOLFSSL_FFDHE_8192    = 260,
     WOLFSSL_FFDHE_END     = 511,
 
-#ifdef WOLFSSL_HAVE_MLKEM
+    /* Post Quantum KEM code points */
 
-#ifdef WOLFSSL_MLKEM_KYBER
     /* Old code points to keep compatibility with Kyber Round 3.
      * Taken from OQS's openssl provider, see:
      * https://github.com/open-quantum-safe/oqs-provider/blob/main/oqs-template/
@@ -4798,8 +4797,7 @@ enum {
     WOLFSSL_X448_KYBER_LEVEL3     = 12176,
     WOLFSSL_X25519_KYBER_LEVEL3   = 25497,
     WOLFSSL_P256_KYBER_LEVEL3     = 25498,
-#endif /* WOLFSSL_MLKEM_KYBER */
-#ifndef WOLFSSL_NO_ML_KEM
+
     /* Taken from draft-ietf-tls-mlkem, see:
      * https://datatracker.ietf.org/doc/draft-ietf-tls-mlkem/
      */
@@ -4828,8 +4826,6 @@ enum {
     WOLFSSL_SECP521R1MLKEM1024    = 12109,
     WOLFSSL_X25519MLKEM512        = 12214,
     WOLFSSL_X448MLKEM768          = 12215,
-#endif /* WOLFSSL_NO_ML_KEM */
-#endif /* WOLFSSL_HAVE_MLKEM */
     WOLF_ENUM_DUMMY_LAST_ELEMENT(SSL_H)
 };