Visual Studio 2022 is out, and 2019 has added a couple more version
numbers while I wasn't looking.
Also, the main web page that lists the version number mappings now
documents the wrinkle where you sometimes have to disambiguate via
_MSC_FULL_VER (and indeed has added another such case for 16.11), so I
no longer have to link to some unofficial blog post in the comment
explaining that.
(*Also*, if _MSC_FULL_VER is worth checking, then it's worth putting
in the build info!)
If term_get_userpass_input is called with term->ldisc not yet set up,
then we had a special-case handler that returns an error message - but
it does it via the same subroutine that returns normal results, which
also turns off the prompt callback in term->ldisc! Need an extra NULL
check in that subroutine. Thanks Coverity.
Spotted by Coverity: if you _just_ gave a filename to bidi_test,
without any previous argument that set testfn to something other than
NULL, the program would crash rather than giving an error message.
(It's only a test program, but test programs you only run once in a
blue moon are the ones that _most_ need to explain their command-line
syntax to you carefully, because you've forgotten it since last time
you used them!)
Also, conditionalised a memcpy on the size not being 0, because it's
illegal to pass a null pointer to memcpy _even_ if size==0. (That
would only happen with a test case containing a zero-length string,
but whatever.)
If you try to use a saved session for SSH proxying which specifies a
protocol that is not SSH or bare-SSH-connection, you get a clean error
return from the proxy setup code - *provided* it's at least a protocol
known to this particular build of PuTTY. If it's one so outlandish
that backend_vt_from_proto returns NULL, there'd have been a crash.
I don't think any such protocol currently exists, but if in the next
version of PuTTY some additional protocol becomes supported, it will
trip this error in the current version.
Spotted by Coverity.
When the textlen parameter became a size_t, it became unsigned, so it
stopped being useful to assert() its non-negativity.
Spotted by Coverity. Harmless, but ordinary compilers have been known
to emit annoying warnings about that kind of thing too, so it's worth
fixing just to avoid noise.
I tried setting this up on a different Windows machine today and had
some slightly different experiences. I found that in at least some
situations the command 'Include c:\...\pageant.conf' will cause
OpenSSH to emit a log message saying it's trying to open the file
'~/.ssh/c:\...\pageant.conf', which it then doesn't find. But 'Include
pageant.conf' works, because that's interpreted relative to the .ssh
directory that it's already found.
(I don't know why this happened on one Windows machine and not
another, since I only have a sample size of two. But an obvious guess
would be a bug fix in the Windows OpenSSH port, present in the version
on one of the machines I tried, and not in the other. Certainly that
failure mode looks to me like 'apply Unix instead of Windows rules to
decide what's an absolute pathname'.)
Also, clarified that all of this only works with the version of
OpenSSH that's available as a Windows optional feature, and not with
the MSYS-based one that ships with Windows git.
When I was writing the documentation for the new command-line options,
I wondered why there was an existing section for the corresponding GUI
setting for each option I'd added except strong primes. Now I've found
it: strong primes are discussed in the same section as prime-
generation methods. So I can replace the second explanation with a
cross-reference.
Those were forbidden so that we could still compile on pre-C99 C
compilers. But now we expect C99 everywhere (or at least most of it,
excluding the parts that MSVC never implemented and C11 made
optional), so // comments aren't forbidden any more.
Most of the comments in this code base are still old-style, but that's
now a matter of stylistic consistency rather than hard requirement.
Correcting a source file name in the docs just now reminded me that
I've seen a lot of outdated source file names elsewhere in the code,
due to all the reorganisation since we moved to cmake. Here's a giant
pass of trying to make them all accurate again.
This was needed at the time it was introduced in commit
c99338b750, because uxputty.c (as was) handled its non-option
arguments directly (that was how Unix PuTTY and pterm arranged to have
different sets of them), and sometimes did it by converting them into
option arguments and feeding them to cmdline.c, so it still needed to
not fail to link when not linked against cmdline.c (for the
GtkApplication based front end).
But now the non-option argument handling is centralised into cmdline.c
itself, with a system of flags indicating which arguments a particular
tool expects. So that stub is no longer needed.
It's been so long since Windows Plink kept its stdio subthreads in its
own main source file that I'd forgotten it had ever done so! They've
lived in a separate module for managing Windows HANDLE-based I/O for
ages. That module has recently changed its filename, but this piece of
documentation was so out of date that the old filename wasn't in there
- it was still mentioning the filename _before_ that.
Jacob reported that on Debian buster, the command sequence
cmake $srcdir
cmake --build .
cmake --build . --target doc
would fail at the third step, with the make error "No rule to make
target 'doc/cmake_version.but', needed by 'doc/html/index.html'".
That seems odd, because the file ${VERSION_BUT} _was_ declared as a
dependency of the rule that builds doc/html/*.html, and _cmake_ knew
what rule built it (namely the custom target 'cmake_version_but'). I
suspect this is a bug in cmake 3.13, because the same command sequence
works fine with cmake 3.20.
However, it's possible to work around, by means of adding the cmake
_target name_ to the dependencies for any rule that uses that file,
instead of relying on it to map the output _file_ name to that target.
While I'm at it, I've transformed the rules that build copy.but and
licence.but in the same way, turning those too into custom targets
instead of custom commands (I've found that the former are more
generally reliable across a range of cmake versions), and including
the target names themselves as dependencies.
After a discussion with a user recently, I investigated the Windows
native ssh.exe, and found it uses a Windows named pipe to talk to its
ssh-agent, in exactly the same way Pageant does. So if you tell
ssh.exe where to find Pageant's pipe, it can talk directly to Pageant,
and then you can have just one SSH agent.
The slight problem is that Pageant's pipe name is not stable. It's
generated using the same system as connection-sharing pipe names, and
contains a hex hash value whose preimage was fed through
CryptProtectData. And the problem with _that_ is that CryptProtectData
apparently reinitialises its seed between login sessions (though it's
stable within a login session), which I hadn't fully realised when I
reused the same pipe-name construction code.
One possibility, of course, would be to change Pageant so that it uses
a fixed pipe name. But after a bit of thought, I think I actually like
this feature, because the Windows named pipe namespace isn't
segregated into areas writable by only particular users, so anyone
using that namespace on a multiuser Windows box is potentially
vulnerable to someone else squatting on the name you wanted to use.
Using this system makes that harder, because the squatter won't be
able to predict what the name is going to be! (Unless you shut down
Pageant and start it up again within one login session - but there's
only so much we can do. And squatting is at most a DoS, because
PuTTY's named-pipe client code checks ownership of the other end of
the pipe in all cases.)
So instead I've gone for a different approach. Windows Pageant now
supports an extra command-line option to write out a snippet of
OpenSSH config file format on startup, containing an 'IdentityAgent'
directive which points at the location of its named pipe. So you can
use the 'Include' directive in your main .ssh/config to include this
extra snippet, and then ssh.exe invocations will be able to find
wherever the current Pageant has put its pipe.
This imports the following options from command-line PuTTYgen, which
all correspond to controls in Windows PuTTYgen's GUI, and let you set
the GUI controls to initial values of your choice:
-t <key type>
-b <bits>
-E <fingerprint type>
--primes <prime gen policy>
--strong-rsa
--ppk-param <KDF parameters or PPK version etc>
The idea is that if someone generates a lot of keys and has standard
non-default preferences, they can make a shortcut that passes those
preferences on the command line.
These two tools had ad-hoc command loops with similar options, and I
want to extend both (in particular, in a way that introduces options
with arguments). So I've started by throwing together some common code
to do all the tedious bits like finding option arguments wherever they
might be, throwing errors, handling "--" and so on.
Should be no functional change to the existing command-line syntax,
except that now all long options are recognised in both "-foo" and
"--foo" form.
It looks as if I broke this some time around commit cfc9023616,
when I stopped proactively calling term_size() in advance of resizing
the window. A side effect was that I also stopped calling it at all in
the case where we're _not_ resizing the window (because changing the
size of the terminal means adapting the font size to fit a different
amount of stuff in the existing window).
Fixed by moving all the new machinery inside the 'actually resize the
window' branch of the if statement, and restoring the previous
behaviour in the other branch, this time with a comment that will
hopefully stop me making the same mistake again.
Some pointing devices (e.g. gaming mice) can be set to send
mouse-movement events at an extremely high sample rate like 1kHz. This
apparently translates into Windows genuinely sending WM_MOUSEMOVE
messages at that rate. So if you're using one of those mice, then
PuTTYgen's mouse-based entropy collection system will fill its buffer
almost immediately, and give you no perceptible time to actually wave
the mouse around.
I think that in that situation, there's also likely to be a strong
correlation between the contents of successive movement events,
because human-controlled mouse movements aren't fractals - the more
you zoom in on a little piece of one, the more it starts to look more
and more like something moving in a straight line at constant speed,
because the deviations from that happen on a larger time scale than
you're seeing.
So this commit adds a rate limit, not on the _collection_ of the data
(we'll still take all the bits we can get, thanks!), but on the rate
at which we increment the _counter_ for how much entropy we think
we've collected so far. That way, the user still has to spend time
wiggling the mouse back and forth in a way that varies with muscle
motions and introduces randomness.
There's no point having a separate boolean flag. All we have to do is
remember to NULL out the strbuf point state->entropy when we free the
strbuf (which is a good idea in any case!), and then we can use the
non-NULL-ness of that pointer as the indicator that we're currently
collecting entropy.
This offloads the memory management into centralised code which is
better at it than the ad-hoc code here. Now I don't have to predict in
advance how much memory the entropy will consume, and can change that
decision on the fly.
In the previous state of the code, we first tested agent_exists() to
decide whether to be the long-running Pageant server or a short-lived
client; then, during the command-line parsing loop, we prompted for
passphrases to add keys presented on the command line (to ourself or
the server, respectively); *then* we set up the named pipe and
WM_COPYDATA receiver window to actually start functioning as a server,
if we decided that was our role.
A consequence is that if a user started up two Pageants each with an
encrypted key on the command line, there would be a race condition:
each one would decide that it was _going_ to be the server, then
prompt for a passphrase, and then try to set itself up as the server
once the passphrase is entered. So whichever one's passphrase prompt
was answered second would add its key to its own internal data
structures, then fail to set up the server's named pipe, terminate
with an error, and end up not having added its key to the _surviving_
server.
This change reorders the setup steps so that the command-line parsing
loop does not add the keys immediately; instead it merely caches the
key filenames provided. Then we make the decision about whether we're
the server, and set up both the named pipe and WM_COPYDATA window if
we are; and finally, we go back to our list of key filenames and
actually add them, either to ourself (if we're the server) or to some
other Pageant (if we're a client).
Moreover, the decision about whether to be the server is now wrapped
in an interprocess mutex similar to the one used in connection
sharing, which means that even if two or more Pageants are started up
as close to simultaneously as possible, they should avoid a race
condition and successfully manage to agree on exactly one of
themselves to be the server. For example, a user reported that this
could occur if you put shortcuts to multiple private key files in your
Windows Startup folder, so that they were all launched simultaneously
at startup.
One slightly odd behaviour that remains: if the server Pageant has to
prompt for private key passphrases at startup, then it won't actually
start _servicing_ requests from other Pageants until it's finished
dealing with its own prompts. As a result, if you do start up two
Pageants at once each with an encrypted key file on its command line,
the second one won't even manage to present its passphrase prompt
until the first one's prompt is dismissed, because it will block
waiting for the initial check of the key list. But it will get there
in the end, so that's only a cosmetic oddity.
It would be nice to arrange that Pageant GUI operations don't block
unrelated agent requests (e.g. by having the GUI and the agent code
run in separate threads). But that's a bigger problem, not specific to
startup time - the same thing happens if you interactively load a key
via Pageant's file dialog. And it would require a major reorganisation
to fix that fully, because currently the GUI code depends on being
able to call _internal_ Pageant query functions like
pageant_count_ssh2_keys() that don't work by constructing an agent
request at all.
This update covers several recently added features: SSH proxying, HTTP
Digest proxy auth, and interactive prompting for proxy auth in general.
Also, downplayed the use of 'plink -nc' as a Local-type proxy command.
It still works, but it's no longer the recommended way of tunnelling
SSH over SSH, so there's no need to explain it quite so
enthusiastically.
I was just writing the documentation for the new proxy type, which
caused me to realise that the thing I obviously wanted to write in the
documentation was not actually true. Let's make it true, and then I
can document it!
I have _no_ idea how I managed to leave this out of the list of
examples when I first wrote this man page. It should have been the
very first one I thought of, since Cygwin was the platform I wrote
cygtermd for, and one of psusan's primary purposes was to be a
productised and improved replacement for cygtermd!
Oh well, better late than never.
All the seat functions that request an interactive prompt of some kind
to the user - both the main seat_get_userpass_input and the various
confirmation dialogs for things like host keys - were using a simple
int return value, with the general semantics of 0 = "fail", 1 =
"proceed" (and in the case of seat_get_userpass_input, answers to the
prompts were provided), and -1 = "request in progress, wait for a
callback".
In this commit I change all those functions' return types to a new
struct called SeatPromptResult, whose primary field is an enum
replacing those simple integer values.
The main purpose is that the enum has not three but _four_ values: the
"fail" result has been split into 'user abort' and 'software abort'.
The distinction is that a user abort occurs as a result of an
interactive UI action, such as the user clicking 'cancel' in a dialog
box or hitting ^D or ^C at a terminal password prompt - and therefore,
there's no need to display an error message telling the user that the
interactive operation has failed, because the user already knows,
because they _did_ it. 'Software abort' is from any other cause, where
PuTTY is the first to know there was a problem, and has to tell the
user.
We already had this 'user abort' vs 'software abort' distinction in
other parts of the code - the SSH backend has separate termination
functions which protocol layers can call. But we assumed that any
failure from an interactive prompt request fell into the 'user abort'
category, which is not true. A couple of examples: if you configure a
host key fingerprint in your saved session via the SSH > Host keys
pane, and the server presents a host key that doesn't match it, then
verify_ssh_host_key would report that the user had aborted the
connection, and feel no need to tell the user what had gone wrong!
Similarly, if a password provided on the command line was not
accepted, then (after I fixed the semantics of that in the previous
commit) the same wrong handling would occur.
So now, those Seat prompt functions too can communicate whether the
user or the software originated a connection abort. And in the latter
case, we also provide an error message to present to the user. Result:
in those two example cases (and others), error messages should no
longer go missing.
Implementation note: to avoid the hassle of having the error message
in a SeatPromptResult being a dynamically allocated string (and hence,
every recipient of one must always check whether it's non-NULL and
free it on every exit path, plus being careful about copying the
struct around), I've instead arranged that the structure contains a
function pointer and a couple of parameters, so that the string form
of the message can be constructed on demand. That way, the only users
who need to free it are the ones who actually _asked_ for it in the
first place, which is a much smaller set.
(This is one of the rare occasions that I regret not having C++'s
extra features available in this code base - a unique_ptr or
shared_ptr to a string would have been just the thing here, and the
compiler would have done all the hard work for me of remembering where
to insert the frees!)
In cmdline_get_passwd_input(), there's a boolean 'tried_once' which is
set the first time the function returns the password set by -pw or
-pwfile. The idea, as clearly commented in the code, was that if
cmdline_get_password_input asks for that password _again_, we return
failure, as if the user had refused to make a second attempt.
But that wasn't actually what happened, because when we set tried_once
to true, we also set cmdline_password to NULL, which causes the second
call to the function to behave as if no password was ever provided at
all. So after the -pw password failed, we'd fall back to asking
interactively.
This change moves the check of cmdline_password to after the check of
tried_once, restoring the originally intended behaviour: password
authentication will now _only_ be done via the pre-set password, if
there is one.
This seems like an xkcd #1172 kind of change: now that it's been wrong
for a while, _someone_ has probably found the unintended behaviour
useful, and started relying on it. So it may become necessary to add
an option to set the behaviour either way. But for the moment, let's
try it the way I originally intended it.
In SSH-1 there's a function that takes a void * that it casts to the
state of the login layer. The corresponding function in SSH-2 casts it
to the state of a differently named layer, but I had still called the
parameter 'loginv'.
I happened to notice in passing that this function doesn't have any
tests (although it will have been at least somewhat tested by the
cmdgen interop test system).
This involved writing a wrapper that passes the passphrase and salt as
ptrlens, and I decided it made more sense to make the same change to
the original function too and adjust the call sites appropriately.
I derived a test case by getting OpenSSH itself to make an encrypted
key file, and then using the inputs and output from the password hash
operation that decrypted it again.
