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.

[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

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #952

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 2
2 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]

Fenrir Automated Review — PR #952

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

[yosuke-wolfssl]

Hello @aidangarske ,

I fixed the issues you mentioned.
Could you please review this again ?
Thank you for your time.

[aidangarske]

Skoll Multi-Scan Review

Modes: review + review-security + audit
Overall recommendation: COMMENT
Findings: 4 total — 4 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [Medium] [audit] DoKexInit client-side branch (WOLFSSH_ENDPOINT_CLIENT alias mapping) lacks a direct asymmetric-algo test — src/internal.c:4313-4326
• [Low] [review+review-security] Asymmetric AEAD reset between C2S and S2C branches in DoKexInit — src/internal.c:4427-4478
• [Low] [review-security] Pointer aliases used after first ret==WS_SUCCESS block may trip strict-uninit warnings — src/internal.c:4253-4326,4427-4527
• [Info] [review-security] Removed redundant ID_NONE/MSGID_NONE initializations in HandshakeInfoNew rely on enum values being 0 — src/internal.c:572-576

Review generated by Skoll

[yosuke-wolfssl]

Hi @aidangarske ,

Sorry to take your time.
Can you review again please ?

[aidangarske]

Skoll Multi-Scan Review

Modes: audit + review + review-security
Overall recommendation: COMMENT
Findings: 7 total — 7 posted, 0 skipped
6 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [Medium] [audit] Asymmetric S2C-only encryption-algo mismatch error path is untested — src/internal.c:4448-4466
• [Medium] [audit] Asymmetric S2C-only MAC-algo mismatch error path is untested — src/internal.c:4510-4533
• [Medium] [review] TestIndependentAlgoNegotiation tests do not verify all ID/Sz fields are reset on each ssh instance — tests/regress.c:2161-2332
• [Low] [audit] wolfSSH_TestGenerateKeys NULL-argument validation is untested — src/internal.c:17962-17967
• [Low] [review] Missing blank line between TestGenerateKeysSplit and TestGenerateKeysSplitClient — tests/regress.c:2446-2447
• [Low] [review] New tests silently ignore wolfSSH_TestDoKexInit return without explanation — tests/regress.c:2191,2227,2278,2313,2366,2420,2479,2532
• [Low] [review] DoKexInit reads side from ssh->ctx->side a second time after caching it in side — src/internal.c:4622,4642,4666,4673

Review generated by Skoll

[aidangarske]

🟠 [Medium] Asymmetric S2C-only encryption-algo mismatch error path is untested · path

Pre-PR, the S2C encryption algorithm was matched against the C2S-chosen algorithm only (MatchIdLists(side, list, listSz, &algoId, 1)). Post-PR, S2C is matched independently against ssh->algoListCipher via cannedList. This introduces a NEW failure mode: C2S successfully matches but S2C does not (or vice versa) — yielding WS_MATCH_ENC_ALGO_E from the S2C block. The new tests in TestIndependentAlgoNegotiation/...Client only cover the success paths for asymmetric cipher selection; no test exercises asymmetric mismatch where one direction agrees and the other fails. Grep confirms WS_MATCH_ENC_ALGO_E has zero references in tests/.

Fix: Add a regress.c test that builds a kex_init payload where the C2S cipher list contains an algorithm in the local algoListCipher but the S2C cipher list does not (e.g., local accepts aes128-cbc,aes256-ctr; payload offers C2S=aes128-cbc, S2C=3des-cbc). Verify wolfSSH_TestDoKexInit returns WS_MATCH_ENC_ALGO_E. Mirror the test for the inverse case (C2S fails, S2C succeeds).

[aidangarske]

🟠 [Medium] Asymmetric S2C-only MAC-algo mismatch error path is untested · path

The S2C MAC negotiation was rewritten from MatchIdLists(side, list, listSz, &algoId, 1) (which forced S2C to equal C2S) to an independent match against ssh->algoListMac. The new failure path WS_MATCH_MAC_ALGO_E can now trigger when C2S MAC matches but S2C MAC does not (and vice versa for the C2S branch at lines 4484-4508). Neither branch's WS_MATCH_MAC_ALGO_E return is exercised by any test. Grep across the repo confirms no test references WS_MATCH_MAC_ALGO_E.

Fix: Add a regress.c test that supplies asymmetric MAC lists where one direction shares no entries with ssh->algoListMac (e.g., local accepts hmac-sha1,hmac-sha2-256; payload offers C2S=hmac-sha1, S2C=hmac-md5). Assert wolfSSH_TestDoKexInit returns WS_MATCH_MAC_ALGO_E. Cover the inverse C2S-mismatch case as well.

[aidangarske]

🟠 [Medium] TestIndependentAlgoNegotiation tests do not verify all ID/Sz fields are reset on each ssh instance · test

The new tests only assert the split fields (peerEncryptId/peerMacId/peerAeadMode/peerBlockSz/peerMacSz) for the side they care about, but never assert peerBlockSz/peerMacSz directly. Given that the diff intentionally drops the explicit ID_NONE/MSGID_NONE initializers in HandshakeInfoNew (relying on the WMEMSET zero plus the assumption that ID_NONE == 0), an asymmetric subtle regression — e.g. peerBlockSz left at its MIN_BLOCK_SZ default in a code path that should overwrite it — would not be caught. A few additional AssertIntEQ lines for peerBlockSz, peerMacSz, blockSz, macSz after each negotiation would harden the tests against future refactors of HandshakeInfoNew or DoKexInit.

Fix: Add explicit assertions for the new peerBlockSz/peerMacSz fields (and for blockSz/macSz for symmetry) in both server and client variants.

[aidangarske]

🔵 [Low] wolfSSH_TestGenerateKeys NULL-argument validation is untested · test

The newly added test helper validates ssh == NULL || ssh->handshake == NULL and returns WS_BAD_ARGUMENT, but no test in tests/regress.c (or anywhere else) calls it with NULL inputs to verify the guard. All four new test sites (TestGenerateKeysSplit, TestGenerateKeysSplitClient sub-tests A and B) invoke it only after AssertNotNull(ssh->handshake). While the function is test-only infrastructure, a sibling invariant check is cheap to add.

Fix: Add a one-line assertion in regress.c (e.g., inside or adjacent to TestGenerateKeysSplit): AssertIntEQ(wolfSSH_TestGenerateKeys(NULL), WS_BAD_ARGUMENT); and a second call with a freshly-created WOLFSSH* whose handshake has been freed/nulled to verify the second branch.

[aidangarske]

🔵 [Low] Missing blank line between TestGenerateKeysSplit and TestGenerateKeysSplitClient · style

Every other top-level function in regress.c (including the other three new test functions added by this PR) is separated by a blank line. The closing brace of TestGenerateKeysSplit is followed immediately by the next function definition, which breaks consistency.

Fix: Add a blank line between the two functions to match the surrounding style.

[aidangarske]

🔵 [Low] New tests silently ignore wolfSSH_TestDoKexInit return without explanation · test

All four new tests call (void)wolfSSH_TestDoKexInit(ssh, payload, payloadSz, &idx); and then assert handshake state. The pre-existing RunFirstPacketFollowsCase (line 2088) has an explanatory comment about why the return is ignored (the tail of DoKexInit calls SendKexInit which fails for a stripped-down server with no keys). The four new test functions reuse this pattern but do not carry a similar explanation, so a future reader may mistakenly think the discarded return is a bug. For the client-side variants the rationale is even less obvious, since the privateKeyCount check does not apply to clients — the SendKexInit failure there comes from wolfSSH_SendPacket having no IO callback set up.

Fix: Add a brief comment in each new test (or factor a helper that wraps the call) explaining why the return value is intentionally discarded.