From 787c358d373bbef8383b385770747b1a6a4ab3ca Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Wed, 18 May 2022 13:04:56 +0100 Subject: [PATCH] Fix command-line password handling in Restart Session. When the user provides a password on the PuTTY command line, via -pw or -pwfile, the flag 'tried_once' inside cmdline_get_passwd_input() is intended to arrange that we only try sending that password once, and after we've sent it, we don't try again. But this plays badly with the 'Restart Session' operation. If the connection is lost and then restarted at user request, we _do_ want to send that password again! So this commit moves that static variable out into a small state structure held by the client of cmdline_get_passwd_input. Each client can decide how to manage that state itself. Clients that support 'Restart Session' - i.e. just GUI PuTTY itself - will initialise the state at the same time as instantiating the backend, so that every time the session is restarted, we return to (correctly) believing that we _haven't_ yet tried the password provided on the command line. But clients that don't support 'Restart Session' - i.e. Plink and file transfer tools - can do the same thing that cmdline.c was doing before: just keep the state in a static variable. This also means that the GUI login tools will now retain the command-line password in memory, whereas previously they'd have wiped it out once it was used. But the other tools will still wipe and free the password, because I've also added a 'bool restartable' flag to cmdline_get_passwd_input to let it know when it _is_ allowed to do that. In the GUI tools, I don't see any way to get round that, because if the session is restarted you _have_ to still have the password to use again. (And you can't infer that that will never happen from the CONF_close_on_exit setting, because that too could be changed in mid-session.) On the other hand, I think it's not all that worrying, because the use of either -pw or -pwfile means that a persistent copy of your password is *already* stored somewhere, so another one isn't too big a stretch. (Due to the change of -pw policy in 0.77, the effect of this bug was that an attempt to reconnect in a session set up this way would lead to "Configured password was not accepted". In 0.76, the failure mode was different: PuTTY would interactively prompt for the password, having wiped it out of memory after it was used the first time round.) --- cmdline.c | 28 ++++++++++++++-------- defs.h | 2 ++ putty.h | 7 +++++- stubs/nocmdline.c | 3 ++- unix/plink.c | 7 +++++- unix/sftp.c | 8 ++++++- unix/window.c | 5 +++- utils/CMakeLists.txt | 1 + utils/cmdline_get_passwd_input_state_new.c | 9 +++++++ windows/plink.c | 7 +++++- windows/sftp.c | 8 ++++++- windows/window.c | 6 ++++- 12 files changed, 73 insertions(+), 18 deletions(-) create mode 100644 utils/cmdline_get_passwd_input_state_new.c diff --git a/cmdline.c b/cmdline.c index 9f37f2a0..8e48b358 100644 --- a/cmdline.c +++ b/cmdline.c @@ -81,10 +81,9 @@ void cmdline_cleanup(void) * -1 return means that we aren't capable of processing the prompt and * someone else should do it. */ -SeatPromptResult cmdline_get_passwd_input(prompts_t *p) +SeatPromptResult cmdline_get_passwd_input( + prompts_t *p, cmdline_get_passwd_input_state *state, bool restartable) { - static bool tried_once = false; - /* * We only handle prompts which don't echo (which we assume to be * passwords), and (currently) we only cope with a password prompt @@ -98,23 +97,32 @@ SeatPromptResult cmdline_get_passwd_input(prompts_t *p) * If we've tried once, return utter failure (no more passwords left * to try). */ - if (tried_once) + if (state->tried) return SPR_SW_ABORT("Configured password was not accepted"); /* * If we never had a password available in the first place, we * can't do anything in any case. (But we delay this test until - * after tried_once, so that after we free cmdline_password below, - * we'll still remember that we _used_ to have one.) + * after trying once, so that even if we free cmdline_password + * below, we'll still remember that we _used_ to have one.) */ if (!cmdline_password) return SPR_INCOMPLETE; prompt_set_result(p->prompts[0], cmdline_password); - smemclr(cmdline_password, strlen(cmdline_password)); - sfree(cmdline_password); - cmdline_password = NULL; - tried_once = true; + state->tried = true; + + if (!restartable) { + /* + * If there's no possibility of needing to do this again after + * a 'Restart Session' event, then wipe our copy of the + * password out of memory. + */ + smemclr(cmdline_password, strlen(cmdline_password)); + sfree(cmdline_password); + cmdline_password = NULL; + } + return SPR_OK; } diff --git a/defs.h b/defs.h index 354c208f..f1bbf51a 100644 --- a/defs.h +++ b/defs.h @@ -118,6 +118,8 @@ typedef struct Seat Seat; typedef struct SeatVtable SeatVtable; typedef struct SeatPromptResult SeatPromptResult; +typedef struct cmdline_get_passwd_input_state cmdline_get_passwd_input_state; + typedef struct TermWin TermWin; typedef struct TermWinVtable TermWinVtable; diff --git a/putty.h b/putty.h index fc5c2941..9a8c4a1f 100644 --- a/putty.h +++ b/putty.h @@ -2525,10 +2525,15 @@ void printer_finish_job(printer_job *); * zero out password arguments in the hope of not having them show up * avoidably in Unix 'ps'. */ +struct cmdline_get_passwd_input_state { bool tried; }; +#define CMDLINE_GET_PASSWD_INPUT_STATE_INIT { .tried = false } +extern const cmdline_get_passwd_input_state cmdline_get_passwd_input_state_new; + int cmdline_process_param(const char *, char *, int, Conf *); void cmdline_run_saved(Conf *); void cmdline_cleanup(void); -SeatPromptResult cmdline_get_passwd_input(prompts_t *p); +SeatPromptResult cmdline_get_passwd_input( + prompts_t *p, cmdline_get_passwd_input_state *state, bool restartable); bool cmdline_host_ok(Conf *); bool cmdline_verbose(void); bool cmdline_loaded_session(void); diff --git a/stubs/nocmdline.c b/stubs/nocmdline.c index de3cfd0d..60e2cb6b 100644 --- a/stubs/nocmdline.c +++ b/stubs/nocmdline.c @@ -15,7 +15,8 @@ * handling, then there is no such option, so that function always * returns failure. */ -SeatPromptResult cmdline_get_passwd_input(prompts_t *p) +SeatPromptResult cmdline_get_passwd_input( + prompts_t *p, cmdline_get_passwd_input_state *state, bool restartable) { return SPR_INCOMPLETE; } diff --git a/unix/plink.c b/unix/plink.c index 8f53ceda..9e109f01 100644 --- a/unix/plink.c +++ b/unix/plink.c @@ -373,8 +373,13 @@ static bool plink_eof(Seat *seat) static SeatPromptResult plink_get_userpass_input(Seat *seat, prompts_t *p) { + /* Plink doesn't support Restart Session, so we can just have a + * single static cmdline_get_passwd_input_state that's never reset */ + static cmdline_get_passwd_input_state cmdline_state = + CMDLINE_GET_PASSWD_INPUT_STATE_INIT; + SeatPromptResult spr; - spr = cmdline_get_passwd_input(p); + spr = cmdline_get_passwd_input(p, &cmdline_state, false); if (spr.kind == SPRK_INCOMPLETE) spr = console_get_userpass_input(p); return spr; diff --git a/unix/sftp.c b/unix/sftp.c index 17a83a89..9d099f55 100644 --- a/unix/sftp.c +++ b/unix/sftp.c @@ -65,8 +65,14 @@ Filename *platform_default_filename(const char *name) SeatPromptResult filexfer_get_userpass_input(Seat *seat, prompts_t *p) { + /* The file transfer tools don't support Restart Session, so we + * can just have a single static cmdline_get_passwd_input_state + * that's never reset */ + static cmdline_get_passwd_input_state cmdline_state = + CMDLINE_GET_PASSWD_INPUT_STATE_INIT; + SeatPromptResult spr; - spr = cmdline_get_passwd_input(p); + spr = cmdline_get_passwd_input(p, &cmdline_state, false); if (spr.kind == SPRK_INCOMPLETE) spr = console_get_userpass_input(p); return spr; diff --git a/unix/window.c b/unix/window.c index c2e79bf0..173943cb 100644 --- a/unix/window.c +++ b/unix/window.c @@ -160,6 +160,7 @@ struct GtkFrontend { Ldisc *ldisc; Backend *backend; Terminal *term; + cmdline_get_passwd_input_state cmdline_get_passwd_state; LogContext *logctx; bool exited; struct unicode_data ucsdata; @@ -343,7 +344,7 @@ static SeatPromptResult gtk_seat_get_userpass_input(Seat *seat, prompts_t *p) { GtkFrontend *inst = container_of(seat, GtkFrontend, seat); SeatPromptResult spr; - spr = cmdline_get_passwd_input(p); + spr = cmdline_get_passwd_input(p, &inst->cmdline_get_passwd_state, true); if (spr.kind == SPRK_INCOMPLETE) spr = term_get_userpass_input(inst->term, p); return spr; @@ -5105,6 +5106,8 @@ static void start_backend(GtkFrontend *inst) const struct BackendVtable *vt; char *error, *realhost; + inst->cmdline_get_passwd_state = cmdline_get_passwd_input_state_new; + vt = select_backend(inst->conf); seat_set_trust_status(&inst->seat, true); diff --git a/utils/CMakeLists.txt b/utils/CMakeLists.txt index 80fc20b8..2e1296eb 100644 --- a/utils/CMakeLists.txt +++ b/utils/CMakeLists.txt @@ -7,6 +7,7 @@ add_sources_from_current_dir(utils buildinfo.c burnstr.c chomp.c + cmdline_get_passwd_input_state_new.c conf.c conf_dest.c conf_launchable.c diff --git a/utils/cmdline_get_passwd_input_state_new.c b/utils/cmdline_get_passwd_input_state_new.c new file mode 100644 index 00000000..cd39bfa1 --- /dev/null +++ b/utils/cmdline_get_passwd_input_state_new.c @@ -0,0 +1,9 @@ +/* + * A preinitialised cmdline_get_passwd_input_state which makes it easy + * to assign by structure copy. + */ + +#include "putty.h" + +const cmdline_get_passwd_input_state cmdline_get_passwd_input_state_new = + CMDLINE_GET_PASSWD_INPUT_STATE_INIT; diff --git a/windows/plink.c b/windows/plink.c index af95c2b9..94bf4f0c 100644 --- a/windows/plink.c +++ b/windows/plink.c @@ -67,8 +67,13 @@ static bool plink_eof(Seat *seat) static SeatPromptResult plink_get_userpass_input(Seat *seat, prompts_t *p) { + /* Plink doesn't support Restart Session, so we can just have a + * single static cmdline_get_passwd_input_state that's never reset */ + static cmdline_get_passwd_input_state cmdline_state = + CMDLINE_GET_PASSWD_INPUT_STATE_INIT; + SeatPromptResult spr; - spr = cmdline_get_passwd_input(p); + spr = cmdline_get_passwd_input(p, &cmdline_state, false); if (spr.kind == SPRK_INCOMPLETE) spr = console_get_userpass_input(p); return spr; diff --git a/windows/sftp.c b/windows/sftp.c index ad2df4fb..fc8e711b 100644 --- a/windows/sftp.c +++ b/windows/sftp.c @@ -14,8 +14,14 @@ SeatPromptResult filexfer_get_userpass_input(Seat *seat, prompts_t *p) { + /* The file transfer tools don't support Restart Session, so we + * can just have a single static cmdline_get_passwd_input_state + * that's never reset */ + static cmdline_get_passwd_input_state cmdline_state = + CMDLINE_GET_PASSWD_INPUT_STATE_INIT; + SeatPromptResult spr; - spr = cmdline_get_passwd_input(p); + spr = cmdline_get_passwd_input(p, &cmdline_state, false); if (spr.kind == SPRK_INCOMPLETE) spr = console_get_userpass_input(p); return spr; diff --git a/windows/window.c b/windows/window.c index 399829b6..5a70098c 100644 --- a/windows/window.c +++ b/windows/window.c @@ -130,6 +130,8 @@ static int kbd_codepage; static Ldisc *ldisc; static Backend *backend; +static cmdline_get_passwd_input_state cmdline_get_passwd_state; + static struct unicode_data ucsdata; static bool session_closed; static bool reconfiguring = false; @@ -371,6 +373,8 @@ static void start_backend(void) char *error, *realhost; int i; + cmdline_get_passwd_state = cmdline_get_passwd_input_state_new; + vt = backend_vt_from_conf(conf); seat_set_trust_status(&wgs.seat, true); @@ -5909,7 +5913,7 @@ static bool win_seat_eof(Seat *seat) static SeatPromptResult win_seat_get_userpass_input(Seat *seat, prompts_t *p) { SeatPromptResult spr; - spr = cmdline_get_passwd_input(p); + spr = cmdline_get_passwd_input(p, &cmdline_get_passwd_state, true); if (spr.kind == SPRK_INCOMPLETE) spr = term_get_userpass_input(term, p); return spr;