Add independent ciper and MAC algorithms negotiation for each direction

[yosuke-wolfssl]

This PR adds independent ciper and MAC algorithms negotiation for each direction to comply with RFC 4253 section 7.1.

• Add peerEncryptId, peerMacId, peerAeadMode, peerBlockSz, peerMacSz into HandshakeInfo.
• Fix DoKexInit() so that this matches each algorithm independently and store the results into each member in HandshakeInfo.
• Fix DoNewKeys() so that this updates peer algorithm info like ssh->peerEncryptId with ssh->handshake->peerEncryptId and so on.

Also, new regress test for the case is added as well.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds RFC 4253 §7.1-compliant, per-direction cipher/MAC negotiation by tracking and applying independent peer vs local (C2S vs S2C) algorithm selections during KEXINIT/NEWKEYS processing, with a new regression test covering mixed-direction negotiation scenarios.

Changes:

• Extend HandshakeInfo to store peer (opposite direction) cipher/MAC/AEAD and sizes independently.
• Update DoKexInit() to negotiate cipher/MAC independently per direction and populate the new handshake fields.
• Update DoNewKeys() to copy negotiated peer-direction algorithm selections from the handshake into the session, and add a regression test validating negotiation outcomes.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
wolfssh/internal.h	Adds per-direction (peer) negotiated cipher/MAC/AEAD fields to HandshakeInfo.
src/internal.c	Implements independent per-direction algorithm matching and propagates peer settings on NEWKEYS.
tests/regress.c	Adds a regression test ensuring negotiation differs per direction and handles AEAD vs non-AEAD correctly.

Comments suppressed due to low confidence (1)

src/internal.c:4486

• The KEXINIT parsing now duplicates the same 'convert configured algo name-list into ID list' pattern multiple times (cipher S2C, MAC C2S, MAC S2C). Consider extracting a small helper to build cannedList/cannedListSz from ssh->algoListCipher / ssh->algoListMac to reduce repetition and the chance of future divergence between the per-direction negotiation blocks.

    if (ret == WS_SUCCESS && !ssh->handshake->peerAeadMode) {
        cannedAlgoNamesSz = AlgoListSz(ssh->algoListMac);
        cannedListSz = (word32)sizeof(cannedList);
        ret = GetNameListRaw(cannedList, &cannedListSz,
                (const byte*)ssh->algoListMac, cannedAlgoNamesSz);

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #952

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

[wolfSSL-Fenrir-bot]

🔴 [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…

[aidangarske]

Skoll Code Review

Scan type: review-security
Overall recommendation: REQUEST_CHANGES
Findings: 3 total — 3 posted, 0 skipped
3 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [High] GenerateKeys skips both-direction MAC key derivation when only one direction is AEAD — src/internal.c:2696-2709
• [Low] Test coverage asserts DoKexInit parse state only, not key derivation / data path for split-AEAD — tests/regress.c:2142-2210
• [Info] HandshakeInfoNew skips peerMacSz/peerAeadMode initialization (safe due to WMEMSET but easy to miss) — src/internal.c:572-590

Review generated by Skoll

[aidangarske]

🔴 [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.

[aidangarske]

🔵 [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.

[aidangarske]

⚪ [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.