diff --git a/apps/wolfssh/common.c b/apps/wolfssh/common.c index 0092fd8c9..6fa7275d0 100644 --- a/apps/wolfssh/common.c +++ b/apps/wolfssh/common.c @@ -652,6 +652,7 @@ int ClientUserAuth(byte authType, ret = WOLFSSH_USERAUTH_SUCCESS; } else if (authType == WOLFSSH_USERAUTH_PASSWORD) { + ClientClearPassword(); printf("Password: "); fflush(stdout); ClientSetEcho(0); @@ -675,12 +676,23 @@ int ClientUserAuth(byte authType, authData->sf.password.passwordSz = (word32)strlen((const char*)userPassword); } + else { + ClientClearPassword(); + } } return ret; } +/* This app does not link examples/client/common.c, so it has its own copy. + * It answers no keyboard-interactive, so only the password needs wiping. */ +void ClientClearPassword(void) +{ + wc_ForceZero(userPassword, sizeof(userPassword)); +} + + /* type = 2 : shell / execute command settings * type = 0 : password * type = 1 : restore default @@ -908,7 +920,7 @@ void ClientFreeBuffers(void) wc_ForceZero(userPrivateKeyBuf, sizeof(userPrivateKeyBuf)); userPrivateKeySz = 0; - wc_ForceZero(userPassword, sizeof(userPassword)); + ClientClearPassword(); } diff --git a/apps/wolfssh/common.h b/apps/wolfssh/common.h index 3868328d6..2d0e11a99 100644 --- a/apps/wolfssh/common.h +++ b/apps/wolfssh/common.h @@ -31,6 +31,9 @@ WOLFSSH_LOCAL int ClientUserAuth(byte authType, WS_UserAuthData* authData, WOLFSSH_LOCAL int ClientPublicKeyCheck(const byte* pubKey, word32 pubKeySz, void* ctx); WOLFSSH_LOCAL void ClientIPOverride(int flag); +/* Wipes the password ClientUserAuth() last read. The library sends it from + * that buffer after the callback returns, so call this once connect is done. */ +WOLFSSH_LOCAL void ClientClearPassword(void); WOLFSSH_LOCAL void ClientFreeBuffers(void); WOLFSSH_LOCAL int ClientParseDestination(const char* in, char** user, char** hostname, word16* port); diff --git a/apps/wolfssh/wolfssh.c b/apps/wolfssh/wolfssh.c index b6b8c2495..feb69b4ef 100644 --- a/apps/wolfssh/wolfssh.c +++ b/apps/wolfssh/wolfssh.c @@ -1273,6 +1273,9 @@ static THREAD_RETURN WOLFSSH_THREAD wolfSSH_Client(void* args) #endif MODES_STORE(); + /* No stdio buffer keeps a copy of a typed password. */ + if (setvbuf(stdin, NULL, _IONBF, 0) != 0) + fprintf(stderr, "Couldn't unbuffer stdin.\n"); ((func_args*)args)->return_code = 0; @@ -1380,6 +1383,7 @@ static THREAD_RETURN WOLFSSH_THREAD wolfSSH_Client(void* args) #endif ret = NonBlockSSH_connect(ssh); + ClientClearPassword(); if (ret != WS_SUCCESS) err_sys("Couldn't connect SSH stream."); diff --git a/examples/client/client.c b/examples/client/client.c index 37463645f..773577a2f 100644 --- a/examples/client/client.c +++ b/examples/client/client.c @@ -1013,6 +1013,10 @@ THREAD_RETURN WOLFSSH_THREAD client_test(void* args) } myoptind = 0; /* reset for test cases */ +#ifdef WOLFSSH_TERM + ClientUnbufferStdin(); +#endif + if (username == NULL) err_sys("client requires a username parameter."); @@ -1225,6 +1229,7 @@ THREAD_RETURN WOLFSSH_THREAD client_test(void* args) ret = wolfSSH_connect(ssh); else ret = NonBlockSSH_connect(ssh); + ClientClearPassword(); if (ret != WS_SUCCESS) { ClientFreeBuffers(pubKeyName, privKeyName, NULL); wolfSSH_free(ssh); diff --git a/examples/client/common.c b/examples/client/common.c index 9e8e9d22e..fbfa3dd8c 100644 --- a/examples/client/common.c +++ b/examples/client/common.c @@ -444,6 +444,23 @@ int ClientPublicKeyCheck(const byte* pubKey, word32 pubKeySz, void* ctx) } +#if defined(WOLFSSH_TERM) && defined(WOLFSSH_KEYBOARD_INTERACTIVE) +/* Prints a server-supplied string with a '.' in place of each byte that is + * a control character other than tab or newline, DEL, or 0x80 and up. */ +static void ClientPrintSafe(const byte* str) +{ + byte c; + + for (; *str != 0; str++) { + c = *str; + if ((c < 0x20 && c != '\t' && c != '\n') || c >= 0x7F) + c = '.'; + putchar(c); + } +} +#endif + + int ClientUserAuth(byte authType, WS_UserAuthData* authData, void* ctx) @@ -510,6 +527,7 @@ int ClientUserAuth(byte authType, ret = WOLFSSH_USERAUTH_SUCCESS; } else if (authType == WOLFSSH_USERAUTH_PASSWORD) { + ClientClearPassword(); if (defaultPassword != NULL) { passwordSz = (word32)strlen(defaultPassword); if (passwordSz > (word32)sizeof(userPassword)) @@ -544,6 +562,9 @@ int ClientUserAuth(byte authType, authData->sf.password.password = userPassword; authData->sf.password.passwordSz = passwordSz; } + else { + ClientClearPassword(); + } } #if defined(WOLFSSH_TERM) && defined(WOLFSSH_KEYBOARD_INTERACTIVE) else if (authType == WOLFSSH_USERAUTH_KEYBOARD) { @@ -551,11 +572,13 @@ int ClientUserAuth(byte authType, if (authData->sf.keyboard.promptName && authData->sf.keyboard.promptName[0] != '\0') { - printf("%s\n", authData->sf.keyboard.promptName); + ClientPrintSafe(authData->sf.keyboard.promptName); + printf("\n"); } if (authData->sf.keyboard.promptInstruction && authData->sf.keyboard.promptInstruction[0] != '\0') { - printf("%s\n", authData->sf.keyboard.promptInstruction); + ClientPrintSafe(authData->sf.keyboard.promptInstruction); + printf("\n"); } /* The server may send another INFO_REQUEST at any time. */ @@ -589,7 +612,7 @@ int ClientUserAuth(byte authType, * but test the one this function actually returns. */ for (entry = 0; ret == WOLFSSH_USERAUTH_SUCCESS && entry < promptCount; entry++) { - printf("%s", authData->sf.keyboard.prompts[entry]); + ClientPrintSafe(authData->sf.keyboard.prompts[entry]); if (!authData->sf.keyboard.promptEcho[entry]) { ClientSetEcho(0); } @@ -597,6 +620,7 @@ int ClientUserAuth(byte authType, == NULL) { fprintf(stderr, "Getting response failed.\n"); ClientSetEcho(1); + wc_ForceZero(userPassword, sizeof(userPassword)); ret = WOLFSSH_USERAUTH_FAILURE; break; } @@ -613,6 +637,7 @@ int ClientUserAuth(byte authType, WFFLUSH(stdout); keyboardResponses[entry] = (byte*) WSTRDUP((char*)userPassword, NULL, 0); + wc_ForceZero(userPassword, sizeof(userPassword)); if (keyboardResponses[entry] == NULL) { ret = WS_MEMORY_E; break; @@ -633,7 +658,32 @@ int ClientUserAuth(byte authType, } +/* The wolfssh app has its own copy in apps/wolfssh/common.c, with no + * keyboard-interactive responses to free. The keyboard loop above wipes + * userPassword directly: this would free the responses authData still + * points at. */ +void ClientClearPassword(void) +{ +#ifdef WOLFSSH_KEYBOARD_INTERACTIVE + ClientFreeKeyboardResponses(); +#endif + wc_ForceZero(userPassword, sizeof(userPassword)); +} + + #ifdef WOLFSSH_TERM +void ClientUnbufferStdin(void) +{ + static int done = 0; + + if (!done) { + done = 1; + if (setvbuf(stdin, NULL, _IONBF, 0) != 0) + fprintf(stderr, "Couldn't unbuffer stdin.\n"); + } +} + + /* type = 2 : shell / execute command settings * type = 0 : password * type = 1 : restore default @@ -1245,12 +1295,9 @@ void ClientFreeBuffers(const char* pubKeyName, const char* privKeyName, userPrivateKeyTypeSz = 0; } -#ifdef WOLFSSH_KEYBOARD_INTERACTIVE - ClientFreeKeyboardResponses(); -#endif + ClientClearPassword(); wc_ForceZero(userPrivateKeyBuf, sizeof(userPrivateKeyBuf)); userPrivateKeySz = 0; - wc_ForceZero(userPassword, sizeof(userPassword)); pubKeyLoaded = 0; } diff --git a/examples/client/common.h b/examples/client/common.h index 81ffd078c..7f43eb14f 100644 --- a/examples/client/common.h +++ b/examples/client/common.h @@ -32,10 +32,18 @@ int ClientSetPrivateKey(const char* privKeyName, int userEcc, void* heap, const char* tpmKeyAuth); int ClientUseCert(const char* certName, void* heap); int ClientSetEcho(int type); +/* Makes stdin unbuffered, so no stdio buffer keeps a copy of a typed + * password or response. Call before any other stdin I/O; only the first + * call in a process acts. */ +void ClientUnbufferStdin(void); int ClientUserAuth(byte authType, WS_UserAuthData* authData, void* ctx); int ClientPublicKeyCheck(const byte* pubKey, word32 pubKeySz, void* ctx); void ClientIPOverride(int flag); +/* Wipes the password and keyboard-interactive responses ClientUserAuth() + * last read. The library sends them after the callback returns, so call + * this once connect is done. */ +void ClientClearPassword(void); void ClientFreeBuffers(const char* pubKeyName, const char* privKeyName, void* heap); #ifdef WOLFSSH_TPM diff --git a/examples/scpclient/scpclient.c b/examples/scpclient/scpclient.c index 33994a6e5..911c0cfca 100644 --- a/examples/scpclient/scpclient.c +++ b/examples/scpclient/scpclient.c @@ -196,6 +196,10 @@ THREAD_RETURN WOLFSSH_THREAD scp_client(void* args) myoptind = 0; /* reset for test cases */ +#ifdef WOLFSSH_TERM + ClientUnbufferStdin(); +#endif + if (username == NULL) err_sys("client requires a username parameter."); diff --git a/examples/sftpclient/sftpclient.c b/examples/sftpclient/sftpclient.c index 097f70b27..c18d1b4a0 100644 --- a/examples/sftpclient/sftpclient.c +++ b/examples/sftpclient/sftpclient.c @@ -1722,6 +1722,10 @@ THREAD_RETURN WOLFSSH_THREAD sftpclient_test(void* args) } myoptind = 0; /* reset for test cases */ +#ifdef WOLFSSH_TERM + ClientUnbufferStdin(); +#endif + if (username == NULL) err_sys("client requires a username parameter."); @@ -1896,6 +1900,7 @@ THREAD_RETURN WOLFSSH_THREAD sftpclient_test(void* args) ret = wolfSSH_SFTP_connect(ssh); else ret = NonBlockSSH_connect(); + ClientClearPassword(); if (ret != WS_SUCCESS) { /* the return is a generic failure code; the real cause is kept in * the session error */ diff --git a/scripts/sshclient.test b/scripts/sshclient.test index 4e8c26843..2feaa7754 100755 --- a/scripts/sshclient.test +++ b/scripts/sshclient.test @@ -136,10 +136,9 @@ wait_for_output() { # Run the client with its stdin coming from a fifo. # -# The client answers its prompts with stdio, which buffers everything that -# is ready to be read, so anything written along with a prompt's answer is -# swallowed with it and never reaches the session. Writing each piece only -# once the client has asked for it keeps them in separate reads. The +# The client reads its prompt answers from an unbuffered stdin, so the +# session data can follow the password at once and is left in the fifo for +# the session. Each answer is still written only once its prompt is up. The # echoserver only ends the session when it receives a 0x03, so every # session has to send one or both ends wait for the other forever. # @@ -161,11 +160,6 @@ run_client() { wait_for_output "Password:" || exit 1 printf 'upthehill\n' - - # Let the client consume the password before sending the session - # data. A single read that catches both loses the data. - sleep 2 - printf 'hello\003' ) > "$input_file" 2>/dev/null & input_pid=$! diff --git a/tests/auth.c b/tests/auth.c index 27ee85904..5c03f67db 100644 --- a/tests/auth.c +++ b/tests/auth.c @@ -2739,6 +2739,137 @@ static void test_ClientUserAuth_keyboardRounds(void) AssertNotNull(freopen("/dev/null", "r", stdin)); } + +/* A retry with a shorter password leaves nothing of the longer one, + * ClientClearPassword() wipes the buffer the library was handed, and a + * failed read leaves nothing of the last password. */ +static void test_ClientUserAuth_passwordWiped(void) +{ + WS_UserAuthData authData; + const char* fname = "password_wiped.txt"; + WFILE* f = NULL; + const byte* pw; + word32 i; + int ret; + + printf("Testing the password buffer is wiped\n"); + + AssertIntEQ(WFOPEN(NULL, &f, fname, "w"), 0); + AssertNotNull(f); + AssertIntGT(fprintf(f, "longerpassword\nshort\nagain\n"), 0); + WFCLOSE(NULL, f); + AssertNotNull(freopen(fname, "r", stdin)); + AssertIntEQ(remove(fname), 0); + + WMEMSET(&authData, 0, sizeof(authData)); + authData.type = WOLFSSH_USERAUTH_PASSWORD; + ret = ClientUserAuth(WOLFSSH_USERAUTH_PASSWORD, &authData, NULL); + AssertIntEQ(ret, WOLFSSH_USERAUTH_SUCCESS); + AssertIntEQ(authData.sf.password.passwordSz, 14); + + WMEMSET(&authData, 0, sizeof(authData)); + authData.type = WOLFSSH_USERAUTH_PASSWORD; + ret = ClientUserAuth(WOLFSSH_USERAUTH_PASSWORD, &authData, NULL); + AssertIntEQ(ret, WOLFSSH_USERAUTH_SUCCESS); + AssertIntEQ(authData.sf.password.passwordSz, 5); + pw = authData.sf.password.password; + AssertIntEQ(WMEMCMP(pw, "short", 6), 0); + for (i = 6; i < 16; i++) + AssertIntEQ(pw[i], 0); + + ClientClearPassword(); + for (i = 0; i < 16; i++) + AssertIntEQ(pw[i], 0); + + WMEMSET(&authData, 0, sizeof(authData)); + authData.type = WOLFSSH_USERAUTH_PASSWORD; + ret = ClientUserAuth(WOLFSSH_USERAUTH_PASSWORD, &authData, NULL); + AssertIntEQ(ret, WOLFSSH_USERAUTH_SUCCESS); + AssertIntEQ(WMEMCMP(pw, "again", 6), 0); + + /* stdin is at EOF */ + WMEMSET(&authData, 0, sizeof(authData)); + authData.type = WOLFSSH_USERAUTH_PASSWORD; + ret = ClientUserAuth(WOLFSSH_USERAUTH_PASSWORD, &authData, NULL); + AssertIntEQ(ret, WOLFSSH_USERAUTH_FAILURE); + AssertNull(authData.sf.password.password); + for (i = 0; i < 16; i++) + AssertIntEQ(pw[i], 0); + + ClientFreeBuffers(NULL, NULL, NULL); + AssertNotNull(freopen("/dev/null", "r", stdin)); +} + +/* Server-supplied keyboard-interactive strings reach the terminal with a + * '.' for each control byte and each byte 0x80 and up. */ +static void test_ClientUserAuth_keyboardFiltered(void) +{ + WS_UserAuthData authData; + byte* prompts[1]; + byte promptEcho[1]; + const char* inName = "kb_filter_in.txt"; + const char* outName = "kb_filter_out.txt"; + WFILE* f = NULL; + WFILE* outFile = NULL; + char out[512]; + size_t outSz; + size_t i; + int savedStdout; + int ret; + + printf("Testing keyboard-interactive strings are filtered\n"); + + prompts[0] = (byte*)"p\rq\x9b\x7f: "; + promptEcho[0] = 1; + + AssertIntEQ(WFOPEN(NULL, &f, inName, "w"), 0); + AssertNotNull(f); + AssertIntGT(fprintf(f, "resp\n"), 0); + WFCLOSE(NULL, f); + AssertNotNull(freopen(inName, "r", stdin)); + AssertIntEQ(remove(inName), 0); + + KbSetupAuthData(&authData, 1, prompts, promptEcho); + authData.sf.keyboard.promptName = (byte*)"a\x1b[2Jb"; + authData.sf.keyboard.promptInstruction = + (byte*)"\xc2\x9b x \xc3\xa9\t\xa0\xe0\x82\x9b."; + + /* Capture through the descriptor alone, leaving the stdout FILE and its + * buffering mode as they were. The capture file is unlinked while open + * so nothing is left behind if an assert aborts. */ + AssertIntEQ(WFOPEN(NULL, &outFile, outName, "w+"), 0); + AssertNotNull(outFile); + AssertIntEQ(remove(outName), 0); + fflush(stdout); + savedStdout = dup(STDOUT_FILENO); + AssertTrue(savedStdout >= 0); + AssertTrue(dup2(fileno(outFile), STDOUT_FILENO) >= 0); + ret = ClientUserAuth(WOLFSSH_USERAUTH_KEYBOARD, &authData, NULL); + fflush(stdout); + AssertTrue(dup2(savedStdout, STDOUT_FILENO) >= 0); + close(savedStdout); + + AssertIntEQ(ret, WOLFSSH_USERAUTH_SUCCESS); + AssertIntEQ(authData.sf.keyboard.responseCount, 1); + + rewind(outFile); + outSz = fread(out, 1, sizeof(out) - 1, outFile); + WFCLOSE(NULL, outFile); + out[outSz] = '\0'; + + AssertNotNull(WSTRSTR(out, "a.[2Jb\n")); + AssertNotNull(WSTRSTR(out, ".. x ..\t.....\n")); + AssertNotNull(WSTRSTR(out, "p.q..: ")); + for (i = 0; i < outSz; i++) { + AssertTrue((byte)out[i] < 0x7F); + AssertTrue((byte)out[i] >= 0x20 || out[i] == '\t' || + out[i] == '\n'); + } + + ClientFreeBuffers(NULL, NULL, NULL); + AssertNotNull(freopen("/dev/null", "r", stdin)); +} + #endif /* keyboard-interactive ClientUserAuth tests */ int wolfSSH_AuthTest(int argc, char** argv) @@ -2830,6 +2961,8 @@ int wolfSSH_AuthTest(int argc, char** argv) test_ClientUserAuth_keyboardEof(); test_ClientUserAuth_keyboardZeroPrompts(); test_ClientUserAuth_keyboardRounds(); + test_ClientUserAuth_passwordWiped(); + test_ClientUserAuth_keyboardFiltered(); #endif /* Unknown callback return value must not grant auth (issue 2486) */ diff --git a/tests/regress.c b/tests/regress.c index 235d36508..71908804d 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -15507,6 +15507,73 @@ static void TestPasswordEofNoCrash(void) ClientFreeBuffers(); } +#define TEST_STDIN_FILE "regress_stdin.txt" + +/* Points stdin at a file holding text. The caller removes the file once + * stdin is reopened elsewhere; Windows cannot remove an open file. */ +static void StdinFromText(const char* text) +{ + FILE* f; + + f = fopen(TEST_STDIN_FILE, "w"); + AssertNotNull(f); + AssertTrue(fputs(text, f) >= 0); + AssertIntEQ(fclose(f), 0); + AssertNotNull(freopen(TEST_STDIN_FILE, "r", stdin)); +} + +/* A retry with a shorter password leaves nothing of the longer one, + * ClientClearPassword() wipes the buffer the library was handed, and a + * failed read leaves nothing of the last password. */ +static void TestPasswordBufferWiped(void) +{ + WS_UserAuthData auth; + const byte* pw; + word32 i; + + StdinFromText("longerpassword\nshort\nagain\n"); + + WMEMSET(&auth, 0, sizeof(auth)); + auth.type = WOLFSSH_USERAUTH_PASSWORD; + AssertIntEQ(ClientUserAuth(WOLFSSH_USERAUTH_PASSWORD, &auth, NULL), + WOLFSSH_USERAUTH_SUCCESS); + AssertIntEQ(auth.sf.password.passwordSz, 14); + AssertIntEQ(WMEMCMP(auth.sf.password.password, "longerpassword", 14), 0); + + WMEMSET(&auth, 0, sizeof(auth)); + auth.type = WOLFSSH_USERAUTH_PASSWORD; + AssertIntEQ(ClientUserAuth(WOLFSSH_USERAUTH_PASSWORD, &auth, NULL), + WOLFSSH_USERAUTH_SUCCESS); + AssertIntEQ(auth.sf.password.passwordSz, 5); + pw = auth.sf.password.password; + AssertIntEQ(WMEMCMP(pw, "short", 6), 0); + for (i = 6; i < 16; i++) + AssertIntEQ(pw[i], 0); + + ClientClearPassword(); + for (i = 0; i < 16; i++) + AssertIntEQ(pw[i], 0); + + WMEMSET(&auth, 0, sizeof(auth)); + auth.type = WOLFSSH_USERAUTH_PASSWORD; + AssertIntEQ(ClientUserAuth(WOLFSSH_USERAUTH_PASSWORD, &auth, NULL), + WOLFSSH_USERAUTH_SUCCESS); + AssertIntEQ(WMEMCMP(pw, "again", 6), 0); + + /* stdin is at EOF */ + WMEMSET(&auth, 0, sizeof(auth)); + auth.type = WOLFSSH_USERAUTH_PASSWORD; + AssertIntEQ(ClientUserAuth(WOLFSSH_USERAUTH_PASSWORD, &auth, NULL), + WOLFSSH_USERAUTH_FAILURE); + AssertNull(auth.sf.password.password); + for (i = 0; i < 16; i++) + AssertIntEQ(pw[i], 0); + + ClientFreeBuffers(); + AssertNotNull(freopen(TEST_NULL_DEVICE, "r", stdin)); + AssertIntEQ(remove(TEST_STDIN_FILE), 0); +} + /* An idle receive still gets its flush. The receive runs first and reports * want-read, and the queued byte is pushed anyway, so ssh->error carries the * write the socket would not take. */ @@ -20802,6 +20869,7 @@ int main(int argc, char** argv) TestUserAuthPkWithoutServerSigAlgs(); #endif TestPasswordEofNoCrash(); + TestPasswordBufferWiped(); #ifndef NO_WOLFSSH_CLIENT TestWorkerReadsWhenSendWouldBlock(); #endif