From 1f6fa876e3be4eb9753319bee5d17624f8400452 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 29 Jan 2022 17:58:33 +0000 Subject: [PATCH 1/7] do_bidi: remove a pointless assert. When the textlen parameter became a size_t, it became unsigned, so it stopped being useful to assert() its non-negativity. Spotted by Coverity. Harmless, but ordinary compilers have been known to emit annoying warnings about that kind of thing too, so it's worth fixing just to avoid noise. --- terminal/bidi.c | 1 - 1 file changed, 1 deletion(-) diff --git a/terminal/bidi.c b/terminal/bidi.c index c8085ca4..a56570fd 100644 --- a/terminal/bidi.c +++ b/terminal/bidi.c @@ -3602,7 +3602,6 @@ void do_bidi(BidiContext *ctx, bidi_char *text, size_t textlen) #ifdef REMOVE_FORMATTING_CHARACTERS abort(); /* can't use the standard algorithm in a live terminal */ #else - assert(textlen >= 0); do_bidi_new(ctx, text, textlen); #endif } From 7582ce3cd668da1d0546271c095e9d97790fc37a Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 29 Jan 2022 18:00:13 +0000 Subject: [PATCH 2/7] proxy_socks5_free: fix inadequate smemclr. Thanks to Coverity for pointing out that I'd only cleared sizeof(pointer) amount of the struct, not sizeof(the whole thing). --- proxy/socks5.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proxy/socks5.c b/proxy/socks5.c index 66797440..87a0bbc8 100644 --- a/proxy/socks5.c +++ b/proxy/socks5.c @@ -70,7 +70,7 @@ static void proxy_socks5_free(ProxyNegotiator *pn) strbuf_free(s->password); if (s->prompts) free_prompts(s->prompts); - smemclr(s, sizeof(s)); + smemclr(s, sizeof(*s)); sfree(s); } From af6a19e962c13cb61851b53f2d3c34b0b2a631ca Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 29 Jan 2022 18:03:33 +0000 Subject: [PATCH 3/7] sshproxy.c: add missing NULL check. If you try to use a saved session for SSH proxying which specifies a protocol that is not SSH or bare-SSH-connection, you get a clean error return from the proxy setup code - *provided* it's at least a protocol known to this particular build of PuTTY. If it's one so outlandish that backend_vt_from_proto returns NULL, there'd have been a crash. I don't think any such protocol currently exists, but if in the next version of PuTTY some additional protocol becomes supported, it will trip this error in the current version. Spotted by Coverity. --- proxy/sshproxy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proxy/sshproxy.c b/proxy/sshproxy.c index ee2bee6e..4f240d50 100644 --- a/proxy/sshproxy.c +++ b/proxy/sshproxy.c @@ -592,7 +592,7 @@ Socket *sshproxy_new_connection(SockAddr *addr, const char *hostname, * our check is for whether the backend sets the flag promising * that it does. */ - if (!(backvt->flags & BACKEND_SUPPORTS_NC_HOST)) { + if (!backvt || !(backvt->flags & BACKEND_SUPPORTS_NC_HOST)) { sp->errmsg = dupprintf("saved session '%s' is not an SSH session", proxy_hostname); return &sp->sock; From 6344e40e3f7524aa3479953b4e58daae349879de Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 29 Jan 2022 18:05:00 +0000 Subject: [PATCH 4/7] cmdline.c: free cmdline_password whenever it's reset. If you provided two -pw or -pwfile arguments on the same command line, the first password could be left in memory uncleared. Spotted by Coverity. --- cmdline.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/cmdline.c b/cmdline.c index c39c8ad2..9f37f2a0 100644 --- a/cmdline.c +++ b/cmdline.c @@ -585,6 +585,11 @@ int cmdline_process_param(const char *p, char *value, cmdline_error("the -pw option can only be used with the " "SSH protocol"); else { + if (cmdline_password) { + smemclr(cmdline_password, strlen(cmdline_password)); + sfree(cmdline_password); + } + cmdline_password = dupstr(value); /* Assuming that `value' is directly from argv, make a good faith * attempt to trample it, to stop it showing up in `ps' output @@ -608,6 +613,11 @@ int cmdline_process_param(const char *p, char *value, if (!fp) { cmdline_error("unable to open password file '%s'", value); } else { + if (cmdline_password) { + smemclr(cmdline_password, strlen(cmdline_password)); + sfree(cmdline_password); + } + cmdline_password = chomp(fgetline(fp)); if (!cmdline_password) { cmdline_error("unable to read a password from file '%s'", From 6d77541080f463b1c4743e834e41f31169fdf60a Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 29 Jan 2022 18:11:06 +0000 Subject: [PATCH 5/7] bidi_test: minor memory fixes. Spotted by Coverity: if you _just_ gave a filename to bidi_test, without any previous argument that set testfn to something other than NULL, the program would crash rather than giving an error message. (It's only a test program, but test programs you only run once in a blue moon are the ones that _most_ need to explain their command-line syntax to you carefully, because you've forgotten it since last time you used them!) Also, conditionalised a memcpy on the size not being 0, because it's illegal to pass a null pointer to memcpy _even_ if size==0. (That would only happen with a test case containing a zero-length string, but whatever.) --- terminal/bidi_test.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/terminal/bidi_test.c b/terminal/bidi_test.c index 0b63029f..1acd1d68 100644 --- a/terminal/bidi_test.c +++ b/terminal/bidi_test.c @@ -44,7 +44,8 @@ static void run_test(const char *filename, unsigned lineno, { size_t bcs_orig_len = bcs_len; bidi_char *bcs_orig = snewn(bcs_orig_len, bidi_char); - memcpy(bcs_orig, bcs, bcs_orig_len * sizeof(bidi_char)); + if (bcs_orig_len) + memcpy(bcs_orig, bcs, bcs_orig_len * sizeof(bidi_char)); bcs_len = do_bidi_test(ctx, bcs, bcs_len, override); @@ -335,6 +336,12 @@ int main(int argc, char **argv) } else { const char *filename = arg; + if (!testfn) { + fprintf(stderr, "no mode argument provided before filename " + "'%s'\n", filename); + return 1; + } + if (!strcmp(filename, "-")) { testfn("", stdin); } else { From d78d14f917d11deb9418fbd96a1ee05c1315c234 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 29 Jan 2022 18:19:58 +0000 Subject: [PATCH 6/7] HTTP proxy: fix nonsense HTTP version check. Substitution of && for || would have caused us to accept HTTP/1.0 when we meant to reject it. Thanks Coverity! --- proxy/http.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proxy/http.c b/proxy/http.c index f788e52c..081fefb6 100644 --- a/proxy/http.c +++ b/proxy/http.c @@ -479,7 +479,7 @@ static void proxy_http_process_queue(ProxyNegotiator *pn) crStopV; } - if (maj_ver < 1 && (maj_ver == 1 && min_ver < 1)) { + if (maj_ver < 1 || (maj_ver == 1 && min_ver < 1)) { /* Before HTTP/1.1, connections close by default */ s->connection_close = true; } From b7a9cdd6ee059a27821ae2e193c5881ec4d215da Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 29 Jan 2022 18:22:31 +0000 Subject: [PATCH 7/7] term_get_userpass_input: missing NULL check. If term_get_userpass_input is called with term->ldisc not yet set up, then we had a special-case handler that returns an error message - but it does it via the same subroutine that returns normal results, which also turns off the prompt callback in term->ldisc! Need an extra NULL check in that subroutine. Thanks Coverity. --- terminal/terminal.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/terminal/terminal.c b/terminal/terminal.c index b5d01813..3398cd59 100644 --- a/terminal/terminal.c +++ b/terminal/terminal.c @@ -7677,7 +7677,8 @@ static inline SeatPromptResult signal_prompts_t(Terminal *term, prompts_t *p, { assert(p->callback && "Asynchronous userpass input requires a callback"); queue_toplevel_callback(p->callback, p->callback_ctx); - ldisc_enable_prompt_callback(term->ldisc, NULL); + if (term->ldisc) + ldisc_enable_prompt_callback(term->ldisc, NULL); p->spr = spr; return spr; }