From d0aa8b2380ad7aa8e5a660417dfe25a12ff02d5f Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 7 Feb 2015 11:48:49 +0000 Subject: [PATCH 01/33] Improve comments in winhandl.c. To understand the handle leak bug that I fixed in git commit 7549f2da40d3666f2c9527d84d9ed5468e231691, I had to think fairly hard to remind myself what all this code was doing, which means the comments weren't good enough. Expanded and rewritten some of them in the hope that things will be clearer next time. (cherry picked from commit a87a14ae0fc7d4621b5b1fafdb2053b3638b4b2b) Cherry-picker's notes: this apparently pointless commit is required on this branch because it's a dependency of the rather less pointless 9fec2e773873e28f1409f5e1eefaf03483070050. --- windows/winhandl.c | 44 +++++++++++++++++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 7 deletions(-) diff --git a/windows/winhandl.c b/windows/winhandl.c index 6b129ad8..d81054f3 100644 --- a/windows/winhandl.c +++ b/windows/winhandl.c @@ -167,13 +167,27 @@ static DWORD WINAPI handle_input_threadfunc(void *param) SetEvent(ctx->ev_to_main); - if (!ctx->len) + if (!ctx->len) { + /* + * The read operation has returned end-of-file. Telling + * that to the main thread will cause it to set its + * 'defunct' flag and dispose of the handle structure at + * the next opportunity, so we must not touch ctx at all + * after this. + */ break; + } WaitForSingleObject(ctx->ev_from_main, INFINITE); if (ctx->done) { + /* + * The main thread has asked us to shut down. Send back an + * event indicating that we've done so. Hereafter we must + * not touch ctx at all, because the main thread might + * have freed it. + */ SetEvent(ctx->ev_to_main); - break; /* main thread told us to shut down */ + break; } } @@ -280,6 +294,12 @@ static DWORD WINAPI handle_output_threadfunc(void *param) while (1) { WaitForSingleObject(ctx->ev_from_main, INFINITE); if (ctx->done) { + /* + * The main thread has asked us to shut down. Send back an + * event indicating that we've done so. Hereafter we must + * not touch ctx at all, because the main thread might + * have freed it. + */ SetEvent(ctx->ev_to_main); break; } @@ -304,8 +324,16 @@ static DWORD WINAPI handle_output_threadfunc(void *param) } SetEvent(ctx->ev_to_main); - if (!writeret) + if (!writeret) { + /* + * The write operation has suffered an error. Telling that + * to the main thread will cause it to set its 'defunct' + * flag and dispose of the handle structure at the next + * opportunity, so we must not touch ctx at all after + * this. + */ break; + } } if (povl) @@ -601,10 +629,12 @@ void handle_got_event(HANDLE event) if (h->u.g.moribund) { /* - * A moribund handle is already treated as dead from the - * external user's point of view, so do nothing with the - * actual event. Just signal the thread to die if - * necessary, or destroy the handle if not. + * A moribund handle is one which we have either already + * signalled to die, or are waiting until its current I/O op + * completes to do so. Either way, it's treated as already + * dead from the external user's point of view, so we ignore + * the actual I/O result. We just signal the thread to die if + * we haven't yet done so, or destroy the handle if not. */ if (h->u.g.done) { handle_destroy(h); From 4a7632af7f12bae1664c580f67e4aed18d084ba8 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 28 Feb 2015 07:58:29 +0000 Subject: [PATCH 02/33] New 'contrib' tool: a script for faking initial KEX. encodelib.py is a Python library which implements some handy SSH-2 encoding primitives; samplekex.py uses that to fabricate the start of an SSH connection, up to the point where key exchange totally fails its crypto. The idea is that you adapt samplekex.py to construct initial-kex sequences with particular properties, in order to test robustness and security fixes that affect the initial-kex sequence. For example, I used an adaptation of this to test the Diffie-Hellman range check that's just gone into 0.64. (cherry picked from commit 12d5b00d62240d1875be4ac0a6c5d29240696c89) --- .gitignore | 1 + contrib/encodelib.py | 115 +++++++++++++++++++++++++++++++++++++++++++ contrib/samplekex.py | 66 +++++++++++++++++++++++++ 3 files changed, 182 insertions(+) create mode 100644 contrib/encodelib.py create mode 100755 contrib/samplekex.py diff --git a/.gitignore b/.gitignore index 92699284..bea01d86 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,5 @@ *.o +*.pyc .dirstamp .deps /*.pdb diff --git a/contrib/encodelib.py b/contrib/encodelib.py new file mode 100644 index 00000000..196c4677 --- /dev/null +++ b/contrib/encodelib.py @@ -0,0 +1,115 @@ +# Python module to make it easy to manually encode SSH packets, by +# supporting the various uint32, string, mpint primitives. +# +# The idea of this is that you can use it to manually construct key +# exchange sequences of interesting kinds, for testing purposes. + +import struct, random + +def boolean(b): + return "\1" if b else "\0" + +def byte(b): + assert 0 <= b < 0x100 + return chr(b) + +def uint32(u): + assert 0 <= u < 0x100000000 + return struct.pack(">I", u) + +def uint64(u): + assert 0 <= u < 0x10000000000000000 + return struct.pack(">L", u) + +def string(s): + return uint32(len(s)) + s + +def mpint(m): + s = "" + lastbyte = 0 + while m > 0: + lastbyte = m & 0xFF + s = chr(lastbyte) + s + m >>= 8 + if lastbyte & 0x80: + s = "\0" + s + return string(s) + +def name_list(ns): + s = "" + for n in ns: + assert "," not in n + if s != "": + s += "," + s += n + return string(s) + +def ssh_rsa_key_blob(modulus, exponent): + return string(string("ssh-rsa") + mpint(modulus) + mpint(exponent)) + +def ssh_rsa_signature_blob(signature): + return string(string("ssh-rsa") + mpint(signature)) + +def greeting(string): + # Greeting at the start of an SSH connection. + return string + "\r\n" + +# Packet types. +SSH2_MSG_DISCONNECT = 1 +SSH2_MSG_IGNORE = 2 +SSH2_MSG_UNIMPLEMENTED = 3 +SSH2_MSG_DEBUG = 4 +SSH2_MSG_SERVICE_REQUEST = 5 +SSH2_MSG_SERVICE_ACCEPT = 6 +SSH2_MSG_KEXINIT = 20 +SSH2_MSG_NEWKEYS = 21 +SSH2_MSG_KEXDH_INIT = 30 +SSH2_MSG_KEXDH_REPLY = 31 +SSH2_MSG_KEX_DH_GEX_REQUEST = 30 +SSH2_MSG_KEX_DH_GEX_GROUP = 31 +SSH2_MSG_KEX_DH_GEX_INIT = 32 +SSH2_MSG_KEX_DH_GEX_REPLY = 33 +SSH2_MSG_KEXRSA_PUBKEY = 30 +SSH2_MSG_KEXRSA_SECRET = 31 +SSH2_MSG_KEXRSA_DONE = 32 +SSH2_MSG_USERAUTH_REQUEST = 50 +SSH2_MSG_USERAUTH_FAILURE = 51 +SSH2_MSG_USERAUTH_SUCCESS = 52 +SSH2_MSG_USERAUTH_BANNER = 53 +SSH2_MSG_USERAUTH_PK_OK = 60 +SSH2_MSG_USERAUTH_PASSWD_CHANGEREQ = 60 +SSH2_MSG_USERAUTH_INFO_REQUEST = 60 +SSH2_MSG_USERAUTH_INFO_RESPONSE = 61 +SSH2_MSG_GLOBAL_REQUEST = 80 +SSH2_MSG_REQUEST_SUCCESS = 81 +SSH2_MSG_REQUEST_FAILURE = 82 +SSH2_MSG_CHANNEL_OPEN = 90 +SSH2_MSG_CHANNEL_OPEN_CONFIRMATION = 91 +SSH2_MSG_CHANNEL_OPEN_FAILURE = 92 +SSH2_MSG_CHANNEL_WINDOW_ADJUST = 93 +SSH2_MSG_CHANNEL_DATA = 94 +SSH2_MSG_CHANNEL_EXTENDED_DATA = 95 +SSH2_MSG_CHANNEL_EOF = 96 +SSH2_MSG_CHANNEL_CLOSE = 97 +SSH2_MSG_CHANNEL_REQUEST = 98 +SSH2_MSG_CHANNEL_SUCCESS = 99 +SSH2_MSG_CHANNEL_FAILURE = 100 +SSH2_MSG_USERAUTH_GSSAPI_RESPONSE = 60 +SSH2_MSG_USERAUTH_GSSAPI_TOKEN = 61 +SSH2_MSG_USERAUTH_GSSAPI_EXCHANGE_COMPLETE = 63 +SSH2_MSG_USERAUTH_GSSAPI_ERROR = 64 +SSH2_MSG_USERAUTH_GSSAPI_ERRTOK = 65 +SSH2_MSG_USERAUTH_GSSAPI_MIC = 66 + +def clearpkt(msgtype, *stuff): + # SSH-2 binary packet, in the cleartext format used for initial + # setup and kex. + s = byte(msgtype) + for thing in stuff: + s += thing + padlen = 0 + while padlen < 4 or len(s) % 8 != 3: + padlen += 1 + s += byte(random.randint(0,255)) + s = byte(padlen) + s + return string(s) diff --git a/contrib/samplekex.py b/contrib/samplekex.py new file mode 100755 index 00000000..47059837 --- /dev/null +++ b/contrib/samplekex.py @@ -0,0 +1,66 @@ +#!/usr/bin/env python + +# Example Python script to synthesise the server end of an SSH key exchange. + +# This is an output-only script; you run it by means of saying +# something like +# +# samplekex.py | nc -l 2222 | hex dump utility of your choice +# +# and then connecting PuTTY to port 2222. Being output-only, of +# course, it cannot possibly get the key exchange _right_, so PuTTY +# will terminate with an error when the signature in the final message +# doesn't validate. But everything until then should be processed as +# if it was a normal SSH-2 connection, which means you can use this +# script as a starting point for constructing interestingly malformed +# key exchanges to test bug fixes. + +import sys, random +from encodelib import * + +# A random Diffie-Hellman group, taken from an SSH server I made a +# test connection to. +groupgen = 5 +group = 0xf5d3849d2092fd427b4ebd838ea4830397a55f80b644626320dbbe51e8f63ed88148d787c94e7e67e4f393f26c565e1992b0cff8a47a953439462a4d0ffa5763ef60ff908f8ee6c4f6ef9f32b9ba50f01ad56fe7ebe90876a5cf61813a4ad4ba7ec0704303c9bf887d36abbd6c2aa9545fc2263232927e731060f5c701c96dc34016636df438ce30973715f121d767cfb98b5d09ae7b86fa36a051ad3c2941a295a68e2f583a56bc69913ec9d25abef4fdf1e31ede827a02620db058b9f041da051c8c0f13b132c17ceb893fa7c4cd8d8feebd82c5f9120cb221b8e88c5fe4dc17ca020a535484c92c7d4bee69c7703e1fa9a652d444c80065342c6ec0fac23c24de246e3dee72ca8bc8beccdade2b36771efcc350558268f5352ae53f2f71db62249ad9ac4fabdd6dfb099c6cff8c05bdea894390f9860f011cca046dfeb2f6ef81094e7980be526742706d1f3db920db107409291bb4c11f9a7dcbfaf26d808e6f9fe636b26b939de419129e86b1e632c60ec23b65c815723c5d861af068fd0ac8b37f4c06ecbd5cb2ef069ca8daac5cbd67c6182a65fed656d0dfbbb8a430b1dbac7bd6303bec8de078fe69f443a7bc8131a284d25dc2844f096240bfc61b62e91a87802987659b884c094c68741d29aa5ca19b9457e1f9df61c7dbbb13a61a79e4670b086027f20da2af4f5b020725f8828726379f429178926a1f0ea03f + +# An RSA key, generated specially for this script. +rsaexp = 0x10001 +rsamod = 0xB98FE0C0BEE1E05B35FDDF5517B3E29D8A9A6A7834378B6783A19536968968F755E341B5822CAE15B465DECB80EE4116CF8D22DB5A6C85444A68D0D45D9D42008329619BE3CAC3B192EF83DD2B75C4BB6B567E11B841073BACE92108DA7E97E543ED7F032F454F7AC3C6D3F27DB34BC9974A85C7963C546662AE300A61CBABEE274481FD041C41D0145704F5FA9C77A5A442CD7A64827BB0F21FB56FDE388B596A20D7A7D1C5F22DA96C6C2171D90A673DABC66596CD99499D75AD82FEFDBE04DEC2CC7E1414A95388F668591B3F4D58249F80489646ED2C318E77D4F4E37EE8A588E79F2960620E6D28BF53653F1C974C91845F0BABFE5D167E1CA7044EE20D + +# 16 bytes of random data for the start of KEXINIT. +cookie = "".join([chr(random.randint(0,255)) for i in range(16)]) + +sys.stdout.write(greeting("SSH-2.0-Example KEX synthesis")) + +# Expect client to send KEXINIT + +sys.stdout.write( + clearpkt(SSH2_MSG_KEXINIT, + cookie, + name_list(("diffie-hellman-group-exchange-sha256",)), # kex + name_list(("ssh-rsa",)), # host keys + name_list(("aes128-ctr",)), # client->server ciphers + name_list(("aes128-ctr",)), # server->client ciphers + name_list(("hmac-sha2-256",)), # client->server MACs + name_list(("hmac-sha2-256",)), # server->client MACs + name_list(("none",)), # client->server compression + name_list(("none",)), # server->client compression + name_list(()), # client->server languages + name_list(()), # server->client languages + boolean(False), # first kex packet does not follow + uint32(0))) + +# Expect client to send SSH2_MSG_KEX_DH_GEX_REQUEST(0x1000) + +sys.stdout.write( + clearpkt(SSH2_MSG_KEX_DH_GEX_GROUP, + mpint(group), + mpint(groupgen))) + +# Expect client to send SSH2_MSG_KEX_DH_GEX_INIT + +sys.stdout.write( + clearpkt(SSH2_MSG_KEX_DH_GEX_REPLY, + ssh_rsa_key_blob(rsaexp, rsamod), + mpint(random.randint(2, group-2)), + ssh_rsa_signature_blob(random.randint(2, rsamod-2)))) From 9799c563380de554946692d0a017407c33f1f603 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 28 Feb 2015 12:04:54 +0000 Subject: [PATCH 03/33] Reorganise the release checklist. Mostly I'm rearranging things because of the new workflows that git makes available - it's now possible (and indeed sensible) to prepare a lot of stuff in a fairly relaxed manner in local checkouts, and then the process of going live with the release has a lot less manual writing of stuff and a lot more mechanical 'git push' and running of update scripts. However, there's one new item that was actually missed off the previous checklist: turning off nightly pre-release builds after making the release they were a pre-release of. Ahem. (cherry picked from commit 8af53d1b692e6cb3aea05789d1b925fbc397453c) --- CHECKLST.txt | 197 +++++++++++++++++++++++++++------------------------ 1 file changed, 103 insertions(+), 94 deletions(-) diff --git a/CHECKLST.txt b/CHECKLST.txt index dc1db5e0..b9279994 100644 --- a/CHECKLST.txt +++ b/CHECKLST.txt @@ -39,8 +39,15 @@ The website: - putty-website/licence.html -Before tagging a release ------------------------- +Preparing to make a release +--------------------------- + +Now that PuTTY is in git, a lot of the release preparation can be done +in advance, in local checkouts, and not pushed until the actual +process of _releasing_ it. + +To begin with, before dropping the tag, make sure everything is ready +for it: - First of all, go through the source (including the documentation), and the website, and review anything tagged with a comment @@ -52,83 +59,93 @@ Before tagging a release particular, any headline features for the release should get a workout with memory checking enabled! -For a long time we got away with never checking the current version -number in at all - all version numbers were passed into the build -system on the compiler command line, and the _only_ place version -numbers showed up in the source files was in the tag information. - -Unfortunately, those halcyon days are gone, and we do need the -version number checked in in a couple of places. These must be updated -_before_ tagging a new release. - -The file used to generate the Unix snapshot version numbers (which -are - so that the Debian versioning system -orders them correctly with respect to releases): - - - putty/LATEST.VER - -The Windows installer script (_four_ times, on consecutive lines): - - - putty/windows/putty.iss - -It might also be worth going through the documentation looking for -version numbers - we have a couple of transcripts showing the help -text from the command-line tools, and it would be nice to ensure the -whole transcripts (certainly including the version numbers) are up -to date. Sometimes these are marked in between releases as `0.XX', so -it's worth grepping for that too. - - - putty/doc/pscp.but - - putty/doc/plink.but - - putty/doc/psftp.but (in case it ever acquires a similar thing) - -Finally, reset the epoch used for the $(Days) value computed in -Buildscr for the Windows binary version resource. It's probably not a -good idea to set it to _today_ (since it might clash with the -zero-valued field used in actual releases), so perhaps we should start -it 1000 days before the release date so as to have a largish number -recognisable as being the right kind of thing by its order of -magnitude. So, do this: - - perl -e 'printf "%d\n", time/86400 - 1000' - -and then substitute the resulting value into the definition of 'Epoch' -in Buildscr. - -The actual release procedure ----------------------------- - -This is the procedure I (SGT) currently follow (or _should_ follow -:-) when actually making a release, once I'm happy with the position -of the tag. - - Double-check that we have removed anything tagged with a comment containing the words XXX-REMOVE-BEFORE-RELEASE or - XXX-REVIEW-BEFORE-RELEASE. + XXX-REVIEW-BEFORE-RELEASE. ('git grep XXX-RE' should only show up + hits in this file itself.) + + - Now update version numbers in + * putty/LATEST.VER + * putty/windows/putty.iss (four times, on consecutive lines) + * putty/doc/pscp.but (and make sure the rest of the transcript is + up to date) + * putty/doc/plink.but (likewise) + + - Reset the epoch used for the $(Days) value computed in Buildscr for + the Windows binary version resource. It's probably not a good idea + to set it to _today_ (since it might clash with the zero-valued + field used in actual releases), so perhaps we should start it 1000 + days before the release date so as to have a largish number + recognisable as being the right kind of thing by its order of + magnitude. So, do this: + + perl -e 'printf "%d\n", time/86400 - 1000' + + and then substitute the resulting value into the definition of + 'Epoch' in Buildscr. + + - Commit those version number and epoch changes (on the appropriate + branch, of course!), and then make the release tag pointing at the + resulting commit. + + - If the release is on a branch (which I expect it generally will + be), merge that branch to master. - Write a release announcement (basically a summary of the changes since the last release). Squirrel it away in atreus:src/putty/local/announce- in case it's needed again within days of the release going out. - - Build the release: `bob -b 0.XX putty RELEASE=0.XX'. This should - generate a basically valid release directory as `build.out/putty', - and provide link maps and sign.sh alongside that in build.out. + - Update the web site, in a local checkout. + + Adjust front page to say 'The latest version is '. + + Adjust front page to add a news item. + + Adjust Download page to say 'The latest release version ()'. + + Adjust Download page to update filenames of installer and Unix + tarball (both in the hrefs themselves and the link text). + + Check over the Download page and remove any mention of + pre-releases, if there were any before this release. Comment out + the big pre-release section at the top, and also adjust the + sections about source archives at the bottom. + + Adjust header text on Changelog page. (That includes changing + `are new' in previous version to `were new'!) + + .htaccess has an individual redirect for each version number. Add + a new one. + + - If there are any last-minute wishlist entries (e.g. security + vulnerabilities fixed in the new release), write entries for them + in a local checkout of putty-wishlist. + + - Update the wishlist mechanism for the new release. This can be done + without touching individual items by editing the @releases array in + control/bugs2html. + + - Build the release, by checking out the release tag: + git checkout 0.XX + bob -F . RELEASE=0.XX' + This should generate a basically valid release directory as + `build.out/putty', and provide link maps and sign.sh alongside that + in build.out. - Do a bit of checking that the release binaries basically work, report their version numbers accurately, and so on. Test the installer and the Unix source tarball. + - Sign the release: in the `build.out' directory, type + sh sign.sh putty Releases + and enter the passphrases a lot of times. + +The actual release procedure +---------------------------- + +Once all the above preparation is done and the release has been built +locally, this is the procedure for putting it up on the web. + - Save the link maps. Currently I keep these on atreus, in - src/putty/local/maps-. + src/putty-local/maps-. - - Sign the release: in the `build.out' directory, type `./sign.sh - putty Releases', and enter the passphrases a lot of times. + - Upload the entire release directory to atreus:www/putty/. - - Now the whole release directory should be present and correct. - Upload it to atreus:www/putty/. - - - Do final checks on the release directory: + - Do final checks on the release directory in its new location: + verify all the signatures: for i in `find . -name '*.*SA'`; do case $i in *sums*) gpg --verify $i;; *) gpg --verify $i ${i%%.?SA};; esac; done + check the checksum files: @@ -143,31 +160,12 @@ of the tag. - Check the permissions! Actually try downloading from the, to make sure it really works. - - Update the HTTP redirects. - + Update the one at the:www/putty/htaccess which points the - virtual subdir `latest' at the actual latest release dir. TEST - THIS ONE - it's quite important. - + atreus:www/putty/.htaccess has an individual redirect for each - version number. Add a new one. + - Update the HTTP redirect at the:www/putty/htaccess which points the + virtual subdir `latest' at the actual latest release dir. TEST THIS + ONE - it's quite important. - Update the FTP symlink (chiark:ftp/putty-latest -> putty-). - - Update web site. - + Adjust front page to say 'The latest version is '. - + Adjust front page to add a news item. - + Adjust Download page to say 'The latest release version ()'. - + Adjust Download page to update filenames of installer and Unix - tarball (both in the hrefs themselves and the link text). - + Check over the Download page and remove any mention of - pre-releases, if there were any before this release. Comment out - the big pre-release section at the top, and also adjust the - sections about source archives at the bottom. - + Adjust header text on Changelog page. (That includes changing - `are new' in previous version to `were new'!) - - - Update the wishlist. This can be done without touching individual - items by editing the @releases array in control/bugs2html. - - Check the Docs page links correctly to the release docs. (It should do this automatically, owing to the `latest' HTTP redirect.) @@ -175,6 +173,23 @@ of the tag. - Check that the web server attaches the right content type to .HLP and .CNT files. + - Run 'git push' in the website checkout, and then 'git pull' in + ~/www/putty on atreus to fetch the website updates. + + - Push the changes to PuTTY itself. Something like: + git push origin master # update the master branch + git push origin --tags # should push the new release tag + git push origin :pre-0.XX # delete the pre-release branch + + - Run 'git push' in the putty-wishlist checkout. Then run 'git pull' + in ~/pub/putty-wishlist on atreus, and update the wishlist web + pages with the commands + cd ~/pub/putty-wishlist/control + perl bugs2html + + - Check over the web site to make sure all the changes to wishlist + and main web pages are present and correct. + - Run webupdate, so that all the changes on atreus propagate to chiark. Important to do this _before_ announcing that the release is available. @@ -194,13 +209,7 @@ of the tag. + Post it to comp.security.ssh. + Mention it in on mono. + - Edit ~/adm/puttysnap.sh to disable pre-release builds, if they were + previously enabled. + - Relax (slightly). - -After the release ------------------ - -The following want doing some time soon after a release has been made: - - - If the release was made from a branch, make sure the version number - on the _trunk_ is up to date in all the locations listed above, so - that (e.g.) Unix snapshots come out right. From 7fd8b8bb1606401051c6b4866b2b3cb7edadbc65 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 28 Feb 2015 13:10:55 +0000 Subject: [PATCH 04/33] Typo. (cherry picked from commit ac27a1468962895d64ebf6d45a74a03b2afa4050) --- CHECKLST.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHECKLST.txt b/CHECKLST.txt index b9279994..18c2d18f 100644 --- a/CHECKLST.txt +++ b/CHECKLST.txt @@ -121,7 +121,7 @@ for it: - Build the release, by checking out the release tag: git checkout 0.XX - bob -F . RELEASE=0.XX' + bob -F . RELEASE=0.XX This should generate a basically valid release directory as `build.out/putty', and provide link maps and sign.sh alongside that in build.out. From 26de94e7dbceeeab8643c80cfda5a0958a22d818 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 28 Feb 2015 15:47:45 +0000 Subject: [PATCH 05/33] Add a new checklist item. I managed to build from completely the wrong commit this morning, so make sure to double-check next time! (cherry picked from commit 45e89ed7ca42628d0fc85cd4f7fb3efebcd38614) --- CHECKLST.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHECKLST.txt b/CHECKLST.txt index 18c2d18f..da3f2542 100644 --- a/CHECKLST.txt +++ b/CHECKLST.txt @@ -126,6 +126,9 @@ for it: `build.out/putty', and provide link maps and sign.sh alongside that in build.out. + - Double-check in build.log that the release was built from the right + git commit. + - Do a bit of checking that the release binaries basically work, report their version numbers accurately, and so on. Test the installer and the Unix source tarball. From 74f50c9f21b27b789b3e9a0d11eb5ea8d8719e19 Mon Sep 17 00:00:00 2001 From: Jacob Nevins Date: Sun, 1 Mar 2015 12:27:27 +0000 Subject: [PATCH 06/33] Move kh2reg.py link from svn to git. (cherry picked from commit 06d2fb5b372ff076d5e339f5baa3d919cb48870f) --- doc/faq.but | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/faq.but b/doc/faq.but index 06448cb8..72d03b92 100644 --- a/doc/faq.but +++ b/doc/faq.but @@ -163,7 +163,7 @@ the wrong solution and we will not do it. If you have host keys available in the common \i\c{known_hosts} format, we have a script called -\W{http://svn.tartarus.org/sgt/putty/contrib/kh2reg.py?view=markup}\c{kh2reg.py} +\W{http://tartarus.org/~simon-git/gitweb/?p=putty.git;a=blob;f=contrib/kh2reg.py;hb=HEAD}\c{kh2reg.py} to convert them to a Windows .REG file, which can be installed ahead of time by double-clicking or using \c{REGEDIT}. From 0db409bc07e123f62b43b1e77a516d82a6cc1cd6 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 7 Mar 2015 17:10:36 +0000 Subject: [PATCH 07/33] Stop Windows PuTTY becoming unresponsive if server floods us. This was an old bug, fixed around 0.59, which apparently regressed when I rewrote the main event loop using the toplevel_callback mechanism. Investigation just now suggests that it has to do with my faulty assumption that Windows PeekMessage would deliver messages in its message queue in FIFO order (i.e. that the thing calling itself a message queue is actually a _queue_). In fact my WM_NETEVENT seems to like to jump the queue, so that once a steady stream of them starts arriving, we never do anything else in the main event loop (except deal with handles). Worked around in a simple and slightly bodgy way, namely, we don't stop looping on PeekMessage and run our toplevel callbacks until we've either run out of messages completely or else seen at least one that _isn't_ a WM_NETEVENT. That way we should reliably interleave NETEVENT processing with processing of other stuff. (cherry picked from commit 7d97c2a8fdb745905fd61a9ce4abbf822e167cef) --- windows/window.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/windows/window.c b/windows/window.c index 7c320a0a..ebcecac7 100644 --- a/windows/window.c +++ b/windows/window.c @@ -888,12 +888,34 @@ int WINAPI WinMain(HINSTANCE inst, HINSTANCE prev, LPSTR cmdline, int show) } else sfree(handles); - if (PeekMessage(&msg, NULL, 0, 0, PM_REMOVE)) { + while (PeekMessage(&msg, NULL, 0, 0, PM_REMOVE)) { if (msg.message == WM_QUIT) goto finished; /* two-level break */ if (!(IsWindow(logbox) && IsDialogMessage(logbox, &msg))) DispatchMessage(&msg); + + /* + * WM_NETEVENT messages seem to jump ahead of others in + * the message queue. I'm not sure why; the docs for + * PeekMessage mention that messages are prioritised in + * some way, but I'm unclear on which priorities go where. + * + * Anyway, in practice I observe that WM_NETEVENT seems to + * jump to the head of the queue, which means that if we + * were to only process one message every time round this + * loop, we'd get nothing but NETEVENTs if the server + * flooded us with data, and stop responding to any other + * kind of window message. So instead, we keep on round + * this loop until we've consumed at least one message + * that _isn't_ a NETEVENT, or run out of messages + * completely (whichever comes first). And we don't go to + * run_toplevel_callbacks (which is where the netevents + * are actually processed, causing fresh NETEVENT messages + * to appear) until we've done this. + */ + if (msg.message != WM_NETEVENT) + break; } run_toplevel_callbacks(); From b58a34115ac47f264c7cb684d3b66228cadce40e Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 7 Mar 2015 20:57:26 +0000 Subject: [PATCH 08/33] Don't output negative numbers in the ESC[13t report. A minus sign is illegal at that position in a control sequence, so if ESC[13t should report something like ESC[3;-123;234t then we won't accept it as input. Switch to printing the numbers as unsigned, so that negative window coordinates are output as their 32-bit two's complement; experimentation suggests that PuTTY does accept that on input. (cherry picked from commit 2422b18a0f4d758f0660503b068dd19d92de4906) --- terminal.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/terminal.c b/terminal.c index ac8f5dff..d8d0ea0c 100644 --- a/terminal.c +++ b/terminal.c @@ -3995,7 +3995,9 @@ static void term_out(Terminal *term) case 13: if (term->ldisc) { get_window_pos(term->frontend, &x, &y); - len = sprintf(buf, "\033[3;%d;%dt", x, y); + len = sprintf(buf, "\033[3;%u;%ut", + (unsigned)x, + (unsigned)y); ldisc_send(term->ldisc, buf, len, 0); } break; From 02893bcba062ad3a39c41a6a98d4647f417d2b13 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 7 Apr 2015 21:54:41 +0100 Subject: [PATCH 09/33] Clean up a stale foreign handle in winnps.c. I had set up an event object for signalling incoming connections to the named pipe, and then called handle_add_foreign_event to get that event object watched for connections - but when I closed down the listening pipe, I deleted the event object without also cancelling that foreign-event handle, so that winhandl.c would potentially call the callback for a destroyed object. (cherry picked from commit 6f241cef2c9770abf71349dd59547b3e5b4c0301) --- windows/winnps.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/windows/winnps.c b/windows/winnps.c index a172628f..7b4aa0db 100644 --- a/windows/winnps.c +++ b/windows/winnps.c @@ -32,6 +32,7 @@ struct Socket_named_pipe_server_tag { /* The current named pipe object + attempt to connect to it */ HANDLE pipehandle; OVERLAPPED connect_ovl; + struct handle *callback_handle; /* winhandl.c's reference */ /* PuTTY Socket machinery */ Plug plug; @@ -51,6 +52,8 @@ static void sk_namedpipeserver_close(Socket s) { Named_Pipe_Server_Socket ps = (Named_Pipe_Server_Socket) s; + if (ps->callback_handle) + handle_free(ps->callback_handle); CloseHandle(ps->pipehandle); CloseHandle(ps->connect_ovl.hEvent); sfree(ps->error); @@ -220,6 +223,7 @@ Socket new_named_pipe_listener(const char *pipename, Plug plug) ret->psd = NULL; ret->pipename = dupstr(pipename); ret->acl = NULL; + ret->callback_handle = NULL; assert(strncmp(pipename, "\\\\.\\pipe\\", 9) == 0); assert(strchr(pipename + 9, '\\') == NULL); @@ -237,8 +241,9 @@ Socket new_named_pipe_listener(const char *pipename, Plug plug) memset(&ret->connect_ovl, 0, sizeof(ret->connect_ovl)); ret->connect_ovl.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL); - handle_add_foreign_event(ret->connect_ovl.hEvent, - named_pipe_connect_callback, ret); + ret->callback_handle = + handle_add_foreign_event(ret->connect_ovl.hEvent, + named_pipe_connect_callback, ret); named_pipe_accept_loop(ret, FALSE); cleanup: From 2856422eab2169928a44ebbf00e8b8d8ad7e5916 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 7 Apr 2015 22:17:08 +0100 Subject: [PATCH 10/33] Fix a dangerous cross-thread memory access. When a winhandl.c input thread returns EOF to the main thread, the latter might immediately delete the input thread's context. I carefully wrote in a comment that in that case we had to not touch ctx ever again after signalling to the main thread - but the test for whether that was true, which also touched ctx, itself came _after_ the SetEvent which sent that signal. Ahem. Spotted by Minefield, which it looks as if I haven't run for a while. (cherry picked from commit 9fec2e773873e28f1409f5e1eefaf03483070050) --- windows/winhandl.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/windows/winhandl.c b/windows/winhandl.c index d81054f3..7c282fc7 100644 --- a/windows/winhandl.c +++ b/windows/winhandl.c @@ -115,7 +115,7 @@ static DWORD WINAPI handle_input_threadfunc(void *param) struct handle_input *ctx = (struct handle_input *) param; OVERLAPPED ovl, *povl; HANDLE oev; - int readret, readlen; + int readret, readlen, finished; if (ctx->flags & HANDLE_FLAG_OVERLAPPED) { povl = &ovl; @@ -165,18 +165,20 @@ static DWORD WINAPI handle_input_threadfunc(void *param) (ctx->flags & HANDLE_FLAG_IGNOREEOF)) continue; + /* + * If we just set ctx->len to 0, that means the read operation + * has returned end-of-file. Telling that to the main thread + * will cause it to set its 'defunct' flag and dispose of the + * handle structure at the next opportunity, in which case we + * mustn't touch ctx at all after the SetEvent. (Hence we do + * even _this_ check before the SetEvent.) + */ + finished = (ctx->len == 0); + SetEvent(ctx->ev_to_main); - if (!ctx->len) { - /* - * The read operation has returned end-of-file. Telling - * that to the main thread will cause it to set its - * 'defunct' flag and dispose of the handle structure at - * the next opportunity, so we must not touch ctx at all - * after this. - */ + if (finished) break; - } WaitForSingleObject(ctx->ev_from_main, INFINITE); if (ctx->done) { From 5ac299449e188e120fc06a6fd59d5f97840245b0 Mon Sep 17 00:00:00 2001 From: Jacob Nevins Date: Thu, 23 Apr 2015 23:42:45 +0100 Subject: [PATCH 11/33] Old Dropbear servers have the ssh-close-vs-request bug. Add automatic bug detection. (Versions verified by Matt Johnston.) (cherry picked from commit 63dddfc00f4ca44f8cc0a372b419e0ff45008ea2) --- ssh.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/ssh.c b/ssh.c index 6009e06f..13b67b6b 100644 --- a/ssh.c +++ b/ssh.c @@ -2817,11 +2817,15 @@ static void ssh_detect_bugs(Ssh ssh, char *vstring) if (conf_get_int(ssh->conf, CONF_sshbug_chanreq) == FORCE_ON || (conf_get_int(ssh->conf, CONF_sshbug_chanreq) == AUTO && (wc_match("OpenSSH_[2-5].*", imp) || - wc_match("OpenSSH_6.[0-6]*", imp)))) { + wc_match("OpenSSH_6.[0-6]*", imp) || + wc_match("dropbear_0.[2-4][0-9]*", imp) || + wc_match("dropbear_0.5[01]*", imp)))) { /* - * These versions have the SSH-2 channel request bug. 6.7 and - * above do not: + * These versions have the SSH-2 channel request bug. + * OpenSSH 6.7 and above do not: * https://bugzilla.mindrot.org/show_bug.cgi?id=1818 + * dropbear_0.52 and above do not: + * https://secure.ucc.asn.au/hg/dropbear/rev/cd02449b709c */ ssh->remote_bugs |= BUG_SENDS_LATE_REQUEST_REPLY; logevent("We believe remote version has SSH-2 channel request bug"); From 318076a1838c0e270afc5ac5a8081b3a1273cfb4 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 25 Apr 2015 10:46:53 +0100 Subject: [PATCH 12/33] Support RFC 4419. PuTTY now uses the updated version of Diffie-Hellman group exchange, except for a few old OpenSSH versions which Darren Tucker reports only support the old version. FIXME: this needs further work because the Bugs config panel has now overflowed. (cherry picked from commit 62a1bce7cb3ecb98feb57c7f1fd5d55845ce1533) --- config.c | 3 +++ doc/config.but | 17 +++++++++++++++++ putty.h | 1 + settings.c | 2 ++ ssh.c | 35 +++++++++++++++++++++++++++++++++-- ssh.h | 3 ++- windows/winhelp.h | 1 + 7 files changed, 59 insertions(+), 3 deletions(-) diff --git a/config.c b/config.c index c2acd628..395f9c81 100644 --- a/config.c +++ b/config.c @@ -2603,6 +2603,9 @@ void setup_config_box(struct controlbox *b, int midsession, ctrl_droplist(s, "Ignores SSH-2 maximum packet size", 'x', 20, HELPCTX(ssh_bugs_maxpkt2), sshbug_handler, I(CONF_sshbug_maxpkt2)); + ctrl_droplist(s, "Only supports pre-RFC4419 SSH-2 DH GEX", 'd', 20, + HELPCTX(ssh_bugs_oldgex2), + sshbug_handler, I(CONF_sshbug_oldgex2)); ctrl_droplist(s, "Replies to requests on closed channels", 'q', 20, HELPCTX(ssh_bugs_chanreq), sshbug_handler, I(CONF_sshbug_chanreq)); diff --git a/doc/config.but b/doc/config.but index 0f8175f2..1d81460f 100644 --- a/doc/config.but +++ b/doc/config.but @@ -3386,6 +3386,23 @@ reply to a request after it thinks the channel has entirely closed, and terminate with an error along the lines of \q{Received \cw{SSH2_MSG_CHANNEL_FAILURE} for nonexistent channel 256}. +\S{config-ssh-bug-oldgex2} \q{Only supports pre-RFC4419 SSH-2 DH GEX} + +\cfg{winhelp-topic}{ssh.bugs.oldgex2} + +The SSH key exchange method that uses Diffie-Hellman group exchange +was redesigned after its original release, to use a slightly more +sophisticated setup message. Almost all SSH implementations switched +over to the new version. (PuTTY was one of the last.) A few old +servers still only support the old one. + +If this bug is detected, and the client and server negotiate +Diffie-Hellman group exchange, then PuTTY will send the old message +now known as \cw{SSH2_MSG_KEX_DH_GEX_REQUEST_OLD} in place of the new +\cw{SSH2_MSG_KEX_DH_GEX_REQUEST}. + +This is an SSH-2-specific bug. + \H{config-serial} The Serial panel The \i{Serial} panel allows you to configure options that only apply diff --git a/putty.h b/putty.h index 5e043960..e030df79 100644 --- a/putty.h +++ b/putty.h @@ -837,6 +837,7 @@ void cleanup_exit(int); X(INT, NONE, sshbug_rekey2) \ X(INT, NONE, sshbug_maxpkt2) \ X(INT, NONE, sshbug_ignore2) \ + X(INT, NONE, sshbug_oldgex2) \ X(INT, NONE, sshbug_winadj) \ X(INT, NONE, sshbug_chanreq) \ /* \ diff --git a/settings.c b/settings.c index a7a88785..02d7506b 100644 --- a/settings.c +++ b/settings.c @@ -630,6 +630,7 @@ void save_open_settings(void *sesskey, Conf *conf) write_setting_i(sesskey, "BugPKSessID2", 2-conf_get_int(conf, CONF_sshbug_pksessid2)); write_setting_i(sesskey, "BugRekey2", 2-conf_get_int(conf, CONF_sshbug_rekey2)); write_setting_i(sesskey, "BugMaxPkt2", 2-conf_get_int(conf, CONF_sshbug_maxpkt2)); + write_setting_i(sesskey, "BugOldGex2", 2-conf_get_int(conf, CONF_sshbug_oldgex2)); write_setting_i(sesskey, "BugWinadj", 2-conf_get_int(conf, CONF_sshbug_winadj)); write_setting_i(sesskey, "BugChanReq", 2-conf_get_int(conf, CONF_sshbug_chanreq)); write_setting_i(sesskey, "StampUtmp", conf_get_int(conf, CONF_stamp_utmp)); @@ -977,6 +978,7 @@ void load_open_settings(void *sesskey, Conf *conf) i = gppi_raw(sesskey, "BugPKSessID2", 0); conf_set_int(conf, CONF_sshbug_pksessid2, 2-i); i = gppi_raw(sesskey, "BugRekey2", 0); conf_set_int(conf, CONF_sshbug_rekey2, 2-i); i = gppi_raw(sesskey, "BugMaxPkt2", 0); conf_set_int(conf, CONF_sshbug_maxpkt2, 2-i); + i = gppi_raw(sesskey, "BugOldGex2", 0); conf_set_int(conf, CONF_sshbug_oldgex2, 2-i); i = gppi_raw(sesskey, "BugWinadj", 0); conf_set_int(conf, CONF_sshbug_winadj, 2-i); i = gppi_raw(sesskey, "BugChanReq", 0); conf_set_int(conf, CONF_sshbug_chanreq, 2-i); conf_set_int(conf, CONF_ssh_simple, FALSE); diff --git a/ssh.c b/ssh.c index 13b67b6b..735bc174 100644 --- a/ssh.c +++ b/ssh.c @@ -76,6 +76,10 @@ static const char *const ssh2_disconnect_reasons[] = { #define BUG_CHOKES_ON_SSH2_IGNORE 512 #define BUG_CHOKES_ON_WINADJ 1024 #define BUG_SENDS_LATE_REQUEST_REPLY 2048 +#define BUG_SSH2_OLDGEX 4096 + +#define DH_MIN_SIZE 1024 +#define DH_MAX_SIZE 8192 /* * Codes for terminal modes. @@ -247,6 +251,7 @@ static char *ssh2_pkt_type(Pkt_KCtx pkt_kctx, Pkt_ACtx pkt_actx, int type) translate(SSH2_MSG_NEWKEYS); translatek(SSH2_MSG_KEXDH_INIT, SSH2_PKTCTX_DHGROUP); translatek(SSH2_MSG_KEXDH_REPLY, SSH2_PKTCTX_DHGROUP); + translatek(SSH2_MSG_KEX_DH_GEX_REQUEST_OLD, SSH2_PKTCTX_DHGEX); translatek(SSH2_MSG_KEX_DH_GEX_REQUEST, SSH2_PKTCTX_DHGEX); translatek(SSH2_MSG_KEX_DH_GEX_GROUP, SSH2_PKTCTX_DHGEX); translatek(SSH2_MSG_KEX_DH_GEX_INIT, SSH2_PKTCTX_DHGEX); @@ -2805,6 +2810,17 @@ static void ssh_detect_bugs(Ssh ssh, char *vstring) logevent("We believe remote version has SSH-2 ignore bug"); } + if (conf_get_int(ssh->conf, CONF_sshbug_oldgex2) == FORCE_ON || + (conf_get_int(ssh->conf, CONF_sshbug_oldgex2) == AUTO && + (wc_match("OpenSSH_2.[235]*", imp)))) { + /* + * These versions only support the original (pre-RFC4419) + * SSH-2 GEX request. + */ + ssh->remote_bugs |= BUG_SSH2_OLDGEX; + logevent("We believe remote version has outdated SSH-2 GEX"); + } + if (conf_get_int(ssh->conf, CONF_sshbug_winadj) == FORCE_ON) { /* * Servers that don't support our winadj request for one @@ -6595,8 +6611,19 @@ static void do_ssh2_transport(Ssh ssh, void *vin, int inlen, * much data. */ s->pbits = 512 << ((s->nbits - 1) / 64); - s->pktout = ssh2_pkt_init(SSH2_MSG_KEX_DH_GEX_REQUEST); - ssh2_pkt_adduint32(s->pktout, s->pbits); + if (s->pbits < DH_MIN_SIZE) + s->pbits = DH_MIN_SIZE; + if (s->pbits > DH_MAX_SIZE) + s->pbits = DH_MAX_SIZE; + if ((ssh->remote_bugs & BUG_SSH2_OLDGEX)) { + s->pktout = ssh2_pkt_init(SSH2_MSG_KEX_DH_GEX_REQUEST_OLD); + ssh2_pkt_adduint32(s->pktout, s->pbits); + } else { + s->pktout = ssh2_pkt_init(SSH2_MSG_KEX_DH_GEX_REQUEST); + ssh2_pkt_adduint32(s->pktout, DH_MIN_SIZE); + ssh2_pkt_adduint32(s->pktout, s->pbits); + ssh2_pkt_adduint32(s->pktout, DH_MAX_SIZE); + } ssh2_pkt_send_noqueue(ssh, s->pktout); crWaitUntilV(pktin); @@ -6664,7 +6691,11 @@ static void do_ssh2_transport(Ssh ssh, void *vin, int inlen, hash_string(ssh->kex->hash, ssh->exhash, s->hostkeydata, s->hostkeylen); if (!ssh->kex->pdata) { + if (!(ssh->remote_bugs & BUG_SSH2_OLDGEX)) + hash_uint32(ssh->kex->hash, ssh->exhash, DH_MIN_SIZE); hash_uint32(ssh->kex->hash, ssh->exhash, s->pbits); + if (!(ssh->remote_bugs & BUG_SSH2_OLDGEX)) + hash_uint32(ssh->kex->hash, ssh->exhash, DH_MAX_SIZE); hash_mpint(ssh->kex->hash, ssh->exhash, s->p); hash_mpint(ssh->kex->hash, ssh->exhash, s->g); } diff --git a/ssh.h b/ssh.h index 52500ac8..e50fbca3 100644 --- a/ssh.h +++ b/ssh.h @@ -720,7 +720,8 @@ void platform_ssh_share_cleanup(const char *name); #define SSH2_MSG_NEWKEYS 21 /* 0x15 */ #define SSH2_MSG_KEXDH_INIT 30 /* 0x1e */ #define SSH2_MSG_KEXDH_REPLY 31 /* 0x1f */ -#define SSH2_MSG_KEX_DH_GEX_REQUEST 30 /* 0x1e */ +#define SSH2_MSG_KEX_DH_GEX_REQUEST_OLD 30 /* 0x1e */ +#define SSH2_MSG_KEX_DH_GEX_REQUEST 34 /* 0x1e */ #define SSH2_MSG_KEX_DH_GEX_GROUP 31 /* 0x1f */ #define SSH2_MSG_KEX_DH_GEX_INIT 32 /* 0x20 */ #define SSH2_MSG_KEX_DH_GEX_REPLY 33 /* 0x21 */ diff --git a/windows/winhelp.h b/windows/winhelp.h index fc10a8f4..9b21c92c 100644 --- a/windows/winhelp.h +++ b/windows/winhelp.h @@ -148,6 +148,7 @@ #define WINHELP_CTX_ssh_bugs_maxpkt2 "ssh.bugs.maxpkt2:config-ssh-bug-maxpkt2" #define WINHELP_CTX_ssh_bugs_winadj "ssh.bugs.winadj:config-ssh-bug-winadj" #define WINHELP_CTX_ssh_bugs_chanreq "ssh.bugs.winadj:config-ssh-bug-chanreq" +#define WINHELP_CTX_ssh_bugs_oldgex2 "ssh.bugs.oldgex2:config-ssh-bug-oldgex2" #define WINHELP_CTX_serial_line "serial.line:config-serial-line" #define WINHELP_CTX_serial_speed "serial.speed:config-serial-speed" #define WINHELP_CTX_serial_databits "serial.databits:config-serial-databits" From 51ee4eb144e59e5f7f2c6dbeb921a48cb4863e2e Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 25 Apr 2015 10:46:56 +0100 Subject: [PATCH 13/33] Divide the Bugs panel in half. It overflowed as a result of the previous commit. (cherry picked from commit 84e239dd88245cd3308de987b2b0fd6637b2db34) --- config.c | 8 +++++++- doc/config.but | 9 +++++---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/config.c b/config.c index 395f9c81..0ad55413 100644 --- a/config.c +++ b/config.c @@ -2563,7 +2563,7 @@ void setup_config_box(struct controlbox *b, int midsession, if (!midsession) { /* - * The Connection/SSH/Bugs panel. + * The Connection/SSH/Bugs panels. */ ctrl_settitle(b, "Connection/SSH/Bugs", "Workarounds for SSH server bugs"); @@ -2591,6 +2591,12 @@ void setup_config_box(struct controlbox *b, int midsession, ctrl_droplist(s, "Miscomputes SSH-2 encryption keys", 'e', 20, HELPCTX(ssh_bugs_derivekey2), sshbug_handler, I(CONF_sshbug_derivekey2)); + + ctrl_settitle(b, "Connection/SSH/More bugs", + "Further workarounds for SSH server bugs"); + + s = ctrl_getset(b, "Connection/SSH/More bugs", "main", + "Detection of known bugs in SSH servers"); ctrl_droplist(s, "Requires padding on SSH-2 RSA signatures", 'p', 20, HELPCTX(ssh_bugs_rsapad2), sshbug_handler, I(CONF_sshbug_rsapad2)); diff --git a/doc/config.but b/doc/config.but index 1d81460f..c23f4ddb 100644 --- a/doc/config.but +++ b/doc/config.but @@ -3128,7 +3128,7 @@ you do the same on Linux, you can also use it with IPv4. However, ticking \q{Auto} should always give you a port which you can connect to using either protocol. -\H{config-ssh-bugs} \I{SSH server bugs}The Bugs panel +\H{config-ssh-bugs} \I{SSH server bugs}The Bugs and More Bugs panels Not all SSH servers work properly. Various existing servers have bugs in them, which can make it impossible for a client to talk to @@ -3142,9 +3142,10 @@ has been deliberately configured to conceal its version number, or if the server is a version which PuTTY's bug database does not know about, then PuTTY will not know what bugs to expect. -The Bugs panel allows you to manually configure the bugs PuTTY -expects to see in the server. Each bug can be configured in three -states: +The Bugs and More Bugs panels (there are two because we have so many +bug compatibility modes) allow you to manually configure the bugs +PuTTY expects to see in the server. Each bug can be configured in +three states: \b \q{Off}: PuTTY will assume the server does not have the bug. From 9bcb6639cc324b0dd27eab844c098363579644fb Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 26 Apr 2015 10:49:24 +0100 Subject: [PATCH 14/33] Fix a few memory leaks. Patch due to Chris Staite. (cherry picked from commit 78989c97c94ef45b7081d80df1c35f2cc1edfea0) --- ssh.c | 2 ++ sshpubk.c | 9 ++++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/ssh.c b/ssh.c index 735bc174..368cabef 100644 --- a/ssh.c +++ b/ssh.c @@ -9458,6 +9458,8 @@ static void do_ssh2_authconn(Ssh ssh, unsigned char *in, int inlen, logevent("Sent public key signature"); s->type = AUTH_TYPE_PUBLICKEY; key->alg->freekey(key->data); + sfree(key->comment); + sfree(key); } #ifndef NO_GSSAPI diff --git a/sshpubk.c b/sshpubk.c index 63b54b12..a4ecb9d5 100644 --- a/sshpubk.c +++ b/sshpubk.c @@ -837,7 +837,7 @@ unsigned char *ssh2_userkey_loadpub(const Filename *filename, char **algorithm, int public_blob_len; int i; const char *error = NULL; - char *comment; + char *comment = NULL; public_blob = NULL; @@ -862,11 +862,10 @@ unsigned char *ssh2_userkey_loadpub(const Filename *filename, char **algorithm, goto error; /* Select key algorithm structure. */ alg = find_pubkey_alg(b); + sfree(b); if (!alg) { - sfree(b); goto error; } - sfree(b); /* Read the Encryption header line. */ if (!read_header(fp, header) || 0 != strcmp(header, "Encryption")) @@ -913,6 +912,10 @@ unsigned char *ssh2_userkey_loadpub(const Filename *filename, char **algorithm, sfree(public_blob); if (errorstr) *errorstr = error; + if (comment && commentptr) { + sfree(comment); + *commentptr = NULL; + } return NULL; } From 26d3ccdfc5500a8afdbd54938677ef4ef985caaa Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 26 Apr 2015 23:31:11 +0100 Subject: [PATCH 15/33] Use a timing-safe memory compare to verify MACs. Now that we have modes in which the MAC verification happens before any other crypto operation and hence will be the only thing seen by an attacker, it seems like about time we got round to doing it in a cautious way that tries to prevent the attacker from using our memcmp as a timing oracle. So, here's an smemeq() function which has the semantics of !memcmp but attempts to run in time dependent only on the length parameter. All the MAC implementations now use this in place of !memcmp to verify the MAC on input data. (cherry picked from commit 9d5a16402168f82ba1bd695c3e95bb4812ccd0a9) Cherry-picker's notes: the above commit comment isn't really true on this branch, since the ETM packet protocol changes haven't been cherry-picked. But it seemed silly to deliberately leave out even a small safety measure. --- misc.c | 16 ++++++++++++++++ misc.h | 12 ++++++++++++ sshmd5.c | 4 ++-- sshsh256.c | 4 ++-- sshsha.c | 8 ++++---- 5 files changed, 36 insertions(+), 8 deletions(-) diff --git a/misc.c b/misc.c index 63de9d11..94b5ac8a 100644 --- a/misc.c +++ b/misc.c @@ -1019,3 +1019,19 @@ int validate_manual_hostkey(char *key) return FALSE; } + +int smemeq(const void *av, const void *bv, size_t len) +{ + const unsigned char *a = (const unsigned char *)av; + const unsigned char *b = (const unsigned char *)bv; + unsigned val = 0; + + while (len-- > 0) { + val |= *a++ ^ *b++; + } + /* Now val is 0 iff we want to return 1, and in the range + * 0x01..0xFF iff we want to return 0. So subtracting from 0x100 + * will clear bit 8 iff we want to return 0, and leave it set iff + * we want to return 1, so then we can just shift down. */ + return (0x100 - val) >> 8; +} diff --git a/misc.h b/misc.h index a361d706..c38266d0 100644 --- a/misc.h +++ b/misc.h @@ -64,8 +64,20 @@ int validate_manual_hostkey(char *key); struct tm ltime(void); +/* Wipe sensitive data out of memory that's about to be freed. Simpler + * than memset because we don't need the fill char parameter; also + * attempts (by fiddly use of volatile) to inhibit the compiler from + * over-cleverly trying to optimise the memset away because it knows + * the variable is going out of scope. */ void smemclr(void *b, size_t len); +/* Compare two fixed-length chunks of memory for equality, without + * data-dependent control flow (so an attacker with a very accurate + * stopwatch can't try to guess where the first mismatching byte was). + * Returns 0 for mismatch or 1 for equality (unlike memcmp), hinted at + * by the 'eq' in the name. */ +int smemeq(const void *av, const void *bv, size_t len); + /* * Debugging functions. * diff --git a/sshmd5.c b/sshmd5.c index 2fdb5900..493db12c 100644 --- a/sshmd5.c +++ b/sshmd5.c @@ -287,7 +287,7 @@ static int hmacmd5_verresult(void *handle, unsigned char const *hmac) { unsigned char correct[16]; hmacmd5_genresult(handle, correct); - return !memcmp(correct, hmac, 16); + return smemeq(correct, hmac, 16); } static void hmacmd5_do_hmac_internal(void *handle, @@ -327,7 +327,7 @@ static int hmacmd5_verify(void *handle, unsigned char *blk, int len, { unsigned char correct[16]; hmacmd5_do_hmac_ssh(handle, blk, len, seq, correct); - return !memcmp(correct, blk + len, 16); + return smemeq(correct, blk + len, 16); } const struct ssh_mac ssh_hmac_md5 = { diff --git a/sshsh256.c b/sshsh256.c index 60c5217f..85bfeee2 100644 --- a/sshsh256.c +++ b/sshsh256.c @@ -307,7 +307,7 @@ static int hmacsha256_verresult(void *handle, unsigned char const *hmac) { unsigned char correct[32]; hmacsha256_genresult(handle, correct); - return !memcmp(correct, hmac, 32); + return smemeq(correct, hmac, 32); } static int sha256_verify(void *handle, unsigned char *blk, int len, @@ -315,7 +315,7 @@ static int sha256_verify(void *handle, unsigned char *blk, int len, { unsigned char correct[32]; sha256_do_hmac(handle, blk, len, seq, correct); - return !memcmp(correct, blk + len, 32); + return smemeq(correct, blk + len, 32); } const struct ssh_mac ssh_hmac_sha256 = { diff --git a/sshsha.c b/sshsha.c index a5b3a60c..31d6daa9 100644 --- a/sshsha.c +++ b/sshsha.c @@ -342,7 +342,7 @@ static int hmacsha1_verresult(void *handle, unsigned char const *hmac) { unsigned char correct[20]; hmacsha1_genresult(handle, correct); - return !memcmp(correct, hmac, 20); + return smemeq(correct, hmac, 20); } static int sha1_verify(void *handle, unsigned char *blk, int len, @@ -350,7 +350,7 @@ static int sha1_verify(void *handle, unsigned char *blk, int len, { unsigned char correct[20]; sha1_do_hmac(handle, blk, len, seq, correct); - return !memcmp(correct, blk + len, 20); + return smemeq(correct, blk + len, 20); } static void hmacsha1_96_genresult(void *handle, unsigned char *hmac) @@ -372,7 +372,7 @@ static int hmacsha1_96_verresult(void *handle, unsigned char const *hmac) { unsigned char correct[20]; hmacsha1_genresult(handle, correct); - return !memcmp(correct, hmac, 12); + return smemeq(correct, hmac, 12); } static int sha1_96_verify(void *handle, unsigned char *blk, int len, @@ -380,7 +380,7 @@ static int sha1_96_verify(void *handle, unsigned char *blk, int len, { unsigned char correct[20]; sha1_do_hmac(handle, blk, len, seq, correct); - return !memcmp(correct, blk + len, 12); + return smemeq(correct, blk + len, 12); } void hmac_sha1_simple(void *key, int keylen, void *data, int datalen, From 2105b68e6e6068bc2b7182d20f1f11f04130675b Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 26 Apr 2015 23:55:33 +0100 Subject: [PATCH 16/33] Add smemclrs of all hash states we destroy. (cherry picked from commit 16c46ecdaf71e4c9dddcd933778f02d78425f6a5) Conflicts: sshsh512.c Cherry-picker's notes: the conflict was because the original commit also added smemclrs to SHA384_Simple and the ssh_hash structures for SHA-384 and SHA-512, none of which exists on this branch so those changes are irrelevant. --- sshmd5.c | 2 ++ sshsh256.c | 3 +++ sshsh512.c | 1 + sshsha.c | 3 +++ 4 files changed, 9 insertions(+) diff --git a/sshmd5.c b/sshmd5.c index 493db12c..ca5c426c 100644 --- a/sshmd5.c +++ b/sshmd5.c @@ -210,6 +210,7 @@ void MD5Simple(void const *p, unsigned len, unsigned char output[16]) MD5Init(&s); MD5Update(&s, (unsigned char const *)p, len); MD5Final(output, &s); + smemclr(&s, sizeof(s)); } /* ---------------------------------------------------------------------- @@ -227,6 +228,7 @@ void *hmacmd5_make_context(void) void hmacmd5_free_context(void *handle) { + smemclr(handle, 3*sizeof(struct MD5Context)); sfree(handle); } diff --git a/sshsh256.c b/sshsh256.c index 85bfeee2..2f7b52ba 100644 --- a/sshsh256.c +++ b/sshsh256.c @@ -184,6 +184,7 @@ void SHA256_Simple(const void *p, int len, unsigned char *output) { SHA256_Init(&s); SHA256_Bytes(&s, p, len); SHA256_Final(&s, output); + smemclr(&s, sizeof(s)); } /* @@ -211,6 +212,7 @@ static void sha256_final(void *handle, unsigned char *output) SHA256_State *s = handle; SHA256_Final(s, output); + smemclr(s, sizeof(*s)); sfree(s); } @@ -230,6 +232,7 @@ static void *sha256_make_context(void) static void sha256_free_context(void *handle) { + smemclr(handle, 3 * sizeof(SHA256_State)); sfree(handle); } diff --git a/sshsh512.c b/sshsh512.c index 8d0b1ae3..eef733e6 100644 --- a/sshsh512.c +++ b/sshsh512.c @@ -274,6 +274,7 @@ void SHA512_Simple(const void *p, int len, unsigned char *output) { SHA512_Init(&s); SHA512_Bytes(&s, p, len); SHA512_Final(&s, output); + smemclr(&s, sizeof(s)); } #ifdef TEST diff --git a/sshsha.c b/sshsha.c index 31d6daa9..747e4414 100644 --- a/sshsha.c +++ b/sshsha.c @@ -214,6 +214,7 @@ void SHA_Simple(const void *p, int len, unsigned char *output) SHA_Init(&s); SHA_Bytes(&s, p, len); SHA_Final(&s, output); + smemclr(&s, sizeof(s)); } /* @@ -241,6 +242,7 @@ static void sha1_final(void *handle, unsigned char *output) SHA_State *s = handle; SHA_Final(s, output); + smemclr(s, sizeof(*s)); sfree(s); } @@ -260,6 +262,7 @@ static void *sha1_make_context(void) static void sha1_free_context(void *handle) { + smemclr(handle, 3 * sizeof(SHA_State)); sfree(handle); } From ad8c19aa1bdd20399072ac4cd1b36500078ec725 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 27 Apr 2015 06:54:21 +0100 Subject: [PATCH 17/33] Paste error in comment. SSH2_MSG_KEX_DH_GEX_REQUEST_OLD and SSH2_MSG_KEX_DH_GEX_REQUEST were correctly _defined_ as different numbers, but the comments to the right containing the hex representations of their values were accidentally the same. (cherry picked from commit a8658edb17a462da32499752810bd6c989159500) --- ssh.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ssh.h b/ssh.h index e50fbca3..a92add2f 100644 --- a/ssh.h +++ b/ssh.h @@ -721,7 +721,7 @@ void platform_ssh_share_cleanup(const char *name); #define SSH2_MSG_KEXDH_INIT 30 /* 0x1e */ #define SSH2_MSG_KEXDH_REPLY 31 /* 0x1f */ #define SSH2_MSG_KEX_DH_GEX_REQUEST_OLD 30 /* 0x1e */ -#define SSH2_MSG_KEX_DH_GEX_REQUEST 34 /* 0x1e */ +#define SSH2_MSG_KEX_DH_GEX_REQUEST 34 /* 0x22 */ #define SSH2_MSG_KEX_DH_GEX_GROUP 31 /* 0x1f */ #define SSH2_MSG_KEX_DH_GEX_INIT 32 /* 0x20 */ #define SSH2_MSG_KEX_DH_GEX_REPLY 33 /* 0x21 */ From 452c49a996a2f8751176d3c34f914718cbef8280 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 27 Apr 2015 20:48:29 +0100 Subject: [PATCH 18/33] Provide a script to regenerate the Blowfish init tables. Since I've recently published a program that can easily generate the required digits of pi, and since I was messing around in sshblowf.c already, it seemed like a good idea to provide a derivation of all that hex data. (cherry picked from commit 2968563180ae5013976123d8c5106a6c394b96a6) --- sshblowf.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/sshblowf.c b/sshblowf.c index e3b4f509..8a106cbe 100644 --- a/sshblowf.c +++ b/sshblowf.c @@ -16,6 +16,27 @@ typedef struct { /* * The Blowfish init data: hex digits of the fractional part of pi. * (ie pi as a hex fraction is 3.243F6A8885A308D3...) + * + * If you have Simon Tatham's 'spigot' exact real calculator + * available, or any other method of generating 8336 fractional hex + * digits of pi on standard output, you can regenerate these tables + * exactly as below using the following Perl script (adjusting the + * first line or two if your pi-generator is not spigot). + +open my $spig, "spigot -n -B16 -d8336 pi |"; +read $spig, $ignore, 2; # throw away the leading "3." +for my $name ("parray", "sbox0".."sbox3") { + print "static const word32 ${name}[] = {\n"; + my $len = $name eq "parray" ? 18 : 256; + for my $i (1..$len) { + read $spig, $word, 8; + printf "%s0x%s,", ($i%6==1 ? " " : " "), uc $word; + print "\n" if ($i == $len || $i%6 == 0); + } + print "};\n\n"; +} +close $spig; + */ static const word32 parray[] = { 0x243F6A88, 0x85A308D3, 0x13198A2E, 0x03707344, 0xA4093822, 0x299F31D0, From 4c24c8dc5a252a3d1df604ecb0cdfd82267aa94d Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Fri, 8 May 2015 18:57:18 +0100 Subject: [PATCH 19/33] Fix two small memory leaks in config mechanism. The memory dangling off ssd->sesslist should be freed when ssd itself goes away, and the font settings ctrlset we delete in gtkcfg.c should be freed as well once it's been removed from its containing array. Thanks to Ranjini Aravind for pointing these out. (cherry picked from commit f4956a1f9dc66973c1a9c9196ef893412b2545d7) --- config.c | 1 + unix/gtkcfg.c | 1 + 2 files changed, 2 insertions(+) diff --git a/config.c b/config.c index 0ad55413..3100503f 100644 --- a/config.c +++ b/config.c @@ -568,6 +568,7 @@ struct sessionsaver_data { static void sessionsaver_data_free(void *ssdv) { struct sessionsaver_data *ssd = (struct sessionsaver_data *)ssdv; + get_sesslist(&ssd->sesslist, FALSE); sfree(ssd->savedsession); sfree(ssd); } diff --git a/unix/gtkcfg.c b/unix/gtkcfg.c index ab8ef973..958a3f66 100644 --- a/unix/gtkcfg.c +++ b/unix/gtkcfg.c @@ -81,6 +81,7 @@ void gtk_setup_config_box(struct controlbox *b, int midsession, void *win) memmove(b->ctrlsets+i, b->ctrlsets+i+1, (b->nctrlsets-i-1) * sizeof(*b->ctrlsets)); b->nctrlsets--; + ctrl_free_set(s2); break; } } From 3ba1a7cf4b469ceded01c33ec5b76b7e8714d035 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Fri, 8 May 2015 19:04:16 +0100 Subject: [PATCH 20/33] Completely remove the privdata mechanism in dialog.h. The last use of it, to store the contents of the saved session name edit box, was removed nearly two years ago in svn r9923 and replaced by ctrl_alloc_with_free. The mechanism has been unused ever since then, and I suspect any further uses of it would be a bad idea for the same reasons, so let's get rid of it. (cherry picked from commit 42c592c4ef024af30af91241f651f699d6dbff0b) --- dialog.h | 15 --------- macosx/osxctrls.m | 34 -------------------- unix/gtkdlg.c | 34 -------------------- windows/winctrls.c | 79 ---------------------------------------------- 4 files changed, 162 deletions(-) diff --git a/dialog.h b/dialog.h index 6f454f56..67751670 100644 --- a/dialog.h +++ b/dialog.h @@ -634,21 +634,6 @@ int dlg_coloursel_results(union control *ctrl, void *dlg, */ void dlg_refresh(union control *ctrl, void *dlg); -/* - * It's perfectly possible that individual controls might need to - * allocate or store per-dialog-instance data, so here's a - * mechanism. - * - * `dlg_get_privdata' and `dlg_set_privdata' allow the user to get - * and set a void * pointer associated with the control in - * question. `dlg_alloc_privdata' will allocate memory, store a - * pointer to that memory in the private data field, and arrange - * for it to be automatically deallocated on dialog cleanup. - */ -void *dlg_get_privdata(union control *ctrl, void *dlg); -void dlg_set_privdata(union control *ctrl, void *dlg, void *ptr); -void *dlg_alloc_privdata(union control *ctrl, void *dlg, size_t size); - /* * Standard helper functions for reading a controlbox structure. */ diff --git a/macosx/osxctrls.m b/macosx/osxctrls.m index 12e813c9..b4270f9d 100644 --- a/macosx/osxctrls.m +++ b/macosx/osxctrls.m @@ -243,8 +243,6 @@ struct fe_ctrl { NSTableView *tableview; NSScrollView *scrollview; int nradiobuttons; - void *privdata; - int privdata_needs_free; }; static int fe_ctrl_cmp_by_ctrl(void *av, void *bv) @@ -346,16 +344,12 @@ static struct fe_ctrl *fe_ctrl_new(union control *ctrl) c->scrollview = nil; c->radiobuttons = NULL; c->nradiobuttons = 0; - c->privdata = NULL; - c->privdata_needs_free = FALSE; return c; } static void fe_ctrl_free(struct fe_ctrl *c) { - if (c->privdata_needs_free) - sfree(c->privdata); sfree(c->radiobuttons); sfree(c); } @@ -1785,31 +1779,3 @@ void dlg_refresh(union control *ctrl, void *dv) } } } - -void *dlg_get_privdata(union control *ctrl, void *dv) -{ - struct fe_dlg *d = (struct fe_dlg *)dv; - struct fe_ctrl *c = fe_ctrl_byctrl(d, ctrl); - return c->privdata; -} - -void dlg_set_privdata(union control *ctrl, void *dv, void *ptr) -{ - struct fe_dlg *d = (struct fe_dlg *)dv; - struct fe_ctrl *c = fe_ctrl_byctrl(d, ctrl); - c->privdata = ptr; - c->privdata_needs_free = FALSE; -} - -void *dlg_alloc_privdata(union control *ctrl, void *dv, size_t size) -{ - struct fe_dlg *d = (struct fe_dlg *)dv; - struct fe_ctrl *c = fe_ctrl_byctrl(d, ctrl); - /* - * This is an internal allocation routine, so it's allowed to - * use smalloc directly. - */ - c->privdata = smalloc(size); - c->privdata_needs_free = TRUE; - return c->privdata; -} diff --git a/unix/gtkdlg.c b/unix/gtkdlg.c index 2b8fec17..86a71cd6 100644 --- a/unix/gtkdlg.c +++ b/unix/gtkdlg.c @@ -37,8 +37,6 @@ struct Shortcuts { struct uctrl { union control *ctrl; GtkWidget *toplevel; - void *privdata; - int privdata_needs_free; GtkWidget **buttons; int nbuttons; /* for radio buttons */ GtkWidget *entry; /* for editbox, filesel, fontsel */ GtkWidget *button; /* for filesel, fontsel */ @@ -189,8 +187,6 @@ static void dlg_cleanup(struct dlgparam *dp) dp->byctrl = NULL; while ( (uc = index234(dp->bywidget, 0)) != NULL) { del234(dp->bywidget, uc); - if (uc->privdata_needs_free) - sfree(uc->privdata); sfree(uc->buttons); sfree(uc); } @@ -228,34 +224,6 @@ static struct uctrl *dlg_find_bywidget(struct dlgparam *dp, GtkWidget *w) return ret; } -void *dlg_get_privdata(union control *ctrl, void *dlg) -{ - struct dlgparam *dp = (struct dlgparam *)dlg; - struct uctrl *uc = dlg_find_byctrl(dp, ctrl); - return uc->privdata; -} - -void dlg_set_privdata(union control *ctrl, void *dlg, void *ptr) -{ - struct dlgparam *dp = (struct dlgparam *)dlg; - struct uctrl *uc = dlg_find_byctrl(dp, ctrl); - uc->privdata = ptr; - uc->privdata_needs_free = FALSE; -} - -void *dlg_alloc_privdata(union control *ctrl, void *dlg, size_t size) -{ - struct dlgparam *dp = (struct dlgparam *)dlg; - struct uctrl *uc = dlg_find_byctrl(dp, ctrl); - /* - * This is an internal allocation routine, so it's allowed to - * use smalloc directly. - */ - uc->privdata = smalloc(size); - uc->privdata_needs_free = FALSE; - return uc->privdata; -} - union control *dlg_last_focused(union control *ctrl, void *dlg) { struct dlgparam *dp = (struct dlgparam *)dlg; @@ -1832,8 +1800,6 @@ GtkWidget *layout_ctrls(struct dlgparam *dp, struct Shortcuts *scs, uc = snew(struct uctrl); uc->ctrl = ctrl; - uc->privdata = NULL; - uc->privdata_needs_free = FALSE; uc->buttons = NULL; uc->entry = NULL; #if !GTK_CHECK_VERSION(2,4,0) diff --git a/windows/winctrls.c b/windows/winctrls.c index 49c13e8d..9bee9274 100644 --- a/windows/winctrls.c +++ b/windows/winctrls.c @@ -2532,23 +2532,6 @@ void dlg_set_fixed_pitch_flag(void *dlg, int flag) dp->fixed_pitch_fonts = flag; } -struct perctrl_privdata { - union control *ctrl; - void *data; - int needs_free; -}; - -static int perctrl_privdata_cmp(void *av, void *bv) -{ - struct perctrl_privdata *a = (struct perctrl_privdata *)av; - struct perctrl_privdata *b = (struct perctrl_privdata *)bv; - if (a->ctrl < b->ctrl) - return -1; - else if (a->ctrl > b->ctrl) - return +1; - return 0; -} - void dp_init(struct dlgparam *dp) { dp->nctrltrees = 0; @@ -2558,7 +2541,6 @@ void dp_init(struct dlgparam *dp) memset(dp->shortcuts, 0, sizeof(dp->shortcuts)); dp->hwnd = NULL; dp->wintitle = dp->errtitle = NULL; - dp->privdata = newtree234(perctrl_privdata_cmp); dp->fixed_pitch_fonts = TRUE; } @@ -2570,67 +2552,6 @@ void dp_add_tree(struct dlgparam *dp, struct winctrls *wc) void dp_cleanup(struct dlgparam *dp) { - struct perctrl_privdata *p; - - if (dp->privdata) { - while ( (p = index234(dp->privdata, 0)) != NULL ) { - del234(dp->privdata, p); - if (p->needs_free) - sfree(p->data); - sfree(p); - } - freetree234(dp->privdata); - dp->privdata = NULL; - } sfree(dp->wintitle); sfree(dp->errtitle); } - -void *dlg_get_privdata(union control *ctrl, void *dlg) -{ - struct dlgparam *dp = (struct dlgparam *)dlg; - struct perctrl_privdata tmp, *p; - tmp.ctrl = ctrl; - p = find234(dp->privdata, &tmp, NULL); - if (p) - return p->data; - else - return NULL; -} - -void dlg_set_privdata(union control *ctrl, void *dlg, void *ptr) -{ - struct dlgparam *dp = (struct dlgparam *)dlg; - struct perctrl_privdata tmp, *p; - tmp.ctrl = ctrl; - p = find234(dp->privdata, &tmp, NULL); - if (!p) { - p = snew(struct perctrl_privdata); - p->ctrl = ctrl; - p->needs_free = FALSE; - add234(dp->privdata, p); - } - p->data = ptr; -} - -void *dlg_alloc_privdata(union control *ctrl, void *dlg, size_t size) -{ - struct dlgparam *dp = (struct dlgparam *)dlg; - struct perctrl_privdata tmp, *p; - tmp.ctrl = ctrl; - p = find234(dp->privdata, &tmp, NULL); - if (!p) { - p = snew(struct perctrl_privdata); - p->ctrl = ctrl; - p->needs_free = FALSE; - add234(dp->privdata, p); - } - assert(!p->needs_free); - p->needs_free = TRUE; - /* - * This is an internal allocation routine, so it's allowed to - * use smalloc directly. - */ - p->data = smalloc(size); - return p->data; -} From 41f63b6e5dc8ff83306287ae702d875edff4586f Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 18 May 2015 13:57:45 +0100 Subject: [PATCH 21/33] Log identifying information for the other end of connections. When anyone connects to a PuTTY tool's listening socket - whether it's a user of a local->remote port forwarding, a connection-sharing downstream or a client of Pageant - we'd like to log as much information as we can find out about where the connection came from. To that end, I've implemented a function sk_peer_info() in the socket abstraction, which returns a freeform text string as best it can (or NULL, if it can't get anything at all) describing the thing at the other end of the connection. For TCP connections, this is done using getpeername() to get an IP address and port in the obvious way; for Unix-domain sockets, we attempt SO_PEERCRED (conditionalised on some moderately hairy autoconfery) to get the pid and owner of the peer. I haven't implemented anything for Windows named pipes, but I will if I hear of anything useful. (cherry picked from commit c8f83979a368d10e8def1796cdadd7f8f3bebf74) Conflicts: pageant.c Cherry-picker's notes: the conflict was because the original commit also added a use of the same feature in the centralised Pageant code, which doesn't exist on this branch. Also I had to remove 'const' from the type of the second parameter to wrap_send_port_open(), since this branch hasn't had the same extensive const-fixing as master. --- Recipe | 4 ++-- configure.ac | 22 ++++++++++++++++++++ errsock.c | 8 +++++++- network.h | 8 ++++++++ portfwd.c | 19 +++++++++++++++-- proxy.c | 3 ++- ssh.c | 9 ++++++-- ssh.h | 3 ++- sshshare.c | 5 ++++- unix/unix.h | 5 +++++ unix/uxnet.c | 51 +++++++++++++++++++++++++++++++++++++++++++++- unix/uxpeer.c | 32 +++++++++++++++++++++++++++++ unix/uxproxy.c | 3 ++- windows/winhsock.c | 8 +++++++- windows/winnet.c | 48 ++++++++++++++++++++++++++++++++++++++++--- windows/winnps.c | 8 +++++++- 16 files changed, 219 insertions(+), 17 deletions(-) create mode 100644 unix/uxpeer.c diff --git a/Recipe b/Recipe index bac258f1..ca6369d5 100644 --- a/Recipe +++ b/Recipe @@ -230,8 +230,8 @@ SFTP = sftp int64 logging MISC = timing callback misc version settings tree234 proxy conf WINMISC = MISC winstore winnet winhandl cmdline windefs winmisc winproxy + wintime winhsock errsock -UXMISC = MISC uxstore uxsel uxnet cmdline uxmisc uxproxy time -OSXMISC = MISC uxstore uxsel osxsel uxnet uxmisc uxproxy time +UXMISC = MISC uxstore uxsel uxnet uxpeer cmdline uxmisc uxproxy time +OSXMISC = MISC uxstore uxsel osxsel uxnet uxpeer uxmisc uxproxy time # Character set library, for use in pterm. CHARSET = sbcsdat slookup sbcs utf8 toucs fromucs xenc mimeenc macenc localenc diff --git a/configure.ac b/configure.ac index 29fbde6d..5818e032 100644 --- a/configure.ac +++ b/configure.ac @@ -132,6 +132,28 @@ AC_CHECK_FUNCS([getaddrinfo posix_openpt ptsname setresuid strsignal updwtmpx]) AC_CHECK_DECLS([CLOCK_MONOTONIC], [], [], [[#include ]]) AC_SEARCH_LIBS([clock_gettime], [rt], [AC_DEFINE([HAVE_CLOCK_GETTIME],[],[Define if clock_gettime() is available])]) +AC_CACHE_CHECK([for SO_PEERCRED and dependencies], [x_cv_linux_so_peercred], [ + AC_COMPILE_IFELSE([ + AC_LANG_PROGRAM([[ + #define _GNU_SOURCE + #include + #include + ]],[[ + struct ucred cr; + socklen_t crlen = sizeof(cr); + return getsockopt(0, SOL_SOCKET, SO_PEERCRED, &cr, &crlen) + + cr.pid + cr.uid + cr.gid; + ]] + )], + AS_VAR_SET(x_cv_linux_so_peercred, yes), + AS_VAR_SET(x_cv_linux_so_peercred, no) + ) +]) +AS_IF([test AS_VAR_GET(x_cv_linux_so_peercred) = yes], + [AC_DEFINE([HAVE_SO_PEERCRED], [1], + [Define if SO_PEERCRED works in the Linux fashion.])] +) + if test "x$GCC" = "xyes"; then : AC_SUBST(WARNINGOPTS, ['-Wall -Werror']) diff --git a/errsock.c b/errsock.c index 1b3a88a2..854fd8be 100644 --- a/errsock.c +++ b/errsock.c @@ -43,6 +43,11 @@ static const char *sk_error_socket_error(Socket s) return ps->error; } +static char *sk_error_peer_info(Socket s) +{ + return NULL; +} + Socket new_error_socket(const char *errmsg, Plug plug) { static const struct socket_function_table socket_fn_table = { @@ -53,7 +58,8 @@ Socket new_error_socket(const char *errmsg, Plug plug) NULL /* write_eof */, NULL /* flush */, NULL /* set_frozen */, - sk_error_socket_error + sk_error_socket_error, + sk_error_peer_info, }; Error_Socket ret; diff --git a/network.h b/network.h index 675d24c7..319475e7 100644 --- a/network.h +++ b/network.h @@ -38,6 +38,7 @@ struct socket_function_table { void (*set_frozen) (Socket s, int is_frozen); /* ignored by tcp, but vital for ssl */ const char *(*socket_error) (Socket s); + char *(*peer_info) (Socket s); }; typedef union { void *p; int i; } accept_ctx_t; @@ -181,6 +182,13 @@ const char *sk_addr_error(SockAddr addr); */ #define sk_set_frozen(s, is_frozen) (((*s)->set_frozen) (s, is_frozen)) +/* + * Return a (dynamically allocated) string giving some information + * about the other end of the socket, suitable for putting in log + * files. May be NULL if nothing is available at all. + */ +#define sk_peer_info(s) (((*s)->peer_info) (s)) + /* * Simple wrapper on getservbyname(), needed by ssh.c. Returns the * port number, in host byte order (suitable for printf and so on). diff --git a/portfwd.c b/portfwd.c index e3e63e39..edbd7bcc 100644 --- a/portfwd.c +++ b/portfwd.c @@ -157,6 +157,21 @@ static int pfl_closing(Plug plug, const char *error_msg, int error_code, return 1; } +static void wrap_send_port_open(void *channel, char *hostname, int port, + Socket s) +{ + char *peerinfo, *description; + peerinfo = sk_peer_info(s); + if (peerinfo) { + description = dupprintf("forwarding from %s", peerinfo); + sfree(peerinfo); + } else { + description = dupstr("forwarding"); + } + ssh_send_port_open(channel, hostname, port, description); + sfree(description); +} + static int pfd_receive(Plug plug, int urgent, char *data, int len) { struct PortForwarding *pf = (struct PortForwarding *) plug; @@ -371,7 +386,7 @@ static int pfd_receive(Plug plug, int urgent, char *data, int len) return 1; } else { /* asks to forward to the specified host/port for this */ - ssh_send_port_open(pf->c, pf->hostname, pf->port, "forwarding"); + wrap_send_port_open(pf->c, pf->hostname, pf->port, pf->s); } pf->dynamic = 0; @@ -510,7 +525,7 @@ static int pfl_accepting(Plug p, accept_fn_t constructor, accept_ctx_t ctx) return 1; } else { /* asks to forward to the specified host/port for this */ - ssh_send_port_open(pf->c, pf->hostname, pf->port, "forwarding"); + wrap_send_port_open(pf->c, pf->hostname, pf->port, s); } } diff --git a/proxy.c b/proxy.c index e2201c64..51754dc3 100644 --- a/proxy.c +++ b/proxy.c @@ -388,7 +388,8 @@ Socket new_connection(SockAddr addr, char *hostname, sk_proxy_write_eof, sk_proxy_flush, sk_proxy_set_frozen, - sk_proxy_socket_error + sk_proxy_socket_error, + NULL, /* peer_info */ }; static const struct plug_function_table plug_fn_table = { diff --git a/ssh.c b/ssh.c index 368cabef..059b6522 100644 --- a/ssh.c +++ b/ssh.c @@ -7608,9 +7608,14 @@ static void ssh_check_termination(Ssh ssh) } } -void ssh_sharing_downstream_connected(Ssh ssh, unsigned id) +void ssh_sharing_downstream_connected(Ssh ssh, unsigned id, + const char *peerinfo) { - logeventf(ssh, "Connection sharing downstream #%u connected", id); + if (peerinfo) + logeventf(ssh, "Connection sharing downstream #%u connected from %s", + id, peerinfo); + else + logeventf(ssh, "Connection sharing downstream #%u connected", id); } void ssh_sharing_downstream_disconnected(Ssh ssh, unsigned id) diff --git a/ssh.h b/ssh.h index a92add2f..f55136e9 100644 --- a/ssh.h +++ b/ssh.h @@ -44,7 +44,8 @@ void ssh_sharing_remove_x11_display(Ssh ssh, struct X11FakeAuth *auth); void ssh_send_packet_from_downstream(Ssh ssh, unsigned id, int type, const void *pkt, int pktlen, const char *additional_log_text); -void ssh_sharing_downstream_connected(Ssh ssh, unsigned id); +void ssh_sharing_downstream_connected(Ssh ssh, unsigned id, + const char *peerinfo); void ssh_sharing_downstream_disconnected(Ssh ssh, unsigned id); void ssh_sharing_logf(Ssh ssh, unsigned id, const char *logfmt, ...); int ssh_agent_forwarding_permitted(Ssh ssh); diff --git a/sshshare.c b/sshshare.c index 54d58a66..2b2f69a9 100644 --- a/sshshare.c +++ b/sshshare.c @@ -1924,6 +1924,7 @@ static int share_listen_accepting(Plug plug, struct ssh_sharing_state *sharestate = (struct ssh_sharing_state *)plug; struct ssh_sharing_connstate *cs; const char *err; + char *peerinfo; /* * A new downstream has connected to us. @@ -1966,7 +1967,9 @@ static int share_listen_accepting(Plug plug, cs->forwardings = newtree234(share_forwarding_cmp); cs->globreq_head = cs->globreq_tail = NULL; - ssh_sharing_downstream_connected(sharestate->ssh, cs->id); + peerinfo = sk_peer_info(cs->sock); + ssh_sharing_downstream_connected(sharestate->ssh, cs->id, peerinfo); + sfree(peerinfo); return 0; } diff --git a/unix/unix.h b/unix/unix.h index d43e0ae0..917976d6 100644 --- a/unix/unix.h +++ b/unix/unix.h @@ -184,4 +184,9 @@ void *sk_getxdmdata(void *sock, int *lenp); */ extern Backend serial_backend; +/* + * uxpeer.c, wrapping getsockopt(SO_PEERCRED). + */ +int so_peercred(int fd, int *pid, int *uid, int *gid); + #endif diff --git a/unix/uxnet.c b/unix/uxnet.c index 235d6fd9..5058d8ab 100644 --- a/unix/uxnet.c +++ b/unix/uxnet.c @@ -16,6 +16,8 @@ #include #include #include +#include +#include #define DEFINE_PLUG_METHOD_MACROS #include "putty.h" @@ -485,6 +487,7 @@ static int sk_tcp_write(Socket s, const char *data, int len); static int sk_tcp_write_oob(Socket s, const char *data, int len); static void sk_tcp_write_eof(Socket s); static void sk_tcp_set_frozen(Socket s, int is_frozen); +static char *sk_tcp_peer_info(Socket s); static const char *sk_tcp_socket_error(Socket s); static struct socket_function_table tcp_fn_table = { @@ -495,7 +498,8 @@ static struct socket_function_table tcp_fn_table = { sk_tcp_write_eof, sk_tcp_flush, sk_tcp_set_frozen, - sk_tcp_socket_error + sk_tcp_socket_error, + sk_tcp_peer_info, }; static Socket sk_tcp_accept(accept_ctx_t ctx, Plug plug) @@ -1419,6 +1423,51 @@ static void sk_tcp_set_frozen(Socket sock, int is_frozen) uxsel_tell(s); } +static char *sk_tcp_peer_info(Socket sock) +{ + Actual_Socket s = (Actual_Socket) sock; + struct sockaddr_storage addr; + socklen_t addrlen = sizeof(addr); + char buf[INET6_ADDRSTRLEN]; + + if (getpeername(s->s, (struct sockaddr *)&addr, &addrlen) < 0) + return NULL; + if (addr.ss_family == AF_INET) { + return dupprintf + ("%s:%d", + inet_ntoa(((struct sockaddr_in *)&addr)->sin_addr), + (int)ntohs(((struct sockaddr_in *)&addr)->sin_port)); +#ifndef NO_IPV6 + } else if (addr.ss_family == AF_INET6) { + return dupprintf + ("[%s]:%d", + inet_ntop(AF_INET6, &((struct sockaddr_in6 *)&addr)->sin6_addr, + buf, sizeof(buf)), + (int)ntohs(((struct sockaddr_in6 *)&addr)->sin6_port)); +#endif + } else if (addr.ss_family == AF_UNIX) { + /* + * For Unix sockets, the source address is unlikely to be + * helpful. Instead, we try SO_PEERCRED and try to get the + * source pid. + */ + int pid, uid, gid; + if (so_peercred(s->s, &pid, &uid, &gid)) { + char uidbuf[64], gidbuf[64]; + sprintf(uidbuf, "%d", uid); + sprintf(gidbuf, "%d", gid); + struct passwd *pw = getpwuid(uid); + struct group *gr = getgrgid(gid); + return dupprintf("pid %d (%s:%s)", pid, + pw ? pw->pw_name : uidbuf, + gr ? gr->gr_name : gidbuf); + } + return NULL; + } else { + return NULL; + } +} + static void uxsel_tell(Actual_Socket s) { int rwx = 0; diff --git a/unix/uxpeer.c b/unix/uxpeer.c new file mode 100644 index 00000000..57bcfb88 --- /dev/null +++ b/unix/uxpeer.c @@ -0,0 +1,32 @@ +/* + * Unix: wrapper for getsockopt(SO_PEERCRED), conditionalised on + * appropriate autoconfery. + */ + +#ifdef HAVE_CONFIG_H +# include "uxconfig.h" /* leading space prevents mkfiles.pl trying to follow */ +#endif + +#ifdef HAVE_SO_PEERCRED +#define _GNU_SOURCE +#include +#endif + +#include + +#include "putty.h" + +int so_peercred(int fd, int *pid, int *uid, int *gid) +{ +#ifdef HAVE_SO_PEERCRED + struct ucred cr; + socklen_t crlen = sizeof(cr); + if (getsockopt(fd, SOL_SOCKET, SO_PEERCRED, &cr, &crlen) == 0) { + *pid = cr.pid; + *uid = cr.uid; + *gid = cr.gid; + return TRUE; + } +#endif + return FALSE; +} diff --git a/unix/uxproxy.c b/unix/uxproxy.c index dfbcc86a..fe81d5fd 100644 --- a/unix/uxproxy.c +++ b/unix/uxproxy.c @@ -245,7 +245,8 @@ Socket platform_new_connection(SockAddr addr, char *hostname, sk_localproxy_write_eof, sk_localproxy_flush, sk_localproxy_set_frozen, - sk_localproxy_socket_error + sk_localproxy_socket_error, + NULL, /* peer_info */ }; Local_Proxy_Socket ret; diff --git a/windows/winhsock.c b/windows/winhsock.c index 6d7e7da4..6b088bb8 100644 --- a/windows/winhsock.c +++ b/windows/winhsock.c @@ -234,6 +234,11 @@ static const char *sk_handle_socket_error(Socket s) return ps->error; } +static char *sk_handle_peer_info(Socket s) +{ + return NULL; +} + Socket make_handle_socket(HANDLE send_H, HANDLE recv_H, Plug plug, int overlapped) { @@ -245,7 +250,8 @@ Socket make_handle_socket(HANDLE send_H, HANDLE recv_H, Plug plug, sk_handle_write_eof, sk_handle_flush, sk_handle_set_frozen, - sk_handle_socket_error + sk_handle_socket_error, + sk_handle_peer_info, }; Handle_Socket ret; diff --git a/windows/winnet.c b/windows/winnet.c index 039701cf..fdb83c80 100644 --- a/windows/winnet.c +++ b/windows/winnet.c @@ -160,6 +160,8 @@ DECL_WINDOWS_FUNCTION(static, struct servent FAR *, getservbyname, (const char FAR *, const char FAR *)); DECL_WINDOWS_FUNCTION(static, unsigned long, inet_addr, (const char FAR *)); DECL_WINDOWS_FUNCTION(static, char FAR *, inet_ntoa, (struct in_addr)); +DECL_WINDOWS_FUNCTION(static, const char FAR *, inet_ntop, + (int, void FAR *, char *, size_t)); DECL_WINDOWS_FUNCTION(static, int, connect, (SOCKET, const struct sockaddr FAR *, int)); DECL_WINDOWS_FUNCTION(static, int, bind, @@ -174,6 +176,8 @@ DECL_WINDOWS_FUNCTION(static, int, ioctlsocket, (SOCKET, long, u_long FAR *)); DECL_WINDOWS_FUNCTION(static, SOCKET, accept, (SOCKET, struct sockaddr FAR *, int FAR *)); +DECL_WINDOWS_FUNCTION(static, int, getpeername, + (SOCKET, struct sockaddr FAR *, int FAR *)); DECL_WINDOWS_FUNCTION(static, int, recv, (SOCKET, char FAR *, int, int)); DECL_WINDOWS_FUNCTION(static, int, WSAIoctl, (SOCKET, DWORD, LPVOID, DWORD, LPVOID, DWORD, @@ -288,6 +292,7 @@ void sk_init(void) GET_WINDOWS_FUNCTION(winsock_module, getservbyname); GET_WINDOWS_FUNCTION(winsock_module, inet_addr); GET_WINDOWS_FUNCTION(winsock_module, inet_ntoa); + GET_WINDOWS_FUNCTION(winsock_module, inet_ntop); GET_WINDOWS_FUNCTION(winsock_module, connect); GET_WINDOWS_FUNCTION(winsock_module, bind); GET_WINDOWS_FUNCTION(winsock_module, setsockopt); @@ -297,6 +302,7 @@ void sk_init(void) GET_WINDOWS_FUNCTION(winsock_module, shutdown); GET_WINDOWS_FUNCTION(winsock_module, ioctlsocket); GET_WINDOWS_FUNCTION(winsock_module, accept); + GET_WINDOWS_FUNCTION(winsock_module, getpeername); GET_WINDOWS_FUNCTION(winsock_module, recv); GET_WINDOWS_FUNCTION(winsock_module, WSAIoctl); @@ -861,6 +867,7 @@ static int sk_tcp_write_oob(Socket s, const char *data, int len); static void sk_tcp_write_eof(Socket s); static void sk_tcp_set_frozen(Socket s, int is_frozen); static const char *sk_tcp_socket_error(Socket s); +static char *sk_tcp_peer_info(Socket s); extern char *do_select(SOCKET skt, int startup); @@ -874,7 +881,8 @@ static Socket sk_tcp_accept(accept_ctx_t ctx, Plug plug) sk_tcp_write_eof, sk_tcp_flush, sk_tcp_set_frozen, - sk_tcp_socket_error + sk_tcp_socket_error, + sk_tcp_peer_info, }; DWORD err; @@ -1122,7 +1130,8 @@ Socket sk_new(SockAddr addr, int port, int privport, int oobinline, sk_tcp_write_eof, sk_tcp_flush, sk_tcp_set_frozen, - sk_tcp_socket_error + sk_tcp_socket_error, + sk_tcp_peer_info, }; Actual_Socket ret; @@ -1173,7 +1182,8 @@ Socket sk_newlistener(char *srcaddr, int port, Plug plug, int local_host_only, sk_tcp_write_eof, sk_tcp_flush, sk_tcp_set_frozen, - sk_tcp_socket_error + sk_tcp_socket_error, + sk_tcp_peer_info, }; SOCKET s; @@ -1744,6 +1754,38 @@ static const char *sk_tcp_socket_error(Socket sock) return s->error; } +static char *sk_tcp_peer_info(Socket sock) +{ + Actual_Socket s = (Actual_Socket) sock; +#ifdef NO_IPV6 + struct sockaddr_in addr; +#else + struct sockaddr_storage addr; +#endif + int addrlen = sizeof(addr); + char buf[INET6_ADDRSTRLEN]; + + if (p_getpeername(s->s, (struct sockaddr *)&addr, &addrlen) < 0) + return NULL; + + if (((struct sockaddr *)&addr)->sa_family == AF_INET) { + return dupprintf + ("%s:%d", + p_inet_ntoa(((struct sockaddr_in *)&addr)->sin_addr), + (int)p_ntohs(((struct sockaddr_in *)&addr)->sin_port)); +#ifndef NO_IPV6 + } else if (((struct sockaddr *)&addr)->sa_family == AF_INET6) { + return dupprintf + ("[%s]:%d", + p_inet_ntop(AF_INET6, &((struct sockaddr_in6 *)&addr)->sin6_addr, + buf, sizeof(buf)), + (int)p_ntohs(((struct sockaddr_in6 *)&addr)->sin6_port)); +#endif + } else { + return NULL; + } +} + static void sk_tcp_set_frozen(Socket sock, int is_frozen) { Actual_Socket s = (Actual_Socket) sock; diff --git a/windows/winnps.c b/windows/winnps.c index 7b4aa0db..2547fd71 100644 --- a/windows/winnps.c +++ b/windows/winnps.c @@ -71,6 +71,11 @@ static const char *sk_namedpipeserver_socket_error(Socket s) return ps->error; } +static char *sk_namedpipeserver_peer_info(Socket s) +{ + return NULL; +} + static int create_named_pipe(Named_Pipe_Server_Socket ps, int first_instance) { SECURITY_ATTRIBUTES sa; @@ -211,7 +216,8 @@ Socket new_named_pipe_listener(const char *pipename, Plug plug) NULL /* write_eof */, NULL /* flush */, NULL /* set_frozen */, - sk_namedpipeserver_socket_error + sk_namedpipeserver_socket_error, + sk_namedpipeserver_peer_info, }; Named_Pipe_Server_Socket ret; From 82814e18ec55ff08134222b7582090783f8c2513 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 18 May 2015 16:00:13 +0100 Subject: [PATCH 22/33] Log the client process ID for Windows named pipes too. Turns out it didn't take much googling to find the right API function. (cherry picked from commit 5fc4bbf59d420af5019dc086e558e18454eab6b5) --- windows/winhsock.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/windows/winhsock.c b/windows/winhsock.c index 6b088bb8..f52d5211 100644 --- a/windows/winhsock.c +++ b/windows/winhsock.c @@ -236,6 +236,26 @@ static const char *sk_handle_socket_error(Socket s) static char *sk_handle_peer_info(Socket s) { + Handle_Socket ps = (Handle_Socket) s; + ULONG pid; + static HMODULE kernel32_module; + DECL_WINDOWS_FUNCTION(static, BOOL, GetNamedPipeClientProcessId, + (HANDLE, PULONG)); + + if (!kernel32_module) { + kernel32_module = load_system32_dll("kernel32.dll"); + GET_WINDOWS_FUNCTION(kernel32_module, GetNamedPipeClientProcessId); + } + + /* + * Of course, not all handles managed by this module will be + * server ends of named pipes, but if they are, then it's useful + * to log what we can find out about the client end. + */ + if (p_GetNamedPipeClientProcessId && + p_GetNamedPipeClientProcessId(ps->send_H, &pid)) + return dupprintf("process id %lu", (unsigned long)pid); + return NULL; } From 4322e7093eccb78d6d67238979369990211f3edb Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 18 May 2015 21:17:21 +0100 Subject: [PATCH 23/33] Fix a compile warning with -DDEBUG. An unguarded write() in the dputs function caused gcc -Werror to fail to compile. I'm confused that this hasn't bitten me before, though - obviously normal builds of PuTTY condition out the faulty code, but _surely_ this can't be the first time I've enabled the developer diagnostics since gcc started complaining about unchecked syscall returns! (cherry picked from commit 35fde00fd1fdc084a78dc3e4c3f94dbf16bbd236) --- unix/uxmisc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/unix/uxmisc.c b/unix/uxmisc.c index 473aa94e..e65a3869 100644 --- a/unix/uxmisc.c +++ b/unix/uxmisc.c @@ -103,7 +103,7 @@ void dputs(char *buf) debug_fp = fopen("debug.log", "w"); } - write(1, buf, strlen(buf)); + if (write(1, buf, strlen(buf)) < 0) {} /* 'error check' to placate gcc */ fputs(buf, debug_fp); fflush(debug_fp); From 42b6ed842bd6d6d2f58e8a8210db19e82d834196 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Thu, 28 May 2015 18:14:14 +0100 Subject: [PATCH 24/33] Commit my replacement Windows I-beam mouse pointer. Installing this systemwide as the Windows text selection cursor is a workaround for 'black-pointer'. It's a white I-beam with a one-pixel black outline around it, so it should be visible on any background colour. (I suppose that a backdrop of tightly packed I-beams looking just like it might successfully hide it, but that's unlikely :-) I constructed this some years ago for personal use; I needed it again this week and had to go and recover it from a backup of a defunct system, which made me think I really ought to check it in somewhere, and this 'contrib' directory seems like the ideal place. (cherry picked from commit e222db14ff28482b668baf7c21bb415f29e6df58) --- contrib/nice-ibeam.cur | Bin 0 -> 766 bytes 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 contrib/nice-ibeam.cur diff --git a/contrib/nice-ibeam.cur b/contrib/nice-ibeam.cur new file mode 100644 index 0000000000000000000000000000000000000000..af185328407071f52d22c001e6197e621e3ab84c GIT binary patch literal 766 zcmd^-yAgme3_~9|`czO;G6tLB(XtB-B^v~pPZMaM<4_`hq+AN0AyCnEP}496Xn|go z#2{93947A`CR2{to)lGzMoLLW-Yz+NCF;23p+L;B5SQY`zx(Tait&7{9km1pCoRNV Jo_~0Lf)$mepQ-== literal 0 HcmV?d00001 From e6679d46022a8fb8ddb41af29e5c2684f68a7ef2 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 6 Jun 2015 14:52:29 +0100 Subject: [PATCH 25/33] Move BignumInt definitions into a header file. This allows files other than sshbn.c to work with the primitives necessary to build multi-word arithmetic functions satisfying all of PuTTY's portability constraints. (cherry picked from commit 2c60070aad2d959a9e7e850523352c23c6aa7009) Cherry-picker's notes: required on this branch because it's a dependency of f8b27925eee6a37df107a7cd2e718e997a52516e which we want. --- sshbn.c | 85 +--------------------------------------------------- sshbn.h | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 84 deletions(-) create mode 100644 sshbn.h diff --git a/sshbn.c b/sshbn.c index b1ea5d63..953f06b5 100644 --- a/sshbn.c +++ b/sshbn.c @@ -10,90 +10,7 @@ #include "misc.h" -/* - * Usage notes: - * * Do not call the DIVMOD_WORD macro with expressions such as array - * subscripts, as some implementations object to this (see below). - * * Note that none of the division methods below will cope if the - * quotient won't fit into BIGNUM_INT_BITS. Callers should be careful - * to avoid this case. - * If this condition occurs, in the case of the x86 DIV instruction, - * an overflow exception will occur, which (according to a correspondent) - * will manifest on Windows as something like - * 0xC0000095: Integer overflow - * The C variant won't give the right answer, either. - */ - -#if defined __GNUC__ && defined __i386__ -typedef unsigned long BignumInt; -typedef unsigned long long BignumDblInt; -#define BIGNUM_INT_MASK 0xFFFFFFFFUL -#define BIGNUM_TOP_BIT 0x80000000UL -#define BIGNUM_INT_BITS 32 -#define MUL_WORD(w1, w2) ((BignumDblInt)w1 * w2) -#define DIVMOD_WORD(q, r, hi, lo, w) \ - __asm__("div %2" : \ - "=d" (r), "=a" (q) : \ - "r" (w), "d" (hi), "a" (lo)) -#elif defined _MSC_VER && defined _M_IX86 -typedef unsigned __int32 BignumInt; -typedef unsigned __int64 BignumDblInt; -#define BIGNUM_INT_MASK 0xFFFFFFFFUL -#define BIGNUM_TOP_BIT 0x80000000UL -#define BIGNUM_INT_BITS 32 -#define MUL_WORD(w1, w2) ((BignumDblInt)w1 * w2) -/* Note: MASM interprets array subscripts in the macro arguments as - * assembler syntax, which gives the wrong answer. Don't supply them. - * */ -#define DIVMOD_WORD(q, r, hi, lo, w) do { \ - __asm mov edx, hi \ - __asm mov eax, lo \ - __asm div w \ - __asm mov r, edx \ - __asm mov q, eax \ -} while(0) -#elif defined _LP64 -/* 64-bit architectures can do 32x32->64 chunks at a time */ -typedef unsigned int BignumInt; -typedef unsigned long BignumDblInt; -#define BIGNUM_INT_MASK 0xFFFFFFFFU -#define BIGNUM_TOP_BIT 0x80000000U -#define BIGNUM_INT_BITS 32 -#define MUL_WORD(w1, w2) ((BignumDblInt)w1 * w2) -#define DIVMOD_WORD(q, r, hi, lo, w) do { \ - BignumDblInt n = (((BignumDblInt)hi) << BIGNUM_INT_BITS) | lo; \ - q = n / w; \ - r = n % w; \ -} while (0) -#elif defined _LLP64 -/* 64-bit architectures in which unsigned long is 32 bits, not 64 */ -typedef unsigned long BignumInt; -typedef unsigned long long BignumDblInt; -#define BIGNUM_INT_MASK 0xFFFFFFFFUL -#define BIGNUM_TOP_BIT 0x80000000UL -#define BIGNUM_INT_BITS 32 -#define MUL_WORD(w1, w2) ((BignumDblInt)w1 * w2) -#define DIVMOD_WORD(q, r, hi, lo, w) do { \ - BignumDblInt n = (((BignumDblInt)hi) << BIGNUM_INT_BITS) | lo; \ - q = n / w; \ - r = n % w; \ -} while (0) -#else -/* Fallback for all other cases */ -typedef unsigned short BignumInt; -typedef unsigned long BignumDblInt; -#define BIGNUM_INT_MASK 0xFFFFU -#define BIGNUM_TOP_BIT 0x8000U -#define BIGNUM_INT_BITS 16 -#define MUL_WORD(w1, w2) ((BignumDblInt)w1 * w2) -#define DIVMOD_WORD(q, r, hi, lo, w) do { \ - BignumDblInt n = (((BignumDblInt)hi) << BIGNUM_INT_BITS) | lo; \ - q = n / w; \ - r = n % w; \ -} while (0) -#endif - -#define BIGNUM_INT_BYTES (BIGNUM_INT_BITS / 8) +#include "sshbn.h" #define BIGNUM_INTERNAL typedef BignumInt *Bignum; diff --git a/sshbn.h b/sshbn.h new file mode 100644 index 00000000..3d15b948 --- /dev/null +++ b/sshbn.h @@ -0,0 +1,92 @@ +/* + * sshbn.h: the assorted conditional definitions of BignumInt and + * multiply/divide macros used throughout the bignum code to treat + * numbers as arrays of the most conveniently sized word for the + * target machine. Exported so that other code (e.g. poly1305) can use + * it too. + */ + +/* + * Usage notes: + * * Do not call the DIVMOD_WORD macro with expressions such as array + * subscripts, as some implementations object to this (see below). + * * Note that none of the division methods below will cope if the + * quotient won't fit into BIGNUM_INT_BITS. Callers should be careful + * to avoid this case. + * If this condition occurs, in the case of the x86 DIV instruction, + * an overflow exception will occur, which (according to a correspondent) + * will manifest on Windows as something like + * 0xC0000095: Integer overflow + * The C variant won't give the right answer, either. + */ + +#if defined __GNUC__ && defined __i386__ +typedef unsigned long BignumInt; +typedef unsigned long long BignumDblInt; +#define BIGNUM_INT_MASK 0xFFFFFFFFUL +#define BIGNUM_TOP_BIT 0x80000000UL +#define BIGNUM_INT_BITS 32 +#define MUL_WORD(w1, w2) ((BignumDblInt)w1 * w2) +#define DIVMOD_WORD(q, r, hi, lo, w) \ + __asm__("div %2" : \ + "=d" (r), "=a" (q) : \ + "r" (w), "d" (hi), "a" (lo)) +#elif defined _MSC_VER && defined _M_IX86 +typedef unsigned __int32 BignumInt; +typedef unsigned __int64 BignumDblInt; +#define BIGNUM_INT_MASK 0xFFFFFFFFUL +#define BIGNUM_TOP_BIT 0x80000000UL +#define BIGNUM_INT_BITS 32 +#define MUL_WORD(w1, w2) ((BignumDblInt)w1 * w2) +/* Note: MASM interprets array subscripts in the macro arguments as + * assembler syntax, which gives the wrong answer. Don't supply them. + * */ +#define DIVMOD_WORD(q, r, hi, lo, w) do { \ + __asm mov edx, hi \ + __asm mov eax, lo \ + __asm div w \ + __asm mov r, edx \ + __asm mov q, eax \ +} while(0) +#elif defined _LP64 +/* 64-bit architectures can do 32x32->64 chunks at a time */ +typedef unsigned int BignumInt; +typedef unsigned long BignumDblInt; +#define BIGNUM_INT_MASK 0xFFFFFFFFU +#define BIGNUM_TOP_BIT 0x80000000U +#define BIGNUM_INT_BITS 32 +#define MUL_WORD(w1, w2) ((BignumDblInt)w1 * w2) +#define DIVMOD_WORD(q, r, hi, lo, w) do { \ + BignumDblInt n = (((BignumDblInt)hi) << BIGNUM_INT_BITS) | lo; \ + q = n / w; \ + r = n % w; \ +} while (0) +#elif defined _LLP64 +/* 64-bit architectures in which unsigned long is 32 bits, not 64 */ +typedef unsigned long BignumInt; +typedef unsigned long long BignumDblInt; +#define BIGNUM_INT_MASK 0xFFFFFFFFUL +#define BIGNUM_TOP_BIT 0x80000000UL +#define BIGNUM_INT_BITS 32 +#define MUL_WORD(w1, w2) ((BignumDblInt)w1 * w2) +#define DIVMOD_WORD(q, r, hi, lo, w) do { \ + BignumDblInt n = (((BignumDblInt)hi) << BIGNUM_INT_BITS) | lo; \ + q = n / w; \ + r = n % w; \ +} while (0) +#else +/* Fallback for all other cases */ +typedef unsigned short BignumInt; +typedef unsigned long BignumDblInt; +#define BIGNUM_INT_MASK 0xFFFFU +#define BIGNUM_TOP_BIT 0x8000U +#define BIGNUM_INT_BITS 16 +#define MUL_WORD(w1, w2) ((BignumDblInt)w1 * w2) +#define DIVMOD_WORD(q, r, hi, lo, w) do { \ + BignumDblInt n = (((BignumDblInt)hi) << BIGNUM_INT_BITS) | lo; \ + q = n / w; \ + r = n % w; \ +} while (0) +#endif + +#define BIGNUM_INT_BYTES (BIGNUM_INT_BITS / 8) From ae93b52a9c80d84470250ae8595d2cdf1448562a Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 7 Jun 2015 21:09:41 +0100 Subject: [PATCH 26/33] Clean up downstream sockets when upstream loses its SSH connection. If the real SSH connection goes away and we call sharestate_free with downstreams still active, then that in turn calls share_connstate_free on all those downstreams, freeing the things their sockets are using as Plugs but not actually closing the sockets, so further data coming in from downstream gives rise to a use-after-free bug. (Thanks to Timothe Litt for a great deal of help debugging this.) (cherry picked from commit 0b2f283622603242d8bce295e42342649aebbb97) --- sshshare.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sshshare.c b/sshshare.c index 2b2f69a9..1c0e3cba 100644 --- a/sshshare.c +++ b/sshshare.c @@ -502,6 +502,9 @@ static void share_connstate_free(struct ssh_sharing_connstate *cs) sfree(globreq); } + if (cs->sock) + sk_close(cs->sock); + sfree(cs); } From d75d136c68f174c9f94f283ba515fefbe4ce69ab Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 7 Jun 2015 21:14:09 +0100 Subject: [PATCH 27/33] Don't try sending on sharing channels. The final main loop in do_ssh2_authconn will sometimes loop over all currently open channels calling ssh2_try_send_and_unthrottle. If the channel is a sharing one, however, that will reference fields of the channel structure like 'remwindow', which were never initialised in the first place (thanks, valgrind). Fix by excluding CHAN_SHARING channels from that loop. (cherry picked from commit 7366fde1d4831dcc701bc31e9de1113636fba1c5) --- ssh.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ssh.c b/ssh.c index 059b6522..dd969622 100644 --- a/ssh.c +++ b/ssh.c @@ -10325,7 +10325,8 @@ static void do_ssh2_authconn(Ssh ssh, unsigned char *in, int inlen, * Try to send data on all channels if we can. */ for (i = 0; NULL != (c = index234(ssh->channels, i)); i++) - ssh2_try_send_and_unthrottle(ssh, c); + if (c->type != CHAN_SHARING) + ssh2_try_send_and_unthrottle(ssh, c); } } From 0aa18b242e8ddc0ae4646a64af62d3648cea2e4a Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 8 Jun 2015 19:22:55 +0100 Subject: [PATCH 28/33] Provide a stub random_byte() to make 'testbn' compile again. The function bignum_random_in_range() is new to sshbn.c since I last tried to run the bignum test code. (cherry picked from commit 0aa92c8fa2bee2e4c0082adcc9f06ead24989698) --- sshbn.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/sshbn.c b/sshbn.c index 953f06b5..86f05949 100644 --- a/sshbn.c +++ b/sshbn.c @@ -1746,6 +1746,12 @@ void modalfatalbox(char *p, ...) exit(1); } +int random_byte(void) +{ + modalfatalbox("random_byte called in testbn"); + return 0; +} + #define fromxdigit(c) ( (c)>'9' ? ((c)&0xDF) - 'A' + 10 : (c) - '0' ) int main(int argc, char **argv) From 2725a51d3c44d1eb9760f08b8482db25d9401a86 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 8 Jun 2015 19:23:48 +0100 Subject: [PATCH 29/33] Improve integer-type hygiene in bignum code. In many places I was using an 'unsigned int', or an implicit int by virtue of writing an undecorated integer literal, where what was really wanted was a BignumInt. In particular, this substitution breaks in any situation where BignumInt is _larger_ than unsigned - which it is shortly about to be. (cherry picked from commit e28b35b0a39de28fa2f71aa78071d1ad62deaceb) Conflicts: sshbn.c sshccp.c Cherry-picker's notes: the conflicts were because the original commit also modified new code (sshccp.c for dedicated Poly1305 arithmetic routines, sshbn.c for a few bignum functions introduced on trunk for various pieces of new crypto) which doesn't exist on this branch. --- sshbn.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/sshbn.c b/sshbn.c index 86f05949..facdf3d5 100644 --- a/sshbn.c +++ b/sshbn.c @@ -515,7 +515,7 @@ static void monty_reduce(BignumInt *x, const BignumInt *n, } static void internal_add_shifted(BignumInt *number, - unsigned n, int shift) + BignumInt n, int shift) { int word = 1 + (shift / BIGNUM_INT_BITS); int bshift = shift % BIGNUM_INT_BITS; @@ -546,8 +546,7 @@ static void internal_mod(BignumInt *a, int alen, BignumInt *m, int mlen, BignumInt *quot, int qshift) { - BignumInt m0, m1; - unsigned int h; + BignumInt m0, m1, h; int i, k; m0 = m[0]; @@ -559,7 +558,7 @@ static void internal_mod(BignumInt *a, int alen, for (i = 0; i <= alen - mlen; i++) { BignumDblInt t; - unsigned int q, r, c, ai1; + BignumInt q, r, c, ai1; if (i == 0) { h = 0; @@ -614,7 +613,7 @@ static void internal_mod(BignumInt *a, int alen, for (k = mlen - 1; k >= 0; k--) { t = MUL_WORD(q, m[k]); t += c; - c = (unsigned)(t >> BIGNUM_INT_BITS); + c = (BignumInt)(t >> BIGNUM_INT_BITS); if ((BignumInt) t > a[i + k]) c++; a[i + k] -= (BignumInt) t; @@ -697,7 +696,7 @@ Bignum modpow_simple(Bignum base_in, Bignum exp, Bignum mod) /* Skip leading zero bits of exp. */ i = 0; j = BIGNUM_INT_BITS-1; - while (i < (int)exp[0] && (exp[exp[0] - i] & (1 << j)) == 0) { + while (i < (int)exp[0] && (exp[exp[0] - i] & ((BignumInt)1 << j)) == 0) { j--; if (j < 0) { i++; @@ -710,7 +709,7 @@ Bignum modpow_simple(Bignum base_in, Bignum exp, Bignum mod) while (j >= 0) { internal_mul(a + mlen, a + mlen, b, mlen, scratch); internal_mod(b, mlen * 2, m, mlen, NULL, 0); - if ((exp[exp[0] - i] & (1 << j)) != 0) { + if ((exp[exp[0] - i] & ((BignumInt)1 << j)) != 0) { internal_mul(b + mlen, n, a, mlen, scratch); internal_mod(a, mlen * 2, m, mlen, NULL, 0); } else { @@ -847,7 +846,7 @@ Bignum modpow(Bignum base_in, Bignum exp, Bignum mod) /* Skip leading zero bits of exp. */ i = 0; j = BIGNUM_INT_BITS-1; - while (i < (int)exp[0] && (exp[exp[0] - i] & (1 << j)) == 0) { + while (i < (int)exp[0] && (exp[exp[0] - i] & ((BignumInt)1 << j)) == 0) { j--; if (j < 0) { i++; @@ -860,7 +859,7 @@ Bignum modpow(Bignum base_in, Bignum exp, Bignum mod) while (j >= 0) { internal_mul(a + len, a + len, b, len, scratch); monty_reduce(b, n, mninv, scratch, len); - if ((exp[exp[0] - i] & (1 << j)) != 0) { + if ((exp[exp[0] - i] & ((BignumInt)1 << j)) != 0) { internal_mul(b + len, x, a, len, scratch); monty_reduce(a, n, mninv, scratch, len); } else { @@ -1110,7 +1109,8 @@ Bignum bignum_from_bytes(const unsigned char *data, int nbytes) result[i] = 0; for (i = nbytes; i--;) { unsigned char byte = *data++; - result[1 + i / BIGNUM_INT_BYTES] |= byte << (8*i % BIGNUM_INT_BITS); + result[1 + i / BIGNUM_INT_BYTES] |= + (BignumInt)byte << (8*i % BIGNUM_INT_BITS); } while (result[0] > 1 && result[result[0]] == 0) @@ -1206,7 +1206,7 @@ void bignum_set_bit(Bignum bn, int bitnum, int value) abort(); /* beyond the end */ else { int v = bitnum / BIGNUM_INT_BITS + 1; - int mask = 1 << (bitnum % BIGNUM_INT_BITS); + BignumInt mask = (BignumInt)1 << (bitnum % BIGNUM_INT_BITS); if (value) bn[v] |= mask; else From 445395c9d31dd49424cd5a01f8aa3f75a69cfe67 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 8 Jun 2015 19:24:58 +0100 Subject: [PATCH 30/33] Use 64-bit BignumInt wherever __uint128_t is available. gcc and clang both provide a type called __uint128_t when compiling for 64-bit targets, code-generated more or less similarly to the way 64-bit long longs are handled on 32-bit targets (spanning two registers, using ADD/ADC, that sort of thing). Where this is available (and they also provide a handy macro to make it easy to detect), we should obviously use it, so that we can handle bignums a larger chunk at a time and make use of the full width of the hardware's multiplier. Preliminary benchmarking using 'testbn' suggests a factor of about 2.5 improvement. I've added the new possibility to the ifdefs in sshbn.h, and also re-run contrib/make1305.py to generate a set of variants of the poly1305 arithmetic for the new size of BignumInt. (cherry picked from commit f8b27925eee6a37df107a7cd2e718e997a52516e) Conflicts: sshccp.c Cherry-picker's notes: the conflict arose because the original commit also added new 64-bit autogenerated forms of dedicated Poly1305 arithmetic, which doesn't exist on this branch. --- sshbn.h | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/sshbn.h b/sshbn.h index 3d15b948..a043241e 100644 --- a/sshbn.h +++ b/sshbn.h @@ -20,7 +20,24 @@ * The C variant won't give the right answer, either. */ -#if defined __GNUC__ && defined __i386__ +#if defined __SIZEOF_INT128__ +/* gcc and clang both provide a __uint128_t type on 64-bit targets + * (and, when they do, indicate its presence by the above macro), + * using the same 'two machine registers' kind of code generation that + * 32-bit targets use for 64-bit ints. If we have one of these, we can + * use a 64-bit BignumInt and a 128-bit BignumDblInt. */ +typedef __uint64_t BignumInt; +typedef __uint128_t BignumDblInt; +#define BIGNUM_INT_MASK 0xFFFFFFFFFFFFFFFFULL +#define BIGNUM_TOP_BIT 0x8000000000000000ULL +#define BIGNUM_INT_BITS 64 +#define MUL_WORD(w1, w2) ((BignumDblInt)w1 * w2) +#define DIVMOD_WORD(q, r, hi, lo, w) do { \ + BignumDblInt n = (((BignumDblInt)hi) << BIGNUM_INT_BITS) | lo; \ + q = n / w; \ + r = n % w; \ +} while (0) +#elif defined __GNUC__ && defined __i386__ typedef unsigned long BignumInt; typedef unsigned long long BignumDblInt; #define BIGNUM_INT_MASK 0xFFFFFFFFUL From 26fe1e26c0f7ab42440332882295667d4a0ac500 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 13 Jun 2015 15:22:03 +0100 Subject: [PATCH 31/33] Add missing null-pointer checks in key exchange. Assorted calls to ssh_pkt_getstring in handling the later parts of key exchange (post-KEXINIT) were not checked for NULL afterwards, so that a variety of badly formatted key exchange packets would cause a crash rather than a sensible error message. None of these is an exploitable vulnerability - the server can only force a clean null-deref crash, not an access to actually interesting memory. Thanks to '3unnym00n' for pointing out one of these, causing me to find all the rest of them too. (cherry picked from commit 1eb578a488a71284d6b18e46df301e54805f2c35) Conflicts: ssh.c Cherry-picker's notes: the main conflict arose because the original commit also made fixes to the ECDH branch of the big key exchange if statement, which doesn't exist on this branch. Also there was a minor and purely textual conflict, when an error check was added right next to a function call that had acquired an extra parameter on master. --- ssh.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/ssh.c b/ssh.c index dd969622..a37b0441 100644 --- a/ssh.c +++ b/ssh.c @@ -6668,13 +6668,20 @@ static void do_ssh2_transport(Ssh ssh, void *vin, int inlen, } set_busy_status(ssh->frontend, BUSY_CPU); /* cogitate */ ssh_pkt_getstring(pktin, &s->hostkeydata, &s->hostkeylen); - s->hkey = ssh->hostkey->newkey(s->hostkeydata, s->hostkeylen); + if (!s->hostkeydata) { + bombout(("unable to parse key exchange reply packet")); + crStopV; + } s->f = ssh2_pkt_getmp(pktin); if (!s->f) { bombout(("unable to parse key exchange reply packet")); crStopV; } ssh_pkt_getstring(pktin, &s->sigdata, &s->siglen); + if (!s->sigdata) { + bombout(("unable to parse key exchange reply packet")); + crStopV; + } { const char *err = dh_validate_f(ssh->kex_ctx, s->f); @@ -6723,6 +6730,10 @@ static void do_ssh2_transport(Ssh ssh, void *vin, int inlen, } ssh_pkt_getstring(pktin, &s->hostkeydata, &s->hostkeylen); + if (!s->hostkeydata) { + bombout(("unable to parse RSA public key packet")); + crStopV; + } hash_string(ssh->kex->hash, ssh->exhash, s->hostkeydata, s->hostkeylen); s->hkey = ssh->hostkey->newkey(s->hostkeydata, s->hostkeylen); @@ -6730,6 +6741,10 @@ static void do_ssh2_transport(Ssh ssh, void *vin, int inlen, { char *keydata; ssh_pkt_getstring(pktin, &keydata, &s->rsakeylen); + if (!keydata) { + bombout(("unable to parse RSA public key packet")); + crStopV; + } s->rsakeydata = snewn(s->rsakeylen, char); memcpy(s->rsakeydata, keydata, s->rsakeylen); } @@ -6806,6 +6821,10 @@ static void do_ssh2_transport(Ssh ssh, void *vin, int inlen, } ssh_pkt_getstring(pktin, &s->sigdata, &s->siglen); + if (!s->sigdata) { + bombout(("unable to parse signature packet")); + crStopV; + } sfree(s->rsakeydata); } From be9e5ea0a05782df2aa455853d6e1e2efe4772b8 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Thu, 18 Jun 2015 07:05:19 +0100 Subject: [PATCH 32/33] Fix accidental dependence on Windows API quirk in config box. Our config boxes are constructed using the CreateDialog() API function, rather than the modal DialogBox(). CreateDialog() is not that different from CreateWindow(), so windows created with it don't appear on the screen automatically; MSDN says that they must be shown via ShowWindow(), just like non-dialog windows have to be. But we weren't doing that at any point! So how was our config box ever getting displayed at all? Apparently by sheer chance, it turns out. The handler for a selection change in the tree view, which has to delete a whole panel of controls and creates a different set, surrounds that procedure with some WM_SETREDRAW calls and an InvalidateRect(), to prevent flicker while lots of changes were being made. And the creation of the _first_ panelful of controls, at dialog box setup, was done by simply selecting an item in the treeview and expecting that handler to be recursively called. And it appears that calling WM_SETREDRAW(TRUE) and then InvalidateRect was undocumentedly having an effect equivalent to the ShowWindow() we should have called, so that we never noticed the latter was missing. But a recent Vista update (all reports implicate KB3057839) has caused that not to work any more: on an updated Vista machine, in some desktop configurations, it seems that any attempt to fiddle with WM_SETREDRAW during dialog setup can leave the dialog box in a really unhelpful invisible state - the window is _physically there_ (you can see its taskbar entry, and the mouse pointer changes as you move over where its edit boxes are), but 100% transparent. So now we're doing something a bit more sensible. The first panelful of controls is created directly by the WM_INITDIALOG handler, rather than recursing into code that wasn't really designed to run at setup time. To be on the safe side, that handler for treeview selection change is also disabled until the WM_INITDIALOG handler has finished (like we already did with the WM_COMMAND handler), so that we can be sure of not accidentally messing about with WM_SETREDRAW at all during setup. And at the end of setup, we show the window in the sensible way, by a docs-approved call to ShowWindow(). This appears (on the one machine I've so far tested it on) to fix the Vista invisible-window issue, and also it should be more API-compliant and hence safer in future. (cherry picked from commit 6163710f043fb58fc80f6b45c14a92f7036bde75) --- windows/windlg.c | 44 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/windows/windlg.c b/windows/windlg.c index aadf88ea..716d046b 100644 --- a/windows/windlg.c +++ b/windows/windlg.c @@ -454,6 +454,7 @@ static int CALLBACK GenericMainDlgProc(HWND hwnd, UINT msg, HTREEITEM hfirst = NULL; int i; char *path = NULL; + char *firstpath = NULL; for (i = 0; i < ctrlbox->nctrlsets; i++) { struct controlset *s = ctrlbox->ctrlsets[i]; @@ -486,18 +487,26 @@ static int CALLBACK GenericMainDlgProc(HWND hwnd, UINT msg, c++; item = treeview_insert(&tvfaff, j, c, s->pathname); - if (!hfirst) + if (!hfirst) { hfirst = item; + firstpath = s->pathname; + } path = s->pathname; } /* - * Put the treeview selection on to the Session panel. - * This should also cause creation of the relevant - * controls. + * Put the treeview selection on to the first panel in the + * ctrlbox. */ TreeView_SelectItem(treeview, hfirst); + + /* + * And create the actual control set for that panel, to + * match the initial treeview selection. + */ + create_controls(hwnd, firstpath); + dlg_refresh(NULL, &dp); /* and set up control values */ } /* @@ -516,6 +525,18 @@ static int CALLBACK GenericMainDlgProc(HWND hwnd, UINT msg, } } + /* + * Now we've finished creating our initial set of controls, + * it's safe to actually show the window without risking setup + * flicker. + */ + ShowWindow(hwnd, SW_SHOWNORMAL); + + /* + * Set the flag that activates a couple of the other message + * handlers below, which were disabled until now to avoid + * spurious firing during the above setup procedure. + */ SetWindowLongPtr(hwnd, GWLP_USERDATA, 1); return 0; case WM_LBUTTONUP: @@ -530,10 +551,21 @@ static int CALLBACK GenericMainDlgProc(HWND hwnd, UINT msg, case WM_NOTIFY: if (LOWORD(wParam) == IDCX_TREEVIEW && ((LPNMHDR) lParam)->code == TVN_SELCHANGED) { - HTREEITEM i = - TreeView_GetSelection(((LPNMHDR) lParam)->hwndFrom); + /* + * Selection-change events on the treeview cause us to do + * a flurry of control deletion and creation - but only + * after WM_INITDIALOG has finished. The initial + * selection-change event(s) during treeview setup are + * ignored. + */ + HTREEITEM i; TVITEM item; char buffer[64]; + + if (GetWindowLongPtr(hwnd, GWLP_USERDATA) != 1) + return 0; + + i = TreeView_GetSelection(((LPNMHDR) lParam)->hwndFrom); SendMessage (hwnd, WM_SETREDRAW, FALSE, 0); From 06946b4d4b3816fc2b26dd650a0b9d379fabdd85 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Mon, 22 Jun 2015 19:36:57 +0100 Subject: [PATCH 33/33] Fix a mismerge in kex null-pointer checks. I removed a vital line of code while fixing the merge conflicts when cherry-picking 1eb578a488a71284d6b18e46df301e54805f2c35 as 26fe1e26c0f7ab42440332882295667d4a0ac500, causing Diffie-Hellman key exchange to be completely broken because the server's host key was never constructed to verify the signature with. Reinstate it. --- ssh.c | 1 + 1 file changed, 1 insertion(+) diff --git a/ssh.c b/ssh.c index a37b0441..a41a576c 100644 --- a/ssh.c +++ b/ssh.c @@ -6672,6 +6672,7 @@ static void do_ssh2_transport(Ssh ssh, void *vin, int inlen, bombout(("unable to parse key exchange reply packet")); crStopV; } + s->hkey = ssh->hostkey->newkey(s->hostkeydata, s->hostkeylen); s->f = ssh2_pkt_getmp(pktin); if (!s->f) { bombout(("unable to parse key exchange reply packet"));