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.
| /* Check if requested service is 'ssh-userauth' */ | ||
| if (ret == WS_SUCCESS) { | ||
| const char* nameUserAuth = IdToName(ID_SERVICE_USERAUTH); | ||
| if (nameUserAuth == NULL || XSTRCMP(name, nameUserAuth) != 0) { |
There was a problem hiding this comment.
XSTRCMP(name, nameUserAuth) compares C strings and can be bypassed with an SSH "string" that contains an embedded NUL (e.g., "ssh-userauth\0junk"). Because GetString() copies bytes and NUL-terminates, XSTRCMP will stop at the first NUL and treat it as a valid match even if the received string length differs. Use a length-checked, binary-safe comparison (e.g., validate nameSz equals the expected length and compare with XMEMCMP, or parse with GetStringRef + NameToId).
| /* Check if requested service is 'ssh-userauth' */ | |
| if (ret == WS_SUCCESS) { | |
| const char* nameUserAuth = IdToName(ID_SERVICE_USERAUTH); | |
| if (nameUserAuth == NULL || XSTRCMP(name, nameUserAuth) != 0) { | |
| /* Check if requested service is exactly 'ssh-userauth'. */ | |
| if (ret == WS_SUCCESS) { | |
| const char* nameUserAuth = IdToName(ID_SERVICE_USERAUTH); | |
| word32 nameUserAuthSz = 0; | |
| if (nameUserAuth != NULL) { | |
| nameUserAuthSz = (word32)XSTRLEN(nameUserAuth); | |
| } | |
| if (nameUserAuth == NULL || nameSz != nameUserAuthSz || | |
| XMEMCMP(name, nameUserAuth, nameUserAuthSz) != 0) { |
| SendDisconnect(ssh, | ||
| WOLFSSH_DISCONNECT_SERVICE_NOT_AVAILABLE); | ||
| ret = WS_INVALID_STATE_E; |
There was a problem hiding this comment.
The return value of SendDisconnect() is ignored. In non-blocking I/O this can return WS_WANT_WRITE, and overwriting ret with WS_INVALID_STATE_E prevents the caller from retrying the disconnect send, potentially leaving the peer connected without receiving the disconnect. Capture/propagate the SendDisconnect() result (and only return WS_INVALID_STATE_E once the disconnect has been successfully queued/sent).
| SendDisconnect(ssh, | |
| WOLFSSH_DISCONNECT_SERVICE_NOT_AVAILABLE); | |
| ret = WS_INVALID_STATE_E; | |
| ret = SendDisconnect(ssh, | |
| WOLFSSH_DISCONNECT_SERVICE_NOT_AVAILABLE); | |
| if (ret == WS_SUCCESS) { | |
| ret = WS_INVALID_STATE_E; | |
| } |
| /* Check if accepted service is 'ssh-userauth' */ | ||
| if (ret == WS_SUCCESS) { | ||
| const char* nameUserAuth = IdToName(ID_SERVICE_USERAUTH); | ||
| if (nameUserAuth == NULL || XSTRCMP(name, nameUserAuth) != 0) { | ||
| WLOG(WS_LOG_DEBUG, "Accepted unexpected service: %s", name); | ||
| ret = WS_INVALID_STATE_E; |
There was a problem hiding this comment.
XSTRCMP(name, nameUserAuth) is not binary-safe for SSH "string" fields. A malicious or non-conforming peer could send an accepted service like "ssh-userauth\0junk" and pass this check because XSTRCMP stops at the embedded NUL. Prefer a length-based comparison (check nameSz against WSTRLEN(nameUserAuth) and use XMEMCMP) or parse via GetStringRef and compare IDs via NameToId.
Fixes F-604