Now it can be called from places other than Pageant's WinMain(). In
particular, the attempt to make a security descriptor in
lock_interprocess_mutex() is gated on it.
In return, however, I've tightened up the semantics. In normal PuTTY
builds that aren't trying to support pre-NT systems, the function
*unconditionally* returns true, on the grounds that we don't expect to
target any system that doesn't support the security APIs, and if
someone manages to contrive one anyway - or, more likely, if we some
day introduce a bug in our loading of the security API functions -
then this safety catch should make Pageant less likely to accidentally
fall back to 'never mind, just run in insecure mode'.
Turns out that PuTTY hasn't run successfully on legacy Windows since
0.66, in spite of an ongoing intention to keep it working. Among the
reasons for this is that CreateWindowExW simply fails with
ERROR_CALL_NOT_IMPLEMENTED: apparently Win95 and its ilk just didn't
have fully-Unicode windows as an option.
Fixed by resurrecting the previous code from the git history (in
particular, code removed by commit 67e5ceb9a8 was useful), and
including it as a runtime alternative.
One subtlety was that I found I had to name the A and W window classes
differently (by appending ".ansi" to the A one): apparently they
occupy the same namespace even though the names are in different
character sets, so if you somehow manage to register both classes,
they'll collide with each other without that tweak.
These are more functions that don't exist on all our supported legacy
versions of Windows, so we need to follow the same policy as
everywhere else, by trying to acquire them at run time and having a
fallback if they aren't available.
This fixes a load-time failure on versions of Windows too old to have
that function in kernel32.dll.
We use it to determine whether a file was safe to overwrite in the
context of PuTTY session logging: if it's safe, we skip the 'do you
want to overwrite or append?' dialog box.
On earlier Windows you can use FindFirstFile to get a similar effect,
so that's what we fall back to. It's not quite the same, though - if
you pass a wildcard then it will succeed when you'd rather it had
failed. But it's good enough to at least work in normal cases.
This will be used to wrap some of the stranger workarounds we're
keeping in this code base for the purposes of backwards compatibility
to seriously old platforms like Win95.
This way, anyone who needs to use the version data can quickly call
init_winver to make sure it's been set up, and not waste too much faff
redoing the actual work.
By testing on a platform slow enough to show the flicker, I happened
to notice that if your shell prompt resets the window title every time
it's displayed, this was actually resulting in a call to SetWindowText
every time, which caused the GUI to actually do work.
There's certainly no need for that! We can at least avoid bothering if
the new title is identical to the old one.
It turns out that they're still NULL at the first moment that a
SetWindowText call tries to read one of them, because they weren't
initialised at startup! Apparently SetWindowText notices that it's
been passed a null pointer, and does nothing in preference to failing,
but it's still not what I _meant_ to do.
Checking various implementations of these functions against each
other, I noticed by eyeball review that some of the special cases in
mb_to_wc() never check the buffer limit at all. Yikes!
Fortunately, I think there's no vulnerability, because these special
cases are ones that write out at most one wide char per multibyte
char, and at all the call sites (including dup_mb_to_wc) we allocate
that much even for the first attempt. The only exception to that is
the call in key_event() in unix/window.c, which uses a fixed-size
output buffer, but its input will always be the data generated by an X
keystroke event. So that one can only overrun the buffer if an X key
event manages to translate into more than 32 wide characters of text -
and even if that does come up in some exotic edge case, it will at
least not be happening under _enemy_ control.
Unlike on Unix, a Windows process's exit status is a DWORD, i.e. a
32-bit unsigned integer. And exit statuses seen in practice can range
up into the high half of that space. For example, if a process dies of
an 'illegal instruction' exception, then the exit status retrieved by
GetExitCodeProcess will be 0xC000001D == STATUS_ILLEGAL_INSTRUCTION.
If this happens to the process running inside pterm.exe, then
conpty->exitstatus will be set to a value greater than INT_MAX, which
will cause conpty_exitcode to return -1. Unfortunately, a -1 return
from conpty_exitstatus is treated by front ends as saying that the
backend process hasn't exited yet, and is still running. So pterm will
sit around after its subprocess has already terminated, contrary to
your 'close on exit' setting.
Moreover, when cmd.exe exits, it apparently passes on to its parent
process the exit status of the last subcommand it ran. So if you run a
Windows pterm containing an ordinary interactive console session, and
the last subprogram you happen to run inside that session dies of a
fatal signal, then the same thing will happen after you type 'exit' at
the command prompt.
This has been happening to me intermittently ever since I created
pterm.exe in the first place, and I guessed completely wrong about the
cause (I feared some kind of subtle race condition in pterm's use of
the process API). I've only just managed to reproduce it reliably
enough to debug, and I'm relieved to find it's much simpler than that!
The immediate fix, in any case, is to ensure we don't return -1 from
conpty_exitcode unless the process really is still running. And don't
return INT_MAX either, because that indicates 'unclean exit' in a way
that triggers 'close window only on clean exit' (and even Unix pterm
doesn't consider the primary subprocess dying of a signal to count as
unclean). So we clip all out-of-range Windows exception codes to
INT_MAX-1.
In the longer term I think it would be nice to turn exit codes into a
full 32-bit space, and move the special values completely out of it.
That would permit actually keeping the exact exception code and
passing it on to a caller who needed it. For example, if we were to
write a Windows psusan (which I could imagine being occasionally
useful), this way it would be able to return the unmodified full
Windows exit code via the "exit-status" chanreq. But I don't think we
currently have any clients needing that much detail, so that's a more
intrusive cleanup for a later date.
I recently noticed a mysterious delay at connection startup while
using an SSH jump host, and investigated it in case it was a bug in
the new jump host code that ought to be fixed before 0.77 goes out.
strace showed that at the time of the delay PuTTY was doing a DNS
lookup for the destination host, which was hanging due to the
authoritative DNS server in question not being reachable. But that was
odd, because I'd configured it to leave DNS lookup to the proxy,
anticipating exactly that problem.
But on closer investigation, the _proxy_ code was doing exactly what
I'd told it. The DNS lookup was coming from somewhere else: namely, an
(unsuccessful) attempt to set up a GSSAPI context. The GSSAPI library
had called gethostbyname, completely separately from PuTTY's own use
of DNS.
Simple workaround for me: turn off GSSAPI, which doesn't work for that
particular SSH connection anyway, and there's no point spending 30
seconds faffing just to find that out.
But also, if that puzzled me, it's worth documenting!
FreeProxy sends 'algorithm="MD5"' instead of 'algorithm=MD5.' I'm
actually not sure whether that's legal by RFC 7616, but it's certainly
no trouble to parse if we see it.
With all these changes, PuTTY now _can_ successfully make connections
through FreeProxy again, whether it's in Basic or Digest mode.
Another awkward thing that FreeProxy does is to slam the connection
shut after sending its 407 response, at least in Basic auth mode. (It
keeps the connection alive in digest mode, which makes sense to me,
because that's a more stateful system.)
It was surprisingly easy to make the proxy code able to tolerate this!
I've set it up so that a ProxyNegotiator can just set its 'reconnect'
flag on return from the negotiation coroutine, and the effect will be
that proxy.c makes a new connection to the same proxy server before
doing anything else. In particular, you can set that flag _and_ put
data in the output bufchain, and there's no problem - the output data
will be queued directly into the new socket.
FreeProxy sends this as a substitute for the standard 'Connection'
header (with the same contents, i.e. 'keep-alive' or 'close' depending
on whether the TCP connection is going to continue afterwards). The
Internet reckons it's not standard, but it's easy to recognise as an
ad-hoc synonym for 'Connection'.
I had a report that the Windows free-as-in-beer proxy tool 'FreeProxy'
didn't work with the new HTTP proxy code, and it turns out that the
first reason why not is that the error-document in its 407 response is
sent via chunked transfer encoding, which is to say, instead of an
up-front Content-length header, you receive a sequence of chunks each
prefixed with a hex length.
(In 0.76, before the rewritten proxy support, we never even noticed
this because we sent Basic auth details up front in our first attempt,
rather than attempting a no-auth connection first and waiting to see
what kind of auth the proxy asks us for. So we'd only ever see a 407
if the auth details were refused - and since 0.76 didn't have
interactive proxy auth prompts, there was nothing we could do at that
point but abort immediately, without attempting to parse the rest of
the 407 at all.)
Now we spot the Transfer-encoding header and successfully parse
chunked transfers. Happily, we don't need to worry about the further
transfer-encodings such as 'gzip', because we're not actually _using_
the error document - we only have to skip over it to find the end of
the HTTP response.
This still doesn't make PuTTY work with FreeProxy, because there are
further problems hiding behind that one, which I'll fix in following
commits.
A user points out that this has regressed since 0.76, probably when I
reorganised the keyboard control-sequence formatting into centralised
helper functions in terminal.c.
The SCO function keys should behave differently when you press Shift
or Ctrl or both. For example, F1 should generate ESC[M bare, ESC[Y
with Shift, Esc[k with Ctrl, Esc[w with Shift+Ctrl. But in fact, Shift
was having no effect, so those tests would give ESC[M twice and ESC[k
twice.
That was because I was setting 'shift = false' for all function key
types except FUNKY_XTERM_216, after modifying the derived 'index'
value. But the SCO branch of the code doesn't use 'index' (it wouldn't
have the right value in any case), so the sole effect was to forget
about Shift. Easily fixed by disabling that branch for FUNKY_SCO too.
(cherry picked from commit aa01530488)
A user points out that this has regressed since 0.76, probably when I
reorganised the keyboard control-sequence formatting into centralised
helper functions in terminal.c.
The SCO function keys should behave differently when you press Shift
or Ctrl or both. For example, F1 should generate ESC[M bare, ESC[Y
with Shift, Esc[k with Ctrl, Esc[w with Shift+Ctrl. But in fact, Shift
was having no effect, so those tests would give ESC[M twice and ESC[k
twice.
That was because I was setting 'shift = false' for all function key
types except FUNKY_XTERM_216, after modifying the derived 'index'
value. But the SCO branch of the code doesn't use 'index' (it wouldn't
have the right value in any case), so the sole effect was to forget
about Shift. Easily fixed by disabling that branch for FUNKY_SCO too.
There's now a command-line option to make Pageant open an AF_UNIX
socket at a pathname of your choice. This allows it to act as an SSH
agent for any client program willing to use a WinSock AF_UNIX socket.
In particular, this allows WSL 1 processes to talk directly to Windows
Pageant without needing any intermediate process, because the AF_UNIX
sockets in the WSL 1 world interoperate with WinSock's ones.
(However, not WSL 2, which isn't very surprising.)
Apparently when I made Windows Pageant use the winselgui system, I
added the call that gets WSAAsyncSelect response messages sent to
Pageant's window, but I didn't add the switch case in the window
procedure that actually handles those responses. I suppose I didn't
notice at the time because no actual functionality used it - Pageant
has never yet dealt with any real (i.e. Winsock) sockets, only with
HANDLE-based named pipes, which are called 'sockets' in PuTTY's
abstraction, but not by Windows.
Most of the previous large sk_newlistener function is now an inner
function whose address-family parameter is a platform AF_FOO constant
rather than one of our own ADDRTYPE_FOO. sk_newlistener itself is a
trivial wrapper on that, which just does the initial translation from
the input ADDRTYPE_FOO into an AF_FOO.
This will make it possible to drop in alternative wrapper functions
which won't have to make up a pointless ADDRTYPE.
This macro is now an inline function, and as in the previous commit,
each possible value for the main discriminator is now a case in a
switch statement instead of tested in an interlocking set of ?:.
The code that diverges based on the address family is now in the form
of a switch statement, rather than an unwieldy series of chained ifs.
And the final call to bind() has all its arguments worked out in the
previous switch, rather than computing them at the last minute with an
equally unwieldy set of ?: operators that repeat the previous test.
This will make it easier to add more cases, and also, to keep each
case under its own ifdef without losing too much legibility.
This replaces two previous boolean fields 'resolved' and 'namedpipe',
converting them into a single three-valued enum which avoids being
able to represent the meaningless fourth possibility at all. Also, it
provides an open-ended place to add further possibilities.
The new field is very similar to the one in unix/network.c, except
that the UNIX entry for AF_UNIX sockets is missing, and in its place
is the NAMEDPIPE entry for storing the pathnames of Windows named
pipes.
When the window can't be resized for any reason, there will be extra
space inside the drawing area that's not part of our standard
width*font_width+2*window_border. We should include that in the
backing surface and make sure we erase it to the background colour,
otherwise it can end up containing unwanted visual junk.
An example is the same case described in the previous commit: maximise
the window and then start playing about with the font size. If you do
this while running a full-screen application that displays text in the
bottom line, it's easy to see that part of the previous display is
left over and not cleared when the new font size leaves more space at
the bottom than the old one.
If you maximise the terminal window and then press Ctrl-> or Ctrl-< to
change the font size, then the maximised window can't change size, so
what _should_ happen instead is that the terminal adjusts the number
of character cells to whatever the new font size will now permit in
the same size of window as before.
But in fact, the terminal size wasn't changing at all, because the
call to gtkwin_request_resize (called from change_font_size) detected
the maximised window and went straight to gtkwin_deny_term_resize,
which immediately called term_size() to tell the terminal it still had
the same size as before.
This commit switches gtkwin_deny_term_resize so that instead it calls
drawing_area_setup_simple(), which re-runs drawing_area_setup with the
same size the drawing area already had. This should work out the same
in the case where we're _not_ changing the font size, but now also
does the right thing when we are.
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.
Unix Pageant is in a tricky position as a hybrid CLI/GUI application.
It has uses even in a purely CLI environment, but it won't build
without libgtk-3-dev and friends.
The solution, of course - enabled by the migration to cmake - is to
allow it to build without GTK, leaving out just the GTK askpass
functionality. That way you can still use it in any of its CLI modes,
either as a non-graphical SSH agent or as a client for an agent
elsewhere.
(You can still even use it in X lifetime mode, because its connection
to the X server is done using PuTTY's built-in X authentication and
connection setup code. It's only putting up the password prompt window
that you lose in this configuration - so you're still fine as long as
you don't try to add any encrypted keys.)
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.
