Previously, the code to recover and memory-map the file-mapping object
Pageant uses for its IPC, and the code to convey its contents to and
from the cross-platform agent code, were widely separated, with the
former living in the WM_COPYDATA handler in the window procedure, and
the latter in answer_msg.
Now all of that code lives in answer_filemapping_message; WndProc only
handles the _window message_ contents - i.e. ensures the WM_COPYDATA
message has the right dwData id and that its lpData contains an ASCIZ
string - and answer_filemapping_message goes all the way from that
file-mapping object name to calling pageant_handle_msg.
While I'm here, I've also tidied up the code so that it uses the 'goto
cleanup' idiom rather than nesting everything inconveniently deeply,
and arranged that if anything goes wrong then we at least _construct_
an error message (although as yet we don't use that for anything
unless we're compiled with DEBUG_IPC enabled).
On the post-userauth rekey, when we're specifically rekeying in order
to turn on delayed compression, we shouldn't write the Event Log
"Server supports delayed compression; will try this later" message
that we did in the original key exchange. At this point, it _is_
later, and we're about to turn on compression right now!
The new SSH-2 BPP has two functions ssh2_bpp_new_outgoing_crypto and
ssh2_bpp_new_incoming_crypto, which (due to general symmetry) are
_almost_ identical, except that the code that sets up the compression
context in the two directions has to call compress_init in the former
and decompress_init in the latter.
Except that it called compress_init in both, so compression in SSH-2
has been completely broken for a week. How embarrassing. I _remember_
thinking that I'd better make sure to change that one call between the
two, but apparently it fell out of my head before I committed.
When the whole SSH connection is throttled and then unthrottled, we
need to requeue the callback that transfers data to the Socket from
the new outgoing_data queue introduced in commit 9e3522a97.
The user-visible effect of this missing call was that outgoing SFTP
transactions would lock up, because in SFTP mode we enable the
"simple@putty.projects.tartarus.org" mode and essentially turn off the
per-channel window management, so throttling of the whole connection
becomes the main source of back-pressure.
Thanks to Alex Landau for pointing out that commit 8b98fea4a
introduced two uses of it with the arguments one way round and one
with them the other way round. (Plus a fourth use where it doesn't
matter, because the padding at the end of the encrypted blob of an
OpenSSH PEM private key consists of n bytes with value n. :-)
On the basis of majority vote, I've switched the order in the function
definition to match the two of the three call sites that expressed the
same opinion, and fixed the third.
Now we have the new marshalling system, I think it's outlived its
usefulness, because the new system allows us to directly express
various things (e.g. uint16 and non-zero-terminated strings) that were
actually _more_ awkward to do via the variadic interface. So here's a
rewrite that removes send_packet(), and replaces all its call sites
with something that matches our SSH-2 packet construction idioms.
This diff actually _reduces_ the number of lines of code in ssh.c.
Since the variadic system was trying to save code by centralising
things, that seems like the best possible evidence that it wasn't
pulling its weight!
sshbpp.h now defines a classoid that encapsulates both directions of
an SSH binary packet protocol - that is, a system for reading a
bufchain of incoming data and turning it into a stream of PktIn, and
another system for taking a PktOut and turning it into data on an
outgoing bufchain.
The state structure in each of those files contains everything that
used to be in the 'rdpkt2_state' structure and its friends, and also
quite a lot of bits and pieces like cipher and MAC states that used to
live in the main Ssh structure.
One minor effect of this layer separation is that I've had to extend
the packet dispatch table by one, because the BPP layer can no longer
directly trigger sending of SSH_MSG_UNIMPLEMENTED for a message too
short to have a type byte. Instead, I extend the PktIn type field to
use an out-of-range value to encode that, and the easiest way to make
that trigger an UNIMPLEMENTED message is to have the dispatch table
contain an entry for it.
(That's a system that may come in useful again - I was also wondering
about inventing a fake type code to indicate network EOF, so that that
could be propagated through the layers and be handled by whichever one
currently knew best how to respond.)
I've also moved the packet-censoring code into its own pair of files,
partly because I was going to want to do that anyway sooner or later,
and mostly because it's called from the BPP code, and the SSH-2
version in particular has to be called from both the main SSH-2 BPP
and the bare unencrypted protocol used for connection sharing. While I
was at it, I took the opportunity to merge the outgoing and incoming
censor functions, so that the parts that were common between them
(e.g. CHANNEL_DATA messages look the same in both directions) didn't
need to be repeated.
This mirrors the use of one for incoming wire data: now when we send
raw data (be it the initial greeting, or the output of binary packet
construction), we put it on ssh->outgoing_data, and schedule a
callback to transfer that into the socket.
Partly this is in preparation for delegating the task of appending to
that bufchain to a separate self-contained module that won't have
direct access to the connection's Socket. But also, it has the very
nice feature that I get to throw away the ssh_pkt_defer system
completely! That was there so that we could construct more than one
packet in rapid succession, concatenate them into a single blob, and
pass that blob to the socket in one go so that the TCP headers
couldn't contain any trace of where the boundary between them was. But
now we don't need a separate function to do that: calling the ordinary
packet-send routine twice in the same function before returning to the
main event loop will have that effect _anyway_.
ssh.c has been an unmanageably huge monolith of a source file for too
long, and it's finally time I started breaking it up into smaller
pieces. The first step is to move some declarations - basic types like
packets and packet queues, standard constants, enums, and the
coroutine system - into headers where other files can see them.
It is to put_data what memset is to memcpy. Several places
in the code wanted it already, but not _quite_ enough for me to
have written it with the rest of the BinarySink infrastructure
originally.
This saves a malloc and free every time we add or remove a packet from
a packet queue - it can now be done by pure pointer-shuffling instead
of allocating a separate list node structure.
The body pointer was used after encryption to mark the start of the
fully wire-ready packet by ssh2_pkt_construct, and before encryption
by the log_outgoing_packet functions. Now the former returns a nice
modern ptrlen (it never really needed to store the pointer _in_ the
packet structure anyway), and the latter uses an integer 'prefix'
field, which isn't very different in concept but saves effort on
reallocs.
These were only used in the rdpkt coroutines, during construction of
the incoming packet; once it's complete, they're never touched again.
So really they should have been fields in the rdpkt coroutines' state
- and now they are.
The new memory allocation strategy for incoming packets is to defer
creation of the returned pktin structure until we know how big its
data buffer will really need to be, and then use snew_plus to make the
PktIn and the payload block in the same allocation.
When we have to read and keep some amount of the packet before
allocating the returned structure, we do it by having a persistent
buffer in the rdpkt state, which is retained for the whole connection
and only freed once in ssh_free.
They were duplicating values stored in the BinarySource substructure.
Mostly they're not referred to directly any more (instead, we call
get_foo to access the BinarySource); and when they are, we can switch
to reading the same values back out of the BinarySource anyway.
This is the first stage of massively tidying up this very confused
data structure. In this commit, I replace the unified 'struct Packet'
with two structures PktIn and PktOut, each of which contains only the
fields of struct Packet that are actually used for packets going in
that direction - most notably, PktIn doesn't implement BinarySink, and
PktOut doesn't implement BinarySource.
All uses of the old structure were statically determinable to be one
or the other, so I've done that determination and changed all the
types of variables and function signatures.
Unlike PktIn, PktOut is not reference-counted, so there's a new
ssh_pktout_free function.
The most immediately pleasing thing about this change is that it lets
me finally get rid of the tedious comment explaining how the 'length'
field in struct Packet meant something different depending on
direction. Now it's two fields of the same name in two different
structures, I can comment the same thing much less verbosely!
(I've also got rid of the comment claiming the type field was only
used for incoming packets. That wasn't even true! It might have been
once, because you can write an outgoing packet's type byte straight
into its data buffer, but in fact in the current code pktout->type is
nonetheless used in various other places, e.g. log_outgoing_packet.)
In this commit I've only removed the fields from each structure that
were _already_ unused. There are still quite a few we can get rid of
by _making_ them unused.
It was horrible - even if harmless in practice - that it wrote the
NATed channel id over its input buffer, and I think it's worth the
extra memory management to avoid doing that.
This formalises my occasional habit of using a single malloc to make a
block that contains a header structure and a data buffer that a field
of the structure will point to, allowing it to be freed in one go
later. Previously I had to do this by hand, losing the type-checking
advantages of snew; now I've written an snew-style macro to do the
job, plus an accessor macro to cleanly get the auxiliary buffer
pointer afterwards, and switched existing instances of the pattern
over to using that.
I reset this to a very small value during testing, because my real
.Xauthority file is not absurdly enormous, so this was the easiest way
to check the algorithm that periodically moves everything up the
buffer.
Then that test found and fixed a bug, and of course my temporary test
value of MAX_RECORD_SIZE got swept up in the 'git commit -a --amend',
and pushed with the rest of the refactoring, and I didn't notice until
today.
There's always one - I did everything else in the build script, but
forgot to arrange for the wa32 and wa64 output subdirs to have a
.htaccess redirect from a fixed name like 'putty-arm64-installer.msi'
to whatever the real file name is in that particular build.
I've been playing around with GDB's Python scripting system recently,
and this is a thing I've always thought it would be nice to be able to
do: if you load this script (which, on Ubuntu 18.04's gdb, is as
simple as 'source contrib/gdb.py' at the gdb prompt, or similar), then
variables of type Bignum will be printed as (e.g.) 'Bignum(0x12345)',
or 'Bignum(NULL)' if they're null pointers, or a fallback
representation if they're non-null pointers but gdb can't read
anything sensible from them.
A few variables that gcc couldn't tell I'd initialised on all the
important paths, a variable that didn't really need to be there
anyway, and yet another use of GET_WINDOWS_FUNCTION_NO_TYPECHECK.
It's a key type that format doesn't know how to handle, but that's no
excuse to fail an assertion - we have a perfectly good failure code we
can return from the export function, so we should use it.
Rather than squelching the warning, I'm actually paying attention to
the deprecation, in that I'm allowing for the possibility that the
function might stop existing or stop returning success.
The specific thing that's strange about it is that it's _not_ an error
even though the compiler is quite justified in being suspicious about
it! The MS APIs define two different structures to have identical
formats.
The general wisdom these days - in particular as given by the Linux
urandom(4) man page - seems to be that there's no need to use the
blocking /dev/random any more unless you're running at very early boot
time when the system random pool is at serious risk of not having any
entropy in it at all.
In case of non-Linux systems that don't think /dev/urandom is a
standard name, I fall back to /dev/random if /dev/urandom can't be
found.
After Pavel Kryukov pointed out that I have to put _something_ in the
'ssh_key' structure, I thought of an actually useful thing to put
there: why not make it store a pointer to the ssh_keyalg structure?
Then ssh_key becomes a classoid - or perhaps 'traitoid' is a closer
analogy - in the same style as Socket and Plug. And just like Socket
and Plug, I've also arranged a system of wrapper macros that avoid the
need to mention the 'object' whose method you're invoking twice at
each call site.
The new vtable pointer directly replaces an existing field of struct
ec_key (which was usable by several different ssh_keyalgs, so it
already had to store a pointer to the currently active one), and also
replaces the 'alg' field of the ssh2_userkey structure that wraps up a
cryptographic key with its comment field.
I've also taken the opportunity to clean things up a bit in general:
most of the methods now have new and clearer names (e.g. you'd never
know that 'newkey' made a public-only key while 'createkey' made a
public+private key pair unless you went and looked it up, but now
they're called 'new_pub' and 'new_priv' you might be in with a
chance), and I've completely removed the openssh_private_npieces field
after realising that it was duplicating information that is actually
_more_ conveniently obtained by calling the new_priv_openssh method
(formerly openssh_createkey) and throwing away the result.
This parameter returned a substring of the input, which was used for
two purposes. Firstly, it was used to hash the host and server keys
during the initial SSH-1 key setup phase; secondly, it was used to
check the keys in Pageant against the public key blob of a key
specified on the command line.
Unfortunately, those two purposes didn't agree! The first one needs
just the bare key modulus bytes (without even the SSH-1 mpint length
header); the second needs the entire key blob. So, actually, it seems
to have never worked in SSH-1 to say 'putty -i keyfile' and have PuTTY
find that key in Pageant and not have to ask for the passphrase to
decrypt the version on disk.
Fixed by removing that parameter completely, which simplifies all the
_other_ call sites, and replacing it by custom code in those two
places that each does the actually right thing.
It's an SSH-1 specific function, so it should have a name reflecting
that, and it didn't. Also it had one of those outdated APIs involving
passing it a client-allocated buffer and size. Now it has a sensible
name, and internally it constructs the output string using a strbuf
and returns it dynamically allocated.
Another piece of half-finished machinery that I can't have tested
properly when I set up connection sharing: I had the function
ssh_alloc_sharing_rportfwd which is how sshshare.c asks ssh.c to start
sending it channel-open requests for a given remote forwarded port,
but I had no companion function that removes one of those requests
again when a downstream remote port forwarding goes away (either by
mid-session cancel-tcpip-forward or by the whole downstream
disconnecting).
As a result, the _second_ attempt to set up the same remote port
forwarding, after a sharing downstream had done so once and then
stopped, would quietly fail.
This is another bug that must have been around since connection
sharing was introduced, and nobody noticed until I did some unusually
thorough testing yesterday.
When a sharing downstream asks to set up a remote port forwarding, we
pass through the "tcpip-forward" global request, and we also intercept
the reply so that we know that the forwarding has been set up (and
hence that we should be passing "forwarded-tcpip" channel opens for
that port to this downstream). To do that, we set the want-reply flag
in the version of the packet we pass to the server, even if it was
clear in downstream's version; and we also put an item on a queue
local to sshshare.c which reminds us what to do about the reply when
it comes back.
But when the downstream _cancels_ one of those forwardings, I wrote
the code for all parts of that process except adding that queue item.
I even wrote the code to _consume_ the queue item, but somehow I
completely forgot to generate one in the first place! So the enum
value GLOBREQ_CANCEL_TCPIP_FORWARD was declared, tested for, but never
actually assigned to anything.
This must have been a bug introduced during the SSH-2 connection
sharing rework. Apparently nobody's ever re-tested SSH-1 X forwarding
since then - until I did so yesterday in the course of testing my
enormous refactor of the packet unmarshalling code.
I must have introduced this bug yesterday when I rewrote the packet
censoring functions using BinarySource. The base pointer passed to
log_packet was pointing at the right place, but the accompanying
length was the gross rather than net one, as it were - it counted the
extra header data we're about to insert at the _start_ of the packet,
so log_packet() was trying to print that many extra bytes at the _end_
and overrunning its buffer.
If the server sends an SSH_MSG_DISCONNECT, then we call
connection_fatal(). But if the server closes the network connection,
then we call connection_fatal(). In situations where the former
happens, the latter happens too.
Currently, calling connection_fatal twice is especially bad on GTK
because all dialogs are now non-modal and an assertion fails in the
GTK front end when two fatal message boxes try to exist at the same
time (the register_dialog system finds that slot is already occupied).
But regardless of that, we'd rather not even _try_ to print two fatal
boxes, because even if the front end doesn't fail an assertion,
there's no guarantee that the _more useful_ one of the messages will
end up being displayed. So a better fix is to have ssh.c make a
sensible decision about which message is the helpful one - in this
case, the actual error message out of the SSH_MSG_DISCONNECT, rather
than the predictable fact of the connection having been slammed shut
immediately afterwards - and only pass that one to the front end in
the first place.
There are several old functions that the previous commits have removed
all, or nearly all, of the references to. match_ssh_id is superseded
by ptrlen_eq_string; get_ssh_{string,uint32} is yet another replicated
set of decode functions (this time _partly_ centralised into misc.c);
the old APIs for the SSH-1 RSA decode functions are gone (together
with their last couple of holdout clients), as are
ssh{1,2}_{read,write}_bignum and ssh{1,2}_bignum_length.
Particularly odd was the use of ssh1_{read,write}_bignum in the SSH-2
Diffie-Hellman implementation. I'd completely forgotten I did that!
Now replaced with a raw bignum_from_bytes, which is simpler anyway.
I've also replaced the entire SOCKS state machine whose states were
barely-documented literal integers with one that uses an actual enum.
I think the result is a great deal clearer.
In the course of this rewrite I noticed that PuTTY's dynamic port
forwarding had never got round to supporting the SOCKS5 IPv6 address
format - though there was a FIXME comment saying it ought to. So now
it does: if a SOCKS5 client provides a binary IPv6 address (which
PuTTY's _own_ SOCKS5 client, in proxy.c, is quite capable of doing!),
then that will be translated into the usual IPv6 hex literal
representation to put in the "direct-tcpip" channel open request.
Quite a few of the function pointers in the ssh_keyalg vtable now take
ptrlen arguments in place of separate pointer and length pairs.
Meanwhile, the various key types' implementations of those functions
now work by initialising a BinarySource with the input ptrlen and
using the new decode functions to walk along it.
One exception is the openssh_createkey method which reads a private
key in the wire format used by OpenSSH's SSH-2 agent protocol, which
has to consume a prefix of a larger data stream, and tell the caller
how much of that data was the private key. That function now takes an
actual BinarySource, and passes that directly to the decode functions,
so that on return the caller finds that the BinarySource's read
pointer has been advanced exactly past the private key.
This let me throw away _several_ reimplementations of mpint-reading
functions, one in each of sshrsa, sshdss.c and sshecc.c. Worse still,
they didn't all have exactly the SSH-2 semantics, because the thing in
sshrsa.c whose name suggested it was an mpint-reading function
actually tolerated the wrong number of leading zero bytes, which it
had to be able to do to cope with the "ssh-rsa" signature format which
contains a thing that isn't quite an SSH-2 mpint. Now that deviation
is clearly commented!
Another set of localised decoding routines get thrown away here. Also,
I've changed the APIs of a couple of helper functions in x11fwd.c to
take ptrlens in place of zero-terminated C strings, because that's the
format in which they come back from the decode, and it saves mallocing
a zero-terminated version of each one just to pass to those helpers.
The OpenSSH PEM reader is the most interesting conversion out of
these: it was using a standalone function called get_ber_id_len(),
which only skipped over the header of an ASN.1 BER data item and left
the current position at the start of the payload. That's been replaced
by a get_ber() function more in the spirit of the new API, which
consumes the entire BER element, returning its header details and also
a ptrlen pointing at its payload.
(That function could easily be promoted out of import.c to somewhere
more central, if we ever had a need to handle ASN.1 on a larger scale
- e.g. X.509 certificates would find the same function useful. For the
moment, though, it can stay where it is.)
Other than that, this is a fairly mechanical API translation.
Like the corresponding rewrite of conf serialisation, this affects not
just conf_deserialise itself but also the per-platform filename and
fontspec deserialisers.
