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.
Coverity pointed out that I'd checked if the LoadedFile was NULL, set
an error message ... and then accidentally fallen through to the
success handler anyway.
Coverity points out that if rsa_ssh1_public_blob_len sees data it
doesn't like, it returns -1 to indicate an error. But the code that
uses it to parse the SSH1_AGENT_RSA_IDENTITIES_ANSWER payload was
passing it directly to get_data() as a length field, without checking
for that case. Now we do check it, and use it to set the existing
kl->broken flag that indicates that the key list was not correctly
formatted.
Coverity was unhappy that I'd used the packet length as a loop bound
without sanitising it first (on the basis that it had decided anything
coming from GET_32BIT_MSB_FIRST was potentially tainted).
I think this is not a security issue: all that will happen if the
server sends a huge packet length is that we'll try to allocate space
for it. On a 64-bit machine we might even _succeed_; on 32-bit, we'll
fail, and snewn() will abort the program rather than return NULL. So
*technically* this is a remote-triggered crash. But it can only happen
in a situation where the same server could have triggered the
termination of the SFTP connection just as easily by simply closing it
- the only difference is that the client would die with a different
fatal error message.
(In particular, it isn't even a DoS against other processes
participating in a connection-shared SSH session. The upstream will
pass the SFTP data stream through without parsing it, so it and the
other downstreams will be unaffected. Only the particular downstream
operating the SFTP client will run into this problem.)
If named_pipe_agent_gotdata was called with an error or EOF status, it
would call agent_cancel_query(pq), but then accidentally fall through
to the non-error handler which would dereference pq. I meant to return
early in that situation, and Coverity spotted that I'd left out the
early return statement.
Coverity objected to several similar cases in this code in which I'd
checked a pointer for NULL after already having done things to it. I
think all the cases are benign, in that (as the comments tersely
mention) those checks could only fail if the unifontsel system had got
_really_ confused, in which case probably some other bug would have
been on the point of manifesting anyway. But Coverity has a point
anyway: if I'm _going_ to check those values for NULL, let's check
them consistently.
Commit d851df486f deleted a #if / #else / #endif on the grounds
that the condition would now always be true, without also deleting the
code inside the #else. Happily, the then-branch ended with a return,
so it was a benign mistake - the erroneously left-in else-clause code
was unreachable. But now Coverity has pointed it out, let's remove it.
Coverity points out that we don't need to check the output buffer
bound before writing out the first 32 bytes of each full-length
BLAKE2b invocation, because the only time we're doing a full-length
one in the first place is if the output buffer bound was at least 64
bytes.
(More specifically: whenever we're in the while loop, length > 64, so
setting chunk = 32 and then checking if chunk > length has a totally
predictable answer.)
The winelib headers don't have GWL_foo, only GWLP_foo (which, fair
enough, I should have been using already). And a side effect was to
point out some slightly incautious integer types in printf argument
lists.
This has apparently been missing more or less forever (though Unix
Plink does have it). Without this, ssh.c can't call ldisc_update,
which can't pass the current editing and echoing settings through to
seat_echoedit_update. Windows Plink has always _had_ an implementation
of that seat method (and the static function that preceded it), but it
was never able to be called, because of that missing link.
The result was that manual overrides in the Conf to force local
editing/echoing to a particular state were not honoured by Windows
Plink, and neither were mainchan.c's attempts to set the state
automatically based on whether a pty had been allocated at the far end
of the connection.
This seems more useful than the previous behaviour of not prompting for
a passphrase and only emitting the public part; if we want that back
I suppose we could invent a "-O text-public".
Also, document the text dump format a bit in the man page.
