Some new cert-related stuff wasn't documented in the usage message
and/or man page; and the longer-standing "-E fptype" was entirely
omitted from the usage message.
As of the cyclic-dependency fix in b01173c6b7, building from our tarball
using the instructions in its README (using the source tree as build
tree), in the absence of Halibut, would lead to the pre-built man pages
not being installed.
(Also, a load of "Could not build man page" complaints at cmake
generation time, which is how I actually noticed.)
In some compilers (I'm told clang 10, in particular), the NEON
intrinsic vaddq_p128 is missing, even though its input type poly128_t
is provided.
vaddq_p128 is just an XOR of two vector registers, so that's easy to
work around by casting to a more mundane type and back. Added a
configure-time test for that intrinsic, and a workaround to be used in
its absence.
It turns out this isn't actually necessary after all to make the
installer behave in the expected way in the default case (giving a UAC
prompt and installing systemwide). And I'm told it has undesirable
consequences in more complicated cases, which I'm not expert enough in
MSI to fully understand.
In commit 732ec31a17 I made the check for libX11 conditional on
GTK - but I forgot that if we're building without GTK, I should
_define_ NOT_X_WINDOWS, rather than leaving it undefined. As a result,
the build would fail on files like unix/utils/x11_ignore_error.c.
On FreeBSD, the GTK libraries aren't stored on the standard library
path, so pkg-config has to emit a -L option as well as -l options.
This worked fine during the main build, but the -L option wasn't being
passed through to check_symbol_exists() for the tests of Pango API
function availability.
I still haven't got out of the habit of doing this the autotools way,
which doesn't work in cmake. cmake's HAVE_FOO variables are always
defined, and they take values 0 or 1, so testing them with 'defined'
will return the wrong value.
FreeBSD declares setpgrp() as taking two arguments, like Linux's
setpgid(). Detect that at configure time and adjust the call in
Pageant appropriately.
If you have GTK installed on your system but want to build without it
anyway (e.g. if you're buliding a package suitable for headless
systems), it's useful to be able to explicitly instruct PuTTY's build
system not to use GTK even if it's there.
This would already work if you unilaterally set PUTTY_GTK_VERSION to
some value other than 1, 2, 3 or ANY. Added NONE as an officially
supported option, and included it in the list that cmake-gui will
present.
Also, made the check for libX11 conditional on having GTK, since
there's no need to bother with it otherwise.
This was pointed out by another compiler warning. The 'name' parameter
of inquire_cred_by_mech is not a gss_name_t (which is the type of
GSS_C_NO_NAME); it's a gss_name_t *, because it's an _output_
parameter. We're not telling the library that we aren't _passing_ a
name: we're telling it that we don't need it to _return_ us a name. So
the appropriate null pointer representation is just NULL.
(This was harmless apart from a compiler warning, because gss_name_t
is a pointer type in turn and GSS_C_NO_NAME expands to a null pointer
anyway. It was just a wrongly-typed null pointer.)
Heimdal provides its own definitions of OIDs like GSS_C_NT_USER_NAME
in the form of macros, which conflict with our attempt to redefine
them as variables - the macro gets expanded into the middle of the
variable declaration, leaving the poor C compiler trying to parse a
non-declaration along the lines of
const_gss_OID (&__gss_c_nt_anonymous_oid_desc) = oids+5;
Easily fixed by just not redefining these at all if they're already
defined as macros. To make that easier, I've broken up the oids[]
array into individual gss_OID_desc declarations, so I can put each one
inside the appropriate ifdef.
In the process, I've removed the 'const' from the gss_OID_desc
declarations. That's on purpose! The problem is that not all
implementations of the GSSAPI headers make const_gss_OID a pointer to
a *const* gss_OID_desc; sometimes it's just a plain one and the
'const' prefix is just a comment to the user. So removing that const
prevents compiler warnings (or worse) about address-taking a const
thing and assigning it into a non-const pointer.
When linking statically against Kerberos, the setup code in
ssh_got_ssh_version() was trying to look up want_id==0 in the list of
one GSSAPI library, but unfortunately, the id field of that record was
not initialised at all, so if it happened to be nonzero nonsense, the
loop wouldn't find a library at all and would fail an assertion.
On FreeBSD, I'm told, you can't configure Kerberos via pkg-config. So
we need a fallback. Here's some manual code to run krb5-config and
pick apart the result, similar to what I already did with gtk-config
for our (still not dead!) GTK 1 support.
Of the three calls to queue_toplevel_callback in window.c, one of them
was still passing NULL as its context parameter, rather than the new
'wgs'. As a result, a segfault could occur during some session closures.
The char buffer 'blob' allocated in read_blob was never actually used
for anything, so there was no need to allocate it - and also no need
for the assert() just before it, which was added in commit
63a58759b5 to be extra safe against integer overflow when
computing the buffer size.
I feel a bit silly for having added that assertion in the first place
rather than removing the allocation it was protecting! It was
unnecessary even then, and I completely failed to notice :-)
I mentioned recently (in commit 9e7d4c53d8) message that I'm no
longer fond of the variable name 'ret', because it's used in two quite
different contexts: it's the return value from a subroutine you just
called (e.g. 'int ret = read(fd, buf, len);' and then check for error
or EOF), or it's the value you're preparing to return from the
_containing_ routine (maybe by assigning it a default value and then
conditionally modifying it, or by starting at NULL and reallocating,
or setting it just before using the 'goto out' cleanup idiom). In the
past I've occasionally made mistakes by forgetting which meaning the
variable had, or accidentally conflating both uses.
If all else fails, I now prefer 'retd' (short for 'returned') in the
former situation, and 'toret' (obviously, the value 'to return') in
the latter case. But even better is to pick a name that actually says
something more specific about what the thing actually is.
One particular bad habit throughout this codebase is to have a set of
functions that deal with some object type (say 'Foo'), all *but one*
of which take a 'Foo *foo' parameter, but the foo_new() function
starts with 'Foo *ret = snew(Foo)'. If all the rest of them think the
canonical name for the ambient Foo is 'foo', so should foo_new()!
So here's a no-brainer start on cutting down on the uses of 'ret': I
looked for all the cases where it was being assigned the result of an
allocation, and renamed the variable to be a description of the thing
being allocated. In the case of a new() function belonging to a
family, I picked the same name as the rest of the functions in its own
family, for consistency. In other cases I picked something sensible.
One case where it _does_ make sense not to use your usual name for the
variable type is when you're cloning an existing object. In that case,
_neither_ of the Foo objects involved should be called 'foo', because
it's ambiguous! They should be named so you can see which is which. In
the two cases I found here, I've called them 'orig' and 'copy'.
As in the previous refactoring, many thanks to clang-rename for the
help.
In the course of recent refactorings I noticed a couple of cases where
we were doing old-fashioned preallocation of a char array with some
conservative maximum size, then writing into it via *p++ or similar
and hoping we got the calculation right.
Now we have strbuf and dupcat, so we shouldn't ever have to do that.
Fixed as many cases as I could find by searching for allocations of
the form 'snewn(foo, char)'.
Particularly worth a mention was the Windows GSSAPI setup code, which
was directly using the Win32 Registry API, and looks much more legible
using the windows/utils/registry.c wrappers. (But that was why I had
to enhance them in the previous commit so as to be able to open
registry keys read-only: without that, the open operation would
actually fail on this key, which is not user-writable.)
Also unix/askpass.c, which was doing a careful reallocation of its
buffer to avoid secrets being left behind in the vacated memory -
which is now just a matter of ensuring we called strbuf_new_nm().
These handy wrappers on the verbose underlying Win32 registry API have
to lose some expressiveness, and one thing they lost was the ability
to open a registry key without asking for both read and write access.
This meant they couldn't be used for accessing keys not owned by the
calling user.
So far, I've only used them for accessing PuTTY's own saved data,
which means that hasn't been a problem. But I want to use them
elsewhere in an upcoming commit, so I need to fix that.
The obvious thing would be to change the meaning of the existing
'create' boolean flag so that if it's false, we also don't request
write access. The rationale would be that you're either reading or
writing, and if you're writing you want both RW access and to create
keys that don't already exist. But in fact that's not true: you do
want to set create==false and have write access in the case where
you're _deleting_ things from the key (or the whole key). So we really
do need three ways to call the wrapper function.
Rather than add another boolean field to every call site or mess about
with an 'access type' enum, I've taken an in-between route: the
underlying open_regkey_fn *function* takes a 'create' and a 'write'
flag, but at call sites, it's wrapped with a macro anyway (to append
NULL to the variadic argument list), so I've just made three macros
whose names request different access. That makes call sites marginally
_less_ verbose, while still
dh_is_gex() expects to find a 'struct dh_extra' in the 'extra' field
of the kex_alg you pass in, and won't look kindly on finding an
instance of some totally different structure type. We were being
careful about that everywhere in the GSSAPI kex code except for the
final free step.
The conditionalisation of that call on 'protocol == PROT_SSH' has been
around since the beginning of our git history. But in those days,
random_save_seed() was unconditional _internally_ - it would always
create and write to the seed file regardless of whether the random
pool had even been initialised, let alone used.
Now random_save_seed() has its own internal condition which prevents
it doing anything if the random subsystem was never started up in the
first place. So it's better to call it unconditionally from
cleanup_exit, and then it'll be able to do its thing whenever needed,
without having to second-guess based on the top-level protocol.
(In fact, that's what all the other implementations of cleanup_exit()
have done all along. On Unix, and in Windows console apps, we do call
random_save_seed() unconditionally, and expect it to uncomplainingly
do nothing if there's nothing to do.)
(cherry picked from commit 260aad5fca)
In cases where we refuse a resize request, either because it's too
large or because the window is not currently resizable due to being
maximised, we were failing to communicate that back to the Terminal so
that it could stop waiting for the resize and resume processing input.
Those have been there since around 2001. They're in a piece of code
that calls get_fullscreen_rect to find the overall screen size, and
then prevents attempts to resize the window larger than that. The
static variables were arranging that we don't have to call
get_fullscreen_rect more than once.
But, firstly, computers are faster 20 years on; secondly, remote
window-resize requests are intentionally rate-limited (as of commit
d74308e90e), so this shouldn't be the limiting factor anyway; and
thirdly, multi-monitor support has appeared since then, which means
that if the window has been dragged from one monitor to another then
get_fullscreen_rect might _legitimately_ return a different bounding
rectangle when called a second time.
So we should just do the full check every time unconditionally.
(cherry picked from commit 4b3a8cbf61)
This is a piece of refactoring that's been overdue forever. In the
Unix front end, all the variables specific to a particular SSH session
live in a big 'inst' structure, and calls to any of the trait APIs
like Seat or TermWin can recover the right 'inst' from their input
context pointer. But the Windows frontend was written before that kind
of thing became habitual to me, and all its variables have been just
lying around at the top level of window.c, and most of those context
pointers were just ignored.
Now I've swept them all up and put them where they ought to be, in a
big context structure. This was made immeasurably easier by the very
nifty 'clang-rename' tool, which can rename a single variable in a C
source file, and find just the right set of identifiers to change even
if other variables in the file have the same name, even in sub-scopes.
So I started by clang-renaming all the top-level variables in question
to have names beginning with a prefix that didn't previously appear
anywhere in the code; checked that still worked; and then moved all
the declarations into the struct type and did a purely textual
search-and-replace of the prefix with 'wgs->'.
One potential benefit of this change is that it allows more than one
instance of the WinGuiSeat structure to exist in the same process. I
don't have any immediate plans to actually do that, but it's nice to
know it wouldn't be ruled out if we ever did need it.
But that's not the main reason I did it. The main reason is that I
recently looked at the output of a Windows build using clang -Wall,
and was horrified by the number of casual shadowings of
generically-named global variables like 'term' and 'conf' with local
variables of the same name. That seemed like a recipe for confusion,
and I decided the best way to disambiguate them all was to do this
refactoring that I'd wanted anyway for years.
A few uses of the global variables occurred in functions that didn't
have convenient access to the right WinGuiSeat via a callback
parameter of some kind. Those had to be treated specially. Most were
cleaned up in advance by the previous few commits; the remaining fixes
in this commit itself were in functions like modalfatalbox(),
nonfatal() and cleanup_exit(). The error reporting functions want the
terminal HWND to use as a MessageBox parameter; they also have the
side effect of un-hiding the mouse pointer in the terminal window, in
case it's hidden; and cleanup_exit wanted to free some resources
dangling off the WinGuiSeat.
For most of those cases, I've made a linked list of all currently live
WinGuiSeat structures, so that they can loop over _all_ live instances
(whether there's one as usual, none, or maybe more than one in
future). The parent window for non-connection-specific error messages
is found by simply picking one arbitrarily off the linked list (if
any); the cleanups are done by iterating over the _whole_ list.
The mouse-pointer unhiding is dealt with by simply allowing
show_mouseptr to take a _null_ WinGuiSeat pointer. The only thing it
needs the context for at all is to check whether pointer-hiding is
enabled in the session's Conf; so if we're _un_-hiding the pointer we
don't need to check, and can unhide it unconditionally.
The remaining global variables in window.c are the new linked list of
all WinGuiSeat structures; 'wm_mousewheel' and 'trust_icon' which
really should be global across all WinGuiSeats even if we were to have
more than one; and there's a static variable 'cursor_visible' in
show_mouseptr() which is likewise legitimately Seat-independent (since
it records the last value we passed to the Win32 API ShowCursor
function, and _that's_ global across the whole process state).
All of this should cause no functional change whatsoever.
Those have been there since around 2001. They're in a piece of code
that calls get_fullscreen_rect to find the overall screen size, and
then prevents attempts to resize the window larger than that. The
static variables were arranging that we don't have to call
get_fullscreen_rect more than once.
But, firstly, computers are faster 20 years on; secondly, remote
window-resize requests are intentionally rate-limited (as of commit
d74308e90e), so this shouldn't be the limiting factor anyway; and
thirdly, multi-monitor support has appeared since then, which means
that if the window has been dragged from one monitor to another then
get_fullscreen_rect might _legitimately_ return a different bounding
rectangle when called a second time.
So we should just do the full check every time unconditionally.
This is like the seat-independent nonfatal(), but specifies a Seat,
which allows the GUI dialog box to have the right terminal window as
its parent (if there are multiple ones).
Changed over all the nonfatal() calls in the code base that could be
localised to a Seat, which means all the ones that come up if
something goes horribly wrong in host key storage. To make that
possible, I've added a 'seat' parameter to store_host_key(); it turns
out that all its call sites had one available already.
Previously, the timing.c subsystem worked in Windows PuTTY by means of
scheduling WM_TIMER messages to be sent to the terminal window. Now it
uses a separate hidden window instead, and all the machinery for
handling that window lives on its own in windows/utils/gui-timing.c.
Most immediately, this removes a use of wgs.term_hwnd that will become
awkward when I move that structure in a following commit. But also, it
will make it easier to add the same timing subsystem to unrelated GUI
programs, such as Windows Pageant: if we ever decide to implement
automatic deletion or re-encryption of unused keys after a timeout,
this will help make that easier.
That clipboard-writing function is called just once, from the Event
Log dialog procedure, for when the user deliberately copies to the
clipboard. That call always passes must_deselect = true, which means
the conditional WM_IGNORE_CLIP messages are not sent. So it's simpler
to remove that parameter completely, and the conditional calls which
are never used.
Also, the clipboard data copied from the Event Log dialog is being put
in the clipboard associated with the main PuTTY terminal window. But
anything else we copy from a dialog box using Windows's built-in
copy-paste mechanisms would surely be associated with the _dialog_,
not its parent window. So we should do the same thing here. Therefore,
I've added a HWND parameter to write_aclip() and used that in place of
wgs.term_hwnd, so that we can pass in the HWND of the dialog itself.
The conditionalisation of that call on 'protocol == PROT_SSH' has been
around since the beginning of our git history. But in those days,
random_save_seed() was unconditional _internally_ - it would always
create and write to the seed file regardless of whether the random
pool had even been initialised, let alone used.
Now random_save_seed() has its own internal condition which prevents
it doing anything if the random subsystem was never started up in the
first place. So it's better to call it unconditionally from
cleanup_exit, and then it'll be able to do its thing whenever needed,
without having to second-guess based on the top-level protocol.
(In fact, that's what all the other implementations of cleanup_exit()
have done all along. On Unix, and in Windows console apps, we do call
random_save_seed() unconditionally, and expect it to uncomplainingly
do nothing if there's nothing to do.)
I don't know why I never bothered to write it down before, but it's a
good idea to let a pre-release build actually *happen* between turning
them on and updating the website to claim they exist.
Also, for the first time ever, I've just sent out an announcement
email for the 0.78 *pre-releases*, soliciting testing in advance of
the actual release. So, add that to the checklist as well.
We withdrew our FTP download links in July, when chiark's OS upgrade
made its previous ftpd go away. We've had no complaints at all about
that, so I think it's time to decide that FTP is officially obsolete,
and remove it from the script that does the uploads, and from the
release checklist.
In the initial commit 031d86ed5b that introduced them, I
accidentally put them below the 'warn about insecurity' line, which I
didn't mean to. Moved them up to just above the existing group14.
Also, I've arranged them in a slightly weird order, so that the most
preferred group of this collection is the medium-sized group16,
followed by the larger ones (17 and 18) and then the smaller 15.
Rationale: larger is better _until_ it starts costing way too much CPU
time, and group18 can grind quite painfully on a slow machine. (And of
course users are free to reconfigure if they have different
preferences.)
This isn't really ideal, of course. The idea that you might not want
to use group18 *because it's slow* contradicts the basic concept of
PuTTY's current crypto-preferences UI, which assumes that you rank
things by security, which is why there's a dividing line below which
things are assumed insecure. I hope that in a future release we'll
rework the UI so that you can express more subtle ideas of what crypto
you do and don't like. But this will do for the moment.
The GSS versions of the same DH methods are reordered similarly.
Jacob points out that the output of 'puttygen --dump', where the
key_components are used, is much more likely to need to be machine-
than human-readable, and so it makes more sense to use a date/time
format that's invariant under external changes such as locale.
(He also points out that Windows's time zone description strings are
overly verbose!)
With the new larger fixed-group methods, it's less clearly always the
right answer. (Really it seems more sensible to use ECDH over any of
the integer DH, these days.)
Also, reword other kex descriptions a bit.
Simon tells me he was pondering whether chacha20-poly1305 could be
reworked to use the new facilities, but on reflection there's no way to
use it to improve matters.
This went missing in the migration to CMake, and broke compatibility
of the standard 32-bit builds with Windows XP. (Of course, the
'buildold' versions should still have run.)
There doesn't seem to be a convenient CMake option to configure it
cleanly, so I had to do a bodgy string-replace on the variable
containing the linker flags, which I found by source-diving in CMake.
That's fragile enough that I've also put in a check after the fact, so
that we'll find out if it ever stops working.
