The previous fix on pre-0.77 was non-disruptive and just enough to get
through my Coverity build (which uses winelib); but now that I look at
the rest of the Winelib build output, there are some further warnings
I should fix on main.
Most of them are more long/LONG confusion (specific to Winelib, rather
than real Windows); also, there's a multiple macro definition in
jump-list.c because Winelib defines _PROPVARIANT_INIT_DEFINED_ in
place of _PROPVARIANTINIT_DEFINED_ which we were testing for. (Bah.)
And in windows/window.c I used wcscmp without including <wchar.h>.
In spite of long vs LONG I still had to turn off one or two more
DLL-loading typechecks.
save_screenshot() returns a dynamically allocated error message in
case of failure, and Coverity complained of a memory leak when it was
ignored in putty.c.
The memory leak is trivial, because we were about to terminate the
process with an error anyway. But it's a good point that I forgot to
report the error!
Not critical enough to fix on 0.77 (where Coverity found it), but we
might as well make it look sensible on main.
In Winelib, you have to be careful not to say 'unsigned long' where
the API expects ULONG, because Winelib doesn't have the Windows LLP64
nature - its unsigned long is 64 bits, whereas ULONG is 32.
Also, my local Winelib has <dwmapi.h> (used in the new demo-screenshot
system), but doesn't contain some of the definitions inside it. So
I've expanded the cmake test of HAVE_DWMAPI_H so that it actually
checks the things we need, instead of just the existence of the
containing header.
Now we check that we can actually make an ssh_key out of it, and
moreover, that the key is of a sensible kind (i.e. not a certificate
in turn). If that's not true, we report something about the problem in
a new CTRL_TEXT below the public key input box. If the key _is_ valid,
that same text control is used to show its type, length and
fingerprint.
On Windows, I've widened the dialog box a little to make fingerprints
fit sensibly in it.
This is for cases where they're presenting information to the user
that wouldn't wrap sensibly anyway (such as an SSH key fingerprint
which is mostly all one word), and in which newlines might be
significant.
On GTK, the implementing widget is still a GtkLabel, but without the
wrap flag set, and wrapped in a GtkScrolledWindow in case the text is
too wide to fit.
On Windows, I've switched to using an edit box instead of a static
text control, making it readonly, and borderless via my existing
MakeDlgItemBorderless helper function. This doesn't get you an actual
scrollbar, but it does mean you can scroll left and right by dragging
with the mouse.
This doesn't apply to every GtkLabel I instantiate: only the ones
constructed as part of implementing the cross-platform CTRL_TEXT.
Those labels contain information that the dialog box is deliberately
communicating to the user, so it seems a sensible idea to make sure
they can be copy-pasted.
By default, this also seems to cause them to become able to take the
input focus, so I've reverted that. You can select them with the
mouse, but I think having them appear in the tab order is an
awkwardness too far, since they're not active in any other way.
This function has to make an array containing a specific number of
random values that are +1 or -1, and all the rest zero. The simplest
way I could think of to do it at first was to make the array with all
the zeroes at the end and then shuffle the array.
But I couldn't think of a time-safe algorithm to shuffle an array in
such a way that all orders come out equiprobable, that was better than
quadratic time. In fact I still can't think of one. (Making a random
Benes network is the best idea I've come up with: it arranges that
every output order is _possible_, and runs in O(N log N) time, but it
skews the probabilities, which makes it unacceptable.)
However, there's no need to shuffle an array in this application
anyway: we're not actually trying to generate a random _permutation_,
only a random element of (n choose w). So we can just walk linearly
along the array remembering how many nonzero elements we have yet to
output, and using an appropriately chosen random number at each step
to decide whether this will be one of them.
This isn't a significant improvement in the performance of NTRU
overall, but it satisfies my sense of rightness a little, and at least
means I don't have to have a comment in the code apologising for the
terrible algorithm any more.
Now the RSA signature-type checkboxes should be aligned with their
label; the 'Add' and 'Remove' buttons for wildcards should align with
the edit box for entering a wildcard; and the 'Load from file' button
for a public key aligns with the edit box for that.
Previously, in the code that instantiated the dialog.h portable
control spec, an edit control with width=100 would be implemented as a
small Columns object containing the label and the edit control atop
each other. Now, instead, the two controls are placed separately into
the containing Columns.
Combined with the changes just made to the align_next_to system, this
means that you can put buttons to the right of such an edit box and
have them line up with the actual edit box, instead of trying to line
up with the combination of the box and its label.
(The Windows alignment system already identified the specific edit box
control as the relevant one, so this was already working there.)
Various alignments I want to do in the host CA box have shown up
deficiencies in this system, so I've reworked it a bit.
Firstly, you can now specify more than two controls to be tied
together with an align_next_to (e.g. multiple checkboxes alongside
something else).
Secondly, as well as forcing the controls to be the same height as
each other, the layout algorithm will also move the later controls
further _downward_, so that their top y positions also line up. Until
now that hasn't been necessary, because they lined up already.
In the GTK implementation of this via the Columns class, I've renamed
'columns_force_same_height' to 'columns_align_next_to', and similarly
for some of the internal fields, since the latter change makes the
previous names a misnomer.
In the Windows implementation, I found it most convenient to set this
up by following a linked list of align_next_to fields backwards. But
it won't always be convenient to initialise them that way, so I've
also written a crude normaliser that will rewrite those links into a
canonical form. But I only call that on Windows; it's unnecessary in
GTK, where the Columns class provides plenty of per-widget extra
storage so I just keep each alignment class as a circular list.
Apparently I never re-tested that option when I revamped Pageant's
command-line option parsing in commit dc183e1649, because it's
now off by one in figuring out which argument to treat as the start of
the command to be run.
(The new code in that commit is the same shape as the old code but
with variables renamed, and that was the mistake, because in the old
code, the argument index i pointed to the -c option, whereas in the
new code, match_opt has already advanced amo.index to the next word.
So the two index variables _shouldn't_ be treated the same.)
When we linked a new entry on to the global request queue, we forgot
to set its next pointer to NULL, so that when it was removed again,
s->globreq_head could end up pointing to nonsense.
In addition, even if the next pointer happened to be NULL by luck, we
also did not notice that s->globreq_head had become NULL and respond
by nulling out s->globreq_tail, which would leave s->globreq_tail as a
stale pointer to the just-freed list element, causing a memory access
error on the next attempt to link something on to the list.
This could come up in the situation where you open Change Settings and
configure a remote port forwarding, close it (so that the global
request is sent, queued, replied to, and unqueued again), and then
reopen Change Settings and configure a second one (so that the linked
list in the confused state actually gets used).
Introduced in dc7ba12253 earlier today. On GTK these caused no
problems worse than a GTK warning, but I'd better fix them before they
(potentially) do worse on Windows!
As distinct from the type of signature generated by the SSH server
itself from the host key, this lets you exclude (and by default does
exclude) the old "ssh-rsa" SHA-1 signature type from the signature of
the CA on the certificate.
This will allow the central host_ca_new function to pre-populate the
structure with default values for the fields, so that once I add more
options to CA configuration they can take their default values when
loading a saved record from a previous PuTTY version.
I mostly really like the use of 'ptrlen' in place of zero-terminated
strings in PuTTY, but one place it's awkward is when debuggging
through string-handling code, because gdb won't automatically show me
exactly what a ptrlen points to. Now it does.
This means that, on the one hand, an absentminded press of Return
doesn't dismiss the entire CA config box, which would be pretty
annoying if you were half way through entering a load of fiddly stuff.
And on the other hand, you _can_ press Escape to dismiss the box,
which is less likely to happen by accident.
This allows you to load a CA public key from a disk file (in any
format acceptable to ppk_load_pub, which means OpenSSH one-line public
keys and also RFC4716 ones).
In the course of polishing up this dialog box, I'm going to want it to
actually do cryptographic things (such as checking validity of a
public key blob and printing its fingerprint), which means it will
need to link against SSH utility functions.
So I've moved the dialog-box setup and handling code out of config.c
into a new file in the ssh subdirectory and in the ssh library, where
those facilities will be conveniently available.
This also means that dialog-box setup code _won't_ be linked into
PuTTYtel or pterm (on either platform), so I've added a stub source
file to provide its entry-point function in those tools. Also,
provided a const bool to indicate whether that dialog is available,
which we use to decide whether to recognise that command-line option.
Instead of an edit box together with a Browse button that pops up a
sub-dialog, this is _just_ the browse button, only now it has a
user-defined title. I'm about to want to use this for loading CA
public keys from files.
This causes PuTTY to bring up just the host CA configuration dialog
box, and shut down once that box is dismissed.
I can imagine it potentially being useful to users, but in the first
instance, I expect it to be useful to _me_, because it will greatly
streamline testing changes to the UI of that dialog!
This gets rid of that awkward STANDARD_PREFIX system in which every
branch of the old 'union control' had to repeat all the generic
fields, and then call sites had to make an arbitrary decision about
which branch to access them through.
That was the best we could do before accepting C99 features in this
code base. But now we have anonymous unions, so we don't need to put
up with that nonsense any more!
'dlgcontrol' is now a struct rather than a union, and the generic
fields common to all control types are ordinary members of the struct,
so you don't have to refer to them as ctrl->generic.foo at all, just
ctrl->foo, which saves verbiage at the point of use.
The extra per-control fields are still held in structures named after
the control type, so you'll still say ctrl->listbox.height or
whatever. But now those structures are themselves members of an
anonymous union field following the generic fields, so those
sub-structures don't have to reiterate all the standard stuff too.
While I'm here, I've promoted 'context2' from an editbox-specific
field to a generic one (it just seems silly _not_ to allow any control
to have two context fields if it needs it). Also, I had to rename the
boolean field 'tabdelay' to avoid it clashing with the subsidiary
structure field 'tabdelay', now that the former isn't generic.tabdelay
any more.
I'm about to change my mind about whether its top-level nature is
struct or union, and rather than change the key word 'union' to
'struct' at every point of use, it's nicer to just get rid of the
keyword completely. So it has a shiny new name.
In a rekey, we expect to see the same host key again, which we enforce
by comparing its cache string, which we happened to have handy. But
certified host keys don't have cache strings, so this no longer works
reliably - the 'assert(s->keystr)' fails.
(This is what I get for making a zillion short-lived test connections
and not leaving any of them running for more than 2 minutes!)
Instead, we now keep the official public blob of the host key from the
first key exchange, and compare that to the public blob of the one in
the rekey.
prepare_session() is the function that takes a Conf in the form it was
loaded from the configuration, and normalises it into the form that
backends can make sense of easily. In particular, if CONF_host
contains something like "user@hostname", this is the place where the
"user@" is stripped off the hostname field and moved into
CONF_username where the backend will expect to find it.
Therefore, you're _always_ supposed to call prepare_session() before
launching a backend from a Conf you (potentially) got from a saved
session. But the SSH proxy code forgot to.
As a result, if you had a saved session with "user@hostname" in the
hostname field, it would work fine to launch that session directly in
PuTTY, but trying to use the same saved session as an SSH proxy would
fail mysteriously after trying to pass "user@hostname" to
getaddrinfo() and getting nothing useful back.
(On Windows, the error message is especially opaque: an invalid string
of that kind generates an error code that we weren't even tranlsating.
And even if we _do_ translate it, it wouldn't be very meaningful,
because the error in question is WSANO_RECOVERY, which FormatMessage
renders as "A non-recoverable error occurred during a database
lookup." I guess the error in question was that Windows couldn't even
figure out how to translate that string into the format of a DNS
request message!)
In the initial version of SSH proxying that only opened direct-tcpip
channels, this wasn't important. But as of commit 6f7c52dcce, we
now support invoking a command or subsystem on the proxy SSH server,
and those _can_ generate stderr data which we must now separate from
stdout.
Happily, we have a perfectly sensible thing to _do_ with it: the same
thing we'd do with stderr coming from a local proxy subprocess, to
wit, pass it to log_proxy_stderr so that it can appear in the terminal
window (if configured to) and the Event Log.
I just tried to trace through the Windows version's control flow in
response to a confusing bug report, and found that the control flow
itself was so confusing I couldn't make sense of it. Why are we
choosing between getaddrinfo and gethostbyname via #ifndef NO_IPV6,
then re-converging control flow and diverging a second time to report
the error?
So I rewrote the whole thing to have completely separate sections of
code dealing with the three resolution strategies, each with its own
dedicated error reporting system. And then I checked the Unix version
and found it was about as confusing, so I rewrote that too in the same
style. Now the two are mostly the same, except for details: Unix has
an override at the top for a Unix socket pathname, Windows has to cope
with getaddrinfo maybe not being found at run time (so the other cases
aren't in the #else clause), and Windows uses the same error reporting
for both lookup functions whereas Unix has to use the appropriate
gai_strerror or hstrerror.
To begin with, Windows's own API documentation doesn't recommend using
it (for thread-safety reasons), and promises that the error codes
returned from getaddrinfo are aliases for the normal Windows error
code enumeration. So it's safe, and quite likely preferable, to just
use ordinary win_strerror instead.
But more embarrassingly, my attempt to acquire and use gai_strerror
from one or other Winsock DLL didn't even *work*! Because of course
it's a function that handles strings, which means it comes in two
variants, gai_strerrorA and gai_strerrorW, so in order to look it up
using GetProcAddress, I should have specified which I wanted. And I
didn't, so the lookup always failed.
This should improve error reporting in cases of interesting kinds of
DNS failure.
We're assigning string literals into it all over the place, so it
should have been const char * all along. No thanks to any of the
compilers that didn't point that out!
It's a thoroughly pedantic point, but I just spotted that the
comparison of wire data against theoretically platform-dependent char
escapes is a violation of \k{udp-portability}.
In commit 7d44e35bb3 I introduced a bug: we were providing an
array of MAXKEXLIST ints to ssh2_scan_kexinits() to write a list of
server-supplied host keys into, and when MAXKEXLIST stopped being a
thing, I mindlessly replaced it with an array dynamically allocated to
the number of host key types we'd offered the server.
But we return a list of host key types the _server_ offered _us_ (and
that we can speak at all), which isn't necessarily the same thing. In
particular, if you deliberately ask to cache a new host key type from
the specials menu, we send a KEXINIT offering just _one_ host key
type, namely the one you've asked for. But that loop still writes down
all the key types it gets back from the server, which is (almost
certainly) more than one. So the array overflows.
In that situation we don't really need the returned array of key types
at all, but it's easier to just make it work than to add conditionals.
Replaced it with a dynamically grown array in the usual sort of way.
As well as eliminating the null-pointer dereference, I also now
realise that the format-translation code depended on leaving the final
translated string in 'otherstr' in order to pass the host key check
afterwards (if they match).
I've also now realised that this only applies to *SSH-1* RSA keys, so
it's even more obsolete than I thought before. Perhaps I should just
remove this code instead of spending all this effort on fixing it. But
I've done the fix now, so I'll commit it, and then maybe we can remove
it afterwards (and have a working version of it available to resurrect
if ever needed!).
A user points out that in commit 6143a50ed2, when I converted all
use of the registry to functions that return a newly allocated buffer
instead of allocating a buffer themselves beforehand, I overlooked
that one use of the old idiom was reusing the preallocated buffer as
work space.
I _hope_ nobody still needs this code - the 'old-style' host key cache
format it handles was replaced in 2000. If anyone has a PuTTY host key
cache entry that's survived 22 years without either having to be
reinitialised on a new system or changed when the machine's host key
was upgraded, they're doing better than I am!
But if it's still here, it should still work, obviously. Replaced the
reused buffer with a strbuf, which is more robust anyway.
Initial live testing pointed out that the ssh_keyalg corresponding to
the certified version of rsa-sha2-512 was expecting to see the SSH id
string "rsa-sha2-512-cert-v01@openssh.com" at the start of the public
key blob, whereas in fact, the _key_ type identifier is still
"ssh-rsa-...", just as the key type for base rsa-sha2-512 is base
ssh-rsa.
Fixed inside openssh-certs.c, by adding a couple more strings to the
'extra' structure.
As of df3a21d97b, this panel has two "Browse..." buttons, and is the
first to do so. On Windows, the shortcut for this file chooser button
was hardcoded to 'w', causing an assertion failure whenever switching to
the Auth panel.
I've just removed the shortcut -- there are others near enough that it's
easy to reach the "Browse..." button with Tab.
This uses the test-CA code to construct a series of certificates with
various properties so as to check all the error cases of certificate
validation. It also tests the various different key types, and all the
RSA signature flags on both the certified key and the certifying one.
This is mostly intended to be invoked from cryptsuite, so that I can
make test certificates with various features to check the validation
function. But it also has a command-line interface, which currently
contains just enough features that I was able to generate a
certificate and actually make sure OpenSSH accepted it (proving that I
got the format right in this script).
You _could_ expand this script into a full production CA, with a
couple more command-line options, if you didn't mind the slightly
awkward requirement that in command-line mode it insists on doing its
signing via an SSH agent. But for the moment it's only intended for
test purposes.
