Address skoll: fix type-confused ephemeral-key free on KEX switch, cppcheck redundant assign, KEY_EXCHANGE/reconnect tests, rename PQC workflow

Author: aidangarske

.github/workflows/spdm-emu-pqc-test.yml

@@ -1,11 +1,12 @@
-name: SPDM Emulator PQC (ML-DSA) Test
+name: SPDM Emulator PQC Test
 
-# Post-quantum interop, mirroring the classical SPDM Emulator Integration Test
-# matrix (ubuntu 22.04/24.04 x64 + 24.04 aarch64, each with static and dynamic
-# memory). The wc_MlDsaKey context API wolfSPDM verifies with lands
-# post-v5.9.1-stable, so this job pins wolfSSL master. libspdm's ML-DSA is only
-# in its OpenSSL backend (the mbedtls backend stubs it out), so spdm-emu is
-# built with CRYPTO=openssl.
+# Post-quantum interop (ML-DSA signing + ML-KEM key exchange, and a full-PQ
+# combination), mirroring the classical SPDM Emulator Integration Test matrix
+# (ubuntu 22.04/24.04 x64 + 24.04 aarch64, each with static and dynamic memory).
+# The wc_MlDsaKey context API wolfSPDM verifies with lands post-v5.9.1-stable,
+# so this job pins wolfSSL master. libspdm's ML-DSA/ML-KEM are only in its
+# OpenSSL backend (the mbedtls backend stubs them out), so spdm-emu is built
+# with CRYPTO=openssl.
 
 on:
   push:
@@ -85,7 +86,7 @@ jobs:
           make -j"$(nproc)"
           make install
 
-      - name: Run unit tests (includes ML-DSA verify)
+      - name: Run unit tests (includes ML-DSA verify + ML-KEM decap)
         run: make check
         env:
           LD_LIBRARY_PATH: ${{ github.workspace }}/.libs:${{ github.workspace }}/src/.libs:${{ env.HOME }}/wolfssl-install/lib

src/spdm_context.c

@@ -300,6 +300,9 @@ int wolfSPDM_SetKeyExchangePref(WOLFSPDM_CTX* ctx, int advDhe, word16 kemMask)
     if (ctx == NULL) {
         return WOLFSPDM_E_INVALID_ARG;
     }
+    if (advDhe == 0 && kemMask == 0) {
+        return WOLFSPDM_E_INVALID_ARG;  /* must advertise at least one method */
+    }
 #ifndef WOLFSPDM_HAVE_MLKEM
     if (kemMask != 0) {
         return WOLFSPDM_E_INVALID_ARG;  /* ML-KEM not built in */

src/spdm_msg.c

@@ -79,14 +79,14 @@ int wolfSPDM_BuildNegotiateAlgorithms(WOLFSPDM_CTX* ctx, byte* buf, word32* bufS
     /* Fixed header (32 bytes) + up to 5 AlgStructs (4 bytes each) = 52. */
     SPDM_CHECK_BUILD_ARGS(ctx, buf, bufSz, 52);
 
-    advDhe = (ctx->kexAdvDhe != 0);
 #ifdef WOLFSPDM_HAVE_MLKEM
+    advDhe = (ctx->kexAdvDhe != 0);
     advKem = (ctx->spdmVersion >= SPDM_VERSION_14 && ctx->kexAdvKem != 0);
     if (!advDhe && !advKem) {
         advDhe = 1;  /* never advertise zero key-exchange methods */
     }
 #else
-    advDhe = 1;
+    advDhe = 1;  /* DHE is the only key-exchange method without ML-KEM */
 #endif
 
     XMEMSET(buf, 0, 52);
@@ -704,6 +704,16 @@ int wolfSPDM_ParseAlgorithms(WOLFSPDM_CTX* ctx, const byte* buf, word32 bufSz)
         ctx->flags.hasResponderPubKey = 0;
     }
 
+    /* Same hazard for the ephemeral key union: a prior handshake may have left a
+     * key live whose union member is named by the OLD ctx->kexType. Free it now,
+     * before kexType is reassigned below, so the free dispatches on the matching
+     * member (otherwise a reconnect that switches DHE<->ML-KEM would type-confuse
+     * the free in wolfSPDM_FreeEphemeralKey). */
+    if (ctx->flags.ephemeralKeyInit) {
+        wolfSPDM_FreeEphemeralKey(ctx);
+        ctx->flags.ephemeralKeyInit = 0;
+    }
+
     /* DSP0274 1.4 Table 20: PqcAsymSel (offset 20). Present from 1.4; earlier
      * versions leave these bytes reserved-zero. The spec caps the combined
      * bit count of BaseAsymSel and PqcAsymSel at one, so exactly one of the
@@ -1000,10 +1010,6 @@ int wolfSPDM_ParseKeyExchangeRsp(WOLFSPDM_CTX* ctx, const byte* buf, word32 bufS
     ctx->rspSessionId = SPDM_Get16LE(&buf[4]);
     ctx->sessionId = (word32)ctx->reqSessionId | ((word32)ctx->rspSessionId << 16);
 
-    /* Extract responder's ephemeral public key (offset 40 = 4+2+1+1+32) */
-    XMEMCPY(peerPubKeyX, &buf[40], WOLFSPDM_ECC_KEY_SIZE);
-    XMEMCPY(peerPubKeyY, &buf[88], WOLFSPDM_ECC_KEY_SIZE);
-
     signature = buf + sigOffset;
     rspVerifyData = buf + sigOffset + sigSize;
 
@@ -1052,6 +1058,9 @@ int wolfSPDM_ParseKeyExchangeRsp(WOLFSPDM_CTX* ctx, const byte* buf, word32 bufS
     else
 #endif
     if (ctx->kexType == WOLFSPDM_KEX_ECDHE) {
+        /* ExchangeData is the responder's ephemeral point X||Y at offset 40. */
+        XMEMCPY(peerPubKeyX, &buf[40], WOLFSPDM_ECC_KEY_SIZE);
+        XMEMCPY(peerPubKeyY, &buf[88], WOLFSPDM_ECC_KEY_SIZE);
         rc = wolfSPDM_ComputeSharedSecret(ctx, peerPubKeyX, peerPubKeyY);
     }
     if (rc != WOLFSPDM_SUCCESS) {

test/unit_test.c

@@ -795,6 +795,79 @@ static int test_mlkem_decapsulate(void)
     TEST_CTX_FREE();
     TEST_PASS();
 }
+
+/* Reconnect on a reused context switching key-exchange method must free the
+ * prior ephemeral key while ctx->kexType still names its union member (no
+ * type-confused free). Run under valgrind in CI to catch a mismatched free. */
+static int test_kex_reconnect_method_switch(void)
+{
+    byte ek[WOLFSPDM_MLKEM768_EK_SIZE];
+    byte rsp[72];
+    word32 ekSz = sizeof(ek);
+    word32 len;
+    TEST_CTX_SETUP();
+
+    printf("test_kex_reconnect_method_switch...\n");
+    ctx->spdmVersion = SPDM_VERSION_14;
+
+    /* Round 1 leaves a live ML-KEM ephemeral key. */
+    ctx->kemAlgSel = SPDM_KEM_ALGO_ML_KEM_768;
+    ASSERT_SUCCESS(wolfSPDM_GenerateMlKemKey(ctx, ek, &ekSz));
+    ASSERT_EQ(ctx->flags.ephemeralKeyInit, 1, "ML-KEM key live");
+    ASSERT_EQ(ctx->kexType, WOLFSPDM_KEX_MLKEM, "kexType MLKEM");
+
+    /* Reconnect negotiates ECDHE: ParseAlgorithms frees the live ML-KEM key
+     * (still matching kexType) before flipping to ECDHE. */
+    len = build_algorithms_14_kem(rsp, SPDM_DHE_ALGO_SECP384R1, 0);
+    ASSERT_SUCCESS(wolfSPDM_ParseAlgorithms(ctx, rsp, len));
+    ASSERT_EQ(ctx->kexType, WOLFSPDM_KEX_ECDHE, "switched to ECDHE");
+    ASSERT_EQ(ctx->flags.ephemeralKeyInit, 0, "stale ML-KEM key freed");
+
+    /* And the reverse: a live ECDHE key, then a reconnect negotiating ML-KEM. */
+    ASSERT_SUCCESS(wolfSPDM_GenerateEphemeralKey(ctx));
+    ASSERT_EQ(ctx->flags.ephemeralKeyInit, 1, "ECDHE key live");
+    len = build_algorithms_14_kem(rsp, 0, SPDM_KEM_ALGO_ML_KEM_768);
+    ASSERT_SUCCESS(wolfSPDM_ParseAlgorithms(ctx, rsp, len));
+    ASSERT_EQ(ctx->kexType, WOLFSPDM_KEX_MLKEM, "switched to ML-KEM");
+    ASSERT_EQ(ctx->flags.ephemeralKeyInit, 0, "stale ECDHE key freed");
+
+    TEST_CTX_FREE();
+    TEST_PASS();
+}
+
+/* KEY_EXCHANGE with ML-KEM places the encapsulation key ek as ExchangeData at
+ * offset 40; confirm it decodes as a valid ML-KEM-768 public key. */
+static int test_build_key_exchange_mlkem(void)
+{
+    byte buf[WOLFSPDM_KEX_REQ_BUF];
+    word32 bufSz = sizeof(buf);
+    MlKemKey check;
+    int checkInit = 0;
+    TEST_CTX_SETUP();
+
+    printf("test_build_key_exchange_mlkem...\n");
+    ctx->spdmVersion = SPDM_VERSION_14;
+    ctx->kexType = WOLFSPDM_KEX_MLKEM;
+    ctx->kemAlgSel = SPDM_KEM_ALGO_ML_KEM_768;
+
+    ASSERT_SUCCESS(wolfSPDM_BuildKeyExchange(ctx, buf, &bufSz));
+    ASSERT_EQ(buf[1], SPDM_KEY_EXCHANGE, "KEY_EXCHANGE code");
+    /* offset 40 = 4 hdr + 2 sessionId + 2 policy/rsvd + 32 random. */
+    ASSERT_EQ(wc_MlKemKey_Init(&check, WC_ML_KEM_768, NULL, INVALID_DEVID), 0,
+        "MlKemKey_Init");
+    checkInit = 1;
+    ASSERT_EQ(wc_MlKemKey_DecodePublicKey(&check, &buf[40],
+        WOLFSPDM_MLKEM768_EK_SIZE), 0, "ek decodes at offset 40");
+    /* 40 fixed + 1184 ek + 22 OpaqueData block. */
+    ASSERT_EQ(bufSz, 40u + WOLFSPDM_MLKEM768_EK_SIZE + 22u,
+        "ML-KEM KEY_EXCHANGE total size");
+
+    if (checkInit) {
+        wc_MlKemKey_Free(&check);
+    }
+    TEST_CTX_FREE();
+    TEST_PASS();
+}
 #endif /* WOLFSPDM_HAVE_MLKEM */
 
 #ifdef WOLFSPDM_HAVE_CHUNK
@@ -2824,6 +2897,8 @@ int main(void)
     test_negotiate_algorithms_kem_build();
     test_parse_algorithms_kem_select();
     test_mlkem_decapsulate();
+    test_kex_reconnect_method_switch();
+    test_build_key_exchange_mlkem();
 #endif
 #ifdef WOLFSPDM_HAVE_CHUNK
     test_chunk_reassemble();