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]

@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

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #953

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

[yosuke-wolfssl]

Hello @aidangarske ,

It's ready for review again.
Could you please take a look at this ?

[ejohnstown]

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.

[aidangarske]

Skoll Code Review

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

Posted findings

• [Medium] wolfSSH_SetUsernameRaw mutates session state before the service-name check — src/internal.c:8351-8376
• [Low] Unrelated blank-line removal between DoReceive and DoProtoId — src/internal.c:10719
• [Low] Test couples to internal packet layout (byte-5 message-id) without abstraction — tests/unit.c:1027-1035

Review generated by Skoll

[aidangarske]

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

[aidangarske]

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

[aidangarske]

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

[aidangarske]

Skoll Multi-Scan Review

Modes: audit + review-security
Overall recommendation: COMMENT
Findings: 1 total — 1 posted, 0 skipped
1 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [Low] [audit] Unknown-auth-method begin = len consumption path lacks meaningful coverage — src/internal.c:8402-8408

Review generated by Skoll

[aidangarske]

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