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

[dgarske]

@yosuke-wolfssl please see the Fenrir feedback.