Skip to content

Add validation for accept request and reply #902

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
padelsbach wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add validation for accept request and reply #902

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6539,6 +6539,17 @@ static int DoServiceRequest(WOLFSSH* ssh,

ret = GetString(name, &nameSz, buf, len, idx);

/* 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) {
Comment on lines +6542 to +6545
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
/* 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) {

Copilot uses AI. Check for mistakes.
WLOG(WS_LOG_DEBUG, "Requested unsupported service: %s", name);
SendDisconnect(ssh,
WOLFSSH_DISCONNECT_SERVICE_NOT_AVAILABLE);
ret = WS_INVALID_STATE_E;
Comment on lines +6547 to +6549
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
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;
}

Copilot uses AI. Check for mistakes.
}
}

if (ret == WS_SUCCESS) {
WLOG(WS_LOG_DEBUG, "Requesting service: %s", name);
ssh->clientState = CLIENT_USERAUTH_REQUEST_DONE;
Expand All @@ -6557,6 +6568,15 @@ static int DoServiceAccept(WOLFSSH* ssh,

ret = GetString(name, &nameSz, buf, len, idx);

/* 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;
Comment on lines +6571 to +6576
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}
}

if (ret == WS_SUCCESS) {
WLOG(WS_LOG_DEBUG, "Accepted service: %s", name);
ssh->serverState = SERVER_USERAUTH_REQUEST_DONE;
Expand Down
Loading