From 2ef23bb8128503feefa36ca90a85c74e4dd62a9b Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 22 Nov 2014 10:12:47 +0000 Subject: [PATCH 1/6] Fix typo in validate_manual_hostkey(). 'p += strcspn' returns p always non-NULL and sometimes pointing at \0, as opposed to 'p = strchr' which returns p sometimes non-NULL and never pointing at \0. Test the pointer after the call accordingly. Thanks, Coverity. --- misc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/misc.c b/misc.c index 24e42eba..63de9d11 100644 --- a/misc.c +++ b/misc.c @@ -943,7 +943,7 @@ int validate_manual_hostkey(char *key) while ((p += strspn(p, " \t"))[0]) { q = p; p += strcspn(p, " \t"); - if (p) *p++ = '\0'; + if (*p) *p++ = '\0'; /* * Now q is our word. From 068b67d2f6e9b186b3107ebcb1e88a141b7b5ebc Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 22 Nov 2014 10:37:14 +0000 Subject: [PATCH 2/6] Clarify when ldisc->term may be NULL. Namely, any ldisc that you send actual data through should have a terminal attached, because the ldisc editing/echoing system is designed entirely for use with a terminal. The only time you can have an ldisc with no terminal is when it's only ever used by the backend to report changes to the front end in edit/echo status, e.g. by Unix Plink. Coverity spotted an oddity in ldisc_send which after a while I decided would never have actually caused a problem, but OTOH I agree that it was confusing, so now hopefully it's less so. --- ldisc.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/ldisc.c b/ldisc.c index 8f653a18..31185406 100644 --- a/ldisc.c +++ b/ldisc.c @@ -7,6 +7,7 @@ #include #include +#include #include "putty.h" #include "terminal.h" @@ -139,6 +140,17 @@ void ldisc_send(void *handle, char *buf, int len, int interactive) ldisc_update(ldisc->frontend, ECHOING, EDITING); return; } + + /* + * If that wasn't true, then we expect ldisc->term to be non-NULL + * hereafter. (The only front ends which have an ldisc but no term + * are those which do networking but no terminal emulation, in + * which case they need the above if statement to handle + * ldisc_updates passed from the back ends, but should never send + * any actual input through this function.) + */ + assert(ldisc->term); + /* * Notify the front end that something was pressed, in case * it's depending on finding out (e.g. keypress termination for @@ -146,7 +158,7 @@ void ldisc_send(void *handle, char *buf, int len, int interactive) */ frontend_keypress(ldisc->frontend); - if (interactive && ldisc->term) { + if (interactive) { /* * Interrupt a paste from the clipboard, if one was in * progress when the user pressed a key. This is easier than From b6c2346173ee1a8b8cd6ec045bb80243e47400f2 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 22 Nov 2014 10:18:16 +0000 Subject: [PATCH 3/6] Fix uninitialised variable in two Windows event loops. If (Msg)WaitForMultipleObjects returns WAIT_TIMEOUT, we expect 'next' to have been initialised. This can occur without having called run_timers(), if a toplevel callback was pending, so we can't expect run_timers to have reliably initialised 'next'. I'm not actually convinced this could have come up in either of the affected programs (Windows PSFTP and Plink), due to the list of things toplevel callbacks are currently used for, but it certainly wants fixing anyway for the future. Spotted by Coverity. --- windows/winplink.c | 3 +++ windows/winsftp.c | 3 +++ 2 files changed, 6 insertions(+) diff --git a/windows/winplink.c b/windows/winplink.c index 1c4f3307..43826622 100644 --- a/windows/winplink.c +++ b/windows/winplink.c @@ -661,6 +661,7 @@ int main(int argc, char **argv) if (toplevel_callback_pending()) { ticks = 0; + next = now; } else if (run_timers(now, &next)) { then = now; now = GETTICKCOUNT(); @@ -670,6 +671,8 @@ int main(int argc, char **argv) ticks = next - now; } else { ticks = INFINITE; + /* no need to initialise next here because we can never + * get WAIT_TIMEOUT */ } handles = handle_get_events(&nhandles); diff --git a/windows/winsftp.c b/windows/winsftp.c index 25ac6c94..f37ef243 100644 --- a/windows/winsftp.c +++ b/windows/winsftp.c @@ -495,6 +495,7 @@ int do_eventsel_loop(HANDLE other_event) if (toplevel_callback_pending()) { ticks = 0; + next = now; } else if (run_timers(now, &next)) { then = now; now = GETTICKCOUNT(); @@ -504,6 +505,8 @@ int do_eventsel_loop(HANDLE other_event) ticks = next - now; } else { ticks = INFINITE; + /* no need to initialise next here because we can never get + * WAIT_TIMEOUT */ } handles = handle_get_events(&nhandles); From 90dcef3d9e60f7f81193e433771bcb11e057cf11 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 22 Nov 2014 09:58:15 +0000 Subject: [PATCH 4/6] Fix assorted memory leaks. All spotted by Coverity. --- ssh.c | 3 +++ sshshare.c | 16 ++++++++++++++-- windows/winshare.c | 2 ++ 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/ssh.c b/ssh.c index 093c40ec..d531dfd4 100644 --- a/ssh.c +++ b/ssh.c @@ -3867,6 +3867,7 @@ static int do_ssh1_login(Ssh ssh, unsigned char *in, int inlen, s->dlgret = verify_ssh_manual_host_key(ssh, fingerprint, NULL, NULL); if (s->dlgret == 0) { /* did not match */ bombout(("Host key did not appear in manually configured list")); + sfree(keystr); crStop(0); } else if (s->dlgret < 0) { /* none configured; use standard handling */ ssh_set_frozen(ssh, 1); @@ -3893,6 +3894,8 @@ static int do_ssh1_login(Ssh ssh, unsigned char *in, int inlen, NULL, 0, TRUE); crStop(0); } + } else { + sfree(keystr); } } diff --git a/sshshare.c b/sshshare.c index 5b88eb8e..df64d6fe 100644 --- a/sshshare.c +++ b/sshshare.c @@ -857,6 +857,7 @@ static void share_try_cleanup(struct ssh_sharing_connstate *cs) SSH2_MSG_GLOBAL_REQUEST, packet, pos, "cleanup after" " downstream went away"); + sfree(packet); share_remove_forwarding(cs, fwd); i--; /* don't accidentally skip one as a result */ @@ -1594,6 +1595,9 @@ static void share_got_pkt_from_downstream(struct ssh_sharing_connstate *cs, !ssh_agent_forwarding_permitted(cs->parent->ssh)) { unsigned server_id = GET_32BIT(pkt); unsigned char recipient_id[4]; + + sfree(request_name); + chan = share_find_channel_by_server(cs, server_id); if (chan) { PUT_32BIT(recipient_id, chan->downstream_id); @@ -1625,6 +1629,8 @@ static void share_got_pkt_from_downstream(struct ssh_sharing_connstate *cs, int auth_proto, protolen, datalen; int pos; + sfree(request_name); + chan = share_find_channel_by_server(cs, server_id); if (!chan) { char *buf = dupprintf("X11 forwarding request for " @@ -1646,16 +1652,19 @@ static void share_got_pkt_from_downstream(struct ssh_sharing_connstate *cs, want_reply = pkt[15] != 0; single_connection = pkt[16] != 0; auth_proto_str = getstring(pkt+17, pktlen-17); + auth_proto = x11_identify_auth_proto(auth_proto_str); + sfree(auth_proto_str); pos = 17 + getstring_size(pkt+17, pktlen-17); auth_data = getstring(pkt+pos, pktlen-pos); pos += getstring_size(pkt+pos, pktlen-pos); + if (pktlen < pos+4) { err = dupprintf("Truncated CHANNEL_REQUEST(\"x11\") packet"); + sfree(auth_data); goto confused; } screen = GET_32BIT(pkt+pos); - auth_proto = x11_identify_auth_proto(auth_proto_str); if (auth_proto < 0) { /* Reject due to not understanding downstream's * requested authorisation method. */ @@ -1668,6 +1677,7 @@ static void share_got_pkt_from_downstream(struct ssh_sharing_connstate *cs, chan->x11_auth_proto = auth_proto; chan->x11_auth_data = x11_dehexify(auth_data, &chan->x11_auth_datalen); + sfree(auth_data); chan->x11_auth_upstream = ssh_sharing_add_x11_display(cs->parent->ssh, auth_proto, cs, chan); @@ -1700,6 +1710,8 @@ static void share_got_pkt_from_downstream(struct ssh_sharing_connstate *cs, break; } + + sfree(request_name); } ssh_send_packet_from_downstream(cs->parent->ssh, cs->id, @@ -2099,7 +2111,7 @@ Socket ssh_connection_sharing_init(const char *host, int port, sharestate->connections = newtree234(share_connstate_cmp); sharestate->ssh = ssh; sharestate->server_verstring = NULL; - sharestate->sockname = dupstr(sockname); + sharestate->sockname = sockname; sharestate->nextid = 1; return NULL; } diff --git a/windows/winshare.c b/windows/winshare.c index a63325cb..2f21638e 100644 --- a/windows/winshare.c +++ b/windows/winshare.c @@ -157,6 +157,7 @@ int platform_ssh_share(const char *pi_name, Conf *conf, if (!make_private_security_descriptor(MUTEX_ALL_ACCESS, &psd, &acl, logtext)) { sfree(mutexname); + sfree(name); return SHARE_NONE; } @@ -171,6 +172,7 @@ int platform_ssh_share(const char *pi_name, Conf *conf, *logtext = dupprintf("CreateMutex(\"%s\") failed: %s", mutexname, win_strerror(GetLastError())); sfree(mutexname); + sfree(name); LocalFree(psd); LocalFree(acl); return SHARE_NONE; From 69d50b2877ea6cb2a23a75e563dbca7548dacd38 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 22 Nov 2014 09:59:37 +0000 Subject: [PATCH 5/6] Don't reject _and_ accept X forwarding requests! If a sharing downstream asks for an auth method we don't understand, we should send them CHANNEL_FAILURE *and then stop processing*. Ahem. (Spotted while examining this code in the course of Coverity-related fixes, but not itself a Coverity-found problem.) --- sshshare.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sshshare.c b/sshshare.c index df64d6fe..54d58a66 100644 --- a/sshshare.c +++ b/sshshare.c @@ -1672,6 +1672,8 @@ static void share_got_pkt_from_downstream(struct ssh_sharing_connstate *cs, PUT_32BIT(recipient_id, chan->downstream_id); send_packet_to_downstream(cs, SSH2_MSG_CHANNEL_FAILURE, recipient_id, 4, NULL); + sfree(auth_data); + break; } chan->x11_auth_proto = auth_proto; From f45423544437d117d103a3406550dcd535c614b9 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 22 Nov 2014 14:57:06 +0000 Subject: [PATCH 6/6] Add some missing initialisations. Spotted by valgrind, after I was testing all the Coverity bug fixes :-) --- dialog.c | 2 ++ ssh.c | 2 ++ 2 files changed, 4 insertions(+) diff --git a/dialog.c b/dialog.c index cbe95124..cc198775 100644 --- a/dialog.c +++ b/dialog.c @@ -381,6 +381,7 @@ union control *ctrl_droplist(struct controlset *s, char *label, char shortcut, c->listbox.percentwidth = percentage; c->listbox.ncols = 0; c->listbox.percentages = NULL; + c->listbox.hscroll = FALSE; return c; } @@ -397,6 +398,7 @@ union control *ctrl_draglist(struct controlset *s,char *label,char shortcut, c->listbox.percentwidth = 100; c->listbox.ncols = 0; c->listbox.percentages = NULL; + c->listbox.hscroll = FALSE; return c; } diff --git a/ssh.c b/ssh.c index d531dfd4..a7f5882f 100644 --- a/ssh.c +++ b/ssh.c @@ -10598,6 +10598,8 @@ static const char *ssh_init(void *frontend_handle, void **backend_handle, ssh->sent_console_eof = FALSE; ssh->got_pty = FALSE; ssh->bare_connection = FALSE; + ssh->X11_fwd_enabled = FALSE; + ssh->connshare = NULL; ssh->attempting_connshare = FALSE; *backend_handle = ssh;