In commit 9cc586e605 I changed the low-level key-file reading
routines like read_header and read_body so that they read from a
BinarySource via get_byte(), rather than from a FILE * via fgetc. But
I forgot that the two functions don't signal end-of-file the same way,
so testing the return value of get_byte() against EOF is pointless and
will never match, and conversely, real EOF won't be spotted unless you
also examine the error indicator in the BinarySource.
As a result, a key file that ends without a trailing newline will
cause a tight loop in one of those low-level read routines.
(cherry picked from commit d008d235f3)
Since ca9cd983e1, changing colour config mid-session had no effect
(until the palette was reset for some other reason). Now it does take
effect immediately (provided that the palette has not been overridden by
escape sequence -- this is new with ca9cd983e1).
This changes the semantics of palette_reset(): the only important
parameter when doing that is whether we keep escape sequence overrides
-- there's no harm in re-fetching config and platform colours whether or
not they've changed -- so that's what the parameter becomes (with a
sense that doesn't require changing the call sites). The other part of
this change is actually remembering to trigger this when the
configuration is changed.
(cherry picked from commit 1e726c94e8)
A user pointed out that once we've identified the key algorithm from
an apparent public-key blob, we call ssh_key_new_pub on the blob data
and assume it will succeed. But there are plenty of ways it could
still fail, and ssh_key_new_pub could return NULL.
(cherry picked from commit 0c21eb4447)
I was cleaning up the 'struct handle', but not the underlying HANDLE.
As a result, any PuTTY process that makes a request to Pageant keeps
the named pipe connection open until the end of the process's
lifetime.
(cherry picked from commit 6e69223dc2)
I was checking a HANDLE against INVALID_HANDLE_VALUE to decide whether
it should be closed. But ten lines further up, I was setting it
manually to NULL to suppress the close. Oops.
(cherry picked from commit 155d8121e6)
Less than 12 hours after 0.75 went out of the door, a user pointed out
that enabling the 'Use system colours' config option causes an
immediate NULL-dereference crash. The reason is because a chain of
calls from term_init() ends up calling back to the Windows
implementation of the palette_get_overrides() method, which responds
by trying to call functions on the static variable 'term' in window.c,
which won't be initialised until term_init() has returned.
Simple fix: palette_get_overrides() is now given a pointer to the
Terminal that it should be updating, because it can't find it out any
other way.
(cherry picked from commit 571fa3388d)
Since the previous commit is causing an RC2 build of 0.75 anyway,
let's take the opportunity to bring in updates to the docs from main,
so that the release will have the most up-to-date version available.
This is a combined cherry-pick of:
f6142ba29b7c1bea59a3f5d1d4ce4b
Jacob spots that on Windows, current PuTTY is not compatible with
0.74, if one of them acts as a connection sharing upstream and the
other as a downstream. That's because commit 1344d4d1cd
accidentally changed the hash preimage in capi_obfuscate_string() so
that it no longer had an SSH-like string length field at the front. So
the two versions of PuTTY will expect the named pipe to have a
different pathname, and so they won't be able to find each other.
Interoperation between PuTTY versions is not the most important use
case of connection sharing - surely the typical user will invoke it by
activating the same session twice, or by using Duplicate Session. But
it was never intended to deliberately _not_ work, so let's fix it
before 0.75 goes out, so that at least the incompatible behaviour will
only ever have appeared in development snapshots.
I've recently been coming round in general to the idea that -Werror is
fine for developers and centralised binary builds, but has too many
unanticipated failure modes in the field (with everyone's different
versions of compilers, headers etc) to leave turned on for the 'just
download and build' source tarball that's supposed to work everywhere.
On main, I've already made the change to hide it behind a cmake
'strict' setting.
In particular, I've just done pre-release build tests with various
versions of GTK, which reminded me that the GTK 2 installation on
Ubuntu 20.04 fails to build at -Werror, because GTK's own header files
have a warning-generating inconsistency. (glib/gtypes.h declares
GTimeVal as deprecated, and then gtk/gtktooltips.h uses it anyway.)
Clearly this is the kind of thing that ought not to break the build of
a client application!
GetFileType() takes a HANDLE, not a pathname. So passing it the
pathname of the agent named pipe would never have worked at all.
I hadn't noticed, because the only call to that function logical-ORs
its return value with that of wm_copydata_agent_exists(), and the
latter _does_ work.
So if you're running true Pageant, which presents both IPC interfaces,
then there's no problem. But if a Pageant-emulating system wanted to
present only the named-pipe version, then we wouldn't have detected
it. Now we should do.
I just discovered that they weren't happening, and the reason why is
thoroughly annoying. Details are in the long comment I've added to the
WM_VSCROLL handler in WndProc, but the short version is that when you
interactively drag the terminal window's scrollbar, a subsidiary
message loop is launched by DefWndProc, causing all our timer events
to go missing until the user lets go of the scrollbar again. So we
have to manually update the terminal window on scroll events, because
the normal system is out of action.
I assume this changed behaviour round about the big rework of terminal
updating in February. Good job I spotted it just _before_ 0.75, and
not just after!
Previously, 'make install' would install psusan itself in .../bin, but
not install psusan.1 in .../share/man/man1. That's not a sensible
combination. Either it's a test utility so we should install neither,
or it's a fully supported official utility so we should install both.
It's the latter. Man page is now installed, along with the binary.
I've just spent the afternoon playing with it (rather belatedly - this
is the first time I've tried it out since it was first announced!),
and quickly decided that on the one hand it looks quite useful, but on
the other hand, running it in a Windows console is not for me and I'd
prefer to talk to it via PuTTY and psusan, for nicer copy-paste
controls and the ability to forward Pageant into it.
That turns out to be very easy and (I think) useful, so in it goes as
another psusan use case.
As we do in other similar situations. (The resulting passphrase dialog
is annoyingly unsymmetric, but probably less annoying than a Help
button which does nothing, and the situation shouldn't arise with our
standard builds.)
Suggested by Jacob: if this dialog box is going to pop up
_unexpectedly_ - perhaps when people have momentarily forgotten
they're even running Pageant, or at least forgotten they added a key
encrypted,, or maybe haven't found out yet that their IT installed it
- then it could usefully come with a help button that pops up further
explanation of what the dialog box means, and from which you can find
your way to the rest of the help.
I continue to believe that there's nothing I can (or should) do about
the fact that on Windows, Pageant's async passphrase prompt dialog box
doesn't automatically get the input focus when it pops up in response
to a request received via invisible IPC.
However, one thing I can do is add some text to the box that _warns_
people about it, so that at least there's some kind of suggestion that
you should get into the habit of clicking on the passphrase prompt
before typing your passphrase into it.
(I would be less concerned about all of this if it weren't for the
fact that focus is surprisingly non-obvious on Windows 10, at least on
the machine I have here. When the window doesn't have focus, the title
bar has the same background colour, and only the text is fainter. And
perhaps more confusingly, the cursor in the edit box still flashes!
That fooled _me_ a few times to begin with.)
The old behaviour is still present under an ifdef based on _MSC_VER,
so it should still appear in the w32old builds we're still making.
(cherry picked from commit 49b91bc128)
PuTTYgen and its documentation are pretty consistent about calling their
encryption key a 'passphrase', as opposed to a 'password' supplied
directly to a server; but the Argon2 parameters UI reverted to
'password hash', which seemed unecessarily confusing.
I think it's better to use the term 'passphrase' consistently in the UI.
(People who are used to Argon2 being called a 'password hash' can
probably deal.)
This required tweaking the coordinates of the Windows PuTTYgen UI.
If we're publishing the server, then we should say something about the
fact that this option exists to talk to it. Also, if the option exists
on the front page at all in a released version of PuTTY, it behooves
us to document it slightly more usefully than just a handwave at 'this
is specialist and experimental'.
I've been using and testing it for a while now, in various
circumstances, and I think it's reasonably robust.
And if a release version of PuTTY is going to have an option on the
front page that basically exists to talk to psusan, it seems silly to
then turn round and say "But you can't have psusan itself, so nerrr".
So let's not.
Also, Jacob points out that the sooner we encourage distro maintainers
to package psusan, the sooner it will (eventually) percolate through
into all the places where it would be convenient for it to already
exist, like the insides of containers.
SUPDUP came, at my insistence, with a history section in the docs
for people who hadn't heard of it. It seems only fair that the
other obsolete network protocols (or, at least, the ones we *wish*
were obsolete :-) should have the same kind of treatment.
Moved the Raw protocol to below Serial, so that the first two
sections are SSH and Serial, matching the (now very emphatic)
priority order in the config UI.
Similarly, reordered the bullet points in \k{config-hostname}.
I've filled in some text about prime generation methods and Ed448,
which were all the things marked as 'review before release'.
While I'm at it, also filled in a reasonable enough DSA key length
recommendation, because the FIXME comment in that section was within
sight of one of the places I was editing. FIPS 186-4 seemed to think
that RSA and DSA had comparable relationships between the key length
and practical security level, so I see no reason not to use the same
recommendation for both key types.
In commit bb59f27386 I changed a use of the constant GWL_ID to
GWLP_ID, on the grounds that the former caused a build failure under
winelib. But the GWLP constants are supposed to be used with
GetWindowLongPtr, and I was still calling GetWindowLong.
(Benign, since the two sets of constants are the same. But that is the
only case in the whole code base where I'd made that error, and since
it was only introduced a couple of days ago, there's no possibility of
a longstanding historical reason for carefully not touching it!)
Turns out that the precautions against winelib builds failing, which I
put in years ago because I was using winelib as a build setup for
Coverity testing, are all obsolete. My Coverity build scripts runs
fine now without any of them.
Many of VS's warnings are too noisy to be useful, but I just tried the
experiment of turning off the unrecoverable ones and seeing what was
left, and I found a couple of things that actually seem worth fixing.
In a few cases in mpint.c, and in one case in sshzlib.c, we had the
idiom 'size_t var = 1 << bitpos;', and VS pointed out that when '1' is
implicitly a 32-bit int and 'size_t' is 64 bits, this is probably not
what you wanted. Writing '(size_t)1 << bitpos' is safer.
Secondly, VS complained about lots of functions failing to return a
value, or not returning a value on every code path. In every case this
was somewhere that we'd used the local unreachable() idiom to indicate
that those code paths didn't return at all. So the real problem was
that that idiom didn't work in VS. And that's not because VS _can't_
mark functions as noreturn: it has a perfectly good declspec for it.
It was just that we hadn't actually _done_ it. Now added a clause in
the #if in defs.h that spots VS and uses the declspec.
In commit d53b3bcd22 I changed the final setting of kl->broken
so that it wouldn't overwrite a 'true' value set earlier in the
function. But that means it might not be set at all, because I forgot
I now needed to initialise it to false. Ahem.
This replaces the pure radio-button setup that we've always had on the
Session config panel.
Since the last release, that set of radio buttons has been getting out
of hand. We've added two new protocols (SUPDUP, and the 'bare
ssh-connection' aka psusan protocol), neither of which is mainstream
enough to be a sensible thing to wave at all users on the front page
of the config GUI, so that they perhaps start wondering if that's the
protocol they want to use, or get sidetracked by going and looking it
up.
The replacement UI still has radio buttons, but only for the most
common protocols, which will typically be SSH and serial. Everything
else is relegated to a drop-down list sitting next to a third radio
button labelled "Other".
In every be_* module providing a backends[] list, there's also a
variable n_ui_backends which indicates how many of the backends ought
to appear as first-level radio buttons.
(Credit where due: this patch is a joint effort between Jacob and me,
and is one of those rare cases where it would be nice to be able to
put both our names into the Author field of the commit. Failing that,
I can at least mention it here.)
This will let us put two controls side by side (e.g. in disjoint
columns of a multi-col layout) and indicate that instead of the
default behaviour of aligning their top edges, their centreline (or,
even better if available, font baseline) should be aligned.
NFC: nothing uses this yet.
We never expect to be passed a NULL GtkFrontend pointer, and even if
we were, we'd have crashed several lines above this test.
It was benign, of course, but Coverity (which pointed it out) dislikes
this kind of thing on the basis that it's confusing - you ought to
either test it for NULL properly, or not at all - and I see its point.
Coverity points out that it's theoretically possible for the main loop
in radioline_common() to read r.bottom without having gone through the
conditional setup at the start of the function _or_ a previous
iteration of the main loop. I think this can only happen in some silly
case that doesn't actually come up, but on the other hand, it's easy
to add the necessary robustness.
