The text of the host key warnings was replicated in three places: the
Windows rc file, the GTK dialog setup function, and the console.c
shared between both platforms' CLI tools. Now it lives in just one
place, namely ssh/common.c where the rest of the centralised host-key
checking is done, so it'll be easier to adjust the wording in future.
This comes with some extra automation. Paragraph wrapping is no longer
done by hand in any version of these prompts. (Previously we let GTK
do the wrapping on GTK, but on Windows the resource file contained a
bunch of pre-wrapped LTEXT lines, and console.c had pre-wrapped
terminal messages.) And the dialog heights in Windows are determined
automatically based on the amount of stuff in the window.
The main idea of all this is that it'll be easier to set up more
elaborate kinds of host key prompt that deal with certificates (if,
e.g., a server sends us a certified host key which we don't trust the
CA for). But there are side benefits of this refactoring too: each
tool now reliably inserts its own appname in the prompts, and also, on
Windows the entire prompt text is copy-pastable.
Details of implementation: there's a new type SeatDialogText which
holds a set of (type, string) pairs describing the contents of a
prompt. Type codes distinguish ordinary text paragraphs, paragraphs to
be displayed prominently (like key fingerprints), the extra-bold scary
title at the top of the 'host key changed' version of the dialog, and
the various information that lives in the subsidiary 'more info' box.
ssh/common.c constructs this, and passes it to the Seat to present the
actual prompt.
In order to deal with the different UI for answering the prompt, I've
added an extra Seat method 'prompt_descriptions' which returns some
snippets of text to interpolate into the messages. ssh/common.c calls
that while it's still constructing the text, and incorporates the
resulting snippets into the SeatDialogText.
For the moment, this refactoring only affects the host key prompts.
The warnings about outmoded crypto are still done the old-fashioned
way; they probably ought to be similarly refactored to use this new
SeatDialogText system, but it's not immediately critical for the
purpose I have right now.
They were previously using an ad-hoc system for getting hold of their
context structure, which had to be different between the WM_INITDIALOG
handler and everything else. That's exactly what ShinyDialogBox is
good for preventing, so let's use it, to save effort.
Partly, this just seems more sensible, since it may well vary per
platform beyond the ability of intorptr to specify. But more
immediately it means the definition of the HELPCTX macro doesn't have
to use the P() function from dialog.h, which isn't defined in any
circumstances outside the config subsystem. And I'm about to want to
put a help context well outside that subsystem.
This replaces the previous placeholder scheme of having a list of
hostname wildcards with implicit logical-OR semantics (if any wildcard
matched then the certificate would be trusted to sign for that host).
That scheme didn't allow for exceptions within a domain ('everything
in example.com except extra-high-security-machine.example.com'), and
also had no way to specify port numbers.
In the new system, you can still write a hostname wildcard by itself
in the simple case, but now those are just atomic subexpressions in a
boolean-logic domain-specific language I've made up. So if you want
multiple wildcards, you can separate them with || in a single longer
expression, and also you can use && and ! to impose exceptions on top
of that.
Full details of the expression language are in the comment at the top
of utils/cert-expr.c. It'll need documenting properly before release,
of course.
For the sake of backwards compatibility for early adopters who've
already set up configuration in the old system, I've put in some code
that will read the old MatchHosts configuration and automatically
translate it into the equivalent boolean expression (by simply
stringing together the list of wildcards with || between them).
These make a good storage format for mostly-textual data in
configuration, if it can't afford to reserve any character as a
delimiter. Assuming very few characters need to be escaped, the space
cost is lower than base64, and also you can read it by eye.
ptrlen_contains and ptrlen_contains_only are useful for checking that
a string is composed entirely of certain characters, or avoids them.
ptrlen_end makes a pointer to the byte just past the end of the
specified string. And it can be used with make_ptrlen_startend, which
makes a ptrlen out of two pointers instead of a pointer and a length.
This manipulates the selection inside an edit box, to select a
specific range of characters in the contained text. The idea is that
you can use it as a means of error highlighting, if the user has
entered something invalid in that edit box and you want to draw their
attention to the specific location of the part you're unhappy with.
The only *use* of it was removed in commit 6a743399b0, where
instead of blocking the GTK signal that caused a string to be
overwritten, I switched to making a temporary copy of the string. But
I didn't notice that the declaration and assignments could be cleaned
up too.
Large chunks of the GTK setup code had a 2-space indent for some
reason, in place of the usual 4-space in this code base. I've been
meaning to sort it out for ages, because it makes it hard to have a
single set of editor settings suitable for the whole code base.
The polynomial Stein's algorithm in that code was adapted from the
binary Stein in mpint.c. One of the comments which originally said
'dividing by 2' should have been updated to say 'dividing by x' in the
polynomial case, and didn't.
If you had a key stored encrypted in Pageant, and you launched two
PuTTY sessions both trying to generate a signature with the key, and
then didn't type the passphrase into the async passphrase prompt until
after both sessions had made requests, then only one of the requests
would be replied to once you decrypted the key. The other would sit
there waiting for a response that Pageant never got round to sending.
This would also happen in the case where you launched two PuTTY
sessions each trying to use a _different_ encrypted key in Pageant, in
which case Pageant should have presented the next GUI passphrase box
after the first one was dismissed. That didn't happen either.
This was the result of two separate bugs in the code. Firstly, when
signop_coroutine() noticed that a GUI request was already in progress,
it would crReturn without making any arrangement to be called back.
Now there's a queue of 'requests blocked on waiting for some GUI
prompt to finish'.
Secondly, if multiple requests were blocked on the same key, the code
that went over the linked list of them had a bug in which it would
unblock the _first_ list item in every iteration, instead of
unblocking all the items once each.
ctrl_radiobuttons has a variadic argument list that it expects to be
terminated by a null pointer where a const char * should be. So the
terminating NULL at each call site ought to be cast to const char *,
for the usual reason (NULL might expand literally to something not the
same size as a pointer and get mis-marshalled in the variadic argument
list). It wasn't being.
Fixed in the same way as dupcat: I've turned ctrl_radiobuttons into a
macro wrapper on the underlying function, which adds the NULL
automatically with its correct cast. This also saves verbiage at every
call site, because now I can leave out all the NULLs there.
Instead of maintaining a single sparse table mapping Unicode to the
currently selected code page, we now maintain a collection of such
tables mapping Unicode to any code page we've so far found a need to
work with, and we add code pages to that list as necessary, and never
throw them away (since there are a limited number of them).
This means that the wc_to_mb family of functions are effectively
stateless: they no longer depend on a 'struct unicode_data'
corresponding to the current terminal settings. So I've removed that
parameter from all of them.
This fills in the missing piece of yesterday's commit a216d86106:
now wc_to_mb too should be able to handle internally-implemented
character sets, by hastily making their reverse mapping table if it
doesn't already have it.
(That was only a _latent_ bug, because the only use of wc_to_mb in the
cross-platform or Windows code _did_ want to convert to the currently
selected code page, so the old strategy worked in that case. But there
was no protection against an unworkable use of it being added later.)
A user points out that the new charset-aware window title setting
doesn't work if the configured character set is one of the entries in
cp_list[] based on a hard-coded Unicode translation table, such as the
ISO 8859 family.
That's because the Windows mb_to_wc() function assumes that the code
page it's given will always be OK to pass to the Windows API function
MultiByteToWideChar, forgetting that for those internally implemented
single-byte character sets are not.
This commit adds a manual implementation of SBCS -> Unicode based on
those tables, which restores the ability to set a window title
specified in ISO 8859.
However, it's not a full fix to windows/unicode.c in general, because
wc_to_mb has a similar blind spot: it's only prepared to convert
Unicode to an internally implemented SBCS if that SBCS happens to be
the one currently set in ucsdata->line_codepage, because that's when
we've already prepared the reverse lookup table. Probably we ought to
sort that out, and arrange that it can make the reverse lookup table
if suddenly called on to do a different conversion. But that needs
more refactoring, so I haven't done it in this commit.
This is the first release for which I've had to submit a revised Store
entry, and now I've worked out how to do it, I should write it down
for next time.
In the echo "\\versionid foo" statement, the double \ turns into a
single \ during dash's expansion phase, and the remaining '\v' turns
into a vertical tab when dash's 'echo' builtin processes it. We need
twice as many \ to generate a literal \ in the actual output.
I'd forgotten to initialise it at all, which meant it was set to zero
by the initial memset of the whole BidiContext on creation. But in our
enumeration of bidi character types, zero corresponds to L (the most
common left-to-right alphabetic character class), and as a value for
paragraphOverride, that is not neutral.
As a result, a command such as this (assuming UTF-8)
echo -e '\xD7\x90\xD7\x91'
would produce Hebrew aleph and beth in the correct display order
(aleph on the right), but aligned to the left margin of the terminal
instead of the right margin, because the overall direction of the line
was taken to be forcibly overridden to "left-to-right" instead of
being inferred dynamically from the line contents.
do_bidi() is a tiny wrapper on the inner function that does all the
real work. And the inner function has been subjected to the whole
Unicode 14 bidi conformance test. So naturally, the "trivial" but
untested function just outside it is where the embarrassing bug was.
Fixes a cosmetic issue where the new ConPTY error added in 4ae8b742ab
had an ugly "Unable to open connection to".
(Arguably this ought to test a backend property rather than
cmdline_tooltype.)
I thought of a couple more things it's worth double-checking at
release time, and also, moved the 'merge to main' instructions from
the RC build step to the final release phase, because that way they
don't have to be pointlessly redone if commits have appeared on main
in between.
This makes pterm.exe support the same (very small) subset of the
standard option collection that Unix pterm does. Namely, -load (which
won't do anything useful with a hostname to connect to, but is still
useful if you have a saved session containing configuration like
colours or default size or what have you), and also -sessionlog.
To make this work, I've had to move the 'tooltype' definition out of
window.c into {putty,pterm}.c, so that it can be defined differently
in the two.
They were accepted by Unix pterm, by mistake, and then totally ignored
because nothing in pterm ever makes a network connection, so nothing
cares whether you configured it to use IPv4 or IPv6.
Turns out that standard C 'size_t' and the Win32 API's 'SIZE_T' aren't
the same integer type in all cases! They have the same _size_, but in
32-bit, one of them is a typedef for 'unsigned int' and the other for
'unsigned long', leading to annoying pointer-type mismatch warnings.
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.
