Skip to content

Fix SendUserAuthKeyboardResponse() and add regress tests #910

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
+181 −43
Open

Fix SendUserAuthKeyboardResponse() and add regress tests #910

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
111 changes: 68 additions & 43 deletions src/internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15419,55 +15419,80 @@ static int BuildUserAuthRequestPublicKey(WOLFSSH* ssh,
int SendUserAuthKeyboardResponse(WOLFSSH* ssh)
{
byte* output;
int authRet = WOLFSSH_USERAUTH_FAILURE;
int ret = WS_SUCCESS;
word32 idx;
word32 payloadSz = 0;
word32 prompt;
WS_UserAuthData authData;

Copy link
Copy Markdown
Contributor

@dgarske dgarske Apr 9, 2026

Choose a reason for hiding this comment

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

🔵 [Low] authData not zero-initialized unlike peer function SendUserAuthRequest
💡 SUGGEST convention

The stack-local WS_UserAuthData authData; is declared but not zero-initialized. On early-error paths (e.g., WS_BAD_ARGUMENT when ssh == NULL), the struct contains indeterminate stack values until ForceZero at line 15525. While currently safe (no code reads authData on those paths), the peer function SendUserAuthRequest at line 15561 unconditionally calls WMEMSET(&authData, 0, sizeof(authData)); right after declaration. Adding the same pattern here would be more defensive and consistent.

Suggestion:

Suggested change
word32 prompt;
WS_UserAuthData authData;
WLOG(WS_LOG_DEBUG, "Entering SendUserAuthKeyboardResponse()");
WMEMSET(&authData, 0, sizeof(authData));

Copy link
Copy Markdown
Contributor Author

@yosuke-wolfssl yosuke-wolfssl Apr 9, 2026

Choose a reason for hiding this comment

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

Fixed

WLOG(WS_LOG_DEBUG, "Entering SendUserAuthKeyboardResponse()");
WMEMSET(&authData, 0, sizeof(authData));

authData.type = WOLFSSH_USERAUTH_KEYBOARD;
authData.username = (const byte*)ssh->userName;
authData.usernameSz = ssh->userNameSz;
authData.sf.keyboard.promptCount = ssh->kbAuth.promptCount;
authData.sf.keyboard.promptName = ssh->kbAuth.promptName;
authData.sf.keyboard.promptNameSz = ssh->kbAuth.promptName ?
(word32)WSTRLEN((char*)ssh->kbAuth.promptName) : 0;
authData.sf.keyboard.promptInstruction = ssh->kbAuth.promptInstruction;
authData.sf.keyboard.promptInstructionSz = ssh->kbAuth.promptInstruction ?
(word32)WSTRLEN((char*)ssh->kbAuth.promptInstruction) : 0;
authData.sf.keyboard.promptLanguage = ssh->kbAuth.promptLanguage;
authData.sf.keyboard.promptLanguageSz = ssh->kbAuth.promptLanguage ?
(word32)WSTRLEN((char*)ssh->kbAuth.promptLanguage) : 0;
authData.sf.keyboard.prompts = ssh->kbAuth.prompts;
authData.sf.keyboard.promptEcho = ssh->kbAuth.promptEcho;
authData.sf.keyboard.responseCount = 0;

WLOG(WS_LOG_DEBUG, "SUAR: Calling the userauth callback");
ret = ssh->ctx->userAuthCb(WOLFSSH_USERAUTH_KEYBOARD, &authData,
ssh->userAuthCtx);
WLOG(WS_LOG_DEBUG, "Entering SendUserAuthKeyboardResponse()");

WFREE(ssh->kbAuth.promptName, ssh->ctx->heap, 0);
WFREE(ssh->kbAuth.promptInstruction, ssh->ctx->heap, 0);
WFREE(ssh->kbAuth.promptLanguage, ssh->ctx->heap, 0);
WFREE(ssh->kbAuth.promptEcho, ssh->ctx->heap, 0);
for (prompt = 0; prompt < ssh->kbAuth.promptCount; prompt++) {
WFREE((void*)ssh->kbAuth.prompts[prompt], ssh->ctx->heap, 0);
if (ssh == NULL || ssh->ctx == NULL) {
ret = WS_BAD_ARGUMENT;
}
Comment on lines +15433 to 15435
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

The PR description mentions validating an invalid ssh (or missing ctx), but the added regressions only cover missing userAuthCb and PreparePacket() failure. Add a small regression for SendUserAuthKeyboardResponse(NULL) (and/or a case with ssh->ctx == NULL if constructible in the harness) to lock in the new behavior and prevent future regressions.

Copilot uses AI. Check for mistakes.
WFREE(ssh->kbAuth.prompts, ssh->ctx->heap, 0);

if (ret != WOLFSSH_USERAUTH_SUCCESS) {
WLOG(WS_LOG_DEBUG, "SUAR: Couldn't get keyboard auth");
ret = WS_FATAL_ERROR;
if (ret == WS_SUCCESS && ssh->ctx->userAuthCb == NULL) {
ret = WS_INVALID_STATE_E;
}
else if (ssh->kbAuth.promptCount != authData.sf.keyboard.responseCount) {
WLOG(WS_LOG_DEBUG,
"SUAR: Keyboard auth response count does not match request count");
ret = WS_USER_AUTH_E;

if (ret == WS_SUCCESS) {
authData.type = WOLFSSH_USERAUTH_KEYBOARD;
authData.username = (const byte*)ssh->userName;
authData.usernameSz = ssh->userNameSz;
authData.sf.keyboard.promptCount = ssh->kbAuth.promptCount;
authData.sf.keyboard.promptName = ssh->kbAuth.promptName;
authData.sf.keyboard.promptNameSz = ssh->kbAuth.promptName ?
(word32)WSTRLEN((char*)ssh->kbAuth.promptName) : 0;
authData.sf.keyboard.promptInstruction = ssh->kbAuth.promptInstruction;
authData.sf.keyboard.promptInstructionSz = ssh->kbAuth.promptInstruction ?
(word32)WSTRLEN((char*)ssh->kbAuth.promptInstruction) : 0;
authData.sf.keyboard.promptLanguage = ssh->kbAuth.promptLanguage;
authData.sf.keyboard.promptLanguageSz = ssh->kbAuth.promptLanguage ?
(word32)WSTRLEN((char*)ssh->kbAuth.promptLanguage) : 0;
authData.sf.keyboard.prompts = ssh->kbAuth.prompts;
authData.sf.keyboard.promptEcho = ssh->kbAuth.promptEcho;
authData.sf.keyboard.responseCount = 0;

WLOG(WS_LOG_DEBUG, "SUAR: Calling the userauth callback");
authRet = ssh->ctx->userAuthCb(WOLFSSH_USERAUTH_KEYBOARD, &authData,
ssh->userAuthCtx);
}

if (ret == WS_SUCCESS) {
if (authRet != WOLFSSH_USERAUTH_SUCCESS) {
WLOG(WS_LOG_DEBUG, "SUAR: Couldn't get keyboard auth");
ret = WS_FATAL_ERROR;
}
else if (ssh->kbAuth.promptCount != authData.sf.keyboard.responseCount) {
WLOG(WS_LOG_DEBUG,
"SUAR: Keyboard auth response count does not match request count");
ret = WS_USER_AUTH_E;
}
else {
WLOG(WS_LOG_DEBUG, "SUAR: Callback successful keyboard");
}
}
else {
WLOG(WS_LOG_DEBUG, "SUAR: Callback successful keyboard");

if (ssh != NULL && ssh->ctx != NULL) {
WFREE(ssh->kbAuth.promptName, ssh->ctx->heap, 0);
WFREE(ssh->kbAuth.promptInstruction, ssh->ctx->heap, 0);
WFREE(ssh->kbAuth.promptLanguage, ssh->ctx->heap, 0);
WFREE(ssh->kbAuth.promptEcho, ssh->ctx->heap, 0);
if (ssh->kbAuth.prompts != NULL) {
for (prompt = 0; prompt < ssh->kbAuth.promptCount; prompt++) {
WFREE((void*)ssh->kbAuth.prompts[prompt], ssh->ctx->heap, 0);
}
}
WFREE(ssh->kbAuth.prompts, ssh->ctx->heap, 0);

ssh->kbAuth.promptName = NULL;
ssh->kbAuth.promptInstruction = NULL;
ssh->kbAuth.promptLanguage = NULL;
ssh->kbAuth.promptEcho = NULL;
ssh->kbAuth.prompts = NULL;
ssh->kbAuth.promptCount = 0;
}

payloadSz = MSG_ID_SZ;
Expand All @@ -15479,13 +15504,13 @@ int SendUserAuthKeyboardResponse(WOLFSSH* ssh)
ret = PreparePacket(ssh, payloadSz);
}

output = ssh->outputBuffer.buffer;
idx = ssh->outputBuffer.length;

output[idx++] = MSGID_USERAUTH_INFO_RESPONSE;
if (ret == WS_SUCCESS) {
output = ssh->outputBuffer.buffer;
idx = ssh->outputBuffer.length;

if (ret == WS_SUCCESS)
output[idx++] = MSGID_USERAUTH_INFO_RESPONSE;
ret = BuildUserAuthResponseKeyboard(ssh, output, &idx, &authData);
}

if (ret == WS_SUCCESS) {
ssh->outputBuffer.length = idx;
Expand Down
113 changes: 113 additions & 0 deletions tests/regress.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,114 @@ static void TestSftpBufferSendPendingOutput(void)
#endif /* WOLFSSH_SFTP */


#ifdef WOLFSSH_KEYBOARD_INTERACTIVE
static int KbPreparePacketFailUserAuth(byte authType, WS_UserAuthData* authData,
void* ctx)
{
static byte* responses[1];
static word32 responseLens[1];
static byte response[] = "regress";

(void)ctx;

if (authType != WOLFSSH_USERAUTH_KEYBOARD || authData == NULL) {
return WOLFSSH_USERAUTH_INVALID_AUTHTYPE;
}

if (authData->sf.keyboard.promptCount != 1 ||
authData->sf.keyboard.prompts == NULL) {
return WOLFSSH_USERAUTH_INVALID_PASSWORD;
}

responses[0] = response;
responseLens[0] = (word32)sizeof(response) - 1;
authData->sf.keyboard.responseCount = 1;
authData->sf.keyboard.responseLengths = responseLens;
authData->sf.keyboard.responses = responses;

return WOLFSSH_USERAUTH_SUCCESS;
}

static void TestKeyboardResponsePreparePacketFailure(WOLFSSH* ssh,
WOLFSSH_CTX* ctx)
{
byte* prompt;
byte** prompts;
byte* promptEcho;
int ret;
byte* savedBuffer;

AssertNotNull(ssh);
AssertNotNull(ctx);

ResetSession(ssh);
wolfSSH_SetUserAuth(ctx, KbPreparePacketFailUserAuth);

prompt = (byte*)WMALLOC(9, ctx->heap, DYNTYPE_STRING); /* "Password" */
prompts = (byte**)WMALLOC(sizeof(byte*), ctx->heap, DYNTYPE_STRING);
promptEcho = (byte*)WMALLOC(1, ctx->heap, DYNTYPE_STRING);
AssertNotNull(prompt);
AssertNotNull(prompts);
AssertNotNull(promptEcho);

WMEMCPY(prompt, "Password", 8);
prompt[8] = '\0';
prompts[0] = prompt;
promptEcho[0] = 0;

ssh->kbAuth.promptCount = 1;
ssh->kbAuth.prompts = prompts;
ssh->kbAuth.promptEcho = promptEcho;
ssh->kbAuth.promptName = NULL;
ssh->kbAuth.promptInstruction = NULL;
ssh->kbAuth.promptLanguage = NULL;

/* Force PreparePacket() to fail with WS_OVERFLOW_E. */
ssh->outputBuffer.length = 0;
ssh->outputBuffer.idx = 1;

savedBuffer = ssh->outputBuffer.buffer;
ssh->outputBuffer.buffer = NULL;
Comment on lines +622 to +627
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

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

Setting ssh->outputBuffer.buffer = NULL introduces a risk of a null dereference inside PreparePacket() (or any helper it calls) if those functions assume the buffer pointer is valid even when they are about to return an error. To keep the regression deterministic and memory-safe, prefer forcing WS_OVERFLOW_E using only the idx/length invariant (or by using a deliberately too-small but non-NULL buffer, if needed) without nulling the buffer pointer.

Copilot uses AI. Check for mistakes.

ret = SendUserAuthKeyboardResponse(ssh);
AssertIntEQ(ret, WS_OVERFLOW_E);

/* Ensure packet purge/reset happened cleanly. */
AssertIntEQ(ssh->outputBuffer.idx, 0);
AssertIntEQ(ssh->outputBuffer.length, 0);

/* Restore known-good buffer pointer for subsequent tests. */
if (ssh->outputBuffer.buffer == NULL) {
ssh->outputBuffer.buffer = savedBuffer;
}

/* Verify SendUserAuthKeyboardResponse() cleaned up kbAuth state. */
AssertIntEQ(ssh->kbAuth.promptCount, 0);
AssertTrue(ssh->kbAuth.prompts == NULL);
AssertTrue(ssh->kbAuth.promptEcho == NULL);
}

static void TestKeyboardResponseNoUserAuthCallback(WOLFSSH* ssh,
WOLFSSH_CTX* ctx)
{
int ret;

AssertNotNull(ssh);
AssertNotNull(ctx);

ResetSession(ssh);
wolfSSH_SetUserAuth(ctx, NULL);

ret = SendUserAuthKeyboardResponse(ssh);
AssertIntEQ(ret, WS_INVALID_STATE_E);

/* No packet should have been started. */
AssertIntEQ(ssh->outputBuffer.length, 0);
AssertIntEQ(ssh->outputBuffer.idx, 0);
}
#endif /* WOLFSSH_KEYBOARD_INTERACTIVE */


int main(int argc, char** argv)
{
WOLFSSH_CTX* ctx;
Expand Down Expand Up @@ -594,6 +702,11 @@ int main(int argc, char** argv)
TestSftpBufferSendPendingOutput();
#endif

#ifdef WOLFSSH_KEYBOARD_INTERACTIVE
TestKeyboardResponsePreparePacketFailure(ssh, ctx);
TestKeyboardResponseNoUserAuthCallback(ssh, ctx);
#endif

/* TODO: add app-level regressions that simulate stdin EOF/password
* prompts and mid-session socket closes once the test harness can
* drive the wolfssh client without real sockets/tty. */
Expand Down
Loading