From bc6c15ab5f636e05b7e91883f0031a7e06117947 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Wed, 24 Feb 2016 20:13:10 +0000 Subject: [PATCH 1/6] Fix vulnerability CVE-2016-2563 in old scp protocol. There was a rogue sscanf("%s") with no field width limit, targeting a stack-based buffer, and scanning a string containing untrusted data. It occurs in the 'sink' side of the protocol, i.e. when downloading files *from* the server. Our own bug id for this vulnerability is 'vuln-pscp-sink-sscanf'. --- pscp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pscp.c b/pscp.c index 3e41454d..dc9e1f50 100644 --- a/pscp.c +++ b/pscp.c @@ -1528,7 +1528,7 @@ int scp_get_sink_action(struct scp_sink_action *act) { char sizestr[40]; - if (sscanf(act->buf, "%lo %s %n", &act->permissions, + if (sscanf(act->buf, "%lo %39s %n", &act->permissions, sizestr, &i) != 2) bump("Protocol error: Illegal file descriptor format"); act->size = uint64_from_decimal(sizestr); From b49a8db1b42d12a035b0d17d8c101b8556920112 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 29 Feb 2016 19:38:12 +0000 Subject: [PATCH 2/6] Tighten up pointer handling after ssh_pkt_getstring. ssh_pkt_getstring can return (NULL,0) if the input packet is too short to contain a valid string. In quite a few places we were passing the returned pointer,length pair to a printf function with "%.*s" type format, which seems in practice to have not been dereferencing the pointer but the C standard doesn't actually guarantee that. In one place we were doing the same job by hand with memcpy, and apparently that _can_ dereference the pointer in practice (so a server could have caused a NULL-dereference crash by sending an appropriately malformed "x11" type channel open request). And also I spotted a logging call in the "forwarded-tcpip" channel open handler which had forgotten the field width completely, so it was erroneously relying on the string happening to be NUL-terminated in the received packet. I've tightened all of this up in general by normalising (NULL,0) to ("",0) before calling printf("%.*s"), and replacing the two even more broken cases with the corrected version of that same idiom. --- misc.h | 5 +++++ ssh.c | 27 ++++++++++++++------------- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/misc.h b/misc.h index e53f8929..db7f9143 100644 --- a/misc.h +++ b/misc.h @@ -169,4 +169,9 @@ void debug_memdump(void *buf, int len, int L); (cp)[0] = (unsigned char)((value) >> 8), \ (cp)[1] = (unsigned char)(value) ) +/* Replace NULL with the empty string, permitting an idiom in which we + * get a string (pointer,length) pair that might be NULL,0 and can + * then safely say things like printf("%.*s", length, NULLTOEMPTY(ptr)) */ +#define NULLTOEMPTY(s) ((s)?(s):"") + #endif diff --git a/ssh.c b/ssh.c index 1077209a..e1e94d78 100644 --- a/ssh.c +++ b/ssh.c @@ -5419,7 +5419,7 @@ static void ssh1_msg_port_open(Ssh ssh, struct Packet *pktin) ssh_pkt_getstring(pktin, &host, &hostsize); port = ssh_pkt_getuint32(pktin); - pf.dhost = dupprintf("%.*s", hostsize, host); + pf.dhost = dupprintf("%.*s", hostsize, NULLTOEMPTY(host)); pf.dport = port; pfp = find234(ssh->rportfwds, &pf, NULL); @@ -5902,7 +5902,7 @@ static void ssh1_msg_debug(Ssh ssh, struct Packet *pktin) int msglen; ssh_pkt_getstring(pktin, &msg, &msglen); - logeventf(ssh, "Remote debug message: %.*s", msglen, msg); + logeventf(ssh, "Remote debug message: %.*s", msglen, NULLTOEMPTY(msg)); } static void ssh1_msg_disconnect(Ssh ssh, struct Packet *pktin) @@ -5912,7 +5912,8 @@ static void ssh1_msg_disconnect(Ssh ssh, struct Packet *pktin) int msglen; ssh_pkt_getstring(pktin, &msg, &msglen); - bombout(("Server sent disconnect message:\n\"%.*s\"", msglen, msg)); + bombout(("Server sent disconnect message:\n\"%.*s\"", + msglen, NULLTOEMPTY(msg))); } static void ssh_msg_ignore(Ssh ssh, struct Packet *pktin) @@ -7960,7 +7961,8 @@ static void ssh2_msg_channel_open_failure(Ssh ssh, struct Packet *pktin) reason_code = 0; /* ensure reasons[reason_code] in range */ ssh_pkt_getstring(pktin, &reason_string, &reason_length); logeventf(ssh, "Forwarded connection refused by server: %s [%.*s]", - reasons[reason_code], reason_length, reason_string); + reasons[reason_code], reason_length, + NULLTOEMPTY(reason_string)); pfd_close(c->u.pfd.pf); } else if (c->type == CHAN_ZOMBIE) { @@ -8255,9 +8257,7 @@ static void ssh2_msg_channel_open(Ssh ssh, struct Packet *pktin) char *addrstr; ssh_pkt_getstring(pktin, &peeraddr, &peeraddrlen); - addrstr = snewn(peeraddrlen+1, char); - memcpy(addrstr, peeraddr, peeraddrlen); - addrstr[peeraddrlen] = '\0'; + addrstr = dupprintf("%.*s", peeraddrlen, NULLTOEMPTY(peeraddr)); peerport = ssh_pkt_getuint32(pktin); logeventf(ssh, "Received X11 connect request from %s:%d", @@ -8292,13 +8292,14 @@ static void ssh2_msg_channel_open(Ssh ssh, struct Packet *pktin) char *shost; int shostlen; ssh_pkt_getstring(pktin, &shost, &shostlen);/* skip address */ - pf.shost = dupprintf("%.*s", shostlen, shost); + pf.shost = dupprintf("%.*s", shostlen, NULLTOEMPTY(shost)); pf.sport = ssh_pkt_getuint32(pktin); ssh_pkt_getstring(pktin, &peeraddr, &peeraddrlen); peerport = ssh_pkt_getuint32(pktin); realpf = find234(ssh->rportfwds, &pf, NULL); logeventf(ssh, "Received remote port %s:%d open request " - "from %s:%d", pf.shost, pf.sport, peeraddr, peerport); + "from %.*s:%d", pf.shost, pf.sport, + peeraddrlen, NULLTOEMPTY(peeraddr), peerport); sfree(pf.shost); if (realpf == NULL) { @@ -9957,7 +9958,7 @@ static void do_ssh2_authconn(Ssh ssh, unsigned char *in, int inlen, s->cur_prompt->to_server = TRUE; s->cur_prompt->name = dupstr("New SSH password"); s->cur_prompt->instruction = - dupprintf("%.*s", prompt_len, prompt); + dupprintf("%.*s", prompt_len, NULLTOEMPTY(prompt)); s->cur_prompt->instr_reqd = TRUE; /* * There's no explicit requirement in the protocol @@ -10394,13 +10395,13 @@ static void ssh2_msg_disconnect(Ssh ssh, struct Packet *pktin) logevent(buf); sfree(buf); buf = dupprintf("Disconnection message text: %.*s", - msglen, msg); + msglen, NULLTOEMPTY(msg)); logevent(buf); bombout(("Server sent disconnect message\ntype %d (%s):\n\"%.*s\"", reason, (reason > 0 && reason < lenof(ssh2_disconnect_reasons)) ? ssh2_disconnect_reasons[reason] : "unknown", - msglen, msg)); + msglen, NULLTOEMPTY(msg))); sfree(buf); } @@ -10414,7 +10415,7 @@ static void ssh2_msg_debug(Ssh ssh, struct Packet *pktin) ssh2_pkt_getbool(pktin); ssh_pkt_getstring(pktin, &msg, &msglen); - logeventf(ssh, "Remote debug message: %.*s", msglen, msg); + logeventf(ssh, "Remote debug message: %.*s", msglen, NULLTOEMPTY(msg)); } static void ssh2_msg_transport(Ssh ssh, struct Packet *pktin) From 5ee166aab63c6960d9b153e7a307b80c2474825f Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 27 Feb 2016 08:52:45 +0000 Subject: [PATCH 3/6] Mention the new Secure Contact Key in the GPG docs appendix. The reporter of vuln-pscp-sink-sscanf asked for a key to encrypt the vulnerability report with, and having generated one, it seemed like a good idea to make it part of the official PuTTY GPG key set and publish it for the next person to use. --- doc/pgpkeys.but | 51 +++++++++++++++++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 14 deletions(-) diff --git a/doc/pgpkeys.but b/doc/pgpkeys.but index 3f772793..e93ee5ec 100644 --- a/doc/pgpkeys.but +++ b/doc/pgpkeys.but @@ -22,11 +22,11 @@ the origin of files distributed by the PuTTY team.) \H{pgpkeys-pubkey} Public keys -We maintain a set of three keys, stored with different levels of -security due to being used in different ways. See \k{pgpkeys-security} -below for details. +We maintain multiple keys, stored with different levels of security +due to being used in different ways. See \k{pgpkeys-security} below +for details. -The three keys we provide are: +The keys we provide are: \dt Snapshot Key @@ -38,15 +38,20 @@ we send to particular users. \dd Used to sign manually released versions of PuTTY. +\dt Secure Contact Key + +\dd An encryption-capable key suitable for people to send confidential +messages to the PuTTY team, e.g. reports of vulnerabilities. + \dt Master Key -\dd Used to tie the other two keys into the GPG web of trust. The -Master Key signs the other two keys, and other GPG users have signed +\dd Used to tie all the above keys into the GPG web of trust. The +Master Key signs all the other keys, and other GPG users have signed it in turn. -The current issue of those three keys are available for download from -the PuTTY website, and are also available on PGP keyservers using the -key IDs listed below. +The current issue of those keys are available for download from the +PuTTY website, and are also available on PGP keyservers using the key +IDs listed below. \dt \W{http://www.chiark.greenend.org.uk/~sgtatham/putty/keys/master-2015.asc}{\s{Master Key}} @@ -60,6 +65,14 @@ key IDs listed below. \cw{2048R/9DFE2648B43434E4}). Fingerprint: \cw{0054\_DDAA\_8ADA\_15D2\_768A\_\_6DE7\_9DFE\_2648\_B434\_34E4} +\dt \W{http://www.chiark.greenend.org.uk/~sgtatham/putty/keys/contact-2016.asc}{\s{Secure Contact Key}} + +\dd RSA, 2048-bit. Main key ID: \cw{2048R/8A0AF00B} (long version: +\cw{2048R/C4FCAAD08A0AF00B}). Encryption subkey ID: +\cw{2048R/50C2CF5C} (long version: \cw{2048R/9EB39CC150C2CF5C}. +Fingerprint: +\cw{8A26\_250E\_763F\_E359\_75F3\_\_118F\_C4FC\_AAD0\_8A0A\_F00B} + \dt \W{http://www.chiark.greenend.org.uk/~sgtatham/putty/keys/snapshot-2015.asc}{\s{Snapshot Key}} \dd RSA, 2048-bit. Key ID: \cw{2048R/D15F7E8A} (long version: @@ -115,6 +128,12 @@ The Releases private key is kept encrypted on the developers' own local machines. So an attacker wanting to steal it would have to also steal the passphrase. +\S{pgpkeys-contact} The Secure Contact Key + +The Secure Contact Key is stored with a similar level of security to +the Release Key: it is stored with a passphrase, and no automated +script has access to it. + \S{pgpkeys-master} The Master Keys The Master Key signs almost nothing. Its purpose is to bind the other @@ -137,11 +156,15 @@ once. \H{pgpkeys-rollover} Key rollover -Our current three keys were generated in September 2015. Prior to -that, we had a much older set of keys generated in 2000. For each of -the three key types above, we provided both an RSA key \e{and} a DSA -key (because at the time we generated them, RSA was not in practice -available to everyone, due to export restrictions). +Our current keys were generated in September 2015, except for the +Secure Contact Key which was generated in February 2016 (we didn't +think of it until later). + +Prior to that, we had a much older set of keys generated in 2000. For +each of the key types above (other than the Secure Contact Key), we +provided both an RSA key \e{and} a DSA key (because at the time we +generated them, RSA was not in practice available to everyone, due to +export restrictions). The new Master Key is signed with both of the old ones, to show that it really is owned by the same people and not substituted by an From 9c6a600e5bf02d86d2eec8fa47be1277cb22ed8f Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 27 Feb 2016 09:25:23 +0000 Subject: [PATCH 4/6] Make get_user_sid() return the cached copy if one already exists. A user reported in January that locking down our process ACL causes get_user_sid's call to OpenProcessToken to fail with a permissions error. This _shouldn't_ be important, because we'll already have found and cached the user SID before getting that far - but unfortunately the call to get_user_sid in winnpc.c was bypassing the cache and trying the whole process again. This fix changes the memory ownership semantics of get_user_sid(): it's now an error to free the value it gives you, or else the *next* call to get_user_sid() will return a stale pointer. Hence, also removed those frees everywhere they appear. --- windows/winnpc.c | 3 --- windows/winpgnt.c | 4 ---- windows/winpgntc.c | 1 - windows/winsecur.c | 5 ++++- 4 files changed, 4 insertions(+), 9 deletions(-) diff --git a/windows/winnpc.c b/windows/winnpc.c index 0e8ac699..5927df32 100644 --- a/windows/winnpc.c +++ b/windows/winnpc.c @@ -79,7 +79,6 @@ Socket new_named_pipe_client(const char *pipename, Plug plug) ret = new_error_socket(err, plug); sfree(err); CloseHandle(pipehandle); - sfree(usersid); return ret; } @@ -89,12 +88,10 @@ Socket new_named_pipe_client(const char *pipename, Plug plug) sfree(err); CloseHandle(pipehandle); LocalFree(psd); - sfree(usersid); return ret; } LocalFree(psd); - sfree(usersid); return make_handle_socket(pipehandle, pipehandle, plug, TRUE); } diff --git a/windows/winpgnt.c b/windows/winpgnt.c index f4194a68..f6371538 100644 --- a/windows/winpgnt.c +++ b/windows/winpgnt.c @@ -1934,7 +1934,6 @@ static LRESULT CALLBACK WndProc(HWND hwnd, UINT message, debug(("couldn't get default SID\n")); #endif CloseHandle(filemap); - sfree(ourself); return 0; } @@ -1947,7 +1946,6 @@ static LRESULT CALLBACK WndProc(HWND hwnd, UINT message, rc)); #endif CloseHandle(filemap); - sfree(ourself); sfree(ourself2); return 0; } @@ -1968,7 +1966,6 @@ static LRESULT CALLBACK WndProc(HWND hwnd, UINT message, !EqualSid(mapowner, ourself2)) { CloseHandle(filemap); LocalFree(psd); - sfree(ourself); sfree(ourself2); return 0; /* security ID mismatch! */ } @@ -1976,7 +1973,6 @@ static LRESULT CALLBACK WndProc(HWND hwnd, UINT message, debug(("security stuff matched\n")); #endif LocalFree(psd); - sfree(ourself); sfree(ourself2); } else { #ifdef DEBUG_IPC diff --git a/windows/winpgntc.c b/windows/winpgntc.c index 036ca759..06649abc 100644 --- a/windows/winpgntc.c +++ b/windows/winpgntc.c @@ -182,6 +182,5 @@ int agent_query(void *in, int inlen, void **out, int *outlen, sfree(mapname); if (psd) LocalFree(psd); - sfree(usersid); return 1; } diff --git a/windows/winsecur.c b/windows/winsecur.c index 91ce7e92..95c1b6e1 100644 --- a/windows/winsecur.c +++ b/windows/winsecur.c @@ -44,6 +44,9 @@ PSID get_user_sid(void) DWORD toklen, sidlen; PSID sid = NULL, ret = NULL; + if (usersid) + return usersid; + if (!got_advapi()) goto cleanup; @@ -73,7 +76,7 @@ PSID get_user_sid(void) /* Success. Move sid into the return value slot, and null it out * to stop the cleanup code freeing it. */ - ret = sid; + ret = usersid = sid; sid = NULL; cleanup: From 29e8c24f90a47dfaa02bb7be3e5d59a1a6f34d5f Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 27 Feb 2016 09:17:27 +0000 Subject: [PATCH 5/6] Remove spurious -shareexists reference in Plink docs. That option does exist, but only on master; it was not in the 0.66 release. It turned up by mistake when I updated the documentation copy of the Plink online help while preparing the 0.66 release, because I ran plink from the wrong branch. The new release automation should stop that kind of mistake from happening in future. --- doc/plink.but | 2 -- 1 file changed, 2 deletions(-) diff --git a/doc/plink.but b/doc/plink.but index 589d19ef..9f40b5fc 100644 --- a/doc/plink.but +++ b/doc/plink.but @@ -80,8 +80,6 @@ use Plink: \c -N don't start a shell/command (SSH-2 only) \c -nc host:port \c open tunnel in place of session (SSH-2 only) -\c -shareexists -\c test whether a connection-sharing upstream exists Once this works, you are ready to use Plink. From 830b7f8898bf05c95e97ba6cc88692637cc8f60a Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 29 Feb 2016 19:59:59 +0000 Subject: [PATCH 6/6] Update version number for 0.67 release. --- Buildscr | 2 +- LATEST.VER | 2 +- doc/plink.but | 5 ++++- doc/pscp.but | 5 ++++- windows/putty.iss | 8 ++++---- 5 files changed, 14 insertions(+), 8 deletions(-) diff --git a/Buildscr b/Buildscr index c9420658..fb228d9c 100644 --- a/Buildscr +++ b/Buildscr @@ -35,7 +35,7 @@ module putty ifeq "$(RELEASE)" "" set Ndate $(!builddate) ifneq "$(Ndate)" "" in . do echo $(Ndate) | perl -pe 's/(....)(..)(..)/$$1-$$2-$$3/' > date ifneq "$(Ndate)" "" read Date date -set Epoch 15746 # update this at every release +set Epoch 15860 # update this at every release ifneq "$(Ndate)" "" in . do echo $(Ndate) | perl -ne 'use Time::Local; /(....)(..)(..)/ and print timegm(0,0,0,$$3,$$2-1,$$1) / 86400 - $(Epoch)' > days ifneq "$(Ndate)" "" read Days days diff --git a/LATEST.VER b/LATEST.VER index ceee7dd5..7e2a4fb1 100644 --- a/LATEST.VER +++ b/LATEST.VER @@ -1 +1 @@ -0.66 +0.67 diff --git a/doc/plink.but b/doc/plink.but index 9f40b5fc..ec5d162c 100644 --- a/doc/plink.but +++ b/doc/plink.but @@ -41,7 +41,7 @@ use Plink: \c Z:\sysosd>plink \c Plink: command-line connection utility -\c Release 0.66 +\c Release 0.67 \c Usage: plink [options] [user@]host [command] \c ("host" can also be a PuTTY saved session name) \c Options: @@ -80,6 +80,9 @@ use Plink: \c -N don't start a shell/command (SSH-2 only) \c -nc host:port \c open tunnel in place of session (SSH-2 only) +\c -sshlog file +\c -sshrawlog file +\c log protocol details to a file Once this works, you are ready to use Plink. diff --git a/doc/pscp.but b/doc/pscp.but index 89ca90b5..2b197275 100644 --- a/doc/pscp.but +++ b/doc/pscp.but @@ -39,7 +39,7 @@ use PSCP: \c Z:\owendadmin>pscp \c PuTTY Secure Copy client -\c Release 0.66 +\c Release 0.67 \c Usage: pscp [options] [user@]host:source target \c pscp [options] source [source...] [user@]host:target \c pscp [options] -ls [user@]host:filespec @@ -66,6 +66,9 @@ use PSCP: \c -unsafe allow server-side wildcards (DANGEROUS) \c -sftp force use of SFTP protocol \c -scp force use of SCP protocol +\c -sshlog file +\c -sshrawlog file +\c log protocol details to a file (PSCP's interface is much like the Unix \c{scp} command, if you're familiar with that.) diff --git a/windows/putty.iss b/windows/putty.iss index 67c92d0f..5856909e 100644 --- a/windows/putty.iss +++ b/windows/putty.iss @@ -13,10 +13,10 @@ [Setup] AppName=PuTTY -AppVerName=PuTTY version 0.66 -VersionInfoTextVersion=Release 0.66 -AppVersion=0.66 -VersionInfoVersion=0.66.0.0 +AppVerName=PuTTY version 0.67 +VersionInfoTextVersion=Release 0.67 +AppVersion=0.67 +VersionInfoVersion=0.67.0.0 AppPublisher=Simon Tatham AppPublisherURL=http://www.chiark.greenend.org.uk/~sgtatham/putty/ AppReadmeFile={app}\README.txt