My bulk indentation check also turned up a lot of cases where a run-on
function call or if statement didn't have its later lines aligned
correctly relative to the open paren.
I think this is quite easy to do by getting things out of
sync (editing the first line of the function call and forgetting to
update the rest, perhaps even because you never _saw_ the rest during
a search-replace). But a few didn't quite fit into that pattern, in
particular an outright misleading case in unix/askpass.c where the
second line of a call was aligned neatly below the _wrong_ one of the
open parens on the opening line.
Restored as many alignments as I could easily find.
In several pieces of development recently I've run across the
occasional code block in the middle of a function which suddenly
switched to 2-space indent from this code base's usual 4. I decided I
was tired of it, so I ran the whole code base through a re-indenter,
which made a huge mess, and then manually sifted out the changes that
actually made sense from that pass.
Indeed, this caught quite a few large sections with 2-space indent
level, a couple with 8, and a handful of even weirder things like 3
spaces or 12. This commit fixes them all.
The fixed tab stops that we used to use in the old LBS_HASSTRINGS list
box, and that I carefully replicated in the new owner-drawn version,
are no more! Now, every time we refresh the key list, we actually
_measure_ the maximum size of string that needs to fit into each
column, and size the columns based on that.
Now I don't have to worry any more about whether the set of algorithm
names might one day overflow the fixed column width, or whether a
particularly unlucky choice of key with lots of wide letters like M
and W in its base64-encoded SHA256 hash might do the same.
Also, the previous column sizes were pessimistic (for reason of
exactly that worry), so this change generally moves things over
towards the left of the list box - which means there's now room for
longer key comments, and more chance of the suffixes '(encrypted)' or
'(re-encryptable)' being visible on the right.
The test in the Pageant list box code for whether we should display
the bit count of a key was done by checking specifically for ssh_rsa
or ssh_dsa, which of course meant that it didn't catch the certified
versions of those keys.
Now there's yet another footling ssh_keyalg method that asks the
question 'is it worth displaying the bit count?', to which RSA and DSA
answer yes, and the opensshcert family delegates to its base key type,
so that RSA and DSA certified keys also answer yes.
(This isn't the same as ssh_key_public_bits(alg, blob) >= 0. All
supported public key algorithms _can_ display a bit count if called
on. But only in RSA and DSA is it configurable, and therefore worth
bothering to print in the list box.)
Also in this commit, I've fixed a bug in the certificate
implementation of public_bits, which was passing a wrongly formatted
public blob to the underlying key. (Done by factoring out the code
from opensshcert_new_shared which constructed the _correct_ public
blob, and reusing it in public_bits to do the same job.)
If you load a certified key into Windows Pageant, the official SSH id
for the key type is so long that it overflows its space in the list
box and overlaps the key fingerprint hash.
This commit introduces yet another footling little ssh_keyalg method
which returns a shorter human-readable description of the key type,
and uses that in the Windows Pageant list box only.
(Not in the Unix Pageant list, though, because being output to stdout,
that seems like something people are more likely to want to
machine-read, which firstly means we shouldn't change it lightly, and
secondly, if we did change it we'd want to avoid having a variable
number of spaces in the replacement key type text.)
The main key list control in the Pageant window was previously an
ordinary LBS_HASSTRINGS list box, with tab characters aligning the
various parts of the key information into different columns. This was
fragile because any mistake in the font metrics could have overflowed
a tab stop and forced the text to move on to the next one.
Now I've switched the list box into LBS_OWNERDRAWFIXED mode, which
means that in place of a string for each list item I store a struct of
my choice, and I have to draw the list-box entries myself by
responding to WM_DRAWITEM. So now I'm drawing each component of the
key information as a separate call to ExtTextOut (plus one
TabbedTextOut to put the '(encrypted)' suffix on the end), which means
that the tab stops are now guaranteed to appear where I tell them to.
No functional change, for the moment: this is pure refactoring. As
closely as I can tell, the appearance of the list box is
pixel-for-pixel what it was before this commit. But it opens the door
for two further improvements (neither one done in this commit): I can
dynamically choose the tab stop locations based on querying the text
metrics of the strings that will actually need to fit in the columns,
and also, whatever reorganisation I need to do to make certificates
fit sensibly in this list box can now be done without worrying about
breaking anything terribly fragile.
The main list box in the Pageant key list window was identified by a
numeric control id, even though pageant-rc.h has a nice meaningful
macro name for it (and pageant.c uses that).
When PuTTYgen is holding a certified key, I don't think there's any
sensible use for pasting around the full public key in authorized_keys
format, because the whole point is that what you put in
authorized_keys is 'please trust this CA' rather than the specific
key. So instead I've reused the space in the dialog box to indicate
that it's a certificate, and provide a 'more info' sub-dialog.
I'm about to want setupbigedit1 and setupbigedit2 to know the control
ids themselves, and also add more controls to the enum, and it keeps
the diffs more legible if I move the entire enum around unchanged
_first_ and then start making small changes in the middle of it.
The recently added SeatDialogText type was just what I needed to add a
method to the ssh_key vtable for dumping certificate information in a
human-readable format. It will be good for displaying in a Windows
dialog box as well as in cmdgen's text format.
This commit introduces and implements the new method, and adds a
--cert-info mode to command-line Unix PuTTYgen that uses it. The
Windows side will follow shortly.
I must have dashed off that branch of the key reading function without
ever testing it, or I'd have noticed by now that it was looking for
the wrong string to terminate the file. Ahem.
Most of the Windows-specific dialog control construction functions
were passing their string parameters as 'char *' even though they were
string literals. Apparently none of our previous giant constification
patches spotted that.
A user reports that if the Print Spooler service is disabled via
services.msc, then PuTTY can report 'Out of memory!' when you try to
open the Terminal config pane, which is the one containing the combo
box enumerating the available printers.
Apparently this is because the call to EnumPrinters failed with the
error code other than the expected ERROR_INSUFFICIENT_BUFFER, and in
the process, left garbage in the pcbNeeded output parameter. That
wouldn't be too surprising if it had simply _not written_ to that
parameter and therefore it was never initialised at all in the calling
function printer_add_enum. But in fact, printer_add_enum *does*
precautionarily initialise needed=0 before the initial call to
EnumPrinters. So EnumPrinters must have actively written one of its
*own* uninitialised variables into it!
Anyway, the obvious fix is to distinguish ERROR_INSUFFICIENT_BUFFER
from any other kind of EnumPrinters failure (in fact turning off Print
Spooler seems to lead to RPC_S_SERVER_UNAVAILABLE), and not attempt to
proceed in the case of other failures.
In commit 694d5184b7, I introduced the 'just_button' flag for
CTRL_FILESELECT controls, and in commit ddcd93ab12 I added a use
of such a control with the flag set to true.
But I forgot to set it to false everywhere else, which caused an
assertion failure when selecting the Bell pane in Windows PuTTY. Oops.
In the 'xterm 216+' function key mode, a function key pressed with a
combination of Shift, Ctrl and Alt has its usual sequence like
ESC[n~ (for some integer n) turned into ESC[n;m~ where m-1 is a 3-bit
bitmap of currently pressed modifier keys.
This mode now also applies to the keys on the small keypad above the
arrow keys (Ins, Home, PgUp etc). If xterm 216+ mode is selected,
those keys are modified in the same way as the function keys.
As with the function keys, this doesn't guarantee that PuTTY will
_receive_ any particular shifted key of this kind, and not repurpose
it. Just as Alt+F4 still closes the window (at least on Windows)
rather than sending a modified F4 sequence, Shift+Ins will still
perform a paste action rather than sending a modified Ins sequence,
Shift-PgUp will still scroll the scrollback, etc. But the keys not
already used by PuTTY for other purposes should now have their
modern-xterm behaviour in modern-xterm mode.
Thanks to H.Merijn Brand for developing and testing a version of this
patch.
Whenever we successfully send some data to standard output/error,
we're supposed to notify the backend that this has happened, and tell
it how much backlog still remains, by calling backend_unthrottle().
In Unix Plink, the call to backend_unthrottle() was happening on some
but not all calls to try_output(). In particular, it was happening
when we called try_output() as a result of stdout or stderr having
just been reported writable by poll(), but not when we called it from
plink_output() after the backend had just sent us some more data. Of
course that _normally_ works - if you were polling stdout for
writability at all then it's because a previous call had returned
EAGAIN, so that's when you _have_ backlog to dispose of. But it's also
possible, by an accident of timing, that before you get round to doing
that poll, the seat passes you further data and you call try_output()
anyway, and by chance, the blockage has cleared. In that situation,
you end up having cleared your backlog but forgotten to tell the
backend about it - which might mean the backend never unfreezes the
channel or (in 'simple' mode) the entire SSH socket.
A user reported (and I reproduced) that when Plink is compiled on
MacOS, running an interactive session through it and doing
output-intensive activity like scrolling around in htop(1) can quite
easily get it into what turned out to be that stuck state. (I don't
know why MacOS and not any other platform, but since it's a race
condition, that seems like a plausible enough cause of a difference in
timing.)
Also, we were inconsistently computing the backlog size: sometimes it
was the total size of the stdout and stderr bufchains, and sometimes
it was just the size of the one we'd made an effort to empty.
Now the backlog size is consistently stdout+stderr (the same as it is
in Windows Plink), and the call to backend_unthrottle() happens
_inside_ try_output(), so that I don't have to remember it at every
call site.
In the case where a server presents a host key signed by a different
certificate from the one you've configured, it need not _always_ be
evidence of wrongdoing. I can imagine situations in which two CAs
cover overlapping sets of things, and you don't want to blanket-trust
one of them, but you do want to connect to a specific host signed by
that one.
Accordingly, PuTTY's previous policy of unconditionally aborting the
connection if certificate validation fails (which was always intended
as a stopgap until I thought through what I wanted to replace it with)
is now replaced by fallback handling: we present the host key
fingerprint to the user and give them the option to accept and/or
cache it based on the public key itself.
This means that the certified key types have to have a representation
in the host key cache. So I've assigned each one a type id, and
generate the cache string itself by simply falling back to the base
key.
(Rationale for the latter: re-signing a public key with a different
certificate doesn't change the _private_ key, or the set of valid
signatures generated with it. So if you've been convinced for reasons
other than the certificate that a particular private key is in the
possession of $host, then proof of ownership of that private key
should be enough to convince you you're talking to $host no matter
what CA has signed the public half this week.)
We now offer to receive a given certified host key type if _either_ we
have at least one CA configured to trust that host, _or_ we have a
certified key of that type cached. (So once you've decided manually
that you trust a particular key, we can still receive that key and
authenticate the host with it, even if you later delete the CA record
that it didn't match anyway.)
One change from normal (uncertified) host key handling is that for
certified key types _all_ the host key prompts use the stronger
language, with "WARNING - POTENTIAL SECURITY BREACH!" rather than the
mild 'hmm, we haven't seen this host before'. Rationale: if you
expected this CA key and got that one, it _could_ be a bold-as-brass
MITM attempt in which someone hoped you'd accept their entire CA key.
The mild wording is only for the case where we had no previous
expectations _at all_ for the host to violate: not a CA _or_ a cached
key.
In the macro automation for ssh2_bpp_check_unimplemented, I #defined
SSH2_BITMAP_WORD, and 20 lines later, tried to #undef it by the wrong
spelling. Of course this gave no error, so I didn't notice! But I
spotted it just now, so let's fix it.
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.
