Skip to content

keyboard-interactive: send USERAUTH_FAILURE on response-count mismatch#1070

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_6514
Open

keyboard-interactive: send USERAUTH_FAILURE on response-count mismatch#1070
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_6514

Conversation

@yosuke-wolfssl

Copy link
Copy Markdown
Contributor

Description

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, or a count
greater than WOLFSSH_MAX_PROMPTS.

Both guards set ret = WS_USER_AUTH_E. The failure-response gate only calls
SendUserAuthFailure() when authFailure || partialSuccess is set, so no SSH
message was emitted; DoReceive() converts WS_USER_AUTH_E to WS_FATAL_ERROR
and the connection is torn down. 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

In both guards, set authFailure so the existing gate sends
SSH_MSG_USERAUTH_FAILURE and the connection survives (the client may retry).

ret is kept non-success in these branches so the response allocation, parse,
and user-auth callback are skipped:

  • a wire-controlled responseCount (up to 2^32-1) would otherwise drive a
    large allocation, and
  • the auth callback must not run on a protocol-violating message.

The failure path also reports the whole payload consumed (*idx = len),
consistent with the success path's contract.

Testing

Added tests/regress.c coverage using the in-memory packet harness, which feeds
a crafted INFO_RESPONSE straight into DoReceive() and inspects the bytes the
server writes back:

  • TestKbInfoResponseCountMismatchSendsFailureresponseCount (2) disagrees
    with promptCount (1).
  • TestKbInfoResponseTooManySendsFailureresponseCount > WOLFSSH_MAX_PROMPTS.

Both assert DoReceive() returns WS_SUCCESS and the reply is
MSGID_USERAUTH_FAILURE (via the shared AssertKbInfoResponseSentFailure
helper).

$ ./tests/regress.test
regress: PASS
$ ./tests/auth.test    # exit 0
  • Negative control: reverting the authFailure change makes the new tests
    abort at the WS_SUCCESS assertion with ret = -1001 (WS_FATAL_ERROR),
    confirming they catch the regression.
  • Sanitizers: built with -fsanitize=address,undefined (leak detection off
    on macOS); regress.test is clean.

Notes

  • No public API or signature changes.
  • The *idx = len line is a contract-consistency safeguard. In the current
    receive path it is not behaviorally observable (DoReceive force-shrinks the
    input buffer after every packet), but it keeps the handler consistent with its
    siblings and robust to future buffer-management changes.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Jun 29, 2026
Copilot AI review requested due to automatic review settings June 29, 2026 05:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #1070

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants