From 329bdb344c268f91ed726e000028cf52806ef5cb Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 8 May 2021 18:13:06 +0100 Subject: [PATCH 01/16] Make TermWin's palette_get_overrides() take a Terminal *. Less than 12 hours after 0.75 went out of the door, a user pointed out that enabling the 'Use system colours' config option causes an immediate NULL-dereference crash. The reason is because a chain of calls from term_init() ends up calling back to the Windows implementation of the palette_get_overrides() method, which responds by trying to call functions on the static variable 'term' in window.c, which won't be initialised until term_init() has returned. Simple fix: palette_get_overrides() is now given a pointer to the Terminal that it should be updating, because it can't find it out any other way. (cherry picked from commit 571fa3388d61f3270d958503c62f09669665f6f8) --- fuzzterm.c | 2 +- putty.h | 14 ++++++++++---- terminal.c | 2 +- unix/gtkwin.c | 2 +- windows/window.c | 4 ++-- 5 files changed, 15 insertions(+), 9 deletions(-) diff --git a/fuzzterm.c b/fuzzterm.c index c5c29ef4..f53c0a9c 100644 --- a/fuzzterm.c +++ b/fuzzterm.c @@ -93,7 +93,7 @@ static void fuzz_move(TermWin *tw, int x, int y) {} static void fuzz_set_zorder(TermWin *tw, bool top) {} static void fuzz_palette_set(TermWin *tw, unsigned start, unsigned ncolours, const rgb *colours) {} -static void fuzz_palette_get_overrides(TermWin *tw) {} +static void fuzz_palette_get_overrides(TermWin *tw, Terminal *term) {} static const TermWinVtable fuzz_termwin_vt = { .setup_draw_ctx = fuzz_setup_draw_ctx, diff --git a/putty.h b/putty.h index a73817af..6f1f914c 100644 --- a/putty.h +++ b/putty.h @@ -1299,8 +1299,14 @@ struct TermWinVtable { /* Query the front end for any OS-local overrides to the default * colours stored in Conf. The front end should set any it cares - * about by calling term_palette_override. */ - void (*palette_get_overrides)(TermWin *); + * about by calling term_palette_override. + * + * The Terminal object is passed in as a parameter, because this + * can be called as a callback from term_init(). So the TermWin + * itself won't yet have been told where to find its Terminal + * object, because that doesn't happen until term_init + * returns. */ + void (*palette_get_overrides)(TermWin *, Terminal *); }; static inline bool win_setup_draw_ctx(TermWin *win) @@ -1354,8 +1360,8 @@ static inline void win_set_zorder(TermWin *win, bool top) static inline void win_palette_set( TermWin *win, unsigned start, unsigned ncolours, const rgb *colours) { win->vt->palette_set(win, start, ncolours, colours); } -static inline void win_palette_get_overrides(TermWin *win) -{ win->vt->palette_get_overrides(win); } +static inline void win_palette_get_overrides(TermWin *win, Terminal *term) +{ win->vt->palette_get_overrides(win, term); } /* * Global functions not specific to a connection instance. diff --git a/terminal.c b/terminal.c index 0673623a..4bb47352 100644 --- a/terminal.c +++ b/terminal.c @@ -1874,7 +1874,7 @@ static void palette_reset(Terminal *term, bool overrides_only) */ for (unsigned i = 0; i < OSC4_NCOLOURS; i++) term->subpalettes[SUBPAL_PLATFORM].present[i] = false; - win_palette_get_overrides(term->win); + win_palette_get_overrides(term->win, term); /* * Rebuild the composite palette. diff --git a/unix/gtkwin.c b/unix/gtkwin.c index 2a2353d4..61e77187 100644 --- a/unix/gtkwin.c +++ b/unix/gtkwin.c @@ -2614,7 +2614,7 @@ static void gtkwin_palette_set(TermWin *tw, unsigned start, unsigned ncolours, } } -static void gtkwin_palette_get_overrides(TermWin *tw) +static void gtkwin_palette_get_overrides(TermWin *tw, Terminal *term) { /* GTK has no analogue of Windows's 'standard system colours', so GTK PuTTY * has no config option to override the normally configured colours from diff --git a/windows/window.c b/windows/window.c index 091838b4..ccf7b259 100644 --- a/windows/window.c +++ b/windows/window.c @@ -263,7 +263,7 @@ static void wintw_set_maximised(TermWin *, bool maximised); static void wintw_move(TermWin *, int x, int y); static void wintw_set_zorder(TermWin *, bool top); static void wintw_palette_set(TermWin *, unsigned, unsigned, const rgb *); -static void wintw_palette_get_overrides(TermWin *); +static void wintw_palette_get_overrides(TermWin *, Terminal *); static const TermWinVtable windows_termwin_vt = { .setup_draw_ctx = wintw_setup_draw_ctx, @@ -1214,7 +1214,7 @@ static inline rgb rgb_from_colorref(COLORREF cr) return toret; } -static void wintw_palette_get_overrides(TermWin *tw) +static void wintw_palette_get_overrides(TermWin *tw, Terminal *term) { if (conf_get_bool(conf, CONF_system_colour)) { rgb rgb; From bf67ce44d0403a087246b111d8a5c1cb4842e3fe Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 8 May 2021 20:57:18 +0100 Subject: [PATCH 02/16] Reinstate missing bit counts in Windows Pageant GUI. An embarrassing braino of && for || produced a boolean expression that could never evaluate true. (cherry picked from commit 8245510a029b3d5b6774fe931dc2725b0a687540) --- windows/winpgnt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/windows/winpgnt.c b/windows/winpgnt.c index 6d6d1b9b..c0ffcf63 100644 --- a/windows/winpgnt.c +++ b/windows/winpgnt.c @@ -378,7 +378,7 @@ static void keylist_update_callback( ptrlen algname = get_string(src); const ssh_keyalg *alg = find_pubkey_alg_len(algname); - bool include_bit_count = (alg == &ssh_dss && alg == &ssh_rsa); + bool include_bit_count = (alg == &ssh_dss || alg == &ssh_rsa); int wordnumber = 0; for (const char *p = fingerprint; *p; p++) { From 6976bf60827e447f31d19e02d038ff7748dffb6d Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Thu, 13 May 2021 18:20:41 +0100 Subject: [PATCH 03/16] Fix confusion between invalid Windows HANDLE values. I was checking a HANDLE against INVALID_HANDLE_VALUE to decide whether it should be closed. But ten lines further up, I was setting it manually to NULL to suppress the close. Oops. (cherry picked from commit 155d8121e6584f842fa06f5fbad75b2555f60269) --- windows/winpgntc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/windows/winpgntc.c b/windows/winpgntc.c index dd4de0c7..f0f45dd6 100644 --- a/windows/winpgntc.c +++ b/windows/winpgntc.c @@ -266,7 +266,7 @@ static agent_pending_query *named_pipe_agent_query( pq = snew(agent_pending_query); pq->handle = handle_input_new(pipehandle, named_pipe_agent_gotdata, pq, 0); - pipehandle = NULL; /* prevent it being closed below */ + pipehandle = INVALID_HANDLE_VALUE; /* prevent it being closed below */ pq->response = strbuf_new_nm(); pq->callback = callback; pq->callback_ctx = callback_ctx; From 11b89407f5d225e461a266a9c7f23299e5314abf Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Thu, 13 May 2021 18:22:05 +0100 Subject: [PATCH 04/16] Close agent named-pipe handles when queries complete. I was cleaning up the 'struct handle', but not the underlying HANDLE. As a result, any PuTTY process that makes a request to Pageant keeps the named pipe connection open until the end of the process's lifetime. (cherry picked from commit 6e69223dc262755c2cd2a3bba5c188d8fc91943a) --- windows/winpgntc.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/windows/winpgntc.c b/windows/winpgntc.c index f0f45dd6..557dc532 100644 --- a/windows/winpgntc.c +++ b/windows/winpgntc.c @@ -169,6 +169,7 @@ bool agent_exists(void) struct agent_pending_query { struct handle *handle; + HANDLE os_handle; strbuf *response; void (*callback)(void *, void *, int); void *callback_ctx; @@ -266,6 +267,7 @@ static agent_pending_query *named_pipe_agent_query( pq = snew(agent_pending_query); pq->handle = handle_input_new(pipehandle, named_pipe_agent_gotdata, pq, 0); + pq->os_handle = pipehandle; pipehandle = INVALID_HANDLE_VALUE; /* prevent it being closed below */ pq->response = strbuf_new_nm(); pq->callback = callback; @@ -290,6 +292,7 @@ static agent_pending_query *named_pipe_agent_query( void agent_cancel_query(agent_pending_query *pq) { handle_free(pq->handle); + CloseHandle(pq->os_handle); if (pq->response) strbuf_free(pq->response); sfree(pq); From 27a04d96a34029387ed61e73c80ce9210dd51d30 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Wed, 19 May 2021 10:42:42 +0100 Subject: [PATCH 05/16] cmdgen: add missing null pointer check in --dump mode. A user pointed out that once we've identified the key algorithm from an apparent public-key blob, we call ssh_key_new_pub on the blob data and assume it will succeed. But there are plenty of ways it could still fail, and ssh_key_new_pub could return NULL. (cherry picked from commit 0c21eb444794512ba1106d4a3bc3380861566099) --- cmdgen.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cmdgen.c b/cmdgen.c index 920317f5..ef9c2e1c 100644 --- a/cmdgen.c +++ b/cmdgen.c @@ -1283,6 +1283,10 @@ int main(int argc, char **argv) } ssh_key *sk = ssh_key_new_pub( alg, ptrlen_from_strbuf(ssh2blob)); + if (!sk) { + fprintf(stderr, "puttygen: unable to decode public key\n"); + RETURN(1); + } kc = ssh_key_components(sk); ssh_key_free(sk); } From ff53c6716afb31a4ac3732238aa793d3d814530a Mon Sep 17 00:00:00 2001 From: Jacob Nevins Date: Sat, 15 May 2021 22:05:27 +0100 Subject: [PATCH 06/16] Fix changing colours in Change Settings. Since ca9cd983e1, changing colour config mid-session had no effect (until the palette was reset for some other reason). Now it does take effect immediately (provided that the palette has not been overridden by escape sequence -- this is new with ca9cd983e1). This changes the semantics of palette_reset(): the only important parameter when doing that is whether we keep escape sequence overrides -- there's no harm in re-fetching config and platform colours whether or not they've changed -- so that's what the parameter becomes (with a sense that doesn't require changing the call sites). The other part of this change is actually remembering to trigger this when the configuration is changed. (cherry picked from commit 1e726c94e8d53c929a568a74fd54f71148550696) --- putty.h | 2 +- terminal.c | 103 ++++++++++++++++++++++++++++++----------------- windows/window.c | 4 +- 3 files changed, 70 insertions(+), 39 deletions(-) diff --git a/putty.h b/putty.h index 6f1f914c..ef6f7a37 100644 --- a/putty.h +++ b/putty.h @@ -1790,7 +1790,7 @@ void term_keyinputw(Terminal *, const wchar_t * widebuf, int len); void term_get_cursor_position(Terminal *term, int *x, int *y); void term_setup_window_titles(Terminal *term, const char *title_hostname); void term_notify_minimised(Terminal *term, bool minimised); -void term_notify_palette_overrides_changed(Terminal *term); +void term_notify_palette_changed(Terminal *term); void term_notify_window_pos(Terminal *term, int x, int y); void term_notify_window_size_pixels(Terminal *term, int x, int y); void term_palette_override(Terminal *term, unsigned osc4_index, rgb rgb); diff --git a/terminal.c b/terminal.c index 4bb47352..b845baab 100644 --- a/terminal.c +++ b/terminal.c @@ -1629,6 +1629,7 @@ void term_reconfig(Terminal *term, Conf *conf) * Mode, BCE, blinking text, character classes. */ bool reset_wrap, reset_decom, reset_bce, reset_tblink, reset_charclass; + bool palette_changed = false; int i; reset_wrap = (conf_get_bool(term->conf, CONF_wrap_mode) != @@ -1674,6 +1675,29 @@ void term_reconfig(Terminal *term, Conf *conf) } } + /* + * Just setting conf is sufficient to cause colour setting changes + * to appear on the next ESC]R palette reset. But we should also + * check whether any colour settings have been changed, so that + * they can be updated immediately if they haven't been overridden + * by some escape sequence. + */ + { + int i, j; + for (i = 0; i < CONF_NCOLOURS; i++) { + for (j = 0; j < 3; j++) + if (conf_get_int_int(term->conf, CONF_colours, i*3+j) != + conf_get_int_int(conf, CONF_colours, i*3+j)) + break; + if (j < 3) { + /* Actually enacting the change has to be deferred + * until the new conf is installed. */ + palette_changed = true; + break; + } + } + } + conf_free(term->conf); term->conf = conf_copy(conf); @@ -1702,6 +1726,8 @@ void term_reconfig(Terminal *term, Conf *conf) if (!conf_get_str(term->conf, CONF_printer)) { term_print_finish(term); } + if (palette_changed) + term_notify_palette_changed(term); term_schedule_tblink(term); term_schedule_cblink(term); term_copy_stuff_from_conf(term); @@ -1829,44 +1855,41 @@ static void palette_rebuild(Terminal *term) } } -static void palette_reset(Terminal *term, bool overrides_only) +/* + * Rebuild the palette from configuration and platform colours. + * If 'keep_overrides' set, any escape-sequence-specified overrides will + * remain in place. + */ +static void palette_reset(Terminal *term, bool keep_overrides) { - if (!overrides_only) { - for (unsigned i = 0; i < OSC4_NCOLOURS; i++) - term->subpalettes[SUBPAL_CONF].present[i] = true; + for (unsigned i = 0; i < OSC4_NCOLOURS; i++) + term->subpalettes[SUBPAL_CONF].present[i] = true; - /* - * Copy all the palette information out of the Conf. - */ - for (unsigned i = 0; i < CONF_NCOLOURS; i++) { - rgb *col = &term->subpalettes[SUBPAL_CONF].values[ - colour_indices_conf_to_osc4[i]]; - col->r = conf_get_int_int(term->conf, CONF_colours, i*3+0); - col->g = conf_get_int_int(term->conf, CONF_colours, i*3+1); - col->b = conf_get_int_int(term->conf, CONF_colours, i*3+2); - } + /* + * Copy all the palette information out of the Conf. + */ + for (unsigned i = 0; i < CONF_NCOLOURS; i++) { + rgb *col = &term->subpalettes[SUBPAL_CONF].values[ + colour_indices_conf_to_osc4[i]]; + col->r = conf_get_int_int(term->conf, CONF_colours, i*3+0); + col->g = conf_get_int_int(term->conf, CONF_colours, i*3+1); + col->b = conf_get_int_int(term->conf, CONF_colours, i*3+2); + } - /* - * Directly invent the rest of the xterm-256 colours. - */ - for (unsigned i = 0; i < 216; i++) { - rgb *col = &term->subpalettes[SUBPAL_CONF].values[i + 16]; - int r = i / 36, g = (i / 6) % 6, b = i % 6; - col->r = r ? r * 40 + 55 : 0; - col->g = g ? g * 40 + 55 : 0; - col->b = b ? b * 40 + 55 : 0; - } - for (unsigned i = 0; i < 24; i++) { - rgb *col = &term->subpalettes[SUBPAL_CONF].values[i + 232]; - int shade = i * 10 + 8; - col->r = col->g = col->b = shade; - } - - /* - * Get rid of all escape-sequence configuration. - */ - for (unsigned i = 0; i < OSC4_NCOLOURS; i++) - term->subpalettes[SUBPAL_SESSION].present[i] = false; + /* + * Directly invent the rest of the xterm-256 colours. + */ + for (unsigned i = 0; i < 216; i++) { + rgb *col = &term->subpalettes[SUBPAL_CONF].values[i + 16]; + int r = i / 36, g = (i / 6) % 6, b = i % 6; + col->r = r ? r * 40 + 55 : 0; + col->g = g ? g * 40 + 55 : 0; + col->b = b ? b * 40 + 55 : 0; + } + for (unsigned i = 0; i < 24; i++) { + rgb *col = &term->subpalettes[SUBPAL_CONF].values[i + 232]; + int shade = i * 10 + 8; + col->r = col->g = col->b = shade; } /* @@ -1876,6 +1899,14 @@ static void palette_reset(Terminal *term, bool overrides_only) term->subpalettes[SUBPAL_PLATFORM].present[i] = false; win_palette_get_overrides(term->win, term); + if (!keep_overrides) { + /* + * Get rid of all escape-sequence configuration. + */ + for (unsigned i = 0; i < OSC4_NCOLOURS; i++) + term->subpalettes[SUBPAL_SESSION].present[i] = false; + } + /* * Rebuild the composite palette. */ @@ -7593,7 +7624,7 @@ void term_notify_minimised(Terminal *term, bool minimised) term->minimised = minimised; } -void term_notify_palette_overrides_changed(Terminal *term) +void term_notify_palette_changed(Terminal *term) { palette_reset(term, true); } diff --git a/windows/window.c b/windows/window.c index ccf7b259..57abad83 100644 --- a/windows/window.c +++ b/windows/window.c @@ -2394,7 +2394,7 @@ static LRESULT CALLBACK WndProc(HWND hwnd, UINT message, if (conf_get_bool(conf, CONF_system_colour) != conf_get_bool(prev_conf, CONF_system_colour)) - term_notify_palette_overrides_changed(term); + term_notify_palette_changed(term); /* Pass new config data to the terminal */ term_reconfig(term, conf); @@ -3384,7 +3384,7 @@ static LRESULT CALLBACK WndProc(HWND hwnd, UINT message, case WM_SYSCOLORCHANGE: if (conf_get_bool(conf, CONF_system_colour)) { /* Refresh palette from system colours. */ - term_notify_palette_overrides_changed(term); + term_notify_palette_changed(term); init_palette(); /* Force a repaint of the terminal window. */ term_invalidate(term); From fd3f05d215c27703321020f5b906a63c53a3119a Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 23 May 2021 08:59:13 +0100 Subject: [PATCH 07/16] Fix tight loop on reading truncated key files. In commit 9cc586e605e3db1 I changed the low-level key-file reading routines like read_header and read_body so that they read from a BinarySource via get_byte(), rather than from a FILE * via fgetc. But I forgot that the two functions don't signal end-of-file the same way, so testing the return value of get_byte() against EOF is pointless and will never match, and conversely, real EOF won't be spotted unless you also examine the error indicator in the BinarySource. As a result, a key file that ends without a trailing newline will cause a tight loop in one of those low-level read routines. (cherry picked from commit d008d235f3841139daca39efee25cd5423ce31b8) --- sshpubk.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sshpubk.c b/sshpubk.c index 0aa5e0b6..3fad8870 100644 --- a/sshpubk.c +++ b/sshpubk.c @@ -480,7 +480,7 @@ static bool read_header(BinarySource *src, char *header) while (1) { c = get_byte(src); - if (c == '\n' || c == '\r' || c == EOF) + if (c == '\n' || c == '\r' || get_err(src)) return false; /* failure */ if (c == ':') { c = get_byte(src); @@ -503,10 +503,10 @@ static char *read_body(BinarySource *src) while (1) { int c = get_byte(src); - if (c == '\r' || c == '\n' || c == EOF) { - if (c != EOF) { + if (c == '\r' || c == '\n' || get_err(src)) { + if (!get_err(src)) { c = get_byte(src); - if (c != '\r' && c != '\n') + if (c != '\r' && c != '\n' && !get_err(src)) src->pos--; } return strbuf_to_str(buf); From 8f3a0ea69fd88e0fc7a0dd8ec1d4f535592b8eb2 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 12 Jun 2021 14:03:27 +0100 Subject: [PATCH 08/16] Fix Arm crypto build failure on clang post-12. I had manually defined the ACLE feature macro __ARM_FEATURE_CRYPTO before including arm_neon.h, in the expectation that it would turn on the AES, SHA-1 and SHA-256 intrinsics. But up-to-date clang has now separated those intrinsics from each other, and guarded them by two more specific feature macros, one for AES and one for the two SHAs. So just defining __ARM_FEATURE_CRYPTO isn't good enough any more, and my attempts to use crypto intrinsics in the following functions provoke a compile error. The fix is to define the appropriate new feature macro by hand (leaving the old definition in place for earlier clang versions). This fix is only needed on the release branch, of course: on main, we've already done the reorganisation that avoids the need to manually define ACLE feature macros at all, because the accelerated crypto code is compiled in separate objects using command-line compile flags in the way that the toolchain normally expects. --- sshaes.c | 1 + sshsh256.c | 1 + sshsha.c | 1 + 3 files changed, 3 insertions(+) diff --git a/sshaes.c b/sshaes.c index 97ea9f31..6671879e 100644 --- a/sshaes.c +++ b/sshaes.c @@ -1580,6 +1580,7 @@ NI_ENC_DEC(256) */ #define __ARM_NEON 1 #define __ARM_FEATURE_CRYPTO 1 +#define __ARM_FEATURE_AES 1 #define FUNC_ISA __attribute__ ((target("neon,crypto"))) #endif /* USE_CLANG_ATTR_TARGET_AARCH64 */ diff --git a/sshsh256.c b/sshsh256.c index 206a976c..db1f96bd 100644 --- a/sshsh256.c +++ b/sshsh256.c @@ -723,6 +723,7 @@ const ssh_hashalg ssh_sha256_hw = { */ #define __ARM_NEON 1 #define __ARM_FEATURE_CRYPTO 1 +#define __ARM_FEATURE_SHA2 1 #define FUNC_ISA __attribute__ ((target("neon,crypto"))) #endif /* USE_CLANG_ATTR_TARGET_AARCH64 */ diff --git a/sshsha.c b/sshsha.c index 536d474f..a5e79e6a 100644 --- a/sshsha.c +++ b/sshsha.c @@ -690,6 +690,7 @@ const ssh_hashalg ssh_sha1_hw = { */ #define __ARM_NEON 1 #define __ARM_FEATURE_CRYPTO 1 +#define __ARM_FEATURE_SHA2 1 #define FUNC_ISA __attribute__ ((target("neon,crypto"))) #endif /* USE_CLANG_ATTR_TARGET_AARCH64 */ From 746d87fc6f3c98605877415a69dd0c4692b754c6 Mon Sep 17 00:00:00 2001 From: Jacob Nevins Date: Sun, 13 Jun 2021 00:18:42 +0100 Subject: [PATCH 09/16] Fix palette escape sequences sometimes not working. If a batch of palette changes were seen in between window updates, only the last one would take immediate effect. (cherry-picked from commit 5677da64813d38aa1feeb44bca5d098b54f53240) --- terminal.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/terminal.c b/terminal.c index b845baab..f8f9417c 100644 --- a/terminal.c +++ b/terminal.c @@ -1815,6 +1815,16 @@ static void palette_rebuild(Terminal *term) { unsigned min_changed = OSC4_NCOLOURS, max_changed = 0; + if (term->win_palette_pending) { + /* Possibly extend existing range. */ + min_changed = term->win_palette_pending_min; + max_changed = term->win_palette_pending_limit - 1; + } else { + /* Start with empty range. */ + min_changed = OSC4_NCOLOURS; + max_changed = 0; + } + for (unsigned i = 0; i < OSC4_NCOLOURS; i++) { rgb new_value; bool found = false; @@ -1842,10 +1852,14 @@ static void palette_rebuild(Terminal *term) if (min_changed <= max_changed) { /* - * At least one colour changed, so schedule a redraw event to - * pass the result back to the TermWin. This also requires - * invalidating the rest of the window, because usually all - * the text will need redrawing in the new colours. + * At least one colour changed (or we had an update scheduled + * already). Schedule a redraw event to pass the result back + * to the TermWin. This also requires invalidating the rest + * of the window, because usually all the text will need + * redrawing in the new colours. + * (If there was an update pending and this palette rebuild + * didn't actually change anything, we'll harmlessly reinforce + * the existing update request.) */ term->win_palette_pending = true; term->win_palette_pending_min = min_changed; From 1dc5659aa62848f0aeb5de7bd3839fecc7debefa Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 19 Jun 2021 15:39:15 +0100 Subject: [PATCH 10/16] New option to reject 'trivial' success of userauth. Suggested by Manfred Kaiser, who also wrote most of this patch (although outlying parts, like documentation and SSH-1 support, are by me). This is a second line of defence against the kind of spoofing attacks in which a malicious or compromised SSH server rushes the client through the userauth phase of SSH without actually requiring any auth inputs (passwords or signatures or whatever), and then at the start of the connection phase it presents something like a spoof prompt, intended to be taken for part of userauth by the user but in fact with some more sinister purpose. Our existing line of defence against this is the trust sigil system, and as far as I know, that's still working. This option allows a bit of extra defence in depth: if you don't expect your SSH server to trivially accept authentication in the first place, then enabling this option will cause PuTTY to disconnect if it unexpectedly does so, without the user having to spot the presence or absence of a fiddly little sigil anywhere. Several types of authentication count as 'trivial'. The obvious one is the SSH-2 "none" method, which clients always try first so that the failure message will tell them what else they can try, and which a server can instead accept in order to authenticate you unconditionally. But there are two other ways to do it that we know of: one is to run keyboard-interactive authentication and send an empty INFO_REQUEST packet containing no actual prompts for the user, and another even weirder one is to send USERAUTH_SUCCESS in response to the user's preliminary *offer* of a public key (instead of sending the usual PK_OK to request an actual signature from the key). This new option detects all of those, by clearing the 'is_trivial_auth' flag only when we send some kind of substantive authentication response (be it a password, a k-i prompt response, a signature, or a GSSAPI token). So even if there's a further path through the userauth maze we haven't spotted, that somehow avoids sending anything substantive, this strategy should still pick it up. (cherry picked from commit 5f5c710cf3704737e24ffceb2c918e412a2a674f) --- cmdline.c | 8 ++++++++ config.c | 4 ++++ doc/config.but | 43 +++++++++++++++++++++++++++++++++++++++++++ pscp.c | 2 ++ psftp.c | 2 ++ putty.h | 1 + settings.c | 2 ++ ssh.c | 4 +++- ssh1login.c | 14 +++++++++++++- ssh2userauth.c | 20 ++++++++++++++++---- sshppl.h | 2 +- unix/uxplink.c | 2 ++ windows/winhelp.h | 1 + windows/winplink.c | 2 ++ 14 files changed, 100 insertions(+), 7 deletions(-) diff --git a/cmdline.c b/cmdline.c index 7ea5cacc..62d65e19 100644 --- a/cmdline.c +++ b/cmdline.c @@ -598,6 +598,14 @@ int cmdline_process_param(const char *p, char *value, SAVEABLE(0); conf_set_bool(conf, CONF_tryagent, false); } + + if (!strcmp(p, "-no-trivial-auth")) { + RETURN(1); + UNAVAILABLE_IN(TOOLTYPE_NONNETWORK); + SAVEABLE(0); + conf_set_bool(conf, CONF_ssh_no_trivial_userauth, true); + } + if (!strcmp(p, "-share")) { RETURN(1); UNAVAILABLE_IN(TOOLTYPE_NONNETWORK); diff --git a/config.c b/config.c index ca808511..b8348530 100644 --- a/config.c +++ b/config.c @@ -2772,6 +2772,10 @@ void setup_config_box(struct controlbox *b, bool midsession, HELPCTX(ssh_auth_bypass), conf_checkbox_handler, I(CONF_ssh_no_userauth)); + ctrl_checkbox(s, "Disconnect if authentication succeeds trivially", + 'n', HELPCTX(ssh_no_trivial_userauth), + conf_checkbox_handler, + I(CONF_ssh_no_trivial_userauth)); s = ctrl_getset(b, "Connection/SSH/Auth", "methods", "Authentication methods"); diff --git a/doc/config.but b/doc/config.but index a00ae476..77313282 100644 --- a/doc/config.but +++ b/doc/config.but @@ -2623,6 +2623,49 @@ interact with them.) This option only affects SSH-2 connections. SSH-1 connections always require an authentication step. +\S{config-ssh-notrivialauth} \q{Disconnect if authentication succeeds +trivially} + +This option causes PuTTY to abandon an SSH session and disconnect from +the server, if the server accepted authentication without ever having +asked for any kind of password or signature or token. + +This might be used as a security measure. There are some forms of +attack against an SSH client user which work by terminating the SSH +authentication stage early, and then doing something in the main part +of the SSH session which \e{looks} like part of the authentication, +but isn't really. + +For example, instead of demanding a signature from your public key, +for which PuTTY would ask for your key's passphrase, a compromised or +malicious server might allow you to log in with no signature or +password at all, and then print a message that \e{imitates} PuTTY's +request for your passphrase, in the hope that you would type it in. +(In fact, the passphrase for your public key should not be sent to any +server.) + +PuTTY's main defence against attacks of this type is the \q{trust +sigil} system: messages in the PuTTY window that are truly originated +by PuTTY itself are shown next to a small copy of the PuTTY icon, +which the server cannot fake when it tries to imitate the same message +using terminal output. + +However, if you think you might be at risk of this kind of thing +anyway (if you don't watch closely for the trust sigils, or if you +think you're at extra risk of one of your servers being malicious), +then you could enable this option as an extra defence. Then, if the +server tries any of these attacks involving letting you through the +authentication stage, PuTTY will disconnect from the server before it +can send a follow-up fake prompt or other type of attack. + +On the other hand, some servers \e{legitimately} let you through the +SSH authentication phase trivially, either because they are genuinely +public, or because the important authentication step happens during +the terminal session. (An example might be an SSH server that connects +you directly to the terminal login prompt of a legacy mainframe.) So +enabling this option might cause some kinds of session to stop +working. It's up to you. + \S{config-ssh-tryagent} \q{Attempt authentication using Pageant} If this option is enabled, then PuTTY will look for Pageant (the SSH diff --git a/pscp.c b/pscp.c index 0bfda92d..a11d1e18 100644 --- a/pscp.c +++ b/pscp.c @@ -2203,6 +2203,8 @@ static void usage(void) printf(" -i key private key file for user authentication\n"); printf(" -noagent disable use of Pageant\n"); printf(" -agent enable use of Pageant\n"); + printf(" -no-trivial-auth\n"); + printf(" disconnect if SSH authentication succeeds trivially\n"); printf(" -hostkey keyid\n"); printf(" manually specify a host key (may be repeated)\n"); printf(" -batch disable all interactive prompts\n"); diff --git a/psftp.c b/psftp.c index 40cb73f5..c4b5374b 100644 --- a/psftp.c +++ b/psftp.c @@ -2538,6 +2538,8 @@ static void usage(void) printf(" -i key private key file for user authentication\n"); printf(" -noagent disable use of Pageant\n"); printf(" -agent enable use of Pageant\n"); + printf(" -no-trivial-auth\n"); + printf(" disconnect if SSH authentication succeeds trivially\n"); printf(" -hostkey keyid\n"); printf(" manually specify a host key (may be repeated)\n"); printf(" -batch disable all interactive prompts\n"); diff --git a/putty.h b/putty.h index ef6f7a37..7789f217 100644 --- a/putty.h +++ b/putty.h @@ -1428,6 +1428,7 @@ NORETURN void cleanup_exit(int); X(INT, NONE, sshprot) \ X(BOOL, NONE, ssh2_des_cbc) /* "des-cbc" unrecommended SSH-2 cipher */ \ X(BOOL, NONE, ssh_no_userauth) /* bypass "ssh-userauth" (SSH-2 only) */ \ + X(BOOL, NONE, ssh_no_trivial_userauth) /* disable trivial types of auth */ \ X(BOOL, NONE, ssh_show_banner) /* show USERAUTH_BANNERs (SSH-2 only) */ \ X(BOOL, NONE, try_tis_auth) \ X(BOOL, NONE, try_ki_auth) \ diff --git a/settings.c b/settings.c index 547af0dc..32a53c54 100644 --- a/settings.c +++ b/settings.c @@ -609,6 +609,7 @@ void save_open_settings(settings_w *sesskey, Conf *conf) #endif write_setting_s(sesskey, "RekeyBytes", conf_get_str(conf, CONF_ssh_rekey_data)); write_setting_b(sesskey, "SshNoAuth", conf_get_bool(conf, CONF_ssh_no_userauth)); + write_setting_b(sesskey, "SshNoTrivialAuth", conf_get_bool(conf, CONF_ssh_no_trivial_userauth)); write_setting_b(sesskey, "SshBanner", conf_get_bool(conf, CONF_ssh_show_banner)); write_setting_b(sesskey, "AuthTIS", conf_get_bool(conf, CONF_try_tis_auth)); write_setting_b(sesskey, "AuthKI", conf_get_bool(conf, CONF_try_ki_auth)); @@ -1025,6 +1026,7 @@ void load_open_settings(settings_r *sesskey, Conf *conf) gpps(sesskey, "LogHost", "", conf, CONF_loghost); gppb(sesskey, "SSH2DES", false, conf, CONF_ssh2_des_cbc); gppb(sesskey, "SshNoAuth", false, conf, CONF_ssh_no_userauth); + gppb(sesskey, "SshNoTrivialAuth", false, conf, CONF_ssh_no_trivial_userauth); gppb(sesskey, "SshBanner", true, conf, CONF_ssh_show_banner); gppb(sesskey, "AuthTIS", false, conf, CONF_try_tis_auth); gppb(sesskey, "AuthKI", true, conf, CONF_try_ki_auth); diff --git a/ssh.c b/ssh.c index 00a516ea..91e7e869 100644 --- a/ssh.c +++ b/ssh.c @@ -254,7 +254,9 @@ static void ssh_got_ssh_version(struct ssh_version_receiver *rcv, connection_layer, ssh->savedhost, ssh->fullhostname, conf_get_filename(ssh->conf, CONF_keyfile), conf_get_bool(ssh->conf, CONF_ssh_show_banner), - conf_get_bool(ssh->conf, CONF_tryagent), username, + conf_get_bool(ssh->conf, CONF_tryagent), + conf_get_bool(ssh->conf, CONF_ssh_no_trivial_userauth), + username, conf_get_bool(ssh->conf, CONF_change_username), conf_get_bool(ssh->conf, CONF_try_ki_auth), #ifndef NO_GSSAPI diff --git a/ssh1login.c b/ssh1login.c index 8486cbf0..519838d7 100644 --- a/ssh1login.c +++ b/ssh1login.c @@ -27,7 +27,7 @@ struct ssh1_login_state { char *savedhost; int savedport; - bool try_agent_auth; + bool try_agent_auth, is_trivial_auth; int remote_protoflags; int local_protoflags; @@ -105,6 +105,8 @@ PacketProtocolLayer *ssh1_login_new( s->savedhost = dupstr(host); s->savedport = port; s->successor_layer = successor_layer; + s->is_trivial_auth = true; + return &s->ppl; } @@ -645,6 +647,7 @@ static void ssh1_login_process_queue(PacketProtocolLayer *ppl) s->ppl.bpp, SSH1_CMSG_AUTH_RSA_RESPONSE); put_data(pkt, ret + 5, 16); pq_push(s->ppl.out_pq, pkt); + s->is_trivial_auth = false; crMaybeWaitUntilV( (pktin = ssh1_login_pop(s)) != NULL); @@ -814,6 +817,7 @@ static void ssh1_login_process_queue(PacketProtocolLayer *ppl) s->ppl.bpp, SSH1_CMSG_AUTH_RSA_RESPONSE); put_data(pkt, buffer, 16); pq_push(s->ppl.out_pq, pkt); + s->is_trivial_auth = false; mp_free(challenge); mp_free(response); @@ -1105,6 +1109,7 @@ static void ssh1_login_process_queue(PacketProtocolLayer *ppl) put_stringz(pkt, prompt_get_result_ref(s->cur_prompt->prompts[0])); pq_push(s->ppl.out_pq, pkt); } + s->is_trivial_auth = false; ppl_logevent("Sent password"); free_prompts(s->cur_prompt); s->cur_prompt = NULL; @@ -1121,6 +1126,13 @@ static void ssh1_login_process_queue(PacketProtocolLayer *ppl) } } + if (conf_get_bool(s->conf, CONF_ssh_no_trivial_userauth) && + s->is_trivial_auth) { + ssh_proto_error(s->ppl.ssh, "Authentication was trivial! " + "Abandoning session as specified in configuration."); + return; + } + ppl_logevent("Authentication successful"); if (conf_get_bool(s->conf, CONF_compression)) { diff --git a/ssh2userauth.c b/ssh2userauth.c index 01243baf..451d5abe 100644 --- a/ssh2userauth.c +++ b/ssh2userauth.c @@ -28,7 +28,7 @@ struct ssh2_userauth_state { PacketProtocolLayer *transport_layer, *successor_layer; Filename *keyfile; - bool show_banner, tryagent, change_username; + bool show_banner, tryagent, notrivialauth, change_username; char *hostname, *fullhostname; char *default_username; bool try_ki_auth, try_gssapi_auth, try_gssapi_kex_auth, gssapi_fwd; @@ -82,6 +82,7 @@ struct ssh2_userauth_state { int len; PktOut *pktout; bool want_user_input; + bool is_trivial_auth; agent_pending_query *auth_agent_query; bufchain banner; @@ -134,7 +135,7 @@ static const PacketProtocolLayerVtable ssh2_userauth_vtable = { PacketProtocolLayer *ssh2_userauth_new( PacketProtocolLayer *successor_layer, const char *hostname, const char *fullhostname, - Filename *keyfile, bool show_banner, bool tryagent, + Filename *keyfile, bool show_banner, bool tryagent, bool notrivialauth, const char *default_username, bool change_username, bool try_ki_auth, bool try_gssapi_auth, bool try_gssapi_kex_auth, bool gssapi_fwd, struct ssh_connection_shared_gss_state *shgss) @@ -149,6 +150,7 @@ PacketProtocolLayer *ssh2_userauth_new( s->keyfile = filename_copy(keyfile); s->show_banner = show_banner; s->tryagent = tryagent; + s->notrivialauth = notrivialauth; s->default_username = dupstr(default_username); s->change_username = change_username; s->try_ki_auth = try_ki_auth; @@ -157,6 +159,7 @@ PacketProtocolLayer *ssh2_userauth_new( s->gssapi_fwd = gssapi_fwd; s->shgss = shgss; s->last_methods_string = strbuf_new(); + s->is_trivial_auth = true; bufchain_init(&s->banner); bufchain_sink_init(&s->banner_bs, &s->banner); @@ -818,6 +821,7 @@ static void ssh2_userauth_process_queue(PacketProtocolLayer *ppl) sigblob); pq_push(s->ppl.out_pq, s->pktout); s->type = AUTH_TYPE_PUBLICKEY; + s->is_trivial_auth = false; } else { ppl_logevent("Pageant refused signing request"); ppl_printf("Pageant failed to " @@ -1038,6 +1042,7 @@ static void ssh2_userauth_process_queue(PacketProtocolLayer *ppl) ssh_key_free(key->key); sfree(key->comment); sfree(key); + s->is_trivial_auth = false; } #ifndef NO_GSSAPI @@ -1169,6 +1174,7 @@ static void ssh2_userauth_process_queue(PacketProtocolLayer *ppl) * no longer says CONTINUE_NEEDED */ if (s->gss_sndtok.length != 0) { + s->is_trivial_auth = false; s->pktout = ssh_bpp_new_pktout( s->ppl.bpp, SSH2_MSG_USERAUTH_GSSAPI_TOKEN); @@ -1288,7 +1294,6 @@ static void ssh2_userauth_process_queue(PacketProtocolLayer *ppl) * Loop while the server continues to send INFO_REQUESTs. */ while (pktin->type == SSH2_MSG_USERAUTH_INFO_REQUEST) { - ptrlen name, inst; strbuf *sb; @@ -1308,6 +1313,7 @@ static void ssh2_userauth_process_queue(PacketProtocolLayer *ppl) */ s->num_prompts = get_uint32(pktin); for (uint32_t i = 0; i < s->num_prompts; i++) { + s->is_trivial_auth = false; ptrlen prompt = get_string(pktin); bool echo = get_bool(pktin); @@ -1472,7 +1478,7 @@ static void ssh2_userauth_process_queue(PacketProtocolLayer *ppl) pq_push_front(s->ppl.in_pq, pktin); } else if (s->can_passwd) { - + s->is_trivial_auth = false; /* * Plain old password authentication. */ @@ -1731,6 +1737,12 @@ static void ssh2_userauth_process_queue(PacketProtocolLayer *ppl) } userauth_success: + if (s->notrivialauth && s->is_trivial_auth) { + ssh_proto_error(s->ppl.ssh, "Authentication was trivial! " + "Abandoning session as specified in configuration."); + return; + } + /* * We've just received USERAUTH_SUCCESS, and we haven't sent * any packets since. Signal the transport layer to consider diff --git a/sshppl.h b/sshppl.h index b339d67c..2c1dbf42 100644 --- a/sshppl.h +++ b/sshppl.h @@ -116,7 +116,7 @@ PacketProtocolLayer *ssh2_transport_new( PacketProtocolLayer *ssh2_userauth_new( PacketProtocolLayer *successor_layer, const char *hostname, const char *fullhostname, - Filename *keyfile, bool show_banner, bool tryagent, + Filename *keyfile, bool show_banner, bool tryagent, bool notrivialauth, const char *default_username, bool change_username, bool try_ki_auth, bool try_gssapi_auth, bool try_gssapi_kex_auth, diff --git a/unix/uxplink.c b/unix/uxplink.c index 3e2a9b6b..240783f4 100644 --- a/unix/uxplink.c +++ b/unix/uxplink.c @@ -527,6 +527,8 @@ static void usage(void) printf(" -i key private key file for user authentication\n"); printf(" -noagent disable use of Pageant\n"); printf(" -agent enable use of Pageant\n"); + printf(" -no-trivial-auth\n"); + printf(" disconnect if SSH authentication succeeds trivially\n"); printf(" -noshare disable use of connection sharing\n"); printf(" -share enable use of connection sharing\n"); printf(" -hostkey keyid\n"); diff --git a/windows/winhelp.h b/windows/winhelp.h index ae5a7a7f..9011df45 100644 --- a/windows/winhelp.h +++ b/windows/winhelp.h @@ -111,6 +111,7 @@ #define WINHELP_CTX_ssh_kex_repeat "config-ssh-kex-rekey" #define WINHELP_CTX_ssh_kex_manual_hostkeys "config-ssh-kex-manual-hostkeys" #define WINHELP_CTX_ssh_auth_bypass "config-ssh-noauth" +#define WINHELP_CTX_ssh_no_trivial_userauth "config-ssh-notrivialauth" #define WINHELP_CTX_ssh_auth_banner "config-ssh-banner" #define WINHELP_CTX_ssh_auth_privkey "config-ssh-privkey" #define WINHELP_CTX_ssh_auth_agentfwd "config-ssh-agentfwd" diff --git a/windows/winplink.c b/windows/winplink.c index 9bda0712..58d43e6d 100644 --- a/windows/winplink.c +++ b/windows/winplink.c @@ -149,6 +149,8 @@ static void usage(void) printf(" -i key private key file for user authentication\n"); printf(" -noagent disable use of Pageant\n"); printf(" -agent enable use of Pageant\n"); + printf(" -no-trivial-auth\n"); + printf(" disconnect if SSH authentication succeeds trivially\n"); printf(" -noshare disable use of connection sharing\n"); printf(" -share enable use of connection sharing\n"); printf(" -hostkey keyid\n"); From 22d7888b3353279c2c4a4369d5e703826ec5aba4 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 3 Jul 2021 11:01:09 +0100 Subject: [PATCH 11/16] Restore missing screen updates from scrollbar buttons. In commit f69cf86a61a8109, I added a call to term_update that happens when we receive WM_VSCROLL / SB_THUMBPOSITION in the subsidiary message loop that Windows creates during the handling of WM_SYSCOMMAND / SC_VSCROLL. The effect was that interactive dragging of the scrollbar now redraws the window at every step, whereas previously it didn't. A user just pointed out that if you click on one of the scrollbar end buttons and hold it down until it begins emulating key repeat, the same bug occurs: the window isn't redrawn until you release the mouse button and the subsidiary message loop ends. This commit extends the previous fix to cover all of the WM_VSCROLL subtypes, instead of just SB_THUMBPOSITION and SB_THUMBTRACK. Redraws while holding down those scrollbar buttons now work again. (cherry picked from commit 2029aa55c22631bd1c46df0c7ce817770d38d203) --- windows/window.c | 93 +++++++++++++++++++++++------------------------- 1 file changed, 45 insertions(+), 48 deletions(-) diff --git a/windows/window.c b/windows/window.c index 57abad83..0c92a8f5 100644 --- a/windows/window.c +++ b/windows/window.c @@ -3160,57 +3160,54 @@ static LRESULT CALLBACK WndProc(HWND hwnd, UINT message, if (GetScrollInfo(hwnd, SB_VERT, &si) == 0) si.nTrackPos = HIWORD(wParam); term_scroll(term, 1, si.nTrackPos); - - if (in_scrollbar_loop) { - /* - * Allow window updates to happen during interactive - * scroll. - * - * When the user takes hold of our window's scrollbar - * and wobbles it interactively back and forth, the - * first thing that happens is that this window - * procedure receives WM_SYSCOMMAND / SC_VSCROLL. [1] - * The default handler for that window message starts - * a subsidiary message loop, which continues to run - * until the user lets go of the scrollbar again. All - * WM_VSCROLL / SB_THUMBTRACK messages are generated - * by the handlers within that subsidiary message - * loop. - * - * So, during that time, _our_ message loop is not - * running, which means toplevel callbacks and timers - * and so forth are not happening, which means that - * when we redraw the window and set a timer to clear - * the cooldown flag 20ms later, that timer never - * fires, and we aren't able to keep redrawing the - * window. - * - * The 'obvious' answer would be to seize that - * SYSCOMMAND ourselves and inhibit the default - * handler, so that our message loop carries on - * running. But that would mean we'd have to - * reimplement the whole of the scrollbar handler! - * - * So instead we apply a bodge: set a static variable - * that indicates that we're _in_ that sub-loop, and - * if so, decide it's OK to manually call - * term_update() proper, bypassing the timer and - * cooldown and rate-limiting systems completely, - * whenever we see an SB_THUMBTRACK. This shouldn't - * cause a rate overload, because we're only doing it - * once per UI event! - * - * [1] Actually, there's an extra oddity where - * SC_HSCROLL and SC_VSCROLL have their documented - * values the wrong way round. Many people on the - * Internet have noticed this, e.g. - * https://stackoverflow.com/q/55528397 - */ - term_update(term); - } break; } } + + if (in_scrollbar_loop) { + /* + * Allow window updates to happen during interactive + * scroll. + * + * When the user takes hold of our window's scrollbar and + * wobbles it interactively back and forth, or presses on + * one of the arrow buttons at the ends, the first thing + * that happens is that this window procedure receives + * WM_SYSCOMMAND / SC_VSCROLL. [1] The default handler for + * that window message starts a subsidiary message loop, + * which continues to run until the user lets go of the + * scrollbar again. All WM_VSCROLL / SB_THUMBTRACK + * messages are generated by the handlers within that + * subsidiary message loop. + * + * So, during that time, _our_ message loop is not + * running, which means toplevel callbacks and timers and + * so forth are not happening, which means that when we + * redraw the window and set a timer to clear the cooldown + * flag 20ms later, that timer never fires, and we aren't + * able to keep redrawing the window. + * + * The 'obvious' answer would be to seize that SYSCOMMAND + * ourselves and inhibit the default handler, so that our + * message loop carries on running. But that would mean + * we'd have to reimplement the whole of the scrollbar + * handler! + * + * So instead we apply a bodge: set a static variable that + * indicates that we're _in_ that sub-loop, and if so, + * decide it's OK to manually call term_update() proper, + * bypassing the timer and cooldown and rate-limiting + * systems completely, whenever we see an SB_THUMBTRACK. + * This shouldn't cause a rate overload, because we're + * only doing it once per UI event! + * + * [1] Actually, there's an extra oddity where SC_HSCROLL + * and SC_VSCROLL have their documented values the wrong + * way round. Many people on the Internet have noticed + * this, e.g. https://stackoverflow.com/q/55528397 + */ + term_update(term); + } break; case WM_PALETTECHANGED: if ((HWND) wParam != hwnd && pal != NULL) { From ea45d7dcd8cb3c2bf039997ddc3f6a0a846a2b7b Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 3 Jul 2021 11:01:09 +0100 Subject: [PATCH 12/16] Close all thread handles returned from CreateThread. If you don't, they are permanently leaked. A user points out that this is particularly bad in Pageant, with the new named-pipe-based IPC, since it will spawn an input and output I/O thread per named pipe connection, leading to two handles being leaked every time. (cherry picked from commit c714dfc936285fb588d047bb71b27cd396af6490) --- windows/window.c | 6 ++++-- windows/winhandl.c | 12 ++++++++---- windows/winpgen.c | 6 ++++-- windows/winpgnt.c | 6 ++++-- 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/windows/window.c b/windows/window.c index 0c92a8f5..a37f6525 100644 --- a/windows/window.c +++ b/windows/window.c @@ -5440,8 +5440,10 @@ static void wintw_clip_request_paste(TermWin *tw, int clipboard) * that tells us it's OK to paste. */ DWORD in_threadid; /* required for Win9x */ - CreateThread(NULL, 0, clipboard_read_threadfunc, - wgs.term_hwnd, 0, &in_threadid); + HANDLE hThread = CreateThread(NULL, 0, clipboard_read_threadfunc, + wgs.term_hwnd, 0, &in_threadid); + if (hThread) + CloseHandle(hThread); /* we don't need the thread handle */ } /* diff --git a/windows/winhandl.c b/windows/winhandl.c index 085f30a0..82d2aded 100644 --- a/windows/winhandl.c +++ b/windows/winhandl.c @@ -451,8 +451,10 @@ struct handle *handle_input_new(HANDLE handle, handle_inputfn_t gotdata, handles_by_evtomain = newtree234(handle_cmp_evtomain); add234(handles_by_evtomain, h); - CreateThread(NULL, 0, handle_input_threadfunc, - &h->u.i, 0, &in_threadid); + HANDLE hThread = CreateThread(NULL, 0, handle_input_threadfunc, + &h->u.i, 0, &in_threadid); + if (hThread) + CloseHandle(hThread); /* we don't need the thread handle */ h->u.i.busy = true; return h; @@ -482,8 +484,10 @@ struct handle *handle_output_new(HANDLE handle, handle_outputfn_t sentdata, handles_by_evtomain = newtree234(handle_cmp_evtomain); add234(handles_by_evtomain, h); - CreateThread(NULL, 0, handle_output_threadfunc, - &h->u.o, 0, &out_threadid); + HANDLE hThread = CreateThread(NULL, 0, handle_output_threadfunc, + &h->u.o, 0, &out_threadid); + if (hThread) + CloseHandle(hThread); /* we don't need the thread handle */ return h; } diff --git a/windows/winpgen.c b/windows/winpgen.c index 065ada80..56c6a8db 100644 --- a/windows/winpgen.c +++ b/windows/winpgen.c @@ -1160,13 +1160,15 @@ static void start_generating_key(HWND hwnd, struct MainDlgState *state) params->key = &state->key; params->dsskey = &state->dsskey; - if (!CreateThread(NULL, 0, generate_key_thread, - params, 0, &threadid)) { + HANDLE hThread = CreateThread(NULL, 0, generate_key_thread, + params, 0, &threadid); + if (!hThread) { MessageBox(hwnd, "Out of thread resources", "Key generation error", MB_OK | MB_ICONERROR); sfree(params); } else { + CloseHandle(hThread); /* we don't need the thread handle */ state->generation_thread_exists = true; } } diff --git a/windows/winpgnt.c b/windows/winpgnt.c index c0ffcf63..d6e960b6 100644 --- a/windows/winpgnt.c +++ b/windows/winpgnt.c @@ -1656,8 +1656,10 @@ int WINAPI WinMain(HINSTANCE inst, HINSTANCE prev, LPSTR cmdline, int show) DWORD wm_copydata_threadid; wmct.ev_msg_ready = CreateEvent(NULL, false, false, NULL); wmct.ev_reply_ready = CreateEvent(NULL, false, false, NULL); - CreateThread(NULL, 0, wm_copydata_threadfunc, - &inst, 0, &wm_copydata_threadid); + HANDLE hThread = CreateThread(NULL, 0, wm_copydata_threadfunc, + &inst, 0, &wm_copydata_threadid); + if (hThread) + CloseHandle(hThread); /* we don't need the thread handle */ handle_add_foreign_event(wmct.ev_msg_ready, wm_copydata_got_msg, NULL); if (show_keylist_on_startup) From d599e3e687389fa4973df4fea0e213fdee9258eb Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 3 Jul 2021 11:01:09 +0100 Subject: [PATCH 13/16] Avoid crash in MIT Kerberos for Windows on session restart. A user reports that if you have MIT KfW loaded, and your PuTTY session terminates without the PuTTY process exiting, and you select 'Restart Session' from the menu, then a crash occurs inside the Kerberos library itself. Scuttlebutt on the Internet suggested this might be to do with unloading and then reloading the DLL within the process lifetime, which indeed we were doing. Now we avoid doing that for the KfW library in particular, by keeping a tree234 of module handles marked 'never unload this'. This is a workaround at best, but it seems to stop the problem happening in my own tests. (cherry picked from commit 058e390ab5d40e5322b2d082c0e15b0dbde2c1d5) --- windows/wingss.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/windows/wingss.c b/windows/wingss.c index 10e6d2db..79c4921c 100644 --- a/windows/wingss.c +++ b/windows/wingss.c @@ -95,6 +95,28 @@ const char *gsslogmsg = NULL; static void ssh_sspi_bind_fns(struct ssh_gss_library *lib); +static tree234 *libraries_to_never_unload; +static int library_to_never_unload_cmp(void *av, void *bv) +{ + uintptr_t a = (uintptr_t)av, b = (uintptr_t)bv; + return a < b ? -1 : a > b ? +1 : 0; +} +static void ensure_library_tree_exists(void) +{ + if (!libraries_to_never_unload) + libraries_to_never_unload = newtree234(library_to_never_unload_cmp); +} +static bool library_is_in_never_unload_tree(HMODULE module) +{ + ensure_library_tree_exists(); + return find234(libraries_to_never_unload, module, NULL); +} +static void add_library_to_never_unload_tree(HMODULE module) +{ + ensure_library_tree_exists(); + add234(libraries_to_never_unload, module); +} + struct ssh_gss_liblist *ssh_gss_setup(Conf *conf) { HMODULE module; @@ -145,6 +167,23 @@ struct ssh_gss_liblist *ssh_gss_setup(Conf *conf) LOAD_LIBRARY_SEARCH_SYSTEM32 | LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR | LOAD_LIBRARY_SEARCH_USER_DIRS); + + /* + * The MIT Kerberos DLL suffers an internal segfault + * for some reason if you unload and reload one within + * the same process. So, make sure that after we load + * this library, we never free it. + * + * Or rather: after we've loaded it once, if any + * _further_ load returns the same module handle, we + * immediately free it again (to prevent the Windows + * API's internal reference count growing without + * bound). But on the other hand we never free it in + * ssh_gss_cleanup. + */ + if (library_is_in_never_unload_tree(module)) + FreeLibrary(module); + add_library_to_never_unload_tree(module); } sfree(buffer); } @@ -280,7 +319,11 @@ void ssh_gss_cleanup(struct ssh_gss_liblist *list) * another SSH instance still using it. */ for (i = 0; i < list->nlibraries; i++) { - FreeLibrary((HMODULE)list->libraries[i].handle); + if (list->libraries[i].id != 0) { + HMODULE module = (HMODULE)list->libraries[i].handle; + if (!library_is_in_never_unload_tree(module)) + FreeLibrary(module); + } if (list->libraries[i].id == 2) { /* The 'custom' id involves a dynamically allocated message. * Note that we must cast away the 'const' to free it. */ From 6db3ac478383e499d6eea845eda906af9d98796a Mon Sep 17 00:00:00 2001 From: Jacob Nevins Date: Fri, 9 Jul 2021 23:55:15 +0100 Subject: [PATCH 14/16] Document -no-trivial-auth more thoroughly. (cherry-picked from commit 413398af85b27cd83134f5618bd82f81758f9603) --- doc/man-plink.but | 9 +++++++++ doc/man-pscp.but | 9 +++++++++ doc/man-psftp.but | 9 +++++++++ doc/man-putty.but | 9 +++++++++ doc/plink.but | 2 ++ doc/pscp.but | 2 ++ doc/using.but | 9 +++++++++ 7 files changed, 49 insertions(+) diff --git a/doc/man-plink.but b/doc/man-plink.but index 33386227..89dadb09 100644 --- a/doc/man-plink.but +++ b/doc/man-plink.but @@ -203,6 +203,15 @@ which of the agent's keys to use. } \dd Allow use of an authentication agent. (This option is only necessary to override a setting in a saved session.) +\dt \cw{\-no\-trivial\-auth} + +\dd Disconnect from any SSH server which accepts authentication without +ever having asked for any kind of password or signature or token. (You +might want to enable this for a server you always expect to challenge +you, for instance to ensure ensure you don't accidentally type your key +file's passphrase into a compromised server spoofing Plink's passphrase +prompt.) + \dt \cw{\-noshare} \dd Don't test and try to share an existing connection, always make diff --git a/doc/man-pscp.but b/doc/man-pscp.but index b62e8cc2..8011483d 100644 --- a/doc/man-pscp.but +++ b/doc/man-pscp.but @@ -155,6 +155,15 @@ which of the agent's keys to use. } \dd Allow use of an authentication agent. (This option is only necessary to override a setting in a saved session.) +\dt \cw{\-no\-trivial\-auth} + +\dd Disconnect from any SSH server which accepts authentication without +ever having asked for any kind of password or signature or token. (You +might want to enable this for a server you always expect to challenge +you, for instance to ensure ensure you don't accidentally type your key +file's passphrase into a compromised server spoofing PSCP's passphrase +prompt.) + \dt \cw{\-hostkey} \e{key} \dd Specify an acceptable host public key. This option may be specified diff --git a/doc/man-psftp.but b/doc/man-psftp.but index 19f820e3..0c47aa0e 100644 --- a/doc/man-psftp.but +++ b/doc/man-psftp.but @@ -143,6 +143,15 @@ which of the agent's keys to use. } \dd Allow use of an authentication agent. (This option is only necessary to override a setting in a saved session.) +\dt \cw{\-no\-trivial\-auth} + +\dd Disconnect from any SSH server which accepts authentication without +ever having asked for any kind of password or signature or token. (You +might want to enable this for a server you always expect to challenge +you, for instance to ensure ensure you don't accidentally type your key +file's passphrase into a compromised server spoofing PSFTP's passphrase +prompt.) + \dt \cw{\-hostkey} \e{key} \dd Specify an acceptable host public key. This option may be specified diff --git a/doc/man-putty.but b/doc/man-putty.but index a1656d6c..3214a180 100644 --- a/doc/man-putty.but +++ b/doc/man-putty.but @@ -287,6 +287,15 @@ which of the agent's keys to use. } \dd Allow use of an authentication agent. (This option is only necessary to override a setting in a saved session.) +\dt \cw{\-no\-trivial\-auth} + +\dd Disconnect from any SSH server which accepts authentication without +ever having asked for any kind of password or signature or token. (You +might want to enable this for a server you always expect to challenge +you, for instance to ensure ensure you don't accidentally type your key +file's passphrase into a compromised server spoofing PuTTY's passphrase +prompt.) + \dt \cw{\-hostkey} \e{key} \dd Specify an acceptable host public key. This option may be specified diff --git a/doc/plink.but b/doc/plink.but index fcfb5f68..361b59c2 100644 --- a/doc/plink.but +++ b/doc/plink.but @@ -77,6 +77,8 @@ use Plink: \c -i key private key file for user authentication \c -noagent disable use of Pageant \c -agent enable use of Pageant +\c -no-trivial-auth +\c disconnect if SSH authentication succeeds trivially \c -noshare disable use of connection sharing \c -share enable use of connection sharing \c -hostkey keyid diff --git a/doc/pscp.but b/doc/pscp.but index 9d8daccd..2ee35ced 100644 --- a/doc/pscp.but +++ b/doc/pscp.but @@ -62,6 +62,8 @@ use PSCP: \c -i key private key file for user authentication \c -noagent disable use of Pageant \c -agent enable use of Pageant +\c -no-trivial-auth +\c disconnect if SSH authentication succeeds trivially \c -hostkey keyid \c manually specify a host key (may be repeated) \c -batch disable all interactive prompts diff --git a/doc/using.but b/doc/using.but index b583dc8c..02a67808 100644 --- a/doc/using.but +++ b/doc/using.but @@ -1014,6 +1014,15 @@ This option is equivalent to the \q{Private key file for authentication} box in the Auth panel of the PuTTY configuration box (see \k{config-ssh-privkey}). +\S2{using-cmdline-no-trivial-auth} \i\c{-no-trivial-auth}: disconnect +if SSH authentication succeeds trivially + +This option causes PuTTY to abandon an SSH session if the server +accepts authentication without ever having asked for any kind of +password or signature or token. + +See \k{config-ssh-notrivialauth} for why you might want this. + \S2{using-cmdline-loghost} \i\c{-loghost}: specify a \i{logical host name} From be66cb7b15875aff36cceaa327487c538c7e7cc8 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 10 Jul 2021 10:31:07 +0100 Subject: [PATCH 15/16] Fix grammar nit. Ensure 'ensure ensure' doesn't make it into the release documentation :-) (cherry picked from commit 640e46a11254f553c9835b4e9a6f60578c6adc95) --- doc/man-plink.but | 4 ++-- doc/man-pscp.but | 5 ++--- doc/man-psftp.but | 4 ++-- doc/man-putty.but | 4 ++-- 4 files changed, 8 insertions(+), 9 deletions(-) diff --git a/doc/man-plink.but b/doc/man-plink.but index 89dadb09..26e65f71 100644 --- a/doc/man-plink.but +++ b/doc/man-plink.but @@ -208,8 +208,8 @@ to override a setting in a saved session.) \dd Disconnect from any SSH server which accepts authentication without ever having asked for any kind of password or signature or token. (You might want to enable this for a server you always expect to challenge -you, for instance to ensure ensure you don't accidentally type your key -file's passphrase into a compromised server spoofing Plink's passphrase +you, for instance to ensure you don't accidentally type your key file's +passphrase into a compromised server spoofing Plink's passphrase prompt.) \dt \cw{\-noshare} diff --git a/doc/man-pscp.but b/doc/man-pscp.but index 8011483d..60ce4f5e 100644 --- a/doc/man-pscp.but +++ b/doc/man-pscp.but @@ -160,9 +160,8 @@ to override a setting in a saved session.) \dd Disconnect from any SSH server which accepts authentication without ever having asked for any kind of password or signature or token. (You might want to enable this for a server you always expect to challenge -you, for instance to ensure ensure you don't accidentally type your key -file's passphrase into a compromised server spoofing PSCP's passphrase -prompt.) +you, for instance to ensure you don't accidentally type your key file's +passphrase into a compromised server spoofing PSCP's passphrase prompt.) \dt \cw{\-hostkey} \e{key} diff --git a/doc/man-psftp.but b/doc/man-psftp.but index 0c47aa0e..52617291 100644 --- a/doc/man-psftp.but +++ b/doc/man-psftp.but @@ -148,8 +148,8 @@ to override a setting in a saved session.) \dd Disconnect from any SSH server which accepts authentication without ever having asked for any kind of password or signature or token. (You might want to enable this for a server you always expect to challenge -you, for instance to ensure ensure you don't accidentally type your key -file's passphrase into a compromised server spoofing PSFTP's passphrase +you, for instance to ensure you don't accidentally type your key file's +passphrase into a compromised server spoofing PSFTP's passphrase prompt.) \dt \cw{\-hostkey} \e{key} diff --git a/doc/man-putty.but b/doc/man-putty.but index 3214a180..858ec0b0 100644 --- a/doc/man-putty.but +++ b/doc/man-putty.but @@ -292,8 +292,8 @@ to override a setting in a saved session.) \dd Disconnect from any SSH server which accepts authentication without ever having asked for any kind of password or signature or token. (You might want to enable this for a server you always expect to challenge -you, for instance to ensure ensure you don't accidentally type your key -file's passphrase into a compromised server spoofing PuTTY's passphrase +you, for instance to ensure you don't accidentally type your key file's +passphrase into a compromised server spoofing PuTTY's passphrase prompt.) \dt \cw{\-hostkey} \e{key} From 1fd7baa7344bb38d62a024e5dba3a720c67d05cf Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 10 Jul 2021 10:39:20 +0100 Subject: [PATCH 16/16] Update version number for 0.76 release. --- Buildscr | 2 +- LATEST.VER | 2 +- doc/plink.but | 2 +- doc/pscp.but | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Buildscr b/Buildscr index 68a19072..f7556f77 100644 --- a/Buildscr +++ b/Buildscr @@ -35,7 +35,7 @@ module putty ifeq "$(RELEASE)" "" set Ndate $(!builddate) ifneq "$(Ndate)" "" in . do echo $(Ndate) | perl -pe 's/(....)(..)(..)/$$1-$$2-$$3/' > date ifneq "$(Ndate)" "" read Date date -set Epoch 17749 # update this at every release +set Epoch 17818 # update this at every release ifneq "$(Ndate)" "" in . do echo $(Ndate) | perl -ne 'use Time::Local; /(....)(..)(..)/ and print timegm(0,0,0,$$3,$$2-1,$$1) / 86400 - $(Epoch)' > days ifneq "$(Ndate)" "" read Days days diff --git a/LATEST.VER b/LATEST.VER index 39c64a9f..07b41f7d 100644 --- a/LATEST.VER +++ b/LATEST.VER @@ -1 +1 @@ -0.75 +0.76 diff --git a/doc/plink.but b/doc/plink.but index 361b59c2..30dcead1 100644 --- a/doc/plink.but +++ b/doc/plink.but @@ -41,7 +41,7 @@ use Plink: \c C:\>plink \c Plink: command-line connection utility -\c Release 0.75 +\c Release 0.76 \c Usage: plink [options] [user@]host [command] \c ("host" can also be a PuTTY saved session name) \c Options: diff --git a/doc/pscp.but b/doc/pscp.but index 2ee35ced..e816f3e5 100644 --- a/doc/pscp.but +++ b/doc/pscp.but @@ -39,7 +39,7 @@ use PSCP: \c C:\>pscp \c PuTTY Secure Copy client -\c Release 0.75 +\c Release 0.76 \c Usage: pscp [options] [user@]host:source target \c pscp [options] source [source...] [user@]host:target \c pscp [options] -ls [user@]host:filespec