Keep negotiated key sizes when derivation fails

GenerateKeys() now wipes only the iv, encKey and macKey bytes when a
derivation fails, so the negotiated sizes stay put and a second call
cannot report success with no keys.

- tests/regress.c checks a failed server KEXDH_REPLY send wipes K
- AssertHandshakeSucceeds() takes a KEX algorithm for the per-KEX check
- KEX wipe failures name the algorithm and the endpoint

Issue: F-8845, F-11680, F-11681, F-8843, F-11682, F-14001

Author: ejohnstown

src/internal.c

@@ -3724,10 +3724,15 @@ static int GenerateKeys(WOLFSSH* ssh, byte hashId, byte doKeyPad)
     }
 #endif /* SHOW_SECRETS */
 
-    /* Do not keep a partial key set from a failed derivation. */
+    /* Do not keep a partial key set from a failed derivation. Keep the
+     * negotiated sizes so a retry cannot succeed with no keys. */
     if (ret != WS_SUCCESS && cK != NULL) {
-        WS_FORCEZERO(cK, sizeof(Keys));
-        WS_FORCEZERO(sK, sizeof(Keys));
+        WS_FORCEZERO(cK->iv, sizeof(cK->iv));
+        WS_FORCEZERO(cK->encKey, sizeof(cK->encKey));
+        WS_FORCEZERO(cK->macKey, sizeof(cK->macKey));
+        WS_FORCEZERO(sK->iv, sizeof(sK->iv));
+        WS_FORCEZERO(sK->encKey, sizeof(sK->encKey));
+        WS_FORCEZERO(sK->macKey, sizeof(sK->macKey));
     }
 
     return ret;

tests/regress.c

@@ -797,6 +797,7 @@ typedef struct DuplexEndpoint {
     /* What this endpoint put in its own plaintext KEXINIT */
     byte sentStrictKexMarker;
     byte sentExtInfoMarker;
+    byte failSendMsgId; /* nonzero: fail the send of a packet with this id */
 } DuplexEndpoint;
 
 typedef struct {
@@ -1505,6 +1506,12 @@ static int DuplexSend(WOLFSSH* ssh, void* buf, word32 sz, void* ctx)
         return WS_CBIO_ERR_GENERAL;
     }
 
+    if (endpoint->failSendMsgId != 0 &&
+            outputSz > UINT32_SZ + PAD_LENGTH_SZ &&
+            output[UINT32_SZ + PAD_LENGTH_SZ] == endpoint->failSendMsgId) {
+        return WS_CBIO_ERR_GENERAL;
+    }
+
     if (endpoint->mutator != NULL &&
             endpoint->mutator->enabled &&
             IsTerrapinMode(endpoint->mutator->mode) &&
@@ -1741,29 +1748,34 @@ static void RunKexReplyHandshake(KexReplyHarness* harness,
 }
 
 /* The shared secret K does not outlive the key exchange. */
-static void AssertKexSecretWiped(const WOLFSSH* ssh)
+static int KexSecretWiped(const WOLFSSH* ssh)
 {
     word32 i;
 
-    AssertIntEQ(ssh->kSz, 0);
+    if (ssh->kSz != 0)
+        return 0;
     for (i = 0; i < (word32)sizeof(ssh->k); i++) {
-        AssertIntEQ(ssh->k[i], 0);
+        if (ssh->k[i] != 0)
+            return 0;
     }
+    return 1;
 }
 
-static void AssertHandshakeSucceeds(const char* keyAlgo, const char* keyPath)
+/* A NULL kexAlgo uses the harness default. */
+static void AssertHandshakeSucceedsKex(const char* kexAlgo,
+        const char* keyAlgo, const char* keyPath)
 {
     KexReplyHarness harness;
     KexReplyRunResult result;
 
-    InitKexReplyHarnessEx(&harness, keyAlgo, keyPath, 0,
+    InitKexReplyHarnessKex(&harness, kexAlgo, keyAlgo, keyPath, 0,
             REGRESS_MUTATE_SIG_NAME, NULL, 0);
     RunKexReplyHandshake(&harness, &result);
 
     AssertTrue(result.clientSuccess);
     AssertTrue(result.serverSuccess);
-    AssertKexSecretWiped(harness.client);
-    AssertKexSecretWiped(harness.server);
+    AssertTrue(KexSecretWiped(harness.client));
+    AssertTrue(KexSecretWiped(harness.server));
     AssertIntEQ(harness.mutator.mutatedPackets, 0);
     AssertIntEQ(harness.client->connectState, CONNECT_SERVER_CHANNEL_REQUEST_DONE);
     AssertIntEQ(harness.server->acceptState, ACCEPT_CLIENT_SESSION_ESTABLISHED);
@@ -1775,6 +1787,11 @@ static void AssertHandshakeSucceeds(const char* keyAlgo, const char* keyPath)
     FreeKexReplyHarness(&harness);
 }
 
+static void AssertHandshakeSucceeds(const char* keyAlgo, const char* keyPath)
+{
+    AssertHandshakeSucceedsKex(NULL, keyAlgo, keyPath);
+}
+
 static void AssertHandshakeRejectsMutatedReply(const char* keyAlgo,
         const char* keyPath, const char* replaceName, int expectedErr)
 {
@@ -2173,7 +2190,7 @@ static void AssertHandshakeRejectsCorruptedSig(const char* keyAlgo,
             result.clientErr != WS_WANT_WRITE);
     AssertIntEQ(result.clientErr, expectedErr);
     /* K was agreed before the verify failed. */
-    AssertKexSecretWiped(harness.client);
+    AssertTrue(KexSecretWiped(harness.client));
 
     FreeKexReplyHarness(&harness);
 }
@@ -2202,23 +2219,35 @@ static void TestKexSecretWipedPerKex(void)
 #endif
         NULL
     };
-    KexReplyHarness harness;
-    KexReplyRunResult result;
     word32 i;
 
     for (i = 0; kexAlgos[i] != NULL; i++) {
-        InitKexReplyHarnessKex(&harness, kexAlgos[i],
-                REGRESS_DEFAULT_KEY_ALGO, REGRESS_DEFAULT_KEY_PATH, 0,
-                REGRESS_MUTATE_SIG_NAME, NULL, 0);
-        RunKexReplyHandshake(&harness, &result);
+        printf("    KEX secret wipe with %s.\n", kexAlgos[i]);
+        AssertHandshakeSucceedsKex(kexAlgos[i],
+                REGRESS_DEFAULT_KEY_ALGO, REGRESS_DEFAULT_KEY_PATH);
+    }
+}
 
-        AssertTrue(result.clientSuccess);
-        AssertTrue(result.serverSuccess);
-        AssertKexSecretWiped(harness.client);
-        AssertKexSecretWiped(harness.server);
+/* The server wipes K when sending its KEXDH_REPLY fails after key
+ * agreement. */
+static void TestKexSecretWipedOnServerSendFail(void)
+{
+    KexReplyHarness harness;
+    KexReplyRunResult result;
 
-        FreeKexReplyHarness(&harness);
-    }
+    InitKexReplyHarness(&harness, REGRESS_DEFAULT_KEY_ALGO,
+            REGRESS_DEFAULT_KEY_PATH, 0, NULL);
+    harness.serverIo.failSendMsgId = MSGID_KEXDH_REPLY;
+    RunKexReplyHandshake(&harness, &result);
+
+    AssertFalse(result.serverSuccess);
+    AssertTrue(result.serverErr != WS_WANT_READ &&
+            result.serverErr != WS_WANT_WRITE);
+    /* H is hashed over K, so K was agreed. */
+    AssertTrue(harness.server->sessionIdSz > 0);
+    AssertTrue(KexSecretWiped(harness.server));
+
+    FreeKexReplyHarness(&harness);
 }
 
 #ifndef WOLFSSH_NO_RSA_SHA2_256
@@ -16839,12 +16868,26 @@ static void TestGenerateKeysSplit(void)
     AssertTrue(WMEMCMP(ssh->handshake->peerKeys.encKey,
                        ssh->handshake->keys.encKey, AES_128_KEY_SIZE) != 0);
 
-    /* A failed derivation leaves no key material behind. */
+    /* A failed derivation leaves no key material behind but keeps the
+     * negotiated sizes. */
     AssertTrue(wolfSSH_TestGenerateKeys(ssh, WC_HASH_TYPE_NONE) != WS_SUCCESS);
     WMEMSET(&zeroKeys, 0, sizeof(zeroKeys));
-    AssertTrue(WMEMCMP(&ssh->handshake->keys, &zeroKeys, sizeof(Keys)) == 0);
-    AssertTrue(WMEMCMP(&ssh->handshake->peerKeys, &zeroKeys,
-                       sizeof(Keys)) == 0);
+    AssertTrue(WMEMCMP(ssh->handshake->keys.iv, zeroKeys.iv,
+                       sizeof(zeroKeys.iv)) == 0);
+    AssertTrue(WMEMCMP(ssh->handshake->keys.encKey, zeroKeys.encKey,
+                       sizeof(zeroKeys.encKey)) == 0);
+    AssertTrue(WMEMCMP(ssh->handshake->keys.macKey, zeroKeys.macKey,
+                       sizeof(zeroKeys.macKey)) == 0);
+    AssertTrue(WMEMCMP(ssh->handshake->peerKeys.iv, zeroKeys.iv,
+                       sizeof(zeroKeys.iv)) == 0);
+    AssertTrue(WMEMCMP(ssh->handshake->peerKeys.encKey, zeroKeys.encKey,
+                       sizeof(zeroKeys.encKey)) == 0);
+    AssertTrue(WMEMCMP(ssh->handshake->peerKeys.macKey, zeroKeys.macKey,
+                       sizeof(zeroKeys.macKey)) == 0);
+    AssertIntEQ(ssh->handshake->peerKeys.encKeySz, AES_128_KEY_SIZE);
+    AssertIntEQ(ssh->handshake->peerKeys.macKeySz, WC_SHA_DIGEST_SIZE);
+    AssertIntEQ(ssh->handshake->keys.encKeySz, AES_256_KEY_SIZE);
+    AssertIntEQ(ssh->handshake->keys.macKeySz, WC_SHA256_DIGEST_SIZE);
 
     wolfSSH_free(ssh);
 
@@ -18727,6 +18770,7 @@ int main(int argc, char** argv)
     #endif
     TestKexDhReplyRejectsSigNameOverrun();
     TestKexSecretWipedPerKex();
+    TestKexSecretWipedOnServerSendFail();
     #ifdef REGRESS_TRUNC_KEX_ALGO
     TestKexDhReplyTruncatedFSendsDisconnect();
     TestKexDhInitTruncatedESendsDisconnect();