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
+147 −2
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
20 changes: 18 additions & 2 deletions src/internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8367,6 +8367,7 @@ static int DoUserAuthRequest(WOLFSSH* ssh,
word32 begin;
int ret = WS_SUCCESS;
byte authNameId;
byte serviceValid = 1;
Copy link
Copy Markdown
Member

@aidangarske aidangarske Apr 24, 2026

Choose a reason for hiding this comment

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

⚪ [Info] serviceValid could be replaced by a guard-style early return block for readability

A new local byte serviceValid = 1; is introduced solely to skip the auth-method dispatch block after the short-circuit. The function already uses the chained if (ret == WS_SUCCESS) pattern throughout. An alternative would be to leave ret as WS_SUCCESS (since the packet was handled) and skip the dispatch by leaving authData.authName zero-length and authNameId == ID_UNKNOWN, which already falls into the else-branch at line 8441. But that would still call wolfSSH_SetUsernameRaw a second time and SendUserAuthFailure a second time, which is undesirable. The serviceValid flag is fine as-is.

Fix: Keep as-is. Flag is clearer than refactoring around the existing chained-if pattern.

WS_UserAuthData authData;

WLOG(WS_LOG_DEBUG, "Entering DoUserAuthRequest()");
Expand Down Expand Up @@ -8401,10 +8402,19 @@ static int DoUserAuthRequest(WOLFSSH* ssh,
authData.serviceName = buf + begin;
begin += authData.serviceNameSz;

ret = GetSize(&authData.authNameSz, buf, len, &begin);
if (NameToId((const char*)authData.serviceName, authData.serviceNameSz)
Copy link
Copy Markdown
Member

@aidangarske aidangarske Apr 24, 2026

Choose a reason for hiding this comment

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

🔵 [Low] Inconsistent cast style in new NameToId call

The new service-name check casts authData.serviceName as (const char*), but the immediately adjacent NameToId call on line 8420 for authData.authName uses (char*). Both compile fine because NameToId takes const char*, but the mixed styles inside the same function are distracting.

Fix: Pick one cast style for NameToId in this function (either both (char*) or both (const char*)) so the two call sites read identically.

Copy link
Copy Markdown
Member

@aidangarske aidangarske Apr 24, 2026

Choose a reason for hiding this comment

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

🔵 [Low] Error from SendUserAuthFailure overwrites ret, making invalid-service path return non-success

When the service name is invalid the code calls ret = SendUserAuthFailure(ssh, 0). If SendUserAuthFailure() fails (e.g., WS_SOCKET_ERROR_E, WS_WANT_WRITE), that error is returned to the outer dispatcher as if parsing itself failed. In the unknown-auth-method path below (line 8444) the same pattern is used, so this is internally consistent with the existing convention — but it is worth confirming that propagating a send error here is the intended behavior (e.g., WS_WANT_WRITE from a non-blocking socket could reach DoReceive here and bubble up differently than the caller expects). The unit test uses a capture-callback that always returns success, so this path is not exercised.

Fix: Confirm that it is acceptable to return the SendUserAuthFailure() result (including WS_WANT_WRITE) to the DoReceive loop when the only parsing failure was a bad service name. If the intent is "we handled this request, stay alive," consider saving the send result separately from ret or adding a test where the IO callback returns WS_CBIO_ERR_WANT_WRITE.

!= ID_SERVICE_CONNECTION) {
WLOG(WS_LOG_DEBUG, "DUAR: Invalid service name");
Copy link
Copy Markdown
Contributor

@padelsbach padelsbach Apr 24, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

The other debug logs in this function do not have DUAR -- consider updating those to match this one, following convention from DoUserAuthRequestNone.

serviceValid = 0;
ret = SendUserAuthFailure(ssh, 0);
Copy link
Copy Markdown
Member

@aidangarske aidangarske Apr 24, 2026

Choose a reason for hiding this comment

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

🔵 [Low] *idx = len is written even when SendUserAuthFailure returned an error

On the invalid-service path, ret = SendUserAuthFailure(ssh, 0); *idx = len; advances idx unconditionally. If SendUserAuthFailure returns a non-success value (e.g., a non-blocking WS_WANT_WRITE or an allocation failure inside PreparePacket), the function returns the error to DoPacket but idx is still fully consumed. For most error paths this is harmless because the caller tears the connection down, but it does mean the decoded-length cursor no longer matches what was actually processed. Other handlers in this file leave idx alone when ret is non-success.

Fix: Consider only advancing *idx = len when SendUserAuthFailure returns WS_SUCCESS, matching the idx-preservation convention used elsewhere in this function on error paths.

Copy link
Copy Markdown
Member

@aidangarske aidangarske Apr 24, 2026

Choose a reason for hiding this comment

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

⚪ [Info] *idx = len set before SendUserAuthFailure result is checked

*idx is written unconditionally immediately after (and independent of) the SendUserAuthFailure() call. The surrounding convention in this function (and in DoReceive) is "advance the buffer index on success, leave untouched on failure." Here the index is advanced even when the send fails. The DoReceive dispatcher at src/internal.c:10178 will still advance ssh->inputBuffer.idx by payloadIdx even on error, so the behavior is not incorrect — but the asymmetry with the valid-path write *idx = begin; at line 8452 (guarded by if (ret == WS_SUCCESS && serviceValid)) reads slightly oddly.

Fix: Optional: move *idx = len; above the SendUserAuthFailure call to make it clear the assignment is unconditional and not contingent on send success, or leave as-is. Not a correctness issue.

*idx = len;
Copy link
Copy Markdown
Contributor

@padelsbach padelsbach Apr 24, 2026

Choose a reason for hiding this comment

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

Other error cases in this function do not set *idx. Why is this needed for this specific error? Can you add a comment?

}
else {
ret = GetSize(&authData.authNameSz, buf, len, &begin);
}
}

if (ret == WS_SUCCESS) {
if (ret == WS_SUCCESS && serviceValid) {
authData.authName = buf + begin;
begin += authData.authNameSz;
authNameId = NameToId((char*)authData.authName, authData.authNameSz);
Expand Down Expand Up @@ -10773,6 +10783,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);
}
#endif


Expand Down
127 changes: 127 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,128 @@ 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)
* 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)
Copy link
Copy Markdown
Member

@aidangarske aidangarske Apr 24, 2026

Choose a reason for hiding this comment

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

🔵 [Low] Test assumes output path succeeds on a fresh session without documentation

The test relies on no cipher being negotiated so the captured packet is plaintext with MSGID_USERAUTH_FAILURE at byte offset 5. That assumption is already documented in test_DoChannelRequest (lines 792-797) but not here. A one-line comment referencing the same packet-layout invariant would save the next reader from retracing the logic.

Fix: Add a short comment explaining why byte 5 holds the message ID, for parity with test_DoChannelRequest.

{
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[] = {
Copy link
Copy Markdown
Member

@aidangarske aidangarske Apr 24, 2026

Choose a reason for hiding this comment

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

🔵 [Low] Test does not cover empty or oversize service names

The new test exercises three cases (ssh-connection, ssh-agent, bad). It does not cover the boundary cases serviceNameSz == 0 (zero-length service name) or a service name equal to another known service ID (e.g., ssh-userauth). Both should also be rejected by the new NameToId(...) != ID_SERVICE_CONNECTION check and would provide stronger evidence that only ssh-connection is accepted. NameToId("", 0) returns ID_UNKNOWN, and NameToId("ssh-userauth", 12) returns ID_SERVICE_USERAUTH, both of which should short-circuit — but neither is currently verified.

Fix: Add a zero-length service-name case and an ssh-userauth service-name case to the cases[] table to verify the check is strict and not dependent on NameToId returning ID_UNKNOWN specifically.

/* 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" },
};
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 ||
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 +1332,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