While I'm at it, I've brought it all into a single function: the
parsing of data from Conf, the list of modes, and even the old
callback system for writing to the destination buffer is now a simple
if statement that formats mode parameters as byte or uint32 depending
on SSH version. Also, the terminal speeds and the end byte are part of
the same setup, so it's all together in one place instead of scattered
all over ssh.c.
It doesn't really have to be in ssh.c sharing that file's internal
data structures; it's as much an independent object implementation as
any of the less trivial Channel instances. So it's another thing we
can get out of that too-large source file.
It's really just a concatenator for a pair of linked lists, but
unhelpfully restricted in which of the lists it replaces with the
output. Better to have a three-argument function that puts the output
wherever you like, whether it overlaps either or neither one of the
inputs.
We used it to suppress reading from the network at every point during
protocol setup where PuTTY was waiting for a user response to a dialog
box (e.g. a host key warning). The purpose of this was to avoid
dropping an important packet while the coroutine was listening to one
of its other input parameters (as it were). But now everything is
queue-based, packets will stay queued until we're ready to look at
them anyway; so it's better _not_ to freeze the connection, so that
messages we _can_ handle in between (e.g. SSH_MSG_DEBUG or
SSH_MSG_IGNORE) can still be processed.
That dispenses with all uses of ssh_set_frozen except for its use by
ssh_throttle_conn to exert back-pressure on the server in SSH1 which
doesn't have per-channel windows. So I've moved that last use _into_
ssh_throttle_conn, and now the function is completely gone.
These are essentially data-structure maintenance, and it seems silly
to have them be part of the same file that manages the topmost
structure of the SSH connection.
Some upcoming restructuring I've got planned will need to pass output
packets back and forth on queues, as well as input ones. So here's a
change that arranges that we can have a PktInQueue and a PktOutQueue,
sharing most of their implementation via a PacketQueueBase structure
which links together the PacketQueueNode fields in the two packet
structures.
There's a tricksy bit of macro manoeuvring to get all of this
type-checked, so that I can't accidentally link a PktOut on to a
PktInQueue or vice versa. It works by having the main queue functions
wrapped by macros; when receiving a packet structure on input, they
type-check it against the queue structure and then automatically look
up its qnode field to pass to the underlying PacketQueueBase function;
on output, they translate a returned PacketQueueNode back to its
containing packet type by calling a 'get' function pointer.
Now there's a centralised routine in misc.c to do the sanitisation,
which copies data on to an outgoing bufchain. This allows me to remove
from_backend_untrusted() completely from the frontend API, simplifying
code in several places.
Two use cases for untrusted-terminal-data sanitisation were in the
terminal.c prompts handler, and in the collection of SSH-2 userauth
banners. Both of those were writing output to a bufchain anyway, so
it was very convenient to just replace a bufchain_add with
sanitise_term_data and then not have to worry about it again.
There was also a simplistic sanitiser in uxcons.c, which I've now
replaced with a call to the good one - and in wincons.c there was a
FIXME saying I ought to get round to that, which now I have!
Getting it out of the overgrown ssh.c is worthwhile in itself! But
there are other benefits of this reorganisation too.
One is that I get to remove ssh->current_incoming_data_fn, because now
_all_ incoming network data is handled by whatever the current BPP is.
So now we only indirect through the BPP, not through some other
preliminary function pointer _and_ the BPP.
Another is that all _outgoing_ network data is now handled centrally,
including our outgoing version string - which means that a hex dump of
that string now shows up in the raw-data log file, from which it was
previously conspicuous by its absence.
I'm quite surprised I haven't needed this for anything else yet. I
suppose if I had it, I could have written most of my ptrlen_eq_strings
in terms of it, and saved a lot of gratuitous runtime strlens.
This should make it easier to do formatted-string based logging
outside ssh.c, because I can wrap up a local macro in any source file
I like that expands to logevent_and_free(wherever my Frontend is,
dupprintf(macro argument)).
It caused yet another stub function to be needed in testbn, but there
we go.
(Also, while I'm here, removed a redundant declaration of logevent
itself from ssh.h. The one in putty.h is all we need.)
This replaces the previous log(n)^2 algorithm for channel-number
allocation, which binary-searched the space of tree indices using a
log-time call to index234() at each step, with a single log-time pass
down the tree which only has to check the returned channel number
against the returned tree index at each step.
I'm under no illusions that this was a critical performance issue, but
it's been offending my sense of algorithmic elegance for a while.
This is a thing I've been meaning to set up for a while: it's a
pull-based search system (that is, the caller takes each step of the
search manually, rather than providing a callback), which lets the
caller inspect every step of the search, including the index of each
candidate element in the tree. This allows flexible kinds of query
that play the element and its index off against each other.
I've also rewritten the existing findrelpos234() search function using
the new one as a primitive, because that simplifies it a lot!
Added a missing #include of string.h; arranged that the test program
reports the number of errors detected at the end so I don't have to
search its entire output for "ERROR"; made the test program return a
nonzero exit status if any error was found.
This is a vtable that wraps up all the functionality required from the
SSH connection layer by associated modules like port forwarding and
connection sharing. This extra layer of indirection adds nothing
useful right now, but when I later separate the SSH-1 and SSH-2
connection layer implementations, it will be convenient for each one
to be able to implement this vtable in terms of its own internal data
structures.
To simplify this vtable, I've moved a lot of the logging duties
relating to connection sharing out of ssh.c into sshshare.c: now it
handles nearly all the logging itself relating to setting up
connection sharing in the first place and downstreams connecting and
disconnecting. The only exception is the 'Reusing a shared connection'
announcement in the console window, which is now done in ssh.c by
detecting downstream status immediately after setup.
The tree234 storing currently active port forwardings - both local and
remote - now lives in portfwd.c, as does the complicated function that
updates it based on a Conf listing the new set of desired forwardings.
Local port forwardings are passed to ssh.c via the same route as
before - once the listening port receives a connection and portfwd.c
knows where it should be directed to (in particular, after the SOCKS
exchange, if any), it calls ssh_send_port_open.
Remote forwardings are now initiated by calling ssh_rportfwd_alloc,
which adds an entry to the rportfwds tree (which _is_ still in ssh.c,
and still confusingly sorted by a different criterion depending on SSH
protocol version) and sends out the appropriate protocol request.
ssh_rportfwd_remove cancels one again, sending a protocol request too.
Those functions look enough like ssh_{alloc,remove}_sharing_rportfwd
that I've merged those into the new pair as well - now allocating an
rportfwd allows you to specify either a destination host/port or a
sharing context, and returns a handy pointer you can use to cancel the
forwarding later.
Clients outside ssh.c - all implementations of Channel - will now not
see the ssh_channel data type itself, but only a subobject of the
interface type SshChannel. All the sshfwd_* functions have become
methods in that interface type's vtable (though, wrapped in the usual
kind of macros, the call sites look identical).
This paves the way for me to split up the SSH-1 and SSH-2 connection
layers and have each one lay out its channel bookkeeping structure as
it sees fit; as long as they each provide an implementation of the
sshfwd_ method family, the types behind that need not look different.
A minor good effect of this is that the sshfwd_ methods are no longer
global symbols, so they don't have to be stubbed in Unix Pageant to
get it to compile.
This was mildly fiddly because there's a single vtable structure that
implements two distinct interface types, one for compression and one
for decompression - and I have actually confused them before now
(commit d4304f1b7), so I think it's important to make them actually be
separate types!
Most of these were 'void *' because they weren't even reliably a
structure type underneath - the per-OS storage systems would directly
cast read/write/enum settings handles to and from random things like
FILE *, Unix DIR *, or Windows HKEY. So I've wrapped them in tiny
structs for the sake of having a sensible structure tag visible
elsewhere in the code.
'struct draw_ctx' has a structure tag inside gtkwin.c, so as per this
week's standard practice, let's expose the tag elsewhere so that
pointers declared that way can't be confused with anything else.
hmacmd5_do_hmac and hmac_sha1_simple should be consistently referring
to input memory blocks as 'const void *', but one had pointlessly
typed the pointer as 'const unsigned char *' and the other had missed
out the consts.
The new version of ssh_hash has the same nice property as ssh2_mac,
that I can make the generic interface object type function directly as
a BinarySink so that clients don't have to call h->sink() and worry
about the separate sink object they get back from that.
This piece of tidying-up has come out particularly well in terms of
saving tedious repetition and boilerplate. I've managed to remove
three pointless methods from every MAC implementation by means of
writing them once centrally in terms of the implementation-specific
methods; another method (hmacmd5_sink) vanished because I was able to
make the interface type 'ssh2_mac' be directly usable as a BinarySink
by way of a new delegation system; and because all the method
implementations can now find their own vtable, I was even able to
merge a lot of keying and output functions that had previously
differed only in length parameters by having them look up the lengths
in whatever vtable they were passed.
This is more or less the same job as the SSH-1 case, only more
extensive, because we have a wider range of ciphers.
I'm a bit disappointed about the AES case, in particular, because I
feel as if it ought to have been possible to arrange to combine this
layer of vtable dispatch with the subsidiary one that selects between
hardware and software implementations of the underlying cipher. I may
come back later and have another try at that, in fact.
The interchangeable system of SSH-1 ciphers previously followed the
same pattern as the backends and the public-key algorithms, in that
all the clients would maintain two separate pointers, one to the
vtable and the other to the individual instance / context. Now I've
merged them, just as I did with those other two, so that you only cart
around a single pointer, which has a vtable pointer inside it and a
type distinguishing it from an instance of any of the other
interchangeable sets of algorithms.
ATOFFSET in dialog.h became obsolete when the old 'struct Config' gave
way to the new Conf, because its only use was to identify fields in
struct Config for the generic control handlers to update.
And lenof in ssh.h is redundant because there's also a copy in misc.h.
(Which is already included _everywhere_ that lenof is used - I didn't
even need to add any instances of #include "misc.h" after removing the
copy in ssh.h.)
This was a particularly confusing piece of type-danger, because three
different types were passed outside sshshare.c as 'void *' and only
human vigilance prevented one coming back as the wrong one. Now they
all keep their opaque structure tags when they move through other
parts of the code.
There's now an interface called 'Channel', which handles the local
side of an SSH connection-layer channel, in terms of knowing where to
send incoming channel data to, whether to close the channel, etc.
Channel and the previous 'struct ssh_channel' mutually refer. The
latter contains all the SSH-specific parts, and as much of the common
logic as possible: in particular, Channel doesn't have to know
anything about SSH packet formats, or which SSH protocol version is in
use, or deal with all the fiddly stuff about window sizes - with the
exception that x11fwd.c's implementation of it does have to be able to
ask for a small fixed initial window size for the bodgy system that
distinguishes upstream from downstream X forwardings.
I've taken the opportunity to move the code implementing the detailed
behaviour of agent forwarding out of ssh.c, now that all of it is on
the far side of a uniform interface. (This also means that if I later
implement agent forwarding directly to a Unix socket as an
alternative, it'll be a matter of changing just the one call to
agentf_new() that makes the Channel to plug into a forwarding.)
This is another major source of unexplained 'void *' parameters
throughout the code.
In particular, the currently unused testback.c actually gave the wrong
pointer type to its internal store of the frontend handle - it cast
the input void * to a Terminal *, from which it got implicitly cast
back again when calling from_backend, and nobody noticed. Now it uses
the right type internally as well as externally.
Nearly every part of the code that ever handles a full backend
structure has historically done it using a pair of pointer variables,
one pointing at a constant struct full of function pointers, and the
other pointing to a 'void *' state object that's passed to each of
those.
While I'm modernising the rest of the code, this seems like a good
time to turn that into the same more or less type-safe and less
cumbersome system as I'm using for other parts of the code, such as
Socket, Plug, BinaryPacketProtocol and so forth: the Backend structure
contains a vtable pointer, and a system of macro wrappers handles
dispatching through that vtable.
Same principle again - the more of these structures have globally
visible tags (even if the structure contents are still opaque in most
places), the fewer of them I can mistake for each other.
That's one fewer anonymous 'void *' which might be accidentally
confused with some other pointer type if I misremember the order of
function arguments.
While I'm here, I've made its pointer-nature explicit - that is,
'Ldisc' is now a typedef for the structure type itself rather than a
pointer to it. A stylistic change only, but it feels more natural to
me these days for a thing you're going to eventually pass to a 'free'
function.
This source file isn't actually built as part of even a test binary,
so it hasn't been kept up to date with internal API changes. But it
might still come in useful in the future (I think its original purpose
was to substitute for a normal backend in order to test the GUI side
of a new PuTTY port before the network side was written), so I'll at
least try to carry on keeping it updated.
This commit adds the new ids and fingerprints in the keys appendix of
the manual, and moves the old ones down into the historic-keys
section. I've tweaked a few pieces of wording for ongoing use, so that
they don't imply a specific number of past key rollovers.
The -pgpfp option in all the tools now shows the new Master Key
fingerprint and the previous (2015) one. I've adjusted all the uses of
the #defines in putty.h so that future rollovers should only have to
modify the #defines themselves.
Most importantly, sign.sh bakes in the ids of the current release and
snapshot keys, so that snapshots will automatically be signed with the
new snapshot key and the -r option will invoke the new release key.
I had previously left the platform field (in line 7 of the installer
database's SummaryInformation table) set at "x86" instead of any value
you might expect such as "Arm" or "Arm64", because I found that an MSI
file with either of the latter values was rejected by WoA's msiexec as
invalid.
It turns out this is because I _also_ needed to upgrade the installer
database schema version to a higher value than I even knew existed:
apparently the problem is that those platform fields aren't present in
the older schema. A test confirms that this works.
Unfortunately, WiX 3 doesn't actually know _how_ to write MSIs with
those platform values. But that's OK, because diffing the x86 and x64
MSIs against each other suggested that there were basically no other
changes in the database tables - so I can just generate the installer
as if for x64, and then rewrite that one field after installer
construction using GNOME msitools to take apart the binary file
structure and put it back together. (Those are the same tools I'm
using as part of my system for running WiX on Linux in the first
place.)
This commit introduces a script to do that post-hoc bodging, and calls
it from Buildscr. I've also changed over the choice of Program Files
folder for the Arm installers so that it's ProgramFiles64Folder
instead of ProgramFilesFolder - so now the Windows on Arm installer
doesn't incongruously default to installing in C:\Program Files (x86)!
Now when we construct a packet containing sensitive data, we just set
a field saying '... and make it take up at least this much space, to
disguise its true size', and nothing in the rest of the system worries
about that flag until ssh2bpp.c acts on it.
Also, I've changed the strategy for doing the padding. Previously, we
were following the real packet with an SSH_MSG_IGNORE to make up the
size. But that was only a partial defence: it works OK against passive
traffic analysis, but an attacker proxying the TCP stream and
dribbling it out one byte at a time could still have found out the
size of the real packet by noting when the dribbled data provoked a
response. Now I put the SSH_MSG_IGNORE _first_, which should defeat
that attack.
But that in turn doesn't work when we're doing compression, because we
can't predict the compressed sizes accurately enough to make that
strategy sensible. Fortunately, compression provides an alternative
strategy anyway: if we've got zlib turned on when we send one of these
sensitive packets, then we can pad out the compressed zlib data as
much as we like by adding empty RFC1951 blocks (effectively chaining
ZLIB_PARTIAL_FLUSHes). So both strategies should now be dribble-proof.
The return value wasn't used to indicate failure; it only indicated
whether any compression was being done at all or whether the
compression method was ssh_comp_none, and we can tell the latter just
as well by the fact that its init function returns a null context
pointer.
Yesterday's reinstatement of ssh_free_pktout revealed - via valgrind
spotting the use-after-free - that the code that prefixed sensible
packets with IV-muddling SSH_MSG_IGNOREs was actually sending a second
copy of the sensible packet in place of the IGNORE, due to a typo.
I think ever since commit 679fa90df last month, PuTTY has been
forgetting to free any of its outgoing packet structures after turning
them into their encrypted wire format. And apparently no users of the
development snapshots have noticed - including me!
I just spotted it while I was looking through this module anyway. It
was using %.100s to prevent an sprintf buffer overflow, which hasn't
been necessary since I switched everything over to dupprintf, and also
it was printing the integer value of GetLastError() without using my
convenient translation wrapper win_strerror.
That's the same value as in the OpenSSH source code, so it should be
large enough that anyone needing to sign a larger message will have
other problems too.
I heard recently that at least one third-party client of Pageant
exists, and that it's used to generate signatures to use with TLS
client certificates. Apparently the signature scheme is compatible,
but TLS tends to need signatures over more data than will fit in
AGENT_MAX_MSGLEN.
Before the BinarySink refactor in commit b6cbad89f, this was OK
because the Windows Pageant IPC didn't check the size of the _input_
message against AGENT_MAX_MSGLEN, only the output one. But then we
started checking both, so that third-party TLS client started failing.
Now we use VirtualQuery to find out the actual size of the file
mapping we've been passed, and our only requirement is that the input
and output messages should both fit in _that_. So TLS should work
again, and also, other clients should be able to retrieve longer lists
of public keys if they pass a larger file mapping.
One side effect of this change is that Pageant's reply message is now
written directly into the shared-memory region. Previously, it was
written into a separate buffer and then memcpy()ed over after
pageant_handle_msg returned, but now the buffer is variable-size, it
seems to me to make more sense to avoid that extra not-entirely
controlled malloc. So I've done one very small reordering of
statements in the cross-platform pageant_handle_msg(), which fixes the
only case I could find where that function started writing its output
before it had finished using the contents of the input buffer.
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.
