GtkIMContext has focus_in and focus_out methods for telling it when the
corresponding widget gains or loses keyboard focus. It's not obvious to
me why these are necessary, but PuTTY now calls them when it sees
focus-in and focus-out events for the terminal window. Somehow, this
has caused Hangul input to start working in PuTTY. I can't yet
see what I'm typing for lack of proper preedit support, though.
[ECMA-48] section 8.3.27 specifies the format of Device Control String
(DCS) commands which are used for XTGETTCAP and other sequences.
We don't parse DCS commands. This causes this command to wrongly
output some characters:
printf '\033P+q616d\033\\'
Fix that by parsing DCS commands just like other OSC-like commands.
(Apart from the initial characters, DCS has the same format as OSC.)
We also allow 0x07 as sequence terminator which does not seem specified
but a lot of people use it with OSC; it's fine because 0x07 is not
allowed in the OSC/DCS payload.
[ECMA-48]: https://www.ecma-international.org/wp-content/uploads/ECMA-48_2nd_edition_august_1979.pdf
It was a bit far to the right, looking at risk of falling off. Now
moved it as far left as it will go without the top right corner of the
computer monitor peeking out from behind it.
If I'm going to use this as a means of generating bitmap icons at
large sizes, I want it to support all the same modes as the existing
bitmap script. So this adds a mode to the SVG generator that produces
the same black and white colour scheme as the existing monochrome
bitmap icons.
(Plus, who knows, the black and white SVGs might come in useful for
other purposes. Printing as a logo on black-and-white printers springs
to mind.)
The existing monochrome icons aren't greyscale: all colours are
literally either black or white, except for the cardboard box in the
installer icon, which is halftoned. Here I've rendered that box as
mid-grey. When I convert the rendered SVG output to an actual
1-bit (plus alpha) image, I'll have to redo that halftoning.
It looked nasty that the back corner of the monitor didn't line up
exactly with the outline of the system box behind it. Now I choose the
y offset between the two components to ensure it does. Also adjusted
the monitor's depth so that it fits better with the new alignment.
We weren't building _all_ the icons in true-colour mode, because most
don't change anyway. The installer ones do, so let's build them. Works
better with the preview page.
A user reported recently that if you connect to a Telnet server via a
proxy that requires authentication, and enter the auth details
manually in the PuTTY terminal window, then the entire Telnet session
is shown with trust sigils to its left.
This happens because telnet.c calls seat_set_trust_status(false) as
soon as it's called new_connection() to make the Socket. But the
interactive proxy authentication dialogue hasn't happened yet, at that
point. So the proxy resets the trust status to true and asks for a
username and password, and then nothing ever resets it to false,
because telnet.c thought it had already done that.
The solution is to defer the Telnet backend's change of trust status
to when we get the notification that the socket is properly connected,
which arrives via plug_log(PLUGLOG_CONNECT_SUCCESS).
The same bug occurs in raw.c and supdup.c, but not in rlogin.c,
because Rlogin has an initial authentication exchange known to the
protocol, and already delays resetting the trust status until after
that has concluded.
Colin Watson reported that a build failure occurred in the AArch64
Debian build of PuTTY 0.83:
gcc now defaults to enabling branch protection using AArch64 pointer
authentication, if the target architecture version supports it.
Debian's base supported architecture does not, but Armv8.4-A does. So
when I changed the compile flags for enable_dit.c to add
-march=armv8.4-a, it didn't _just_ allow me to write the 'msr dit, %0'
instruction in my asm statement; it also unexpectedly turned on
pointer authentication in the containing function, which caused a
SIGILL when running on a pre-Armv8.4-A CPU, because although the code
correctly skipped the instruction that set DIT, it was already inside
enable_dit() at that point and couldn't avoid going through the
unsupported 'retaa' instruction which tries to check an auth code on
the return address.
An obvious approach would be to add -mbranch-protection=none to the
compile flags for enable_dit.c. Another approach is to leave the
_compiler_ flags alone, and change the architecture in the assembler,
either via a fiddly -Wa,... option or by putting a .arch directive
inside the asm statement. But both have downsides. Turning off branch
protection is fine for the Debian build, but has the unwanted side
effect of turning it off (in that one function) even in builds
targeting a later architecture which _did_ want branch protection. And
changing the assembler's architecture risks changing it _down_ instead
of up, again perhaps invalidating other instructions generated by the
compiler (like if some later security feature is introduced that gcc
also wants to turn on by default).
So instead I've taken the much simpler approach of not bothering to
change the target architecture at all, and instead generating the move
into DIT by hardcoding its actual instruction encoding. This meant I
also had to force the input value into a specific register, but I
don't think that does any harm (not _even_ wasting an extra
instruction in codegen). Now we should avoid interfering with any
security features the compiler wants to turn on or off: all of that
should be independent of the instruction I really wanted.
We used to have a practice of \IM-ing every command-line option for the
index, but haven't kept it up.
Add these for all existing indexed command-line options, plus some
related tidying.
(That's Halibut's non-breaking hyphen.)
Triggered by noticing that the changes in 54f6fefe61 happened to come
out badly in the text-only rendering, but I noticed there were many more
instances in the main docs where non-breaking hyphens would help.
After all these years, this checklist is _still_ hard for me to get
right. In the 0.83 runup this month, I prepared everything about the
RC build in advance, but nothing about the announcements, website
updates etc, and had to do all of that on release day.
So I've completely removed the section "Preparing to make the
release", which was ambiguous about whether it's done in advance or on
the day. Now all the text parts (website, wishlist, announcements) are
folded into the "make a release candidate" section, in the hope that
I'll remember to do them all at the same time, which should mean
- people have a few days to review the text _and_ test the RC build
- because they go together, I also remember to revise the text if a
new RC build is needed (e.g. mention whatever extra fix it has).
The "actual release procedure" section is now down to _only_ the
things I have to do on the day, which is basically uploading
everything, going live, and communicating the release.
Spotted by Coverity: we've just allocated a strbuf to hold the output
of the classical half of the hybrid key exchange, but if that output
isn't generated due to some kind of failure, we forgot to free the
strbuf on exit.
This was introduced in commit e7acb9f6968d482, as a side effect of
also wanting to call wmemchr to find a NUL wide character in the
buffer returned from GetDlgItemTextW. But the previous commit has
superseded that code, so now we don't use wmemchr in this code base
any more. Remove the machinery that provides it, saving a now-useless
cmake configure-time check.
This rewrite, due to SATO Kentaro, uses GetWindowTextLength (which I
hadn't known about) to find the correct size to allocate the buffer
the first time, avoiding the need to keep growing it until a call to
GetDlgItemText doesn't have to truncate the result.
[SGT: the helper function do_filereq_w expects its filename size to be
in characters, not bytes, because it's used both as an index into the
wchar_t buffer and also as nMaxFile in the OPENFILENAMEW structure
which is also documented as measured in characters. So at the call
site it should be measured via lenof rather than sizeof. This patch
has done the same with the char version, which makes no functional
difference but keeps the code more consistent.]
A user reported that the following sequence of events leads to Pageant
crashing:
- load an encrypted key into Pageant for decryption later
- attempt to use the key, so that Pageant prompts for the passphrase
- before entering the passphrase, abort the attempt to use the
key (e.g. by closing the PuTTY that was trying to use it)
- now enter the passphrase at the Pageant prompt, once the need for
it has gone away.
Once the key is decrypted, unblock_requests_for_key() goes through the
linked list of blocked PageantSignOp attached to the private key
record it's just decrypted, and tries to unblock them. The
PageantSignOp belonging to the aborted Pageant request is still linked
on that list, which it shouldn't be, because it's also been freed by
pageant_unregister_client when that traversed the separate linked list
of PageantAsyncOp associated with that client connection. So the
private key's list of blocked requests contained a stale pointer.
Now PageantSignOp's implementation of the PageantAsyncOp free method
makes sure to unlink the signop from any list it's on before freeing
it.
This can come up, for example, if the terminal receives a ^E character
and has an empty answerback string configured.
Without this early return, we append zero bytes to ldisc's ordinary
bufchain input_queue, which is harmless; but we also append a
zero-length record to ldisc's list of (type, length) chunks describing
which parts of the input bufchain should be treated as interactive or
as coming from special dedicated keystrokes (e.g. telling Return apart
from ^M).
That zero-length record is not _immediately_ harmful, but when the
user next presses a key, it will have a different type from the empty
answerback data, so that another chunk record is appended to the list
after the zero-length one. And then ldisc_input_queue_callback goes
into a tight loop, because it keeps trying to consume bytes from the
start of the input bufchain but bounding the size at the length of the
first (type, length) chunk, which is zero. So it consumes 0 bytes,
finds the bufchain still isn't empty, and loops round again.
When retrieving Unicode text from an edit box in the GUI configurer,
we were using plain memchr() to look for a terminating NUL. But of
course you have to use wmemchr() to look for a UTF-16 NUL, or else
memchr() will generate a false positive on the UTF-16 version of (at
least) any ASCII character!
(I also have to provide a fallback implementation of wmemchr for the
w32old builds, which don't have it in the libc they build against.
It's as simple as possible, and we use the libc version where
possible.)
This is a combined cherry-pick of three consecutive commits from main:
b088d77d580b8f7 GTK: hard-code some last-ditch fallback fonts.
7f4cccde2ae53c0 GTK: fixes to the previous font fallback patch.
6155365076c47a8 GTK: switch the default to client-side fonts.
The combined effect is that now PuTTY's built-in default font is
client-side rather than server-side (advantaging Wayland and
disadvantaging legacy GTK1 builds, which seems like a sensible
tradeoff these days), and also, if the configured main font can't be
found, we'll try falling back to either the client- or server-side
default (whichever is available) before giving up completely and
whinging on standard error.
"server:fixed" was a good default when GTK1 was common and non-X11
environments were rare. Now it's the other way round - Wayland is very
common and the GTK1 configuration of PuTTY is legacy - so it's time to
make the default GTK font a client-side one.
Of course, anyone with an existing saved session (including Default
Settings) won't be affected by this change; it only helps new users
without an existing ~/.putty at all. That's why we _also_ need the
fallbacks introduced by the previous couple of commits. But we can at
least start making it sensible for new users.
(I considered keeping the #if, and switching it round so that it tests
GTK_CHECK_VERSION(2,0,0) rather than NOT_X_WINDOWS, i.e. selects the
client-side default whenever client-side fonts _are_ available,
instead of only when server-side fonts _aren't_. That way, in GTK1
builds, the Conf default font would _still_ be "server:fixed". But I
think this is firstly too marginal to worry about, and secondly, it's
more futureproof to make the default the same everywhere: if anyone
still stuck on a GTK1 environment later manages to update it, then
their saved settings are less likely to have had a legacy thing
written into them. And the GTK1 build will still run out of the box
because of the last-ditch fallback mechanism I've just added.)
I'd forgotten that I'd already chosen a default client-side font, for
NOT_X_WINDOWS builds. I should have made the two defaults match! Now
both default font names are defined in the header file.
If the user's choice of fonts can't be instantiated during initial
terminal-window setup, then we now automatically try two fallback
options, "client:Monospace 10" and "server:fixed", and only give a
fatal error if _no_ option allows us to open a terminal window.
Previously, on Wayland, PuTTY and pterm with default configuration
would completely fail to open a terminal window at all, because our
default font configuration is still the trad X11 "server:fixed", and
on Wayland, X11-style server-side bitmap fonts don't exist at all.
Conversely, in the GTK1 build of PuTTY which we're still supporting,
_client-side_ fonts aren't supported, so if a user had configured one
in their normal PuTTY configuration, then the GTK1 version would
similarly fail to launch.
Now both of those cases should work, because the fallbacks include a
client-side font _and_ a server-side one, and I hope that any usable
Pango system will make "Monospace" map to _some_ locally available
font, and that any remotely sensible X server has 'fixed'
I think it would be even better if there was a mechanism for the Conf
to specify a fallback list of fonts. For example, this might be
specified via a new multifont prefix along the lines of
choices:client:Monospace 10:server:fixed
with the semantics that the "choices:" prefix means that the rest of
the string is split up at every other colon to find a list of fonts to
try to make. Then we could not only set PuTTY's default to a list of
possibilities likely to find a usable font everywhere, but also, users
could configure their own list of preferred fallbacks.
But I haven't thought of a good answer to the design question of what
should happen if a Conf font setting looks like that and the user
triggers the GUI font selector! (Also, you'd need to figure out what
happened if a 'choices:' string had another 'choices' in it...)
The cmake script that determines the current git head commit, in order
to bake it into the binaries for development builds from a git
checkout, wasn't working reliably on Windows: sometimes it reported
that the source directory didn't seem to be a git repository, when in
fact it was.
This occurred because gitcommit.cmake starts by trying to work out
_whether_ the source directory is the top level of a git worktree at
all, by the technique of running `git rev-parse --show-toplevel` (to
print the top-level path of the git worktree _containing_ $PWD, if
any) and comparing it with ${CMAKE_SOURCE_DIR}. But the comparison was
done as a plain string, which leads to problems if more than one
string can represent the same actual directory.
On Windows, this can occur for two reasons that I know of. One reason
is related to Windows itself: if you map a network file server path to
a local drive letter, then the same directory is accessible as a UNC
path (e.g. \\hostname\share\subdir) and via the drive letter (e.g.
N:\subdir). And it can happen that CMAKE_SOURCE_DIR and git's output
disagree on which representation to use, causing the string comparison
to return a false negative.
(This can also affect filesystems in a WSL instance, accessed from
native Windows via \\wsl$\instance\path, because Windows implements
that as a network file share even though the network in question is
purely in the imagination of that one machine.)
The other reason is related more specifically to git, because some
versions of Windows git are built on top of MSYS or MINGW or that kind
of shim layer, and model Windows drive letters as subdirectories of a
Unixlike VFS root. So you might also find that the two strings
disagree on whether you're in C:\Users\alice\src\putty or
/c/Users/alice/src/putty.
I think this commit should work around both of these problems. Reading
the man page for `git rev-parse` more carefully, it has an option
`--show-cdup`, which returns a _relative_ path from $PWD to the
top-level directory of the worktree: that is, it will be a sequence of
`../../..` as long as necessary, including length zero. So you can use
that to directly query whether you're at the top of a git worktree: if
`git rev-parse --show-cdup` returns the empty string and a success
status, then you are. (If you're deeper in a worktree it will return a
non-empty string, and if you're not in a worktree at all it will
return a failure status and print an error message to stderr.)
This is a cherry-pick of Stefan Tauner's patch from main, but without
my followup refactoring, since the refactoring seemed to me to have a
(small but easily avoidable) chance of introducing a bug in 0.83.
The only downside of the original patch is that it contains a variable
name telling a lie: 'osc_is_apc' should really read 'this isn't an OSC
but one of APC, SOS and PM'. But we don't actually treat those three
things differently, so the functionality is fine.
(cherry picked from commit b6b95f23e563211437e51322edc9118b63a3ca40)
I didn't like the previous patch setting a flag claiming that two
kinds of thing were APC which aren't in fact APC. So I've made a
little enum to distinguish them in the code. There's an outside chance
we might want to handle some case of these in future, in which case
this makes it easier, but more likely it's just making me feel less
wrong about it.
But I've also folded the two existing flags osc_is_apc and osc_w into
that single enum field, which I think is an improvement.
SOS (Start of string) and PM (privacy message) are opening delimiters defined
in ECMA-48 similar to APC. With this change they are treated exactly like APC,
i.e., like fake OSC sequences that are ignored and not printed.
Signed-off-by: Stefan Tauner <stefan.tauner@artech.at>
DIT, for 'Data-Independent Timing', is a bit you can set in the
processor state on sufficiently new Arm CPUs, which promises that a
long list of instructions will deliberately avoid varying their timing
based on the input register values. Just what you want for keeping
your constant-time crypto primitives constant-time.
As far as I'm aware, no CPU has _yet_ implemented any data-dependent
optimisations, so DIT is a safety precaution against them doing so in
future. It would be embarrassing to be caught without it if a future
CPU does do that, so we now turn on DIT in the PuTTY process state.
I've put a call to the new enable_dit() function at the start of every
main() and WinMain() belonging to a program that might do
cryptography (even testcrypt, in case someone uses it for something!),
and in case I missed one there, also added a second call at the first
moment that any cryptography-using part of the code looks as if it
might become active: when an instance of the SSH protocol object is
configured, when the system PRNG is initialised, and when selecting
any cryptographic authentication protocol in an HTTP or SOCKS proxy
connection. With any luck those precautions between them should ensure
it's on whenever we need it.
Arm's own recommendation is that you should carefully choose the
granularity at which you enable and disable DIT: there's a potential
time cost to turning it on and off (I'm not sure what, but plausibly
something of the order of a pipeline flush), so it's a performance hit
to do it _inside_ each individual crypto function, but if CPUs start
supporting significant data-dependent optimisation in future, then it
will also become a noticeable performance hit to just leave it on
across the whole process. So you'd like to do it somewhere in the
middle: for example, you might turn on DIT once around the whole
process of verifying and decrypting an SSH packet, instead of once for
decryption and once for MAC.
With all respect to that recommendation as a strategy for maximum
performance, I'm not following it here. I turn on DIT at the start of
the PuTTY process, and then leave it on. Rationale:
1. PuTTY is not otherwise a performance-critical application: it's
not likely to max out your CPU for any purpose _other_ than
cryptography. The most CPU-intensive non-cryptographic thing I can
imagine a PuTTY process doing is the complicated computation of
font rendering in the terminal, and that will normally be cached
(you don't recompute each glyph from its outline and hints for
every time you display it).
2. I think a bigger risk lies in accidental side channels from having
DIT turned off when it should have been on. I can imagine lots of
causes for that. Missing a crypto operation in some unswept corner
of the code; confusing control flow (like my coroutine macros)
jumping with DIT clear into the middle of a region of code that
expected DIT to have been set at the beginning; having a reference
counter of DIT requests and getting it out of sync.
In a more sophisticated programming language, it might be possible to
avoid the risk in #2 by cleverness with the type system. For example,
in Rust, you could have a zero-sized type that acts as a proof token
for DIT being enabled (it would be constructed by a function that also
sets DIT, have a Drop implementation that clears DIT, and be !Send so
you couldn't use it in a thread other than the one where DIT was set),
and then you could require all the actual crypto functions to take a
DitToken as an extra parameter, at zero runtime cost. Then "oops I
forgot to set DIT around this piece of crypto" would become a compile
error. Even so, you'd have to take some care with coroutine-structured
code (what happens if a Rust async function yields while holding a DIT
token?) and with nesting (if you have two DIT tokens, you don't want
dropping the inner one to clear DIT while the outer one is still there
to wrongly convince callees that it's set). Maybe in Rust you could
get this all to work reliably. But not in C!
DIT is an optional feature of the Arm architecture, so we must first
test to see if it's supported. This is done the same way as we already
do for the various Arm crypto accelerators: on ELF-based systems,
check the appropriate bit in the 'hwcap' words in the ELF aux vector;
on Mac, look for an appropriate sysctl flag.
On Windows I don't know of a way to query the DIT feature, _or_ of a
way to write the necessary enabling instruction in an MSVC-compatible
way. I've _heard_ that it might not be necessary, because Windows
might just turn on DIT unconditionally and leave it on, in an even
more extreme version of my own strategy. I don't have a source for
that - I heard it by word of mouth - but I _hope_ it's true, because
that would suit me very well! Certainly I can't write code to enable
DIT without knowing (a) how to do it, (b) how to know if it's safe.
Nonetheless, I've put the enable_dit() call in all the right places in
the Windows main programs as well as the Unix and cross-platform code,
so that if I later find out that I _can_ put in an explicit enable of
DIT in some way, I'll only have to arrange to set HAVE_ARM_DIT and
compile the enable_dit() function appropriately.
I'm not sure why these have never bothered me before, but a test build
I just made for a completely different reason complained about them!
findtest() did a binary search using a while loop, and then used
variables set in the loop body, which gcc objected to on the grounds
that the body might have run 0 times and not initialised those
variables. Also in the same function gcc objected to the idea that
findrelpos234() might have returned NULL and not set 'index'. I think
neither of these things can actually have _happened_, but let's stop
the compiler complaining anyway.
In 0.81 and before, we put an application manifest (XML-formatted
Windows resource) into all the GUI tools on purpose, and the CLI tools
like Plink didn't have one. But in 0.82, the CLI tools do have one,
and it's a small default one we didn't write ourselves, inserted by
some combination of cmake and clang-imitating-MSVC (I haven't checked
which of those is the cause).
This appears to have happened as a side effect of a build-tools
update, not on purpose. And its effect is that Windows XP now objects
to our plink.exe, because it's very picky about manifest format (we
have an old 'xp-wont-run' bug record about that).
Since it seemed to work fine to not have a manifest at all in 0.81,
let's go back to that. We were already passing /manifest:no to inhibit
the default manifest in the GUI tools, to stop it fighting with our
custom one; now I've moved /manifest:no into the global linker flags,
so it's applied to _all_ binaries, whether we're putting our own
manifest in or not.
In protocols other than PROT_RAW, the new line editing system differed
from the old one in not considering ^M or ^J (typed using the actual
Ctrl key, so distinct from pressing Return) to mean "I've finished
editing this line, please send it". This commit reinstates that
behaviour.
It turned out that a third-party tool (namely PuTTY Connection Manager),
which automatically answers prompts for the user, was terminating them
by sending ^J in place of the Return key. We don't know why (and it's
now unmaintained), but it was. So this change should make that tool
start working again.
I exclude PROT_RAW above because in that protocol the line editing has
much weirder handling for ^M and ^J, which lineedit replicated
faithfully from the old code: either control character by itself is
treated literally (displaying as "^M" or "^J" in the terminal), but if
you type the two in sequence in that order, then the ^J deletes the ^M
from the edit buffer and enters the line, so that the sequence CR LF
acts as a newline overall. I haven't changed that behaviour here, but
I have added a regression test of it to test_lineedit.
