From 7eb7d5e2e9117132940f6bc865a23aa3cc1b60ff Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 6 Nov 2021 14:33:03 +0000 Subject: [PATCH] New Seat query, has_mixed_input_stream(). (TL;DR: to suppress redundant 'Press Return to begin session' prompts in between hops of a jump-host configuration, in Plink.) This new query method directly asks the Seat the question: is the same stream of input used to provide responses to interactive login prompts, and the session input provided after login concludes? It's used to suppress the last-ditch anti-spoofing defence in Plink of interactively asking 'Access granted. Press Return to begin session', on the basis that any such spoofing attack works by confusing the user about what's a legit login prompt before the session begins and what's sent by the server after the main session begins - so if those two things take input from different places, the user can't be confused. This doesn't change the existing behaviour of Plink, which was already suppressing the antispoof prompt in cases where its standard input was redirected from something other than a terminal. But previously it was doing it within the can_set_trust_status() seat query, and I've now moved it out into a separate query function. The reason why these need to be separate is for SshProxy, which needs to give an unusual combination of answers when run inside Plink. For can_set_trust_status(), it needs to return whatever the parent Seat returns, so that all the login prompts for a string of proxy connections in session will be antispoofed the same way. But you only want that final 'Access granted' prompt to happen _once_, after all the proxy connection setup phases are done, because up until then you're still in the safe hands of PuTTY itself presenting an unbroken sequence of legit login prompts (even if they come from a succession of different servers). Hence, SshProxy unconditionally returns 'no' to the query of whether it has a single mixed input stream, because indeed, it never does - for purposes of session input it behaves like an always-redirected Plink, no matter what kind of real Seat it ends up sending its pre-session login prompts to. --- proxy/sshproxy.c | 1 + pscp.c | 1 + psftp.c | 1 + putty.h | 17 +++++++++++++++++ ssh/connection1-client.c | 6 +++++- ssh/connection2-client.c | 9 +++++++-- ssh/server.c | 1 + ssh/sesschan.c | 1 + unix/console.c | 27 ++++++++++++++++++--------- unix/plink.c | 1 + unix/window.c | 1 + utils/nullseat.c | 2 ++ utils/tempseat.c | 7 +++++++ windows/console.c | 27 ++++++++++++++++++--------- windows/plink.c | 1 + windows/window.c | 1 + 16 files changed, 83 insertions(+), 21 deletions(-) diff --git a/proxy/sshproxy.c b/proxy/sshproxy.c index a26ccb04..49d1ea98 100644 --- a/proxy/sshproxy.c +++ b/proxy/sshproxy.c @@ -502,6 +502,7 @@ static const SeatVtable SshProxy_seat_vt = { .stripctrl_new = sshproxy_stripctrl_new, .set_trust_status = sshproxy_set_trust_status, .can_set_trust_status = sshproxy_can_set_trust_status, + .has_mixed_input_stream = nullseat_has_mixed_input_stream_no, .verbose = sshproxy_verbose, .interactive = sshproxy_interactive, .get_cursor_position = nullseat_get_cursor_position, diff --git a/pscp.c b/pscp.c index 13e89812..837e89d9 100644 --- a/pscp.c +++ b/pscp.c @@ -87,6 +87,7 @@ static const SeatVtable pscp_seat_vt = { .stripctrl_new = console_stripctrl_new, .set_trust_status = nullseat_set_trust_status, .can_set_trust_status = nullseat_can_set_trust_status_yes, + .has_mixed_input_stream = nullseat_has_mixed_input_stream_no, .verbose = cmdline_seat_verbose, .interactive = nullseat_interactive_no, .get_cursor_position = nullseat_get_cursor_position, diff --git a/psftp.c b/psftp.c index 14483569..d0317c2e 100644 --- a/psftp.c +++ b/psftp.c @@ -68,6 +68,7 @@ static const SeatVtable psftp_seat_vt = { .stripctrl_new = console_stripctrl_new, .set_trust_status = nullseat_set_trust_status, .can_set_trust_status = nullseat_can_set_trust_status_yes, + .has_mixed_input_stream = nullseat_has_mixed_input_stream_no, .verbose = cmdline_seat_verbose, .interactive = nullseat_interactive_yes, .get_cursor_position = nullseat_get_cursor_position, diff --git a/putty.h b/putty.h index 0047156f..232bbe30 100644 --- a/putty.h +++ b/putty.h @@ -1289,6 +1289,18 @@ struct SeatVtable { */ bool (*can_set_trust_status)(Seat *seat); + /* + * Query whether this Seat's interactive prompt responses and its + * session input come from the same place. + * + * If false, this is used to suppress the final 'Press Return to + * begin session' anti-spoofing prompt in Plink. For example, + * Plink itself sets this flag if its standard input is redirected + * (and therefore not coming from the same place as the console + * it's sending its prompts to). + */ + bool (*has_mixed_input_stream)(Seat *seat); + /* * Ask the seat whether it would like verbose messages. */ @@ -1365,6 +1377,8 @@ static inline void seat_set_trust_status(Seat *seat, bool trusted) { seat->vt->set_trust_status(seat, trusted); } static inline bool seat_can_set_trust_status(Seat *seat) { return seat->vt->can_set_trust_status(seat); } +static inline bool seat_has_mixed_input_stream(Seat *seat) +{ return seat->vt->has_mixed_input_stream(seat); } static inline bool seat_verbose(Seat *seat) { return seat->vt->verbose(seat); } static inline bool seat_interactive(Seat *seat) @@ -1437,6 +1451,8 @@ StripCtrlChars *nullseat_stripctrl_new( void nullseat_set_trust_status(Seat *seat, bool trusted); bool nullseat_can_set_trust_status_yes(Seat *seat); bool nullseat_can_set_trust_status_no(Seat *seat); +bool nullseat_has_mixed_input_stream_yes(Seat *seat); +bool nullseat_has_mixed_input_stream_no(Seat *seat); bool nullseat_verbose_no(Seat *seat); bool nullseat_verbose_yes(Seat *seat); bool nullseat_interactive_no(Seat *seat); @@ -1463,6 +1479,7 @@ StripCtrlChars *console_stripctrl_new( Seat *seat, BinarySink *bs_out, SeatInteractionContext sic); void console_set_trust_status(Seat *seat, bool trusted); bool console_can_set_trust_status(Seat *seat); +bool console_has_mixed_input_stream(Seat *seat); /* * Other centralised seat functions. diff --git a/ssh/connection1-client.c b/ssh/connection1-client.c index 7429b892..41bf9716 100644 --- a/ssh/connection1-client.c +++ b/ssh/connection1-client.c @@ -544,5 +544,9 @@ SshChannel *ssh1_serverside_agent_open(ConnectionLayer *cl, Channel *chan) bool ssh1_connection_need_antispoof_prompt(struct ssh1_connection_state *s) { seat_set_trust_status(s->ppl.seat, false); - return !seat_can_set_trust_status(s->ppl.seat); + if (!seat_has_mixed_input_stream(s->ppl.seat)) + return false; + if (seat_can_set_trust_status(s->ppl.seat)) + return false; + return true; } diff --git a/ssh/connection2-client.c b/ssh/connection2-client.c index 88338500..f17d1e21 100644 --- a/ssh/connection2-client.c +++ b/ssh/connection2-client.c @@ -501,6 +501,11 @@ void ssh2channel_send_terminal_size_change(SshChannel *sc, int w, int h) bool ssh2_connection_need_antispoof_prompt(struct ssh2_connection_state *s) { seat_set_trust_status(s->ppl.seat, false); - bool success = seat_can_set_trust_status(s->ppl.seat); - return (!success && !ssh_is_bare(s->ppl.ssh)); + if (!seat_has_mixed_input_stream(s->ppl.seat)) + return false; + if (seat_can_set_trust_status(s->ppl.seat)) + return false; + if (ssh_is_bare(s->ppl.ssh)) + return false; + return true; } diff --git a/ssh/server.c b/ssh/server.c index 99237616..347aa14b 100644 --- a/ssh/server.c +++ b/ssh/server.c @@ -128,6 +128,7 @@ static const SeatVtable server_seat_vt = { .stripctrl_new = nullseat_stripctrl_new, .set_trust_status = nullseat_set_trust_status, .can_set_trust_status = nullseat_can_set_trust_status_no, + .has_mixed_input_stream = nullseat_has_mixed_input_stream_no, .verbose = nullseat_verbose_no, .interactive = nullseat_interactive_no, .get_cursor_position = nullseat_get_cursor_position, diff --git a/ssh/sesschan.c b/ssh/sesschan.c index 5379b308..15b8bb4e 100644 --- a/ssh/sesschan.c +++ b/ssh/sesschan.c @@ -207,6 +207,7 @@ static const SeatVtable sesschan_seat_vt = { .stripctrl_new = nullseat_stripctrl_new, .set_trust_status = nullseat_set_trust_status, .can_set_trust_status = nullseat_can_set_trust_status_no, + .has_mixed_input_stream = nullseat_has_mixed_input_stream_no, .verbose = nullseat_verbose_no, .interactive = nullseat_interactive_no, .get_cursor_position = nullseat_get_cursor_position, diff --git a/unix/console.c b/unix/console.c index da795614..4131abdc 100644 --- a/unix/console.c +++ b/unix/console.c @@ -328,19 +328,11 @@ void console_set_trust_status(Seat *seat, bool trusted) bool console_can_set_trust_status(Seat *seat) { - if (console_batch_mode || !is_interactive() || !console_antispoof_prompt) { + if (console_batch_mode) { /* * In batch mode, we don't need to worry about the server * mimicking our interactive authentication, because the user * already knows not to expect any. - * - * If standard input isn't connected to a terminal, likewise, - * because even if the server did send a spoof authentication - * prompt, the user couldn't respond to it via the terminal - * anyway. - * - * We also return true without enabling any defences if the - * user has purposely disabled the antispoof prompt. */ return true; } @@ -348,6 +340,23 @@ bool console_can_set_trust_status(Seat *seat) return false; } +bool console_has_mixed_input_stream(Seat *seat) +{ + if (!is_interactive() || !console_antispoof_prompt) { + /* + * If standard input isn't connected to a terminal, then even + * if the server did send a spoof authentication prompt, the + * user couldn't respond to it via the terminal anyway. + * + * We also pretend this is true if the user has purposely + * disabled the antispoof prompt. + */ + return false; + } + + return true; +} + /* * Warn about the obsolescent key file format. * diff --git a/unix/plink.c b/unix/plink.c index dee0cbec..446bef21 100644 --- a/unix/plink.c +++ b/unix/plink.c @@ -411,6 +411,7 @@ static const SeatVtable plink_seat_vt = { .stripctrl_new = console_stripctrl_new, .set_trust_status = console_set_trust_status, .can_set_trust_status = console_can_set_trust_status, + .has_mixed_input_stream = console_has_mixed_input_stream, .verbose = cmdline_seat_verbose, .interactive = plink_seat_interactive, .get_cursor_position = nullseat_get_cursor_position, diff --git a/unix/window.c b/unix/window.c index c5894ffa..c9d635ab 100644 --- a/unix/window.c +++ b/unix/window.c @@ -414,6 +414,7 @@ static const SeatVtable gtk_seat_vt = { .stripctrl_new = gtk_seat_stripctrl_new, .set_trust_status = gtk_seat_set_trust_status, .can_set_trust_status = gtk_seat_can_set_trust_status, + .has_mixed_input_stream = nullseat_has_mixed_input_stream_yes, .verbose = nullseat_verbose_yes, .interactive = nullseat_interactive_yes, .get_cursor_position = gtk_seat_get_cursor_position, diff --git a/utils/nullseat.c b/utils/nullseat.c index a53f563e..43a338fb 100644 --- a/utils/nullseat.c +++ b/utils/nullseat.c @@ -41,6 +41,8 @@ StripCtrlChars *nullseat_stripctrl_new( void nullseat_set_trust_status(Seat *seat, bool trusted) {} bool nullseat_can_set_trust_status_yes(Seat *seat) { return true; } bool nullseat_can_set_trust_status_no(Seat *seat) { return false; } +bool nullseat_has_mixed_input_stream_yes(Seat *seat) { return true; } +bool nullseat_has_mixed_input_stream_no(Seat *seat) { return false; } bool nullseat_verbose_no(Seat *seat) { return false; } bool nullseat_verbose_yes(Seat *seat) { return true; } bool nullseat_interactive_no(Seat *seat) { return false; } diff --git a/utils/tempseat.c b/utils/tempseat.c index eac10f8d..e1956eaa 100644 --- a/utils/tempseat.c +++ b/utils/tempseat.c @@ -198,6 +198,12 @@ static bool tempseat_can_set_trust_status(Seat *seat) return seat_can_set_trust_status(ts->realseat); } +static bool tempseat_has_mixed_input_stream(Seat *seat) +{ + TempSeat *ts = container_of(seat, TempSeat, seat); + return seat_has_mixed_input_stream(ts->realseat); +} + /* ---------------------------------------------------------------------- * Methods that should never be called on a TempSeat, so we can put an * unreachable() in them. @@ -324,6 +330,7 @@ static const struct SeatVtable tempseat_vt = { .stripctrl_new = tempseat_stripctrl_new, .set_trust_status = tempseat_set_trust_status, .can_set_trust_status = tempseat_can_set_trust_status, + .has_mixed_input_stream = tempseat_has_mixed_input_stream, .verbose = tempseat_verbose, .interactive = tempseat_interactive, .get_cursor_position = tempseat_get_cursor_position, diff --git a/windows/console.c b/windows/console.c index abd31270..9fa9d7f0 100644 --- a/windows/console.c +++ b/windows/console.c @@ -194,19 +194,11 @@ void console_set_trust_status(Seat *seat, bool trusted) bool console_can_set_trust_status(Seat *seat) { - if (console_batch_mode || !is_interactive() || !console_antispoof_prompt) { + if (console_batch_mode) { /* * In batch mode, we don't need to worry about the server * mimicking our interactive authentication, because the user * already knows not to expect any. - * - * If standard input isn't connected to a terminal, likewise, - * because even if the server did send a spoof authentication - * prompt, the user couldn't respond to it via the terminal - * anyway. - * - * We also return true without enabling any defences if the - * user has purposely disabled the antispoof prompt. */ return true; } @@ -214,6 +206,23 @@ bool console_can_set_trust_status(Seat *seat) return false; } +bool console_has_mixed_input_stream(Seat *seat) +{ + if (!is_interactive() || !console_antispoof_prompt) { + /* + * If standard input isn't connected to a terminal, then even + * if the server did send a spoof authentication prompt, the + * user couldn't respond to it via the terminal anyway. + * + * We also pretend this is true if the user has purposely + * disabled the antispoof prompt. + */ + return false; + } + + return true; +} + /* * Ask whether to wipe a session log file before writing to it. * Returns 2 for wipe, 1 for append, 0 for cancel (don't log). diff --git a/windows/plink.c b/windows/plink.c index ef982be1..8dc44c9e 100644 --- a/windows/plink.c +++ b/windows/plink.c @@ -105,6 +105,7 @@ static const SeatVtable plink_seat_vt = { .stripctrl_new = console_stripctrl_new, .set_trust_status = console_set_trust_status, .can_set_trust_status = console_can_set_trust_status, + .has_mixed_input_stream = console_has_mixed_input_stream, .verbose = cmdline_seat_verbose, .interactive = plink_seat_interactive, .get_cursor_position = nullseat_get_cursor_position, diff --git a/windows/window.c b/windows/window.c index 1aa83743..8ab371bb 100644 --- a/windows/window.c +++ b/windows/window.c @@ -352,6 +352,7 @@ static const SeatVtable win_seat_vt = { .stripctrl_new = win_seat_stripctrl_new, .set_trust_status = win_seat_set_trust_status, .can_set_trust_status = win_seat_can_set_trust_status, + .has_mixed_input_stream = nullseat_has_mixed_input_stream_yes, .verbose = nullseat_verbose_yes, .interactive = nullseat_interactive_yes, .get_cursor_position = win_seat_get_cursor_position,