Mostly so that we don't have to malloc contiguous space for them
inside PuTTY; since we've already got a handy constant saying how big
is too big, we might as well use it to sanity-check the contents of
our agent forwarding channels.
The previous agent-forwarding system worked by passing each complete
query received from the input to agent_query() as soon as it was
ready. So if the remote client were to pipeline multiple requests,
then Unix PuTTY (in which agent_query() works asynchronously) would
parallelise them into many _simultaneous_ connections to the real
agent - and would not track which query went out first, so that if the
real agent happened to send its replies (to what _it_ thought were
independent clients) in the wrong order, then PuTTY would serialise
the replies on to the forwarding channel in whatever order it got
them, which wouldn't be the order the remote client was expecting.
To solve this, I've done a considerable rewrite, which keeps the
request stream in a bufchain, and only removes data from the bufchain
when it has a complete request. Then, if agent_query decides to be
asynchronous, the forwarding system waits for _that_ agent response
before even trying to extract the next request's worth of data from
the bufchain.
As an added bonus (in principle), this gives agent-forwarding channels
some actual flow control for the first time ever! If a client spams us
with an endless stream of rapid requests, and never reads its
responses, then the output side of the channel will run out of window,
which causes us to stop processing requests until we have space to
send responses again, which in turn causes us to stop granting extra
window on the input side, which serves the client right.
Now, instead of returning a boolean indicating whether the query has
completed or is still pending, agent_query() returns NULL to indicate
that the query _has_ completed, and if it hasn't, it returns a pointer
to a context structure representing the pending query, so that the
latter can be used to cancel the query if (for example) you later
decide you need to free the thing its callback was using as a context.
This should fix a potential race-condition segfault if you overload an
agent forwarding channel and then close it abruptly. (Which nobody
will be doing for sensible purposes, of course! But I ran across this
while stress-testing other aspects of agent forwarding.)
From ssh2_channel_got_eof() to ssh2_msg_channel_eof(). This removes
the only SSH-2 specicifity from the former. ssh2_channel_got_eof()
can also be called from ssh2_msg_channel_close(), but that calls
ssh2_channel_check_close() already.
Nothing ever sets them to NULL, and the various paths by which the
channel types can be set to CHAN_X11 or CHAN_SOCKDATA all ensure thet
the relevant union members are non-NULL. All the removed conditionals
have been converted into assertions, just in case I'm wrong.
It's redundant with the halfopen flag and is a misuse of the channel
type field. Happily, everything that depends on CHAN_SOCKDATA_DORMANT
also checks halfopen, so removing it is trivial.
Now it disconnects if the server sends
SSH_MSG_CHANNEL_OPEN_CONFIRMATION or SSH_MSG_CHANNEL_OPEN_FAILURE for
a channel that isn't half-open. Assertions in the SSH-2 handlers for
these messages rely on this behaviour even though it's never been
enforced before.
All but one caller was doing this unconditionally. The one conditional
call was when initialising the main channel, and in consequence PuTTY
leaked a channel structure when the server refused to open the main
channel. Now it doesn't.
An opcode for this was recently published in
https://tools.ietf.org/html/draft-sgtatham-secsh-iutf8-00 .
The default setting is conditional on frontend_is_utf8(), which is
consistent with the pty back end's policy for setting the same flag
locally. Of course, users can override the setting either way in the
GUI configurer, the same as all other tty modes.
Previously, the code that marshalled tty settings into the "pty-req"
request was iterating through the subkeys stored in ssh->conf, meaning
that if a session had been saved before we gained support for a
particular tty mode, the iteration wouldn't visit that mode at all and
hence wouldn't send even the default setting for it.
Now we iterate over the array of known mode identifiers in
ssh_ttymodes[] and look each one up in ssh->conf, rather than vice
versa. This means that when we add support for a new tty mode with a
nontrivial policy for choosing its default state, we should start
using the default handler immediately, rather than bizarrely waiting
for users to save a session after the change.
Also add an assertion to do_ssh2_transport to catch this.
This bug would be highly unlikely to manifest accidentally, but I
think you could trigger it by setting the data-based rekey threshold
very low.
All calls to ssh2_add_channel_data() were followed by a call to
ssh2_try_send(), so it seems sensible to replace ssh2_add_channel_data()
with ssh2_send_channel_data(), which does both.
Specifically, don't try to unblock all channels just because we've got
something to send on the main one. It looks like the code to do that
was left over from when SSH_MSG_CHANNEL_ADJUST was handled in
do_ssh2_authconn().
The UI now only has "1" and "2" options for SSH protocol version, which
behave like the old "1 only" and "2 only" options; old
SSH-N-with-fallback settings are interpreted as SSH-N-only.
This prevents any attempt at a protocol downgrade attack.
Most users should see no difference; those poor souls who still have to
work with SSH-1 equipment now have to explicitly opt in.
If you're connecting to a new server and it _only_ provides host key
types you've configured to be below the warning threshold, it's OK to
give the standard askalg() message. But if you've newly demoted a host
key type and now reconnect to some server for which that type was the
best key you had cached, the askalg() wording isn't really appropriate
(it's not that the key we've settled on is the first type _supported
by the server_, it's that it's the first type _cached by us_), and
also it's potentially helpful to list the better algorithms so that
the user can pick one to cross-certify.
When Jacob introduced this message in d0d3c47a0, he was right to
assume that hostkey_algs[] and ssh->uncert_hostkeys[] were sorted in
the same order. Unfortunately, he became wrong less than an hour later
when I committed d06098622. Now we avoid making any such assumption.
Since we got a dynamic preference order, it's been bailing out at a
random point, and listing keys we wouldn't use.
(It would still be nice to only mention keys that we'd actually use, but
that's now quite fiddly.)
I noticed this in passing while tinkering with the hostkey_algs array:
these arrays are full of pointers-to-const, but are not also
themselves declared const, which they should have been all along.
Now we actually have enough of them to worry about, and especially
since some of the types we support are approved by organisations that
people might make their own decisions about whether to trust, it seems
worth having a config list for host keys the same way we have one for
kex types and ciphers.
To make room for this, I've created an SSH > Host Keys config panel,
and moved the existing host-key related configuration (manually
specified fingerprints) into there from the Kex panel.
I got momentarily confused between whether the special code
(TS_LOCALSTART+i) meant the ith entry in the variable
uncert_hostkeys[] array, or the ith entry in the fixed hostkey_algs[]
array. Now I think everything agrees on it being the latter.
If a server offers host key algorithms that we don't have a stored key
for, they will now appear in a submenu of the Special Commands menu.
Selecting one will force a repeat key exchange with that key, and if
it succeeds, will add the new host key to the cache. The idea is that
the new key sent by the server is protected by the crypto established
in the previous key exchange, so this is just as safe as typing some
command like 'ssh-keygen -lf /etc/ssh/ssh_host_ed25519_key.pub' at the
server prompt and transcribing the results manually.
This allows switching over to newer host key algorithms if the client
has begun to support them (e.g. people using PuTTY's new ECC
functionality for the first time), or if the server has acquired a new
key (e.g. due to a server OS upgrade).
At the moment, it's only available manually, for a single host key
type at a time. Automating it is potentially controversial for
security policy reasons (what if someone doesn't agree this is what
they want in their host key cache, or doesn't want to switch over to
using whichever of the keys PuTTY would now put top of the list?), for
code plumbing reasons (chaining several of these rekeys might be more
annoying than doing one at a time) and for CPU usage reasons (rekeys
are expensive), but even so, it might turn out to be a good idea in
future.
The last list we returned is now stored in the main Ssh structure
rather than being a static array in ssh_get_specials.
The main point of this is that I want to start adding more dynamic
things to it, for which I can't predict the array's max length in
advance.
But also this fixes a conceptual wrongness, in that if a process had
more than one Ssh instance in it then their specials arrays would have
taken turns occupying the old static array, and although the current
single-threaded client code in the GUI front ends wouldn't have minded
(it would have read out the contents just once immediately after
get_specials returned), it still feels as if it was a bug waiting to
happen.
ssh_pkt_getstring can return (NULL,0) if the input packet is too short
to contain a valid string.
In quite a few places we were passing the returned pointer,length pair
to a printf function with "%.*s" type format, which seems in practice
to have not been dereferencing the pointer but the C standard doesn't
actually guarantee that. In one place we were doing the same job by
hand with memcpy, and apparently that _can_ dereference the pointer in
practice (so a server could have caused a NULL-dereference crash by
sending an appropriately malformed "x11" type channel open request).
And also I spotted a logging call in the "forwarded-tcpip" channel
open handler which had forgotten the field width completely, so it was
erroneously relying on the string happening to be NUL-terminated in
the received packet.
I've tightened all of this up in general by normalising (NULL,0) to
("",0) before calling printf("%.*s"), and replacing the two even more
broken cases with the corrected version of that same idiom.
It has three settings: on, off, and 'only until session starts'. The
idea of the last one is that if you use something like 'ssh -v' as
your proxy command, you probably wanted to see the initial SSH
connection-setup messages while you were waiting to see if the
connection would be set up successfully at all, but probably _didn't_
want a slew of diagnostics from rekeys disrupting your terminal in
mid-emacs once the session had got properly under way.
Default is off, to avoid startling people used to the old behaviour. I
wonder if I should have set it more aggressively, though.
I'm about to want to make a change to all those functions at once, and
since they're almost identical, it seemed easiest to pull them out
into a common helper. The new source file be_misc.c is intended to
contain helper code common to all network back ends (crypto and
non-crypto, in particular), and initially it contains a
backend_socket_log() function which is the common part of ssh_log(),
telnet_log(), rlogin_log() etc.
We've always had the back-end code unconditionally print 'Looking up
host' before calling name_lookup. But name_lookup doesn't always do an
actual lookup - in cases where the connection will be proxied and
we're configured to let the proxy do the DNS for us, it just calls
sk_nonamelookup to return a dummy SockAddr with the unresolved name
still in it. It's better to print a message that varies depending on
whether we're _really_ doing DNS or not, e.g. so that people can tell
the difference between DNS failure and proxy misconfiguration.
Hence, those log messages are now generated inside name_lookup(),
which takes a couple of extra parameters for the purpose - a frontend
pointer to pass to logevent(), and a reason string so that it can say
what the hostname it's (optionally) looking up is going to be used
for. (The latter is intended for possible use in logging subsidiary
lookups for port forwarding, though the moment I haven't changed
the current setup where those connection setups aren't logged in
detail - we just pass NULL in that situation.)
When we set ssh->sc{cipher,mac} to s->sc{cipher,mac}_tobe
conditionally, we should be conditionalising on the values we're
_reading_, not the ones we're about to overwrite.
Thanks to Colin Harrison for this patch.
Apparently if you maintain a branch for a long time where you only
compile with a non-default ifdef enabled, it becomes possible to not
notice a typo you left in the default branch :-)
This adds the "none" cipher and MAC, and also disables kex signure
verification and host-key checking. Since a client like this is
completely insecure, it also rewrites the client version string to
start "ISH", which should make it fail to interoperate with a real SSH
server. The server version string is still expected to begin "SSH" so
the real packet captures can be used against it.
