Add validation for accept request and reply#902
Add validation for accept request and reply#902padelsbach wants to merge 1 commit intowolfSSL:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds stricter validation around SSH service negotiation messages to ensure that only the expected ssh-userauth service is accepted during the service request/accept phase (Fixes F-604).
Changes:
- Validate incoming
MSGID_SERVICE_REQUESTservice name againstssh-userauthand disconnect when unsupported. - Validate incoming
MSGID_SERVICE_ACCEPTservice name againstssh-userauthand fail when unexpected.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
padelsbach
force-pushed
the
request-validation
branch
from
51f23aa to
e69bdab
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #902
Scan targets checked: wolfssh-bugs, wolfssh-compliance, 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.
|
|
||
| ret = GetString(name, &nameSz, buf, len, idx); | ||
|
|
||
| /* Requested service must be 'ssh-userauth' */ | ||
| if (ret == WS_SUCCESS) { | ||
| const char* nameUserAuth = IdToName(ID_SERVICE_USERAUTH); | ||
| if (nameUserAuth == NULL | ||
| || nameSz != (word32)XSTRLEN(nameUserAuth) | ||
| || XMEMCMP(name, nameUserAuth, nameSz) != 0) { | ||
| WLOG(WS_LOG_DEBUG, "Accepted unexpected service: %s", name); | ||
| ret = WS_INVALID_STATE_E; | ||
| } | ||
| } | ||
|
|
||
| if (ret == WS_SUCCESS) { | ||
| WLOG(WS_LOG_DEBUG, "Accepted service: %s", name); | ||
| ssh->serverState = SERVER_USERAUTH_REQUEST_DONE; |
There was a problem hiding this comment.
🔹 [Low] DoServiceAccept does not send SSH_MSG_DISCONNECT on unexpected service name
Category: Disconnect and error handling violations
The new validation in DoServiceAccept detects when the server accepts an unexpected service (not ssh-userauth) but only sets ret = WS_INVALID_STATE_E without sending SSH_MSG_DISCONNECT. By contrast, the parallel validation added in DoServiceRequest (line 6549) correctly calls SendDisconnect(ssh, WOLFSSH_DISCONNECT_SERVICE_NOT_AVAILABLE) before returning the error. Per RFC 4253 §11.1, an implementation SHOULD send SSH_MSG_DISCONNECT when it detects a protocol violation, rather than simply closing the connection. The asymmetry between the two functions suggests this was an oversight rather than an intentional design choice.
/* Requested service must be 'ssh-userauth' */
if (ret == WS_SUCCESS) {
const char* nameUserAuth = IdToName(ID_SERVICE_USERAUTH);
if (nameUserAuth == NULL
|| nameSz != (word32)XSTRLEN(nameUserAuth)
|| XMEMCMP(name, nameUserAuth, nameSz) != 0) {
WLOG(WS_LOG_DEBUG, "Accepted unexpected service: %s", name);
ret = WS_INVALID_STATE_E;
}
}Recommendation: Add a SendDisconnect call before setting the error, matching the pattern used in DoServiceRequest: (void)SendDisconnect(ssh, WOLFSSH_DISCONNECT_SERVICE_NOT_AVAILABLE); with a reason code of WOLFSSH_DISCONNECT_SERVICE_NOT_AVAILABLE or WOLFSSH_DISCONNECT_PROTOCOL_ERROR.
Fixes F-604