When the user provides a password on the PuTTY command line, via -pw
or -pwfile, the flag 'tried_once' inside cmdline_get_passwd_input() is
intended to arrange that we only try sending that password once, and
after we've sent it, we don't try again.
But this plays badly with the 'Restart Session' operation. If the
connection is lost and then restarted at user request, we _do_ want to
send that password again!
So this commit moves that static variable out into a small state
structure held by the client of cmdline_get_passwd_input. Each client
can decide how to manage that state itself.
Clients that support 'Restart Session' - i.e. just GUI PuTTY itself -
will initialise the state at the same time as instantiating the
backend, so that every time the session is restarted, we return
to (correctly) believing that we _haven't_ yet tried the password
provided on the command line.
But clients that don't support 'Restart Session' - i.e. Plink and file
transfer tools - can do the same thing that cmdline.c was doing
before: just keep the state in a static variable.
This also means that the GUI login tools will now retain the
command-line password in memory, whereas previously they'd have wiped
it out once it was used. But the other tools will still wipe and free
the password, because I've also added a 'bool restartable' flag to
cmdline_get_passwd_input to let it know when it _is_ allowed to do
that.
In the GUI tools, I don't see any way to get round that, because if
the session is restarted you _have_ to still have the password to use
again. (And you can't infer that that will never happen from the
CONF_close_on_exit setting, because that too could be changed in
mid-session.) On the other hand, I think it's not all that worrying,
because the use of either -pw or -pwfile means that a persistent copy
of your password is *already* stored somewhere, so another one isn't
too big a stretch.
(Due to the change of -pw policy in 0.77, the effect of this bug was
that an attempt to reconnect in a session set up this way would lead
to "Configured password was not accepted". In 0.76, the failure mode
was different: PuTTY would interactively prompt for the password,
having wiped it out of memory after it was used the first time round.)
When talking to a web proxy which requires a password, our HTTP proxy
code sends an initial attempt to connect without authentication,
receives the 407 status indicating that authentication was required
(and which kind), and then closes and reopens the connection (if given
"Connection: close"). Then, on the next attempt, we try again with
authentication, and expect the first thing in the input bufchain to be
the HTTP status response to the revised request.
But what happened about the error document that followed those HTTP
headers? It - or at least some of it - would already have been in the
input bufchain.
With an HTTP/1.1 proxy, we already read it and discarded it (either
via a Content-length header or chunked transfer encoding), before we
set the 'reconnect' flag. So, assuming the proxy HTTP server is
behaving sensibly, our input bufchain ought to be empty at the point
when we start the fresh connection.
But if the proxy only speaks HTTP/1.0 (which does still happen -
'tinyproxy' is a still-current example), then we didn't get a
Content-length header either, so we didn't run any of the code that
skips over the error document. (And HTTP/1.0 implicitly has
"Connection: close" semantics, which is why that doesn't matter.) As a
result, some of it would still be in the input bufchain, and never got
cleared out, and we'd try to parse _that_ as if it was the HTTP
response from the second network connection.
The simple solution is that when we close and reopen the proxy network
connection, we also clear the input bufchain, so that the fresh
connection starts from a clean slate.
This is a partial cherry-pick of commit de66b0313a from main,
which allows all the forms of OSC sequence termination to apply in the
preliminary states as well as OSC_STRING.
The reporting user only mentioned the case of OSC 112 BEL, and not the
various forms of ST. So the former is actually known to be occurring
in the wild, and is also the least complicated part of the full patch
on main. Therefore I think this part is worthwhile and reasonably safe
to cherry-pick to 0.77 just before a release, whereas I'd be
uncomfortable making the rest of the changes at this late stage.
In Winelib, you have to be careful not to say 'unsigned long' where
the API expects ULONG, because Winelib doesn't have the Windows LLP64
nature - its unsigned long is 64 bits, whereas ULONG is 32.
Also, my local Winelib has <dwmapi.h> (used in the new demo-screenshot
system), but doesn't contain some of the definitions inside it. So
I've expanded the cmake test of HAVE_DWMAPI_H so that it actually
checks the things we need, instead of just the existence of the
containing header.
Apparently I never re-tested that option when I revamped Pageant's
command-line option parsing in commit dc183e1649, because it's
now off by one in figuring out which argument to treat as the start of
the command to be run.
(The new code in that commit is the same shape as the old code but
with variables renamed, and that was the mistake, because in the old
code, the argument index i pointed to the -c option, whereas in the
new code, match_opt has already advanced amo.index to the next word.
So the two index variables _shouldn't_ be treated the same.)
When we linked a new entry on to the global request queue, we forgot
to set its next pointer to NULL, so that when it was removed again,
s->globreq_head could end up pointing to nonsense.
In addition, even if the next pointer happened to be NULL by luck, we
also did not notice that s->globreq_head had become NULL and respond
by nulling out s->globreq_tail, which would leave s->globreq_tail as a
stale pointer to the just-freed list element, causing a memory access
error on the next attempt to link something on to the list.
This could come up in the situation where you open Change Settings and
configure a remote port forwarding, close it (so that the global
request is sent, queued, replied to, and unqueued again), and then
reopen Change Settings and configure a second one (so that the linked
list in the confused state actually gets used).
prepare_session() is the function that takes a Conf in the form it was
loaded from the configuration, and normalises it into the form that
backends can make sense of easily. In particular, if CONF_host
contains something like "user@hostname", this is the place where the
"user@" is stripped off the hostname field and moved into
CONF_username where the backend will expect to find it.
Therefore, you're _always_ supposed to call prepare_session() before
launching a backend from a Conf you (potentially) got from a saved
session. But the SSH proxy code forgot to.
As a result, if you had a saved session with "user@hostname" in the
hostname field, it would work fine to launch that session directly in
PuTTY, but trying to use the same saved session as an SSH proxy would
fail mysteriously after trying to pass "user@hostname" to
getaddrinfo() and getting nothing useful back.
(On Windows, the error message is especially opaque: an invalid string
of that kind generates an error code that we weren't even tranlsating.
And even if we _do_ translate it, it wouldn't be very meaningful,
because the error in question is WSANO_RECOVERY, which FormatMessage
renders as "A non-recoverable error occurred during a database
lookup." I guess the error in question was that Windows couldn't even
figure out how to translate that string into the format of a DNS
request message!)
When a subsidiary part of the SSH system wants to abort the whole
connection, it's supposed to call ssh_sw_abort_deferred, on pain of
free-order confusion. Elsewhere in mainchan.c I was getting this
right, but I missed a couple.
We call setlocale() at the start of the function to get the current
LC_CTYPE locale, then set it to what we need during the function, and
then call setlocale() at the end to put it back again. But the middle
call is allowed to invalidate the pointer returned from the first, so
we have to save it in our own allocated storage until the end of the
function.
This bit me during development just now, and I was surprised that it
hadn't come up before! But I suppose this is one of those things
that's only _allowed_ to fail, and need not in all circumstances -
perhaps it depends on what your LC_CTYPE was set to before.
Using a new screenshot-taking module I just added in windows/utils,
these new options allow me to start up one of the tools with
demonstration window contents and automatically save a .BMP screenshot
to disk. This will allow me to keep essentially the same set of demo
images and update them easily to keep pace with the current appearance
of the real tools as PuTTY - and Windows itself - both evolve.
Once we've actually loaded a key file, the job of updating the UI
fields is now done by a subroutine update_ui_after_load(), so that I
can call it from a different context in an upcoming commit.
I've been keeping them up to date with API changes as far as making
sure they still _compile_, but today I tried to actually run them, and
found that they were making a couple of segfault-inducing mistakes:
not filling in their vtable pointer, and not returning a 'realhost'
string. Now fixed.
If an SSH proxy socket is frozen for long enough, and the SSH server
continues to send, then sooner or later the proxy SSH connection will
end up having to freeze its underlying physical socket too. When the
proxy socket is later unfrozen, it needs to pass that unfreezing on in
turn.
The way this should happen is that when the SshProxy begins to clear
the backlog of data passed to it from the proxy SSH connection via
seat_stdout, it should call backend_unthrottle to inform that proxy
connection that the backlog is clearing.
But there was no backlog_unthrottle call in the whole of sshproxy.c.
Now there is.
I got a pterm into a stuck state this morning by an accidental mouse
action. I'd intended to press Ctrl + right-click to pop up the context
menu, but I accidentally pressed down the left button first, starting
a selection drag, and then while the left button was still held down,
pressed down the right button as well, triggering the menu.
The effect was that the context menu appeared while term->selstate was
set to DRAGGING, in which state terminal output is suppressed, and
which is only unset by a mouse-button release event. But then that
release event went to the popup menu, and the terminal window never
got it. So the terminal stayed stuck forever - or rather, until I
guessed the cause and did another selection drag to reset it.
This happened to me on GTK, but once I knew how I'd done it, I found I
could reproduce the same misbehaviour on Windows by the same method.
Added a simplistic fix, on both platforms, that cancels a selection
drag if the popup menu is summoned part way through it.
While testing the unrelated pile of commits just past, I accidentally
started a Cygwin saved session I hadn't run in ages which used the old
Telnet-based cygtermd as a local proxy command, and found that it
presented the Cygwin prompt with a trust sigil. Oops!
It turns out that this is because interactor_return_seat does two
things that can change the real seat's trust status, and it does them
in the wrong order: it defaults the status back to trusted (as if the
seat was brand new, because that's how they start out), and it calls
tempseat_flush which may have buffered a trust-status reset while the
seat was borrowed. The former should not override the latter!
This is another fallback needed on Win95, where the Win32 API
functions to convert between multibyte and wide strings exist, but
they haven't heard of the UTF-8 code page. PuTTY can't really do
without that these days.
(In particular, if a server sends a remote window-title escape
sequence while the terminal is in UTF-8 mode, then _something_ needs
to translate the UTF-8 data into Unicode for Windows to reconvert into
the character set used in window titles.)
This is a weird enough thing to be doing that I've put it under the
new #ifdef LEGACY_WINDOWS, so behaviour in the standard builds should
be unchanged.
This makes Pageant run on Win95 again. Previously (after fixing the
startup-time failures due to missing security APIs) it would go into
an uninterruptible CPU-consuming spin in the message loop when every
attempt to retrieve its messages failed because PeekMessageW doesn't
work at all on the 95 series.
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)
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.
