Conversation
LinuxJedi
commented
Apr 8, 2026
LinuxJedi
commented
- Fix oct2dec typo
- Fix leak in linked-list
- Fix Nucleus month handling
- Fix DoDisconnect to signal connection termination
- fix DoChannelOpen failure response and add regression test
- Validate the host key signature algorithm name in DoKexDhReply()
F-2069
F-2070
DoDisconnect was returning WS_SUCCESS after receiving SSH_MSG_DISCONNECT, allowing the session to continue processing packets. Per RFC 4253 §11.1, no further data should be accepted after a disconnect message. Add new WS_DISCONNECT error code and return it from DoDisconnect so callers tear down the connection immediately. F-605
Send SSH_MSG_CHANNEL_OPEN_FAILURE for unclassified channel open errors instead of incorrectly falling back to SSH_MSG_REQUEST_FAILURE. Normalize OPEN_OK error cases to an administrative-prohibited channel open failure with a generic description, and add white-box regressions covering callback rejection plus optional direct-tcpip and agent-null paths. F-2076
The client-side KEXDH_REPLY path was parsing the signature blob name and skipping over it without checking that it matched the negotiated host key algorithm. That allowed an RSA server to negotiate rsa-sha2-256 or rsa-sha2-512 but send a signature blob labeled ssh-rsa instead. Fix this by comparing the signature blob name against the expected signature type derived from handshake->pubKeyId before verifying the signature bytes. Add regress coverage that drives an in-memory client/server handshake, rewrites the server's first KEXDH_REPLY on the wire, and verifies the client rejects rsa-sha2-256 and rsa-sha2-512 replies whose signature blob name is downgraded to ssh-rsa. F-2077
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple static-analysis and correctness findings across wolfSSH core and regression tests, tightening protocol handling and fixing a few platform-specific and memory-management issues.
Changes:
- Add a dedicated disconnect error code and propagate disconnect as a terminal condition.
- Harden KEXDH reply processing by validating the signature algorithm name against the negotiated hostkey algorithm (with regression coverage).
- Fix SFTP handle-list unlinking and correct Nucleus
mktime()month handling; also fix octal digit validation and channel-open failure responses.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
wolfssh/error.h |
Adds WS_DISCONNECT and updates WS_LAST_E to include the new terminal error code. |
src/internal.c |
Implements disconnect propagation, validates KEXDH reply signature name, fixes wolfSSH_oct2dec() digit check, and ensures channel-open failures send CHANNEL_OPEN_FAIL. |
src/wolfsftp.c |
Fixes handle-list head removal logic and corrects Nucleus month conversion for mktime(). |
tests/regress.c |
Adds regression coverage for channel-open failure responses and KEXDH reply signature-name downgrade rejection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #908
Scan targets checked: wolfssh-bugs, wolfssh-compliance, wolfssh-consttime, wolfssh-defaults, wolfssh-mutation, wolfssh-proptest, wolfssh-src, wolfssh-zeroize
Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
CI is reported an issue?
ERROR - tests/api.c line 1293 failed with:
expected: wolfSSH_SFTP_Open(ssh, tmp->fName, 0x00000001, ((void*)0), handle, &handleSz) == WS_SUCCESS
result: -1001 != 0
FAIL tests/api.test (exit status: 134)
No idea what that was, but I can't reproduce it, and neither can the CI. |
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 4 total — 1 posted, 3 skipped
Posted findings
- [Medium] Nucleus hour macro has pre-existing same-class bug as the month macro —
src/wolfsftp.c:4517
Skipped findings
- [Low] Test helper uses htonl instead of codebase's c32toa convention
- [Medium] DoDisconnect does not advance idx past description and language strings
- [Info] Removal of blank line between PreparePacket and BundlePacket
Review generated by Skoll via openclaw
src/wolfsftp.c
Outdated
| * make it compatible with Unix time stamp. */ | ||
| #define WS_GETMON(d) (_GETMON(d) - 5) | ||
| #define WS_GETMON(d) (_GETMON(d) - 1) | ||
| #define WS_GETHOUR(t) (_GETHOUR(t) - 1) |
There was a problem hiding this comment.
🟡 [Medium] Nucleus hour macro has pre-existing same-class bug as the month macro
💡 SUGGEST question
This PR correctly fixes WS_GETMON from (_GETMON(d) - 5) to (_GETMON(d) - 1). The adjacent WS_GETHOUR(t) macro is defined as (_GETHOUR(t) - 1), which subtracts 1 from hours. DOS time format stores hours as 0-23 and mktime() also expects 0-23, so the -1 looks suspicious. The comment only mentions months, not hours. This is a pre-existing issue NOT introduced by this PR, but since the PR is specifically fixing Nucleus time handling, it's worth investigating whether WS_GETHOUR also needs correction.
Recommendation: Verify whether Nucleus stores hours as 1-24 (justifying the -1) or 0-23 (meaning -1 is a bug). If it's a bug, fix it in this PR since you're already touching Nucleus time handling.
