Simon Tatham [Sun, 9 Aug 2015 10:33:43 +0000 (11:33 +0100)]
GTK 3 prep: write a replacement for gtk_quit_add().
GTK 2 has deprecated it and provided no replacement; a bug tracker
entry I found on the subject suggested that it was functionality that
didn't really belong in GTK, and glib ought to provide a replacement
instead, which would be a perfectly fine thing to suggest if they had
waited for glib to get round to doing so *before* throwing out a
function people were actually using. Sigh.
Anyway, it turns out that subsidiary invocations of gtk_main() don't
happen inside GTK as far as I can see, so all I need to do is to make
sure my own invocations of gtk_main() are followed by a cleanup
function which runs any quit functions that I've registered.
That was the last deprecated GTK function, so we now build cleanly
with -DGTK_DISABLE_DEPRECATED. (But, as mentioned a couple of commits
ago, we still don't build with -DGDK_DISABLE_DEPRECATED, because that
has migrating to Cairo drawing as a prerequisite.)
Simon Tatham [Sun, 9 Aug 2015 08:59:25 +0000 (09:59 +0100)]
Use gtkcompat.h to slim down a few ifdefs.
Now that I've got a general place to centralise handling of at least
the simple differences between GTK 1 and 2, I should use it wherever
possible. So this commit removes just a small number of ifdefs which
are either obsoleted by definitions already in gtkcompat.h (like
set_size_request vs set_usize), or can easily be replaced by adding
another (e.g. gtk_color_selection_set_has_opacity_control).
Simon Tatham [Sat, 8 Aug 2015 17:02:01 +0000 (18:02 +0100)]
GTK 3 prep: stop using *nearly* all GTK deprecated functions.
Building with -DGTK_DISABLE_DEPRECATED, we now suffer only one compile
failure, for the use of gtk_quit_add() in idle_toplevel_callback_func.
That function is apparently removed with no replacement in GTK 3, so
I'll need to find a completely different approach to getting toplevel
callbacks to run only in the outermost instance of gtk_main().
Also, this change doesn't do anything about the use of *GDK*
deprecated functions, because those include the entire family of
old-style drawing functions - i.e. the only way to build cleanly with
-DGDK_DISABLE_DEPRECATED will be to switch to Cairo drawing.
Simon Tatham [Sat, 8 Aug 2015 17:22:05 +0000 (18:22 +0100)]
Switch to using gtk_window_parse_geometry in GTK 2.
On GTK versions where it's available, this is a much nicer way of
handling the -geometry command-line option, since not only do we get
all the faffing about with gravity for free, it also automatically
sets the user-position WM hints.
Simon Tatham [Sat, 8 Aug 2015 16:34:25 +0000 (17:34 +0100)]
GTK 3 prep: replace GtkFileSelection with GtkFileChooserDialog.
I've put in a special #define to control this selection, in case I
decide that for reasons of taste I'd prefer to switch back to
GtkFileSelection in GTK2 which supports both!
Simon Tatham [Sat, 8 Aug 2015 16:32:15 +0000 (17:32 +0100)]
GTK 3 prep: use gtk_color_selection_get_current_color().
Replaces the deprecated gtk_color_selection_set_color() which took an
array of four doubles (RGBA), and instead takes a 'GdkColor' struct
containing four 16-bit integers.
For GTK1, we still have to retain the original version.
Simon Tatham [Sat, 8 Aug 2015 16:29:02 +0000 (17:29 +0100)]
GTK 3 prep: use the glib names for base object types.
All the things like GtkType, GtkObject, gtk_signal_connect and so on
should now consistently have the new-style glib names like GType,
GObject, g_signal_connect, etc.
Simon Tatham [Sat, 8 Aug 2015 15:35:19 +0000 (16:35 +0100)]
Allow direct use of X11 to be conditionally compiled out.
A major aim of introducing GTK 3 support is to permit compiling for
non-X11 platforms that GTK 3 supports, so I'm going to need to be able
to build as a pure GTK application with no use of X11 internals.
Naturally, I don't intend to stop supporting the hybrid GTK+X11 mode
in which X server-side bitmap fonts are available.
Use of X11 can be removed by compiling with -DNOT_X_WINDOWS. That's
the same compatibility flag that was already used by the unfinished OS
X port to disable the X-specific parts of uxpty.c; now it just applies
to more source files.
(There's no 'configure' option to set this flag at present. I haven't
worked out whether we'll need one yet.)
Simon Tatham [Sat, 8 Aug 2015 15:23:54 +0000 (16:23 +0100)]
GTK 3 prep: use GDK_KEY_<keyname> constants, not GDK_<keyname>.
GTK 2 doesn't _documentedly_ provide a helpful compile option to let
us check this one in advance of GTK 3, but you can fake one anyway by
compiling with -D__GDK_KEYSYMS_COMPAT_H__, so that gdkkeysyms-compat.h
will believe that it's already been included :-) We now build cleanly
under GTK 2 with that predefine.
Simon Tatham [Sat, 8 Aug 2015 14:06:15 +0000 (15:06 +0100)]
GTK 3 prep: do not include individual GTK/GDK headers.
This is the first of several cleanup steps recommended by the GTK 2->3
migration guide.
I intend to begin work towards compatibility with GTK 3, but without
breaking GTK 2 and even GTK 1 compatibility in the process; GTK 2 is
still useful to _me_ (not least because it permits much easier support
of old-style server-side X11 fonts), and I recall hearing a rumour
that at least one kind of strange system can only run GTK 1, so for
the moment I don't intend to stop supporting either.
Including gdkkeysyms.h is not optional in GTK 2, because gdk.h does
not include it. In GTK 3 it does, so we don't explicitly reinclude it
ourselves.
We now build cleanly in GTK2 with -DGTK_DISABLE_SINGLE_INCLUDES. (But
that doesn't say much, because we did already! Apparently gdkkeysyms.h
was a special case which that #define didn't forbid.)
Simon Tatham [Sat, 8 Aug 2015 14:04:28 +0000 (15:04 +0100)]
Make gtkask.c compile under GTK 1.
This is less than ideal - passphrase input now happens in ISO 8859-1,
and the passphrase prompt window is neither centred nor always-on-top.
But it basically works, and restores bare-minimum GTK 1 support to the
codebase as a whole.
Simon Tatham [Sat, 8 Aug 2015 12:35:44 +0000 (13:35 +0100)]
New formatting directive in logfile naming: &P for port number.
Users have requested this from time to time, for distinguishing log
file names when there's more than one SSH server running on different
ports of the same host. Since we do take account of that possibility
in other areas (e.g. we cache host keys indexed by (host,port) rather
than just host), it doesn't seem unreasonable to do so here too.
Simon Tatham [Thu, 6 Aug 2015 18:25:56 +0000 (19:25 +0100)]
Work around a failure in Windows 10 jump lists.
We've had several reports that launching saved sessions from the
Windows 10 jump list fails; Changyu Li reports that this is because we
create those IShellLink objects with a command line string starting
with @, and in Windows 10 that causes the SetArguments method to
silently do the wrong thing.
Simon Tatham [Wed, 5 Aug 2015 17:44:37 +0000 (18:44 +0100)]
New 'contrib' script to sort out email-corrupted packet logs.
If a PuTTY SSH packet log has gone through line-wrapping at 72
columns, destroying the long lines of the packet hex dumps, then this
script will reconstitute it as best it can, by reconstructing the
ASCII section at the end of the dump from the (hopefully) undamaged
hex part, and using that to spot wrapped lines and remove the
subsequent debris.
Simon Tatham [Sat, 1 Aug 2015 21:11:16 +0000 (22:11 +0100)]
Don't try to load GSSAPI libs unless we'll use them.
A user reports that in a particular situation one of the calls to
LoadLibrary from wingss.c has unwanted side effects, and points out
that this happens even when the saved session has GSSAPI disabled. So
I've evaluated as much as possible of the condition under which we
check the results of GSS library loading, and deferred the library
loading itself until after that condition says we even care about the
results.
Simon Tatham [Tue, 28 Jul 2015 17:49:23 +0000 (18:49 +0100)]
Fix braino in gtkask.c loop conditions.
If you're counting up to ms_limit in steps of ms_step, it's silly to
add ms_step at the end of the loop body _and_ increment the loop
variable by 1 in the loop header. I must have been half asleep.
Simon Tatham [Mon, 27 Jul 2015 19:06:02 +0000 (20:06 +0100)]
Handle the VK_PACKET virtual key code.
This is generated in response to the SendInput() Windows API call, if
that in turn is passed an KEYBDINPUT structure with KEYEVENTF_UNICODE
set. That method of input generation is used by programs such as
'WinCompose' to send an arbitrary Unicode character as if it had been
typed at the keyboard, even if the keyboard doesn't actually provide a
key for it.
Like VK_PROCESSKEY, this key code is an exception to our usual policy
of manually translating keystrokes: we handle it by calling
TranslateMessage, to get back the Unicode character it contains as a
WM_CHAR message.
(If that Unicode character in turn is outside the BMP, it may come
back as a pair of WM_CHARs in succession containing UTF-16 surrogates;
if so, that's OK, because the new Unicode WM_CHAR handler can cope.)
Simon Tatham [Mon, 27 Jul 2015 19:06:02 +0000 (20:06 +0100)]
Turn the Windows PuTTY main window into a Unicode window.
This causes WM_CHAR messages sent to us to have a wParam containing a
16-bit value encoded in UTF-16, rather than an 8-bit value encoded in
the system code page.
As far as I can tell, there aren't many other knock-on effects - e.g.
you can still interact with the window using ordinary char-based API
functions such as SetWindowText, and the Windows API will do the
necessary conversions behind the scenes. However, even so, I'm half
expecting some sort of unforeseen bug to show up as a result of this.
Simon Tatham [Mon, 27 Jul 2015 19:06:02 +0000 (20:06 +0100)]
New centralised helper function dup_mb_to_wc().
PuTTY's main mb_to_wc() function is all very well for embedding in
fiddly data pipelines, but for the simple job of turning a C string
into a C wide string, really I want something much more like
dupprintf. So here is one.
I've had to put it in a new separate source file miscucs.c rather than
throwing it into misc.c, because misc.c is linked into tools that
don't also include a module providing the internal Unicode API (winucs
or uxucs). The new miscucs.c appears only in Unicode-using tools.
Simon Tatham [Sat, 25 Jul 2015 10:28:32 +0000 (11:28 +0100)]
Post-0.65 release checklist updates.
The -F option is no longer needed to bob in this situation; that
hasn't been the directory I keep release announcements in for a long
time; the Docs page needs adjusting for pre-release retirement as well
as the Downloads page.
Simon Tatham [Sat, 25 Jul 2015 10:07:38 +0000 (11:07 +0100)]
Add a commentary assertion in config dialog setup.
Coverity complained that some paths through the loop in the
WM_INITDIALOG handler might leave firstpath==NULL. In fact this can't
happen because the input data to that loop is largely static and we
know what it looks like, but it doesn't seem unreasonable to add an
assertion anyway, to keep static checkers happy and as an explanatory
quasi-comment for humans.
Ben Harris [Wed, 24 Jun 2015 20:58:11 +0000 (21:58 +0100)]
When encrypting packet length with ChaCha20, treat sequence number as 32 bits.
While ChaCha20 takes a 64-bit nonce, SSH-2 defines the message
sequence number to wrap at 2^32 and OpenSSH stores it in a u_int32_t,
so the upper 32 bits should always be zero. PuTTY was getting this
wrong, and either using an incorrect nonce or causing GCC to complain
about an invalid shift, depending on the size of "unsigned long". Now
I think it gets it right.
Simon Tatham [Mon, 22 Jun 2015 18:43:22 +0000 (19:43 +0100)]
Inaugural merge from branch 'pre-0.65'.
This is a null merge (done with '-s ours'), not changing the code on
master at all: it just adds an ancestor relationship so that any fresh
commits on pre-0.65 can be cleanly merged to here in the obvious way.
Simon Tatham [Mon, 22 Jun 2015 18:37:27 +0000 (19:37 +0100)]
Fix a crash when connection-sharing during userauth.
If a sharing downstream disconnected while we were still in userauth
(probably by deliberate user action, since such a downstream would
have just been sitting there waiting for upstream to be ready for it)
then we could crash by attempting to count234(ssh->channels) before
the ssh->channels tree had been set up in the first place.
A simple null-pointer check fixes it. Thanks to Antti Seppanen for the
report.
Simon Tatham [Mon, 22 Jun 2015 18:36:57 +0000 (19:36 +0100)]
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.
Simon Tatham [Thu, 18 Jun 2015 06:05:19 +0000 (07:05 +0100)]
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.
Simon Tatham [Sat, 13 Jun 2015 14:22:03 +0000 (15:22 +0100)]
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-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.
Simon Tatham [Mon, 8 Jun 2015 18:24:58 +0000 (19:24 +0100)]
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-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.
Simon Tatham [Mon, 8 Jun 2015 18:23:48 +0000 (19:23 +0100)]
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-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.
Simon Tatham [Sun, 7 Jun 2015 20:14:09 +0000 (21:14 +0100)]
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.
Simon Tatham [Sun, 7 Jun 2015 20:09:41 +0000 (21:09 +0100)]
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.)
Simon Tatham [Sat, 6 Jun 2015 13:52:29 +0000 (14:52 +0100)]
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.
Simon Tatham [Thu, 28 May 2015 17:14:14 +0000 (18:14 +0100)]
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.
Simon Tatham [Mon, 18 May 2015 20:17:21 +0000 (21:17 +0100)]
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!
Simon Tatham [Mon, 18 May 2015 12:57:45 +0000 (13:57 +0100)]
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-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.
Simon Tatham [Fri, 8 May 2015 18:04:16 +0000 (19:04 +0100)]
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.
Simon Tatham [Fri, 8 May 2015 17:57:18 +0000 (18:57 +0100)]
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.
Simon Tatham [Mon, 27 Apr 2015 19:48:29 +0000 (20:48 +0100)]
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.
Simon Tatham [Mon, 27 Apr 2015 05:54:21 +0000 (06:54 +0100)]
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-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.
Simon Tatham [Sun, 26 Apr 2015 22:31:11 +0000 (23:31 +0100)]
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-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.
Simon Tatham [Sat, 25 Apr 2015 09:46:53 +0000 (10:46 +0100)]
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.
Simon Tatham [Tue, 7 Apr 2015 21:17:08 +0000 (22:17 +0100)]
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.
Simon Tatham [Tue, 7 Apr 2015 20:54:41 +0000 (21:54 +0100)]
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.
Simon Tatham [Sat, 7 Mar 2015 20:57:26 +0000 (20:57 +0000)]
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.
Simon Tatham [Sat, 7 Mar 2015 17:10:36 +0000 (17:10 +0000)]
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.
Simon Tatham [Sat, 28 Feb 2015 12:04:54 +0000 (12:04 +0000)]
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.
Simon Tatham [Sat, 28 Feb 2015 07:58:29 +0000 (07:58 +0000)]
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.
Simon Tatham [Sat, 7 Feb 2015 11:48:49 +0000 (11:48 +0000)]
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-picker's notes: this apparently pointless commit is required on
this branch because it's a dependency of the rather less pointless 9fec2e773873e28f1409f5e1eefaf03483070050.
Simon Tatham [Thu, 18 Jun 2015 06:05:19 +0000 (07:05 +0100)]
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.
Simon Tatham [Sat, 13 Jun 2015 14:22:03 +0000 (15:22 +0100)]
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.
Simon Tatham [Mon, 8 Jun 2015 18:24:58 +0000 (19:24 +0100)]
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.
Simon Tatham [Mon, 8 Jun 2015 18:23:48 +0000 (19:23 +0100)]
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.
Simon Tatham [Sun, 7 Jun 2015 20:14:09 +0000 (21:14 +0100)]
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.
Simon Tatham [Sun, 7 Jun 2015 20:09:41 +0000 (21:09 +0100)]
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.)
Simon Tatham [Sun, 7 Jun 2015 11:26:26 +0000 (12:26 +0100)]
Dedicated routines for poly1305 arithmetic.
Rather than doing arithmetic mod 2^130-5 using the general-purpose
Bignum library, which requires lots of mallocs and frees per operation
and also uses a general-purpose divide routine for each modular
reduction, we now have some dedicated routines in sshccp.c to do
arithmetic mod 2^130-5 in a more efficient way, and hopefully also
with data-independent performance.
Because PuTTY's target platforms don't all use the same size of bignum
component, I've arranged to auto-generate the arithmetic functions
using a Python script living in the 'contrib' directory. As and when
we need to support an extra BignumInt size, that script should still
be around to re-run with different arguments.
Simon Tatham [Sat, 6 Jun 2015 13:52:29 +0000 (14:52 +0100)]
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.
Simon Tatham [Sun, 7 Jun 2015 11:46:33 +0000 (12:46 +0100)]
Make log messages look slightly nicer.
I'd rather see the cipher and MAC named separately, with a hint that
the two are linked together in some way, than see the cipher called by
a name including the MAC and the MAC init message have an ugly
'<implicit>' in it.
Ben Harris [Sat, 30 May 2015 08:10:48 +0000 (09:10 +0100)]
Add a common function to add an algorithm to KEXINIT.
This allows for sharing a bit of code, and it also means that
deduplication of KEXINIT algorithms can be done while working out the
list of algorithms rather than when constructing the KEXINIT packet
itself.
Ben Harris [Fri, 29 May 2015 21:40:50 +0000 (22:40 +0100)]
Add have_ssh_host_key() and use it to influence algorithm selection.
The general plan is that if PuTTY knows a host key for a server, it
should preferentially ask for the same type of key so that there's some
chance of actually getting the same key again. This should mean that
when a server (or PuTTY) adds a new host key type, PuTTY doesn't
gratuitously switch to that key type and then warn the user about an
unrecognised key.
Simon Tatham [Thu, 28 May 2015 17:14:14 +0000 (18:14 +0100)]
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.
Simon Tatham [Tue, 19 May 2015 08:56:56 +0000 (09:56 +0100)]
Add a reference to a spec for Curve25519.
It doesn't seem to be all that good a spec, in that it seems to be
specified in terms of functions in libssh and hence based on the
assumption that you already know exactly what those functions do. But
it's something, at least.
Simon Tatham [Tue, 19 May 2015 08:54:17 +0000 (09:54 +0100)]
Fix construction of the output bignum in Curve25519 kex.
We were doing an endianness flip on the output elliptic-curve point.
Endianness flips of bignums, of course, have to specify how many bytes
they're imagining the value to have (that's how you decide whether to
convert 0xA1A2 into 0xA2A1 or 0xA2A10000 or 0xA2A1000000000000 etc),
and we had chosen our byte count based on the highest set bit in the
_output value_ - but in fact we should have chosen it based on the
size of the curve's modulus, leading to a failure about 1/256 of the
time when the MSB happened to come out zero so the two byte counts
differed.
(Also added a missing smemclr, while I was there.)
Simon Tatham [Tue, 19 May 2015 07:42:23 +0000 (08:42 +0100)]
Log which elliptic curve we're using for ECDH kex.
It seems like quite an important thing to mention in the event log!
Suppose there's a bug affecting only one curve, for example? Fixed-
group Diffie-Hellman has always logged the group, but the ECDH log
message just told you the hash and not also the curve.
To implement this, I've added a 'textname' field to all elliptic
curves, whether they're used for kex or signing or both, suitable for
use in this log message and any others we might find a need for in
future.
Simon Tatham [Mon, 18 May 2015 20:17:21 +0000 (21:17 +0100)]
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!
Simon Tatham [Mon, 18 May 2015 12:57:45 +0000 (13:57 +0100)]
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.
Simon Tatham [Sun, 17 May 2015 15:40:36 +0000 (16:40 +0100)]
askpass: don't treat releases of Ret or Esc as presses.
Caused an embarrassing failure just now trying to run the test program
from a command prompt - I had Return still held down by the time it
started up, and my release of it immediately terminated input :-)
Ben Harris [Sun, 17 May 2015 09:53:27 +0000 (10:53 +0100)]
Restructure KEXINIT generation and parsing.
The new code remembers the contents and meaning of the outgoing KEXINIT
and uses this to drive the algorithm negotiation, rather than trying to
reconstruct what the outgoing KEXINIT probably said. This removes the
need to maintain the KEXINIT generation and parsing code precisely in
parallel.
It also fixes a bug whereby PuTTY would have selected the wrong host key
type in cases where the server gained a host key type between rekeys.
Simon Tatham [Fri, 15 May 2015 13:01:35 +0000 (14:01 +0100)]
Fix mpint signedness bug in importing PEM ECDSA keys.
The OpenSSH PEM format contains a big integer with the top bit
potentially set, which we handle by copying the data into a faked up
instance of our own private key format, and passing that to
ecdsa_createkey(). But our own private key format expects an SSH-2
standard mpint, i.e. with the top bit reliably clear, so this might
fail for no good reason.
Fixed by prefixing a zero byte unconditionally when constructing the
fake private blob.
Simon Tatham [Fri, 15 May 2015 12:27:15 +0000 (13:27 +0100)]
Remove pointless NULL checks in the ECC code.
snew(), and most of the bignum functions, are deliberately written to
fail an assertion and terminate the program rather than return NULL,
so there's no point carefully checking their every return value for
NULL. This removes a huge amount of pointless error-checking code, and
makes the elliptic curve arithmetic almost legible in places :-)
I've kept error checks after modinv(), because that can return NULL if
asked to invert zero. bigsub() can also fail in principle, because our
bignums are non-negative only, but in the couple of cases where it's
used there's a preceding compare that should prevent it, so I've just
added assertions.
Simon Tatham [Fri, 15 May 2015 12:01:33 +0000 (13:01 +0100)]
Windows PuTTYgen: fix mis-setting of radio buttons.
The menu options and radio buttons for key type were not consistently
setting each other when selected: in particular, selecting from the
menu did not cause the ED25519 radio button to be either set or unset
when that would have been appropriate.
Looks as if I failed to catch in code review the fact that we should
have _one_ call to each of CheckRadioButton and CheckMenuRadioItem,
and they should both have the right 'first' and 'last' parameters
Simon Tatham [Fri, 15 May 2015 10:15:42 +0000 (11:15 +0100)]
Giant const-correctness patch of doom!
Having found a lot of unfixed constness issues in recent development,
I thought perhaps it was time to get proactive, so I compiled the
whole codebase with -Wwrite-strings. That turned up a huge load of
const problems, which I've fixed in this commit: the Unix build now
goes cleanly through with -Wwrite-strings, and the Windows build is as
close as I could get it (there are some lingering issues due to
occasional Windows API functions like AcquireCredentialsHandle not
having the right constness).
Notable fallout beyond the purely mechanical changing of types:
- the stuff saved by cmdline_save_param() is now explicitly
dupstr()ed, and freed in cmdline_run_saved.
- I couldn't make both string arguments to cmdline_process_param()
const, because it intentionally writes to one of them in the case
where it's the argument to -pw (in the vain hope of being at least
slightly friendly to 'ps'), so elsewhere I had to temporarily
dupstr() something for the sake of passing it to that function
- I had to invent a silly parallel version of const_cmp() so I could
pass const string literals in to lookup functions.
- stripslashes() in pscp.c and psftp.c has the annoying strchr nature
Simon Tatham [Fri, 15 May 2015 09:13:06 +0000 (10:13 +0100)]
Clean up hash selection in ECDSA.
Removed another set of ad-hoc tests of the key size to decide which
hash to use for the signature system, and replaced them with a
straightforward pointer to an ssh_hash structure in the 'extra' area.
Simon Tatham [Fri, 15 May 2015 09:13:05 +0000 (10:13 +0100)]
Clean up elliptic curve selection and naming.
The ec_name_to_curve and ec_curve_to_name functions shouldn't really
have had to exist at all: whenever any part of the PuTTY codebase
starts using sshecc.c, it's starting from an ssh_signkey or ssh_kex
pointer already found by some other means. So if we make sure not to
lose that pointer, we should never need to do any string-based lookups
to find the curve we want, and conversely, when we need to know the
name of our curve or our algorithm, we should be able to look it up as
a straightforward const char * starting from the algorithm pointer.
This commit cleans things up so that that is indeed what happens. The
ssh_signkey and ssh_kex structures defined in sshecc.c now have
'extra' fields containing pointers to all the necessary stuff;
ec_name_to_curve and ec_curve_to_name have been completely removed;
struct ec_curve has a string field giving the curve's name (but only
for those curves which _have_ a name exposed in the wire protocol,
i.e. the three NIST ones); struct ec_key keeps a pointer to the
ssh_signkey it started from, and uses that to remember the algorithm
name rather than reconstructing it from the curve. And I think I've
got rid of all the ad-hockery scattered around the code that switches
on curve->fieldBits or manually constructs curve names using stuff
like sprintf("nistp%d"); the only remaining switch on fieldBits
(necessary because that's the UI for choosing a curve in PuTTYgen) is
at least centralised into one place in sshecc.c.
One user-visible result is that the format of ed25519 host keys in the
registry has changed: there's now no curve name prefix on them,
because I think it's not really right to make up a name to use. So any
early adopters who've been using snapshot PuTTY in the last week will
be inconvenienced; sorry about that.
Simon Tatham [Fri, 15 May 2015 09:12:08 +0000 (10:12 +0100)]
Provide an 'extra' pointer in ssh_signkey and ssh_kex.
This gives families of public key and kex functions (by which I mean
those sharing a set of methods) a place to store parameters that allow
the methods to vary depending on which exact algorithm is in use.
The ssh_kex structure already had a set of parameters specific to
Diffie-Hellman key exchange; I've moved those into sshdh.c and made
them part of the 'extra' structure for that family only, so that
unrelated kex methods don't have to faff about saying NULL,NULL,0,0.
(This required me to write an extra accessor function for ssh.c to ask
whether a DH method was group-exchange style or fixed-group style, but
that doesn't seem too silly.)