I'm aiming for windows/winpgnt.c to only contain the parts of Windows
Pageant that are actually to do with handling the Windows API, and for
all the actual agent logic to be cross-platform.
This commit is a start: I've moved every function and internal
variable that was easy to move. But it doesn't get all the way there -
there's still a lot of logic in add_keyfile() and get_keylist*() that
would be good to move out to cross-platform code, but it's harder
because that code is currently quite intertwined with details of
Windows OS interfacing such as printing message boxes and passphrase
prompts and calling back out to agent_query if the Pageant doing that
job isn't the primary one.
Several of the functions in ssh2_signkey, and one or two SSH-1 key
functions too, were still taking assorted non-const buffer parameters
that had never been properly constified. Sort them all out.
This is better than listing all the algorithm names in yet another
place that will then need updating when a new key format is added.
However, that also means I need to find a new place to put the
'npieces' value I was previously setting up differently per key type;
since that's a fundamental property of the key format, I've moved it
to a constant field in the ssh_signkey structure, and filled that
field in for all the existing key types with the values from the
replaced code in openssh_read_new().
This was a lot less work than the importer, partly because the bcrypt
primitive is already working now, and mostly because we don't have to
handle the possible cross product of ciphers and kdfs in full and
completely hypothetical generality - we can emit a fixed choice of
either nothing or our chosen pair.
I thought it would be a good idea to share the loading code on the
basis that the outer header line + base64 format isn't too different,
but in fact I ended up faffing endlessly with mode bits and unions and
constantly re-testing in every subfunction which kind of key it was,
so that small saving wasn't worth it.
It's all very well for these two different formats to share a type
code as long as we're only loading them and not saving, but as soon as
we need to save one or the other, we'll need different type codes
after all.
This commit introduces the openssh_new_write() function, but for the
moment, it always returns failure.
I'd somehow managed to declare an enum in cmdgen.c with key types
OPENSSH and SSHCOM, and use it interchangeably with the one in ssh.h
with SSH_KEYTYPE_OPENSSH and SSH_KEYTYPE_SSHCOM.
It so happened that the relevant two enum values matched up! So this
hasn't caused a bug yet, but it's an accident waiting to happen. Fix
it before it does.
This is import only, for the moment: I haven't written an exporter
yet. Also, we currently don't support the format's full generality - a
new-style OpenSSH key file can contain multiple keys, but this code
currently only handles files with one key in them. That should be easy
to change, though, given only a little UI.
I'm about to use these in a new piece of code, but they may come in
helpful elsewhere as well. match_ssh_id in particular wraps an idiom
that's quite common in the rest of the codebase.
Since I've recently published a program that can easily generate the
required digits of pi, and since I was messing around in sshblowf.c
already, it seemed like a good idea to provide a derivation of all
that hex data.
This isn't the same as the standard bcrypt; it's OpenSSH's
modification that they use for their new-style key format.
In order to implement this, I've broken up blowfish_setkey() into two
subfunctions, and provided one of them with an extra optional salt
parameter, which is NULL in ordinary Blowfish but used by bcrypt.
Also, I've exposed some of sshblowf.c's internal machinery for the new
sshbcrypt.c to use.
SSH2_MSG_KEX_DH_GEX_REQUEST_OLD and SSH2_MSG_KEX_DH_GEX_REQUEST were
correctly _defined_ as different numbers, but the comments to the
right containing the hex representations of their values were
accidentally the same.
Now that we have modes in which the MAC verification happens before
any other crypto operation and hence will be the only thing seen by an
attacker, it seems like about time we got round to doing it in a
cautious way that tries to prevent the attacker from using our memcmp
as a timing oracle.
So, here's an smemeq() function which has the semantics of !memcmp but
attempts to run in time dependent only on the length parameter. All
the MAC implementations now use this in place of !memcmp to verify the
MAC on input data.
This causes the initial length field of the SSH-2 binary packet to be
unencrypted (with the knock-on effect that now the packet length not
including MAC must be congruent to 4 rather than 0 mod the cipher
block size), and then the MAC is applied over the unencrypted length
field and encrypted ciphertext (prefixed by the sequence number as
usual). At the cost of exposing some information about the packet
lengths to an attacker (but rarely anything they couldn't have
inferred from the TCP headers anyway), this closes down any
possibility of a MITM using the client as a decryption oracle, unless
they can _first_ fake a correct MAC.
ETM mode is enabled by means of selecting a different MAC identifier,
all the current ones of which are constructed by appending
"-etm@openssh.com" to the name of a MAC that already existed.
We currently prefer the original SSH-2 binary packet protocol (i.e. we
list all the ETM-mode MACs last in our KEXINIT), on the grounds that
it's better tested and more analysed, so at the moment the new mode is
only activated if a server refuses to speak anything else.
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.
When a winhandl.c input thread returns EOF to the main thread, the
latter might immediately delete the input thread's context. I
carefully wrote in a comment that in that case we had to not touch ctx
ever again after signalling to the main thread - but the test for
whether that was true, which also touched ctx, itself came _after_ the
SetEvent which sent that signal. Ahem.
Spotted by Minefield, which it looks as if I haven't run for a while.
I had set up an event object for signalling incoming connections to
the named pipe, and then called handle_add_foreign_event to get that
event object watched for connections - but when I closed down the
listening pipe, I deleted the event object without also cancelling
that foreign-event handle, so that winhandl.c would potentially call
the callback for a destroyed object.
A minus sign is illegal at that position in a control sequence, so if
ESC[13t should report something like ESC[3;-123;234t then we won't
accept it as input. Switch to printing the numbers as unsigned, so
that negative window coordinates are output as their 32-bit two's
complement; experimentation suggests that PuTTY does accept that on
input.
This was an old bug, fixed around 0.59, which apparently regressed
when I rewrote the main event loop using the toplevel_callback
mechanism.
Investigation just now suggests that it has to do with my faulty
assumption that Windows PeekMessage would deliver messages in its
message queue in FIFO order (i.e. that the thing calling itself a
message queue is actually a _queue_). In fact my WM_NETEVENT seems to
like to jump the queue, so that once a steady stream of them starts
arriving, we never do anything else in the main event loop (except
deal with handles).
Worked around in a simple and slightly bodgy way, namely, we don't
stop looping on PeekMessage and run our toplevel callbacks until we've
either run out of messages completely or else seen at least one that
_isn't_ a WM_NETEVENT. That way we should reliably interleave NETEVENT
processing with processing of other stuff.
Mostly I'm rearranging things because of the new workflows that git
makes available - it's now possible (and indeed sensible) to prepare a
lot of stuff in a fairly relaxed manner in local checkouts, and then
the process of going live with the release has a lot less manual
writing of stuff and a lot more mechanical 'git push' and running of
update scripts.
However, there's one new item that was actually missed off the
previous checklist: turning off nightly pre-release builds after
making the release they were a pre-release of. Ahem.
encodelib.py is a Python library which implements some handy SSH-2
encoding primitives; samplekex.py uses that to fabricate the start of
an SSH connection, up to the point where key exchange totally fails
its crypto.
The idea is that you adapt samplekex.py to construct initial-kex
sequences with particular properties, in order to test robustness and
security fixes that affect the initial-kex sequence. For example, I
used an adaptation of this to test the Diffie-Hellman range check
that's just gone into 0.64.
The absence of these could have prevented sensitive private key
information from being properly cleared out of memory that PuTTY tools
had finished with.
Thanks to Patrick Coleman for spotting this and sending a patch.
We incremented buf by a few bytes, so we must decrement the
corresponding length by the same amount, or else makekey() could
overrun.
Thanks to Patrick Coleman for the patch.
Florent Daigniere of Matta points out that RFC 4253 actually
_requires_ us to refuse to accept out-of-range values, though it isn't
completely clear to me why this should be a MUST on the receiving end.
Matta considers this to be a security vulnerability, on the grounds
that if a server should accidentally send an obviously useless value
such as 1 then we will fail to reject it and agree a key that an
eavesdropper could also figure out. Their id for this vulnerability is
MATTA-2015-002.
Bare string exceptions aren't supported any more.
Patch by Will Aoki, plus a backward compatibility tweak from Colin Watson.
Seen working with Python 2.4.3 and 2.7.6.
To understand the handle leak bug that I fixed in git commit
7549f2da40, I had to think fairly hard
to remind myself what all this code was doing, which means the
comments weren't good enough. Expanded and rewritten some of them in
the hope that things will be clearer next time.
If (say) a read handle returns EOF, and its gotdata function responds
by calling handle_free(), then we want the handle to have already had
its defunct flag set so that the handle can be destroyed. Otherwise
handle_free will set the 'done' flag to ask the subthread to
terminate, and then sit and wait for it to say it's done so -
forgetting that it signalled termination already by returning EOF, and
hence will not be responding to that signal.
Ditto for write errors on write handles, though that should happen
less often.
The code for cleaning up handle structures works by the main thread
asking the per-handle subthread to shut down by means of setting its
'done' flag, and then once the subthread signals back through its
event object that it's done so, the main thread frees all its
resources and removes the event object from the list of things being
checked in the program's event loop.
But read threads were not sending back that final event acknowledging
a request to shut down, so their event objects were never being
cleaned up.
Bug spotted by Ronald Weiss.
It would be rare to have a host keypair in .ppk format or on a client
machine to load into PuTTYgen, and it might confuse people into thinking
they are required to do so.
It tries to use the local username as the remote username if it has no
better ideas, but the presence of Default Settings would defeat this,
even if it had no username set. Reported by Jonathan Amery.
