Skip to content

Add independent ciper and MAC algorithms negotiation for each direction #952

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yosuke-wolfssl wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add independent ciper and MAC algorithms negotiation for each direction #952

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 56 additions & 29 deletions src/internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,9 @@ static HandshakeInfo* HandshakeInfoNew(void* heap)
newHs->encryptId = ID_NONE;
Copy link
Copy Markdown
Member

@aidangarske aidangarske Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ [Info] HandshakeInfoNew skips peerMacSz/peerAeadMode initialization (safe due to WMEMSET but easy to miss) · Logic

The new initializer block sets peerEncryptId, peerMacId, and peerBlockSz, mirroring the existing local-side initializers. It does not set peerAeadMode or peerMacSz. Functionally this is fine because WMEMSET(newHs, 0, sizeof(HandshakeInfo)) zeros the struct first and the desired default for both fields is zero (non-AEAD, zero MAC size). This is purely a consistency/readability observation — future readers adding new peer fields may miss that WMEMSET already handles zero defaults, and the selective initialization becomes less symmetric with the local side. No action required for security.

Fix: Optional: for symmetry, omit the explicit = ID_NONE / = 0 lines that merely restate the memset defaults, or add the missing peerMacSz/peerAeadMode / macSz / aeadMode lines to document intent.

Note: Referenced line (src/internal.c:572-590) is outside the diff hunk. Comment anchored to nearest changed region.

newHs->macId = ID_NONE;
newHs->blockSz = MIN_BLOCK_SZ;
newHs->peerEncryptId = ID_NONE;
newHs->peerMacId = ID_NONE;
newHs->peerBlockSz = MIN_BLOCK_SZ;
newHs->eSz = (word32)sizeof(newHs->e);
newHs->xSz = (word32)sizeof(newHs->x);
#ifndef WOLFSSH_NO_DH_GEX_SHA256
Expand Down Expand Up @@ -4422,6 +4425,22 @@ static int DoKexInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)
ret = WS_MATCH_ENC_ALGO_E;
}
}
if (ret == WS_SUCCESS) {
Copy link
Copy Markdown
Member

@aidangarske aidangarske Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 [High] GenerateKeys skips both-direction MAC key derivation when only one direction is AEAD · Logic

GenerateKeys derives MAC key 'E' (C2S) and 'F' (S2C) inside a single if (!ssh->handshake->aeadMode) gate. Before this PR, DoKexInit enforced that the S2C algorithms had to match the already-selected C2S algorithms (the old code passed &algoId, 1 as the canned list when matching S2C enc and S2C MAC), so a single aeadMode flag correctly described both directions. After this PR, each direction is negotiated independently, and handshake->aeadMode now only reflects the S2C direction (peerAeadMode reflects C2S). When C2S is non-AEAD and S2C is AEAD — e.g. exactly the configuration exercised by the new TestIndependentAlgoNegotiation sub-test B (aes128-cbc C2S + aes256-gcm@openssh.com S2C) — handshake->aeadMode == 1 causes the guard to be false and NEITHER 'E' nor 'F' is generated. The C2S/peer HMAC key (peerKeys.macKey on the server) therefore stays all-zero from HandshakeInfoNew's WMEMSET, and the first MAC verification of an inbound C2S packet will use a zero key and fail, tearing down the session. The new regress test does not catch this because it only asserts handshake field values after DoKexInit and never drives GenerateKeys or the data path. The condition must gate each direction independently (e.g. generate 'E' when the C2S direction is non-AEAD, 'F' when the S2C direction is non-AEAD).

Fix: Split the gate so 'E' (C2S MAC key) is generated when the C2S direction is non-AEAD and 'F' (S2C MAC key) is generated when the S2C direction is non-AEAD. On the server: gate 'E' on !peerAeadMode (C2S) and 'F' on !aeadMode (S2C); on the client these roles are swapped. Then extend the regress test beyond DoKexInit to verify GenerateKeys derives the right keys (or a higher-level test that completes keying and exchanges data) to keep this regression from reappearing.

ssh->handshake->peerEncryptId = algoId;
Copy link
Copy Markdown

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 [High] DoKexInit stores peer/local algo info in wrong struct members for client endpoints · Logic errors

handshake->peerKeys/peerEncryptId/peerAeadMode/peerBlockSz/peerMacId/peerMacSz are now unconditionally filled from the C2S negotiation and handshake->keys etc. from S2C. However, GenerateKeys() (src/internal.c:2666-2673) and the ssh->peerKeys/ssh->keys…

Fix: Select which fields receive C2S vs S2C results based on ssh->ctx->side: clients must put S2C into peerKeys/peerEncryptId/... and C2S into…

ssh->handshake->peerAeadMode = AeadModeForId(algoId);
ssh->handshake->peerBlockSz = BlockSzForId(algoId);
ssh->handshake->peerKeys.encKeySz = KeySzForId(algoId);
if (!ssh->handshake->peerAeadMode) {
ssh->handshake->peerKeys.ivSz = ssh->handshake->peerBlockSz;
}
else {
/* Reaching here requires peerAeadMode==1, which requires an AEAD
* cipher ID, which requires WOLFSSH_NO_AES_GCM to be unset, which
* means WOLFSSH_NO_AEAD is also unset (see internal.h). */
ssh->handshake->peerKeys.ivSz = AEAD_NONCE_SZ;
ssh->handshake->peerMacSz = ssh->handshake->peerBlockSz;
}
Comment thread
yosuke-wolfssl marked this conversation as resolved.
}
Comment thread
yosuke-wolfssl marked this conversation as resolved.

/* Enc Algorithms - Server to Client */
if (ret == WS_SUCCESS) {
Expand All @@ -4430,7 +4449,13 @@ static int DoKexInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)
ret = GetNameList(list, &listSz, buf, len, &begin);
}
if (ret == WS_SUCCESS) {
algoId = MatchIdLists(side, list, listSz, &algoId, 1);
cannedAlgoNamesSz = AlgoListSz(ssh->algoListCipher);
cannedListSz = (word32)sizeof(cannedList);
ret = GetNameListRaw(cannedList, &cannedListSz,
(const byte*)ssh->algoListCipher, cannedAlgoNamesSz);
}
if (ret == WS_SUCCESS) {
algoId = MatchIdLists(side, list, listSz, cannedList, cannedListSz);
if (algoId == ID_UNKNOWN) {
WLOG(WS_LOG_DEBUG, "Unable to negotiate Encryption Algo S2C");
ret = WS_MATCH_ENC_ALGO_E;
Expand All @@ -4440,21 +4465,14 @@ static int DoKexInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)
ssh->handshake->encryptId = algoId;
ssh->handshake->aeadMode = AeadModeForId(algoId);
ssh->handshake->blockSz = BlockSzForId(algoId);
ssh->handshake->keys.encKeySz =
ssh->handshake->peerKeys.encKeySz =
KeySzForId(algoId);
ssh->handshake->keys.encKeySz = KeySzForId(algoId);
if (!ssh->handshake->aeadMode) {
ssh->handshake->keys.ivSz =
ssh->handshake->peerKeys.ivSz =
ssh->handshake->blockSz;
ssh->handshake->keys.ivSz = ssh->handshake->blockSz;
}
else {
#ifndef WOLFSSH_NO_AEAD
ssh->handshake->keys.ivSz =
ssh->handshake->peerKeys.ivSz =
AEAD_NONCE_SZ;
/* Same invariant: aeadMode==1 implies !WOLFSSH_NO_AEAD. */
ssh->handshake->keys.ivSz = AEAD_NONCE_SZ;
ssh->handshake->macSz = ssh->handshake->blockSz;
#endif
}
}

Expand All @@ -4464,7 +4482,7 @@ static int DoKexInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)
listSz = (word32)sizeof(list);
ret = GetNameList(list, &listSz, buf, len, &begin);
}
if (ret == WS_SUCCESS && !ssh->handshake->aeadMode) {
if (ret == WS_SUCCESS && !ssh->handshake->peerAeadMode) {
cannedAlgoNamesSz = AlgoListSz(ssh->algoListMac);
cannedListSz = (word32)sizeof(cannedList);
ret = GetNameListRaw(cannedList, &cannedListSz,
Expand All @@ -4476,6 +4494,11 @@ static int DoKexInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)
WLOG(WS_LOG_DEBUG, "Unable to negotiate MAC Algo C2S");
ret = WS_MATCH_MAC_ALGO_E;
}
else {
ssh->handshake->peerMacId = algoId;
ssh->handshake->peerMacSz = MacSzForId(algoId);
ssh->handshake->peerKeys.macKeySz = KeySzForId(algoId);
}
}
}

Expand All @@ -4486,17 +4509,21 @@ static int DoKexInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)
ret = GetNameList(list, &listSz, buf, len, &begin);
}
if (ret == WS_SUCCESS && !ssh->handshake->aeadMode) {
algoId = MatchIdLists(side, list, listSz, &algoId, 1);
if (algoId == ID_UNKNOWN) {
WLOG(WS_LOG_DEBUG, "Unable to negotiate MAC Algo S2C");
ret = WS_MATCH_MAC_ALGO_E;
}
else {
ssh->handshake->macId = algoId;
ssh->handshake->macSz = MacSzForId(algoId);
ssh->handshake->keys.macKeySz =
ssh->handshake->peerKeys.macKeySz =
KeySzForId(algoId);
cannedAlgoNamesSz = AlgoListSz(ssh->algoListMac);
cannedListSz = (word32)sizeof(cannedList);
ret = GetNameListRaw(cannedList, &cannedListSz,
(const byte*)ssh->algoListMac, cannedAlgoNamesSz);
if (ret == WS_SUCCESS) {
algoId = MatchIdLists(side, list, listSz, cannedList, cannedListSz);
if (algoId == ID_UNKNOWN) {
WLOG(WS_LOG_DEBUG, "Unable to negotiate MAC Algo S2C");
ret = WS_MATCH_MAC_ALGO_E;
}
else {
ssh->handshake->macId = algoId;
ssh->handshake->macSz = MacSzForId(algoId);
ssh->handshake->keys.macKeySz = KeySzForId(algoId);
}
}
}

Expand Down Expand Up @@ -6238,11 +6265,11 @@ static int DoNewKeys(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)
}

if (ret == WS_SUCCESS) {
ssh->peerEncryptId = ssh->handshake->encryptId;
ssh->peerMacId = ssh->handshake->macId;
ssh->peerBlockSz = ssh->handshake->blockSz;
ssh->peerMacSz = ssh->handshake->macSz;
ssh->peerAeadMode = ssh->handshake->aeadMode;
ssh->peerEncryptId = ssh->handshake->peerEncryptId;
ssh->peerMacId = ssh->handshake->peerMacId;
ssh->peerBlockSz = ssh->handshake->peerBlockSz;
ssh->peerMacSz = ssh->handshake->peerMacSz;
ssh->peerAeadMode = ssh->handshake->peerAeadMode;
WMEMCPY(&ssh->peerKeys, &ssh->handshake->peerKeys, sizeof(Keys));

switch (ssh->peerEncryptId) {
Expand Down
110 changes: 110 additions & 0 deletions tests/regress.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1989,6 +1989,36 @@ static word32 BuildKexInitPayload(WOLFSSH* ssh, const char* kexList,
return idx;
}

#if !defined(WOLFSSH_NO_AES_CBC) && !defined(WOLFSSH_NO_AES_CTR) \
&& !defined(WOLFSSH_NO_HMAC_SHA1) && !defined(WOLFSSH_NO_HMAC_SHA2_256)
/* Like BuildKexInitPayload but with explicit per-direction cipher/MAC lists. */
static word32 BuildKexInitPayloadFull(const char* kexList,
const char* keyList, const char* encC2S, const char* encS2C,
const char* macC2S, const char* macS2C,
byte firstPacketFollows, byte* out, word32 outSz)
{
word32 idx = 0;

AssertTrue(idx + COOKIE_SZ <= outSz);
WMEMSET(out + idx, 0, COOKIE_SZ);
idx += COOKIE_SZ;
idx = AppendString(out, outSz, idx, kexList);
idx = AppendString(out, outSz, idx, keyList);
idx = AppendString(out, outSz, idx, encC2S);
idx = AppendString(out, outSz, idx, encS2C);
idx = AppendString(out, outSz, idx, macC2S);
idx = AppendString(out, outSz, idx, macS2C);
idx = AppendString(out, outSz, idx, "none");
idx = AppendString(out, outSz, idx, "none");
idx = AppendString(out, outSz, idx, "");
idx = AppendString(out, outSz, idx, "");
idx = AppendByte(out, outSz, idx, firstPacketFollows);
idx = AppendUint32(out, outSz, idx, 0); /* reserved */

return idx;
}
#endif /* AES_CBC + AES_CTR + HMAC guards (BuildKexInitPayloadFull) */

typedef struct {
const char* description;
const char* kexList;
Expand Down Expand Up @@ -2107,6 +2137,79 @@ static void TestFirstPacketFollows(void)
TestFirstPacketFollowsSkipped();
}

#if !defined(WOLFSSH_NO_AES_CBC) && !defined(WOLFSSH_NO_AES_CTR) \
&& !defined(WOLFSSH_NO_HMAC_SHA1) && !defined(WOLFSSH_NO_HMAC_SHA2_256)
static void TestIndependentAlgoNegotiation(void)
Copy link
Copy Markdown
Member

@aidangarske aidangarske Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [Low] Test coverage asserts DoKexInit parse state only, not key derivation / data path for split-AEAD · Missing Tests

TestIndependentAlgoNegotiation only drives wolfSSH_TestDoKexInit and asserts fields on ssh->handshake (e.g. peerEncryptId, encryptId, aeadMode). It does not drive GenerateKeys, DoNewKeys, or end-to-end encryption/MAC with the negotiated mixed ciphers. As a result, the GenerateKeys bug above (not generating a MAC key for the non-AEAD direction when the other direction is AEAD) is not caught even though sub-test B constructs exactly the offending configuration. This elevates the risk of future regressions in the split-AEAD area.

Fix: Extend the test (or add a companion test) that completes keying for a split-AEAD configuration and verifies that the correct MAC keys are derived (non-zero macKey with the expected macKeySz for the non-AEAD side, zero-sized MAC key on the AEAD side). A round-trip send/receive with HMAC verification would be ideal.

{
WOLFSSH_CTX* ctx;
WOLFSSH* ssh;
byte payload[512];
word32 payloadSz;
word32 idx;

ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL);
AssertNotNull(ctx);

/* Sub-test A: different non-AEAD cipher and MAC per direction */
ssh = wolfSSH_new(ctx);
AssertNotNull(ssh);
AssertIntEQ(wolfSSH_SetAlgoListKex(ssh, FPF_KEX_GOOD), WS_SUCCESS);
AssertIntEQ(wolfSSH_SetAlgoListKey(ssh, FPF_KEY_GOOD), WS_SUCCESS);
AssertIntEQ(wolfSSH_SetAlgoListCipher(ssh,
"aes128-cbc,aes256-ctr"), WS_SUCCESS);
AssertIntEQ(wolfSSH_SetAlgoListMac(ssh,
"hmac-sha1,hmac-sha2-256"), WS_SUCCESS);
idx = 0;
payloadSz = BuildKexInitPayloadFull(
FPF_KEX_GOOD, FPF_KEY_GOOD,
"aes128-cbc", /* C2S enc */
"aes256-ctr", /* S2C enc */
"hmac-sha1", /* C2S MAC */
"hmac-sha2-256", /* S2C MAC */
0, payload, (word32)sizeof(payload));
(void)wolfSSH_TestDoKexInit(ssh, payload, payloadSz, &idx);
AssertNotNull(ssh->handshake);
AssertIntEQ(ssh->handshake->peerEncryptId, ID_AES128_CBC);
AssertIntEQ(ssh->handshake->encryptId, ID_AES256_CTR);
AssertIntEQ(ssh->handshake->peerMacId, ID_HMAC_SHA1);
AssertIntEQ(ssh->handshake->macId, ID_HMAC_SHA2_256);
AssertIntEQ(ssh->handshake->peerAeadMode, 0);
AssertIntEQ(ssh->handshake->aeadMode, 0);
wolfSSH_free(ssh);

#ifndef WOLFSSH_NO_AES_GCM
/* Sub-test B: AEAD S2C, non-AEAD C2S — MAC only negotiated for C2S */
ssh = wolfSSH_new(ctx);
AssertNotNull(ssh);
AssertIntEQ(wolfSSH_SetAlgoListKex(ssh, FPF_KEX_GOOD), WS_SUCCESS);
AssertIntEQ(wolfSSH_SetAlgoListKey(ssh, FPF_KEY_GOOD), WS_SUCCESS);
AssertIntEQ(wolfSSH_SetAlgoListCipher(ssh,
"aes128-cbc,aes256-gcm@openssh.com"), WS_SUCCESS);
AssertIntEQ(wolfSSH_SetAlgoListMac(ssh,
"hmac-sha1,hmac-sha2-256"), WS_SUCCESS);
idx = 0;
payloadSz = BuildKexInitPayloadFull(
FPF_KEX_GOOD, FPF_KEY_GOOD,
"aes128-cbc", /* C2S enc: non-AEAD */
"aes256-gcm@openssh.com", /* S2C enc: AEAD */
"hmac-sha1", /* C2S MAC: negotiated */
"hmac-sha2-256", /* S2C MAC: skipped (aeadMode) */
0, payload, (word32)sizeof(payload));
(void)wolfSSH_TestDoKexInit(ssh, payload, payloadSz, &idx);
AssertNotNull(ssh->handshake);
AssertIntEQ(ssh->handshake->peerEncryptId, ID_AES128_CBC);
AssertIntEQ(ssh->handshake->encryptId, ID_AES256_GCM);
AssertIntEQ(ssh->handshake->peerAeadMode, 0);
AssertIntEQ(ssh->handshake->aeadMode, 1);
AssertIntEQ(ssh->handshake->peerMacId, ID_HMAC_SHA1);
AssertIntEQ(ssh->handshake->macId, ID_NONE);
wolfSSH_free(ssh);
#endif /* !WOLFSSH_NO_AES_GCM */

wolfSSH_CTX_free(ctx);
}
#endif /* AES_CBC + AES_CTR + HMAC guards */

#endif /* first_packet_follows coverage guard */


Expand Down Expand Up @@ -2155,6 +2258,13 @@ int main(int argc, char** argv)
&& !defined(WOLFSSH_NO_CURVE25519_SHA256) \
&& !defined(WOLFSSH_NO_RSA_SHA2_256)
TestFirstPacketFollows();
#endif
#if !defined(WOLFSSH_NO_ECDH_SHA2_NISTP256) && !defined(WOLFSSH_NO_RSA) \
&& !defined(WOLFSSH_NO_CURVE25519_SHA256) \
&& !defined(WOLFSSH_NO_RSA_SHA2_256) \
&& !defined(WOLFSSH_NO_AES_CBC) && !defined(WOLFSSH_NO_AES_CTR) \
&& !defined(WOLFSSH_NO_HMAC_SHA1) && !defined(WOLFSSH_NO_HMAC_SHA2_256)
TestIndependentAlgoNegotiation();
#endif
TestDisconnectSetsDisconnectError();
TestClientBuffersIdempotent();
Expand Down
5 changes: 5 additions & 0 deletions wolfssh/internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -636,9 +636,14 @@ typedef struct HandshakeInfo {
byte encryptId;
byte macId;
byte aeadMode;
byte peerEncryptId;
byte peerMacId;
byte peerAeadMode;

byte blockSz;
byte macSz;
byte peerBlockSz;
byte peerMacSz;

Keys keys;
Keys peerKeys;
Expand Down
Loading