When running on Wayland, gdk_display_get_name() can return things like
"wayland-0" rather than valid X display names. PuTTY nonetheless
treated them as X display names, meaning that when running under
Wayland, pterm would set DISPLAY to "wayland-0" in subprocesses, and
PuTTY's X forwarding wouldn't work properly.
To fix this, places that call gdk_display_get_name() now only do so on
displays for which GDK_IS_X_DISPLAY() is true. As with
GDK_IS_X_WINDOW(), this requires some backward-compatibility for GDK
versions where everything is implicitly running on X.
To make this work usefully, pterm now also won't unset DISPLAY if it
can't get an X display name and instead will pass through whatever value
of DISPLAY it received. I think that's better behaviour anyway.
There are two separate parts of PuTTY that call gdk_display_get_name().
platform_get_x_display() in unix/putty.c is used for X forwarding, while
gtk_seat_get_x_display() in unix/window.c is used used for setting DISPLAY
and recording in utmp. I've updated both of them.
When the user pressed Return at the end of a line, we were calling the
TermLineEditor's receiver function once for each character in the line
buffer. A Telnet user reported from looking at packet traces that this
leads to each character being sent in its own TCP segment, which is
wasteful and silly, and a regression in 0.82 compared to 0.81.
You can see the SSH version of the phenomenon even more easily in
PuTTY's own SSH logs, without having to look at the TCP layer at all:
you get a separate SSH2_MSG_CHANNEL_DATA per character when sending a
line that you entered via local editing in the GUI terminal.
The fix in this commit makes lineedit_send_line() collect keystrokes
into a temporary bufchain and pass them on to the backend in chunks
the size of a bufchain block.
This is better, but still not completely ideal: lineedit_send_line()
is often followed by a call to lineedit_send_newline(), and there's no
buffering done between _those_ functions. So you'll still see a
separate SSH message / Telnet TCP segment for the newline after the
line.
I haven't fixed that in this commit, for two reasons. First, unlike
the character-by-character sending of the line content, it's not a
regression in 0.82: previous versions also sent the newline in a
separate packet and nobody complained about that. Second, it's much
more difficult, because newlines are handled specially - in particular
by the Telnet backend, which sometimes turns them into a wire sequence
CR LF that can't be generated by passing any literal byte to
backend_send. So you'd need to violate a load of layers, or else have
multiple parts of the system buffer up output and then arrange to
release it on a toplevel callback or some such. Much more code, more
risk of bugs, and less gain.
This occurred if the SSH server closed the connection for any
reason (in practice usually a timeout, but reproducible more easily by
manually killing a test server process) while the user was in the
middle of any kind of interactive prompt-based login in the GUI PuTTY
terminal (be it simple password, k-i, private key passphrase,
whatever).
The problem was that term->userpass_state wasn't cleaned up when the
connection died, and then if you started a fresh SSH session in the
same terminal, the attempt to create a new term->userpass_state would
find there was one already there.
The simplest place to insert the missing cleanup is the call to
term_provide_backend(), because that's a terminal API function which
is already called to notify the terminal that one backend has gone
away and the next one has turned up.
(In fact, it's called twice, once to set term->backend to NULL when
the first session closes, and again when the session is restarted. I
see no harm in making the cleanup unconditional, not bothering to tell
the difference between the two cases.)
This centralises into windows/utils/request_file.c all of the code
that deals with the OPENFILENAME structure, and decides centrally
whether to use the Unicode or ANSI version of that structure and its
associated APIs. Now the output of any request_file function is our
own 'Filename' abstract type, instead of a raw char or wchar_t buffer,
which means that _any_ file dialog can produce a full Unicode filename
if the user wants to select one - and yet, in the w32old build, they
all uniformly fall back to the ANSI version, which is the only one
that works at all pre-NT.
A side effect: I've turned the FILTER_FOO_FILES family of definitions
from platform-specific #defines into a reasonably sensible enum. This
didn't affect the GTK side of things , because I'd never got round to
figuring out how to filter a file dialog down to a subset of files in
GTK, and still haven't. So I've just moved the existing FIXME comment
from platform.h to dialog.c.
A user reported shortly after 0.82 was released that they were
experiencing display corruption when connecting to a PDP-10 running
ITS using PuTTY's SUPDUP backend, and that the nature of the
corruption was consistent with a missing clear-to-EOL operation.
Without the SUPDUP or ITS expertise to debug it ourselves, we are
indebted to Scott Michel for identifying where, and providing a patch.
(However, now that the patch is presented, it's obvious even to me
that a line should be cleared here! The comment in PuTTY's own code
mentions clearing the line that the cursor has moved on to, and the
same text appears in RFC 734.)
draft-kampanakis-curdle-ssh-pq-ke defines the packet names
SSH_MSG_KEX_HYBRID_INIT and SSH_MSG_KEX_HYBRID_REPLY. They have the
same numbers as ECDH_INIT and ECDH_REPLY, and don't change anything
else, so this is just a naming change. But I think it's a good one,
because the post-quantum KEMs are less symmetric than ECDH (they're
much more like Ben's RSA kex in concept, though very different in
detail), and shouldn't try to pretend they're the same kind of thing.
Also this enables logparse.pl to give a warning about the fact that
one string in each packet contains two separate keys glomphed together.
For the latter reason (and also because it's easier in my code
structure) I've also switched to using the HYBRID naming for the
existing NTRU + Curve25519 hybrid method, even though the
Internet-Draft for that one still uses the ECDH names. Sorry, but I
think it's clearer!
As standardised by NIST in FIPS 203, this is a lattice-based
post-quantum KEM.
Very vaguely, the idea of it is that your public key is a matrix A and
vector t, and the private key is the knowledge of how to decompose t
into two vectors with all their coefficients small, one transformed by
A relative to the other. Encryption of a binary secret starts by
turning each bit into one of two maximally separated residues mod a
prime q, and then adding 'noise' based on the public key in the form
of small increments and decrements mod q, again with some of the noise
transformed by A relative to the rest. Decryption uses the knowledge
of t's decomposition to align the two sets of noise so that the
_large_ changes (which masked the secret from an eavesdropper) cancel
out, leaving only a collection of small changes to the original secret
vector. Then the vector of input bits can be recovered by assuming
that those accumulated small pieces of noise haven't concentrated in
any particular residue enough to push it more than half way to the
other of its possible starting values.
A weird feature of it is that decryption is not a true mathematical
inverse of encryption. The assumption that the noise doesn't get large
enough to flip any bit of the secret is only probabilistically valid,
not a hard guarantee. In other words, key agreement can fail, simply
by getting particularly unlucky with the distribution of your random
noise! However, the probability of a failure is very low - less than
2^-138 even for ML-KEM-512, and gets even smaller with the larger
variants.
An awkward feature for our purposes is that the matrix A, containing a
large number of residues mod the prime q=3329, is required to be
constructed by a process of rejection sampling, i.e. generating random
12-bit values and throwing away the out-of-range ones. That would be a
real pain for our side-channel testing system, which generally handles
rejection sampling badly (since it necessarily involves data-dependent
control flow and timing variation). Fortunately, the matrix and the
random seed it was made from are both public: the matrix seed is
transmitted as part of the public key, so it's not necessary to try to
hide it. Accordingly, I was able to get the implementation to pass
testsc by means of not varying the matrix seed between runs, which is
justified by the principle of testsc that you vary the _secrets_ to
ensure timing is independent of them - and the matrix seed isn't a
secret, so you're allowed to keep it the same.
The three hybrid algorithms, defined by the current Internet-Draft
draft-kampanakis-curdle-ssh-pq-ke, include one hybrid of ML-KEM-768
with Curve25519 in exactly the same way we were already hybridising
NTRU Prime with Curve25519, and two more hybrids of ML-KEM with ECDH
over a NIST curve. The former hybrid interoperates with the
implementation in OpenSSH 9.9; all three interoperate with the fork
'openssh-oqs' at github.com/open-quantum-safe/openssh, and also with
the Python library AsyncSSH.
It's actually the limiting factor on how small the whole PuTTY
configuration dialog box can be: when KEX_MAX increased from 10 to 11
with the introduction of NTRU, the config box got taller. Now it's
back at 10.
This adds a ssh_hashalg defining SHAKE256 with a 32-byte output, in
addition to the 114-byte output we already have.
Also, it defines a new API for using SHAKE128 and SHAKE256 in the more
general form of an extendable output function, which is to say that
you still have to put in all the input before reading any output, but
once you start reading output you can just keep going until you have
enough.
Both of these will be needed in an upcoming commit implementing ML-KEM.
Now ntru.c contains just the NTRU business, and kex-hybrid.c contains
the system for running a post-quantum and a classical KEX and hashing
together the results. In between them is a new small vtable API for
the key encapsulation mechanisms that the post-quantum standardisation
effort seems to be settling on.
aligned_alloc() is used by testsc for all its memory allocation, to
avoid false-positive timing variations that depend on memory alignment
rather than actual secret data. But I'd forgotten that aligned_alloc
requires the allocation size to be a multiple of the requested
alignment.
This showed up when I ran testsc in dry-run mode, and my normal build
happened to be using ASan, which complains at the invalid allocation
size. But it was theoretically a problem in all builds of
testsc. (Though, as far as I'm aware, not practically; and it _only_
affected testsc.)
By putting the wrong error-type enum value in a ScanKexinitsResult, I
accidentally caused nonsense messages of the form
Selected key exchange algorithm "foo,bar,baz" does not correspond to any supported algorithm
where "foo,bar,baz" is the full comma-separated list sent by the
server, so it's not even _an_ algorithm as the message suggests.
Now the message is the one it should have been all along:
Couldn't agree a key exchange algorithm (available: foo,bar,baz)
I only observed this in the GTK1 build, but I don't know for sure it
can't happen in other situations, so there's no reason not to be
careful.
What seems to happen is that when the user clicks Cancel on the Change
Settings dialog box, we call gtk_widget_destroy on the window, which
emits the "destroy" signal on the window, our handler for which frees
the whole dlgparam. But _then_ GTK goes through and cleans up all the
sub-widgets of the dialog box, and some of those generate extra
events. In particular, destroying a list box is done by first deleting
all the list entries - and if one of those is selected, the list box's
selection changes, triggering an event which calls our callback that
tries to look up the control in the dlgparam we just freed.
My simple workaround is to defer actually freeing the dlgparam, via a
toplevel callback. Then it's still lying around empty while all those
random events are firing.
On Windows, this means they can use non-CP_ACP characters. Also it has
the side effect of cloning the filename out of the CmdlineArgList,
which makes it still valid after cmdline_arg_list_free (oops).
In commit 20f818af12 I renamed a lot of variables called 'ret',
by using clang-rename to do the heavy lifting. But clang-rename only
saw instances of the variable name that the _compiler_ saw. The ones
that never got through the preprocessor weren't renamed, and I didn't
eyeball the patch hard enough to find instances in the #else branch of
ifdefs that should also have been renamed.
Thanks to Lars Wendler for the report and the fixes.
If this occurs before cmdline_run_saved, then the latter will use its
saved pointers to arguments in the freed CmdlineArgList.
Affects uses of PuTTY without a saved session (like 'putty -ssh
foohost'), and a very small number of pterm options, in particular
-sessionlog.
This is the simplest possible fix: just remove the free completely,
so that the parsed command-line arguments leak. There's at most one
instance of them per process, so it doesn't matter.
I had allocated a string, advanced a pointer along it, and then freed
that pointer instead of the pointer to the start of the string.
I'd already applied the correct fix in tolocal() in commit
841bf321d4, but it needed applying in get_dir_list() too.
This reverts the change to is_interactive() by commit 80aed96286,
which switched it to using the new conio system. Now we're back to
doing it the same way as we used to: we check if stdin is a console.
The only use of is_interactive() on Windows is deciding whether to
present the console antispoof prompt. (On Unix it has an additional
use in cmdgen, for deciding whether to emit progress reports, but on
Windows that doesn't come up).
On Unix this is based on stdin being a tty, which means that a
command such as "plink host do stuff </dev/null" omits the antispoof
prompt. That's deliberate: the prompt is to defend against attacks
where the user sends interactive input to the SSH session channel
believing it to be directed at userauth, but if the input _isn't_
coming from the interactive terminal where the user is answering
userauth prompts, then they can't do that even if they are fooled.
On Windows, I think the same argument applies, now that we're reading
userauth prompts from the console in the same way as Unix. So
is_interactive() now does the analogous thing on Windows.
Conveniently, this _also_ means is_interactive() is back to exactly
how it was before the conio rewrite, which means it's one fewer thing
that can unexpectedly change and break someone's workflow. (Otherwise
I might also have wanted to change its behaviour based on
-legacy-stdio-handling, which would be extra ugly.)
In 0.81, some prompts (such as host-key prompts) went to stderr, while
others (such as username and password prompts) went to stdout.
With -legacy-stdio-prompts (or if we otherwise couldn't get console
handles), we were sending all such prompts to stdout, which might have
messed up someone's workflow if they were interacting with the
non-username/password prompts programmatically.
The fix in commit 31ab5b8e30 was incomplete. It works when you
maximise the window by pressing the maximise button in the title bar,
but not when you drag the window to the very top of the screen. In the
latter case, the resize at the instant of maximisation works right,
but then we get a WM_EXITSIZEMOVE when the drag is released,
triggering one final resize which ignored the window border again.
That was because wm_size_resize_term() was being given a flag on
purpose telling it to ignore the border: apparently at some point in
the past, ignoring the border for even a normally maximised
window (not even full-screen) was done on purpose. But for the reasons
in the previous commit, I'm changing that.
Just as with other recent changes like usernames, this allows the
remote command line to include characters outside the system code
page, encoding as UTF-8 on the wire (as the SSH protocol has wanted
all along).
When term_input_data_from_charset receives data in a specified
character set (that isn't a negative number indicating "just send
binary"), it was translating from that character set into wide-
character Unicode, but then forgetting to translate back again into
the terminal's configured character set.
Introduced by the rewrite in commit 4f756d2a4d. Affected the
answerback string sent in response to ^E, and I think potentially some
paths through term_keyinput too, although I haven't quite worked out
which ones would have been affected.
The UCS-2 translation of the access-mode string ("r", "wb" etc) was
not being freed, because the free call appeared after an unconditional
return statement.
Spotted by Coverity, although now I wonder why an ordinary compiler
warning hadn't long since pointed this one out!
They weren't being enacted if you ran psftp without a hostname and
later issued an 'open' command, because in that code path,
cmdline_run_saved() was never called. Without SAVEABLE, the options
are processed immediately instead of being deferred for later.
Also, it's pointless. The purpose of marking command-line options as
SAVEABLE is so that their processing can be correctly ordered with
respect to loading a saved session, so that they can reliably override
settings that the saved session might have defined another way. (E.g.
"plink -A sessionname": processing those options in strict
left-to-right order, the saved session's opinion about agent
forwarding would be used, regardless of the -A option.) So anything
that's not stored in Conf (and hence the saved session) at all doesn't
have any reason to be SAVEABLE.
I think I must have put the SAVEABLE in by thoughtless clone-and-hack.
Typing just 'pscp' for help doesn't work since ecfa6b2734 (and is a
particularly infuriating suggestion if that's what you did to elicit
this message).
When Windows console prompting moved away from stdin/stderr by default
(80aed96286), 0.78 was the most recent release; but we've made several
more releases off branches since then.
draft-ietf-sshm-ntruprime-ssh-00 asserts that it's identical to the
@openssh.com version we already implement:
'[sntrup761x25519-sha512@openssh.com] became the default key exchange
algorithm in OpenSSH during 2022. That is identical to the
"sntrup761x25519-sha512" mechanism described in this document.'
Literally speaking, it's not true that PuTTY only connects to the
server you told it to. It typically has to connect to a DNS server
first to find out where that server _is_. (If you've provided a
hostname, and if that hostname isn't in /etc/hosts or equivalent.)
Of course, if you're concerned about people _in your organisation's
network_ finding out where you've been connecting to, you have bigger
problems, because whether you did a DNS lookup or not they can
certainly see your IP-layer headers. But that really is outside the
scope of this document. I only mention DNS out of pedantry, because
not doing so made "does not connect to any other site" technically
inaccurate. (Perhaps even: only inaccurate if the DNS lookup happens
over TCP :-)
Re-reading the wording, I think I was a bit cavalier about "if you
don't like the host key cache recording where you've been, check host
keys yourself." It should be more like "check host keys yourself,
SERIOUSLY, WE REALLY MEAN IT, DO NOT LEAVE THIS STEP OUT."
In the original HTML-only version of the privacy document, there were
two major sections at <h2> level, "stuff stored locally" and "stuff
sent over the network", each with subsections at <h3> level describing
individual aspects. But somehow when I translated it into Halibut to
put it into the manual, they all became \H and the nesting was lost.
If we're setting ssh->s to NULL, we ought to free the thing it
previously pointed to (having extracted the error message first).
At the very least this is a memory leak.
But in fact it's worse, because not freeing it also means not
cancelling its toplevel callbacks. And if you don't do that, then a
failure to set up an SSH connection proxied over another SSH
connection will generate two error dialog boxes in succession, the
second one from the callback that should have been cancelled here.
On Windows that callback never gets called, because we exit the whole
process before getting into the main message loop which might run the
callback. But on Unix, we do go to the main message loop (we don't
have a separate one for the error box), which causes an assertion
failure in register_dialog() when the second box finds the
DIALOG_SLOT_CONNECTION_FATAL slot already occupied.
