Add service-name check and regress test

[yosuke-wolfssl]

This PR adds the service-name check to comply with RFC 4252 section 5.
If service name is not equal to "ssh-connection", SSH_MSG_USERAUTH_FAILURE would be sent to the peer.
Also, new unit test is added to exercise service-name validation.

[Copilot]

Pull request overview

This PR adds RFC 4252 §5 compliance by validating the service-name in SSH_MSG_USERAUTH_REQUEST and rejecting requests that do not target "ssh-connection" while keeping the connection open for retry. It also introduces a new internal test hook and a unit test intended to exercise the new validation behavior.

Changes:

• Add service-name validation in DoUserAuthRequest() and send SSH_MSG_USERAUTH_FAILURE when invalid.
• Expose DoUserAuthRequest() via a new wolfSSH_TestDoUserAuthRequest() internal test wrapper.
• Add a new unit test to exercise service-name validation logic.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
wolfssh/internal.h	Adds a new internal test API declaration for invoking DoUserAuthRequest() from unit tests.
src/internal.c	Implements the service-name check and the corresponding test wrapper.
tests/unit.c	Adds a unit test for service-name validation and wires it into the unit test runner.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[padelsbach]

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

[padelsbach]

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

[padelsbach]

@yosuke-wolfssl, can you please rebase to resolve conflicts?

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #953

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

[aidangarske]

Skoll Code Review

Scan type: review
Overall recommendation: APPROVE
Findings: 7 total — 7 posted, 0 skipped
7 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [Low] Inconsistent cast style in new NameToId call — src/internal.c:8405-8406
• [Low] Error from SendUserAuthFailure overwrites ret, making invalid-service path return non-success — src/internal.c:8405-8411
• [Low] *idx = len is written even when SendUserAuthFailure returned an error — src/internal.c:8409-8411
• [Low] Test assumes output path succeeds on a fresh session without documentation — tests/unit.c:952-1041
• [Low] Test does not cover empty or oversize service names — tests/unit.c:964-973
• [Info] serviceValid could be replaced by a guard-style early return block for readability — src/internal.c:8370,8417
• [Info] *idx = len set before SendUserAuthFailure result is checked — src/internal.c:8409-8410

Review generated by Skoll

[aidangarske]

🔵 [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.

[aidangarske]

🔵 [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.

[aidangarske]

🔵 [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.

[aidangarske]

🔵 [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.

[aidangarske]

🔵 [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.

[aidangarske]

⚪ [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.

[aidangarske]

⚪ [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.