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
+198 −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;
}
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
130 changes: 130 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,129 @@ 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;

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;

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

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

/* 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);
}

static void TestKeyboardResponseNullSsh(void)
{
int ret;

ret = SendUserAuthKeyboardResponse(NULL);
AssertIntEQ(ret, WS_BAD_ARGUMENT);
}

static void TestKeyboardResponseNullCtx(WOLFSSH* ssh)
{
WOLFSSH_CTX* savedCtx;
int ret;

AssertNotNull(ssh);

savedCtx = ssh->ctx;
ssh->ctx = NULL;

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

ssh->ctx = savedCtx;
}
#endif /* WOLFSSH_KEYBOARD_INTERACTIVE */


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

#ifdef WOLFSSH_KEYBOARD_INTERACTIVE
TestKeyboardResponsePreparePacketFailure(ssh, ctx);
TestKeyboardResponseNoUserAuthCallback(ssh, ctx);
TestKeyboardResponseNullSsh();
TestKeyboardResponseNullCtx(ssh);
#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