From 055b37cc5544af505179330317090b352c8bf014 Mon Sep 17 00:00:00 2001 From: "Pavel I. Kryukov" Date: Sun, 15 Dec 2019 13:57:52 +0300 Subject: [PATCH 01/62] sclog.c: print 'stores' for memory stores (cherry picked from commit bac0a4dba7b8532c16ec5ca1380e1e3c491cdb3d) --- test/sclog/sclog.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/sclog/sclog.c b/test/sclog/sclog.c index 39b7b5d6..b4871152 100644 --- a/test/sclog/sclog.c +++ b/test/sclog/sclog.c @@ -388,10 +388,10 @@ static dr_emit_flags_t instrument_instr( if (instr_reads_memory(instr) || instr_writes_memory(instr)) { for (int i = 0, limit = instr_num_srcs(instr); i < limit; i++) try_mem_opnd(drcontext, bb, instr, &loc, - instr_get_src(instr, i), false); + instr_get_src(instr, i), instr_writes_memory(instr)); for (int i = 0, limit = instr_num_dsts(instr); i < limit; i++) try_mem_opnd(drcontext, bb, instr, &loc, - instr_get_dst(instr, i), false); + instr_get_dst(instr, i), instr_writes_memory(instr)); } /* From 609e527d6d8ec881c751030de6b2a55764228c34 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 15 Dec 2019 20:39:01 +0000 Subject: [PATCH 02/62] testsc: print the address of main(). As explained in the comment in the code, this makes it easier to map addresses in the log files back to addresses in the code, if the testsc image is built as a position-independent executable. (cherry picked from commit 2804789be8640d5d4f0bd72e26c7ed1ff94e279e) --- testsc.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/testsc.c b/testsc.c index 2005a210..79c5ba1f 100644 --- a/testsc.c +++ b/testsc.c @@ -1454,6 +1454,15 @@ int main(int argc, char **argv) if (is_dry_run) { printf("Dry run (DynamoRIO instrumentation not detected)\n"); } else { + /* Print the address of main() in this run. The idea is that + * if this image is compiled to be position-independent, then + * PC values in the logs won't match the ones you get if you + * disassemble the binary, so it'll be harder to match up the + * log messages to the code. But if you know the address of a + * fixed (and not inlined) function in both worlds, you can + * find out the offset between them. */ + printf("Live run, main = %p\n", (void *)main); + if (!outdir) { fprintf(stderr, "expected -O option\n"); return 1; From 9dd00020ae8a1542eb9f094a277e30f5f2234146 Mon Sep 17 00:00:00 2001 From: "Pavel I. Kryukov" Date: Sun, 15 Dec 2019 23:10:15 +0300 Subject: [PATCH 03/62] Update out_of_memory stub function for utils.c test (cherry picked from commit 056288677bd77b62435956975a937f47ce0f4dc8) --- utils.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/utils.c b/utils.c index 6cd1cf4c..3b0f75c5 100644 --- a/utils.c +++ b/utils.c @@ -182,10 +182,21 @@ int main(void) printf("passed %d failed %d total %d\n", passes, fails, passes+fails); return fails != 0 ? 1 : 0; } + /* Stubs to stop the rest of this module causing compile failures. */ -void modalfatalbox(const char *fmt, ...) {} -int conf_get_int(Conf *conf, int primary) { return 0; } -char *conf_get_str(Conf *conf, int primary) { return NULL; } +static NORETURN void fatal_error(const char *p, ...) +{ + va_list ap; + fprintf(stderr, "host_string_test: "); + va_start(ap, p); + vfprintf(stderr, p, ap); + va_end(ap); + fputc('\n', stderr); + exit(1); +} + +void out_of_memory(void) { fatal_error("out of memory"); } + #endif /* TEST_HOST_STRFOO */ /* From ef936e72a2e4f99aeba73616def926aa79ad7733 Mon Sep 17 00:00:00 2001 From: "Pavel I. Kryukov" Date: Mon, 16 Dec 2019 12:19:30 +0300 Subject: [PATCH 04/62] cgtest: return non-zero if any test failed (cherry picked from commit 83408f928de23f033db211eb107825631bbcb390) --- cmdgen.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmdgen.c b/cmdgen.c index 8b39f1d8..5c17ce2b 100644 --- a/cmdgen.c +++ b/cmdgen.c @@ -1679,7 +1679,7 @@ int main(int argc, char **argv) test(1, "puttygen", "-C", "spurious-new-comment", pubfilename, NULL); } printf("%d passes, %d fails\n", passes, fails); - return 0; + return fails == 0 ? 0 : 1; } #endif From 3d44cef8ea8ab6e00d8c618d028001a7c25762a4 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 21 Dec 2019 13:31:02 +0000 Subject: [PATCH 05/62] winsftp.c: avoid creating multiple netevents. The do_select function is called with a boolean parameter indicating whether we're supposed to start or stop paying attention to network activity on a given socket. So if we freeze and unfreeze the socket in mid-session because of backlog, we'll call do_select(s, false) to freeze it, and do_select(s, true) to unfreeze it. But the implementation of do_select in the Windows SFTP code predated the rigorous handling of socket backlogs, so it assumed that do_select(s, true) would only be called at initialisation time, i.e. only once, and therefore that it was safe to use that flag as a cue to set up the Windows event object to associate with socket activity. Hence, every time the socket was frozen and unfrozen, we would create a new netevent at unfreeze time, leaking the old one. I think perhaps part of the reason why that was hard to figure out was that the boolean parameter was called 'startup' rather than 'enable'. To make it less confusing the next time I read this code, I've also renamed it, and while I was at it, adjusted another related comment. (cherry picked from commit bd5c957e5bd0f850ab30c6b1ab94bfbbabe9f006) --- windows/window.c | 4 ++-- windows/winplink.c | 4 ++-- windows/winsftp.c | 14 +++++++++----- windows/winstuff.h | 2 +- 4 files changed, 14 insertions(+), 10 deletions(-) diff --git a/windows/window.c b/windows/window.c index f9e09aa3..a17a9854 100644 --- a/windows/window.c +++ b/windows/window.c @@ -1026,10 +1026,10 @@ void cleanup_exit(int code) /* * Set up, or shut down, an AsyncSelect. Called from winnet.c. */ -char *do_select(SOCKET skt, bool startup) +char *do_select(SOCKET skt, bool enable) { int msg, events; - if (startup) { + if (enable) { msg = WM_NETEVENT; events = (FD_CONNECT | FD_READ | FD_WRITE | FD_OOB | FD_CLOSE | FD_ACCEPT); diff --git a/windows/winplink.c b/windows/winplink.c index dce5691f..39ca8fb5 100644 --- a/windows/winplink.c +++ b/windows/winplink.c @@ -190,10 +190,10 @@ static void version(void) exit(0); } -char *do_select(SOCKET skt, bool startup) +char *do_select(SOCKET skt, bool enable) { int events; - if (startup) { + if (enable) { events = (FD_CONNECT | FD_READ | FD_WRITE | FD_OOB | FD_CLOSE | FD_ACCEPT); } else { diff --git a/windows/winsftp.c b/windows/winsftp.c index b120890c..91c3f2fd 100644 --- a/windows/winsftp.c +++ b/windows/winsftp.c @@ -467,19 +467,21 @@ char *dir_file_cat(const char *dir, const char *file) */ static SOCKET sftp_ssh_socket = INVALID_SOCKET; static HANDLE netevent = INVALID_HANDLE_VALUE; -char *do_select(SOCKET skt, bool startup) +char *do_select(SOCKET skt, bool enable) { int events; - if (startup) + if (enable) sftp_ssh_socket = skt; else sftp_ssh_socket = INVALID_SOCKET; + if (netevent == INVALID_HANDLE_VALUE) + netevent = CreateEvent(NULL, false, false, NULL); + if (p_WSAEventSelect) { - if (startup) { + if (enable) { events = (FD_CONNECT | FD_READ | FD_WRITE | FD_OOB | FD_CLOSE | FD_ACCEPT); - netevent = CreateEvent(NULL, false, false, NULL); } else { events = 0; } @@ -732,7 +734,9 @@ char *ssh_sftp_get_cmdline(const char *prompt, bool no_fds_ok) do { ret = do_eventsel_loop(ctx->event); - /* Error return can only occur if netevent==NULL, and it ain't. */ + /* do_eventsel_loop can't return an error (unlike + * ssh_sftp_loop_iteration, which can return -1 if select goes + * wrong or if the socket doesn't exist). */ assert(ret >= 0); } while (ret == 0); diff --git a/windows/winstuff.h b/windows/winstuff.h index 0b191d54..c189c37a 100644 --- a/windows/winstuff.h +++ b/windows/winstuff.h @@ -350,7 +350,7 @@ DECL_WINDOWS_FUNCTION(GLOBAL, int, select, * Provided by each client of winnet.c, and called by winnet.c to turn * on or off WSA*Select for a given socket. */ -char *do_select(SOCKET skt, bool startup); +char *do_select(SOCKET skt, bool enable); /* * Network-subsystem-related functions provided in other Windows modules. From b77bcae02133f65c5446d48f0d570214d8006223 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 24 Dec 2019 10:52:38 +0000 Subject: [PATCH 06/62] Fix cursor save/restore with [?1047 alt-screen sequences. A long time ago, in commit 09f86ce7e, I introduced a separate copy of the saved cursor position (used by the ESC 7 / ESC 8 sequences) for the main and alternate screens. The idea was to fix mishandling of an input sequence of the form ESC 7 (save cursor) ESC [?47h (switch to alternate screen) ... ESC 7 ESC 8 (save and restore cursor, while in alternate screen) ... ESC [?47l (switch back from alternate screen) ESC 8 (restore cursor, expecting it to match the _first_ ESC 7) in which, before the fix, the second ESC 7 would overwrite the position saved by the first one. So the final ESC 8 would restore the cursor position to wherever it happened to have been saved in the alternate screen, instead of where it was saved before switching _to_ the alternate screen. I've recently noticed that the same bug still happens if you use the alternative escape sequences ESC[?1047h and ESC[?1047l to switch to the alternate screen, instead of ESC[?47h and ESC[?47l. This is because that version of the escape sequence sets the internal flag 'keep_cur_pos' in the call to swap_screen, whose job is to arrange that the actual cursor position doesn't change at the instant of the switch. But the code that swaps the _saved_ cursor position in and out is also conditioned on keep_cur_pos, so the 1047 variant of the screen-swap sequence was bypassing that too, and behaving as if there was just a single saved cursor position inside and outside the alternate screen. I don't know why I did it that way in 2006. It could have been deliberate for some reason, or it could just have been mindless copy and paste from the existing cursor-related swap code. But checking with xterm now, it definitely seems to be wrong: the 1047 screen swap preserves the _actual_ cursor position across the swap, but still has independent _saved_ cursor positions in the two screens. So now PuTTY does the same. (cherry picked from commit 421a8ca5d9e78bc1ac0df0c6dacc04756f5a96e0) --- terminal.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/terminal.c b/terminal.c index 60262d24..8d682ccb 100644 --- a/terminal.c +++ b/terminal.c @@ -2089,35 +2089,35 @@ static void swap_screen(Terminal *term, int which, term->alt_sco_acs = t; tp = term->savecurs; - if (!reset && !keep_cur_pos) + if (!reset) term->savecurs = term->alt_savecurs; term->alt_savecurs = tp; t = term->save_cset; - if (!reset && !keep_cur_pos) + if (!reset) term->save_cset = term->alt_save_cset; term->alt_save_cset = t; t = term->save_csattr; - if (!reset && !keep_cur_pos) + if (!reset) term->save_csattr = term->alt_save_csattr; term->alt_save_csattr = t; t = term->save_attr; - if (!reset && !keep_cur_pos) + if (!reset) term->save_attr = term->alt_save_attr; term->alt_save_attr = t; ttc = term->save_truecolour; - if (!reset && !keep_cur_pos) + if (!reset) term->save_truecolour = term->alt_save_truecolour; term->alt_save_truecolour = ttc; bt = term->save_utf; - if (!reset && !keep_cur_pos) + if (!reset) term->save_utf = term->alt_save_utf; term->alt_save_utf = bt; bt = term->save_wnext; - if (!reset && !keep_cur_pos) + if (!reset) term->save_wnext = term->alt_save_wnext; term->alt_save_wnext = bt; t = term->save_sco_acs; - if (!reset && !keep_cur_pos) + if (!reset) term->save_sco_acs = term->alt_save_sco_acs; term->alt_save_sco_acs = t; } From 874ce8239cd7ebcf8e12effdc18e0ccd7c477d73 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 30 Dec 2019 23:23:42 +0000 Subject: [PATCH 07/62] Fix handling of scroll position when swapping screens. If the user is scrolled back in the scrollback when a screen-swap takes place, and if we're not configured to reset the scrollback completely on the grounds that the swap is display activity, then we should do the same thing we do for other kinds of display activity: strive to keep the scroll position pointing at the same text. In this case, that means adjusting term->disptop by the number of virtual lines added to the scrollback to allow the main screen to be viewed while the alt screen is active. This improves the quality of behaviour in that corner case, but more importantly, it should also fix a case of the dreaded line==NULL assertion failure, which someone just reported against 0.73 when exiting tmux (hence, switching away from the alt screen) while scrolled back in a purely virtual scrollback buffer: the virtual scrollback lines vanished, but disptop was still set to a negative value, which made it out of range. (cherry picked from commit 22453b46daf2b03f5fe8b2bfac35b818f6c789fe) --- terminal.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/terminal.c b/terminal.c index 8d682ccb..ad54463b 100644 --- a/terminal.c +++ b/terminal.c @@ -2046,6 +2046,21 @@ static void swap_screen(Terminal *term, int which, reset = false; /* do no weird resetting if which==0 */ if (which != term->alt_which) { + if (term->erase_to_scrollback && term->alt_screen && + term->alt_which && term->disptop < 0) { + /* + * We're swapping away from the alternate screen, so some + * lines are about to vanish from the virtual scrollback. + * Adjust disptop by that much, so that (if we're not + * resetting the scrollback anyway on a display event) the + * current scroll position still ends up pointing at the + * same text. + */ + term->disptop += term->alt_sblines; + if (term->disptop > 0) + term->disptop = 0; + } + term->alt_which = which; ttr = term->alt_screen; @@ -2120,6 +2135,26 @@ static void swap_screen(Terminal *term, int which, if (!reset) term->save_sco_acs = term->alt_save_sco_acs; term->alt_save_sco_acs = t; + + if (term->erase_to_scrollback && term->alt_screen && + term->alt_which && term->disptop < 0) { + /* + * Inverse of the adjustment at the top of this function. + * This time, we're swapping _to_ the alternate screen, so + * some lines are about to _appear_ in the virtual + * scrollback, and we adjust disptop in the other + * direction. + * + * Both these adjustments depend on the value stored in + * term->alt_sblines while the alt screen is selected, + * which is why we had to do one _before_ switching away + * from it and the other _after_ switching to it. + */ + term->disptop -= term->alt_sblines; + int limit = -sblines(term); + if (term->disptop < limit) + term->disptop = limit; + } } if (reset && term->screen) { From 9407aef7049bb45b8fcefa06f793b7f4ac480c52 Mon Sep 17 00:00:00 2001 From: Jacob Nevins Date: Wed, 1 Jan 2020 16:54:24 +0000 Subject: [PATCH 08/62] It is, once again, a new year. (cherry picked from commit e5107478f3119c2ee3d40a3e7dd29bdf96490db2) --- LICENCE | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/LICENCE b/LICENCE index 98c8c85a..148912ae 100644 --- a/LICENCE +++ b/LICENCE @@ -1,4 +1,4 @@ -PuTTY is copyright 1997-2019 Simon Tatham. +PuTTY is copyright 1997-2020 Simon Tatham. Portions copyright Robert de Bath, Joris van Rantwijk, Delian Delchev, Andreas Schultz, Jeroen Massar, Wez Furlong, Nicolas Barry, From 6f1f04839b99b4f0a66bfb4c4b32c650925234ad Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 6 Jan 2020 19:58:01 +0000 Subject: [PATCH 09/62] testcrypt: fix malformatted error message. I managed to get two format parameters reversed in the message when a return type is unhandled. (cherry picked from commit 9cf2db5f94592c43b2423372ee19d3d8ec0ea5df) --- test/testcrypt.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/testcrypt.py b/test/testcrypt.py index 42da9d89..ab1e485d 100644 --- a/test/testcrypt.py +++ b/test/testcrypt.py @@ -182,7 +182,7 @@ def make_retval(rettype, word, unpack_strings): assert word == b"true" or word == b"false" return word == b"true" raise TypeError("Can't deal with return value {!r} of type {!r}" - .format(rettype, word)) + .format(word, rettype)) def make_retvals(rettypes, retwords, unpack_strings=True): assert len(rettypes) == len(retwords) # FIXME: better exception From a56b79b598902c7393fd79c63373a8b4eba4833c Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Thu, 9 Jan 2020 19:52:19 +0000 Subject: [PATCH 10/62] Change line ending wording in PPK format spec. This doesn't affect what files are _legal_: the spec said we tolerated three kinds of line ending, and it still says we tolerate the same three. But I noticed that we're actually outputting \n by preference, whereas the spec said we prefer \r\n. I'd rather change the docs than the code. (cherry picked from commit cbfd7dadacbed92ed25d132cdc04cee819e9202a) --- sshpubk.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sshpubk.c b/sshpubk.c index 442fb5ed..c9423192 100644 --- a/sshpubk.c +++ b/sshpubk.c @@ -400,8 +400,8 @@ bool rsa_ssh1_savekey(const Filename *filename, RSAKey *key, /* * PuTTY's own format for SSH-2 keys is as follows: * - * The file is text. Lines are terminated by CRLF, although CR-only - * and LF-only are tolerated on input. + * The file is text. Lines are terminated by LF by preference, + * although CRLF and CR-only are tolerated on input. * * The first line says "PuTTY-User-Key-File-2: " plus the name of the * algorithm ("ssh-dss", "ssh-rsa" etc). From d51b3d7eb63bf72a85950d008109201c0608e9dc Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 6 Jan 2020 19:55:41 +0000 Subject: [PATCH 11/62] Add BinarySource_REWIND_TO. A partial cherry-pick of commit 32d61d7c48b7cf0eb7445fb410e339bbcadd1d61 omitting the parts that aren't needed on this branch. --- marshal.c | 11 +++++++++++ marshal.h | 6 ++++++ 2 files changed, 17 insertions(+) diff --git a/marshal.c b/marshal.c index a9b1074a..1b35b455 100644 --- a/marshal.c +++ b/marshal.c @@ -235,6 +235,17 @@ ptrlen BinarySource_get_pstring(BinarySource *src) return make_ptrlen(consume(len), len); } +void BinarySource_REWIND_TO__(BinarySource *src, size_t pos) +{ + if (pos <= src->len) { + src->pos = pos; + src->err = BSE_NO_ERROR; /* clear any existing error */ + } else { + src->pos = src->len; + src->err = BSE_OUT_OF_DATA; /* new error if we rewind out of range */ + } +} + static void stdio_sink_write(BinarySink *bs, const void *data, size_t len) { stdio_sink *sink = BinarySink_DOWNCAST(bs, stdio_sink); diff --git a/marshal.h b/marshal.h index bc020a8d..5cbc0797 100644 --- a/marshal.h +++ b/marshal.h @@ -255,6 +255,10 @@ static inline void BinarySource_INIT__(BinarySource *src, ptrlen data) (object)->binarysource_) #define BinarySource_COPIED(obj) \ ((obj)->binarysource_->binarysource_ = (obj)->binarysource_) +#define BinarySource_REWIND_TO(src, pos) \ + BinarySource_REWIND_TO__((src)->binarysource_, pos) +#define BinarySource_REWIND(src) \ + BinarySource_REWIND_TO__((src)->binarysource_, 0) #define get_data(src, len) \ BinarySource_get_data(BinarySource_UPCAST(src), len) @@ -305,6 +309,8 @@ ptrlen BinarySource_get_pstring(BinarySource *); mp_int *BinarySource_get_mp_ssh1(BinarySource *src); mp_int *BinarySource_get_mp_ssh2(BinarySource *src); +void BinarySource_REWIND_TO__(BinarySource *src, size_t pos); + /* * A couple of useful standard BinarySink implementations, which live * as sensibly here as anywhere else: one that makes a BinarySink From 3b1f458a0dbf1608172551196de727da4ecd56aa Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 6 Jan 2020 19:58:25 +0000 Subject: [PATCH 12/62] testcrypt: add return_val_string_asciz_const. A partial cherry-pick of commit 5cfc90ff0de9d51c1556186e4935e9f4cb0cb7a0 keeping just the one function I need on this branch. --- testcrypt.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/testcrypt.c b/testcrypt.c index 06f2f32b..0035c0c0 100644 --- a/testcrypt.c +++ b/testcrypt.c @@ -495,14 +495,19 @@ static void return_boolean(strbuf *out, bool b) strbuf_catf(out, "%s\n", b ? "true" : "false"); } -static void return_val_string_asciz(strbuf *out, char *s) +static void return_val_string_asciz_const(strbuf *out, const char *s) { strbuf *sb = strbuf_new(); put_data(sb, s, strlen(s)); - sfree(s); return_val_string(out, sb); } +static void return_val_string_asciz(strbuf *out, char *s) +{ + return_val_string_asciz_const(out, s); + sfree(s); +} + #define NULLABLE_RETURN_WRAPPER(type_name, c_type) \ static void return_opt_##type_name(strbuf *out, c_type ptr) \ { \ From 7ccc368a57f7364e728f57a512af87264a884ef3 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Thu, 9 Jan 2020 02:37:58 +0000 Subject: [PATCH 13/62] New test script 'agenttest.py' for testing Pageant. Well, actually, two new test programs. agenttest.py is the actual test; it depends on agenttestgen.py which generates a collection of test private keys, using the newly exposed testcrypt interface to our key generation code. In this commit I've also factored out some Python SSH marshalling code from cryptsuite, and moved it into a module ssh.py which the agent tests can reuse. (cherry picked from commit 8c7b0a787f6dbe29cbb40f68f59b558c0003e3e5) --- test/agenttest.py | 252 ++++++++++++++++++++++++++++++++++++++++++ test/agenttestdata.py | 14 +++ test/agenttestgen.py | 89 +++++++++++++++ test/cryptsuite.py | 27 +---- test/ssh.py | 98 ++++++++++++++++ 5 files changed, 454 insertions(+), 26 deletions(-) create mode 100755 test/agenttest.py create mode 100644 test/agenttestdata.py create mode 100755 test/agenttestgen.py create mode 100644 test/ssh.py diff --git a/test/agenttest.py b/test/agenttest.py new file mode 100755 index 00000000..9e44d556 --- /dev/null +++ b/test/agenttest.py @@ -0,0 +1,252 @@ +#!/usr/bin/python3 + +import sys +import os +import socket +import base64 +import itertools +import collections + +from ssh import * +import agenttestdata + +test_session_id = b'Test16ByteSessId' +assert len(test_session_id) == 16 +test_message_to_sign = b'test message to sign' + +TestSig2 = collections.namedtuple("TestSig2", "flags sig") + +class Key2(collections.namedtuple("Key2", "comment public sigs openssh")): + def public_only(self): + return Key2(self.comment, self.public, None, None) + + def Add(self): + alg = ssh_decode_string(self.public) + msg = (ssh_byte(SSH2_AGENTC_ADD_IDENTITY) + + ssh_string(alg) + + self.openssh + + ssh_string(self.comment)) + return agent_query(msg) + + verb = "sign" + def Use(self, flags): + msg = (ssh_byte(SSH2_AGENTC_SIGN_REQUEST) + + ssh_string(self.public) + + ssh_string(test_message_to_sign)) + if flags is not None: + msg += ssh_uint32(flags) + rsp = agent_query(msg) + t, rsp = ssh_decode_byte(rsp, True) + assert t == SSH2_AGENT_SIGN_RESPONSE + sig, rsp = ssh_decode_string(rsp, True) + assert len(rsp) == 0 + return sig + + def Del(self): + msg = (ssh_byte(SSH2_AGENTC_REMOVE_IDENTITY) + + ssh_string(self.public)) + return agent_query(msg) + + @staticmethod + def DelAll(): + msg = (ssh_byte(SSH2_AGENTC_REMOVE_ALL_IDENTITIES)) + return agent_query(msg) + + @staticmethod + def List(): + msg = (ssh_byte(SSH2_AGENTC_REQUEST_IDENTITIES)) + rsp = agent_query(msg) + t, rsp = ssh_decode_byte(rsp, True) + assert t == SSH2_AGENT_IDENTITIES_ANSWER + nk, rsp = ssh_decode_uint32(rsp, True) + keylist = [] + for _ in range(nk): + p, rsp = ssh_decode_string(rsp, True) + c, rsp = ssh_decode_string(rsp, True) + keylist.append(Key2(c, p, None, None)) + assert len(rsp) == 0 + return keylist + + @classmethod + def make_examples(cls): + cls.examples = agenttestdata.key2examples(cls, TestSig2) + + def iter_testsigs(self): + for testsig in self.sigs: + if testsig.flags == 0: + yield testsig._replace(flags=None) + yield testsig + + def iter_tests(self): + for testsig in self.iter_testsigs(): + yield ([testsig.flags], + " (flags={})".format(testsig.flags), + testsig.sig) + +class Key1(collections.namedtuple( + "Key1", "comment public challenge response private")): + def public_only(self): + return Key1(self.comment, self.public, None, None, None) + + def Add(self): + msg = (ssh_byte(SSH1_AGENTC_ADD_RSA_IDENTITY) + + self.private + + ssh_string(self.comment)) + return agent_query(msg) + + verb = "decrypt" + def Use(self, challenge): + msg = (ssh_byte(SSH1_AGENTC_RSA_CHALLENGE) + + self.public + + ssh1_mpint(challenge) + + test_session_id + + ssh_uint32(1)) + rsp = agent_query(msg) + t, rsp = ssh_decode_byte(rsp, True) + assert t == SSH1_AGENT_RSA_RESPONSE + assert len(rsp) == 16 + return rsp + + def Del(self): + msg = (ssh_byte(SSH1_AGENTC_REMOVE_RSA_IDENTITY) + + self.public) + return agent_query(msg) + + @staticmethod + def DelAll(): + msg = (ssh_byte(SSH1_AGENTC_REMOVE_ALL_RSA_IDENTITIES)) + return agent_query(msg) + + @staticmethod + def List(): + msg = (ssh_byte(SSH1_AGENTC_REQUEST_RSA_IDENTITIES)) + rsp = agent_query(msg) + t, rsp = ssh_decode_byte(rsp, True) + assert t == SSH1_AGENT_RSA_IDENTITIES_ANSWER + nk, rsp = ssh_decode_uint32(rsp, True) + keylist = [] + for _ in range(nk): + b, rsp = ssh_decode_uint32(rsp, True) + e, rsp = ssh1_get_mpint(rsp, True) + m, rsp = ssh1_get_mpint(rsp, True) + c, rsp = ssh_decode_string(rsp, True) + keylist.append(Key1(c, ssh_uint32(b)+e+m, None, None, None)) + assert len(rsp) == 0 + return keylist + + @classmethod + def make_examples(cls): + cls.examples = agenttestdata.key1examples(cls) + + def iter_tests(self): + yield [self.challenge], "", self.response + +def agent_query(msg): + msg = ssh_string(msg) + s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) + s.connect(os.environ["SSH_AUTH_SOCK"]) + s.send(msg) + length = ssh_decode_uint32(s.recv(4)) + assert length < AGENT_MAX_MSGLEN + return s.recv(length) + +def enumerate_bits(iterable): + return ((1<>1) + diff = new ^ old + assert diff != 0 and (diff & (diff-1)) == 0 + yield old, new, diff + old = new + assert old == 0 + +class TestRunner: + def __init__(self): + self.ok = True + + @staticmethod + def fmt_response(response): + return "'{}'".format( + base64.encodebytes(response).decode("ASCII").replace("\n","")) + + @staticmethod + def fmt_keylist(keys): + return "{{{}}}".format( + ",".join(key.comment.decode("ASCII") for key in sorted(keys))) + + def expect_success(self, text, response): + if response == ssh_byte(SSH_AGENT_SUCCESS): + print(text, "=> success") + elif response == ssh_byte(SSH_AGENT_FAILURE): + print("FAIL!", text, "=> failure") + self.ok = False + else: + print("FAIL!", text, "=>", self.fmt_response(response)) + self.ok = False + + def check_keylist(self, K, expected_keys): + keys = K.List() + print("list keys =>", self.fmt_keylist(keys)) + if set(keys) != set(expected_keys): + print("FAIL! Should have been", self.fmt_keylist(expected_keys)) + self.ok = False + + def gray_code_test(self, K): + bks = list(enumerate_bits(K.examples)) + + self.check_keylist(K, {}) + + for old, new, diff in gray_code(len(K.examples)): + bit, key = next((bit, key) for bit, key in bks if diff & bit) + + if new & bit: + self.expect_success("insert " + key.comment.decode("ASCII"), + key.Add()) + else: + self.expect_success("delete " + key.comment.decode("ASCII"), + key.Del()) + + self.check_keylist(K, [key.public_only() for bit, key in bks + if new & bit]) + + def sign_test(self, K): + for key in K.examples: + for params, message, expected_answer in key.iter_tests(): + key.Add() + actual_answer = key.Use(*params) + key.Del() + record = "{} with {}{}".format( + K.verb, key.comment.decode("ASCII"), message) + if actual_answer == expected_answer: + print(record, "=> success") + else: + print("FAIL!", record, "=> {} but expected {}".format( + self.fmt_response(actual_answer), + self.fmt_response(expected_answer))) + self.ok = False + + def run(self): + self.expect_success("init: delete all ssh2 keys", Key2.DelAll()) + + for K in [Key2, Key1]: + self.gray_code_test(K) + self.sign_test(K) + + # TODO: negative tests of all kinds. + +def main(): + Key2.make_examples() + Key1.make_examples() + + tr = TestRunner() + tr.run() + if tr.ok: + print("Test run passed") + else: + sys.exit("Test run failed!") + +if __name__ == "__main__": + main() diff --git a/test/agenttestdata.py b/test/agenttestdata.py new file mode 100644 index 00000000..596c069b --- /dev/null +++ b/test/agenttestdata.py @@ -0,0 +1,14 @@ +# DO NOT EDIT DIRECTLY! Autogenerated by agenttestgen.py +# +# To regenerate, run +# python3 agenttestgen.py > agenttestdata.py +# +# agenttestgen.py depends on the testcrypt system, so you must also +# have built testcrypt in the parent directory, or else set +# PUTTY_TESTCRYPT to point at a working implementation of it. + + +def key2examples(Key2, TestSig2): + return [Key2(comment=b'RSA-1024', public=b'\x00\x00\x00\x07ssh-rsa\x00\x00\x00\x01%\x00\x00\x00\x81\x00\x87B\x16\x99\x8c96\x92\xe7\x00-\xc5\xf0%\x13:\xe5a?_\x14\xb5\x15{%\xed\xd4\rB\x98\x02~\xb0\xfdWc\xa6\x8fSz\xa7\xfd\x94\xe1\xcegx\xe3\x14\xba\x87A4\xef\xb0\x056\x9c\x80r\x18\xd7Q\xb69\xed\x9a5\xba\x8b\xf8\xee\x84F\xceD\xfa\xccn\xd6\x9ba8\x8f\xb5\x9dz\x0b\xf1\xa3\xe9vH\x1dr\r[x\xbb\xd9\xd6\xf3\xcb~W\x8fYu\xd5|G)\xa9\xa8_\x91A\x1f\xef\x80\x83\xb3jp)\xef\xe8\x05', sigs=[TestSig2(flags=0, sig=b'\x00\x00\x00\x07ssh-rsa\x00\x00\x00\x801\xaa\xb4t\xe10\x83Q\xe4\x18\x84\x1e\xdeN$\xde;\t\xf9\xae"H\r>\xa9\x91B"\xfd\x01\\19\xee*\xb9\xc1\x8a\x1b*:\xc3\t\x91\x85\xae\x7f\xf8\x84\x08\xbd\x89P\xa9\xdb\t\x8fc\x95\xa4\xcb\xda\x19<\x14\xc4\x1a|\n\xef)\xf4\xc8\xfb\xc1\x04"\xf6\x8a9\xac\xec\xa6x>\xd3-\xb1\xf7\x1e\x04\x8a\xd4k\xcb\x12\xf9\xc1\xaa\x1aV\xc3\xb8\xdd\xf8\x01\x10Z\xdd\x8c\xcd\x12w\x83pJOr\xb8\xed\x84\xa5\xf5&\r:\xd7H'), TestSig2(flags=2, sig=b"\x00\x00\x00\x0crsa-sha2-256\x00\x00\x00\x80J)H\xfe\x18t\xc8\xa9$\x07*\xc9\x16\xb3\\\x8cK\x7f\xdd\xd8\xb0g\xed\x10o\xac\xe8\xd3\xefQj\xe2\x9fi\x13\x9b\x93\x07`}\x12\x9f\xc1Y\x19{\xb8\xc0\x8c\xe6\x03\xfd\x8d\xc1\xfat\xf0T\x055\x02r:AOM\x18x\xb6:\xb7g\xe5k\x12$\xabX)\xf8\xe9\x12\xa2\x04\xff\xfa^\xc7 G\x9c7\x92\x03>^\x00,\x1e\x063\x16\x9b\xd4.'\x01\xa4Lv\xd2\xae\xf0\xc0\xed\x8d\xf6'aj\x1aq`\xc3\x85\x08\xc2\x8a"), TestSig2(flags=4, sig=b'\x00\x00\x00\x0crsa-sha2-512\x00\x00\x00\x80;k\xd5\xf4\xe8\xec\xeb\x8b\xe13L}\x96\x918?\xd9\x90\t\x9dN\xec+|<\xc1\xc6\x10\xf6]y\x8c\xf9a\xd5\x07c:\x90\x7fzBQ\xe49\x87s\x1d\x81Kz\xe9\xee\n\xf3|\xee6\x84A\xd0\xec\xc0\xf9h\xc5$\x13\xd8r\xa2j;\xb6$?*\xd59\xcb\xdf\x85\x19U\x1f\x10\xb4\xd8C\xbb"`\x8a)\x0fy`|\xd7\xa3\xec`tw\x8a>(\x0fj\x08\xbc\x92\xd8K\xb0qP\xbe\xd9\xaf\x91\x16\xd7\x9a\x9a\r\x9d\xe5')], openssh=b'\x00\x00\x00\x81\x00\x87B\x16\x99\x8c96\x92\xe7\x00-\xc5\xf0%\x13:\xe5a?_\x14\xb5\x15{%\xed\xd4\rB\x98\x02~\xb0\xfdWc\xa6\x8fSz\xa7\xfd\x94\xe1\xcegx\xe3\x14\xba\x87A4\xef\xb0\x056\x9c\x80r\x18\xd7Q\xb69\xed\x9a5\xba\x8b\xf8\xee\x84F\xceD\xfa\xccn\xd6\x9ba8\x8f\xb5\x9dz\x0b\xf1\xa3\xe9vH\x1dr\r[x\xbb\xd9\xd6\xf3\xcb~W\x8fYu\xd5|G)\xa9\xa8_\x91A\x1f\xef\x80\x83\xb3jp)\xef\xe8\x05\x00\x00\x00\x01%\x00\x00\x00\x80 \xe6\x8f\xe0)\x06\xffo\xd7S\x12\r\x8dp\xcdS\x83\xe78\xed\x9d@\xcd\xdf\xafG\xb0\x1e\xe6\xafZ\x8d\x84\xffZr/n\xf8\xa1Ku\x08\x89\xf3\xef\xa7\xc8\x88\x80f\x16\xc7\xaf\xec\x8b\xa5\x80\x03\x91_\xfd\x06\t_\xc5\xbf\xc1\xcb\xe3\r12k\xaeY\xe4RA\xab\xf1\xba\x8a\x99\xcf\xaf\x06\xf1\x82\xc6\x9d\xd5g[\xb2\xfb\xd8\xbc\x9b]\xb9l\t\xb2\x0b\xc9\x98JK\xfe\x8a\xd6\xfc[\xd19\xe7N|\xa6m\x9ei\xce&\xc6\xfcM\x00\x00\x00A\x00\x95\xc13\x16\xd2O*\xbc8\xc6{D\xb2\xe5\x85\x9ao\xf9\xfc.\xea\xe7\x9d\xca4}\x9c7\xca\xe1\x1e\xdb`o\x88w\x0c\xa3\xba\xde\nF\xb7\xf2x}\xd9\x00\x00\x00A\x00\xa3h\x11\x1a\x99\xc6\xa4\x0eAj\x93\xff\xa0&D(\x05#MoE\xdb\x8d\xf4\x99\xc8\xd2\xffv\xf1\x90C~\xe4\xed\xce\x1f\x85\x9f\x92\xce\xacMR\xd7\x0c\xb9z\x87\xea\xe97/\x97\xbd\x19q:TLB\xb7$\r'), Key2(comment=b'DSA-1024', public=b'\x00\x00\x00\x07ssh-dss\x00\x00\x00\x81\x00\xbc\x0c\xb0L\xfc<\x03TyZQ\xe1\xef\xd4\xd5\xe4\xa2\xb3\xaf\x14t\x0f\\,!E\xdbf\x01\x9e\x95\xddr\xeb\xab\xae;\xc1\xe3\x0c\xe9\xd9\x15\xc2\xa9\xc3g\x04\xa5\xf1\x965\xf1\x81\x9dS\x9c\x83en\x93\x11\xe0p\n)\xdaZ\x17y\xff\xf2\xbf\x9b[;"E1\xf0\xde\xbd\xe1;\x9a6Xnc\x8f\xd3\x1dg\xd1\x80\xa9\x8em\x86t\xc8\xa9!\xcd\xb3\xe4mx\xd5\x93%R\xbb9u\xd2\x99p\xe2\xbe\xf3\xfb%\xebd\xc4\x86\xe3\x00\x00\x00\x15\x00\xec^\x98\x84x\xc1\xa8`\xcfB\xc7\x1e\xf0\x8d\xd3\x89\xa3\xa8\xec\xc7\x00\x00\x00\x80Q\xecf\xfd\xcc\x9a+\xcclxu\x1b\x0b\xd7\xfd\xedDP\xd1\x82~H\x0eqn\x1e\xed\xd8\xad\xe9\xe3\xf8!\x1d\\\xb4\xde\xc1e\xd7\xc0(\x1dpQ\xee\xad\xeez\xe1\xb3\xa4\x12d\x92\x1a\x89\x82u\x99\xa3$\x85\x9c\xb840\xe7\xd6&O\x85~\xd6\xac\x1eq\xa1\x06\xa2\xd1ro\xd0}>\xc0O\xaf\x8a\xf8@B\r7\xf6\x89\xda\xd0\xb9\x0e\xb6\xae\xcdh\x1a\x86%\xdcN\x8cE\xd8\xcd(\x19\xa6y\x9a\xc0\xc2\xd6;\xc3\xc9{\x104\x00\x00\x00\x80`\xc1\xe8\x18\xe4\xd0\x16\xf9[l\xce\\L*\x19\x14"20K\xc6\x18*\xe5\x91\x80\xbf+\xec\xfe\xd3D\xf7\xa6\xf9Y0x#sl\x88BU\x7f\x1c\r\xb3\x08EL\x86\xa2\x8c\x81\x15pD\xac\x9c\xa3\x8e\x02\x89\xb9\xb6\x9f\xaa\xd0\xc8\x89o\x81Qm\x18f0\\\x92\x1f\xbf\xa1\x8d8\x8a\xb1\xec\xb8G\xbd9b\x8d\x7f\x9bk\xb9x\xe5\xde\xce/\'f\xc4\x8bmX\xd9 \xb4\xd5\xe8\xd1\xe1\xc8\xeb\xe7\xbc2LG\x90]\\%\x9f', sigs=[TestSig2(flags=0, sig=b"\x00\x00\x00\x07ssh-dss\x00\x00\x00(\xeb\xf2\xb0 2(\x93a\xfc\x0f\xad\x1al\xd5\xd0n\xd5\x10\x9d\\G\x18]?\xf4h5D\x12WL\xe6#\xa0\x89'\x17\xd3;\xb7")], openssh=b'\x00\x00\x00\x81\x00\xbc\x0c\xb0L\xfc<\x03TyZQ\xe1\xef\xd4\xd5\xe4\xa2\xb3\xaf\x14t\x0f\\,!E\xdbf\x01\x9e\x95\xddr\xeb\xab\xae;\xc1\xe3\x0c\xe9\xd9\x15\xc2\xa9\xc3g\x04\xa5\xf1\x965\xf1\x81\x9dS\x9c\x83en\x93\x11\xe0p\n)\xdaZ\x17y\xff\xf2\xbf\x9b[;"E1\xf0\xde\xbd\xe1;\x9a6Xnc\x8f\xd3\x1dg\xd1\x80\xa9\x8em\x86t\xc8\xa9!\xcd\xb3\xe4mx\xd5\x93%R\xbb9u\xd2\x99p\xe2\xbe\xf3\xfb%\xebd\xc4\x86\xe3\x00\x00\x00\x15\x00\xec^\x98\x84x\xc1\xa8`\xcfB\xc7\x1e\xf0\x8d\xd3\x89\xa3\xa8\xec\xc7\x00\x00\x00\x80Q\xecf\xfd\xcc\x9a+\xcclxu\x1b\x0b\xd7\xfd\xedDP\xd1\x82~H\x0eqn\x1e\xed\xd8\xad\xe9\xe3\xf8!\x1d\\\xb4\xde\xc1e\xd7\xc0(\x1dpQ\xee\xad\xeez\xe1\xb3\xa4\x12d\x92\x1a\x89\x82u\x99\xa3$\x85\x9c\xb840\xe7\xd6&O\x85~\xd6\xac\x1eq\xa1\x06\xa2\xd1ro\xd0}>\xc0O\xaf\x8a\xf8@B\r7\xf6\x89\xda\xd0\xb9\x0e\xb6\xae\xcdh\x1a\x86%\xdcN\x8cE\xd8\xcd(\x19\xa6y\x9a\xc0\xc2\xd6;\xc3\xc9{\x104\x00\x00\x00\x80`\xc1\xe8\x18\xe4\xd0\x16\xf9[l\xce\\L*\x19\x14"20K\xc6\x18*\xe5\x91\x80\xbf+\xec\xfe\xd3D\xf7\xa6\xf9Y0x#sl\x88BU\x7f\x1c\r\xb3\x08EL\x86\xa2\x8c\x81\x15pD\xac\x9c\xa3\x8e\x02\x89\xb9\xb6\x9f\xaa\xd0\xc8\x89o\x81Qm\x18f0\\\x92\x1f\xbf\xa1\x8d8\x8a\xb1\xec\xb8G\xbd9b\x8d\x7f\x9bk\xb9x\xe5\xde\xce/\'f\xc4\x8bmX\xd9 \xb4\xd5\xe8\xd1\xe1\xc8\xeb\xe7\xbc2LG\x90]\\%\x9f\x00\x00\x00\x15\x00\x85mio\xa4\xa2\xcc\xadnC\x94\x84I*;\xf40\xc9\xd7\xa9'), Key2(comment=b'ECDSA-p256', public=b'\x00\x00\x00\x13ecdsa-sha2-nistp256\x00\x00\x00\x08nistp256\x00\x00\x00A\x04D\x01\'\xac\xa9\xeaJ#\x1e\x80\x1e\xd2R"xq\xb2h\x128c\x11\xf5\xe8\x19,;\x1d\xcf\xf4h\x8c\xaeQ\xee\x15\xc2\xdb\xc77\x80\xc4\xc2\x15\x1d0s\xe1\xbfa\xd9}pz\xc6af4d\xbd\xc6\xc6\x1as', sigs=[TestSig2(flags=0, sig=b'\x00\x00\x00\x13ecdsa-sha2-nistp256\x00\x00\x00J\x00\x00\x00!\x00\xe0\x0b\xa1\x01\xc1\xc6A\x0c\x0c\x02\x9f\xc2B\x18G=\xfa+\xde\x8a/)x\xea\xc0R\xc0\x92\xe9\t?\xc7\x00\x00\x00!\x00\xa3\xcd^!\xa5\x0f"\xcd\x9c\x82\xe0\x01\x9c\xf1\xcaa\x83\x83\x848\xe4\xfb\xd9p]\xe1\xcc<\xdb\xde\x99\xe8')], openssh=b'\x00\x00\x00\x08nistp256\x00\x00\x00A\x04D\x01\'\xac\xa9\xeaJ#\x1e\x80\x1e\xd2R"xq\xb2h\x128c\x11\xf5\xe8\x19,;\x1d\xcf\xf4h\x8c\xaeQ\xee\x15\xc2\xdb\xc77\x80\xc4\xc2\x15\x1d0s\xe1\xbfa\xd9}pz\xc6af4d\xbd\xc6\xc6\x1as\x00\x00\x00!\x00\xc3\x8b\xc3A\xde\xfd\xd4\xcb\xf7\x9c\xa0\xc7L\xd1\xb0\xfe\x8e\xf2\xf6o\xe4"\x88K\x15p0\xbc\x0b\x19\xa7\xad'), Key2(comment=b'Ed25519', public=b'\x00\x00\x00\x0bssh-ed25519\x00\x00\x00 \xd6\x9aC\xb8\xa4\x1b\x95\x8a\x97\x9a\x9d\x95\xaa5\xd5\x9b\xc3B\xd2\xd1*\x85\xde.E"\x1c\xe9\xef\xc0\x06\xdb', sigs=[TestSig2(flags=0, sig=b'\x00\x00\x00\x0bssh-ed25519\x00\x00\x00@\xda\xcbw6\xff\xb9\xc1)\x1e\xfa\xef/s!u\xe1\xc0%\xd9-;\x97<\xbc2o\x1a\xef\x17\xd3\x8dvU\xbf\x8d,\xed\xe8S\xe8\xc27\xdb\x1d\xe7\xda\\\xce\xdc\x00\xad\x97BpC\x9a\xec\xd3\r1\x9f\xc0n\x0b')], openssh=b'\x00\x00\x00 \xd6\x9aC\xb8\xa4\x1b\x95\x8a\x97\x9a\x9d\x95\xaa5\xd5\x9b\xc3B\xd2\xd1*\x85\xde.E"\x1c\xe9\xef\xc0\x06\xdb\x00\x00\x00@\rV\xff\xf36D\xe3\xb2\xcc\xda\xa1\x11\x9d\xa2\xa7\xc0\'}C\xcb"\xf0x\rlL\xfc\xcc\x99\x10\x91\xc8\xd6\x9aC\xb8\xa4\x1b\x95\x8a\x97\x9a\x9d\x95\xaa5\xd5\x9b\xc3B\xd2\xd1*\x85\xde.E"\x1c\xe9\xef\xc0\x06\xdb')] +def key1examples(Key1): + return [Key1(comment=b'RSA-1024a', public=b"\x00\x00\x04\x00\x00\x06%\x04\x00\x98\x8d\xac\xa1\xff\xd1\x05\xd4@\x93\x11\xfc\xd8\xb5\x8c\x18\xa8/\x9ePh\x06y,\xc1\xdd\xc2q\x90\xe0g\xebIgl\x12\xacs.\xc1\xd7\xd0,\x8d\xd4\xa4\xd1\x88F\x1dW\xa6\xb9\x808+0u`B\xa8\xd2z\x0c>}\xaeA\xa7\x945\x91\x0c\xd5@5s\xa8R\xc31\xc5\x8e'\xec6\x00\x98\xdd\x0b\x93\xa8\x8e\xe6\xa9\x19\xa2\xbaf\xd6\xa8@\x1b\x82\xf4\xf5j\xc4\x06\xdd\x08\x7f\xcce\xcdc%\xc4W\xd7k\xd2\xe3\xcf\xa2\xbaI=", challenge=105855771610781217219148893240371739231586890974005582821084910669621353003219053895226458126049169949952763904000891276244889049724351186264170655451406153989624471223276671179692313150356649135789040179224142632652879598695018927369451625737003799565133274183885945180682771324880529165922297762364545903323, response=b'U\xb2\xdf(\xb8\xc7\xf5\r\x16\xa0%O9l\xdb\xf0', private=b'\x00\x00\x04\x00\x04\x00\x98\x8d\xac\xa1\xff\xd1\x05\xd4@\x93\x11\xfc\xd8\xb5\x8c\x18\xa8/\x9ePh\x06y,\xc1\xdd\xc2q\x90\xe0g\xebIgl\x12\xacs.\xc1\xd7\xd0,\x8d\xd4\xa4\xd1\x88F\x1dW\xa6\xb9\x808+0u`B\xa8\xd2z\x0c>}\xaeA\xa7\x945\x91\x0c\xd5@5s\xa8R\xc31\xc5\x8e\'\xec6\x00\x98\xdd\x0b\x93\xa8\x8e\xe6\xa9\x19\xa2\xbaf\xd6\xa8@\x1b\x82\xf4\xf5j\xc4\x06\xdd\x08\x7f\xcce\xcdc%\xc4W\xd7k\xd2\xe3\xcf\xa2\xbaI=\x00\x06%\x04\x00\x94n+m0@\xfe\xc0\xad\x88--];\x04\xd9\xb8e\xae\xca\xc6\x14"\xdfp\x84\xbd09\xef\x19\x00\x9arv\xfdi\x84\xd3\x8c+\xed$nR[,\xbb\xf11N]\x07\x83\xacE\xb2\x9b\xb7\x9a\xcd\xc5\xde\x86\xe7\xf9O\xf9\xa0\xce\xca\x085\xe7\xb7En\x94\x1e;T)SH\xff\x85\xe5\x8d\x1f\xa6,\xc7\xffV\x80\xf2E=\x08\x8e\xe6\x9e\xb2}py\xad\xa55Z\xf4\xed\xc8^+tnIN)vE\xbd\x82\xb665\xdd\x01\xfb\x04d\xa4\xb80\x98l\xd0!l\x99[\x12\x1c\x85\xda\xa5\xd0#\xcc\x7f\x80\xcdE~\x8bF \xf1;\x9d\xfb\xc0\xa3\x93\xa2\xb89^\x91D\xb7\xd3\x18\xbdx\x93U\xc3{\xa4\t,F\xb5\xdd\xadk\xc9@\xd0$\xc6\x03\x02\x00\x9a\xa4\\\xb4\xe5\x05\xe0&\xb0\x06\xc3\x9dQ`\xe4w\xe3-6N "\xa2\x9a%\xf16T\x92%\x16\xf9\xfc\xb5\xc6\xc4\xbb\xa8\xf7?\xa7"\xa1\x9do\x10A3\x14\x12\x18Uj\x19k\x19\x93\x99\xc9\xab\xfa\xa7\x15\xcb\x02\x00\xfc\x8a\xdb\xcc2\x9d[\x1a\xd0\x12\x1c\xad7\xbdk\xaa\xc6Ql\xeb7;\x87f\x8fv\xafM\x8b\xa8\xaa\n40\x90)\xb8t\tBaU\xba<\xcb\xa1\x12\xad\xaad\xb3\x0e\xf4\xfc\x07\x13;\x1c\x17]P[|\x17'), Key1(comment=b'RSA-1024b', public=b'\x00\x00\x04\x00\x00\x06%\x04\x00\x97\xe4sJ\xf8i\x83\x9f\xe8k%\xc6\xb7\xcbm!\xd7\xdd\xd5!N\xad<8\x0e\x1f\xa15yV\xbcr\xec\x8c\xca\x94\xed\x0c\xdbDC\x9e\xe1\xf5\xe4_\xb6>\x19\xe0\xdf\xb1te\xc7n\x86\xf7\xd15\x9e\xfc\x81\x90V\x92\xae\x1cb\xcc\xde\x05k\x8eNIa\x87\x1a\x8aG\xdd\xc9\xc9K\xfe\xb3W\xb0%\xe2\x10bs\x18\xe2\x07I\xf8\x88P\x04i!\xa9\xdd5\x12\x12\xdbp\x06\x03\xbb\x0e(\x82\x0e\xe7\xe7r\x17\xdbN\x91\x141Q', challenge=106369452810277819728395930691403679528939443121481728310811020449278450935158409081846069415863931371431145424909167586350456375465223628112328681772147389155179853401808803792809242283837570604791348990555643874877574733757512944970974196016255159184273573080435875970788050018918135917906633435695051286334, response=b'U\xb2\xdf(\xb8\xc7\xf5\r\x16\xa0%O9l\xdb\xf0', private=b'\x00\x00\x04\x00\x04\x00\x97\xe4sJ\xf8i\x83\x9f\xe8k%\xc6\xb7\xcbm!\xd7\xdd\xd5!N\xad<8\x0e\x1f\xa15yV\xbcr\xec\x8c\xca\x94\xed\x0c\xdbDC\x9e\xe1\xf5\xe4_\xb6>\x19\xe0\xdf\xb1te\xc7n\x86\xf7\xd15\x9e\xfc\x81\x90V\x92\xae\x1cb\xcc\xde\x05k\x8eNIa\x87\x1a\x8aG\xdd\xc9\xc9K\xfe\xb3W\xb0%\xe2\x10bs\x18\xe2\x07I\xf8\x88P\x04i!\xa9\xdd5\x12\x12\xdbp\x06\x03\xbb\x0e(\x82\x0e\xe7\xe7r\x17\xdbN\x91\x141Q\x00\x06%\x03\xfe1C,O\xaa\x83\x15\xee\xac>m\x1d\xda\xbe\x84BS\xd9>4P\xde=\x0bB\xd9\xd3kI\xf2\x9d\xfb\xc2W,\xf2\x07\xb1$\x84\xd7\xa9&\xb0\x9d\x18\x1fn\x16;\x18\x1d\xe0\x8f\xb6M\\4\xb2\x8d\xee_\xbbP\xe7 j!\xc7W\xcb\xc9\x19\nM\x90\xfe4\xe5U\xef[\xdc&A\xde\xd9\x84\x02\xdek\xec\xaf\xb4\xd0I\xaaR\xa6\xc1\x8b\xbc\x13\xf1?,\xc6{\n\x02p\xa7\'\xa1\xb9\xf8\x1f\xeb\x99\xe2\xcf\xc4%"+Mu\x9d\x02\x00\xc4\x89W\x05\x8f\xff\xabX6\x8f\x9fQ\x19\xb8\xc2\xc2Y|\xa90g\xa9\xa7\xa9\x17 \xeb\xbbSMd\xf4YW\xa7\x93\xfcn\xc3AI\x04tK\x1a\xd74`\xec]+\xd8\x91`W@\xc7\xa6G\x82\x99\xac\x8c\x9b\x02\x00\xacs\x1e6\xa5\x10\xb2\xd1\xc9|\x87\x15\xb6\xd9*\x05O\x9e\x95\xec\x1f\xac\xbc/2\xc1\xdb\xa7\x97w@\xfe?d\xb2|\xd4\x96\x02\xc8y\xdf; \x89\x0b'), Key1(comment=b'RSA-768d', public=b'\x00\x00\x03\x00\x00\x06%\x03\x00\x9e4w\xb6C\x1c\xb1\xcdV\x96\t\x14\x04T\xb5\xca\x0ct?a8\xfd-\xb1l\x83/\xc3\x95\x97\x8b \xcdZW\x15\x87G\xa8\x1d\xea(\x1d\x03V\xe8\xe8/M.\xe6\xd6\x8d,\xf3>$"R\xcciYwp*\xc7z\x0c\xc3/k\x87\xc1{4\x1bw\xf1\x00\xda~\x84\x0e\xb0)\'\x84\x9e<\xd1\x19\x18\x81\xcb\xffo', challenge=600986133602165113984107876330614577281000470334319933978738264340033662158902949574073710382789301043986608302703563989903269508211841666685953915670687064469873711668788199922957307325206107166514327102609966835942325119838536897, response=b'U\xb2\xdf(\xb8\xc7\xf5\r\x16\xa0%O9l\xdb\xf0', private=b'\x00\x00\x03\x00\x03\x00\x9e4w\xb6C\x1c\xb1\xcdV\x96\t\x14\x04T\xb5\xca\x0ct?a8\xfd-\xb1l\x83/\xc3\x95\x97\x8b \xcdZW\x15\x87G\xa8\x1d\xea(\x1d\x03V\xe8\xe8/M.\xe6\xd6\x8d,\xf3>$"R\xcciYwp*\xc7z\x0c\xc3/k\x87\xc1{4\x1bw\xf1\x00\xda~\x84\x0e\xb0)\'\x84\x9e<\xd1\x19\x18\x81\xcb\xffo\x00\x06%\x02\xfe"4\xdb\x9d\x07\x97\x80c\xbf\xb1\xbc\xc6\x0e\xc65$\xc4l)a!\x14%\x8e%L\xcc\x0e\x9c\xe2~\xf2U\xea\x04\xfd\xbcb\x856\xe6\x856\xb4\x9d+px\x97\x0c\x17\xde\x93\xd8zOAO\xea\xab\xa2p\x14\x87\xf5\x1c\x00\xb2\xa3A\x08\x14\xe9v\xb8\xd2\xbf\x03C\xf2\xa3\xfeV/BZ\x11\x82#\xff)\xf8\x93\x0f\xf0m\x01\x80\x85\xde\xf4\x1b\xd2Pu\xf3\xd8\xf8Z\xabZ\x92q\xef\xab\x06\x85\xcd\xc3\x85\xd6?j\xd4\xaa\x96l\xeeRZ\xab8\x05\x84xT\xdd\x15j\xea9O_\xf4`R\x01\x80\xc4\xeb\x12\x92\xdc\x05\\b\x8c\xec\x11\x10\xc5\xaa\xdf\x97\x1eD\x92\x06_\xb3\xc28C\xceH\xa7\x8a\xf2F\xe6+\x01\xbf\xad\x81\x99&2\xcc\xff\xa2\xaad\x04\x08\x8d\x01\x80\xcd\xab\xe5\xdeE^a-\t$\xa4a\xd4h8\xe4>\xe1d\xcc0n\xe3\xee\xc5\xe7\xd4\xa59\x8f\x9f\xb2\x1d\n\x00h\x14\xad\xcdq\x89UTPu\x9e>\xeb')] diff --git a/test/agenttestgen.py b/test/agenttestgen.py new file mode 100755 index 00000000..bc2068bc --- /dev/null +++ b/test/agenttestgen.py @@ -0,0 +1,89 @@ +#!/usr/bin/env python3 + +def generate(): + import hashlib + + print("""\ +# DO NOT EDIT DIRECTLY! Autogenerated by agenttestgen.py +# +# To regenerate, run +# python3 agenttestgen.py > agenttestdata.py +# +# agenttestgen.py depends on the testcrypt system, so you must also +# have built testcrypt in the parent directory, or else set +# PUTTY_TESTCRYPT to point at a working implementation of it. + +""") + + from testcrypt import (rsa_generate, dsa_generate, ecdsa_generate, + eddsa_generate, random_clear, random_queue, + ssh_key_public_blob, ssh_key_openssh_blob, + ssh_key_sign, rsa1_generate, rsa_ssh1_encrypt, + rsa_ssh1_public_blob, rsa_ssh1_private_blob_agent, + mp_from_bytes_be) + from agenttest import (Key2, TestSig2, test_message_to_sign, + Key1, test_session_id) + import ssh + + keygen2 = [ + ('RSA-1024', lambda: rsa_generate(1024), + (ssh.SSH_AGENT_RSA_SHA2_256, ssh.SSH_AGENT_RSA_SHA2_512)), + ('DSA-1024', lambda: dsa_generate(1024)), + ('ECDSA-p256', lambda: ecdsa_generate(256)), + ('Ed25519', lambda: eddsa_generate(256)), + ] + + keys2 = [] + + for record in keygen2: + if len(record) == 2: + record += ((),) + comment, genfn, flaglist = record + flaglist = (0,) + flaglist + + random_clear() + random_queue(b''.join(hashlib.sha512('{}{:d}'.format(comment, j) + .encode('ASCII')).digest() + for j in range(1000))) + key = genfn() + sigs = [TestSig2(flags, ssh_key_sign(key, test_message_to_sign, flags)) + for flags in flaglist] + + keys2.append(Key2(comment.encode("ASCII"), + ssh_key_public_blob(key), + sigs, + ssh_key_openssh_blob(key))) + + print("def key2examples(Key2, TestSig2):\n return {!r}".format(keys2)) + + keygen1 = [ + ('RSA-1024a', 1024), + ('RSA-1024b', 1024), + ('RSA-768c', 768), + ('RSA-768d', 768), + ] + + keys1 = [] + + for comment, bits in keygen1: + random_clear() + random_queue(b''.join(hashlib.sha512('{}{:d}'.format(comment, j) + .encode('ASCII')).digest() + for j in range(1000))) + key = rsa1_generate(bits) + preimage = b'Test128BitRSA1ChallengeCleartext' + assert len(preimage) == 32 + challenge_bytes = rsa_ssh1_encrypt(preimage, key) + assert len(challenge_bytes) > 0 + challenge = int(mp_from_bytes_be(challenge_bytes)) + response = hashlib.md5(preimage + test_session_id).digest() + + keys1.append(Key1(comment.encode("ASCII"), + rsa_ssh1_public_blob(key, 'exponent_first'), + challenge, response, + rsa_ssh1_private_blob_agent(key))) + + print("def key1examples(Key1):\n return {!r}".format(keys1)) + +if __name__ == "__main__": + generate() diff --git a/test/cryptsuite.py b/test/cryptsuite.py index f002a109..f76c3ed2 100755 --- a/test/cryptsuite.py +++ b/test/cryptsuite.py @@ -15,41 +15,16 @@ except ImportError: from eccref import * from testcrypt import * +from ssh import * try: base64decode = base64.decodebytes except AttributeError: base64decode = base64.decodestring -def nbits(n): - # Mimic mp_get_nbits for ordinary Python integers. - assert 0 <= n - smax = next(s for s in itertools.count() if (n >> (1 << s)) == 0) - toret = 0 - for shift in reversed([1 << s for s in range(smax)]): - if n >> shift != 0: - n >>= shift - toret += shift - assert n <= 1 - if n == 1: - toret += 1 - return toret - def unhex(s): return binascii.unhexlify(s.replace(" ", "").replace("\n", "")) -def ssh_uint32(n): - return struct.pack(">L", n) -def ssh_string(s): - return ssh_uint32(len(s)) + s -def ssh1_mpint(x): - bits = nbits(x) - bytevals = [0xFF & (x >> (8*n)) for n in range((bits-1)//8, -1, -1)] - return struct.pack(">H" + "B" * len(bytevals), bits, *bytevals) -def ssh2_mpint(x): - bytevals = [0xFF & (x >> (8*n)) for n in range(nbits(x)//8, -1, -1)] - return struct.pack(">L" + "B" * len(bytevals), len(bytevals), *bytevals) - def rsa_bare(e, n): rsa = rsa_new() get_rsa_ssh1_pub(ssh_uint32(nbits(n)) + ssh1_mpint(e) + ssh1_mpint(n), diff --git a/test/ssh.py b/test/ssh.py new file mode 100644 index 00000000..2fe9f9a3 --- /dev/null +++ b/test/ssh.py @@ -0,0 +1,98 @@ +import struct +import itertools + +def nbits(n): + # Mimic mp_get_nbits for ordinary Python integers. + assert 0 <= n + smax = next(s for s in itertools.count() if (n >> (1 << s)) == 0) + toret = 0 + for shift in reversed([1 << s for s in range(smax)]): + if n >> shift != 0: + n >>= shift + toret += shift + assert n <= 1 + if n == 1: + toret += 1 + return toret + +def ssh_byte(n): + return struct.pack("B", n) + +def ssh_uint32(n): + return struct.pack(">L", n) + +def ssh_string(s): + return ssh_uint32(len(s)) + s + +def ssh1_mpint(x): + bits = nbits(x) + bytevals = [0xFF & (x >> (8*n)) for n in range((bits-1)//8, -1, -1)] + return struct.pack(">H" + "B" * len(bytevals), bits, *bytevals) + +def ssh2_mpint(x): + bytevals = [0xFF & (x >> (8*n)) for n in range(nbits(x)//8, -1, -1)] + return struct.pack(">L" + "B" * len(bytevals), len(bytevals), *bytevals) + +def decoder(fn): + def decode(s, return_rest = False): + item, length_consumed = fn(s) + if return_rest: + return item, s[length_consumed:] + else: + return item + return decode + +@decoder +def ssh_decode_byte(s): + return struct.unpack_from("B", s, 0)[0], 1 + +@decoder +def ssh_decode_uint32(s): + return struct.unpack_from(">L", s, 0)[0], 4 + +@decoder +def ssh_decode_string(s): + length = ssh_decode_uint32(s) + assert length + 4 <= len(s) + return s[4:length+4], length+4 + +@decoder +def ssh1_get_mpint(s): # returns it unconsumed, still in wire encoding + nbits = struct.unpack_from(">H", s, 0)[0] + nbytes = (nbits + 7) // 8 + assert nbytes + 2 <= len(s) + return s[:nbytes+2], nbytes+2 + +@decoder +def ssh1_decode_mpint(s): + nbits = struct.unpack_from(">H", s, 0)[0] + nbytes = (nbits + 7) // 8 + assert nbytes + 2 <= len(s) + data = s[2:nbytes+2] + v = 0 + for b in struct.unpack("B" * len(data), data): + v = (v << 8) | b + return v, nbytes+2 + +AGENT_MAX_MSGLEN = 262144 + +SSH1_AGENTC_REQUEST_RSA_IDENTITIES = 1 +SSH1_AGENT_RSA_IDENTITIES_ANSWER = 2 +SSH1_AGENTC_RSA_CHALLENGE = 3 +SSH1_AGENT_RSA_RESPONSE = 4 +SSH1_AGENTC_ADD_RSA_IDENTITY = 7 +SSH1_AGENTC_REMOVE_RSA_IDENTITY = 8 +SSH1_AGENTC_REMOVE_ALL_RSA_IDENTITIES = 9 +SSH_AGENT_FAILURE = 5 +SSH_AGENT_SUCCESS = 6 +SSH2_AGENTC_REQUEST_IDENTITIES = 11 +SSH2_AGENT_IDENTITIES_ANSWER = 12 +SSH2_AGENTC_SIGN_REQUEST = 13 +SSH2_AGENT_SIGN_RESPONSE = 14 +SSH2_AGENTC_ADD_IDENTITY = 17 +SSH2_AGENTC_REMOVE_IDENTITY = 18 +SSH2_AGENTC_REMOVE_ALL_IDENTITIES = 19 +SSH2_AGENTC_EXTENSION = 27 + +SSH_AGENT_RSA_SHA2_256 = 2 +SSH_AGENT_RSA_SHA2_512 = 4 From e1344d6ca72f7ce8e9de31f73e635efe24712dec Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 13 Jan 2020 19:02:55 +0000 Subject: [PATCH 14/62] Fix ldisc_send() assertion in terminal answerback. A user reports that if the ^E answerback string is configured to be empty, then causing the answerback to be sent fails the assertion in ldisc_send introduced in commit c269dd013. I thought I'd caught all of the remaining cases of this in commit 4634cd47f, but apparently not. (cherry picked from commit 43a63019f5bdc627170212ec5ed8c1333ad4727b) --- terminal.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/terminal.c b/terminal.c index ad54463b..db397e9c 100644 --- a/terminal.c +++ b/terminal.c @@ -3406,7 +3406,8 @@ static void term_out(Terminal *term) strbuf *buf = term_input_data_from_charset( term, DEFAULT_CODEPAGE, term->answerback, term->answerbacklen); - ldisc_send(term->ldisc, buf->s, buf->len, false); + if (buf->len) + ldisc_send(term->ldisc, buf->s, buf->len, false); strbuf_free(buf); } break; From ae84c959ac42131329c1eccc6e939eb0cfbb9be7 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 14 Jan 2020 06:39:32 +0000 Subject: [PATCH 15/62] PuTTYgen: permit and prefer 255 as bit count for ed25519. In setting up the ECC tests for cmdgen, I noticed that OpenSSH and PuTTYgen disagree on the bit length to put in a key fingerprint for an ed25519 key: we think 255, they think 256. On reflection, I think 255 is more accurate, which is why I bodged get_fp() in the test suite to ignore that difference when checking our key fingerprint against OpenSSH's. But having done that, it now seems silly that if you unnecessarily specify a bit count at ed25519 generation time, cmdgen will insist that it be 256! 255 is now permitted everywhere an ed25519 bit count is input. 256 is also still allowed for backwards compatibility but 255 is preferred by the error message if you give any other value. (cherry picked from commit 187cc8bfccaf9a3ddbe7b344adf5618ba524243e) --- cmdgen.c | 6 +++--- sshecc.c | 2 +- windows/winpgen.c | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/cmdgen.c b/cmdgen.c index 5c17ce2b..7ec6dbae 100644 --- a/cmdgen.c +++ b/cmdgen.c @@ -486,7 +486,7 @@ int main(int argc, char **argv) bits = 384; break; case ED25519: - bits = 256; + bits = 255; break; default: bits = DEFAULT_RSADSA_BITS; @@ -499,8 +499,8 @@ int main(int argc, char **argv) errs = true; } - if (keytype == ED25519 && (bits != 256)) { - fprintf(stderr, "puttygen: invalid bits for ED25519, choose 256\n"); + if (keytype == ED25519 && (bits != 255) && (bits != 256)) { + fprintf(stderr, "puttygen: invalid bits for ED25519, choose 255\n"); errs = true; } diff --git a/sshecc.c b/sshecc.c index dd9a7e3e..3005e288 100644 --- a/sshecc.c +++ b/sshecc.c @@ -1549,7 +1549,7 @@ bool ec_ed_alg_and_curve_by_bits( int bits, const struct ec_curve **curve, const ssh_keyalg **alg) { switch (bits) { - case 256: *alg = &ssh_ecdsa_ed25519; break; + case 255: case 256: *alg = &ssh_ecdsa_ed25519; break; default: return false; } *curve = ((struct ecsign_extra *)(*alg)->extra)->curve(); diff --git a/windows/winpgen.c b/windows/winpgen.c index 353caa02..efb9a392 100644 --- a/windows/winpgen.c +++ b/windows/winpgen.c @@ -365,7 +365,7 @@ static DWORD WINAPI generate_key_thread(void *param) ecdsa_generate(params->eckey, params->curve_bits, progress_update, &prog); else if (params->keytype == ED25519) - eddsa_generate(params->edkey, 256, progress_update, &prog); + eddsa_generate(params->edkey, 255, progress_update, &prog); else rsa_generate(params->key, params->key_bits, progress_update, &prog); From 2c66217af81f16675e611ed6a65bc92efc161bf2 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 21 Jan 2020 20:04:15 +0000 Subject: [PATCH 16/62] Fix undefined behaviour in safegrowarray. UBsan points out that if the input pointer is NULL, we'll pass it to memcpy, which is technically illegal by the C standard _even_ if the length you pass with it is zero. (cherry picked from commit 88d5948ead1226a0e364980d93d19fb9f5124f33) --- memory.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/memory.c b/memory.c index 43dd8666..97ae9401 100644 --- a/memory.c +++ b/memory.c @@ -121,9 +121,11 @@ void *safegrowarray(void *ptr, size_t *allocated, size_t eltsize, void *toret; if (secret) { toret = safemalloc(newsize, eltsize, 0); - memcpy(toret, ptr, oldsize * eltsize); - smemclr(ptr, oldsize * eltsize); - sfree(ptr); + if (oldsize) { + memcpy(toret, ptr, oldsize * eltsize); + smemclr(ptr, oldsize * eltsize); + sfree(ptr); + } } else { toret = saferealloc(ptr, newsize, eltsize); } From 34a0460f0561aa2376659775d5e1c3307a14ccde Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 21 Jan 2020 20:16:28 +0000 Subject: [PATCH 17/62] New functions to shrink a strbuf. These are better than my previous approach of just assigning to sb->len, because firstly they check by assertion that the new length is within range, and secondly they preserve the invariant that the byte stored in the buffer just after the length runs out is \0. Switched to using the new functions everywhere a grep could turn up opportunities. (cherry picked from commit 5891142aee5cab4aedd4a6be32e80fba806f3326) --- misc.h | 3 +++ pscp.c | 8 ++++---- scpserver.c | 6 +++--- ssh1login-server.c | 2 +- ssh1login.c | 2 +- ssh2kex-server.c | 2 +- ssh2transport.c | 6 +++--- ssh2userauth.c | 2 +- sshverstring.c | 3 +-- telnet.c | 4 ++-- terminal.c | 10 +++++----- testcrypt.c | 2 +- unix/gtkdlg.c | 2 +- unix/uxnet.c | 4 ++-- unix/uxstore.c | 2 +- utils.c | 14 ++++++++++++++ windows/winpgnt.c | 2 +- 17 files changed, 45 insertions(+), 29 deletions(-) diff --git a/misc.h b/misc.h index b9fb5721..7f53d4e0 100644 --- a/misc.h +++ b/misc.h @@ -73,9 +73,12 @@ strbuf *strbuf_new_nm(void); void strbuf_free(strbuf *buf); void *strbuf_append(strbuf *buf, size_t len); +void strbuf_shrink_to(strbuf *buf, size_t new_len); +void strbuf_shrink_by(strbuf *buf, size_t amount_to_remove); char *strbuf_to_str(strbuf *buf); /* does free buf, but you must free result */ void strbuf_catf(strbuf *buf, const char *fmt, ...); void strbuf_catfv(strbuf *buf, const char *fmt, va_list ap); +static inline void strbuf_clear(strbuf *buf) { strbuf_shrink_to(buf, 0); } strbuf *strbuf_new_for_agent_query(void); void strbuf_finalise_agent_query(strbuf *buf); diff --git a/pscp.c b/pscp.c index 4724622d..ef65a591 100644 --- a/pscp.c +++ b/pscp.c @@ -1306,7 +1306,7 @@ int scp_get_sink_action(struct scp_sink_action *act) act->action = SCP_SINK_RETRY; } else { act->action = SCP_SINK_DIR; - act->buf->len = 0; + strbuf_clear(act->buf); put_asciz(act->buf, stripslashes(fname, false)); act->name = act->buf->s; act->size = 0; /* duhh, it's a directory */ @@ -1326,7 +1326,7 @@ int scp_get_sink_action(struct scp_sink_action *act) * It's a file. Return SCP_SINK_FILE. */ act->action = SCP_SINK_FILE; - act->buf->len = 0; + strbuf_clear(act->buf); put_asciz(act->buf, stripslashes(fname, false)); act->name = act->buf->s; if (attrs.flags & SSH_FILEXFER_ATTR_SIZE) { @@ -1354,7 +1354,7 @@ int scp_get_sink_action(struct scp_sink_action *act) char ch; act->settime = false; - act->buf->len = 0; + strbuf_clear(act->buf); while (!done) { if (!ssh_scp_recv(&ch, 1)) @@ -1387,7 +1387,7 @@ int scp_get_sink_action(struct scp_sink_action *act) &act->mtime, &act->atime) == 2) { act->settime = true; backend_send(backend, "", 1); - act->buf->len = 0; + strbuf_clear(act->buf); continue; /* go round again */ } bump("Protocol error: Illegal time format"); diff --git a/scpserver.c b/scpserver.c index 02e2fd07..3123dc16 100644 --- a/scpserver.c +++ b/scpserver.c @@ -1074,7 +1074,7 @@ static void scp_sink_coroutine(ScpSink *scp) * Send an ack, and read a command. */ sshfwd_write(scp->sc, "\0", 1); - scp->command->len = 0; + strbuf_clear(scp->command); while (1) { crMaybeWaitUntilV(scp->input_eof || bufchain_size(&scp->data) > 0); if (scp->input_eof) @@ -1095,7 +1095,7 @@ static void scp_sink_coroutine(ScpSink *scp) /* * Parse the command. */ - scp->command->len--; /* chomp the newline */ + strbuf_shrink_by(scp->command, 1); /* chomp the newline */ scp->command_chr = scp->command->len > 0 ? scp->command->s[0] : '\0'; if (scp->command_chr == 'T') { unsigned long dummy1, dummy2; @@ -1131,7 +1131,7 @@ static void scp_sink_coroutine(ScpSink *scp) ptrlen leafname = make_ptrlen( p, scp->command->len - (p - scp->command->s)); - scp->filename_sb->len = 0; + strbuf_clear(scp->filename_sb); put_datapl(scp->filename_sb, scp->head->destpath); if (scp->head->isdir) { if (scp->filename_sb->len > 0 && diff --git a/ssh1login-server.c b/ssh1login-server.c index e7ff93de..d4988516 100644 --- a/ssh1login-server.c +++ b/ssh1login-server.c @@ -218,7 +218,7 @@ static void ssh1_login_server_process_queue(PacketProtocolLayer *ppl) if (rsa_ssh1_decrypt_pkcs1(s->sesskey, larger, data)) { mp_free(s->sesskey); s->sesskey = mp_from_bytes_be(ptrlen_from_strbuf(data)); - data->len = 0; + strbuf_clear(data); if (rsa_ssh1_decrypt_pkcs1(s->sesskey, smaller, data) && data->len == sizeof(s->session_key)) { memcpy(s->session_key, data->u, sizeof(s->session_key)); diff --git a/ssh1login.c b/ssh1login.c index 98b7fb5b..7f431ce7 100644 --- a/ssh1login.c +++ b/ssh1login.c @@ -516,7 +516,7 @@ static void ssh1_login_process_queue(PacketProtocolLayer *ppl) get_rsa_ssh1_pub(s->asrc, &s->key, RSA_SSH1_EXPONENT_FIRST); end = s->asrc->pos; - s->agent_comment->len = 0; + strbuf_clear(s->agent_comment); put_datapl(s->agent_comment, get_string(s->asrc)); if (get_err(s->asrc)) { ppl_logevent("Pageant key list packet was truncated"); diff --git a/ssh2kex-server.c b/ssh2kex-server.c index 40377153..44134ccb 100644 --- a/ssh2kex-server.c +++ b/ssh2kex-server.c @@ -57,7 +57,7 @@ void ssh2kex_coroutine(struct ssh2_transport_state *s, bool *aborted) assert(s->hkey); } - s->hostkeyblob->len = 0; + strbuf_clear(s->hostkeyblob); ssh_key_public_blob(s->hkey, BinarySink_UPCAST(s->hostkeyblob)); s->hostkeydata = ptrlen_from_strbuf(s->hostkeyblob); diff --git a/ssh2transport.c b/ssh2transport.c index 223ec5b7..982cee8c 100644 --- a/ssh2transport.c +++ b/ssh2transport.c @@ -265,7 +265,7 @@ static void ssh2_mkkey( */ keylen_padded = ((keylen + hlen - 1) / hlen) * hlen; - out->len = 0; + strbuf_clear(out); key = strbuf_append(out, keylen_padded); /* First hlen bytes. */ @@ -1083,7 +1083,7 @@ static void ssh2_transport_process_queue(PacketProtocolLayer *ppl) * Construct our KEXINIT packet, in a strbuf so we can refer to it * later. */ - s->client_kexinit->len = 0; + strbuf_clear(s->client_kexinit); put_byte(s->outgoing_kexinit, SSH2_MSG_KEXINIT); random_read(strbuf_append(s->outgoing_kexinit, 16), 16); ssh2_write_kexinit_lists( @@ -1121,7 +1121,7 @@ static void ssh2_transport_process_queue(PacketProtocolLayer *ppl) s->ppl.bpp->pls->actx, pktin->type)); return; } - s->incoming_kexinit->len = 0; + strbuf_clear(s->incoming_kexinit); put_byte(s->incoming_kexinit, SSH2_MSG_KEXINIT); put_data(s->incoming_kexinit, get_ptr(pktin), get_avail(pktin)); diff --git a/ssh2userauth.c b/ssh2userauth.c index 32dde8d0..d3bbe7a9 100644 --- a/ssh2userauth.c +++ b/ssh2userauth.c @@ -620,7 +620,7 @@ static void ssh2_userauth_process_queue(PacketProtocolLayer *ppl) /* * Save the methods string for use in error messages. */ - s->last_methods_string->len = 0; + strbuf_clear(s->last_methods_string); put_datapl(s->last_methods_string, methods); /* diff --git a/sshverstring.c b/sshverstring.c index 4828c37b..84f23fde 100644 --- a/sshverstring.c +++ b/sshverstring.c @@ -303,8 +303,7 @@ void ssh_verstring_handle_input(BinaryPacketProtocol *bpp) while (s->vstring->len > 0 && (s->vstring->s[s->vstring->len-1] == '\r' || s->vstring->s[s->vstring->len-1] == '\n')) - s->vstring->len--; - s->vstring->s[s->vstring->len] = '\0'; + strbuf_shrink_by(s->vstring, 1); bpp_logevent("Remote version: %s", s->vstring->s); diff --git a/telnet.c b/telnet.c index 034de3a6..832a4434 100644 --- a/telnet.c +++ b/telnet.c @@ -580,7 +580,7 @@ static void do_telnet_read(Telnet *telnet, const char *buf, size_t len) break; case SEENSB: telnet->sb_opt = c; - telnet->sb_buf->len = 0; + strbuf_clear(telnet->sb_buf); telnet->state = SUBNEGOT; break; case SUBNEGOT: @@ -604,7 +604,7 @@ static void do_telnet_read(Telnet *telnet, const char *buf, size_t len) if (outbuf->len >= 4096) { c_write(telnet, outbuf->u, outbuf->len); - outbuf->len = 0; + strbuf_clear(outbuf); } } diff --git a/terminal.c b/terminal.c index db397e9c..a3885303 100644 --- a/terminal.c +++ b/terminal.c @@ -440,11 +440,11 @@ static void makerle(strbuf *b, termline *ldata, if (hdrsize == 0) { assert(prevpos == hdrpos + 1); runpos = hdrpos; - b->len = prevpos+prevlen; + strbuf_shrink_to(b, prevpos+prevlen); } else { memmove(b->u + prevpos+1, b->u + prevpos, prevlen); runpos = prevpos; - b->len = prevpos+prevlen+1; + strbuf_shrink_to(b, prevpos+prevlen+1); /* * Terminate the previous run of ordinary * literals. @@ -461,7 +461,7 @@ static void makerle(strbuf *b, termline *ldata, oldstate = state; makeliteral(b, c, &state); tmplen = b->len - tmppos; - b->len = tmppos; + strbuf_shrink_to(b, tmppos); if (tmplen != thislen || memcmp(b->u + runpos+1, b->u + tmppos, tmplen)) { state = oldstate; @@ -519,7 +519,7 @@ static void makerle(strbuf *b, termline *ldata, assert(hdrsize <= 128); b->u[hdrpos] = hdrsize - 1; } else { - b->len = hdrpos; + strbuf_shrink_to(b, hdrpos); } } static void makeliteral_chr(strbuf *b, termchar *c, unsigned long *state) @@ -3057,7 +3057,7 @@ static strbuf *term_input_data_from_unicode( int rv; rv = wc_to_mb(term->ucsdata->line_codepage, 0, widebuf, len, bufptr, len + 1, NULL, term->ucsdata); - buf->len = rv < 0 ? 0 : rv; + strbuf_shrink_to(buf, rv < 0 ? 0 : rv); } return buf; diff --git a/testcrypt.c b/testcrypt.c index 0035c0c0..ec7356f6 100644 --- a/testcrypt.c +++ b/testcrypt.c @@ -760,7 +760,7 @@ strbuf *rsa_ssh1_decrypt_pkcs1_wrapper(mp_int *input, RSAKey *key) /* Again, return "" on failure */ strbuf *sb = strbuf_new(); if (!rsa_ssh1_decrypt_pkcs1(input, key, sb)) - sb->len = 0; + strbuf_clear(sb); return sb; } #define rsa_ssh1_decrypt_pkcs1 rsa_ssh1_decrypt_pkcs1_wrapper diff --git a/unix/gtkdlg.c b/unix/gtkdlg.c index 85a9210b..27b1e55c 100644 --- a/unix/gtkdlg.c +++ b/unix/gtkdlg.c @@ -3835,7 +3835,7 @@ static void eventlog_list_handler(union control *ctrl, dlgparam *dp, /* * Construct the data to use as the selection. */ - es->seldata->len = 0; + strbuf_clear(es->seldata); for (i = 0; i < es->ninitial; i++) { if (dlg_listbox_issel(ctrl, dp, i)) strbuf_catf(es->seldata, "%s\n", es->events_initial[i]); diff --git a/unix/uxnet.c b/unix/uxnet.c index a1db9ca3..dbef8b23 100644 --- a/unix/uxnet.c +++ b/unix/uxnet.c @@ -252,7 +252,7 @@ SockAddr *sk_namelookup(const char *host, char **canonicalname, int address_fami return ret; } /* This way we are always sure the h->h_name is valid :) */ - realhost->len = 0; + strbuf_clear(realhost); strbuf_catf(realhost, "%s", h->h_name); for (n = 0; h->h_addr_list[n]; n++); ret->addresses = snewn(n, unsigned long); @@ -267,7 +267,7 @@ SockAddr *sk_namelookup(const char *host, char **canonicalname, int address_fami * success return from inet_addr. */ ret->superfamily = IP; - realhost->len = 0; + strbuf_clear(realhost); strbuf_catf(realhost, "%s", host); ret->addresses = snew(unsigned long); ret->naddresses = 1; diff --git a/unix/uxstore.c b/unix/uxstore.c index b3a5b8a7..9db713d1 100644 --- a/unix/uxstore.c +++ b/unix/uxstore.c @@ -564,7 +564,7 @@ bool enum_settings_next(settings_e *handle, strbuf *out) size_t baselen = fullpath->len; while ( (de = readdir(handle->dp)) != NULL ) { - fullpath->len = baselen; + strbuf_shrink_to(fullpath, baselen); put_datapl(fullpath, ptrlen_from_asciz(de->d_name)); if (stat(fullpath->s, &st) < 0 || !S_ISREG(st.st_mode)) diff --git a/utils.c b/utils.c index 3b0f75c5..667281d6 100644 --- a/utils.c +++ b/utils.c @@ -429,6 +429,20 @@ void *strbuf_append(strbuf *buf_o, size_t len) return toret; } +void strbuf_shrink_to(strbuf *buf, size_t new_len) +{ + assert(new_len <= buf->len); + buf->len = new_len; + buf->s[buf->len] = '\0'; +} + +void strbuf_shrink_by(strbuf *buf, size_t amount_to_remove) +{ + assert(amount_to_remove <= buf->len); + buf->len -= amount_to_remove; + buf->s[buf->len] = '\0'; +} + static void strbuf_BinarySink_write( BinarySink *bs, const void *data, size_t len) { diff --git a/windows/winpgnt.c b/windows/winpgnt.c index 0dbf32e8..4374f2fe 100644 --- a/windows/winpgnt.c +++ b/windows/winpgnt.c @@ -677,7 +677,7 @@ static void update_sessions(void) sb = strbuf_new(); while(ERROR_SUCCESS == RegEnumKey(hkey, index_key, buf, MAX_PATH)) { if(strcmp(buf, PUTTY_DEFAULT) != 0) { - sb->len = 0; + strbuf_clear(sb); unescape_registry_key(buf, sb); memset(&mii, 0, sizeof(mii)); From 697cfa5b7fef47e38a631a0ed3ee5379afdf619e Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 21 Jan 2020 20:19:47 +0000 Subject: [PATCH 18/62] Use strbuf to store results in prompts_t. UBsan pointed out another memcpy from NULL (again with length 0) in the prompts_t system. When I looked at it, I realised that firstly prompt_ensure_result_size was an early not-so-good implementation of sgrowarray_nm that would benefit from being replaced with a call to the real one, and secondly, the whole system for storing prompt results should really have been replaced with strbufs with the no-move option, because that's doing all the same jobs better. So, now each prompt_t holds a strbuf in place of its previous manually managed string. prompt_ensure_result_size is gone (the console prompt-reading functions use strbuf_append, and everything else just adds to the strbuf in the usual marshal.c way). New functions exist to retrieve a prompt_t's result, either by reference or copied. (cherry picked from commit cd6bc14f04382127a6617e10a82beb0232675d70) --- cmdgen.c | 11 ++++++----- misc.c | 38 ++++++++++++-------------------------- putty.h | 18 +++--------------- rlogin.c | 6 ++++-- ssh1login.c | 18 +++++++++++------- ssh2userauth.c | 26 +++++++++++++------------- terminal.c | 15 +++++---------- unix/uxcons.c | 23 +++++++++++------------ unix/uxpgnt.c | 2 +- windows/wincons.c | 30 ++++++++++++++---------------- 10 files changed, 80 insertions(+), 107 deletions(-) diff --git a/cmdgen.c b/cmdgen.c index 7ec6dbae..03a1eab2 100644 --- a/cmdgen.c +++ b/cmdgen.c @@ -53,10 +53,10 @@ int console_get_userpass_input(prompts_t *p) int ret = 1; for (i = 0; i < p->n_prompts; i++) { if (promptsgot < nprompts) { - p->prompts[i]->result = dupstr(prompts[promptsgot++]); + prompt_set_result(p->prompts[i], prompts[promptsgot++]); if (cgtest_verbose) printf(" prompt \"%s\": response \"%s\"\n", - p->prompts[i]->prompt, p->prompts[i]->result); + p->prompts[i]->prompt, p->prompts[i]->result->s); } else { promptsgot++; /* track number of requests anyway */ ret = 0; @@ -774,7 +774,7 @@ int main(int argc, char **argv) perror("puttygen: unable to read passphrase"); return 1; } else { - old_passphrase = dupstr(p->prompts[0]->result); + old_passphrase = prompt_get_result(p->prompts[0]); free_prompts(p); } } @@ -918,12 +918,13 @@ int main(int argc, char **argv) perror("puttygen: unable to read new passphrase"); return 1; } else { - if (strcmp(p->prompts[0]->result, p->prompts[1]->result)) { + if (strcmp(prompt_get_result_ref(p->prompts[0]), + prompt_get_result_ref(p->prompts[1]))) { free_prompts(p); fprintf(stderr, "puttygen: passphrases do not match\n"); return 1; } - new_passphrase = dupstr(p->prompts[0]->result); + new_passphrase = prompt_get_result(p->prompts[0]); free_prompts(p); } } diff --git a/misc.c b/misc.c index 0e52f7ae..b7da8560 100644 --- a/misc.c +++ b/misc.c @@ -51,43 +51,29 @@ void add_prompt(prompts_t *p, char *promptstr, bool echo) prompt_t *pr = snew(prompt_t); pr->prompt = promptstr; pr->echo = echo; - pr->result = NULL; - pr->resultsize = 0; + pr->result = strbuf_new_nm(); sgrowarray(p->prompts, p->prompts_size, p->n_prompts); p->prompts[p->n_prompts++] = pr; } -void prompt_ensure_result_size(prompt_t *pr, int newlen) -{ - if ((int)pr->resultsize < newlen) { - char *newbuf; - newlen = newlen * 5 / 4 + 512; /* avoid too many small allocs */ - - /* - * We don't use sresize / realloc here, because we will be - * storing sensitive stuff like passwords in here, and we want - * to make sure that the data doesn't get copied around in - * memory without the old copy being destroyed. - */ - newbuf = snewn(newlen, char); - memcpy(newbuf, pr->result, pr->resultsize); - smemclr(pr->result, pr->resultsize); - sfree(pr->result); - pr->result = newbuf; - pr->resultsize = newlen; - } -} void prompt_set_result(prompt_t *pr, const char *newstr) { - prompt_ensure_result_size(pr, strlen(newstr) + 1); - strcpy(pr->result, newstr); + strbuf_clear(pr->result); + put_datapl(pr->result, ptrlen_from_asciz(newstr)); +} +const char *prompt_get_result_ref(prompt_t *pr) +{ + return pr->result->s; +} +char *prompt_get_result(prompt_t *pr) +{ + return dupstr(pr->result->s); } void free_prompts(prompts_t *p) { size_t i; for (i=0; i < p->n_prompts; i++) { prompt_t *pr = p->prompts[i]; - smemclr(pr->result, pr->resultsize); /* burn the evidence */ - sfree(pr->result); + strbuf_free(pr->result); sfree(pr->prompt); sfree(pr); } diff --git a/putty.h b/putty.h index 8f9643e5..5fb90366 100644 --- a/putty.h +++ b/putty.h @@ -636,19 +636,7 @@ GLOBAL char *cmdline_session_name; typedef struct { char *prompt; bool echo; - /* - * 'result' must be a dynamically allocated array of exactly - * 'resultsize' chars. The code for actually reading input may - * realloc it bigger (and adjust resultsize accordingly) if it has - * to. The caller should free it again when finished with it. - * - * If resultsize==0, then result may be NULL. When setting up a - * prompt_t, it's therefore easiest to initialise them this way, - * which means all actual allocation is done by the callee. This - * is what add_prompt does. - */ - char *result; - size_t resultsize; + strbuf *result; } prompt_t; typedef struct { /* @@ -682,8 +670,8 @@ typedef struct { prompts_t *new_prompts(); void add_prompt(prompts_t *p, char *promptstr, bool echo); void prompt_set_result(prompt_t *pr, const char *newstr); -void prompt_ensure_result_size(prompt_t *pr, int len); -/* Burn the evidence. (Assumes _all_ strings want free()ing.) */ +char *prompt_get_result(prompt_t *pr); +const char *prompt_get_result_ref(prompt_t *pr); void free_prompts(prompts_t *p); /* diff --git a/rlogin.c b/rlogin.c index e03210e5..c4f582b1 100644 --- a/rlogin.c +++ b/rlogin.c @@ -237,7 +237,8 @@ static const char *rlogin_init(Seat *seat, Backend **backend_handle, if (ret >= 0) { /* Next terminal output will come from server */ seat_set_trust_status(rlogin->seat, false); - rlogin_startup(rlogin, rlogin->prompt->prompts[0]->result); + rlogin_startup(rlogin, prompt_get_result_ref( + rlogin->prompt->prompts[0])); } } @@ -286,7 +287,8 @@ static size_t rlogin_send(Backend *be, const char *buf, size_t len) if (ret >= 0) { /* Next terminal output will come from server */ seat_set_trust_status(rlogin->seat, false); - rlogin_startup(rlogin, rlogin->prompt->prompts[0]->result); + rlogin_startup(rlogin, prompt_get_result_ref( + rlogin->prompt->prompts[0])); /* that nulls out rlogin->prompt, so then we'll start sending * data down the wire in the obvious way */ } diff --git a/ssh1login.c b/ssh1login.c index 7f431ce7..f14068e4 100644 --- a/ssh1login.c +++ b/ssh1login.c @@ -416,7 +416,7 @@ static void ssh1_login_process_queue(PacketProtocolLayer *ppl) ssh_user_close(s->ppl.ssh, "No username provided"); return; } - s->username = dupstr(s->cur_prompt->prompts[0]->result); + s->username = prompt_get_result(s->cur_prompt->prompts[0]); free_prompts(s->cur_prompt); s->cur_prompt = NULL; } @@ -676,7 +676,7 @@ static void ssh1_login_process_queue(PacketProtocolLayer *ppl) "User aborted at passphrase prompt"); return; } - passphrase = dupstr(s->cur_prompt->prompts[0]->result); + passphrase = prompt_get_result(s->cur_prompt->prompts[0]); free_prompts(s->cur_prompt); s->cur_prompt = NULL; } @@ -988,8 +988,10 @@ static void ssh1_login_process_queue(PacketProtocolLayer *ppl) * we can use the primary defence. */ int bottom, top, pwlen, i; + const char *pw = prompt_get_result_ref( + s->cur_prompt->prompts[0]); - pwlen = strlen(s->cur_prompt->prompts[0]->result); + pwlen = strlen(pw); if (pwlen < 16) { bottom = 0; /* zero length passwords are OK! :-) */ top = 15; @@ -1003,7 +1005,7 @@ static void ssh1_login_process_queue(PacketProtocolLayer *ppl) for (i = bottom; i <= top; i++) { if (i == pwlen) { pkt = ssh_bpp_new_pktout(s->ppl.bpp, s->pwpkt_type); - put_stringz(pkt, s->cur_prompt->prompts[0]->result); + put_stringz(pkt, pw); pq_push(s->ppl.out_pq, pkt); } else { strbuf *random_data = strbuf_new_nm(); @@ -1026,7 +1028,8 @@ static void ssh1_login_process_queue(PacketProtocolLayer *ppl) ppl_logevent("Sending length-padded password"); pkt = ssh_bpp_new_pktout(s->ppl.bpp, s->pwpkt_type); - put_asciz(padded_pw, s->cur_prompt->prompts[0]->result); + put_asciz(padded_pw, prompt_get_result_ref( + s->cur_prompt->prompts[0])); size_t pad = 63 & -padded_pw->len; random_read(strbuf_append(padded_pw, pad), pad); put_stringsb(pkt, padded_pw); @@ -1038,12 +1041,13 @@ static void ssh1_login_process_queue(PacketProtocolLayer *ppl) */ ppl_logevent("Sending unpadded password"); pkt = ssh_bpp_new_pktout(s->ppl.bpp, s->pwpkt_type); - put_stringz(pkt, s->cur_prompt->prompts[0]->result); + put_stringz(pkt, prompt_get_result_ref( + s->cur_prompt->prompts[0])); pq_push(s->ppl.out_pq, pkt); } } else { pkt = ssh_bpp_new_pktout(s->ppl.bpp, s->pwpkt_type); - put_stringz(pkt, s->cur_prompt->prompts[0]->result); + put_stringz(pkt, prompt_get_result_ref(s->cur_prompt->prompts[0])); pq_push(s->ppl.out_pq, pkt); } ppl_logevent("Sent password"); diff --git a/ssh2userauth.c b/ssh2userauth.c index d3bbe7a9..a913aff2 100644 --- a/ssh2userauth.c +++ b/ssh2userauth.c @@ -432,7 +432,7 @@ static void ssh2_userauth_process_queue(PacketProtocolLayer *ppl) } sfree(s->locally_allocated_username); /* for change_username */ s->username = s->locally_allocated_username = - dupstr(s->cur_prompt->prompts[0]->result); + prompt_get_result(s->cur_prompt->prompts[0]); free_prompts(s->cur_prompt); } else { if ((flags & FLAG_VERBOSE) || (flags & FLAG_INTERACTIVE)) @@ -884,7 +884,7 @@ static void ssh2_userauth_process_queue(PacketProtocolLayer *ppl) return; } passphrase = - dupstr(s->cur_prompt->prompts[0]->result); + prompt_get_result(s->cur_prompt->prompts[0]); free_prompts(s->cur_prompt); } else { passphrase = NULL; /* no passphrase needed */ @@ -1374,8 +1374,8 @@ static void ssh2_userauth_process_queue(PacketProtocolLayer *ppl) s->ppl.bpp, SSH2_MSG_USERAUTH_INFO_RESPONSE); put_uint32(s->pktout, s->num_prompts); for (uint32_t i = 0; i < s->num_prompts; i++) { - put_stringz(s->pktout, - s->cur_prompt->prompts[i]->result); + put_stringz(s->pktout, prompt_get_result_ref( + s->cur_prompt->prompts[i])); } s->pktout->minlen = 256; pq_push(s->ppl.out_pq, s->pktout); @@ -1457,7 +1457,7 @@ static void ssh2_userauth_process_queue(PacketProtocolLayer *ppl) * Squirrel away the password. (We may need it later if * asked to change it.) */ - s->password = dupstr(s->cur_prompt->prompts[0]->result); + s->password = prompt_get_result(s->cur_prompt->prompts[0]); free_prompts(s->cur_prompt); /* @@ -1583,20 +1583,20 @@ static void ssh2_userauth_process_queue(PacketProtocolLayer *ppl) * (A side effect is that the user doesn't have to * re-enter it if they louse up the new password.) */ - if (s->cur_prompt->prompts[0]->result[0]) { + if (s->cur_prompt->prompts[0]->result->s[0]) { smemclr(s->password, strlen(s->password)); /* burn the evidence */ sfree(s->password); - s->password = - dupstr(s->cur_prompt->prompts[0]->result); + s->password = prompt_get_result( + s->cur_prompt->prompts[0]); } /* * Check the two new passwords match. */ - got_new = (strcmp(s->cur_prompt->prompts[1]->result, - s->cur_prompt->prompts[2]->result) - == 0); + got_new = !strcmp( + prompt_get_result_ref(s->cur_prompt->prompts[1]), + prompt_get_result_ref(s->cur_prompt->prompts[2])); if (!got_new) /* They don't. Silly user. */ ppl_printf("Passwords do not match\r\n"); @@ -1614,8 +1614,8 @@ static void ssh2_userauth_process_queue(PacketProtocolLayer *ppl) put_stringz(s->pktout, "password"); put_bool(s->pktout, true); put_stringz(s->pktout, s->password); - put_stringz(s->pktout, - s->cur_prompt->prompts[1]->result); + put_stringz(s->pktout, prompt_get_result_ref( + s->cur_prompt->prompts[1])); free_prompts(s->cur_prompt); s->pktout->minlen = 256; pq_push(s->ppl.out_pq, s->pktout); diff --git a/terminal.c b/terminal.c index a3885303..08592568 100644 --- a/terminal.c +++ b/terminal.c @@ -7156,7 +7156,6 @@ char *term_get_ttymode(Terminal *term, const char *mode) struct term_userpass_state { size_t curr_prompt; bool done_prompt; /* printed out prompt yet? */ - size_t pos; /* cursor position */ }; /* Tiny wrapper to make it easier to write lots of little strings */ @@ -7211,7 +7210,6 @@ int term_get_userpass_input(Terminal *term, prompts_t *p, bufchain *input) if (!s->done_prompt) { term_write(term, ptrlen_from_asciz(pr->prompt)); s->done_prompt = true; - s->pos = 0; } /* Breaking out here ensures that the prompt is printed even @@ -7226,8 +7224,6 @@ int term_get_userpass_input(Terminal *term, prompts_t *p, bufchain *input) case 10: case 13: term_write(term, PTRLEN_LITERAL("\r\n")); - prompt_ensure_result_size(pr, s->pos + 1); - pr->result[s->pos] = '\0'; /* go to next prompt, if any */ s->curr_prompt++; s->done_prompt = false; @@ -7235,18 +7231,18 @@ int term_get_userpass_input(Terminal *term, prompts_t *p, bufchain *input) break; case 8: case 127: - if (s->pos > 0) { + if (pr->result->len > 0) { if (pr->echo) term_write(term, PTRLEN_LITERAL("\b \b")); - s->pos--; + strbuf_shrink_by(pr->result, 1); } break; case 21: case 27: - while (s->pos > 0) { + while (pr->result->len > 0) { if (pr->echo) term_write(term, PTRLEN_LITERAL("\b \b")); - s->pos--; + strbuf_shrink_by(pr->result, 1); } break; case 3: @@ -7264,8 +7260,7 @@ int term_get_userpass_input(Terminal *term, prompts_t *p, bufchain *input) */ if (!pr->echo || (c >= ' ' && c <= '~') || ((unsigned char) c >= 160)) { - prompt_ensure_result_size(pr, s->pos + 1); - pr->result[s->pos++] = c; + put_byte(pr->result, c); if (pr->echo) term_write(term, make_ptrlen(&c, 1)); } diff --git a/unix/uxcons.c b/unix/uxcons.c index 4811c177..3e8a7ea5 100644 --- a/unix/uxcons.c +++ b/unix/uxcons.c @@ -566,7 +566,6 @@ int console_get_userpass_input(prompts_t *p) for (curr_prompt = 0; curr_prompt < p->n_prompts; curr_prompt++) { struct termios oldmode, newmode; - int len; prompt_t *pr = p->prompts[curr_prompt]; tcgetattr(infd, &oldmode); @@ -580,19 +579,21 @@ int console_get_userpass_input(prompts_t *p) console_write(outfp, ptrlen_from_asciz(pr->prompt)); - len = 0; + bool failed = false; while (1) { - int ret; + size_t toread = 65536; + size_t prev_result_len = pr->result->len; + void *ptr = strbuf_append(pr->result, toread); + int ret = read(infd, ptr, toread); - prompt_ensure_result_size(pr, len * 5 / 4 + 512); - ret = read(infd, pr->result + len, pr->resultsize - len - 1); if (ret <= 0) { - len = -1; + failed = true; break; } - len += ret; - if (pr->result[len - 1] == '\n') { - len--; + + strbuf_shrink_to(pr->result, prev_result_len + ret); + if (pr->result->s[pr->result->len - 1] == '\n') { + strbuf_shrink_by(pr->result, 1); break; } } @@ -602,12 +603,10 @@ int console_get_userpass_input(prompts_t *p) if (!pr->echo) console_write(outfp, PTRLEN_LITERAL("\n")); - if (len < 0) { + if (failed) { console_close(outfp, infd); return 0; /* failure due to read error */ } - - pr->result[len] = '\0'; } console_close(outfp, infd); diff --git a/unix/uxpgnt.c b/unix/uxpgnt.c index 61a94334..3d83fcf0 100644 --- a/unix/uxpgnt.c +++ b/unix/uxpgnt.c @@ -338,7 +338,7 @@ static char *askpass_tty(const char *prompt) free_prompts(p); return NULL; } else { - char *passphrase = dupstr(p->prompts[0]->result); + char *passphrase = prompt_get_result(p->prompts[0]); free_prompts(p); return passphrase; } diff --git a/windows/wincons.c b/windows/wincons.c index 81df8cee..d91d9cef 100644 --- a/windows/wincons.c +++ b/windows/wincons.c @@ -490,7 +490,6 @@ int console_get_userpass_input(prompts_t *p) for (curr_prompt = 0; curr_prompt < p->n_prompts; curr_prompt++) { DWORD savemode, newmode; - size_t len; prompt_t *pr = p->prompts[curr_prompt]; GetConsoleMode(hin, &savemode); @@ -503,22 +502,23 @@ int console_get_userpass_input(prompts_t *p) console_write(hout, ptrlen_from_asciz(pr->prompt)); - len = 0; + bool failed = false; while (1) { + DWORD toread = 65536; + size_t prev_result_len = pr->result->len; + void *ptr = strbuf_append(pr->result, toread); + DWORD ret = 0; - - prompt_ensure_result_size(pr, len * 5 / 4 + 512); - - if (!ReadFile(hin, pr->result + len, pr->resultsize - len - 1, - &ret, NULL) || ret == 0) { - len = (size_t)-1; + if (!ReadFile(hin, ptr, toread, &ret, NULL) || ret == 0) { + failed = true; break; } - len += ret; - if (pr->result[len - 1] == '\n') { - len--; - if (pr->result[len - 1] == '\r') - len--; + + strbuf_shrink_to(pr->result, prev_result_len + ret); + if (pr->result->s[pr->result->len - 1] == '\n') { + strbuf_shrink_by(pr->result, 1); + if (pr->result->s[pr->result->len - 1] == '\r') + strbuf_shrink_by(pr->result, 1); break; } } @@ -528,11 +528,9 @@ int console_get_userpass_input(prompts_t *p) if (!pr->echo) console_write(hout, PTRLEN_LITERAL("\r\n")); - if (len == (size_t)-1) { + if (failed) { return 0; /* failure due to read error */ } - - pr->result[len] = '\0'; } return 1; /* success */ From 0021ad352d0c9a1d9667dc4006bd39fa6bb305c7 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Wed, 22 Jan 2020 22:24:41 +0000 Subject: [PATCH 19/62] Introduce and use strbuf_chomp. Those chomp operations in wincons.c and uxcons.c looked ugly, and I'm not totally convinced they couldn't underrun the buffer by 1 byte in weird circumstances. strbuf_chomp is neater. (cherry picked from commit 7590d0625ba6e994a00191aa708d3aadee70c407) --- misc.h | 1 + scpserver.c | 2 +- unix/uxcons.c | 4 +--- utils.c | 9 +++++++++ windows/wincons.c | 6 ++---- 5 files changed, 14 insertions(+), 8 deletions(-) diff --git a/misc.h b/misc.h index 7f53d4e0..590c8a3b 100644 --- a/misc.h +++ b/misc.h @@ -79,6 +79,7 @@ char *strbuf_to_str(strbuf *buf); /* does free buf, but you must free result */ void strbuf_catf(strbuf *buf, const char *fmt, ...); void strbuf_catfv(strbuf *buf, const char *fmt, va_list ap); static inline void strbuf_clear(strbuf *buf) { strbuf_shrink_to(buf, 0); } +bool strbuf_chomp(strbuf *buf, char char_to_remove); strbuf *strbuf_new_for_agent_query(void); void strbuf_finalise_agent_query(strbuf *buf); diff --git a/scpserver.c b/scpserver.c index 3123dc16..e594f1ab 100644 --- a/scpserver.c +++ b/scpserver.c @@ -1095,7 +1095,7 @@ static void scp_sink_coroutine(ScpSink *scp) /* * Parse the command. */ - strbuf_shrink_by(scp->command, 1); /* chomp the newline */ + strbuf_chomp(scp->command, '\n'); scp->command_chr = scp->command->len > 0 ? scp->command->s[0] : '\0'; if (scp->command_chr == 'T') { unsigned long dummy1, dummy2; diff --git a/unix/uxcons.c b/unix/uxcons.c index 3e8a7ea5..da7b8935 100644 --- a/unix/uxcons.c +++ b/unix/uxcons.c @@ -592,10 +592,8 @@ int console_get_userpass_input(prompts_t *p) } strbuf_shrink_to(pr->result, prev_result_len + ret); - if (pr->result->s[pr->result->len - 1] == '\n') { - strbuf_shrink_by(pr->result, 1); + if (strbuf_chomp(pr->result, '\n')) break; - } } tcsetattr(infd, TCSANOW, &oldmode); diff --git a/utils.c b/utils.c index 667281d6..5acb53cb 100644 --- a/utils.c +++ b/utils.c @@ -443,6 +443,15 @@ void strbuf_shrink_by(strbuf *buf, size_t amount_to_remove) buf->s[buf->len] = '\0'; } +bool strbuf_chomp(strbuf *buf, char char_to_remove) +{ + if (buf->len > 0 && buf->s[buf->len-1] == char_to_remove) { + strbuf_shrink_by(buf, 1); + return true; + } + return false; +} + static void strbuf_BinarySink_write( BinarySink *bs, const void *data, size_t len) { diff --git a/windows/wincons.c b/windows/wincons.c index d91d9cef..4fb5eb3f 100644 --- a/windows/wincons.c +++ b/windows/wincons.c @@ -515,10 +515,8 @@ int console_get_userpass_input(prompts_t *p) } strbuf_shrink_to(pr->result, prev_result_len + ret); - if (pr->result->s[pr->result->len - 1] == '\n') { - strbuf_shrink_by(pr->result, 1); - if (pr->result->s[pr->result->len - 1] == '\r') - strbuf_shrink_by(pr->result, 1); + if (strbuf_chomp(pr->result, '\n')) { + strbuf_chomp(pr->result, '\r'); break; } } From 97b39eeca35f5e54429d1ee29a334b7f2e0537c3 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 26 Jan 2020 09:50:07 +0000 Subject: [PATCH 20/62] Work around console I/O size limit on Windows 7. A user reports that the ReadFile call in console_get_userpass_input fails with ERROR_NOT_ENOUGH_MEMORY on Windows 7, and further reports that this problem only happens if you tell ReadFile to read more than 31366 bytes in a single call. That seems to be a thing that other people have found as well: I turned up a similar workaround in Ruby's Win32 support module, except that there it's for WriteConsole. So I'm reducing my arbitrary read size of 64K to 16K, which is well under that limit. This issue became noticeable in PuTTY as of the recent commit cd6bc14f0, which reworked console_get_userpass_input to use strbufs. Previously we were trying to read an amount proportional to the existing size of the buffer, so as to grow the buffer exponentially to save quadratic-time reallocation. That was OK in practice, since the initial read size was nice and small. But in principle, the same bug was present in that version of the code, just latent - if we'd ever been called on to read a _really large_ amount of data, then _eventually_ the input size parameter to ReadFile would have grown beyond that mysterious limit! (cherry picked from commit 7b79d22021296e4ae941b383ab34971a9f2b0a1b) --- windows/wincons.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/windows/wincons.c b/windows/wincons.c index 4fb5eb3f..ffc93040 100644 --- a/windows/wincons.c +++ b/windows/wincons.c @@ -504,7 +504,23 @@ int console_get_userpass_input(prompts_t *p) bool failed = false; while (1) { - DWORD toread = 65536; + /* + * Amount of data to try to read from the console in one + * go. This isn't completely arbitrary: a user reported + * that trying to read more than 31366 bytes at a time + * would fail with ERROR_NOT_ENOUGH_MEMORY on Windows 7, + * and Ruby's Win32 support module has evidence of a + * similar workaround: + * + * https://github.com/ruby/ruby/blob/0aa5195262d4193d3accf3e6b9bad236238b816b/win32/win32.c#L6842 + * + * To keep things simple, I stick with a nice round power + * of 2 rather than trying to go to the very limit of that + * bug. (We're typically reading user passphrases and the + * like here, so even this much is overkill really.) + */ + DWORD toread = 16384; + size_t prev_result_len = pr->result->len; void *ptr = strbuf_append(pr->result, toread); From 45198e10c521a8491ef261b554ea7a118a9844df Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 26 Jan 2020 15:16:47 +0000 Subject: [PATCH 21/62] Update _MSC_VER translation table. The entry for 19.0 which we included in advance of its listing on the official page is now confirmed, and also three followup versions. (cherry picked from commit 0a4e068adaa6162d2d42a33a23e7687e19525a81) --- misc.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/misc.c b/misc.c index b7da8560..610f0f3d 100644 --- a/misc.c +++ b/misc.c @@ -229,16 +229,20 @@ char *buildinfo(const char *newline) /* * List of _MSC_VER values and their translations taken from * https://docs.microsoft.com/en-us/cpp/preprocessor/predefined-macros - * except for 1920, which is not yet listed on that page as of - * 2019-03-22, and was determined experimentally by Sean Kain. * * The pointless #if 0 branch containing this comment is there so * that every real clause can start with #elif and there's no * anomalous first clause. That way the patch looks nicer when you * add extra ones. */ +#elif _MSC_VER == 1923 + strbuf_catf(buf, " 2019 (16.3)"); +#elif _MSC_VER == 1922 + strbuf_catf(buf, " 2019 (16.2)"); +#elif _MSC_VER == 1921 + strbuf_catf(buf, " 2019 (16.1)"); #elif _MSC_VER == 1920 - strbuf_catf(buf, " 2019 (16.x)"); + strbuf_catf(buf, " 2019 (16.0)"); #elif _MSC_VER == 1916 strbuf_catf(buf, " 2017 version 15.9"); #elif _MSC_VER == 1915 From 03f6e88385d44150245936fdb6b4d07279183470 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 9 Feb 2020 08:22:20 +0000 Subject: [PATCH 22/62] Greatly improve printf format-string checking. I've added the gcc-style attribute("printf") to a lot of printf-shaped functions in this code base that didn't have it. To make that easier, I moved the wrapping macro into defs.h, and also enabled it if we detect the __clang__ macro as well as __GNU__ (hence, it will be used when building for Windows using clang-cl). The result is that a great many format strings in the code are now checked by the compiler, where they were previously not. This causes build failures, which I'll fix in the next commit. (cherry picked from commit cbfba7a0e954fe0f2c3602594fa609be21a35833) --- cmdgen.c | 4 ++-- defs.h | 20 ++++++++++++++++++++ logging.c | 2 +- misc.h | 26 +++----------------------- network.h | 3 ++- pageant.c | 9 ++------- pscp.c | 6 +++--- putty.h | 13 +++++++------ scpserver.c | 9 ++++++--- ssh.h | 12 ++++++------ sshshare.c | 8 ++++---- testcrypt.c | 2 +- testsc.c | 2 +- tree234.c | 2 +- 14 files changed, 59 insertions(+), 59 deletions(-) diff --git a/cmdgen.c b/cmdgen.c index 03a1eab2..b391bd1d 100644 --- a/cmdgen.c +++ b/cmdgen.c @@ -1195,7 +1195,7 @@ void test(int retval, ...) sfree(argv); } -void filecmp(char *file1, char *file2, char *fmt, ...) +PRINTF_LIKE(3, 4) void filecmp(char *file1, char *file2, char *fmt, ...) { /* * Ideally I should do file comparison myself, to maximise the @@ -1260,7 +1260,7 @@ char *get_fp(char *filename) return cleanup_fp(buf); } -void check_fp(char *filename, char *fp, char *fmt, ...) +PRINTF_LIKE(3, 4) void check_fp(char *filename, char *fp, char *fmt, ...) { char *newfp; diff --git a/defs.h b/defs.h index afeb5062..0a4f08c8 100644 --- a/defs.h +++ b/defs.h @@ -27,6 +27,26 @@ uintmax_t strtoumax(const char *nptr, char **endptr, int base); #include #endif +#if defined __GNUC__ || defined __clang__ +/* + * On MinGW, the correct compiler format checking for vsnprintf() etc + * can depend on compile-time flags; these control whether you get + * ISO C or Microsoft's non-standard format strings. + * We sometimes use __attribute__ ((format)) for our own printf-like + * functions, which are ultimately interpreted by the toolchain-chosen + * printf, so we need to take that into account to get correct warnings. + */ +#ifdef __MINGW_PRINTF_FORMAT +#define PRINTF_LIKE(fmt_index, ellipsis_index) \ + __attribute__ ((format (__MINGW_PRINTF_FORMAT, fmt_index, ellipsis_index))) +#else +#define PRINTF_LIKE(fmt_index, ellipsis_index) \ + __attribute__ ((format (printf, fmt_index, ellipsis_index))) +#endif +#else /* __GNUC__ */ +#define PRINTF_LIKE(fmt_index, ellipsis_index) +#endif /* __GNUC__ */ + typedef struct conf_tag Conf; typedef struct terminal_tag Terminal; typedef struct term_utf8_decode term_utf8_decode; diff --git a/logging.c b/logging.c index 173ed080..e324c77c 100644 --- a/logging.c +++ b/logging.c @@ -58,7 +58,7 @@ static void logwrite(LogContext *ctx, ptrlen data) * Convenience wrapper on logwrite() which printf-formats the * string. */ -static void logprintf(LogContext *ctx, const char *fmt, ...) +static PRINTF_LIKE(2, 3) void logprintf(LogContext *ctx, const char *fmt, ...) { va_list ap; char *data; diff --git a/misc.h b/misc.h index 590c8a3b..d75243af 100644 --- a/misc.h +++ b/misc.h @@ -24,30 +24,10 @@ char *host_strchr(const char *s, int c); char *host_strrchr(const char *s, int c); char *host_strduptrim(const char *s); -#ifdef __GNUC__ -/* - * On MinGW, the correct compiler format checking for vsnprintf() etc - * can depend on compile-time flags; these control whether you get - * ISO C or Microsoft's non-standard format strings. - * We sometimes use __attribute__ ((format)) for our own printf-like - * functions, which are ultimately interpreted by the toolchain-chosen - * printf, so we need to take that into account to get correct warnings. - */ -#ifdef __MINGW_PRINTF_FORMAT -#define PUTTY_PRINTF_ARCHETYPE __MINGW_PRINTF_FORMAT -#else -#define PUTTY_PRINTF_ARCHETYPE printf -#endif -#endif /* __GNUC__ */ - char *dupstr(const char *s); char *dupcat_fn(const char *s1, ...); #define dupcat(...) dupcat_fn(__VA_ARGS__, (const char *)NULL) -char *dupprintf(const char *fmt, ...) -#ifdef __GNUC__ - __attribute__ ((format (PUTTY_PRINTF_ARCHETYPE, 1, 2))) -#endif - ; +char *dupprintf(const char *fmt, ...) PRINTF_LIKE(1, 2); char *dupvprintf(const char *fmt, va_list ap); void burnstr(char *string); @@ -76,7 +56,7 @@ void *strbuf_append(strbuf *buf, size_t len); void strbuf_shrink_to(strbuf *buf, size_t new_len); void strbuf_shrink_by(strbuf *buf, size_t amount_to_remove); char *strbuf_to_str(strbuf *buf); /* does free buf, but you must free result */ -void strbuf_catf(strbuf *buf, const char *fmt, ...); +void strbuf_catf(strbuf *buf, const char *fmt, ...) PRINTF_LIKE(2, 3); void strbuf_catfv(strbuf *buf, const char *fmt, va_list ap); static inline void strbuf_clear(strbuf *buf) { strbuf_shrink_to(buf, 0); } bool strbuf_chomp(strbuf *buf, char char_to_remove); @@ -263,7 +243,7 @@ static inline NORETURN void unreachable_internal(void) { abort(); } */ #ifdef DEBUG -void debug_printf(const char *fmt, ...); +void debug_printf(const char *fmt, ...) PRINTF_LIKE(1, 2); void debug_memdump(const void *buf, int len, bool L); #define debug(...) (debug_printf(__VA_ARGS__)) #define dmemdump(buf,len) (debug_memdump(buf, len, false)) diff --git a/network.h b/network.h index 355da2b3..49d517d2 100644 --- a/network.h +++ b/network.h @@ -265,7 +265,8 @@ char *get_hostname(void); * Trivial socket implementation which just stores an error. Found in * errsock.c. */ -Socket *new_error_socket_fmt(Plug *plug, const char *fmt, ...); +Socket *new_error_socket_fmt(Plug *plug, const char *fmt, ...) + PRINTF_LIKE(2, 3); /* * Trivial plug that does absolutely nothing. Found in nullplug.c. diff --git a/pageant.c b/pageant.c index 70b21743..ba334cc3 100644 --- a/pageant.c +++ b/pageant.c @@ -129,13 +129,8 @@ void pageant_make_keylist2(BinarySink *bs) } } -static void plog(void *logctx, pageant_logfn_t logfn, const char *fmt, ...) -#ifdef __GNUC__ -__attribute__ ((format (PUTTY_PRINTF_ARCHETYPE, 3, 4))) -#endif - ; - -static void plog(void *logctx, pageant_logfn_t logfn, const char *fmt, ...) +static PRINTF_LIKE(3, 4) void plog(void *logctx, pageant_logfn_t logfn, + const char *fmt, ...) { /* * This is the wrapper that takes a variadic argument list and diff --git a/pscp.c b/pscp.c index ef65a591..5f69e4c3 100644 --- a/pscp.c +++ b/pscp.c @@ -113,7 +113,7 @@ static void abandon_stats(void) } } -static void tell_user(FILE *stream, const char *fmt, ...) +static PRINTF_LIKE(2, 3) void tell_user(FILE *stream, const char *fmt, ...) { char *str, *str2; va_list ap; @@ -224,7 +224,7 @@ static void ssh_scp_init(void) /* * Print an error message and exit after closing the SSH link. */ -static NORETURN void bump(const char *fmt, ...) +static NORETURN PRINTF_LIKE(1, 2) void bump(const char *fmt, ...) { char *str, *str2; va_list ap; @@ -1541,7 +1541,7 @@ int scp_finish_filerecv(void) * Send an error message to the other side and to the screen. * Increment error counter. */ -static void run_err(const char *fmt, ...) +static PRINTF_LIKE(1, 2) void run_err(const char *fmt, ...) { char *str, *str2; va_list ap; diff --git a/putty.h b/putty.h index 5fb90366..1498370e 100644 --- a/putty.h +++ b/putty.h @@ -1003,7 +1003,7 @@ static inline bool seat_set_trust_status(Seat *seat, bool trusted) /* Unlike the seat's actual method, the public entry point * seat_connection_fatal is a wrapper function with a printf-like API, * defined in misc.c. */ -void seat_connection_fatal(Seat *seat, const char *fmt, ...); +void seat_connection_fatal(Seat *seat, const char *fmt, ...) PRINTF_LIKE(2, 3); /* Handy aliases for seat_output which set is_stderr to a fixed value. */ static inline size_t seat_stdout(Seat *seat, const void *data, size_t len) @@ -1218,8 +1218,8 @@ static inline bool win_is_utf8(TermWin *win) /* * Global functions not specific to a connection instance. */ -void nonfatal(const char *, ...); -NORETURN void modalfatalbox(const char *, ...); +void nonfatal(const char *, ...) PRINTF_LIKE(1, 2); +NORETURN void modalfatalbox(const char *, ...) PRINTF_LIKE(1, 2); NORETURN void cleanup_exit(int); /* @@ -1703,7 +1703,7 @@ void logfclose(LogContext *logctx); void logtraffic(LogContext *logctx, unsigned char c, int logmode); void logflush(LogContext *logctx); void logevent(LogContext *logctx, const char *event); -void logeventf(LogContext *logctx, const char *fmt, ...); +void logeventf(LogContext *logctx, const char *fmt, ...) PRINTF_LIKE(2, 3); void logeventvf(LogContext *logctx, const char *fmt, va_list ap); /* @@ -1917,7 +1917,8 @@ bool is_interactive(void); void console_print_error_msg(const char *prefix, const char *msg); void console_print_error_msg_fmt_v( const char *prefix, const char *fmt, va_list ap); -void console_print_error_msg_fmt(const char *prefix, const char *fmt, ...); +void console_print_error_msg_fmt(const char *prefix, const char *fmt, ...) + PRINTF_LIKE(2, 3); /* * Exports from printing.c. @@ -1955,7 +1956,7 @@ bool cmdline_host_ok(Conf *); #define TOOLTYPE_PORT_ARG 64 extern int cmdline_tooltype; -void cmdline_error(const char *, ...); +void cmdline_error(const char *, ...) PRINTF_LIKE(1, 2); /* * Exports from config.c. diff --git a/scpserver.c b/scpserver.c index e594f1ab..3a136286 100644 --- a/scpserver.c +++ b/scpserver.c @@ -431,7 +431,8 @@ static char *scp_source_err_base(ScpSource *scp, const char *fmt, va_list ap) sshfwd_write_ext(scp->sc, true, "\012", 1); return msg; } -static void scp_source_err(ScpSource *scp, const char *fmt, ...) +static PRINTF_LIKE(2, 3) void scp_source_err( + ScpSource *scp, const char *fmt, ...) { va_list ap; @@ -439,7 +440,8 @@ static void scp_source_err(ScpSource *scp, const char *fmt, ...) sfree(scp_source_err_base(scp, fmt, ap)); va_end(ap); } -static void scp_source_abort(ScpSource *scp, const char *fmt, ...) +static PRINTF_LIKE(2, 3) void scp_source_abort( + ScpSource *scp, const char *fmt, ...) { va_list ap; char *msg; @@ -1312,7 +1314,8 @@ static void scp_error_send_message_cb(void *vscp) sshfwd_initiate_close(scp->sc, scp->message); } -static ScpError *scp_error_new(SshChannel *sc, const char *fmt, ...) +static PRINTF_LIKE(2, 3) ScpError *scp_error_new( + SshChannel *sc, const char *fmt, ...) { va_list ap; ScpError *scp = snew(ScpError); diff --git a/ssh.h b/ssh.h index af0f9d47..d3c31c8f 100644 --- a/ssh.h +++ b/ssh.h @@ -403,12 +403,12 @@ void ssh_conn_processed_data(Ssh *ssh); void ssh_check_frozen(Ssh *ssh); /* Functions to abort the connection, for various reasons. */ -void ssh_remote_error(Ssh *ssh, const char *fmt, ...); -void ssh_remote_eof(Ssh *ssh, const char *fmt, ...); -void ssh_proto_error(Ssh *ssh, const char *fmt, ...); -void ssh_sw_abort(Ssh *ssh, const char *fmt, ...); -void ssh_sw_abort_deferred(Ssh *ssh, const char *fmt, ...); -void ssh_user_close(Ssh *ssh, const char *fmt, ...); +void ssh_remote_error(Ssh *ssh, const char *fmt, ...) PRINTF_LIKE(2, 3); +void ssh_remote_eof(Ssh *ssh, const char *fmt, ...) PRINTF_LIKE(2, 3); +void ssh_proto_error(Ssh *ssh, const char *fmt, ...) PRINTF_LIKE(2, 3); +void ssh_sw_abort(Ssh *ssh, const char *fmt, ...) PRINTF_LIKE(2, 3); +void ssh_sw_abort_deferred(Ssh *ssh, const char *fmt, ...) PRINTF_LIKE(2, 3); +void ssh_user_close(Ssh *ssh, const char *fmt, ...) PRINTF_LIKE(2, 3); /* Bit positions in the SSH-1 cipher protocol word */ #define SSH1_CIPHER_IDEA 1 diff --git a/sshshare.c b/sshshare.c index 24f4c125..98071d8e 100644 --- a/sshshare.c +++ b/sshshare.c @@ -705,8 +705,8 @@ static void share_remove_forwarding(struct ssh_sharing_connstate *cs, sfree(fwd); } -static void log_downstream(struct ssh_sharing_connstate *cs, - const char *logfmt, ...) +static PRINTF_LIKE(2, 3) void log_downstream(struct ssh_sharing_connstate *cs, + const char *logfmt, ...) { va_list ap; char *buf; @@ -719,8 +719,8 @@ static void log_downstream(struct ssh_sharing_connstate *cs, sfree(buf); } -static void log_general(struct ssh_sharing_state *sharestate, - const char *logfmt, ...) +static PRINTF_LIKE(2, 3) void log_general(struct ssh_sharing_state *sharestate, + const char *logfmt, ...) { va_list ap; char *buf; diff --git a/testcrypt.c b/testcrypt.c index ec7356f6..c92311f4 100644 --- a/testcrypt.c +++ b/testcrypt.c @@ -35,7 +35,7 @@ #include "mpint.h" #include "ecc.h" -static NORETURN void fatal_error(const char *p, ...) +static NORETURN PRINTF_LIKE(1, 2) void fatal_error(const char *p, ...) { va_list ap; fprintf(stderr, "testcrypt: "); diff --git a/testsc.c b/testsc.c index 79c5ba1f..760a9482 100644 --- a/testsc.c +++ b/testsc.c @@ -81,7 +81,7 @@ #include "mpint.h" #include "ecc.h" -static NORETURN void fatal_error(const char *p, ...) +static NORETURN PRINTF_LIKE(1, 2) void fatal_error(const char *p, ...) { va_list ap; fprintf(stderr, "testsc: "); diff --git a/tree234.c b/tree234.c index 0ec2b520..bf68e23f 100644 --- a/tree234.c +++ b/tree234.c @@ -1072,7 +1072,7 @@ int n_errors = 0; /* * Error reporting function. */ -void error(char *fmt, ...) +PRINTF_LIKE(1, 2) void error(char *fmt, ...) { va_list ap; printf("ERROR: "); From cb671ec2d8189520945bb90676738f33b952b17c Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 26 Jan 2020 15:00:13 +0000 Subject: [PATCH 23/62] Fix format string mistakes revealed by new checking. An assortment of errors: int vs size_t confusion (probably undetected since the big switchover in commit 0cda34c6f), some outright spurious parameters after the format string (copy-paste errors), a particularly silly one in pscp.c (a comma between two halves of what should have been a single string literal), and a _missing_ format string in ssh.c (but luckily in a context where the only text that would be wrongly treated as a format string was error messages generated elsewhere in PuTTY). (cherry picked from commit 247866a9d3e7b6ecae951a6bee7f876e399d3d01) --- logging.c | 4 ++-- misc.c | 2 +- pscp.c | 2 +- ssh.c | 2 +- ssh2connection.c | 3 ++- sshhmac.c | 2 +- sshrsa.c | 2 +- sshshare.c | 3 ++- 8 files changed, 11 insertions(+), 9 deletions(-) diff --git a/logging.c b/logging.c index e324c77c..d61038c9 100644 --- a/logging.c +++ b/logging.c @@ -344,7 +344,7 @@ void log_packet(LogContext *ctx, int direction, int type, /* If we're about to stop omitting, it's time to say how * much we omitted. */ if ((blktype != PKTLOG_OMIT) && omitted) { - logprintf(ctx, " (%d byte%s omitted)\r\n", + logprintf(ctx, " (%zu byte%s omitted)\r\n", omitted, (omitted==1?"":"s")); omitted = 0; } @@ -387,7 +387,7 @@ void log_packet(LogContext *ctx, int direction, int type, /* Tidy up */ if (omitted) - logprintf(ctx, " (%d byte%s omitted)\r\n", + logprintf(ctx, " (%zu byte%s omitted)\r\n", omitted, (omitted==1?"":"s")); logflush(ctx); } diff --git a/misc.c b/misc.c index 610f0f3d..9ca15c8f 100644 --- a/misc.c +++ b/misc.c @@ -223,7 +223,7 @@ char *buildinfo(const char *newline) #else strbuf_catf(buf, ", emulating "); #endif - strbuf_catf(buf, "Visual Studio", newline); + strbuf_catf(buf, "Visual Studio"); #if 0 /* diff --git a/pscp.c b/pscp.c index 5f69e4c3..cadf7916 100644 --- a/pscp.c +++ b/pscp.c @@ -1789,7 +1789,7 @@ static void sink(const char *targ, const char *src) with_stripctrl(santarg, act.name) { tell_user(stderr, "warning: remote host sent a" " compound pathname '%s'", sanname); - tell_user(stderr, " renaming local", + tell_user(stderr, " renaming local" " file to '%s'", santarg); } } diff --git a/ssh.c b/ssh.c index 417d6c11..719d9e61 100644 --- a/ssh.c +++ b/ssh.c @@ -562,7 +562,7 @@ void ssh_deferred_abort_callback(void *vctx) Ssh *ssh = (Ssh *)vctx; char *msg = ssh->deferred_abort_message; ssh->deferred_abort_message = NULL; - ssh_sw_abort(ssh, msg); + ssh_sw_abort(ssh, "%s", msg); sfree(msg); } diff --git a/ssh2connection.c b/ssh2connection.c index 28054d43..a8c20f2d 100644 --- a/ssh2connection.c +++ b/ssh2connection.c @@ -750,7 +750,8 @@ static bool ssh2_connection_filter_queue(struct ssh2_connection_state *s) "Received %s for channel %d with no outstanding " "channel request", ssh2_pkt_type(s->ppl.bpp->pls->kctx, - s->ppl.bpp->pls->actx, pktin->type)); + s->ppl.bpp->pls->actx, pktin->type), + c->localid); return true; } ocr->handler(c, pktin, ocr->ctx); diff --git a/sshhmac.c b/sshhmac.c index 232d7680..29db9fbf 100644 --- a/sshhmac.c +++ b/sshhmac.c @@ -44,7 +44,7 @@ static ssh2_mac *hmac_new(const ssh2_macalg *alg, ssh_cipher *cipher) ctx->digest = snewn(ctx->hashalg->hlen, uint8_t); ctx->text_name = strbuf_new(); - strbuf_catf(ctx->text_name, "HMAC-%s", + strbuf_catf(ctx->text_name, "HMAC-%s%s", ctx->hashalg->text_basename, extra->suffix); if (extra->annotation || ctx->hashalg->annotation) { strbuf_catf(ctx->text_name, " ("); diff --git a/sshrsa.c b/sshrsa.c index 81522aa8..504a73e7 100644 --- a/sshrsa.c +++ b/sshrsa.c @@ -296,7 +296,7 @@ char *rsa_ssh1_fingerprint(RSAKey *key) ssh_hash_final(hash, digest); out = strbuf_new(); - strbuf_catf(out, "%d ", mp_get_nbits(key->modulus)); + strbuf_catf(out, "%zu ", mp_get_nbits(key->modulus)); for (i = 0; i < 16; i++) strbuf_catf(out, "%s%02x", i ? ":" : "", digest[i]); if (key->comment) diff --git a/sshshare.c b/sshshare.c index 98071d8e..573114cd 100644 --- a/sshshare.c +++ b/sshshare.c @@ -1794,8 +1794,9 @@ static void share_receive(Plug *plug, int urgent, const char *data, size_t len) } if (cs->recvlen > 0 && cs->recvbuf[cs->recvlen-1] == '\015') cs->recvlen--; /* trim off \r before \n */ + ptrlen verstring = make_ptrlen(cs->recvbuf, cs->recvlen); log_downstream(cs, "Downstream version string: %.*s", - cs->recvlen, cs->recvbuf); + PTRLEN_PRINTF(verstring)); cs->got_verstring = true; /* From 8453b9239c0961c9d988fcd1164400b5ceb284ad Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 9 Feb 2020 08:22:56 +0000 Subject: [PATCH 24/62] New wrapper macro for printf("%zu"), for old VS compat. A user reports that Visual Studio 2013 and earlier have printf implementations in their C library that don't support the 'z' modifier to indicate that an integer argument is size_t. The 'I' modifier apparently works in place of it. To avoid littering ifdefs everywhere, I've invented my own inttypes.h style macros to wrap size_t formatting directives, which are defined to %zu and %zx normally, or %Iu and %Ix in old-VS mode. Those are in defs.h, and they're used everywhere that a %z might otherwise get into the Windows build. (cherry picked from commit 82a7e8c4ac25a13011e1b23f04faba7a52258e2a) --- defs.h | 7 +++++++ logging.c | 7 ++++--- sshprng.c | 8 ++++---- sshpubk.c | 4 ++-- sshrsa.c | 4 ++-- testcrypt.c | 2 +- testsc.c | 4 ++-- windows/winpgnt.c | 2 +- 8 files changed, 23 insertions(+), 15 deletions(-) diff --git a/defs.h b/defs.h index 0a4f08c8..b9577036 100644 --- a/defs.h +++ b/defs.h @@ -22,9 +22,16 @@ #define PRIdMAX "I64d" #define PRIXMAX "I64X" #define SCNu64 "I64u" +#define SIZEx "Ix" +#define SIZEu "Iu" uintmax_t strtoumax(const char *nptr, char **endptr, int base); #else #include +/* Because we still support older MSVC libraries which don't recognise the + * standard C "z" modifier for size_t-sized integers, we must use an + * inttypes.h-style macro for those */ +#define SIZEx "zx" +#define SIZEu "zu" #endif #if defined __GNUC__ || defined __clang__ diff --git a/logging.c b/logging.c index d61038c9..a12caa5a 100644 --- a/logging.c +++ b/logging.c @@ -344,7 +344,7 @@ void log_packet(LogContext *ctx, int direction, int type, /* If we're about to stop omitting, it's time to say how * much we omitted. */ if ((blktype != PKTLOG_OMIT) && omitted) { - logprintf(ctx, " (%zu byte%s omitted)\r\n", + logprintf(ctx, " (%"SIZEu" byte%s omitted)\r\n", omitted, (omitted==1?"":"s")); omitted = 0; } @@ -352,7 +352,8 @@ void log_packet(LogContext *ctx, int direction, int type, /* (Re-)initialise dumpdata as necessary * (start of row, or if we've just stopped omitting) */ if (!output_pos && !omitted) - sprintf(dumpdata, " %08zx%*s\r\n", p-(p%16), 1+3*16+2+16, ""); + sprintf(dumpdata, " %08"SIZEx"%*s\r\n", + p-(p%16), 1+3*16+2+16, ""); /* Deal with the current byte. */ if (blktype == PKTLOG_OMIT) { @@ -387,7 +388,7 @@ void log_packet(LogContext *ctx, int direction, int type, /* Tidy up */ if (omitted) - logprintf(ctx, " (%zu byte%s omitted)\r\n", + logprintf(ctx, " (%"SIZEu" byte%s omitted)\r\n", omitted, (omitted==1?"":"s")); logflush(ctx); } diff --git a/sshprng.c b/sshprng.c index ea40c99f..e333e836 100644 --- a/sshprng.c +++ b/sshprng.c @@ -177,7 +177,7 @@ static void prng_seed_BinarySink_write( prng *pr = BinarySink_DOWNCAST(bs, prng); prng_impl *pi = container_of(pr, prng_impl, Prng); assert(pi->keymaker); - prngdebug("prng: got %zu bytes of seed\n", len); + prngdebug("prng: got %"SIZEu" bytes of seed\n", len); put_data(pi->keymaker, data, len); } @@ -228,7 +228,7 @@ void prng_read(prng *pr, void *vout, size_t size) assert(!pi->keymaker); - prngdebug("prng_read %zu\n", size); + prngdebug("prng_read %"SIZEu"\n", size); uint8_t *out = (uint8_t *)vout; for (; size > 0; size--) { @@ -256,7 +256,7 @@ void prng_add_entropy(prng *pr, unsigned source_id, ptrlen data) index++; } - prngdebug("prng_add_entropy source=%u size=%zu -> collector %zi\n", + prngdebug("prng_add_entropy source=%u size=%"SIZEu" -> collector %zi\n", source_id, data.len, index); put_datapl(pi->collectors[index], data); @@ -272,7 +272,7 @@ void prng_add_entropy(prng *pr, unsigned source_id, ptrlen data) uint32_t reseed_index = ++pi->reseeds; prngdebug("prng entropy reseed #%"PRIu32"\n", reseed_index); for (size_t i = 0; i < NCOLLECTORS; i++) { - prngdebug("emptying collector %zu\n", i); + prngdebug("emptying collector %"SIZEu"\n", i); ssh_hash_final(pi->collectors[i], pi->pending_output); put_data(&pi->Prng, pi->pending_output, pi->hashalg->hlen); pi->collectors[i] = ssh_hash_new(pi->hashalg); diff --git a/sshpubk.c b/sshpubk.c index c9423192..6ad84788 100644 --- a/sshpubk.c +++ b/sshpubk.c @@ -1362,8 +1362,8 @@ char *ssh1_pubkey_str(RSAKey *key) dec1 = mp_get_decimal(key->exponent); dec2 = mp_get_decimal(key->modulus); - buffer = dupprintf("%zd %s %s%s%s", mp_get_nbits(key->modulus), dec1, dec2, - key->comment ? " " : "", + buffer = dupprintf("%"SIZEu" %s %s%s%s", mp_get_nbits(key->modulus), + dec1, dec2, key->comment ? " " : "", key->comment ? key->comment : ""); sfree(dec1); sfree(dec2); diff --git a/sshrsa.c b/sshrsa.c index 504a73e7..23330620 100644 --- a/sshrsa.c +++ b/sshrsa.c @@ -296,7 +296,7 @@ char *rsa_ssh1_fingerprint(RSAKey *key) ssh_hash_final(hash, digest); out = strbuf_new(); - strbuf_catf(out, "%zu ", mp_get_nbits(key->modulus)); + strbuf_catf(out, "%"SIZEu" ", mp_get_nbits(key->modulus)); for (i = 0; i < 16; i++) strbuf_catf(out, "%s%02x", i ? ":" : "", digest[i]); if (key->comment) @@ -779,7 +779,7 @@ char *rsa2_invalid(ssh_key *key, unsigned flags) const ssh_hashalg *halg = rsa2_hash_alg_for_flags(flags, &sign_alg_name); if (nbytes < rsa_pkcs1_length_of_fixed_parts(halg)) { return dupprintf( - "%zu-bit RSA key is too short to generate %s signatures", + "%"SIZEu"-bit RSA key is too short to generate %s signatures", bits, sign_alg_name); } diff --git a/testcrypt.c b/testcrypt.c index c92311f4..aa41a7a5 100644 --- a/testcrypt.c +++ b/testcrypt.c @@ -1115,7 +1115,7 @@ int main(int argc, char **argv) for (size_t i = 0; i < sb->len; i++) if (sb->s[i] == '\n') lines++; - fprintf(outfp, "%zu\n%s", lines, sb->s); + fprintf(outfp, "%"SIZEu"\n%s", lines, sb->s); fflush(outfp); strbuf_free(sb); sfree(line); diff --git a/testsc.c b/testsc.c index 760a9482..7127032e 100644 --- a/testsc.c +++ b/testsc.c @@ -160,7 +160,7 @@ VOLATILE_WRAPPED_DEFN(, void, log_to_file, (const char *filename)) static const char *outdir = NULL; char *log_filename(const char *basename, size_t index) { - return dupprintf("%s/%s.%04zu", outdir, basename, index); + return dupprintf("%s/%s.%04"SIZEu, outdir, basename, index); } static char *last_filename; @@ -1574,7 +1574,7 @@ int main(int argc, char **argv) printf("All tests passed\n"); return 0; } else { - printf("%zu tests failed\n", nrun - npass); + printf("%"SIZEu" tests failed\n", nrun - npass); return 1; } } diff --git a/windows/winpgnt.c b/windows/winpgnt.c index 4374f2fe..4c4ec6af 100644 --- a/windows/winpgnt.c +++ b/windows/winpgnt.c @@ -884,7 +884,7 @@ static char *answer_filemapping_message(const char *mapname) mapsize = mbi.RegionSize; } #ifdef DEBUG_IPC - debug("region size = %zd\n", mapsize); + debug("region size = %"SIZEu"\n", mapsize); #endif if (mapsize < 5) { err = dupstr("mapping smaller than smallest possible request"); From 14c6ddca6373d701ef156cb6d5f5008ccd06c83e Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 26 Jan 2020 14:39:40 +0000 Subject: [PATCH 25/62] Fix misplaced parens in window.c. This was pointed out as a compiler warning when I test-built with up-to-date clang-cl. It looks as if it would cause the IDM_FULLSCREEN item on the system menu to be wrongly greyed/ungreyed, but in fact I think it's benign, because MF_BYCOMMAND == 0. So it's _just_ a warning fix, luckily! (cherry picked from commit 213723a718fdf874418eea734bf1016518830a71) --- windows/window.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/windows/window.c b/windows/window.c index a17a9854..b5c730eb 100644 --- a/windows/window.c +++ b/windows/window.c @@ -2369,8 +2369,8 @@ static LRESULT CALLBACK WndProc(HWND hwnd, UINT message, for (i = 0; i < lenof(popup_menus); i++) EnableMenuItem(popup_menus[i].menu, IDM_FULLSCREEN, MF_BYCOMMAND | - (resize_action == RESIZE_DISABLED) - ? MF_GRAYED : MF_ENABLED); + (resize_action == RESIZE_DISABLED + ? MF_GRAYED : MF_ENABLED)); /* Gracefully unzoom if necessary */ if (IsZoomed(hwnd) && (resize_action == RESIZE_DISABLED)) ShowWindow(hwnd, SW_RESTORE); From f7a3280e271629fd5f6c048cf17d5bbba3ea1616 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 26 Jan 2020 16:46:16 +0000 Subject: [PATCH 26/62] Fix text name of hmac-sha1-96-buggy. I carefully set up separate mechanisms for the "-96" suffix on the hash name and the "bug-compatible" in parens after it, so that the latter could share its parens with annotations from the underlying hash. And then I forgot to _use_ the second mechanism! Also added ssh2_mac_text_name to the testcrypt API so I could check it easily. The result before this fix: >>> ssh2_mac_text_name(ssh2_mac_new("hmac_sha1_96_buggy", None)) 'HMAC-SHA-1-96 (bug-compatible) (unaccelerated)' And after, which is what I intended all along: >>> ssh2_mac_text_name(ssh2_mac_new("hmac_sha1_96_buggy", None)) 'HMAC-SHA-1-96 (bug-compatible, unaccelerated)' (cherry picked from commit 600bf247d304fc18c9d6b3ec0d18c609253d52a9) --- sshhmac.c | 4 ++-- testcrypt.h | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/sshhmac.c b/sshhmac.c index 29db9fbf..2f870f36 100644 --- a/sshhmac.c +++ b/sshhmac.c @@ -222,7 +222,7 @@ const ssh2_macalg ssh_hmac_sha1_96 = { }; const struct hmac_extra ssh_hmac_sha1_buggy_extra = { - &ssh_sha1, " (bug-compatible)" + &ssh_sha1, "", "bug-compatible" }; const ssh2_macalg ssh_hmac_sha1_buggy = { @@ -233,7 +233,7 @@ const ssh2_macalg ssh_hmac_sha1_buggy = { }; const struct hmac_extra ssh_hmac_sha1_96_buggy_extra = { - &ssh_sha1, "-96 (bug-compatible)" + &ssh_sha1, "-96", "bug-compatible" }; const ssh2_macalg ssh_hmac_sha1_96_buggy = { diff --git a/testcrypt.h b/testcrypt.h index 98a1daf0..690bfa2b 100644 --- a/testcrypt.h +++ b/testcrypt.h @@ -136,6 +136,7 @@ FUNC2(void, ssh2_mac_setkey, val_mac, val_string_ptrlen) FUNC1(void, ssh2_mac_start, val_mac) FUNC2(void, ssh2_mac_update, val_mac, val_string_ptrlen) FUNC1(val_string, ssh2_mac_genresult, val_mac) +FUNC1(val_string_asciz_const, ssh2_mac_text_name, val_mac) /* * The ssh_key abstraction. All the uses of BinarySink and From e564a5f05d4411ad80a3e3e219e9e906384171fa Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 27 Jan 2020 19:40:58 +0000 Subject: [PATCH 27/62] Fix a memory leak in ssh1_channel_close_local. Leak Sanitiser was kind enough to point this out to me during testing of the port forwarding rework: chan_log_close_msg() returns a dynamically allocated char *, which the caller is supposed to free. (cherry picked from commit 22350d76685fa61c928a75c951e1fe7890817ed9) --- ssh1connection.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ssh1connection.c b/ssh1connection.c index 01df6e52..c7396166 100644 --- a/ssh1connection.c +++ b/ssh1connection.c @@ -520,10 +520,12 @@ static void ssh1_channel_close_local(struct ssh1_channel *c, { struct ssh1_connection_state *s = c->connlayer; PacketProtocolLayer *ppl = &s->ppl; /* for ppl_logevent */ - const char *msg = chan_log_close_msg(c->chan); + char *msg = chan_log_close_msg(c->chan); - if (msg != NULL) + if (msg != NULL) { ppl_logevent("%s%s%s", msg, reason ? " " : "", reason ? reason : ""); + sfree(msg); + } chan_free(c->chan); c->chan = zombiechan_new(); From 964058b5efde468c7490ea066de43b2124370d98 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Wed, 29 Jan 2020 06:13:41 +0000 Subject: [PATCH 28/62] Make prototype for new_prompts() consistent. In commit b4c8fd9d8 which introduced the Seat trait, I got a bit confused about the prototype of new_prompts(). Previously it took a 'Frontend *' parameter; I edited the call sites to pass a 'Seat *' instead, but the actual function definition takes no parameters at all - and rightly so, because the 'Frontend *' inside the prompts_t has been removed and _not_ replaced with a 'Seat *', so the constructor would have nothing to do with such a thing anyway. But I wrote the function declaration in putty.h with '()' rather than '(void)' (too much time spent in C++), and so the compiler never spotted the mismatch. Now new_prompts() is consistently nullary everywhere it appears: the prototype in the header is a proper (void) one, and the call sites have been modified to not pointlessly give it a Seat or null pointer. (cherry picked from commit d183484742670f09d5963505ff9fef5e314a4c26) --- cmdgen.c | 2 +- putty.h | 2 +- ssh1login.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cmdgen.c b/cmdgen.c index b391bd1d..4bb906f7 100644 --- a/cmdgen.c +++ b/cmdgen.c @@ -903,7 +903,7 @@ int main(int argc, char **argv) * we have just generated a key. */ if (!new_passphrase && (change_passphrase || keytype != NOKEYGEN)) { - prompts_t *p = new_prompts(NULL); + prompts_t *p = new_prompts(); int ret; p->to_server = false; diff --git a/putty.h b/putty.h index 1498370e..33835582 100644 --- a/putty.h +++ b/putty.h @@ -667,7 +667,7 @@ typedef struct { void *data; /* slot for housekeeping data, managed by * seat_get_userpass_input(); initially NULL */ } prompts_t; -prompts_t *new_prompts(); +prompts_t *new_prompts(void); void add_prompt(prompts_t *p, char *promptstr, bool echo); void prompt_set_result(prompt_t *pr, const char *newstr); char *prompt_get_result(prompt_t *pr); diff --git a/ssh1login.c b/ssh1login.c index f14068e4..c129b866 100644 --- a/ssh1login.c +++ b/ssh1login.c @@ -648,7 +648,7 @@ static void ssh1_login_process_queue(PacketProtocolLayer *ppl) ppl_printf("No passphrase required.\r\n"); passphrase = NULL; } else { - s->cur_prompt = new_prompts(s->ppl.seat); + s->cur_prompt = new_prompts(); s->cur_prompt->to_server = false; s->cur_prompt->from_server = false; s->cur_prompt->name = dupstr("SSH key passphrase"); @@ -787,7 +787,7 @@ static void ssh1_login_process_queue(PacketProtocolLayer *ppl) /* * Otherwise, try various forms of password-like authentication. */ - s->cur_prompt = new_prompts(s->ppl.seat); + s->cur_prompt = new_prompts(); if (conf_get_bool(s->conf, CONF_try_tis_auth) && (s->supported_auths_mask & (1 << SSH1_AUTH_TIS)) && From 8c227b0cc0f95dbe7937ea635c914d0a0abe2e44 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Wed, 29 Jan 2020 06:13:41 +0000 Subject: [PATCH 29/62] Fix misdef of platform_get_x11_unix_address on Windows. Similarly to the previous commit, this function had an inconsistent parameter list between Unix and Windows, because the Windows source file that defines it (winnet.c) didn't include ssh.h where its prototype lives, so the compiler never checked. Luckily, the discrepancy was that the Windows version of the function was declared as taking an extra parameter which it ignored, so the fix is very easy. (cherry picked from commit b7f011aed74a6f0442571da4f15c794c7adff99c) --- windows/winnet.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/windows/winnet.c b/windows/winnet.c index ab38e547..16648d56 100644 --- a/windows/winnet.c +++ b/windows/winnet.c @@ -1817,8 +1817,7 @@ char *get_hostname(void) return dupstr(hostbuf); } -SockAddr *platform_get_x11_unix_address(const char *display, int displaynum, - char **canonicalname) +SockAddr *platform_get_x11_unix_address(const char *display, int displaynum) { SockAddr *ret = snew(SockAddr); memset(ret, 0, sizeof(SockAddr)); From 5b09e4c250d2cf77f5fecbaa6284fd8a100984f4 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Wed, 29 Jan 2020 06:13:41 +0000 Subject: [PATCH 30/62] Fix technical-UB uses of the preprocessor. A recent test-compile at high warning level points out that if you define a macro with a ... at the end of the parameter list, then every call should at least include the comma before the variadic part. That is, if you #define MACRO(x,y,...) then you shouldn't call MACRO(1,2) with no comma after the 2. But that's what I had done in one of my definitions of FUNC0 in the fiddly testcrypt system. In a similar vein, it's a mistake to use the preprocessor 'defined' operator when it's expanded from another macro. Adjusted the setup of BB_OK in mpint_i.h to avoid doing that. (Neither of these has yet caused a problem in any real compile, but best to fix them before they do.) (cherry picked from commit f40d31b5ccb6b771d516916294cf75eac3f15ed5) --- mpint_i.h | 6 +++++- testcrypt.c | 34 +++++++++++++++++----------------- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/mpint_i.h b/mpint_i.h index 70fdc6e1..23f6dc9d 100644 --- a/mpint_i.h +++ b/mpint_i.h @@ -59,7 +59,11 @@ /* You can lower the BignumInt size by defining BIGNUM_OVERRIDE on the * command line to be your chosen max value of BIGNUM_INT_BITS_BITS */ -#define BB_OK(b) (!defined BIGNUM_OVERRIDE || BIGNUM_OVERRIDE >= b) +#if defined BIGNUM_OVERRIDE +#define BB_OK(b) ((b) <= BIGNUM_OVERRIDE) +#else +#define BB_OK(b) (1) +#endif #if defined __SIZEOF_INT128__ && BB_OK(6) diff --git a/testcrypt.c b/testcrypt.c index aa41a7a5..fde77328 100644 --- a/testcrypt.c +++ b/testcrypt.c @@ -1001,30 +1001,28 @@ static void process_line(BinarySource *in, strbuf *out) { ptrlen id = get_word(in); -#define DISPATCH_COMMAND(cmd) \ - if (ptrlen_eq_string(id, #cmd)) { \ - handle_##cmd(in, out); \ - return; \ - } +#define DISPATCH_INTERNAL(cmdname, handler) do { \ + if (ptrlen_eq_string(id, cmdname)) { \ + handler(in, out); \ + return; \ + } \ + } while (0) + +#define DISPATCH_COMMAND(cmd) DISPATCH_INTERNAL(#cmd, handle_##cmd) DISPATCH_COMMAND(hello); DISPATCH_COMMAND(free); DISPATCH_COMMAND(newstring); DISPATCH_COMMAND(getstring); DISPATCH_COMMAND(mp_literal); DISPATCH_COMMAND(mp_dump); +#undef DISPATCH_COMMAND -#define FUNC(rettype, function, ...) \ - if (ptrlen_eq_string(id, #function)) { \ - handle_##function(in, out); \ - return; \ - } - -#define FUNC0 FUNC -#define FUNC1 FUNC -#define FUNC2 FUNC -#define FUNC3 FUNC -#define FUNC4 FUNC -#define FUNC5 FUNC +#define FUNC0(ret,func) DISPATCH_INTERNAL(#func, handle_##func); +#define FUNC1(ret,func,x) DISPATCH_INTERNAL(#func, handle_##func); +#define FUNC2(ret,func,x,y) DISPATCH_INTERNAL(#func, handle_##func); +#define FUNC3(ret,func,x,y,z) DISPATCH_INTERNAL(#func, handle_##func); +#define FUNC4(ret,func,x,y,z,v) DISPATCH_INTERNAL(#func, handle_##func); +#define FUNC5(ret,func,x,y,z,v,w) DISPATCH_INTERNAL(#func, handle_##func); #include "testcrypt.h" @@ -1035,6 +1033,8 @@ static void process_line(BinarySource *in, strbuf *out) #undef FUNC1 #undef FUNC0 +#undef DISPATCH_INTERNAL + fatal_error("command '%.*s': unrecognised", PTRLEN_PRINTF(id)); } From fe732487ade7a77f90f3b07d772593722367b77b Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Thu, 30 Jan 2020 06:40:21 +0000 Subject: [PATCH 31/62] Fix two accidental overwrites of 'flags'. When I came to actually remove the global 'flags' word, I found that I got compile failures in two functions that should never have been accessing it at all, because they forgot to declare _local_ variables of the same name. Yikes! (Of course, _now_ that's harmless, because I've just removed all the actual semantics from the global variable. But I'm about to remove the variable too, so these bugs would become compile failures.) (cherry picked from commit 33715c07e342248c5dfbfb38ad30707566453cc4) --- sftpserver.c | 1 + unix/gtkfont.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/sftpserver.c b/sftpserver.c index 1ff3d378..91711d59 100644 --- a/sftpserver.c +++ b/sftpserver.c @@ -15,6 +15,7 @@ struct sftp_packet *sftp_handle_request( { struct sftp_packet *reply; unsigned id; + uint32_t flags; ptrlen path, dstpath, handle, data; uint64_t offset; unsigned length; diff --git a/unix/gtkfont.c b/unix/gtkfont.c index 4e62fb34..f0e7e712 100644 --- a/unix/gtkfont.c +++ b/unix/gtkfont.c @@ -1107,7 +1107,7 @@ static void x11font_enum_fonts(GtkWidget *widget, * font, which we do by examining the spacing field * again. */ - flags = FONTFLAG_SERVERSIDE; + int flags = FONTFLAG_SERVERSIDE; if (!strchr("CcMm", xlfd->spacing[0])) flags |= FONTFLAG_NONMONOSPACED; From 414d35a50872f53bca0ac7d62d9bf9f16da17716 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 9 Feb 2020 08:23:24 +0000 Subject: [PATCH 32/62] Change PSCP's default protocol to SSH. Apparently it's been set on Telnet for the entire lifetime of PSCP. It can't have caused any trouble, or we'd have noticed by now, but it still seems silly to set it to something that PSCP clearly can't handle! (cherry picked from commit 6f0adb243a2bd30b1f8d608d6bc00b94c3f33f3d) --- pscp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pscp.c b/pscp.c index cadf7916..3ba0aaaf 100644 --- a/pscp.c +++ b/pscp.c @@ -2231,7 +2231,7 @@ int psftp_main(int argc, char *argv[]) int i; bool sanitise_stderr = true; - default_protocol = PROT_TELNET; + default_protocol = PROT_SSH; flags = 0 #ifdef FLAG_SYNCAGENT From 92c1f31569f6b60facdef6782652a0380dc86bda Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 9 Feb 2020 08:23:48 +0000 Subject: [PATCH 33/62] cgtest: add missing \n in an error message. (cherry picked from commit c25dc9c2fd4ee90180a3c62a983bbd1726338ccd) --- cmdgen.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cmdgen.c b/cmdgen.c index 4bb906f7..f6a3d08d 100644 --- a/cmdgen.c +++ b/cmdgen.c @@ -1336,7 +1336,8 @@ int main(int argc, char **argv) pubfilename, tmpfilename1); if (system(cmdbuf) || (fp = get_fp(tmpfilename1)) == NULL) { - printf("UNABLE to test fingerprint matching against OpenSSH"); + printf("UNABLE to test fingerprint matching against " + "OpenSSH\n"); } sfree(cmdbuf); } From 6864bcddbb0ed12a76acff1024c730880a784f44 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 2 Feb 2020 18:53:15 +0000 Subject: [PATCH 34/62] userauth: fix two small memory leaks. Happened to notice these while I was testing the last few commits. (cherry picked from commit 84fa07cfebba2e82738568427d9feb55f782d7cd) --- ssh2userauth.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ssh2userauth.c b/ssh2userauth.c index a913aff2..da506362 100644 --- a/ssh2userauth.c +++ b/ssh2userauth.c @@ -782,6 +782,7 @@ static void ssh2_userauth_process_queue(PacketProtocolLayer *ppl) ppl_printf("Pageant failed to " "provide a signature\r\n"); s->suppress_wait_for_response_packet = true; + ssh_free_pktout(s->pktout); } } } @@ -1334,6 +1335,8 @@ static void ssh2_userauth_process_queue(PacketProtocolLayer *ppl) } if (sb->len) s->cur_prompt->instruction = strbuf_to_str(sb); + else + strbuf_free(sb); /* * Our prompts_t is fully constructed now. Get the From 35cc7b1cb6df8d6e9c478bb87d33e9a28538dfa1 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 2 Feb 2020 18:53:24 +0000 Subject: [PATCH 35/62] userauth: fill in missing error path when agent goes away. If the agent client code doesn't even manage to read a full response message at all (for example, because the agent it's talking to is Pageant running in debug mode and you just ^Ced it or it crashed, which is what's been happening to me all afternoon), then previously, the userauth code would loop back round to the top of the main loop without having actually sent any request, so the client code would deadlock waiting for a response to nothing. (cherry picked from commit 563cb062b801fd447f5a5b04b9161890d8966e4a) --- ssh2userauth.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ssh2userauth.c b/ssh2userauth.c index da506362..62c4b409 100644 --- a/ssh2userauth.c +++ b/ssh2userauth.c @@ -784,6 +784,13 @@ static void ssh2_userauth_process_queue(PacketProtocolLayer *ppl) s->suppress_wait_for_response_packet = true; ssh_free_pktout(s->pktout); } + } else { + ppl_logevent("Pageant failed to respond to " + "signing request"); + ppl_printf("Pageant failed to " + "respond to signing request\r\n"); + s->suppress_wait_for_response_packet = true; + ssh_free_pktout(s->pktout); } } From 7c3778ad67aa6ae1d3c6d8c9de08c4298daa82f6 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Wed, 5 Feb 2020 19:32:22 +0000 Subject: [PATCH 36/62] Track the total size of every PacketQueue. The queue-node structure shared between PktIn and PktOut now has a 'formal_size' field, which is initialised appropriately by the various packet constructors. And the PacketQueue structure has a 'total_size' field which tracks the sum of the formal sizes of all the packets on the queue, and is automatically updated by the push, pop and concatenate functions. No functional change, and nothing uses the new fields yet: this is infrastructure that will be used in the next commit. (cherry picked from commit 0ff13ae77329a906d5ce5dbcf534a071eedd69f3) --- ssh.h | 2 ++ ssh1bpp.c | 1 + ssh2bpp-bare.c | 1 + ssh2bpp.c | 1 + sshcommon.c | 35 +++++++++++++++++++++++++++++++---- 5 files changed, 36 insertions(+), 4 deletions(-) diff --git a/ssh.h b/ssh.h index d3c31c8f..06d53528 100644 --- a/ssh.h +++ b/ssh.h @@ -52,6 +52,7 @@ struct ssh_channel; typedef struct PacketQueueNode PacketQueueNode; struct PacketQueueNode { PacketQueueNode *next, *prev; + size_t formal_size; /* contribution to PacketQueueBase's total_size */ bool on_free_queue; /* is this packet scheduled for freeing? */ }; @@ -84,6 +85,7 @@ typedef struct PktOut { typedef struct PacketQueueBase { PacketQueueNode end; + size_t total_size; /* sum of all formal_size fields on the queue */ struct IdempotentCallback *ic; } PacketQueueBase; diff --git a/ssh1bpp.c b/ssh1bpp.c index 0bc729e7..2fd684c5 100644 --- a/ssh1bpp.c +++ b/ssh1bpp.c @@ -236,6 +236,7 @@ static void ssh1_bpp_handle_input(BinaryPacketProtocol *bpp) NULL, 0, NULL); } + s->pktin->qnode.formal_size = get_avail(s->pktin); pq_push(&s->bpp.in_pq, s->pktin); { diff --git a/ssh2bpp-bare.c b/ssh2bpp-bare.c index 8dc3b117..dd2f918a 100644 --- a/ssh2bpp-bare.c +++ b/ssh2bpp-bare.c @@ -129,6 +129,7 @@ static void ssh2_bare_bpp_handle_input(BinaryPacketProtocol *bpp) continue; } + s->pktin->qnode.formal_size = get_avail(s->pktin); pq_push(&s->bpp.in_pq, s->pktin); s->pktin = NULL; } diff --git a/ssh2bpp.c b/ssh2bpp.c index 79b97b31..97179ca9 100644 --- a/ssh2bpp.c +++ b/ssh2bpp.c @@ -589,6 +589,7 @@ static void ssh2_bpp_handle_input(BinaryPacketProtocol *bpp) continue; } + s->pktin->qnode.formal_size = get_avail(s->pktin); pq_push(&s->bpp.in_pq, s->pktin); { diff --git a/sshcommon.c b/sshcommon.c index 26073843..0d56460f 100644 --- a/sshcommon.c +++ b/sshcommon.c @@ -35,6 +35,7 @@ void pq_base_push(PacketQueueBase *pqb, PacketQueueNode *node) node->prev = pqb->end.prev; node->next->prev = node; node->prev->next = node; + pqb->total_size += node->formal_size; if (pqb->ic) queue_idempotent_callback(pqb->ic); @@ -47,6 +48,7 @@ void pq_base_push_front(PacketQueueBase *pqb, PacketQueueNode *node) node->next = pqb->end.next; node->next->prev = node; node->prev->next = node; + pqb->total_size += node->formal_size; if (pqb->ic) queue_idempotent_callback(pqb->ic); @@ -72,6 +74,23 @@ static IdempotentCallback ic_pktin_free = { pktin_free_queue_callback, NULL, false }; +static inline void pq_unlink_common(PacketQueueBase *pqb, + PacketQueueNode *node) +{ + node->next->prev = node->prev; + node->prev->next = node->next; + + /* Check total_size doesn't drift out of sync downwards, by + * ensuring it doesn't underflow when we do this subtraction */ + assert(pqb->total_size >= node->formal_size); + pqb->total_size -= node->formal_size; + + /* Check total_size doesn't drift out of sync upwards, by checking + * that it's returned to exactly zero whenever a queue is + * emptied */ + assert(pqb->end.next != &pqb->end || pqb->total_size == 0); +} + static PktIn *pq_in_after(PacketQueueBase *pqb, PacketQueueNode *prev, bool pop) { @@ -80,14 +99,14 @@ static PktIn *pq_in_after(PacketQueueBase *pqb, return NULL; if (pop) { - node->next->prev = node->prev; - node->prev->next = node->next; + pq_unlink_common(pqb, node); node->prev = pktin_freeq_head.prev; node->next = &pktin_freeq_head; node->next->prev = node; node->prev->next = node; node->on_free_queue = true; + queue_idempotent_callback(&ic_pktin_free); } @@ -102,8 +121,8 @@ static PktOut *pq_out_after(PacketQueueBase *pqb, return NULL; if (pop) { - node->next->prev = node->prev; - node->prev->next = node->next; + pq_unlink_common(pqb, node); + node->prev = node->next = NULL; } @@ -115,6 +134,7 @@ void pq_in_init(PktInQueue *pq) pq->pqb.ic = NULL; pq->pqb.end.next = pq->pqb.end.prev = &pq->pqb.end; pq->after = pq_in_after; + pq->pqb.total_size = 0; } void pq_out_init(PktOutQueue *pq) @@ -122,6 +142,7 @@ void pq_out_init(PktOutQueue *pq) pq->pqb.ic = NULL; pq->pqb.end.next = pq->pqb.end.prev = &pq->pqb.end; pq->after = pq_out_after; + pq->pqb.total_size = 0; } void pq_in_clear(PktInQueue *pq) @@ -153,6 +174,8 @@ void pq_base_concatenate(PacketQueueBase *qdest, { struct PacketQueueNode *head1, *tail1, *head2, *tail2; + size_t total_size = q1->total_size + q2->total_size; + /* * Extract the contents from both input queues, and empty them. */ @@ -164,6 +187,7 @@ void pq_base_concatenate(PacketQueueBase *qdest, q1->end.next = q1->end.prev = &q1->end; q2->end.next = q2->end.prev = &q2->end; + q1->total_size = q2->total_size = 0; /* * Link the two lists together, handling the case where one or @@ -206,6 +230,8 @@ void pq_base_concatenate(PacketQueueBase *qdest, if (qdest->ic) queue_idempotent_callback(qdest->ic); } + + qdest->total_size = total_size; } /* ---------------------------------------------------------------------- @@ -235,6 +261,7 @@ static void ssh_pkt_adddata(PktOut *pkt, const void *data, int len) sgrowarrayn_nm(pkt->data, pkt->maxlen, pkt->length, len); memcpy(pkt->data + pkt->length, data, len); pkt->length += len; + pkt->qnode.formal_size = pkt->length; } static void ssh_pkt_BinarySink_write(BinarySink *bs, From ee8baee4fab55ac10c8f2b2b92aa5979f5b24da4 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Wed, 5 Feb 2020 19:34:29 +0000 Subject: [PATCH 37/62] Account for packet queues in ssh_sendbuffer(). Ever since I reworked the SSH code to have multiple internal packet queues, there's been a long-standing FIXME in ssh_sendbuffer() saying that we ought to include the data buffered in those queues as part of reporting how much data is buffered on standard input. Recently a user reported that 'proftpd', or rather its 'mod_sftp' add-on that implements an SFTP-only SSH server, exposes a bug related to that missing piece of code. The xfer_upload system in sftp.c starts by pushing SFTP write messages into the SSH code for as long as sftp_sendbuffer() (which ends up at ssh_sendbuffer()) reports that not too much data is buffered locally. In fact what happens is that all those messages end up on the packet queues between SSH protocol layers, so they're not counted by sftp_sendbuffer(), so we just keep going until there's some other reason to stop. Usually the reason we stop is because we've filled up the SFTP channel's SSH-layer window, so we need the server to send us a WINDOW_ADJUST before we're allowed to send any more data. So we return to the main event loop and start waiting for reply packets. And when the window is moderate (e.g. OpenSSH currently seems to present about 2MB), this isn't really noticeable. But proftpd presents the maximum-size window of 2^32-1 bytes, and as a result we just keep shovelling more and more packets into the internal packet queues until PSFTP has grown to 4GB in size, and only then do we even return to the event loop and start actually sending them down the network. Moreover, this happens again at rekey time, because while a rekey is in progress, ssh2transport stops emptying the queue of outgoing packets sent by its higher layer - so, again, everything just keeps buffering up somewhere that sftp_sendbuffer can't see it. But this commit fixes it! Each PacketProtocolLayer now provides a vtable method for asking how much data it currently has queued. Most of them share a default implementation which just returns the newly added total_size field from their pq_out; the exception is ssh2transport, which also has to account for data queued in its higher layer. And ssh_sendbuffer() adds that on to the quantity it already knew about in other locations, to give a more realistic idea of the currently buffered data. (cherry picked from commit cd97b7e7ea37ee146a2797350f61d77f66dd3d8b) --- ssh.c | 3 ++- ssh1connection.c | 1 + ssh1login-server.c | 1 + ssh1login.c | 1 + ssh2connection.c | 1 + ssh2transport.c | 11 +++++++++++ ssh2userauth-server.c | 1 + ssh2userauth.c | 1 + sshcommon.c | 5 +++++ sshppl.h | 8 ++++++++ 10 files changed, 32 insertions(+), 1 deletion(-) diff --git a/ssh.c b/ssh.c index 719d9e61..c2951499 100644 --- a/ssh.c +++ b/ssh.c @@ -996,7 +996,8 @@ static size_t ssh_sendbuffer(Backend *be) backlog = ssh_stdin_backlog(ssh->cl); - /* FIXME: also include sizes of pqs */ + if (ssh->base_layer) + backlog += ssh_ppl_queued_data_size(ssh->base_layer); /* * If the SSH socket itself has backed up, add the total backup diff --git a/ssh1connection.c b/ssh1connection.c index c7396166..b3e200b3 100644 --- a/ssh1connection.c +++ b/ssh1connection.c @@ -43,6 +43,7 @@ static const struct PacketProtocolLayerVtable ssh1_connection_vtable = { ssh1_connection_want_user_input, ssh1_connection_got_user_input, ssh1_connection_reconfigure, + ssh_ppl_default_queued_data_size, NULL /* no layer names in SSH-1 */, }; diff --git a/ssh1login-server.c b/ssh1login-server.c index d4988516..3316a6c3 100644 --- a/ssh1login-server.c +++ b/ssh1login-server.c @@ -65,6 +65,7 @@ static const struct PacketProtocolLayerVtable ssh1_login_server_vtable = { ssh1_login_server_want_user_input, ssh1_login_server_got_user_input, ssh1_login_server_reconfigure, + ssh_ppl_default_queued_data_size, NULL /* no layer names in SSH-1 */, }; diff --git a/ssh1login.c b/ssh1login.c index c129b866..803e590e 100644 --- a/ssh1login.c +++ b/ssh1login.c @@ -80,6 +80,7 @@ static const struct PacketProtocolLayerVtable ssh1_login_vtable = { ssh1_login_want_user_input, ssh1_login_got_user_input, ssh1_login_reconfigure, + ssh_ppl_default_queued_data_size, NULL /* no layer names in SSH-1 */, }; diff --git a/ssh2connection.c b/ssh2connection.c index a8c20f2d..666d6db7 100644 --- a/ssh2connection.c +++ b/ssh2connection.c @@ -30,6 +30,7 @@ static const struct PacketProtocolLayerVtable ssh2_connection_vtable = { ssh2_connection_want_user_input, ssh2_connection_got_user_input, ssh2_connection_reconfigure, + ssh_ppl_default_queued_data_size, "ssh-connection", }; diff --git a/ssh2transport.c b/ssh2transport.c index 982cee8c..88f1f6d4 100644 --- a/ssh2transport.c +++ b/ssh2transport.c @@ -71,6 +71,7 @@ static void ssh2_transport_special_cmd(PacketProtocolLayer *ppl, static bool ssh2_transport_want_user_input(PacketProtocolLayer *ppl); static void ssh2_transport_got_user_input(PacketProtocolLayer *ppl); static void ssh2_transport_reconfigure(PacketProtocolLayer *ppl, Conf *conf); +static size_t ssh2_transport_queued_data_size(PacketProtocolLayer *ppl); static void ssh2_transport_set_max_data_size(struct ssh2_transport_state *s); static unsigned long sanitise_rekey_time(int rekey_time, unsigned long def); @@ -84,6 +85,7 @@ static const struct PacketProtocolLayerVtable ssh2_transport_vtable = { ssh2_transport_want_user_input, ssh2_transport_got_user_input, ssh2_transport_reconfigure, + ssh2_transport_queued_data_size, NULL, /* no protocol name for this layer */ }; @@ -2028,3 +2030,12 @@ static int ssh2_transport_confirm_weak_crypto_primitive( return seat_confirm_weak_crypto_primitive( s->ppl.seat, type, name, ssh2_transport_dialog_callback, s); } + +static size_t ssh2_transport_queued_data_size(PacketProtocolLayer *ppl) +{ + struct ssh2_transport_state *s = + container_of(ppl, struct ssh2_transport_state, ppl); + + return (ssh_ppl_default_queued_data_size(ppl) + + ssh_ppl_queued_data_size(s->higher_layer)); +} diff --git a/ssh2userauth-server.c b/ssh2userauth-server.c index 37753956..7e67e557 100644 --- a/ssh2userauth-server.c +++ b/ssh2userauth-server.c @@ -46,6 +46,7 @@ static const struct PacketProtocolLayerVtable ssh2_userauth_server_vtable = { NULL /* want_user_input */, NULL /* got_user_input */, NULL /* reconfigure */, + ssh_ppl_default_queued_data_size, "ssh-userauth", }; diff --git a/ssh2userauth.c b/ssh2userauth.c index 62c4b409..dace56f1 100644 --- a/ssh2userauth.c +++ b/ssh2userauth.c @@ -120,6 +120,7 @@ static const struct PacketProtocolLayerVtable ssh2_userauth_vtable = { ssh2_userauth_want_user_input, ssh2_userauth_got_user_input, ssh2_userauth_reconfigure, + ssh_ppl_default_queued_data_size, "ssh-userauth", }; diff --git a/sshcommon.c b/sshcommon.c index 0d56460f..c19880b5 100644 --- a/sshcommon.c +++ b/sshcommon.c @@ -835,6 +835,11 @@ void ssh_ppl_user_output_string_and_free(PacketProtocolLayer *ppl, char *text) sfree(text); } +size_t ssh_ppl_default_queued_data_size(PacketProtocolLayer *ppl) +{ + return ppl->out_pq->pqb.total_size; +} + /* ---------------------------------------------------------------------- * Common helper functions for clients and implementations of * BinaryPacketProtocol. diff --git a/sshppl.h b/sshppl.h index 4d55d380..cfd6d6be 100644 --- a/sshppl.h +++ b/sshppl.h @@ -19,6 +19,7 @@ struct PacketProtocolLayerVtable { bool (*want_user_input)(PacketProtocolLayer *ppl); void (*got_user_input)(PacketProtocolLayer *ppl); void (*reconfigure)(PacketProtocolLayer *ppl, Conf *conf); + size_t (*queued_data_size)(PacketProtocolLayer *ppl); /* Protocol-level name of this layer. */ const char *name; @@ -73,6 +74,8 @@ static inline void ssh_ppl_got_user_input(PacketProtocolLayer *ppl) { ppl->vt->got_user_input(ppl); } static inline void ssh_ppl_reconfigure(PacketProtocolLayer *ppl, Conf *conf) { ppl->vt->reconfigure(ppl, conf); } +static inline size_t ssh_ppl_queued_data_size(PacketProtocolLayer *ppl) +{ return ppl->vt->queued_data_size(ppl); } /* ssh_ppl_free is more than just a macro wrapper on the vtable; it * does centralised parts of the freeing too. */ @@ -90,6 +93,11 @@ void ssh_ppl_setup_queues(PacketProtocolLayer *ppl, * avoid dereferencing itself on return from this function! */ void ssh_ppl_replace(PacketProtocolLayer *old, PacketProtocolLayer *new); +/* Default implementation of queued_data_size, which just adds up the + * sizes of all the packets in pq_out. A layer can override this if it + * has other things to take into account as well. */ +size_t ssh_ppl_default_queued_data_size(PacketProtocolLayer *ppl); + PacketProtocolLayer *ssh1_login_new( Conf *conf, const char *host, int port, PacketProtocolLayer *successor_layer); From aed81648cc314155a1219c57ba03ad5a00c662be Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 25 Feb 2020 21:27:34 +0000 Subject: [PATCH 38/62] Fix a deadlock in SFTP upload. I tried to do an SFTP upload through connection sharing the other day and found that pscp sent some data and then hung. Now I debug it, what seems to have happened was that we were looping in sftp_recv() waiting for an SFTP packet from the remote, but we didn't have any outstanding SFTP requests that the remote was going to reply to. Checking further, xfer_upload_ready() reported true, so we _could_ have sent something - but the logic in the upload loop had a hole through which we managed to get into 'waiting for a packet' state. I think what must have happened is that xfer_upload_ready() reported false so that we entered sftp_recv(), but then the event loop inside sftp_recv() ran a toplevel callback that made xfer_upload_ready() return true. So, the fix: sftp_recv() is our last-ditch fallback, and we always try emptying our callback queue and rechecking upload_ready before we resort to waiting for a remote packet. This not only fixes the hang I observed: it also hugely improves the upload speed. My guess is that the bug must have been preventing us from filling our outgoing request pipeline a _lot_ - but I didn't notice it until the one time the queue accidentally ended up empty, rather than just sparse enough to make transfers slow. Annoyingly, I actually considered this fix back when I was trying to fix the proftpd issue mentioned in commit cd97b7e7e. I decided fixing ssh_sendbuffer() was a better idea. In fact it would have been an even better idea to do both! Oh well, better late than never. (cherry picked from commit 3a633bed3505b15bb65d4709315a8c29b6497fe8) --- pscp.c | 9 +++++++++ psftp.c | 10 ++++++++++ 2 files changed, 19 insertions(+) diff --git a/pscp.c b/pscp.c index 3ba0aaaf..c9b149c2 100644 --- a/pscp.c +++ b/pscp.c @@ -812,6 +812,15 @@ int scp_send_filedata(char *data, int len) } while (!xfer_upload_ready(scp_sftp_xfer)) { + if (toplevel_callback_pending()) { + /* If we have pending callbacks, they might make + * xfer_upload_ready start to return true. So we should + * run them and then re-check xfer_upload_ready, before + * we go as far as waiting for an entire packet to + * arrive. */ + run_toplevel_callbacks(); + continue; + } pktin = sftp_recv(); ret = xfer_upload_gotpkt(scp_sftp_xfer, pktin); if (ret <= 0) { diff --git a/psftp.c b/psftp.c index dd7edfd1..df31a806 100644 --- a/psftp.c +++ b/psftp.c @@ -724,6 +724,16 @@ bool sftp_put_file(char *fname, char *outfname, bool recurse, bool restart) } } + if (toplevel_callback_pending() && !err && !eof) { + /* If we have pending callbacks, they might make + * xfer_upload_ready start to return true. So we should + * run them and then re-check xfer_upload_ready, before + * we go as far as waiting for an entire packet to + * arrive. */ + run_toplevel_callbacks(); + continue; + } + if (!xfer_done(xfer)) { pktin = sftp_recv(); ret = xfer_upload_gotpkt(xfer, pktin); From 67ca9703bebe647d1c7400c052acb2af034c0105 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Wed, 5 Feb 2020 19:45:27 +0000 Subject: [PATCH 39/62] Fix minor memory leak in psftp batch mode. Spotted by Leak Sanitiser, while I was investigating the PSFTP / proftpd issue mentioned in the previous commit (with ASan on as usual). The two very similar loops that read PSFTP commands from the interactive prompt and a batch file differed in one respect: only one of them remembered to free the command afterwards. Now I've moved the freeing code out into a subroutine that both loops can use. (cherry picked from commit bf0f323fb47c34f392a09c3bc1359a82e8339959) --- psftp.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/psftp.c b/psftp.c index df31a806..a5e92911 100644 --- a/psftp.c +++ b/psftp.c @@ -2316,6 +2316,16 @@ struct sftp_command *sftp_getcmd(FILE *fp, int mode, int modeflags) return cmd; } +static void sftp_cmd_free(struct sftp_command *cmd) +{ + if (cmd->words) { + for (size_t i = 0; i < cmd->nwords; i++) + sfree(cmd->words[i]); + sfree(cmd->words); + } + sfree(cmd); +} + static int do_sftp_init(void) { struct sftp_packet *pktin; @@ -2390,13 +2400,7 @@ int do_sftp(int mode, int modeflags, char *batchfile) if (!cmd) break; ret = cmd->obey(cmd); - if (cmd->words) { - int i; - for(i = 0; i < cmd->nwords; i++) - sfree(cmd->words[i]); - sfree(cmd->words); - } - sfree(cmd); + sftp_cmd_free(cmd); if (ret < 0) break; } @@ -2413,6 +2417,7 @@ int do_sftp(int mode, int modeflags, char *batchfile) if (!cmd) break; ret = cmd->obey(cmd); + sftp_cmd_free(cmd); if (ret < 0) break; if (ret == 0) { From eccd4bf781e99f57902a6189e4acb572479d0c49 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Thu, 6 Feb 2020 23:48:36 +0000 Subject: [PATCH 40/62] pollwrap: stop returning unasked-for rwx statuses. The sets of poll(2) events that we check in order to return SELECT_R and SELECT_W overlap: to be precise, they have POLLERR in common. So if an fd signals POLLERR, then pollwrap_get_fd_rwx will respond by saying that it has both SELECT_R and SELECT_W available on it - even if the caller had only asked for one of those. In other words, you can get a spurious SELECT_W notification on an fd that you never asked for SELECT_W on in the first place. This definitely isn't what I'd meant that API to do. In particular, if a socket in the middle of an asynchronous connect() signals POLLERR, then Unix Plink will call select_result for it with SELECT_R and then SELECT_W respectively. The former will notice that it's got an error condition and call plug_closing - and _then_ the latter will decide that it's writable and set s->connected! The plan was to only select it for write until it was connected, but this bug in pollwrap was defeating that plan. Now pollwrap_get_fd_rwx should only ever return a set of rwx flags that's a subset of the one that the client asked for via pollwrap_add_fd_rwx. (cherry picked from commit 78974fce896068b099cfc99ac7be27d5fe3bb075) --- unix/uxpoll.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/unix/uxpoll.c b/unix/uxpoll.c index da74ebf9..474926bb 100644 --- a/unix/uxpoll.c +++ b/unix/uxpoll.c @@ -126,29 +126,44 @@ int pollwrap_poll_timeout(pollwrapper *pw, int milliseconds) return poll(pw->fds, pw->nfd, milliseconds); } -int pollwrap_get_fd_events(pollwrapper *pw, int fd) +static void pollwrap_get_fd_events_revents(pollwrapper *pw, int fd, + int *events_p, int *revents_p) { pollwrap_fdtopos *f2p, f2p_find; + int events = 0, revents = 0; assert(fd >= 0); f2p_find.fd = fd; f2p = find234(pw->fdtopos, &f2p_find, NULL); - if (!f2p) - return 0; + if (f2p) { + events = pw->fds[f2p->pos].events; + revents = pw->fds[f2p->pos].revents; + } - return pw->fds[f2p->pos].revents; + if (events_p) + *events_p = events; + if (revents_p) + *revents_p = revents; +} + +int pollwrap_get_fd_events(pollwrapper *pw, int fd) +{ + int revents; + pollwrap_get_fd_events_revents(pw, fd, NULL, &revents); + return revents; } int pollwrap_get_fd_rwx(pollwrapper *pw, int fd) { - int revents = pollwrap_get_fd_events(pw, fd); + int events, revents; + pollwrap_get_fd_events_revents(pw, fd, &events, &revents); int rwx = 0; - if (revents & SELECT_R_OUT) + if ((events & POLLIN) && (revents & SELECT_R_OUT)) rwx |= SELECT_R; - if (revents & SELECT_W_OUT) + if ((events & POLLOUT) && (revents & SELECT_W_OUT)) rwx |= SELECT_W; - if (revents & SELECT_X_OUT) + if ((events & POLLPRI) && (revents & SELECT_X_OUT)) rwx |= SELECT_X; return rwx; } From b29af6df360d400389e05c7e68bddab73d55ff07 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 8 Feb 2020 15:35:03 +0000 Subject: [PATCH 41/62] Improve stop-bits messages in serial setup. On Windows, due to a copy-paste goof, the message that should have read "Configuring n stop bits" instead ended with "data bits". While I'm here, I've arranged that the "1 stop bit" case of that message is in the singular. And then I've done the same thing again on Unix, because I noticed that message was unconditionally plural too. (cherry picked from commit bdb7b47a5ec9809c9ea2544a02c36968a714c788) --- unix/uxser.c | 4 ++-- windows/winser.c | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/unix/uxser.c b/unix/uxser.c index f2b1cadf..f38a8eac 100644 --- a/unix/uxser.c +++ b/unix/uxser.c @@ -199,8 +199,8 @@ static const char *serial_configure(Serial *serial, Conf *conf) } else { options.c_cflag &= ~CSTOPB; } - logeventf(serial->logctx, "Configuring %d stop bits", - (options.c_cflag & CSTOPB ? 2 : 1)); + logeventf(serial->logctx, "Configuring %s", + (options.c_cflag & CSTOPB ? "2 stop bits" : "1 stop bit")); options.c_iflag &= ~(IXON|IXOFF); #ifdef CRTSCTS diff --git a/windows/winser.c b/windows/winser.c index d87ca6c9..c63aefeb 100644 --- a/windows/winser.c +++ b/windows/winser.c @@ -131,12 +131,12 @@ static const char *serial_configure(Serial *serial, HANDLE serport, Conf *conf) logeventf(serial->logctx, "Configuring %u data bits", dcb.ByteSize); switch (conf_get_int(conf, CONF_serstopbits)) { - case 2: dcb.StopBits = ONESTOPBIT; str = "1"; break; - case 3: dcb.StopBits = ONE5STOPBITS; str = "1.5"; break; - case 4: dcb.StopBits = TWOSTOPBITS; str = "2"; break; + case 2: dcb.StopBits = ONESTOPBIT; str = "1 stop bit"; break; + case 3: dcb.StopBits = ONE5STOPBITS; str = "1.5 stop bits"; break; + case 4: dcb.StopBits = TWOSTOPBITS; str = "2 stop bits"; break; default: return "Invalid number of stop bits (need 1, 1.5 or 2)"; } - logeventf(serial->logctx, "Configuring %s data bits", str); + logeventf(serial->logctx, "Configuring %s", str); switch (conf_get_int(conf, CONF_serparity)) { case SER_PAR_NONE: dcb.Parity = NOPARITY; str = "no"; break; From 706eb63c316bf1d9caccff207e18f7932e75122b Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 9 Feb 2020 08:24:06 +0000 Subject: [PATCH 42/62] Minor memory leaks in Pageant client code. (cherry picked from commit 8677ee00fb55ddfa0eb7d554bfb9da0c778a8acf) --- pageant.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pageant.c b/pageant.c index ba334cc3..6ade4ccb 100644 --- a/pageant.c +++ b/pageant.c @@ -1294,6 +1294,7 @@ int pageant_add_keyfile(Filename *filename, const char *passphrase, return PAGEANT_ACTION_FAILURE; } + sfree(skey->comment); ssh_key_free(skey->key); sfree(skey); sfree(response); @@ -1391,6 +1392,7 @@ int pageant_enum_keys(pageant_key_enum_fn_t callback, void *callback_ctx, callback(callback_ctx, fingerprint, cbkey.comment, &cbkey); sfree(fingerprint); sfree(cbkey.comment); + strbuf_free(cbkey.blob); } sfree(keylist); From 11d67b5a9143770fb227d0b574d3b2f1f3b1b6eb Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 8 Feb 2020 18:57:57 +0000 Subject: [PATCH 43/62] uxpgnt --askpass: explicitly fflush(stdout) on exit. I'm not really sure why that's necessary: by my understanding of the C standard, it shouldn't be. But my observation is that when compiling with {Address,Leak} Sanitiser enabled, pageant --askpass can somehow manage to exit without having actually written the passphrase to its standard output. (cherry picked from commit c618d6baac454feac8d09c0a307a5e70ae090326) --- unix/uxpgnt.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/unix/uxpgnt.c b/unix/uxpgnt.c index 3d83fcf0..65a19a6f 100644 --- a/unix/uxpgnt.c +++ b/unix/uxpgnt.c @@ -1093,6 +1093,8 @@ int main(int argc, char **argv) return 1; puts(passphrase); + fflush(stdout); + smemclr(passphrase, strlen(passphrase)); sfree(passphrase); return 0; From 5b45c32ab7c2c093360b8559b6622852d52f8660 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 16 Feb 2020 11:49:44 +0000 Subject: [PATCH 44/62] uxpty.c: add a missing include. This file exports several functions defined in sshserver.h, and the declarations weren't being type-checked against the definitions. (cherry picked from commit 37d91aabffc07c00b194623cb44010f4a95e9290) --- unix/uxpty.c | 1 + 1 file changed, 1 insertion(+) diff --git a/unix/uxpty.c b/unix/uxpty.c index 2533d958..2e1e12cb 100644 --- a/unix/uxpty.c +++ b/unix/uxpty.c @@ -25,6 +25,7 @@ #include "putty.h" #include "ssh.h" +#include "sshserver.h" /* to check the prototypes of server-needed things */ #include "tree234.h" #ifndef OMIT_UTMP From 45e4fbf2072407c9afceb95367be21c51539419e Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Wed, 19 Feb 2020 19:08:51 +0000 Subject: [PATCH 45/62] Fix handling of large RHS in mp_add_integer_into. While looking over the code for other reasons, I happened to notice that the internal function mp_add_masked_integer_into was using a totally wrong condition to check whether it was about to do an out-of-range right shift: it was comparing a shift count measured in bits against BIGNUM_INT_BYTES. The resulting bug hasn't shown up in the code so far, which I assume is just because no caller is passing any RHS to mp_add_integer_into bigger than about 1 or 2. And it doesn't show up in the test suite because I hadn't tested those functions. Now I am testing them, and the newly added test fails when built for 16-bit BignumInt if you back out the actual fix in this commit. (cherry picked from commit 921118dbea743cc1ba78e21b01924343a077064f) --- mpint.c | 5 +++-- test/cryptsuite.py | 16 ++++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/mpint.c b/mpint.c index 8140fafe..3d7a3987 100644 --- a/mpint.c +++ b/mpint.c @@ -758,7 +758,7 @@ static BignumCarry mp_add_masked_integer_into( for (size_t i = 0; i < rw; i++) { BignumInt aword = mp_word(a, i); size_t shift = i * BIGNUM_INT_BITS; - BignumInt bword = shift < BIGNUM_INT_BYTES ? b >> shift : 0; + BignumInt bword = shift < CHAR_BIT*sizeof(b) ? b >> shift : 0; BignumInt out; bword = (bword ^ b_xor) & b_and; BignumADC(out, carry, aword, bword, carry); @@ -799,7 +799,8 @@ static void mp_add_integer_into_shifted_by_words( * shift n down. If it's 0, we add zero bits into r, and * leave n alone. */ BignumInt bword = n & -(BignumInt)indicator; - uintmax_t new_n = (BIGNUM_INT_BITS < 64 ? n >> BIGNUM_INT_BITS : 0); + uintmax_t new_n = (BIGNUM_INT_BYTES < sizeof(n) ? + n >> BIGNUM_INT_BITS : 0); n ^= (n ^ new_n) & -(uintmax_t)indicator; BignumInt aword = mp_word(a, i); diff --git a/test/cryptsuite.py b/test/cryptsuite.py index f76c3ed2..995110b0 100755 --- a/test/cryptsuite.py +++ b/test/cryptsuite.py @@ -335,6 +335,22 @@ class mpint(MyTestBase): bm = mp_copy(bi) self.assertEqual(int(mp_mul(am, bm)), ai * bi) + def testAddInteger(self): + initial = mp_copy(4444444444444444444444444) + + x = mp_new(mp_max_bits(initial) + 64) + + # mp_{add,sub}_integer_into should be able to cope with any + # uintmax_t. Test a number that requires more than 32 bits. + mp_add_integer_into(x, initial, 123123123123123) + self.assertEqual(int(x), 4444444444567567567567567) + mp_sub_integer_into(x, initial, 123123123123123) + self.assertEqual(int(x), 4444444444321321321321321) + + # mp_mul_integer_into only takes a uint16_t integer input + mp_mul_integer_into(x, initial, 10001) + self.assertEqual(int(x), 44448888888888888888888884444) + def testDivision(self): divisors = [1, 2, 3, 2**16+1, 2**32-1, 2**32+1, 2**128-159, 141421356237309504880168872420969807856967187537694807] From b2e5b20ab72ed5e4b7ed744c03ab4c472eccad57 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 2 Mar 2020 18:34:52 +0000 Subject: [PATCH 46/62] mpint: clean up handling of uintmax_t. Functions like mp_copy_integer_into, mp_add_integer_into and mp_hs_integer all take an ordinary C integer in the form of a uintmax_t, and perform an operation between that and an mp_int. In order to do that, they have to break it up into some number of BignumInt, via bit shifts. But in C, shifting by an amount equal to or greater than the width of the type is undefined behaviour, and you risk the compiler generating nonsense or complaining at compile time. I did various dodges in those functions to try to avoid that, but didn't manage to use the same idiom everywhere. Sometimes I'd leave the integer in its original form and shift it right by increasing multiples of BIGNUM_INT_BITS; sometimes I'd shift it down in place every time. And mostly I'd do the conditional shift by checking against sizeof(n), but once I did it by shifting by half the word and then the other half. Now refactored so that there's a pair of functions to shift a uintmax_t left or right by BIGNUM_INT_BITS in what I hope is a UB-safe manner, and changed all the code I could find to use them. (cherry picked from commit 3ea69c290e8764282d3c52f351dfdc166e926590) --- mpint.c | 52 ++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 38 insertions(+), 14 deletions(-) diff --git a/mpint.c b/mpint.c index 3d7a3987..8ac7a50e 100644 --- a/mpint.c +++ b/mpint.c @@ -33,6 +33,35 @@ static inline BignumInt mp_word(mp_int *x, size_t i) return i < x->nw ? x->w[i] : 0; } +/* + * Shift an ordinary C integer by BIGNUM_INT_BITS, in a way that + * avoids writing a shift operator whose RHS is greater or equal to + * the size of the type, because that's undefined behaviour in C. + * + * In fact we must avoid even writing it in a definitely-untaken + * branch of an if, because compilers will sometimes warn about + * that. So you can't just write 'shift too big ? 0 : n >> shift', + * because even if 'shift too big' is a constant-expression + * evaluating to false, you can still get complaints about the + * else clause of the ?:. + * + * So we have to re-check _inside_ that clause, so that the shift + * count is reset to something nonsensical but safe in the case + * where the clause wasn't going to be taken anyway. + */ +static uintmax_t shift_right_by_one_word(uintmax_t n) +{ + bool shift_too_big = BIGNUM_INT_BYTES >= sizeof(n); + return shift_too_big ? 0 : + n >> (shift_too_big ? 0 : BIGNUM_INT_BITS); +} +static uintmax_t shift_left_by_one_word(uintmax_t n) +{ + bool shift_too_big = BIGNUM_INT_BYTES >= sizeof(n); + return shift_too_big ? 0 : + n << (shift_too_big ? 0 : BIGNUM_INT_BITS); +} + static mp_int *mp_make_sized(size_t nw) { mp_int *x = snew_plus(mp_int, nw * sizeof(BignumInt)); @@ -260,12 +289,8 @@ unsigned mp_get_bit(mp_int *x, size_t bit) uintmax_t mp_get_integer(mp_int *x) { uintmax_t toret = 0; - for (size_t i = x->nw; i-- > 0 ;) { - /* Shift in two stages to avoid undefined behaviour if the - * shift count equals the integer width */ - toret = (toret << (BIGNUM_INT_BITS/2)) << (BIGNUM_INT_BITS/2); - toret |= x->w[i]; - } + for (size_t i = x->nw; i-- > 0 ;) + toret = shift_left_by_one_word(toret) | x->w[i]; return toret; } @@ -757,8 +782,8 @@ static BignumCarry mp_add_masked_integer_into( { for (size_t i = 0; i < rw; i++) { BignumInt aword = mp_word(a, i); - size_t shift = i * BIGNUM_INT_BITS; - BignumInt bword = shift < CHAR_BIT*sizeof(b) ? b >> shift : 0; + BignumInt bword = b; + b = shift_right_by_one_word(b); BignumInt out; bword = (bword ^ b_xor) & b_and; BignumADC(out, carry, aword, bword, carry); @@ -799,8 +824,7 @@ static void mp_add_integer_into_shifted_by_words( * shift n down. If it's 0, we add zero bits into r, and * leave n alone. */ BignumInt bword = n & -(BignumInt)indicator; - uintmax_t new_n = (BIGNUM_INT_BYTES < sizeof(n) ? - n >> BIGNUM_INT_BITS : 0); + uintmax_t new_n = shift_right_by_one_word(n); n ^= (n ^ new_n) & -(uintmax_t)indicator; BignumInt aword = mp_word(a, i); @@ -846,8 +870,8 @@ unsigned mp_hs_integer(mp_int *x, uintmax_t n) { BignumInt carry = 1; for (size_t i = 0; i < x->nw; i++) { - size_t shift = i * BIGNUM_INT_BITS; - BignumInt nword = shift < CHAR_BIT*sizeof(n) ? n >> shift : 0; + BignumInt nword = n; + n = shift_right_by_one_word(n); BignumInt dummy_out; BignumADC(dummy_out, carry, x->w[i], ~nword, carry); (void)dummy_out; @@ -872,8 +896,8 @@ unsigned mp_eq_integer(mp_int *x, uintmax_t n) { BignumInt diff = 0; for (size_t i = 0; i < x->nw; i++) { - size_t shift = i * BIGNUM_INT_BITS; - BignumInt nword = shift < CHAR_BIT*sizeof(n) ? n >> shift : 0; + BignumInt nword = n; + n = shift_right_by_one_word(n); diff |= x->w[i] ^ nword; } return 1 ^ normalise_to_1(diff); /* return 1 if diff _is_ zero */ From f86f396578ea10aed17669cd99fa0260d0b425e0 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 2 Mar 2020 18:42:31 +0000 Subject: [PATCH 47/62] Fix mp_{eq,hs}_integer(tiny, huge). The comparison functions between an mp_int and an integer worked by walking along the mp_int, comparing each of its words to the corresponding word of the integer. When they ran out of mp_int, they'd stop. But this overlooks the possibility that they might not have run out of _integer_ yet! If BIGNUM_INT_BITS is defined to be less than the size of a uintmax_t, then comparing (say) the uintmax_t 0x8000000000000001 against a one-word mp_int containing 0x0001 would return equality, because it would never get as far as spotting the high bit of the integer. Fixed by iterating up to the max of the number of BignumInts in the mp_int and the number that cover a uintmax_t. That means we have to use mp_word() instead of a direct array lookup to get the mp_int words to compare against, since now the word indices might be out of range. (cherry picked from commit 289d8873ec4de802f00a6aaa2d60013d4d911cc1) --- mpint.c | 10 ++++++---- test/cryptsuite.py | 13 +++++++++++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/mpint.c b/mpint.c index 8ac7a50e..6113d179 100644 --- a/mpint.c +++ b/mpint.c @@ -869,11 +869,12 @@ unsigned mp_cmp_hs(mp_int *a, mp_int *b) unsigned mp_hs_integer(mp_int *x, uintmax_t n) { BignumInt carry = 1; - for (size_t i = 0; i < x->nw; i++) { + size_t nwords = sizeof(n)/BIGNUM_INT_BYTES; + for (size_t i = 0, e = size_t_max(x->nw, nwords); i < e; i++) { BignumInt nword = n; n = shift_right_by_one_word(n); BignumInt dummy_out; - BignumADC(dummy_out, carry, x->w[i], ~nword, carry); + BignumADC(dummy_out, carry, mp_word(x, i), ~nword, carry); (void)dummy_out; } return carry; @@ -895,10 +896,11 @@ unsigned mp_cmp_eq(mp_int *a, mp_int *b) unsigned mp_eq_integer(mp_int *x, uintmax_t n) { BignumInt diff = 0; - for (size_t i = 0; i < x->nw; i++) { + size_t nwords = sizeof(n)/BIGNUM_INT_BYTES; + for (size_t i = 0, e = size_t_max(x->nw, nwords); i < e; i++) { BignumInt nword = n; n = shift_right_by_one_word(n); - diff |= x->w[i] ^ nword; + diff |= mp_word(x, i) ^ nword; } return 1 ^ normalise_to_1(diff); /* return 1 if diff _is_ zero */ } diff --git a/test/cryptsuite.py b/test/cryptsuite.py index 995110b0..5564c11f 100755 --- a/test/cryptsuite.py +++ b/test/cryptsuite.py @@ -252,6 +252,19 @@ class mpint(MyTestBase): mp_max_into(am_big, am, bm) self.assertEqual(int(am_big), max(ai, bi)) + # Test mp_{eq,hs}_integer in the case where the integer is as + # large as possible and the bignum contains very few words. In + # modes where BIGNUM_INT_BITS < 64, this used to go wrong. + mp10 = mp_new(4) + mp_add_integer_into(mp10, mp10, 10) + highbit = 1 << 63 + self.assertEqual(mp_hs_integer(mp10, highbit | 9), 0) + self.assertEqual(mp_hs_integer(mp10, highbit | 10), 0) + self.assertEqual(mp_hs_integer(mp10, highbit | 11), 0) + self.assertEqual(mp_eq_integer(mp10, highbit | 9), 0) + self.assertEqual(mp_eq_integer(mp10, highbit | 10), 0) + self.assertEqual(mp_eq_integer(mp10, highbit | 11), 0) + def testConditionals(self): testnumbers = [(mp_copy(n),n) for n in fibonacci_scattered()] for am, ai in testnumbers: From ee26ab8617402c81c34a7e2b0c613cd77f769d68 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 9 Mar 2020 19:23:24 +0000 Subject: [PATCH 48/62] kh2reg: fix Python 3 iterator bug with multiple hostnames. A known_hosts line can have multiple comma-separated hostnames on it, or more usually a hostname and an IP address. In the RSA and DSA key handlers, I was making a list of the integer parameters of the public key by using the 'map' function, and then iterating over it once per hostname on the line. But in Python 3, the 'map' function returns an iterator, not a list, so after you've iterated to its end once, it's empty, and iterating over it a second time stops immediately. As a result, the registry line for the second hostname was coming out empty. (cherry picked from commit 143f8a2d1096e9649930ed5d909bf59433c105c4) --- contrib/kh2reg.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contrib/kh2reg.py b/contrib/kh2reg.py index 272124d5..5a8e2f3e 100755 --- a/contrib/kh2reg.py +++ b/contrib/kh2reg.py @@ -225,7 +225,7 @@ def handle_line(line, output_formatter, try_hosts): # Treat as SSH-1-type host key. # Format: hostpat bits10 exp10 mod10 comment... # (PuTTY doesn't store the number of bits.) - keyparams = map (int, fields[2:4]) + keyparams = list(map(int, fields[2:4])) keytype = "rsa" else: @@ -259,12 +259,12 @@ def handle_line(line, output_formatter, try_hosts): keytype = "rsa2" # The rest of the subfields we can treat as an opaque list # of bignums (same numbers and order as stored by PuTTY). - keyparams = map (strtoint, subfields[1:]) + keyparams = list(map(strtoint, subfields[1:])) elif sshkeytype == "ssh-dss": keytype = "dss" # Same again. - keyparams = map (strtoint, subfields[1:]) + keyparams = list(map(strtoint, subfields[1:])) elif sshkeytype in nist_curves: keytype = sshkeytype From fade8e81bfc2eb18379a91b9de818032205ddfe3 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 9 Mar 2020 19:27:17 +0000 Subject: [PATCH 49/62] kh2reg: stop using deprecated base64.decodestring. Python 3 gave me a warning that I should have been using decodebytes instead. (cherry picked from commit 1efded20a1a66509b0a2b23c4342c96b395c6437) --- contrib/kh2reg.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contrib/kh2reg.py b/contrib/kh2reg.py index 5a8e2f3e..82773f88 100755 --- a/contrib/kh2reg.py +++ b/contrib/kh2reg.py @@ -73,8 +73,8 @@ class HMAC(object): def openssh_hashed_host_match(hashed_host, try_host): if hashed_host.startswith(b'|1|'): salt, expected = hashed_host[3:].split(b'|') - salt = base64.decodestring(salt) - expected = base64.decodestring(expected) + salt = base64.decodebytes(salt) + expected = base64.decodebytes(expected) mac = HMAC(hashlib.sha1, 64) else: return False # unrecognised magic number prefix @@ -232,7 +232,7 @@ def handle_line(line, output_formatter, try_hosts): # Treat as SSH-2-type host key. # Format: hostpat keytype keyblob64 comment... - sshkeytype, blob = fields[1], base64.decodestring( + sshkeytype, blob = fields[1], base64.decodebytes( fields[2].encode("ASCII")) # 'blob' consists of a number of From fee2e42be60a37d4172ebd17751111ec760190c5 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Fri, 13 Mar 2020 22:48:15 +0000 Subject: [PATCH 50/62] uxser: add a missing uxsel_del. If we close a serial port fd, we shouldn't leave it active in uxsel. That never ends well. (cherry picked from commit 6b77fc627a7b428e5d4854bb1092fb23f25e9ddc) --- unix/uxser.c | 1 + 1 file changed, 1 insertion(+) diff --git a/unix/uxser.c b/unix/uxser.c index f38a8eac..f3aaec6c 100644 --- a/unix/uxser.c +++ b/unix/uxser.c @@ -333,6 +333,7 @@ static const char *serial_init(Seat *seat, Backend **backend_handle, static void serial_close(Serial *serial) { if (serial->fd >= 0) { + uxsel_del(serial->fd); close(serial->fd); serial->fd = -1; } From 464ab136c2a6432de7e11a548cac58dc8dbc0c6e Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Fri, 13 Mar 2020 22:48:53 +0000 Subject: [PATCH 51/62] Cope with "delete_window" event on the GTK config box. That causes the config dialog to terminate with result -1, which wasn't handled at all by the result-receiving code. So GTK PuTTY would continue running its main loop even though it had no windows open and wasn't ever planning to do anything. (cherry picked from commit 4fc5d7a5f5528e2f75e781a2fd0a1fd775f53858) --- unix/gtkmain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/unix/gtkmain.c b/unix/gtkmain.c index 520cd6ee..e7a0eff3 100644 --- a/unix/gtkmain.c +++ b/unix/gtkmain.c @@ -566,7 +566,7 @@ static void post_initial_config_box(void *vctx, int result) if (result > 0) { new_session_window(ctx.conf, ctx.geometry_string); - } else if (result == 0) { + } else { /* In this main(), which only runs one session in total, a * negative result from the initial config box means we simply * terminate. */ From 9331bb3c57628357812d5f7a5b3964c200e16875 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Fri, 3 Apr 2020 17:53:36 +0100 Subject: [PATCH 52/62] Fix null-dereference in ssh2_channel_response. In the SSH-2 connection layer, an outstanding_channel_request structure comes with a handler to be called back with the reply packet, when the other end sends one. But sometimes it doesn't - if the channel begins to close before the request has been replied to - in which case the handler function is called with a NULL packet pointer. The common ssh2_channel_response function that handles most of the client-side channel requests was not prepared to cope with that pointer being null. Fixed by making it handle a null return the same as CHANNEL_FAILURE. (cherry picked from commit e4b6a7efd2cab27a681c9a832f6c650abf57c7be) --- ssh2connection-client.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/ssh2connection-client.c b/ssh2connection-client.c index d9e909d2..87df45ae 100644 --- a/ssh2connection-client.c +++ b/ssh2connection-client.c @@ -315,7 +315,11 @@ SshChannel *ssh2_serverside_agent_open(ConnectionLayer *cl, Channel *chan) static void ssh2_channel_response( struct ssh2_channel *c, PktIn *pkt, void *ctx) { - chan_request_response(c->chan, pkt->type == SSH2_MSG_CHANNEL_SUCCESS); + /* If pkt==NULL (because this handler has been called in response + * to CHANNEL_CLOSE arriving while the request was still + * outstanding), we treat that the same as CHANNEL_FAILURE. */ + chan_request_response(c->chan, + pkt && pkt->type == SSH2_MSG_CHANNEL_SUCCESS); } void ssh2channel_start_shell(SshChannel *sc, bool want_reply) From b267f35cf74ecb46580dbf4191273e6632d655b7 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Fri, 3 Apr 2020 17:56:30 +0100 Subject: [PATCH 53/62] gtk: fill in missing case in scroll_event(). If gdk_event_get_scroll_deltas() return failure for a given GdkEventScroll, it doesn't follow that that event has no usable scrolling action in it at all. The fallback is to call gdk_event_get_scroll_direction() instead, which is less precise but still gives _something_ you can use. So in that situation, instead of just returning false, we can fall through to the handling we use for pre-GTK3 scroll events (which are always imprecise). In particular, I've noticed recently that if you run GTK 3 PuTTY in the virtual X display created by vnc4server, and connect to it using xtightvncviewer, then scroll-wheel actions passed through from the VNC client will cause scroll_event() to receive low-res GdkEventScroll structures of exactly this kind. So scroll-wheel activity on the terminal window wasn't causing a scroll in that environment, and with this patch, it does. (cherry picked from commit 0fd30113f14657448ce455b7f72104d884f39e6d) --- unix/gtkwin.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/unix/gtkwin.c b/unix/gtkwin.c index e07a7216..4ce89c66 100644 --- a/unix/gtkwin.c +++ b/unix/gtkwin.c @@ -2182,21 +2182,26 @@ gboolean button_event(GtkWidget *widget, GdkEventButton *event, gpointer data) gboolean scroll_event(GtkWidget *widget, GdkEventScroll *event, gpointer data) { GtkFrontend *inst = (GtkFrontend *)data; + GdkScrollDirection dir; #if GTK_CHECK_VERSION(3,4,0) gdouble dx, dy; if (gdk_event_get_scroll_deltas((GdkEvent *)event, &dx, &dy)) { return scroll_internal(inst, dy, event->state, event->x, event->y); - } else + } else if (!gdk_event_get_scroll_direction((GdkEvent *)event, &dir)) { return false; + } #else + dir = event->direction; +#endif + guint button; GdkEventButton *event_button; gboolean ret; - if (event->direction == GDK_SCROLL_UP) + if (dir == GDK_SCROLL_UP) button = 4; - else if (event->direction == GDK_SCROLL_DOWN) + else if (dir == GDK_SCROLL_DOWN) button = 5; else return false; @@ -2216,7 +2221,6 @@ gboolean scroll_event(GtkWidget *widget, GdkEventScroll *event, gpointer data) ret = button_internal(inst, event_button); gdk_event_free((GdkEvent *)event_button); return ret; -#endif } #endif From 8896cf55bca32a87be109c5d80f2343ff31cf600 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 5 Apr 2020 11:20:25 +0100 Subject: [PATCH 54/62] gtkfont: use PANGO_PIXELS_CEIL to work out font metrics. Colin reports that on betas of Ubuntu 20.04, Pango has switched to getting its font metrics from HarfBuzz, and a side effect is apparently that they're being returned in the full precision of PANGO_SCALE fixed point. Previously, Pango appears to have been returning values that were always a whole number of pixels scaled by PANGO_SCALE. Moreover, it looks as if it was rounding the font ascent and descent _up_ to a whole number of pixels, rather than rounding to nearest. But our code rounds to nearest, which means that now the same font gets allocated fewer vertical pixels, which can be enough to cut off some ascenders or descenders. Pango already provides the macro PANGO_PIXELS_CEIL, so it's easy to switch over to using it. This should arrange that any text that fits within the font's stated ascent/descent measurements will also fit in the character cell. (cherry picked from commit f9a46a958157e1d6bd126560623599ed8ee50f32) --- unix/gtkfont.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/unix/gtkfont.c b/unix/gtkfont.c index f0e7e712..a4c70088 100644 --- a/unix/gtkfont.c +++ b/unix/gtkfont.c @@ -1458,8 +1458,10 @@ static unifont *pangofont_create_internal(GtkWidget *widget, pfont->u.vt = &pangofont_vtable; pfont->u.width = PANGO_PIXELS(pango_font_metrics_get_approximate_digit_width(metrics)); - pfont->u.ascent = PANGO_PIXELS(pango_font_metrics_get_ascent(metrics)); - pfont->u.descent = PANGO_PIXELS(pango_font_metrics_get_descent(metrics)); + pfont->u.ascent = + PANGO_PIXELS_CEIL(pango_font_metrics_get_ascent(metrics)); + pfont->u.descent = + PANGO_PIXELS_CEIL(pango_font_metrics_get_descent(metrics)); pfont->u.height = pfont->u.ascent + pfont->u.descent; pfont->u.want_fallback = false; #ifdef DRAW_TEXT_CAIRO From b22b4cc19fb10f9bd2ceab67d6057271a9708951 Mon Sep 17 00:00:00 2001 From: Jacob Nevins Date: Tue, 14 Apr 2020 20:36:46 +0100 Subject: [PATCH 55/62] On Windows, show hidden mouse pointer on error. If a terminal window closed with a popup (due to a network error, for instance) while the mouse pointer was hidden by 'Hide mouse pointer when typing in window', the mouse pointer could remain hidden while over the terminal window, making it hard to navigate to the popup. (cherry picked from commit d9c4ce9fd8ed21b64bf35367b45374bd7b1afcf0) --- windows/window.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/windows/window.c b/windows/window.c index b5c730eb..f32c9665 100644 --- a/windows/window.c +++ b/windows/window.c @@ -89,6 +89,7 @@ #endif static Mouse_Button translate_button(Mouse_Button button); +static void show_mouseptr(bool show); static LRESULT CALLBACK WndProc(HWND, UINT, WPARAM, LPARAM); static int TranslateKey(UINT message, WPARAM wParam, LPARAM lParam, unsigned char *output); @@ -1199,6 +1200,7 @@ static void wintw_set_raw_mouse_mode(TermWin *tw, bool activate) static void win_seat_connection_fatal(Seat *seat, const char *msg) { char *title = dupprintf("%s Fatal Error", appname); + show_mouseptr(true); MessageBox(hwnd, msg, title, MB_ICONERROR | MB_OK); sfree(title); @@ -2128,9 +2130,11 @@ static void win_seat_notify_remote_exit(Seat *seat) /* exitcode == INT_MAX indicates that the connection was closed * by a fatal error, so an error box will be coming our way and * we should not generate this informational one. */ - if (exitcode != INT_MAX) + if (exitcode != INT_MAX) { + show_mouseptr(true); MessageBox(hwnd, "Connection closed by remote host", appname, MB_OK | MB_ICONINFORMATION); + } } } } @@ -5422,6 +5426,7 @@ void modalfatalbox(const char *fmt, ...) va_start(ap, fmt); message = dupvprintf(fmt, ap); va_end(ap); + show_mouseptr(true); title = dupprintf("%s Fatal Error", appname); MessageBox(hwnd, message, title, MB_SYSTEMMODAL | MB_ICONERROR | MB_OK); sfree(message); @@ -5440,6 +5445,7 @@ void nonfatal(const char *fmt, ...) va_start(ap, fmt); message = dupvprintf(fmt, ap); va_end(ap); + show_mouseptr(true); title = dupprintf("%s Error", appname); MessageBox(hwnd, message, title, MB_ICONERROR | MB_OK); sfree(message); @@ -5552,6 +5558,7 @@ static void wintw_bell(TermWin *tw, int mode) if (!p_PlaySound || !p_PlaySound(bell_wavefile->path, NULL, SND_ASYNC | SND_FILENAME)) { char *buf, *otherbuf; + show_mouseptr(true); buf = dupprintf( "Unable to play sound file\n%s\nUsing default sound instead", bell_wavefile->path); From 426a2048cc21fe5cc01aa890a0a67a238940f447 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 2 May 2020 16:11:19 +0100 Subject: [PATCH 56/62] pty_backend_create: set up SIGCHLD handler earlier. Mark Wooding points out that when running with the +ut flag, we close pty_utmp_helper_pipe during pty backend setup, which causes the previously forked helper process to terminate. If that termination happens quickly enough, then the code later in pty_backend_create won't have set up the SIGCHLD handler and its pipe yet, so when we get to the main event loop, we'll fail to notice that subprocess waiting to be reaped, and leave it lying around as a zombie. An easy fix is to move the handler and pipe setup to before the code that potentially closes pty_utmp_helper_pipe, so that there isn't a race condition any more. (cherry picked from commit 7ffa6ed41eaa1e8b347f2b930838a441e168a498) --- unix/uxpty.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/unix/uxpty.c b/unix/uxpty.c index 2e1e12cb..721074fc 100644 --- a/unix/uxpty.c +++ b/unix/uxpty.c @@ -890,6 +890,15 @@ Backend *pty_backend_create( pty->fds[i].pty = pty; } + if (pty_signal_pipe[0] < 0) { + if (pipe(pty_signal_pipe) < 0) { + perror("pipe"); + exit(1); + } + cloexec(pty_signal_pipe[0]); + cloexec(pty_signal_pipe[1]); + } + pty->seat = seat; pty->backend.vt = &pty_backend; @@ -1248,14 +1257,6 @@ Backend *pty_backend_create( add234(ptys_by_pid, pty); } - if (pty_signal_pipe[0] < 0) { - if (pipe(pty_signal_pipe) < 0) { - perror("pipe"); - exit(1); - } - cloexec(pty_signal_pipe[0]); - cloexec(pty_signal_pipe[1]); - } pty_uxsel_setup(pty); return &pty->backend; From 602656eee7de9ce8525e6c09a4cf3b5e8193678a Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 16 May 2020 16:08:21 +0100 Subject: [PATCH 57/62] fuzzterm.c: fix prototypes of stub dlg functions. I'd forgotten to #include "dialog.h" in that file, which meant nothing was checking the prototypes of the stub implementations of the dlg_* function family against the real versions. They almost all needed a 'void *dlg' parameter updating to 'dlgparam *dp', which is a change dating from commit 3aae1f9d76 nearly two years ago. And a handful of them also still had 'int' that should be now have become 'bool'. (cherry picked from commit c373fe979fcc395c3efd0b12bd11b9061e45d7a4) --- fuzzterm.c | 65 +++++++++++++++++++++++++++++------------------------- 1 file changed, 35 insertions(+), 30 deletions(-) diff --git a/fuzzterm.c b/fuzzterm.c index f1085589..d5e87794 100644 --- a/fuzzterm.c +++ b/fuzzterm.c @@ -4,6 +4,7 @@ #define PUTTY_DO_GLOBALS #include "putty.h" +#include "dialog.h" #include "terminal.h" /* For Unix in particular, but harmless if this main() is reused elsewhere */ @@ -142,39 +143,43 @@ void timer_change_notify(unsigned long next) { } /* needed by config.c and sercfg.c */ -void dlg_radiobutton_set(union control *ctrl, void *dlg, int whichbutton) { } -int dlg_radiobutton_get(union control *ctrl, void *dlg) { return 0; } -void dlg_checkbox_set(union control *ctrl, void *dlg, int checked) { } -int dlg_checkbox_get(union control *ctrl, void *dlg) { return 0; } -void dlg_editbox_set(union control *ctrl, void *dlg, char const *text) { } -char *dlg_editbox_get(union control *ctrl, void *dlg) { return dupstr("moo"); } -void dlg_listbox_clear(union control *ctrl, void *dlg) { } -void dlg_listbox_del(union control *ctrl, void *dlg, int index) { } -void dlg_listbox_add(union control *ctrl, void *dlg, char const *text) { } -void dlg_listbox_addwithid(union control *ctrl, void *dlg, +void dlg_radiobutton_set(union control *ctrl, dlgparam *dp, int whichbutton) { } +int dlg_radiobutton_get(union control *ctrl, dlgparam *dp) { return 0; } +void dlg_checkbox_set(union control *ctrl, dlgparam *dp, bool checked) { } +bool dlg_checkbox_get(union control *ctrl, dlgparam *dp) { return false; } +void dlg_editbox_set(union control *ctrl, dlgparam *dp, char const *text) { } +char *dlg_editbox_get(union control *ctrl, dlgparam *dp) +{ return dupstr("moo"); } +void dlg_listbox_clear(union control *ctrl, dlgparam *dp) { } +void dlg_listbox_del(union control *ctrl, dlgparam *dp, int index) { } +void dlg_listbox_add(union control *ctrl, dlgparam *dp, char const *text) { } +void dlg_listbox_addwithid(union control *ctrl, dlgparam *dp, char const *text, int id) { } -int dlg_listbox_getid(union control *ctrl, void *dlg, int index) { return 0; } -int dlg_listbox_index(union control *ctrl, void *dlg) { return -1; } -int dlg_listbox_issel(union control *ctrl, void *dlg, int index) { return 0; } -void dlg_listbox_select(union control *ctrl, void *dlg, int index) { } -void dlg_text_set(union control *ctrl, void *dlg, char const *text) { } -void dlg_filesel_set(union control *ctrl, void *dlg, Filename *fn) { } -Filename *dlg_filesel_get(union control *ctrl, void *dlg) { return NULL; } -void dlg_fontsel_set(union control *ctrl, void *dlg, FontSpec *fn) { } -FontSpec *dlg_fontsel_get(union control *ctrl, void *dlg) { return NULL; } -void dlg_update_start(union control *ctrl, void *dlg) { } -void dlg_update_done(union control *ctrl, void *dlg) { } -void dlg_set_focus(union control *ctrl, void *dlg) { } -void dlg_label_change(union control *ctrl, void *dlg, char const *text) { } -union control *dlg_last_focused(union control *ctrl, void *dlg) { return NULL; } -void dlg_beep(void *dlg) { } -void dlg_error_msg(void *dlg, const char *msg) { } -void dlg_end(void *dlg, int value) { } -void dlg_coloursel_start(union control *ctrl, void *dlg, +int dlg_listbox_getid(union control *ctrl, dlgparam *dp, int index) +{ return 0; } +int dlg_listbox_index(union control *ctrl, dlgparam *dp) { return -1; } +bool dlg_listbox_issel(union control *ctrl, dlgparam *dp, int index) +{ return false; } +void dlg_listbox_select(union control *ctrl, dlgparam *dp, int index) { } +void dlg_text_set(union control *ctrl, dlgparam *dp, char const *text) { } +void dlg_filesel_set(union control *ctrl, dlgparam *dp, Filename *fn) { } +Filename *dlg_filesel_get(union control *ctrl, dlgparam *dp) { return NULL; } +void dlg_fontsel_set(union control *ctrl, dlgparam *dp, FontSpec *fn) { } +FontSpec *dlg_fontsel_get(union control *ctrl, dlgparam *dp) { return NULL; } +void dlg_update_start(union control *ctrl, dlgparam *dp) { } +void dlg_update_done(union control *ctrl, dlgparam *dp) { } +void dlg_set_focus(union control *ctrl, dlgparam *dp) { } +void dlg_label_change(union control *ctrl, dlgparam *dp, char const *text) { } +union control *dlg_last_focused(union control *ctrl, dlgparam *dp) +{ return NULL; } +void dlg_beep(dlgparam *dp) { } +void dlg_error_msg(dlgparam *dp, const char *msg) { } +void dlg_end(dlgparam *dp, int value) { } +void dlg_coloursel_start(union control *ctrl, dlgparam *dp, int r, int g, int b) { } -bool dlg_coloursel_results(union control *ctrl, void *dlg, +bool dlg_coloursel_results(union control *ctrl, dlgparam *dp, int *r, int *g, int *b) { return false; } -void dlg_refresh(union control *ctrl, void *dlg) { } +void dlg_refresh(union control *ctrl, dlgparam *dp) { } bool dlg_is_visible(union control *ctrl, dlgparam *dp) { return false; } const char *const appname = "FuZZterm"; From c39f6a22238ac0a8eaa55fcf10b7e6f4da9d424c Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 16 May 2020 16:11:55 +0100 Subject: [PATCH 58/62] sshserver.c: fix prototype of mainchan_new. Again, there was a missing #include in that file which meant that the definition of the function was never being checked against the declaration visible to other source files. (cherry picked from commit c5aa7fc31c4b57d07967ed8a88b77ff79cb5d44f) --- sshserver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sshserver.c b/sshserver.c index 7889b7d4..42559d8d 100644 --- a/sshserver.c +++ b/sshserver.c @@ -9,6 +9,7 @@ #include "ssh.h" #include "sshbpp.h" #include "sshppl.h" +#include "sshchan.h" #include "sshserver.h" #ifndef NO_GSSAPI #include "sshgssc.h" @@ -85,7 +86,7 @@ void ssh_check_frozen(Ssh *ssh) {} mainchan *mainchan_new( PacketProtocolLayer *ppl, ConnectionLayer *cl, Conf *conf, - int term_width, int term_height, int is_simple, SshChannel **sc_out) + int term_width, int term_height, bool is_simple, SshChannel **sc_out) { return NULL; } void mainchan_get_specials( mainchan *mc, add_special_fn_t add_special, void *ctx) {} From d527b1a886175ad587f91db31b36c1bb0d7b7ad2 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 16 May 2020 16:14:13 +0100 Subject: [PATCH 59/62] uxucs.c: fix type of wcrtomb return value. wcrtomb returns a size_t, so it's silly to immediately assign it into an int variable. Apparently running gcc with LTO enabled points this out as an error. This was benign as far as I can see: the obvious risk of integer overflow could only happen if the OS wanted to convert a single wide character into more than 2^31 bytes, and the test of the return value against (size_t)-1 for an error check seems to work anyway in practice, although I suspect that's only because of implementation- defined behaviour in gcc at the point where the size_t is narrowed to a signed int. (cherry picked from commit 99f5fa34ab918894939c8aca00e44730223c0e54) --- unix/uxucs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/unix/uxucs.c b/unix/uxucs.c index a3d6f5ba..c1d76a42 100644 --- a/unix/uxucs.c +++ b/unix/uxucs.c @@ -68,7 +68,7 @@ int wc_to_mb(int codepage, int flags, const wchar_t *wcstr, int wclen, memset(&state, 0, sizeof state); while (wclen > 0) { - int i = wcrtomb(output, wcstr[0], &state); + size_t i = wcrtomb(output, wcstr[0], &state); if (i == (size_t)-1 || i > n - mblen) break; memcpy(mbstr+n, output, i); From 6aa1dcb1ca07e2a495c20aaf9ea093f0dcc2d31f Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 14 Jun 2020 10:06:17 +0100 Subject: [PATCH 60/62] Remove assertion that len != 0 in ldisc_send. A user reported another situation in which that assertion can fail: if you paste text into the terminal that consists 100% of characters not available in the CONF_line_codepage character set, then the translation step generates the empty string as output, and that gets passed to ldisc_send by term_paste without checking. Previous bugs of this kind (see commits 4634cd47f7 and 43a63019f5) were fixed by adding a check before calling ldisc_send. But in commit 4634cd47f7 I said that probably at some point the right fix would be to remove the assertion in ldisc_send itself, so that passing len==0 becomes legal. (The assertion was there in the first place to catch cases where len==0 was used with its obsolete special meaning of signalling 'please update your status'.) Well, I think it's finally time. The assertion is removed: it's now legal again to call ldisc_send with an empty buffer, and its meaning is no longer the archaic special thing, but the trivial one of sending zero characters through the line discipline. (cherry picked from commit cd3e917fd0c52865c8978a4e277050580299fbac) --- ldisc.c | 1 - 1 file changed, 1 deletion(-) diff --git a/ldisc.c b/ldisc.c index 8eb80175..f097c040 100644 --- a/ldisc.c +++ b/ldisc.c @@ -130,7 +130,6 @@ void ldisc_send(Ldisc *ldisc, const void *vbuf, int len, bool interactive) int keyflag = 0; assert(ldisc->term); - assert(len); if (interactive) { /* From bbf6daf61221749c59640394424e20ab5f175f2f Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 14 Jun 2020 10:12:59 +0100 Subject: [PATCH 61/62] Remove obsolete checks for ldisc_send with len == 0. This reverts commit 4634cd47f75e74a697840fb32f18edb7f1cf41da and commit 43a63019f5bdc627170212ec5ed8c1333ad4727b, both of which introduced checks at ldisc_send call sites to avoid triggering the assertion that len != 0 inside ldisc_send. Now that assertion is gone, it's OK to call ldisc_send without checking the buffer size. (cherry picked from commit 2bbed67d9e088fe577edd3eaf6a8f4514bb38420) --- terminal.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/terminal.c b/terminal.c index 08592568..8f65a73e 100644 --- a/terminal.c +++ b/terminal.c @@ -3406,8 +3406,7 @@ static void term_out(Terminal *term) strbuf *buf = term_input_data_from_charset( term, DEFAULT_CODEPAGE, term->answerback, term->answerbacklen); - if (buf->len) - ldisc_send(term->ldisc, buf->s, buf->len, false); + ldisc_send(term->ldisc, buf->s, buf->len, false); strbuf_free(buf); } break; @@ -3656,7 +3655,7 @@ static void term_out(Terminal *term) break; case 'Z': /* DECID: terminal type query */ compatibility(VT100); - if (term->ldisc && term->id_string[0]) + if (term->ldisc) ldisc_send(term->ldisc, term->id_string, strlen(term->id_string), false); break; @@ -3974,7 +3973,7 @@ static void term_out(Terminal *term) case 'c': /* DA: terminal type query */ compatibility(VT100); /* This is the response for a VT102 */ - if (term->ldisc && term->id_string[0]) + if (term->ldisc) ldisc_send(term->ldisc, term->id_string, strlen(term->id_string), false); break; @@ -4431,8 +4430,7 @@ static void term_out(Terminal *term) len = strlen(p); ldisc_send(term->ldisc, "\033]L", 3, false); - if (len > 0) - ldisc_send(term->ldisc, p, len, false); + ldisc_send(term->ldisc, p, len, false); ldisc_send(term->ldisc, "\033\\", 2, false); } @@ -4447,8 +4445,7 @@ static void term_out(Terminal *term) len = strlen(p); ldisc_send(term->ldisc, "\033]l", 3, false); - if (len > 0) - ldisc_send(term->ldisc, p, len, false); + ldisc_send(term->ldisc, p, len, false); ldisc_send(term->ldisc, "\033\\", 2, false); } From 371c7d12f5a475e6351257592b6a9e89b0797787 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 11 Feb 2020 19:11:02 +0000 Subject: [PATCH 62/62] Remove white dialog background in MSI user interface. We received a report that if you enable Windows 10's high-contrast mode, the text in PuTTY's installer UI becomes invisible, because it's displayed in the system default foreground colour against a background of the white right-hand side of our 'msidialog.bmp' image. That's fine when the system default fg is black, but high-contrast mode flips it to white, and now you have white on white text, oops. Some research in the WiX bug tracker suggests that in Windows 10 you don't actually have to use BMP files for your installer images any more: you can use PNG, and PNGs can be transparent. However, someone else reported that that only works in up-to-date versions of Windows. And in fact there's no need to go that far. A more elegant answer is to simply not cover the whole dialog box with our background image in the first place. I've reduced the size of the background image so that it _only_ contains the pretty picture on the left-hand side, and omits the big white rectangle that used to sit under the text. So now the RHS of the dialog is not covered by any image at all, which has the same effect as it being covered with a transparent image, except that it doesn't require transparency support from msiexec. Either way, the background for the text ends up being the system's default dialog-box background, in the absence of any images or controls placed on top of it - so when the high-contrast mode is enabled, it flips to black at the same time as the text flips to white, and everything works as it should. The slight snag is that the pre-cooked WiX UI dialog specifications let you override the background image itself, but not the Width and Height fields in the control specifications that refer to them. So if you just try to drop in a narrow image in the most obvious way, it gets stretched across the whole window. But that's not a show-stopper, because we're not 100% dependent on getting WiX to produce exactly the right output. We already have the technology to postprocess the MSI _after_ it comes out of WiX: we're using it to fiddle the target-platform field for the Windows on Arm installers. So all I had to do was to turn msiplatform.py into a more general msifixup.py, add a second option to change the width of the dialog background image, and run it on the x86 installers as well as the Arm ones. --- Buildscr | 21 ++++++++---- windows/make_install_images.sh | 22 +++++++++++++ windows/{msiplatform.py => msifixup.py} | 43 ++++++++++++++++--------- 3 files changed, 65 insertions(+), 21 deletions(-) create mode 100755 windows/make_install_images.sh rename windows/{msiplatform.py => msifixup.py} (52%) diff --git a/Buildscr b/Buildscr index 2b789d4c..a98f5f3f 100644 --- a/Buildscr +++ b/Buildscr @@ -152,8 +152,7 @@ in putty do perl -i~ -pe 'y/\015//d;s/$$/\015/' LICENCE # Some gratuitous theming for the MSI installer UI. in putty/icons do make -j$(nproc) -in putty do convert -size 164x312 'gradient:blue-white' -distort SRT -90 -swirl 180 \( -size 329x312 canvas:white \) +append \( icons/putty-48.png -geometry +28+24 \) -composite \( icons/pscp-48.png -geometry +88+96 \) -composite \( icons/puttygen-48.png -geometry +28+168 \) -composite \( icons/pageant-48.png -geometry +88+240 \) -composite windows/msidialog.bmp -in putty do convert -size 493x58 canvas:white \( icons/putty-48.png -geometry +440+5 \) -composite windows/msibanner.bmp +in putty do ./windows/make_install_images.sh mkdir putty/windows/build32 mkdir putty/windows/build64 @@ -200,10 +199,20 @@ in putty/windows with wixonlinux do candle -arch x64 -dRealPlatform=x64 -dDllOk= in putty/windows with wixonlinux do candle -arch x64 -dRealPlatform=Arm -dDllOk=no -dBuilddir=abuild32/ -dWinver="$(Winver)" -dPuttytextver="$(Puttytextver)" installer.wxs && light -ext WixUIExtension -ext WixUtilExtension -sval installer.wixobj -o installera32.msi -spdb in putty/windows with wixonlinux do candle -arch x64 -dRealPlatform=Arm64 -dDllOk=no -dBuilddir=abuild64/ -dWinver="$(Winver)" -dPuttytextver="$(Puttytextver)" installer.wxs && light -ext WixUIExtension -ext WixUtilExtension -sval installer.wixobj -o installera64.msi -spdb -# Bodge the platform fields for the Windows on Arm installers, since -# WiX 3 doesn't understand Arm platform names itself. -in putty/windows do ./msiplatform.py installera32.msi Arm -in putty/windows do ./msiplatform.py installera64.msi Arm64 +# Change the width field for our dialog background image so that it +# doesn't stretch across the whole dialog. (WiX's default one does; we +# replace it with a narrow one so that the text to the right of it +# shows up on system default background colour, meaning that +# high-contrast mode doesn't make the text white on white. But that +# means we also have to modify the width field, and there's nothing in +# WiX's source syntax to make that happen.) +# +# Also bodge the platform fields for the Windows on Arm installers, +# since WiX 3 doesn't understand Arm platform names itself. +in putty/windows do ./msifixup.py installer32.msi --dialog-bmp-width=123 +in putty/windows do ./msifixup.py installer64.msi --dialog-bmp-width=123 +in putty/windows do ./msifixup.py installera32.msi --dialog-bmp-width=123 --platform=Arm +in putty/windows do ./msifixup.py installera64.msi --dialog-bmp-width=123 --platform=Arm64 # Sign the Windows installers. ifneq "$(cross_winsigncode)" "" in putty/windows do $(cross_winsigncode) -i https://www.chiark.greenend.org.uk/~sgtatham/putty/ -n "PuTTY Installer" installer32.msi installer64.msi installera32.msi installera64.msi diff --git a/windows/make_install_images.sh b/windows/make_install_images.sh new file mode 100755 index 00000000..49041e24 --- /dev/null +++ b/windows/make_install_images.sh @@ -0,0 +1,22 @@ +#!/bin/sh + +# Script to make the bitmap files that go into the PuTTY MSI installer. + +set -e + +# For convenience, allow this script to be run from the Windows +# subdirectory as well as the top level of the source tree. +if test -f installer.wxs -a ! -f putty.h -a -f ../putty.h; then + cd .. +fi + +convert -size 164x312 'gradient:blue-white' -distort SRT -90 -swirl 180 \ + \( icons/putty-48.png -geometry +28+24 \) -composite \ + \( icons/pscp-48.png -geometry +88+96 \) -composite \ + \( icons/puttygen-48.png -geometry +28+168 \) -composite \ + \( icons/pageant-48.png -geometry +88+240 \) -composite \ + windows/msidialog.bmp + +convert -size 493x58 canvas:white \ + \( icons/putty-48.png -geometry +440+5 \) -composite \ + windows/msibanner.bmp diff --git a/windows/msiplatform.py b/windows/msifixup.py similarity index 52% rename from windows/msiplatform.py rename to windows/msifixup.py index eea4272f..4d4bb3e4 100755 --- a/windows/msiplatform.py +++ b/windows/msifixup.py @@ -16,30 +16,43 @@ def run(command, verbose): sys.stdout.write("".join( "> {}\n".format(line) for line in out.splitlines())) -def set_platform(msi, platform, verbose): - run(["msidump", "-t", msi], verbose) +def make_changes(msi, args): + run(["msidump", "-t", msi], args.verbose) + build_cmd = ["msibuild", msi] - summary_stream = "_SummaryInformation.idt" + def change_table(filename): + with open(filename) as fh: + lines = [line.rstrip("\r\n").split("\t") + for line in iter(fh.readline, "")] - with open(summary_stream) as fh: - lines = [line.rstrip("\r\n").split("\t") - for line in iter(fh.readline, "")] + for line in lines[3:]: + yield line - for line in lines[3:]: - if line[0] == "7": - line[1] = ";".join([platform] + line[1].split(";", 1)[1:]) + with open(filename, "w") as fh: + for line in lines: + fh.write("\t".join(line) + "\r\n") - with open(summary_stream, "w") as fh: - for line in lines: - fh.write("\t".join(line) + "\r\n") + build_cmd.extend(["-i", filename]) - run(["msibuild", msi, "-i", summary_stream], verbose) + if args.platform is not None: + for line in change_table("_SummaryInformation.idt"): + if line[0] == "7": + line[1] = ";".join([args.platform] + line[1].split(";", 1)[1:]) + + if args.dialog_bmp_width is not None: + for line in change_table("Control.idt"): + if line[9] == "WixUI_Bmp_Dialog": + line[5] = args.dialog_bmp_width + + run(build_cmd, args.verbose) def main(): parser = argparse.ArgumentParser( description='Change the platform field of an MSI installer package.') parser.add_argument("msi", help="MSI installer file.") - parser.add_argument("platform", help="New value for the platform field.") + parser.add_argument("--platform", help="Change the platform field.") + parser.add_argument("--dialog-bmp-width", help="Change the width field" + " in all uses of WixUI_Bmp_Dialog.") parser.add_argument("-v", "--verbose", action="store_true", help="Log what this script is doing.") parser.add_argument("-k", "--keep", action="store_true", @@ -51,7 +64,7 @@ def main(): try: tempdir = tempfile.mkdtemp(dir=msidir) os.chdir(tempdir) - set_platform(msi, args.platform, args.verbose) + make_changes(msi, args) finally: if args.keep: sys.stdout.write(