Skip to content

Add service-name check and regress test #953

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
yosuke-wolfssl wants to merge 1 commit into
base: master
Choose a base branch
from
+160 −5
Open

Add service-name check and regress test #953

Show file tree
Hide file tree
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
28 changes: 23 additions & 5 deletions src/internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8327,6 +8327,7 @@ static int DoUserAuthRequest(WOLFSSH* ssh,
word32 begin;
int ret = WS_SUCCESS;
byte authNameId;
byte serviceValid = 1;
Comment thread
yosuke-wolfssl marked this conversation as resolved.
WS_UserAuthData authData;

WLOG(WS_LOG_DEBUG, "Entering DoUserAuthRequest()");
Expand Down Expand Up @@ -8361,13 +8362,23 @@ static int DoUserAuthRequest(WOLFSSH* ssh,
authData.serviceName = buf + begin;
begin += authData.serviceNameSz;

ret = GetSize(&authData.authNameSz, buf, len, &begin);
Copy link
Copy Markdown
Contributor

@ejohnstown ejohnstown Apr 28, 2026

Choose a reason for hiding this comment

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

The function GetStringRef() was added for cases like this. It does all the appropriate length checking and it updates the idx. I just never got around to replacing all GetSize() and adjusting the idx myself with this function.

GetStringRef will give you a pointer into buf that points at the string, and it gives you the length. You don't need to free it, and the pointer is to const.

if (NameToId((const char*)authData.serviceName, authData.serviceNameSz)
Comment thread
yosuke-wolfssl marked this conversation as resolved.
Comment thread
aidangarske marked this conversation as resolved.
!= ID_SERVICE_CONNECTION) {
WLOG(WS_LOG_DEBUG, "DUAR: Invalid service name");
Comment thread
yosuke-wolfssl marked this conversation as resolved.
serviceValid = 0;
ret = SendUserAuthFailure(ssh, 0);
Comment thread
aidangarske marked this conversation as resolved.
Comment thread
yosuke-wolfssl marked this conversation as resolved.
/* Consume all remaining data */
*idx = len;
Comment thread
yosuke-wolfssl marked this conversation as resolved.
}
else {
ret = GetSize(&authData.authNameSz, buf, len, &begin);
}
}
Copy link
Copy Markdown
Member

@aidangarske aidangarske Apr 28, 2026

Choose a reason for hiding this comment

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

🟠 [Medium] wolfSSH_SetUsernameRaw mutates session state before the service-name check

The username is committed into ssh->userName at line 8351 before the new service-name validation runs. If the peer sends a request with a non-ssh-connection service name (or any other malformed thing later in parsing), the session keeps the attacker-supplied username while we send USERAUTH_FAILURE. The PR doesn't introduce this ordering, but by adding a new reject path between the username set and the auth-method dispatch it widens the window where this matters. A second retry will overwrite the field, so this is mostly cosmetic, but consider deferring wolfSSH_SetUsernameRaw until after we accept the request (or at least until after the service name is validated) so that rejected requests don't leave residual state. This would also let you drop the duplicate wolfSSH_SetUsernameRaw call at line 8410-8413.

Fix: Set the username only on a fully-validated request to avoid storing state from rejected attempts.


if (ret == WS_SUCCESS) {
if (ret == WS_SUCCESS && serviceValid) {
authData.authName = buf + begin;
begin += authData.authNameSz;
authNameId = NameToId((char*)authData.authName, authData.authNameSz);
authNameId = NameToId((const char*)authData.authName, authData.authNameSz);
ssh->authId = authNameId;

if (authNameId == ID_USERAUTH_PASSWORD)
Expand All @@ -8390,8 +8401,10 @@ static int DoUserAuthRequest(WOLFSSH* ssh,
#endif
else {
Copy link
Copy Markdown
Member

@aidangarske aidangarske Apr 28, 2026

Choose a reason for hiding this comment

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

🔵 [Low] Unknown-auth-method begin = len consumption path lacks meaningful coverage · test-coverage

The PR adds begin = len; (with comment /* Consume all remaining data */) in the unknown-auth-method else branch so that DoUserAuthRequest() defensively skips any trailing auth-method-specific payload when a request has a valid service name but an unsupported auth method. The new test test_DoUserAuthRequest_serviceName (tests/unit.c:953-1048) does exercise this branch via the valid svc unknown auth case (service ssh-connection, auth method xyz-unknown), but the test buffer ends exactly at the end of the auth-method name with no trailing bytes. As a result, begin already equals len before the new assignment is reached, and the test's idx != len assertion would still pass even if the new begin = len; line were removed. Because DoReceive() advances inputBuffer.idx from the handler-reported payloadIdx (src/internal.c:10146-10159), the PR still lacks a test proving that an unsupported auth method with extra trailing bytes is fully consumed and leaves packet decoding aligned. A regression that re-introduced an offset bug in this branch would not be caught.

Fix: Extend test_DoUserAuthRequest_serviceName() with a valid "ssh-connection" case that includes an unsupported auth method plus extra trailing bytes (for example, fake password-method fields), then verify wolfSSH_TestDoUserAuthRequest() still returns WS_SUCCESS, sends MSGID_USERAUTH_FAILURE, and advances idx to len. Without this scenario, the consumption-of-remaining-data behavior added by this PR is not under test.

WLOG(WS_LOG_DEBUG,
"invalid userauth type: %s", IdToName(authNameId));
"DUAR: invalid userauth type: %s", IdToName(authNameId));
ret = SendUserAuthFailure(ssh, 0);
/* Consume all remaining data */
begin = len;
}

if (ret == WS_SUCCESS) {
Expand Down Expand Up @@ -10716,7 +10729,6 @@ int DoReceive(WOLFSSH* ssh)
return ret;
Copy link
Copy Markdown
Member

@aidangarske aidangarske Apr 28, 2026

Choose a reason for hiding this comment

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

🔵 [Low] Unrelated blank-line removal between DoReceive and DoProtoId

The diff removes a blank line at the top of the gap between DoReceive and DoProtoId. This change has nothing to do with the service-name check and just adds noise to the diff. Consider keeping unrelated whitespace edits out of feature PRs to keep git blame clean.

Fix: Revert the stray formatting hunk.

Note: Referenced line (src/internal.c:10719) is outside the diff hunk. Comment anchored to nearest changed region.

}


int DoProtoId(WOLFSSH* ssh)
{
int ret;
Expand Down Expand Up @@ -17907,6 +17919,12 @@ int wolfSSH_TestChannelPutData(WOLFSSH_CHANNEL* channel, byte* data,
return ChannelPutData(channel, data, dataSz);
}

int wolfSSH_TestDoUserAuthRequest(WOLFSSH* ssh, byte* buf, word32 len,
word32* idx)
{
return DoUserAuthRequest(ssh, buf, len, idx);
}

#ifndef WOLFSSH_NO_DH_GEX_SHA256

int wolfSSH_TestDoKexDhGexRequest(WOLFSSH* ssh, byte* buf, word32 len,
Expand Down
135 changes: 135 additions & 0 deletions tests/unit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -918,6 +918,136 @@ static int test_DoChannelRequest(void)
return result;
}

/* Capture buffer for the service-name unit test. Separate from the channel-
* request capture so the two tests can run independently in any order. */
static byte s_authSvcCapture[256];
static word32 s_authSvcCaptureSz = 0;

static int CaptureIoSendAuthSvc(WOLFSSH* ssh, void* buf, word32 sz, void* ctx)
{
(void)ssh; (void)ctx;
s_authSvcCaptureSz = (sz < (word32)sizeof(s_authSvcCapture))
? sz : (word32)sizeof(s_authSvcCapture);
WMEMCPY(s_authSvcCapture, buf, s_authSvcCaptureSz);
return (int)sz;
}

/* Verify DoUserAuthRequest rejects non-"ssh-connection" service names per
* RFC 4252 Section 5. For each case we assert:
* 1. ret == WS_SUCCESS (connection stays open for retry)
* 2. SSH_MSG_USERAUTH_FAILURE is actually sent (captured at packet byte 5:
* [4-byte packet_length][1-byte padding_length][1-byte msg_id]...)
* 3. *idx == len (entire payload consumed; buffer stays aligned)
*
* For invalid-service cases the auth-method field is intentionally omitted
* from the payload. DoUserAuthRequest must short-circuit at the service-name
* check and still satisfy all three assertions — proving it never tries to
* parse the missing auth-method field. If the short-circuit were absent,
* GetSize() for authNameSz would hit end-of-buffer and return WS_BUFFER_E,
* failing assertion 1.
*
* For the valid-service case, auth method "xyz-unknown" (always unsupported
* regardless of compile-time options) is included. The function reaches
* auth-method dispatch, falls to the unknown-method else-branch, and sends
* USERAUTH_FAILURE via that normal path. */
static int test_DoUserAuthRequest_serviceName(void)
Comment thread
yosuke-wolfssl marked this conversation as resolved.
{
WOLFSSH_CTX* ctx = NULL;
WOLFSSH* ssh = NULL;
int result = 0;
struct {
const char* svcName;
word32 svcNameSz;
const char* authMethod; /* NULL = omit field (proves short-circuit) */
word32 authMethodSz;
int expectRet;
const char* label;
} cases[] = {
Comment thread
yosuke-wolfssl marked this conversation as resolved.
/* valid service: auth dispatch fires, fails on unknown method */
{ "ssh-connection", 14, "xyz-unknown", 11, WS_SUCCESS,
"valid svc unknown auth" },
/* invalid service: short-circuit, auth-method field absent */
{ "ssh-agent", 9, NULL, 0, WS_SUCCESS,
"invalid ssh-agent svc" },
{ "bad", 3, NULL, 0, WS_SUCCESS,
"invalid bad svc" },
/* zero-length service name: NameToId("",0)==ID_UNKNOWN, must reject */
{ "", 0, NULL, 0, WS_SUCCESS,
"zero-length svc" },
/* ssh-userauth: NameToId returns ID_SERVICE_USERAUTH, not
* ID_SERVICE_CONNECTION, so must also be rejected */
{ "ssh-userauth", 12, NULL, 0, WS_SUCCESS,
"invalid ssh-userauth svc" },
};
int i;

ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL);
if (ctx == NULL) return -500;
wolfSSH_SetIOSend(ctx, CaptureIoSendAuthSvc);

ssh = wolfSSH_new(ctx);
if (ssh == NULL) { wolfSSH_CTX_free(ctx); return -501; }

for (i = 0; i < (int)(sizeof(cases)/sizeof(cases[0])); i++) {
byte buf[128];
word32 len = 0, idx = 0;
word32 snsz = cases[i].svcNameSz;
int ret;

s_authSvcCaptureSz = 0;
WMEMSET(s_authSvcCapture, 0, sizeof(s_authSvcCapture));

/* username: "user" */
buf[len++] = 0; buf[len++] = 0; buf[len++] = 0; buf[len++] = 4;
WMEMCPY(buf + len, "user", 4); len += 4;

/* service name */
buf[len++] = (byte)(snsz >> 24); buf[len++] = (byte)(snsz >> 16);
buf[len++] = (byte)(snsz >> 8); buf[len++] = (byte)snsz;
WMEMCPY(buf + len, cases[i].svcName, snsz); len += snsz;

/* auth method: omit for invalid-service cases to prove short-circuit */
if (cases[i].authMethod != NULL) {
word32 amsz = cases[i].authMethodSz;
buf[len++] = (byte)(amsz >> 24); buf[len++] = (byte)(amsz >> 16);
buf[len++] = (byte)(amsz >> 8); buf[len++] = (byte)amsz;
WMEMCPY(buf + len, cases[i].authMethod, amsz); len += amsz;
}

ret = wolfSSH_TestDoUserAuthRequest(ssh, buf, len, &idx);

if (ret != cases[i].expectRet) {
printf("DoUserAuthRequest_svcName[%s]: ret=%d expected=%d\n",
cases[i].label, ret, cases[i].expectRet);
result = -502 - i;
break;
}

/* MSGID_USERAUTH_FAILURE must be in the captured packet. */
if (s_authSvcCaptureSz <= 5 ||
Copy link
Copy Markdown
Member

@aidangarske aidangarske Apr 28, 2026

Choose a reason for hiding this comment

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

🔵 [Low] Test couples to internal packet layout (byte-5 message-id) without abstraction

The assertion s_authSvcCapture[5] != MSGID_USERAUTH_FAILURE hard-codes the offset of the message id in the framed packet (4-byte length + 1-byte padding length + msg id). This is the same approach used by test_DoChannelRequest, so the inconsistency is repo-wide rather than PR-introduced, but every new copy of this pattern increases the cleanup cost if/when packet framing changes. Consider extracting a small helper like static int CaptureMsgId(byte* buf, word32 len) that the existing channel-request test could also adopt.

Fix: Optional cleanup; not a merge blocker.

s_authSvcCapture[5] != MSGID_USERAUTH_FAILURE) {
printf("DoUserAuthRequest_svcName[%s]: USERAUTH_FAILURE not sent "
"(capSz=%u byte5=0x%02x)\n", cases[i].label,
s_authSvcCaptureSz,
s_authSvcCaptureSz > 5 ? s_authSvcCapture[5] : 0);
result = -520 - i;
break;
}

/* All cases must consume the entire payload. */
if (idx != len) {
printf("DoUserAuthRequest_svcName[%s]: idx=%u expected len=%u\n",
cases[i].label, idx, len);
result = -510 - i;
break;
}
}

wolfSSH_free(ssh);
wolfSSH_CTX_free(ctx);
return result;
}

#if !defined(WOLFSSH_NO_RSA)

/* 2048-bit RSA private key (PKCS#1 DER).
Expand Down Expand Up @@ -1210,6 +1340,11 @@ int wolfSSH_UnitTest(int argc, char** argv)
unitResult = test_ChannelPutData();
printf("ChannelPutData: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED"));
testResult = testResult || unitResult;

unitResult = test_DoUserAuthRequest_serviceName();
printf("DoUserAuthRequest_serviceName: %s\n",
(unitResult == 0 ? "SUCCESS" : "FAILED"));
testResult = testResult || unitResult;
#endif

#ifdef WOLFSSH_KEYGEN
Expand Down
2 changes: 2 additions & 0 deletions wolfssh/internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1338,6 +1338,8 @@ enum WS_MessageIdLimits {
WOLFSSH_API int wolfSSH_TestDoKexDhInit(WOLFSSH* ssh, byte* buf,
word32 len, word32* idx);
WOLFSSH_API int wolfSSH_TestChannelPutData(WOLFSSH_CHANNEL*, byte*, word32);
WOLFSSH_API int wolfSSH_TestDoUserAuthRequest(WOLFSSH* ssh, byte* buf,
word32 len, word32* idx);
#ifndef WOLFSSH_NO_DH_GEX_SHA256
WOLFSSH_API int wolfSSH_TestDoKexDhGexRequest(WOLFSSH* ssh, byte* buf,
word32 len, word32* idx);
Expand Down
Loading