Centralize AES cipher lifecycle#1043
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates wolfSSH session initialization to properly initialize the AES cipher contexts in SshInit() before they are configured and later freed, aligning usage with wolfCrypt’s wc_AesInit()/wc_AesFree() lifecycle expectations.
Changes:
- Add
wc_AesInit()calls forssh->encryptCipher.aesandssh->decryptCipher.aesduringSshInit(). - Add an early failure path when cipher initialization fails.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1043
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
aidangarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-security + bugsOverall recommendation: COMMENT
Findings: 4 total — 4 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] [review+review-security] New cipher lifecycle state and error/cleanup paths in CipherSetKey/CipherClear lack regression coverage —
src/internal.c:1202-1344 - [Low] [review+review-security] New code uses truthiness pointer checks instead of explicit != NULL —
src/internal.c:1204,1272,1293 - [Info] [review+review-security] Missing space around assignment operator —
src/internal.c:1233 - [Info] [review+review-security] DoNewKeys now errors on unknown peer cipher instead of silently succeeding —
src/internal.c:6668
Review generated by Skoll
- Add CipherSetKey()/CipherClear() to init, key, and free the Aes contexts; init lazily and track isInit/cipherType. - Free only inited contexts, fixing wc_AesFree on uninitialized ones. - DoNewKeys now fails an unknown peer cipher with WS_INVALID_ALGO_ID instead of WS_SUCCESS, matching SendNewKeys. - Add regression coverage for isInit/cipherType and a never-keyed free.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1043
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
The encrypt/decrypt Aes contexts were keyed and freed without ever calling wc_AesInit. Centralize the lifecycle instead of sprinkling init calls: