Skip to content

Conversation

@larryli
Copy link
Owner

@larryli larryli commented Dec 18, 2024

close #79 #81

sgtatham and others added 30 commits September 16, 2022 09:18
Of the three calls to queue_toplevel_callback in window.c, one of them
was still passing NULL as its context parameter, rather than the new
'wgs'. As a result, a segfault could occur during some session closures.
While cmake might create it, it's not something we ship any more.
(Just in case anyone's entry point is this example, and concludes they
have to reverse-engineer the protocol from the script.)
Python 3's stderr was fully-buffered when non-interactive, unlike
Python 2 and more or less everything else, until 3.9 in 2020(!):
https://bugs.python.org/issue13601

(It would be less faff to sys.stderr.reconfigure(line_buffering=True)
at the start, but that was only added in 3.7, whereas the 'flush'
argument to print() dates back to 3.3, so I chose that to minimise
the risk of version dependencies getting in the way of using this as
a working example.)
Oops. The previous merge came from a version of the release tag we
ended up not using. This one reflects what really went into the
release.
I didn't actually get these things wrong during the submission of
0.78, but I did notice that I'd forgotten to write them down.
We still don't build or ship a PDF PuTTY manual by default, but we may
as well conveniently expose Halibut's ability to do so.

(I don't guarantee the resulting PDF is particularly pretty -- some of
our overlong code lines do go off the right margin currently.)
I just happened to notice ARG1 and ARGN in the code that builds the
dispatch table in process_line(), which aren't used at all, because
they date from a previous version of the testcrypt-func.h macro
system. They were supposed to be replaced everywhere with the unified
ARG.

So why didn't the missing definition of ARG break anything? Because
ARG only ever appears in the variadic part of a FUNC_INNER call - and
for this particular trawl of testcrypt-func.h, the variadic part isn't
ever used in the macro expansion in the first place. So there's no
need to define ARG and VOID to anything at all, not even the empty
string.
A server attempt to resize the window (for instance via DECCOLM) when
"When window is resized" was set to "Forbid resizing completely" would
cause all terminal output to be suspended, due to the resize attempt
never being acknowledged.

(There are other code paths like this, which I've fixed for
completeness, but I don't think they have any effect: the terminal
filters out resize attempts to the current size before this point, and
even if a server can get such a request through the SUPDUP protocol, the
test for that is wrong and will never fire -- this needs fixing
separately.)
When a host certificate was used outside its valid date range, we were
displaying the current time where we meant to show the relevant bound of
the validity range.
Like 5f3b743, specifically reassure the user that taking the
add-to-cache action will not cause the CA that signed the key to be
trusted in any wider context, in the case where there was no previous
certified key cached. (I don't know why I missed this out before.)
sgtatham and others added 28 commits October 23, 2024 07:56
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.
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."
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 :-)
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.'
When Windows console prompting moved away from stdin/stderr by default
(80aed96), 0.78 was the most recent release; but we've made several
more releases off branches since then.
Typing just 'pscp' for help doesn't work since ecfa6b2 (and is a
particularly infuriating suggestion if that's what you did to elicit
this message).
If you specified --encrypted or one of its synonyms while not adding
keys, Pageant would start going on about the unrelated -E option for
some reason.
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.
The w32old builds couldn't read a password from the console at all,
because ReadConsoleW would always return failure.
Spotted by Coverity: we should check the value_type of the Conf
setting and return failure _before_ allocating the new conf_entry.
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!
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 4f756d2. 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.
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).
The fix in commit 31ab5b8 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.
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 backslash-continuation inside the block comment may not
technically be necessary, but I think should be harmless.)
This reverts the change to is_interactive() by commit 80aed96,
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.)
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
841bf32, but it needed applying in get_dir_list() too.
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.
@larryli larryli merged commit 2cd3b3e into master Dec 19, 2024
3 checks passed
@larryli larryli deleted the translate/0.82 branch December 20, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PuTTY 0.82 has been released.

8 participants