Caused an embarrassing failure just now trying to run the test program
from a command prompt - I had Return still held down by the time it
started up, and my release of it immediately terminated input :-)
The new code remembers the contents and meaning of the outgoing KEXINIT
and uses this to drive the algorithm negotiation, rather than trying to
reconstruct what the outgoing KEXINIT probably said. This removes the
need to maintain the KEXINIT generation and parsing code precisely in
parallel.
It also fixes a bug whereby PuTTY would have selected the wrong host key
type in cases where the server gained a host key type between rekeys.
The OpenSSH PEM format contains a big integer with the top bit
potentially set, which we handle by copying the data into a faked up
instance of our own private key format, and passing that to
ecdsa_createkey(). But our own private key format expects an SSH-2
standard mpint, i.e. with the top bit reliably clear, so this might
fail for no good reason.
Fixed by prefixing a zero byte unconditionally when constructing the
fake private blob.
snew(), and most of the bignum functions, are deliberately written to
fail an assertion and terminate the program rather than return NULL,
so there's no point carefully checking their every return value for
NULL. This removes a huge amount of pointless error-checking code, and
makes the elliptic curve arithmetic almost legible in places :-)
I've kept error checks after modinv(), because that can return NULL if
asked to invert zero. bigsub() can also fail in principle, because our
bignums are non-negative only, but in the couple of cases where it's
used there's a preceding compare that should prevent it, so I've just
added assertions.
The menu options and radio buttons for key type were not consistently
setting each other when selected: in particular, selecting from the
menu did not cause the ED25519 radio button to be either set or unset
when that would have been appropriate.
Looks as if I failed to catch in code review the fact that we should
have _one_ call to each of CheckRadioButton and CheckMenuRadioItem,
and they should both have the right 'first' and 'last' parameters
Having found a lot of unfixed constness issues in recent development,
I thought perhaps it was time to get proactive, so I compiled the
whole codebase with -Wwrite-strings. That turned up a huge load of
const problems, which I've fixed in this commit: the Unix build now
goes cleanly through with -Wwrite-strings, and the Windows build is as
close as I could get it (there are some lingering issues due to
occasional Windows API functions like AcquireCredentialsHandle not
having the right constness).
Notable fallout beyond the purely mechanical changing of types:
- the stuff saved by cmdline_save_param() is now explicitly
dupstr()ed, and freed in cmdline_run_saved.
- I couldn't make both string arguments to cmdline_process_param()
const, because it intentionally writes to one of them in the case
where it's the argument to -pw (in the vain hope of being at least
slightly friendly to 'ps'), so elsewhere I had to temporarily
dupstr() something for the sake of passing it to that function
- I had to invent a silly parallel version of const_cmp() so I could
pass const string literals in to lookup functions.
- stripslashes() in pscp.c and psftp.c has the annoying strchr nature
Removed another set of ad-hoc tests of the key size to decide which
hash to use for the signature system, and replaced them with a
straightforward pointer to an ssh_hash structure in the 'extra' area.
The ec_name_to_curve and ec_curve_to_name functions shouldn't really
have had to exist at all: whenever any part of the PuTTY codebase
starts using sshecc.c, it's starting from an ssh_signkey or ssh_kex
pointer already found by some other means. So if we make sure not to
lose that pointer, we should never need to do any string-based lookups
to find the curve we want, and conversely, when we need to know the
name of our curve or our algorithm, we should be able to look it up as
a straightforward const char * starting from the algorithm pointer.
This commit cleans things up so that that is indeed what happens. The
ssh_signkey and ssh_kex structures defined in sshecc.c now have
'extra' fields containing pointers to all the necessary stuff;
ec_name_to_curve and ec_curve_to_name have been completely removed;
struct ec_curve has a string field giving the curve's name (but only
for those curves which _have_ a name exposed in the wire protocol,
i.e. the three NIST ones); struct ec_key keeps a pointer to the
ssh_signkey it started from, and uses that to remember the algorithm
name rather than reconstructing it from the curve. And I think I've
got rid of all the ad-hockery scattered around the code that switches
on curve->fieldBits or manually constructs curve names using stuff
like sprintf("nistp%d"); the only remaining switch on fieldBits
(necessary because that's the UI for choosing a curve in PuTTYgen) is
at least centralised into one place in sshecc.c.
One user-visible result is that the format of ed25519 host keys in the
registry has changed: there's now no curve name prefix on them,
because I think it's not really right to make up a name to use. So any
early adopters who've been using snapshot PuTTY in the last week will
be inconvenienced; sorry about that.
This gives families of public key and kex functions (by which I mean
those sharing a set of methods) a place to store parameters that allow
the methods to vary depending on which exact algorithm is in use.
The ssh_kex structure already had a set of parameters specific to
Diffie-Hellman key exchange; I've moved those into sshdh.c and made
them part of the 'extra' structure for that family only, so that
unrelated kex methods don't have to faff about saying NULL,NULL,0,0.
(This required me to write an extra accessor function for ssh.c to ask
whether a DH method was group-exchange style or fixed-group style, but
that doesn't seem too silly.)
Not all of them, but the ones that don't get a 'void *key' parameter.
This means I can share methods between multiple ssh_signkey
structures, and still give those methods an easy way to find out which
public key method they're dealing with, by loading parameters from a
larger structure in which the ssh_signkey is the first element.
(In OO terms, I'm arranging that all static methods of my public key
classes get a pointer to the class vtable, to make up for not having a
pointer to the class instance.)
I haven't actually done anything with the new facility in this commit,
but it will shortly allow me to clean up the constant lookups by curve
name in the ECDSA code.
All the name strings in ssh_cipher, ssh_mac, ssh_hash, ssh_signkey
point to compile-time string literals, hence should obviously be const
char *.
Most of these const-correctness patches are just a mechanical job of
adding a 'const' in the one place you need it right now, and then
chasing the implications through the code adding further consts until
it compiles. But this one has actually shown up a bug: the 'algorithm'
output parameter in ssh2_userkey_loadpub was sometimes returning a
pointer to a string literal, and sometimes a pointer to dynamically
allocated memory, so callers were forced to either sometimes leak
memory or sometimes free a bad thing. Now it's consistently
dynamically allocated, and should be freed everywhere too.
Adding an extra radio button to the key-type selector caused it to
wrap on to another line and push the bottom of the containing control
box down off the bottom of the window.
In the long term, should more public key formats continue to appear,
we'll probably have to replace the radio buttons with something more
extensible like a drop-down list. For the moment, though, I've fixed
it by just reducing the space per radio button to bring all five
controls back on to the same line.
To fit the text into the smaller space, I also removed the 'SSH-2'
prefix on each key type, which ought to be unnecessary these days
since SSH-2 is a well established default. Only the SSH-1 RSA key type
is still labelled with an SSH version. (And I've moved it to the far
end rather than the start of the line, while I'm here.)
I've no reason to believe it will _currently_ be called with the
'passphrases' tree not even set up yet, but I managed to get that to
happen while playing about with experimental code just now, and it
seemed like a good safety check to keep in general.
I've written my own analogue of OpenSSH's ssh-askpass. At the moment,
it's contained inside Pageant proper, though it could easily be
compiled into a standalone binary as well or instead.
Unlike OpenSSH's version, I don't use a GTK edit box; instead I just
process key events myself and append them to a buffer. The big
advantage of doing this is that I can arrange for ^W and ^U to
function as they do in terminal line editing, i.e. delete a word or
delete the whole line.
^W in particular is really valuable when typing a multiple-word
passphrase unseen. If you feel yourself making the kind of typo in
which you're not sure if you pressed six keys or just five, you can
hit ^W and restart just that word, without either having to go right
back to the beginning or carry on and see if you feel lucky.
A delete-word function would of course be an information leak in even
an obscured edit box (displaying a blob per character), so instead I
give a visual acknowledgment of keypresses by a more ad-hoc means: I
display three lights in the box, and every meaningful keypress turns
off the currently active one and instead turns on a randomly selected
one of the others. (So the lit light doesn't even indicate _mod 3_ how
many keys have been pressed.)
I had freed the comment string coming back from pageant_add_keyfile,
but not NULLed out the pointer, so that the cleanup code at the end of
the function would have freed it again.
I've decided against implementing an option exactly analogous to
'ssh-add -L' (printing the full public key of everything in the
agent). Instead, you can identify a specific key to display in full,
by any of the same means -d lets you use, and then print it in either
of the public key formats we support.
There were ad-hoc functions for fingerprinting a bare key blob in both
cmdgen.c and pageant.c, not quite doing the same thing. Also, every
SSH-2 public key algorithm in the code base included a dedicated
fingerprint() method, which is completely pointless since SSH-2 key
fingerprints are computed in an algorithm-independent way (just hash
the standard-format public key blob), so each of those methods was
just duplicating the work of the public_blob() method with a less
general output mechanism.
Now sshpubk.c centrally provides an ssh2_fingerprint_blob() function
that does all the real work, plus an ssh2_fingerprint() function that
wraps it and deals with calling public_blob() to get something to
fingerprint. And the fingerprint() method has been completely removed
from ssh_signkey and all its implementations, and good riddance.
There was a fair amount of duplication between Windows and Unix
PuTTYgen, and some confusion over writing things to FILE * and
formatting them internally into strings. I think all the public-key
output code now lives in sshpubk.c, and there's only one copy of the
code to generate each format.
Unlike ssh-add, we can identify the key by its comment or by a prefix
of its fingerprint as well as using a public key file on disk. The
string given as an argument to -d is interpreted as whichever of those
things matches; disambiguating prefixes are available if needed.
Obviously PuTTY can't actually do public-key authentication itself, if
you give it a public rather than private key file. But it can still
match the supplied public key file against the list of keys in the
agent, and narrow down to that. So if for some reason you're
forwarding an agent to a machine you don't want to trust with your
_private_ key file (even encrypted), you can still use the '-i' option
to select which key from the agent to use, by uploading just the
public key file to that machine.
The rsakey_pubblob() and ssh2_userkey_loadpub() functions, which
expected to be given a private key file and load only the unencrypted
public half, now also cope with any of the public-only formats I know
about (SSH-1 only has one, whereas SSH-2 has the RFC 4716 format and
OpenSSH's one-line format) and return an appropriate public key blob
from each of those too.
cmdgen now supports this functionality, by permitting public key files
to be loaded and used by any operation that doesn't need the private
key: so you can convert back and forth between the SSH-2 public
formats, or list the file's fingerprint.
It's just ssh_pkt_addstring_data but using strlen to get the length of
string to add, so make that explicit by having it call
ssh_pkt_addstring_data. Good compilers should be unaffected by this
change.
You can now load keys at Pageant init time, by putting the key file
names as bare arguments on the command line, e.g. 'pageant -T key.ppk'
or 'pageant key.ppk --exec some command'; also, 'pageant -a key.ppk'
behaves more or less like ssh-add, contacting an existing agent to add
the key.
The askpass() function currently supports terminal-based prompting
only. X11 askpass is yet to be implemented.
This brings in the code we'll need to request passphrases from the
terminal, and to talk to an existing SSH agent as a client.
Adding uxcons.c required adjusting the set of stub functions in
uxpgnt.c: uxcons.c removed the need for several, but added one of its
own (log_eventlog). A net win, though.
I've moved the setup and running of the actual agent server into
run_agent(), so that main() is now only command-line parsing and
validation. We recognise a collection of new command-line options for
talking to an existing agent as a client (analogous to ssh-add), which
go to a new run_client() function, but I haven't filled in that
function itself yet.
Those must have been wrong _forever_, but because Windows Pageant
doesn't mind if the message length is longer than it should be, I've
never noticed before. How embarrassing.
This is only intended for use in Unix Pageant; for any application
that's actually trying to get something else useful done at the same
time as the agent request is pending, it's much more sensible to use
the more rigorous existing approach of requesting a callback once the
agent request is answered.
Adding this mode is the easiest way to allow Unix Pageant's
command-line key loading to work, but it doesn't solve the underlying
problem that the supposedly cross-platform pageant_add_keyfile will
not work on a platform where we really _are_ constrained to do agent
requests asynchronously (perhaps because we're a GUI app in some
system that doesn't let us control our own top-level event loop).
If and when that situation arises, I'll have no choice but to turn
pageant_add_keyfile and friends (specifically, any function in
pageant.c that calls agent_query) into coroutine-structured functions,
and have clients call them repeatedly until they return 'finished'.
But for now, this is a lot easier!
Now --exec instantly terminates option processing, by treating
everything after it as the command. This means it doesn't matter if
the --exec command word looks like another option, and it also means
we can simplify the handling of real non-option argument words, when I
get round to adding some for loading keys.
I've now centralised into pageant.c all the logic about trying to load
keys of any type, with no passphrase or with the passphrases used in
previous key-loading actions or with a new user-supplied passphrase,
whether we're the main Pageant process ourself or are talking to
another one as a client. The only part of that code remaining in
winpgnt.c is the user interaction via dialog boxes, which of course is
the part that will need to be done differently on other platforms.
This is intended to be a useful mode when you want to run an ssh agent
in a terminal session with no X11 available. You just execute a
command along the lines of eval $(pageant -T), and then Pageant will
run in the background for the rest of that terminal session - and when
the terminal session ends, so that Pageant loses its controlling tty,
it will take that as the signal to shut down. So, no need to manually
kill it, and unlike 'pageant --exec $SHELL', you can also do this half
way through a session if you don't realise until later that you need
an SSH agent, without losing any shell command history or other shell
context that you've accumulated so far in the session.
Unfortunately, I haven't been able to find any reliable way to
actually implement this -T mode, short of having Pageant wake up at
regular intervals and try to open /dev/tty to see if it's still there.
I had hoped that I could arrange to reliably get SIGHUP, or select on
/dev/tty for exceptional conditions, or some such, but nothing I've
tried along those lines seems to work.
The source paper, and OpenSSH, agree that the lowest bit index used
from the hash of the private key is bit 3, i.e. bits 0,1,2 at the
bottom are all zero. We were only clearing bits 0 and 1, which would
have worked for about half of keys. I must have got lucky during
testing!
When I implemented reading and writing of the new format a couple of
weeks ago, I kept them strictly separate in the UI, so you have to ask
for the format you want when exporting. But in fact this is silly,
because not every key type can be saved in both formats, and OpenSSH
itself has the policy of using the old format for key types it can
handle, unless specifically asked to use the new one.
So I've now arranged that the key file format enum has three values
for OpenSSH: PEM, NEW and AUTO. Files being loaded are identified as
either PEM or NEW, which describe the two physical file formats. But
exporting UIs present either AUTO or NEW, where AUTO is the virtual
format meaning 'save in the old format if possible, otherwise the new
one'.
This is the kex protocol id "curve25519-sha256@libssh.org", so called
because it's over the prime field of order 2^255 - 19.
Arithmetic in this curve is done using the Montgomery representation,
rather than the Weierstrass representation. So 'struct ec_curve' has
grown a discriminant field and a union of subtypes.
I'm finding missing constifications all over the place this week.
Turns out that dmemdump() has been taking a non-const memory pointer
ever since the beginning, and it's never come up until now. How silly.
The last use of it, to store the contents of the saved session name
edit box, was removed nearly two years ago in svn r9923 and replaced
by ctrl_alloc_with_free. The mechanism has been unused ever since
then, and I suspect any further uses of it would be a bad idea for the
same reasons, so let's get rid of it.
The memory dangling off ssd->sesslist should be freed when ssd itself
goes away, and the font settings ctrlset we delete in gtkcfg.c should
be freed as well once it's been removed from its containing array.
Thanks to Ranjini Aravind for pointing these out.
