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.
During startup, calculate the top/left position of the window so that
the whole window is visible. Without this change, sometimes the window
is clipped vertically and you can't see the bottom.
Thanks to Bruno Kraychete da Costa for the original version of this
patch; I've tweaked it slightly, but the basic strategy of adjusting
the output of GetWindowRect against the monitor's working area is all
his.
Debian #1084583 points out that Python 3.13 is going to get rid of the
'pipes' module completely. shlex.quote has been available as a
replacement for ages.
(Not that Debian actually cares, since they don't re-run our wobbly
edifice of MSI build bodges! But thanks to some bug reporter for
pointing it out anyway.)
Jacob reminds me that that's another piece of saved data, less obvious
than the ones I'd already documented.
I'd actually forgotten myself _exactly_ which actions cause data to be
added to the jump list. But it's easy to check. The only functions
exported from jump-list.c are add_session_to_jumplist(),
remove_session_from_jumplist() and clear_jumplist(); the only one of
those that _records_ information (rather than removing it) is
add_session_to_jumplist(); and that is called in exactly one place,
namely at the end of load_session() in settings.c.
On most keyboard modes the escape-sequence for Alt-Fn is the same as
for Fn alone but with an additional ESC prepended.
However, this additional ESC should not be sent for keyboard modes
that sends a different escape-sequence when Alt is pressed, like e.g.
Xterm216+, as this is not expected by the server-side application.
Signed-off-by: Anders Larsen <al@alarsen.net>
During the 0.81 release process, I found out that the Windows Store
now requires applications to provide a privacy policy, so I had to
write one in order to get 0.81 into the Store.
This initially seemed like makework (especially having to do it in a
hurry as a prerequisite to get a really important security fix
distributed!). But after I started writing it, I found there was
actually quite a lot to say. It's easy to think "PuTTY doesn't phone
home to the developers, that's all, we're done". But of course it
_does_ store information on your machine (host key cache, saved
sessions, etc). And it does send information to servers on the
network (only the ones you ask it to, but even so). And it's not 100%
obvious in every case what is and isn't stored, and what a privacy-
conscious individual might be revealing about themself by doing this
or that thing.
So I think the web page I hastily put up at the time of the 0.81
release deserves to be promoted into part of the documentation. Here's
a (very lightly) copy-edited version in the form of a docs appendix.
(Once this is committed and built, I expect I'll turn the privacy web
page into a mirror of this docs appendix, in the same way as the
website FAQ and feedback pages.)
The immediate usefulness of this is in pterm.exe: when the user uses
-e to specify a command to run in the pterm, we retrieve the command
in Unicode, store it in CONF_remote_cmd as UTF-8, and then in conpty.c
we can extract it in the same form and convert it back to Unicode to
pass losslessly to CreateProcessW. So now non-ACP Unicode works in
that part of the pterm command line.
While testing the new command-line handling, I tried actually using
that option for the first time in a long time, and saw
These are the fingerprints of the PuTTY PGP Master Keys. They
can
be used to establish a trust path from this executable to another
one. See the manual for more information.
because Windows MessageBox() had done its own wrapping on the text, at
a slightly narrower width than the one implied by the hard newlines in
the input string. Removed the mid-paragraph newlines, so that
Windows's wrapping will be the only wrapping.
That's not a failure outcome. The user asked for some information; we
printed it; nothing went wrong. Mission successful, so exit(0)!
I noticed this because it was sitting right next to some of the
usage() calls modified in the previous commit. Those also had the
misfeature of exiting with failure after successfully printing the
help, possibly due to confusion arising from the way that usage() was
_sometimes_ printed on error as well. But pgp_fingerprints() has no
such excuse. That one's just silly.
In the course of debugging the command-line argument refactoring in
previous commits, I found I wasn't quite sure whether PSCP thought I'd
given it too many arguments, or too few, because it didn't print an
error message saying which: it just printed its giant usage message.
Over the last few years I've come to the belief that this is Just
Wrong anyway. Printing the whole of a giant help message should only
be done when the user asked for it: otherwise, print a short and
to-the-point error, and maybe _suggest_ how to get help, but scrolling
everything else off the user's screen is not a good response to a
typo. I wrote this thought up more fully last year:
https://www.chiark.greenend.org.uk/~sgtatham/quasiblog/stop-helping/
So, time to practise what I preach! The PuTTY tools now follow the
'Stop helping!' principle. You can get full help by saying --help.
Also, when we do print the help, we now exit(0) rather than exit(1),
because there's no reason to report failure: we successfully did what
the user asked us for.
Converting a CmdlineArg straight to a Filename allows us to make the
filename out of the wide-character version of the string on Windows.
So now filenames specified on the command line should generally be
able to handle pathnames containing Unicode characters not in the
system code page.
This change also involves making some char pointers _into_ Filename
structs where they weren't previously: for example, the
'openssh_config_file' variable in Windows Pageant's WinMain().
This is the pathfinding change that proves it's possible for _one_
Conf setting to become Unicode-capable.
That seems like quite a small reward for all the refactoring in the
previous patches this week! But changing over one configuration
setting is enough to get started with: once all the bugs are out of
this one, we can try switching over some more.
Changing the type to CONF_TYPE_STR_AMBI is enough by itself to make
the configuration dialog box write it into Conf as UTF-8, because
conf_editbox_handler automatically checks whether that possibility is
available. However, setting the same Conf entry from the command line
isn't automatic: I had to add code in the handler for the -l
command-line option in cmdline.c.
This commit also doesn't yet handle the _other_ way to specify a
username on the command line: including it as part of the hostname
argument via "putty user@host" or similar. That's more difficult,
because it also requires deciding what to do about UTF-8 in the actual
hostname.
(That looks as if it ought to be possible: Windows should be able to
handle looking up Unicode hostnames if you use GetAddrInfoW() in place
of getaddrinfo(). But plumbing it through everything in between
cmdline.c and windows/network.c is a bigger job than I'm prepared to
do in this proof-of-concept commit.)
This begins the process of enabling our Windows applications to handle
Unicode characters on their command lines which don't fit in the
system code page.
Instead of passing plain strings to cmdline_process_param, we now pass
a partially opaque and platform-specific thing called a CmdlineArg.
This has a method that extracts the argument word as a default-encoded
string, and another one that tries to extract it as UTF-8 (though it
may fail if the UTF-8 isn't available).
On Windows, the command line is now constructed by calling
split_into_argv_w on the Unicode command line returned by
GetCommandLineW(), and the UTF-8 method returns text converted
directly from that wide-character form, not going via the system code
page. So it _can_ include UTF-8 characters that wouldn't have
round-tripped via CP_ACP.
This commit introduces the abstraction and switches over the
cross-platform and Windows argv-handling code to use it, with minimal
functional change. Nothing yet tries to call cmdline_arg_get_utf8().
I say 'cross-platform and Windows' because on the Unix side there's
still a lot of use of plain old argv which I haven't converted. That
would be a much larger project, and isn't currently needed: the
_current_ aim of this abstraction is to get the right things to happen
relating to Unicode on Windows, so for code that doesn't run on
Windows anyway, it's not adding value. (Also there's a tension with
GTK, which wants to talk to standard argv and extract arguments _it_
knows about, so at the very least we'd have to let it munge argv
before importing it into this new system.)
