This matches the way it's done in the GTK backend: the only thing that
happens inside seat_notify_remote_exit is that we schedule a toplevel
callback, and all the real work happens later on when that callback is
called.
The particular situation where this makes a difference is if you
perform a user abort during proxy authentication (e.g. hit ^D at a
proxy password prompt), so that the main SSH backend gets
PLUGCLOSE_USER_ABORT and calls ssh_user_close. That doesn't
immediately close the socket; it just sets ssh->pending_close,
schedules a run of ssh_bpp_output_raw_data_callback, and returns.
So if seat_notify_remote_exit calls back _synchronously_ to
ssh_return_exitcode, it will see that the socket is still open and
return -1. But if it schedules a callback and waits, then the callback
to ssh_bpp_output_raw_data_callback will happen first, which will
close the socket, and then the exit processing will get the right
answer.
So the user-visible effect is that if you ^D a proxy auth prompt, GTK
PuTTY will close the whole window (assuming you didn't set close-on-
exit to 'never'), whereas Windows PuTTY will leave the window open,
and not even know that the connection is now shut (in that it'll still
ask 'Are you sure you want to close this session?' if you try to close
it by hand).
With this fix, Windows PuTTY behaves the same as GTK in this respect.
cmake doesn't have convincing facilities for doing this in its install
step, so the new advice is to do it manually (we've provided no
equivalent to the autotools --enable-setuid or --enable-setgid options,
nor UTMP_USER/GROUP).
Without this, the build of e.g. psusan would fail on systems without
that header (such as Termux on Android).
This is similar to how things were pre-cmake, but not identical. We used
to treat lack of updwtmpx() as a reason to OMIT_UTMP (as of f0dfa73982),
but usage of that function got conditionalised in c19e7215dd, so I
haven't restored that exclusion.
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.
Commit 5390aef3fc broke it, because GTK1 has neither
gtk_label_set_selectable nor gtk_widget_set_can_focus. Happily, those
are both more or less optional (only a minor UI awkwardness arises
from not having them), so I'll just condition them out.
If the user holds down Alt-> so that the key repeats, then a second
call to change_font_size can occur while the window resize from the
previous one has yet to complete. This leads to the new pixel size of
the window from resize #1 being interpreted in the light of the font
size from reesize #2, so that the two get out of step and the
_character_ size of the terminal changes as a result.
The simplest fix is to disallow starting a second font-size-based
window resize while the first is still in flight - which, now that the
'win_resize_pending' flag lives in window.c and not terminal.c, is
easy to achieve.
In the changes around commit 420fe75552, I made the terminal
suspend output processing while it waited for a term_size() callback
in response to a resize request. Because on X11 there are unusual
circumstances in which you never receive that callback, I also added a
last-ditch 5-second timeout, so that eventually we'll resume terminal
output processing regardless.
But the timeout lives in terminal.c, in the cross-platform code. This
is pointless on Windows (where resize processing is synchronous, so we
always finish it before the timer code next gets called anyway), but I
decided it was easier to keep the whole mechanism in terminal.c in the
absence of a good reason not to.
Now I've found that reason. We _also_ generate window resizes locally
to the GTK front end, in response to the key combinations that change
the font size, and _those_ still have an asynchrony problem.
So, to begin with, I'm refactoring the request_resize system so that
now there's an explicit callback from the frontend to the terminal to
say 'Your resize request has now been processed, whether or not you've
received a term_size() call'. On Windows, this simplifies matters
greatly because we always know exactly when to call that, and don't
have to keep a 'have we called term_size() already?' flag. On GTK, the
timing complexity previously in terminal.c has moved into window.c.
No functional change (I hope). The payoff will be in the next commit.
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.
A user reports that the xterm OSC 112 sequence (reset cursor colour)
is sometimes sent as simply OSC 112 BEL, rather than OSC 112 ; BEL.
When xterm parses this, the BEL still acts as an OSC terminator, even
though it appears before the separating semicolon that shifts into the
'absorb the notional command string' state.
PuTTY doesn't support that sequence at all. But currently, the way it
doesn't support it is by treating the BEL completely normally, so that
you get an annoying beep when a client application sends that
abbreviated sequence.
Now we recognise all the OSC terminator sequences even in the OSC
setup termstates, as well as the final OSC_STRING state. That goes
equally for BEL, ST in the form of ESC \, ST in the form of
single-byte 0x9C, and ST in the UTF-8 encoding.
The previous fix on pre-0.77 was non-disruptive and just enough to get
through my Coverity build (which uses winelib); but now that I look at
the rest of the Winelib build output, there are some further warnings
I should fix on main.
Most of them are more long/LONG confusion (specific to Winelib, rather
than real Windows); also, there's a multiple macro definition in
jump-list.c because Winelib defines _PROPVARIANT_INIT_DEFINED_ in
place of _PROPVARIANTINIT_DEFINED_ which we were testing for. (Bah.)
And in windows/window.c I used wcscmp without including <wchar.h>.
In spite of long vs LONG I still had to turn off one or two more
DLL-loading typechecks.
save_screenshot() returns a dynamically allocated error message in
case of failure, and Coverity complained of a memory leak when it was
ignored in putty.c.
The memory leak is trivial, because we were about to terminate the
process with an error anyway. But it's a good point that I forgot to
report the error!
Not critical enough to fix on 0.77 (where Coverity found it), but we
might as well make it look sensible on main.
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.
Now we check that we can actually make an ssh_key out of it, and
moreover, that the key is of a sensible kind (i.e. not a certificate
in turn). If that's not true, we report something about the problem in
a new CTRL_TEXT below the public key input box. If the key _is_ valid,
that same text control is used to show its type, length and
fingerprint.
On Windows, I've widened the dialog box a little to make fingerprints
fit sensibly in it.
This is for cases where they're presenting information to the user
that wouldn't wrap sensibly anyway (such as an SSH key fingerprint
which is mostly all one word), and in which newlines might be
significant.
On GTK, the implementing widget is still a GtkLabel, but without the
wrap flag set, and wrapped in a GtkScrolledWindow in case the text is
too wide to fit.
On Windows, I've switched to using an edit box instead of a static
text control, making it readonly, and borderless via my existing
MakeDlgItemBorderless helper function. This doesn't get you an actual
scrollbar, but it does mean you can scroll left and right by dragging
with the mouse.
This doesn't apply to every GtkLabel I instantiate: only the ones
constructed as part of implementing the cross-platform CTRL_TEXT.
Those labels contain information that the dialog box is deliberately
communicating to the user, so it seems a sensible idea to make sure
they can be copy-pasted.
By default, this also seems to cause them to become able to take the
input focus, so I've reverted that. You can select them with the
mouse, but I think having them appear in the tab order is an
awkwardness too far, since they're not active in any other way.
This function has to make an array containing a specific number of
random values that are +1 or -1, and all the rest zero. The simplest
way I could think of to do it at first was to make the array with all
the zeroes at the end and then shuffle the array.
But I couldn't think of a time-safe algorithm to shuffle an array in
such a way that all orders come out equiprobable, that was better than
quadratic time. In fact I still can't think of one. (Making a random
Benes network is the best idea I've come up with: it arranges that
every output order is _possible_, and runs in O(N log N) time, but it
skews the probabilities, which makes it unacceptable.)
However, there's no need to shuffle an array in this application
anyway: we're not actually trying to generate a random _permutation_,
only a random element of (n choose w). So we can just walk linearly
along the array remembering how many nonzero elements we have yet to
output, and using an appropriately chosen random number at each step
to decide whether this will be one of them.
This isn't a significant improvement in the performance of NTRU
overall, but it satisfies my sense of rightness a little, and at least
means I don't have to have a comment in the code apologising for the
terrible algorithm any more.
Now the RSA signature-type checkboxes should be aligned with their
label; the 'Add' and 'Remove' buttons for wildcards should align with
the edit box for entering a wildcard; and the 'Load from file' button
for a public key aligns with the edit box for that.
Previously, in the code that instantiated the dialog.h portable
control spec, an edit control with width=100 would be implemented as a
small Columns object containing the label and the edit control atop
each other. Now, instead, the two controls are placed separately into
the containing Columns.
Combined with the changes just made to the align_next_to system, this
means that you can put buttons to the right of such an edit box and
have them line up with the actual edit box, instead of trying to line
up with the combination of the box and its label.
(The Windows alignment system already identified the specific edit box
control as the relevant one, so this was already working there.)
Various alignments I want to do in the host CA box have shown up
deficiencies in this system, so I've reworked it a bit.
Firstly, you can now specify more than two controls to be tied
together with an align_next_to (e.g. multiple checkboxes alongside
something else).
Secondly, as well as forcing the controls to be the same height as
each other, the layout algorithm will also move the later controls
further _downward_, so that their top y positions also line up. Until
now that hasn't been necessary, because they lined up already.
In the GTK implementation of this via the Columns class, I've renamed
'columns_force_same_height' to 'columns_align_next_to', and similarly
for some of the internal fields, since the latter change makes the
previous names a misnomer.
In the Windows implementation, I found it most convenient to set this
up by following a linked list of align_next_to fields backwards. But
it won't always be convenient to initialise them that way, so I've
also written a crude normaliser that will rewrite those links into a
canonical form. But I only call that on Windows; it's unnecessary in
GTK, where the Columns class provides plenty of per-widget extra
storage so I just keep each alignment class as a circular list.
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).
Introduced in dc7ba12253 earlier today. On GTK these caused no
problems worse than a GTK warning, but I'd better fix them before they
(potentially) do worse on Windows!
As distinct from the type of signature generated by the SSH server
itself from the host key, this lets you exclude (and by default does
exclude) the old "ssh-rsa" SHA-1 signature type from the signature of
the CA on the certificate.
This will allow the central host_ca_new function to pre-populate the
structure with default values for the fields, so that once I add more
options to CA configuration they can take their default values when
loading a saved record from a previous PuTTY version.
I mostly really like the use of 'ptrlen' in place of zero-terminated
strings in PuTTY, but one place it's awkward is when debuggging
through string-handling code, because gdb won't automatically show me
exactly what a ptrlen points to. Now it does.
This means that, on the one hand, an absentminded press of Return
doesn't dismiss the entire CA config box, which would be pretty
annoying if you were half way through entering a load of fiddly stuff.
And on the other hand, you _can_ press Escape to dismiss the box,
which is less likely to happen by accident.
This allows you to load a CA public key from a disk file (in any
format acceptable to ppk_load_pub, which means OpenSSH one-line public
keys and also RFC4716 ones).
In the course of polishing up this dialog box, I'm going to want it to
actually do cryptographic things (such as checking validity of a
public key blob and printing its fingerprint), which means it will
need to link against SSH utility functions.
So I've moved the dialog-box setup and handling code out of config.c
into a new file in the ssh subdirectory and in the ssh library, where
those facilities will be conveniently available.
This also means that dialog-box setup code _won't_ be linked into
PuTTYtel or pterm (on either platform), so I've added a stub source
file to provide its entry-point function in those tools. Also,
provided a const bool to indicate whether that dialog is available,
which we use to decide whether to recognise that command-line option.
Instead of an edit box together with a Browse button that pops up a
sub-dialog, this is _just_ the browse button, only now it has a
user-defined title. I'm about to want to use this for loading CA
public keys from files.
This causes PuTTY to bring up just the host CA configuration dialog
box, and shut down once that box is dismissed.
I can imagine it potentially being useful to users, but in the first
instance, I expect it to be useful to _me_, because it will greatly
streamline testing changes to the UI of that dialog!
