This is just a linked list of 'struct Packet' with a convenience API
on the front. As yet it's unused, so ssh.c will currently not compile
with gcc -Werror unless you also add -Wno-unused-function. But all the
functions I've added here will be used in later commits in the current
patch series, so that's only a temporary condition.
Previously it was local, which _mostly_ worked, except that if the SSH
host key needed verifying via a non-modal dialog box, there could be a
crReturn in between writing it and reading it.
It's pretty tempting to suggest that because nobody has noticed this
before, SSH-1 can't be needed any more! But actually I suspect the
intervening crReturn has only appeared since the last release,
probably around November when I was messing about with GTK dialog box
modality. (I observed the problem just now on the GTK build, while
trying to check that a completely different set of changes hadn't
broken SSH-1.)
It hasn't been used since 2012, when commit 8e0ab8be5 introduced a new
method of getting the do_ssh2_authconn coroutine started, and didn't
notice that the variable we were previously using was now completely
unused.
The 2-minutely check to see whether new GSS credentials need to be
forwarded to the server is pointless if we're not even in the mode
where we _have_ forwarded a previous set.
This was made obvious by the overly verbose diagnostic fixed in the
previous commit, so it's a good thing that bug was temporarily there!
Now we don't generate that message as a side effect of the periodic
check for new GSS credentials; we only generate it as part of the much
larger slew of messages that happen during a rekey.
Commit d515e4f1a went through a lot of very different shapes before it
was finally pushed. In some of them, GSS kex had its own value in the
kex enumeration, but it was used in ssh.c but not in config.c
(because, as in the final version, it wasn't configured by the same
drag-list system as the rest of them). So we had to distinguish the
set of key exchange ids known to the program as a whole from the set
controllable in the configuration.
In the final version, GSS kex ended up even more separated from the
kex enumeration than that: the enum value KEX_GSS_SHA1_K5 isn't used
at all. Instead, GSS key exchange appears in the list at the point of
translation from the list of enum values into the list of pointers to
data structures full of kex methods.
But after all the changes, everyone involved forgot to revert the part
of the patch which split KEX_MAX in two and introduced the pointless
value KEX_GSS_SHA1_K5! Better late than never: I'm reverting it now,
to avoid confusion, and because I don't have any reason to think the
distinction will be useful for any other purpose.
The former has advantages in terms of keeping Kerberos credentials up
to date, but it also does something sufficiently weird to the usual
SSH host key system that I think it's worth making sure users have a
means of turning it off separately from the less intrusive GSS
userauth.
This is a heavily edited (by me) version of a patch originally due to
Nico Williams and Viktor Dukhovni. Their comments:
* Don't delegate credentials when rekeying unless there's a new TGT
or the old service ticket is nearly expired.
* Check for the above conditions more frequently (every two minutes
by default) and rekey when we would delegate credentials.
* Do not rekey with very short service ticket lifetimes; some GSSAPI
libraries may lose the race to use an almost expired ticket. Adjust
the timing of rekey checks to try to avoid this possibility.
My further comments:
The most interesting thing about this patch to me is that the use of
GSS key exchange causes a switch over to a completely different model
of what host keys are for. This comes from RFC 4462 section 2.1: the
basic idea is that when your session is mostly bidirectionally
authenticated by the GSSAPI exchanges happening in initial kex and
every rekey, host keys become more or less vestigial, and their
remaining purpose is to allow a rekey to happen if the requirements of
the SSH protocol demand it at an awkward moment when the GSS
credentials are not currently available (e.g. timed out and haven't
been renewed yet). As such, there's no need for host keys to be
_permanent_ or to be a reliable identifier of a particular host, and
RFC 4462 allows for the possibility that they might be purely
transient and only for this kind of emergency fallback purpose.
Therefore, once PuTTY has done a GSS key exchange, it disconnects
itself completely from the permanent host key cache functions in
storage.h, and instead switches to a _transient_ host key cache stored
in memory with the lifetime of just that SSH session. That cache is
populated with keys received from the server as a side effect of GSS
kex (via the optional SSH2_MSG_KEXGSS_HOSTKEY message), and used if
later in the session we have to fall back to a non-GSS key exchange.
However, in practice servers we've tested against do not send a host
key in that way, so we also have a fallback method of populating the
transient cache by triggering an immediate non-GSS rekey straight
after userauth (reusing the code path we also use to turn on OpenSSH
delayed encryption without the race condition).
This is a preliminary refactoring for an upcoming change which will
need to affect every use of schedule_timer to wait for the next rekey:
those calls to schedule_timer are now centralised into a function that
does an organised piece of thinking about when the next timer should
be.
A side effect of this change is that the translation from
CONF_ssh_rekey_time to an actual tick count is now better proofed
against integer overflow (just in case the user entered a completely
silly value).
In the 'SSH packets + raw data' logging mode, one of these occurs
immediately after the initial key exchange, at the point where the
transport routine releases any queued higher-layer packets that had
been waiting for KEX to complete. Of course, in the initial KEX there
are never any of those, so we do a zero-length s_write(), which is
harmless but has the side effect of a zero-length raw-data log entry.
If ssh_init encounters a synchronous error, it will call random_unref
before returning. But the Ssh structure it created will still exist,
and if the caller (sensibly) responds by freeing it, then that will
cause a second random_unref, leading to the RNG's refcount going below
zero and failing an assertion.
We never noticed this before because with only one PuTTY connection
per process it was easier to just exit(1) without bothering to clean
things up. Now, with all the multi-sessions-per-process fixes I'm
doing, this has shown up as a problem. But other front ends may
legitimately still just exit - I don't think I can sensibly enforce
_not_ doing so at this late stage - so I've had to arrange to set a
flag in the Ssh saying whether a random_unref is still pending or not.
I think these began to appear as a consequencce of replacing
fatalbox() calls with more sensible error reports: the more specific a
direction I send a report in, the greater the annoying possibility of
re-entrance when the resulting error handler starts closing stuff.
ssh1_rdpkt claimed to be handling SSH1_MSG_DEBUG and SSH1_MSG_IGNORE
packets, but in fact, the handling of those has long since been moved
into the dispatch table; those particular entries are set up in
ssh1_protocol_setup().
When it calls through ocr->handler() to process the response to a
channel request, sometimes that call ends up back in the main SSH-2
authconn coroutine, and sometimes _that_ will call bomb_out(), which
closes the whole SSH connection and frees all the channels - so that
when control returns back up the call stack to
ssh2_msg_channel_response itself which continues working with the
channel it was passed, it's using freed memory and things go badly.
This is the sort of thing I'd _like_ to fix using some kind of
large-scale refactoring along the lines of moving all the actual
free() calls out into top-level callbacks, so that _any_ function
which is holding a pointer to something can rely on that pointer still
being valid after it calls a subroutine. But I haven't worked out all
the details of how that system should work, and doubtless it will turn
out to have problems of its own once I do, so here's a point fix which
simply checks if the whole SSH session has been closed (which is easy
- much easier than checking if that _channel_ structure still exists)
and fixes the immediate bug.
(I think this is the real fix for the problem reported by the user I
mention in commit f0126dd19, because I actually got the details wrong
in the log message for that previous commit: the user's SSH server
wasn't rejecting the _opening_ of the main session channel, it was
rejecting the "shell" channel request, so this code path was the one
being exercised. Still, the other bug was real too, so no harm done!)
A user reported a nonsensical assertion failure (claiming that
ssh->version != 2) which suggested that a channel had somehow outlived
its parent Ssh in the situation where the opening of the main session
channel is rejected by the server. Checking with valgrind suggested
that things start to go wrong at the point where we free the half-set-
up ssh->mainchan before having filled in its type field, so that the
switch in ssh_channel_close_local() picks an arbitrary wrong action.
I haven't reproduced the same failure the user reported, but with this
change, Unix plink is now valgrind-clean in that failure situation.
This has been a FIXME in the code for ages, because back when the main
channel was always a pty session or a program run in a pipe, there
weren't that many circumstances in which the actual CHANNEL_OPEN could
return failure, so it never seemed like a priority to get round to
pulling the error information out of the CHANNEL_OPEN_FAILURE response
message and including it in PuTTY or Plink's local error message.
However, 'plink -nc' is the real reason why this is actually
important; if you tell the SSH server to make a direct-tcpip network
connection as its main channel, then that can fail for all the usual
network-unreliability reasons, and you actually do want to know which
(did you misspell the hostname, or is the target server refusing
connections, or has network connectivity failed?). This actually bit
me today when I had such a network failure, and had to debug it by
pulling that information manually out of a packet log. Time to
eliminate that FIXME.
So I've pulled the error-extracting code out of the previous handler
for OPEN_FAILURE on non-main channels into a separate function, and
arranged to call that function if the main channel open fails too. In
the process I've made a couple of minor tweaks, e.g. if the server
sends back a reason code we haven't heard of, we say _what_ that
reason code was, and also we at least make a token effort to spot if
we see a packet other than OPEN_{CONFIRMATION,FAILURE} reaching the
main loop in response to the main channel-open.
2ce0b680c inadvertently removed this ability in trying to ensure that
everyone got the new IUTF8 mode by default; you could remove a mode from
the list in the UI, but this would just revert PuTTY to its default.
The UI and storage have been revamped; the storage format now explicitly
says when a mode is not to be sent, and the configuration UI always
shows all modes known to PuTTY; if a mode is not to be sent it now shows
up as "(don't send)" in the list.
Old saved settings are migrated so as to preserve previous removals of
longstanding modes, while automatically adding IUTF8.
(In passing, this removes a bug where pressing the 'Remove' button of
the previous UI would populate the value edit box with garbage.)
I think an agent sending a string length exceeding the buffer bounds
by less than 4 could have made PuTTY read beyond its own buffer end.
Not that I really think a hostile SSH agent is likely to be attacking
PuTTY, but it's as well to fix these things anyway!
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.
The previous assertion failure is obviously wrong, but RFC 4253 doesn't
explicitly declare them to be a protocol error. Currently, the incoming
packet isn't logged, which might cause some confusion for log parsers.
Bug found with the help of afl-fuzz.
The previous assertion failure is obviously wrong, but RFC 4253 doesn't
explicitly declare them to be a protocol error. Currently, the incoming
packet isn't logged, which might cause some confusion for log parsers.
Bug found with the help of afl-fuzz.
This protects the Unix platform sharing code in the case where no salt
file exists yet in the connection-sharing directory, in which case
make_dirname() will want to create one by using some random bytes, and
prior to this commit, would fail an assertion because the random
number generator wasn't set up.
It would be neater to just return FALSE from ssh_test_for_upstream in
that situation - if there's no salt file, then no sharing socket can
be valid anyway - but that would involve doing more violence to the
code structure than I'm currently prepared to do for a minor elegance
gain.
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.
(cherry picked from commit 9a08d9a7c1)
A Plink invocation of the form 'plink -shareexists <session>' tests
for a currently live connection-sharing upstream for the session in
question. <session> can be any syntax you'd use with Plink to make the
actual connection (a host/port number, a bare saved session name,
-load, whatever).
I envisage this being useful for things like adaptive proxying - e.g.
if you want to connect to host A which you can't route to directly,
and you might already have a connection to either of hosts B or C
which are viable proxies, then you could write a proxy shell script
which checks whether you already have an upstream for B or C and goes
via whichever one is currently active.
Testing for the upstream's existence has to be done by actually
connecting to its socket, because on Unix the mere existence of a
Unix-domain socket file doesn't guarantee that there's a process
listening to it. So we make a test connection, and then immediately
disconnect; hence, that shows up in the upstream's event log.
This is the part of ssh.c's connect_to_host() which figures out the
host name and port number that logically identify the connection -
i.e. not necessarily where we physically connected to, but what we'll
use to look up the saved session cache, put in the window title bar,
and give to the connection sharing code to identify other connections
to share with.
I'm about to want to use it for another purpose, so it needs to be
moved out into a separate function.
If we've chosen the ChaCha20-Poly1305 option for a cipher, then that
forces the use of its associated MAC. In that situation, we should
avoid even _trying_ to figure out a MAC by examining the MAC string
from the server's KEXINIT, because we won't use the MAC selected by
that method anyway, so there's no point imposing the requirement on
servers to present a MAC we believe in just so we know it's there.
This was breaking interoperation with tinysshd, and is in violation of
OpenSSH's spec for the "chacha20-poly1305@openssh.com" cipher.
The revamp of key generation in commit e460f3083 made the assumption
that you could decide how many bytes of key material to generate by
converting cipher->keylen from bits to bytes. This is a good
assumption for all ciphers except DES/3DES: since the SSH DES key
setup ignores one bit in every byte of key material it's given, you
need more bytes than its keylen field would have you believe. So
currently the DES ciphers aren't being keyed correctly.
The original keylen field is used for deciding how big a DH group to
request, and on that basis I think it still makes sense to keep it
reflecting the true entropy of a cipher key. So it turns out we need
two _separate_ key length fields per cipher - one for the real
entropy, and one for the much more obvious purpose of knowing how much
data to ask for from ssh2_mkkey.
A compensatory advantage, though, is that we can now measure the
latter directly in bytes rather than bits, so we no longer have to
faff about with dividing by 8 and rounding up.
Tim Kosse points out that we now support some combinations of crypto
primitives which break the hardwired assumption that two blocks of
hash output from the session-key derivation algorithm are sufficient
to key every cipher and MAC in the system.
So now ssh2_mkkey is given the desired key length, and performs as
many iterations as necessary.
The key derivation code has been assuming (though non-critically, as
it happens) that the size of the MAC output is the same as the size of
the MAC key. That isn't even a good assumption for the HMAC family,
due to HMAC-SHA1-96 and also the bug-compatible versions of HMAC-SHA1
that only use 16 bytes of key material; so now we have an explicit
key-length field separate from the MAC-length field.
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.
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.
I removed a vital line of code while fixing the merge conflicts when
cherry-picking 1eb578a488 as
26fe1e26c0, 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.
Assorted calls to ssh_pkt_getstring in handling the later parts of key
exchange (post-KEXINIT) were not checked for NULL afterwards, so that
a variety of badly formatted key exchange packets would cause a crash
rather than a sensible error message.
None of these is an exploitable vulnerability - the server can only
force a clean null-deref crash, not an access to actually interesting
memory.
Thanks to '3unnym00n' for pointing out one of these, causing me to
find all the rest of them too.
(cherry picked from commit 1eb578a488)
Conflicts:
ssh.c
Cherry-picker's notes: the main conflict arose because the original
commit also made fixes to the ECDH branch of the big key exchange if
statement, which doesn't exist on this branch. Also there was a minor
and purely textual conflict, when an error check was added right next
to a function call that had acquired an extra parameter on master.
The final main loop in do_ssh2_authconn will sometimes loop over all
currently open channels calling ssh2_try_send_and_unthrottle. If the
channel is a sharing one, however, that will reference fields of the
channel structure like 'remwindow', which were never initialised in
the first place (thanks, valgrind). Fix by excluding CHAN_SHARING
channels from that loop.
(cherry picked from commit 7366fde1d4)
When anyone connects to a PuTTY tool's listening socket - whether it's
a user of a local->remote port forwarding, a connection-sharing
downstream or a client of Pageant - we'd like to log as much
information as we can find out about where the connection came from.
To that end, I've implemented a function sk_peer_info() in the socket
abstraction, which returns a freeform text string as best it can (or
NULL, if it can't get anything at all) describing the thing at the
other end of the connection. For TCP connections, this is done using
getpeername() to get an IP address and port in the obvious way; for
Unix-domain sockets, we attempt SO_PEERCRED (conditionalised on some
moderately hairy autoconfery) to get the pid and owner of the peer. I
haven't implemented anything for Windows named pipes, but I will if I
hear of anything useful.
(cherry picked from commit c8f83979a3)
Conflicts:
pageant.c
Cherry-picker's notes: the conflict was because the original commit
also added a use of the same feature in the centralised Pageant code,
which doesn't exist on this branch. Also I had to remove 'const' from
the type of the second parameter to wrap_send_port_open(), since this
branch hasn't had the same extensive const-fixing as master.
PuTTY now uses the updated version of Diffie-Hellman group exchange,
except for a few old OpenSSH versions which Darren Tucker reports only
support the old version.
FIXME: this needs further work because the Bugs config panel has now
overflowed.
(cherry picked from commit 62a1bce7cb)
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.
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.
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.
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.
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.
