keyboard-interactive: send USERAUTH_FAILURE on response-count mismatch

[yosuke-wolfssl]

Problem

DoUserAuthInfoResponse() tore down the transport instead of sending an
SSH_MSG_USERAUTH_FAILURE when a keyboard-interactive INFO_RESPONSE carried a
responseCount that did not match the server's pending promptCount.

The guard set ret = WS_USER_AUTH_E, but the failure-response gate only sends a
message when authFailure || partialSuccess is set, so nothing was emitted;
DoReceive() converts WS_USER_AUTH_E to WS_FATAL_ERROR and the connection
dies. RFC 4256 requires the server to answer every INFO_RESPONSE with
SUCCESS, FAILURE, or another INFO_REQUEST. Impact is limited to
malformed/malicious clients, but the response is non-conformant.

Addressed by f_6514.

Fix (src/internal.c)

Set authFailure in the mismatch guard so the existing gate sends
SSH_MSG_USERAUTH_FAILURE and the connection survives; the client may retry.
ret stays non-success so the response allocation, parse, and user-auth callback
are skipped on a protocol-violating message.

Two removals came out of review:

• The adjacent responseCount > WOLFSSH_MAX_PROMPTS guard was dead code. The
  mismatch check forces responseCount == promptCount, and promptCount is
  capped at WOLFSSH_MAX_PROMPTS by both of its writers
  (SendUserAuthKeyboardRequest() server-side, DoUserAuthInfoRequest()
  client-side), so the allocation below is bounded transitively.
• An *idx = len line on the failure path was an unobservable no-op:
  DoReceive() calls ShrinkBuffer(&ssh->inputBuffer, 1) on every consumed
  packet, and forcedFree = 1 zeroes length/idx, so a short payload index
  never reaches the next packet.

Net change is one functional line plus a comment.

Tests (tests/regress.c)

Both use the in-memory packet harness, feeding crafted packets into DoReceive()
and inspecting the bytes the server writes back:

• TestKbInfoResponseCountMismatchSendsFailure — responseCount 2 vs
  promptCount 1; asserts WS_SUCCESS and a MSGID_USERAUTH_FAILURE reply.
• TestKbInfoResponseMismatchKeepsFraming — two back-to-back malformed
  responses; asserts the second packet still parses and draws its own failure,
  covering parser state after a rejected response.

Verification

• make check: 10 passed, 1 skipped (external), 0 failed.
• Clean under -Werror across 6 gcc-13 configs; lint clean.
• Negative controls: reverting authFailure = 1 aborts the first test with
  -1001; the framing test fails only when *idx = len is absent and
  ShrinkBuffer() is forced non-free, confirming it detects real desync.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1070

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

[aidangarske]

🐺 Skoll Code Review

Overall recommendation: COMMENT
Findings: 1 total — 1 posted, 0 skipped

Posted findings

• [Medium] Malformed keyboard response failure leaves challenge state active — src/internal.c:7278-7385

Review generated by Skoll.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1070

Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 2
1 finding(s) posted as inline comments (see file-level comments below)

High (1)

Packet-index not advanced on responseCount mismatch, desyncing input buffer

File: src/internal.c:8317
Function: DoUserAuthInfoResponse
Category: Logic errors

*idx = begin at line 8317 only runs when ret == WS_SUCCESS, but in the new mismatch branch ret is WS_USER_AUTH_E at that point and only becomes success afterward via SendUserAuthFailureCount(). *idx (the caller's payloadIdx) is left at 0, so DoPacket() sets ssh->inputBuffer.idx short by the unconsumed payload, corrupting framing for the next packet on the now-surviving connection.

Recommendation: In the mismatch branch also set *idx to the full payload length (len) before returning, independent of the ret == WS_SUCCESS gate.

Referenced code: src/internal.c:8317-8322 (6 lines)

---

This review was generated automatically by Fenrir. Findings are non-blocking.

[yosuke-wolfssl]

Hello @aidangarske, @ejohnstown ,
Sorry for being late. Could you review this again ?