Apart from being pointless, it also triggers a bug in OpenSSH pre-8.1
that causes it to send a repeat EXT_INFO after the rekey concludes,
which trips our quite draconian check for whether EXT_INFO has been
sent at the right time.
The OpenSSH bug: https://bugzilla.mindrot.org/show_bug.cgi?id=2929
Again in the GDK Broadway backend, where we never get a non-Unicode
translation of any keystroke: when we come to the code that handles
Ctrl+letter (and other symbols), we were basing the Ctrl transform on
output[1], that is, the non-Unicode translation we had so far. But we
didn't have one.
Now we check the use_ucsoutput flag to decide which of output and
ucsoutput to start from, and as a result, Ctrl+keys works again on
Broadway.
When you run using the GDK Broadway backend, this turns out to happen,
and it's new in my experience - I was cheerfully iterating over
event->string and calling strlen on it without ever checking it for
NULL.
I must have not recompiled with debug printouts enabled since updating
the internal printf functions to have the gcc printf attribute, or
these warnings would surely have come up before.
If it's worth having command-line options to _specify_ a log file,
it's also worth having options to avoid having to answer an
interactive prompt _about_ that log file every time.
(Particularly useful when debugging, in which I often want to run a
zillion instances of the same quite temporary command line that
involves writing a log file.)
This adds the framework to be able to send it in both client _and_
server (in the post-NEWKEYS slot); it's just that currently only the
server has anything it wants to put in it.
Uppity now announces its public key type list, which is enough by
itself to allow it to accept RFC 8332 rsa-sha2-* signatures during
userauth. (Because the key verification code receives an ssh-rsa host
key and validates it against the SHA2-based key algorithm structure
derived from the id string that was sent separately.)
The information was already centralised in find_pubkey_alg, but that
had a query-based API that couldn't enumerate the key types. Now I
expose an underlying array so that it's possible to iterate over them.
Also, I'd forgotten to add the two new rsa-sha2-* algorithms to
find_pubkey_alg. That's also done as part of this commit.
As with the userauth keys, there's a localised bodge when sending
algorithm names, where I just write a couple of extra entries into the
list when I notice that a key is RSA-typed. Then I arrange that the
selection of those entries sets the new variable s->hkflags to the
right value to pass to ssh_key_sign.
We parse enough of EXT_INFO to spot when the server advertises support
for them, and if it does, we upgrade the key algorithm name from
"ssh-rsa" to one of the other two, and set appropriate signing flags.
This doesn't actually end up using the ssh_rsa_sha256 / ssh_rsa_sha512
vtables I set up two commits ago, because it's easier to just vary the
flags word we pass to ssh_key_sign.
The upgrade is done by ad-hoc special-case code in ssh2userauth.c. I
could have done it by introducing a new ssh_keyalg vtable method for
'please upgrade to your favourite version of yourself according to
some set of flags from the BPP', but it just didn't seem like a good
idea at this stage, because it presupposes that quirks in the
algorithm selection are going to follow a consistent pattern, and I
think it's much more likely that the next weird thing in this area
will be something totally different. So I've left it as a localised
bodge for now, and we can always refactor it into something nicer once
we have more information and know what the nicer thing _is_.
We now add the appropriate advertisement to our KEXINIT that indicates
a willingness to receive EXT_INFO. Code in the BPP enforces that it
must appear in one of the permitted locations in the protocol (in
particular, this ensures a pre-key-exchange MITM can't get away with
inserting it into the initial cleartext segment of the protocol). And
when we receive it, we look through it for extension names we know
about.
No functional change (except for the advertisement in KEXINIT): we
don't yet actually do anything in response to any extension reported
in EXT_INFO.
This is the cleanest part of the RFC 8332 support: I simply add two
more RSA-based SSH-2 key algorithm vtables, both almost identical to
the existing one, with different ssh_id strings and signature flags.
Adding those to the HOSTKEY_ALGORITHMS list macro is enough to ensure
that we advertise support for the new identifiers in our client
KEXINIT, select the appropriate algorithm if the server announces one
or both of them too, and use the right version of the signature
validation.
Its boolean parameters were typed 'int', a hangover from before the
int-to-bool migration last year. It's not used in the current state of
the code base, so nobody noticed until now.
Uppity's built-in SFTP server makes up its file handle identifiers
using random_read(). But when that server is reused in psusan, which
doesn't have the random number generator enabled, you get an assertion
failure.
Introduced by commit 5891142aee, in which I invented
strbuf_shrink_to() and went round replacing lots of assignments of the
form 'sb->len = smaller_value' with calls to it. The bug is also in
0.74, because 34a0460f05 is a cherry-pick of the commit that
introduced it.
The difference between that assignment and strbuf_shrink_to is partly
that the latter checks by assertion that the new length really is
_smaller_ - it doesn't let you accidentally grow a strbuf's length
field beyond the limit of its buffer (or indeed at all). But also,
strbuf_shrink_to re-establishes the strbuf invariant that the text
logically in the buffer is always followed by a zero byte, so that
it's a valid C string.
Unfortunately, in one of the places I made this change, I was storing
binary data in the strbuf (so the terminating NUL is unimportant), and
immediately after decreasing the strbuf's length, I was doing a memcmp
one of whose arguments was the data I'd just chopped off the end of
the strbuf. So it _mattered_ that no random NUL had been splurged over
it.
Specifically, this happened in the run-length encoder used to compress
scrollback data, and had the effect that two components of the
compressed scrollback could be spuriously considered equal, if one of
them started with a legitimate zero byte and the other had a zero byte
written over it by this bug. Thanks to Michael Weller for a nice test
case that demonstrated a compressed scrollback line being decompressed
again as the wrong thing:
"NORMAL TEXT, \033[42mGREEN BACKGROUND\033[0m, NORMAL TEXT AGAIN"
If the above line is printed to the terminal (after being decoded as
if it was a C string literal), then only the words "GREEN BACKGROUND"
get a green background. But after that line is scrolled off the top of
the window, if you find it in the scrollback, then the rest of the
line to the right has also become green-backgrounded due to this bug.
This is a piece of conditioned-out code that I haven't used since I
originally invented the compressed scrollback format, which
decompressed every scrollback line immediately after compressing it to
check that the round-trip conversion worked. Now I have occasion to
actually use it, I find that (of course) changes around it have made
it not quite work any more: the thing the diagnostic code is passing
to decompressline hasn't had its length field filled in yet, because
that gets done 20 lines later.
Now you can compile with -DTERM_CC_DIAGS again, and it doesn't crash
_unless_ it detects the actual bug it was intended to spot.
We use this for detecting the Arm crypto extension and using it to
enable accelerated AES and/or SHA-{1,2}. Previously, I had code that
called glibc's getauxval(3) function, conditioned on #ifdef __linux__.
Now, instead, I do an autoconf test to query the presence of getauxval
itself (so that any other system with the same API can still work),
and alongside it, also check for the analogous FreeBSD libc function
elf_aux_info(3). As a result, building on Arm FreeBSD now gets the
accelerated-crypto autodetection.
I carefully set a 'finished' flag in the main source file on receipt
of the server_instance_terminated() callback, and then I plain forgot
to hook it up to the uxcliloop callback that says whether the program
should carry on running each time round the main loop. Now we actually
check the finished flag, and terminate the program if it's set.
We keep an internal 128-bit counter that's used as part of the hash
preimages. There's no real need to import all the mp_int machinery in
order to implement that: we can do it by hand using a small fixed-size
array and a trivial use of BignumADC. This is another inter-module
dependency that's easy to remove and useful to spinoff programs.
This changes the hash preimage calculation in the PRNG, because we're
now formatting our 128-bit integer in the fixed-length representation
of 16 little-endian bytes instead of as an SSH-2 mpint. This is
harmless (perhaps even mildly beneficial, due to the length now not
depending on how long the PRNG has been running), but means I have to
update the PRNG tests as well.
For use in spinoff programs: this is an alternative to proxy.c, which
provides the same API (to avoid link failures in modules like
x11fwd.c) but implements it in the trivial way, supporting no proxying
at all and just wrapping the underlying sk_new() and friends.
Rather like some of the tricks I did in mpint.h, this replaces the
unparametrised function random_setup_special() with one called
random_setup_custom() taking a hash-algorithm parameter.
The old syntax random_setup_special() still exists, and is a macro
wrapper on random_setup_custom() that passes ssh_sha512 as an
argument. This means I can keep the choice of hash function consistent
between the key generation front ends.
This adds potential flexibility: now, anyone wanting a different kind
of special RNG can make it out of whatever primitive they like. But a
more immediate point is to remove an inter-module dependency:
sshrand.c now doesn't need to be linked against the SHA-512 code.
This is mostly easy: it's just like drawing an underline, except that
you put it at a different height in the character cell. The only
question is _where_ in the character cell.
Pango, and Windows GetOutlineTextMetrics, will tell you exactly where
the font wants to have it. Following xterm, I fall back to 3/8 of the
font's ascent (above the baseline) if either of those is unavailable.
This is a small wrapper on 'sshfs' which allows it to use Plink as its
transport. Mostly useful for when I've already got a PuTTY session
open to a given host with connection sharing enabled, and want to
tunnel over that rather than painstakingly re-establishing a separate
connection.
Two minor memory-leak fixes on 0.74 seem not to be needed on master:
the fix in an early exit path of pageant_add_keyfile is done already
on master in a different way, and the missing sfree(fdlist) in
uxsftp.c is in code that's been completely rewritten in the uxcliloop
refactoring.
Other minor conflicts: the rework in commit b52641644905 of
ssh1login.c collided with the change from FLAG_VERBOSE to
seat_verbose(), and master and 0.74 each added an unrelated extra
field to the end of struct SshServerConfig.
This is a no-op merge, via 'git merge -s ours', which records that all
the commits up to this point on the 0.74 branch are bug fixes
cherry-picked from master, and don't need merging back to master.
From this point onwards, the 0.74 branch will contain fresh work that
_will_ need merging back to master. This preliminary non-merge allows
me to avoid needless conflicts during that process.
We received a report that if you enable Windows 10's high-contrast
mode, the text in PuTTY's installer UI becomes invisible, because it's
displayed in the system default foreground colour against a background
of the white right-hand side of our 'msidialog.bmp' image. That's fine
when the system default fg is black, but high-contrast mode flips it
to white, and now you have white on white text, oops.
Some research in the WiX bug tracker suggests that in Windows 10 you
don't actually have to use BMP files for your installer images any
more: you can use PNG, and PNGs can be transparent. However, someone
else reported that that only works in up-to-date versions of Windows.
And in fact there's no need to go that far. A more elegant answer is
to simply not cover the whole dialog box with our background image in
the first place. I've reduced the size of the background image so that
it _only_ contains the pretty picture on the left-hand side, and omits
the big white rectangle that used to sit under the text. So now the
RHS of the dialog is not covered by any image at all, which has the
same effect as it being covered with a transparent image, except that
it doesn't require transparency support from msiexec. Either way, the
background for the text ends up being the system's default dialog-box
background, in the absence of any images or controls placed on top of
it - so when the high-contrast mode is enabled, it flips to black at
the same time as the text flips to white, and everything works as it
should.
The slight snag is that the pre-cooked WiX UI dialog specifications
let you override the background image itself, but not the Width and
Height fields in the control specifications that refer to them. So if
you just try to drop in a narrow image in the most obvious way, it
gets stretched across the whole window.
But that's not a show-stopper, because we're not 100% dependent on
getting WiX to produce exactly the right output. We already have the
technology to postprocess the MSI _after_ it comes out of WiX: we're
using it to fiddle the target-platform field for the Windows on Arm
installers. So all I had to do was to turn msiplatform.py into a more
general msifixup.py, add a second option to change the width of the
dialog background image, and run it on the x86 installers as well as
the Arm ones.
Coverity points out that this function is mostly written as if it's
intended to allow for term->screen and/or term->alt_screen to be NULL,
but makes an unguarded call to find_last_nonempty_line on one of them.
I don't immediately remember _why_ I needed to deal with those
pointers being null, but it was probably a safety precaution against
swap_screen being called during setup or during reconfiguration, in
which case it seems sensible to keep it even if it's not needed in the
_current_ state of the code. So, added the missing check.
In commit 4ecc3f3c09 I did a knee-jerk fix of a macro of the form
#define SECOND_PASS_ONLY { body; }
on the grounds that it was syntax-unsafe, so I wrapped it in the
standard do while(0):
#define SECOND_PASS_ONLY do { body; } while (0)
But in this case, that was a bogus transformation, because the body
executed 'continue' with the intention of affecting the containing
loop (outside the macro). Moreover, ten lines above the macro
definition was a comment specifically explaining why it _couldn't_ be
wrapped in do while (0) !
Since then I've come up with an alternative break-and-continue-proof
wrapper for macros that are supposed to expand to something that's
syntactically a C statement. So I've used that instead, and while I'm
at it, fixed the neighbouring EXPECTS_ARG as well.
Spotted by Coverity, and well spotted indeed! How embarrassing.
udata[uindex] is a wchar_t, so if we pass it to sprintf("%d") we
should cast it to int (because who knows what primitive integer type
that might have corresponded to otherwise). I had done this in the
first of the two sprintfs that use it, but missed the second one a few
lines further on. Spotted by Coverity.
This mitigates CVE-2020-14002: if you're in the habit of clicking OK
to unknown host keys (the TOFU policy - trust on first use), then an
active attacker looking to exploit that policy to substitute their own
host key in your first connection to a server can use the host key
algorithm order in your KEXINIT to (not wholly reliably) detect
whether you have a key already stored for this host, and if so, abort
their attack to avoid giving themself away.
However, for users who _don't_ use the TOFU policy and instead check
new host keys out of band, the dynamic policy is more useful. So it's
provided as a configurable option.
Now, in both SSH-1 and SSH-2, we go through the whole response from
the SSH agent, parse out the public blob and comment of every key, and
stash them in a data structure to iterate through later.
Previously, we were iterating through the agent response _in situ_,
while it was still stored in the s->agent_response memory buffer in
the form the agent sent it, and had the ongoing s->asrc BinarySource
pointing at it. This led to a remotely triggerable stale-pointer bug:
as soon as we send a _second_ agent request trying to authenticate
with one of the keys, it causes s->agent_response to be freed. In
normal usage this doesn't happen, because if a server sends PK_OK (or
an RSA1 challenge) then it's going to accept our response, so we never
go back to iterating over the rest of the agent's key list. But if a
server sends PK_OK or an RSA1 challenge and _then_ rejects
authentication after we go to the effort of responding, we'll go back
to iterating over the agent's key list and cause a crash.
So now, we extract everything we need from the key-list agent
response, and by the time we're making further agent requests, we
don't need it any more.
If a malicious SSH agent were to send an RSA signature blob _longer_
than the key modulus while BUG_SSH2_RSA_PADDING was enabled, then it
could DoS the client, because the put_padding call would keep
allocating memory in 'strbuf *substr' until address space ran out.
The previous prompts were part of transcripts pasted directly from a
particular historical cmd session, but that's no reason to keep them
lying around confusingly, especially since we keep regenerating some
of those transcripts outside that historical context. Replace them all
with nice simple C:\> which shouldn't confuse anyone with extraneous
detail.
This reverts commit 4634cd47f7 and
commit 43a63019f5, both of which
introduced checks at ldisc_send call sites to avoid triggering the
assertion that len != 0 inside ldisc_send. Now that assertion is gone,
it's OK to call ldisc_send without checking the buffer size.
(cherry picked from commit 2bbed67d9e)
A user reported another situation in which that assertion can fail: if
you paste text into the terminal that consists 100% of characters not
available in the CONF_line_codepage character set, then the
translation step generates the empty string as output, and that gets
passed to ldisc_send by term_paste without checking.
Previous bugs of this kind (see commits 4634cd47f7 and 43a63019f5)
were fixed by adding a check before calling ldisc_send. But in commit
4634cd47f7 I said that probably at some point the right fix would be
to remove the assertion in ldisc_send itself, so that passing len==0
becomes legal. (The assertion was there in the first place to catch
cases where len==0 was used with its obsolete special meaning of
signalling 'please update your status'.)
Well, I think it's finally time. The assertion is removed: it's now
legal again to call ldisc_send with an empty buffer, and its meaning
is no longer the archaic special thing, but the trivial one of sending
zero characters through the line discipline.
(cherry picked from commit cd3e917fd0)
wcrtomb returns a size_t, so it's silly to immediately assign it into
an int variable. Apparently running gcc with LTO enabled points this
out as an error.
This was benign as far as I can see: the obvious risk of integer
overflow could only happen if the OS wanted to convert a single wide
character into more than 2^31 bytes, and the test of the return value
against (size_t)-1 for an error check seems to work anyway in
practice, although I suspect that's only because of implementation-
defined behaviour in gcc at the point where the size_t is narrowed to
a signed int.
(cherry picked from commit 99f5fa34ab)
Again, there was a missing #include in that file which meant that the
definition of the function was never being checked against the
declaration visible to other source files.
(cherry picked from commit c5aa7fc31c)
I'd forgotten to #include "dialog.h" in that file, which meant nothing
was checking the prototypes of the stub implementations of the dlg_*
function family against the real versions. They almost all needed a
'void *dlg' parameter updating to 'dlgparam *dp', which is a change
dating from commit 3aae1f9d76 nearly two years ago. And a handful of
them also still had 'int' that should be now have become 'bool'.
(cherry picked from commit c373fe979f)
Mark Wooding points out that when running with the +ut flag, we close
pty_utmp_helper_pipe during pty backend setup, which causes the
previously forked helper process to terminate. If that termination
happens quickly enough, then the code later in pty_backend_create
won't have set up the SIGCHLD handler and its pipe yet, so when we get
to the main event loop, we'll fail to notice that subprocess waiting
to be reaped, and leave it lying around as a zombie.
An easy fix is to move the handler and pipe setup to before the code
that potentially closes pty_utmp_helper_pipe, so that there isn't a
race condition any more.
(cherry picked from commit 7ffa6ed41e)
If a terminal window closed with a popup (due to a network error,
for instance) while the mouse pointer was hidden by 'Hide mouse
pointer when typing in window', the mouse pointer could remain hidden
while over the terminal window, making it hard to navigate to the
popup.
(cherry picked from commit d9c4ce9fd8)
Colin reports that on betas of Ubuntu 20.04, Pango has switched to
getting its font metrics from HarfBuzz, and a side effect is
apparently that they're being returned in the full precision of
PANGO_SCALE fixed point.
Previously, Pango appears to have been returning values that were
always a whole number of pixels scaled by PANGO_SCALE. Moreover, it
looks as if it was rounding the font ascent and descent _up_ to a
whole number of pixels, rather than rounding to nearest. But our code
rounds to nearest, which means that now the same font gets allocated
fewer vertical pixels, which can be enough to cut off some ascenders
or descenders.
Pango already provides the macro PANGO_PIXELS_CEIL, so it's easy to
switch over to using it. This should arrange that any text that fits
within the font's stated ascent/descent measurements will also fit in
the character cell.
(cherry picked from commit f9a46a9581)
If gdk_event_get_scroll_deltas() return failure for a given
GdkEventScroll, it doesn't follow that that event has no usable
scrolling action in it at all. The fallback is to call
gdk_event_get_scroll_direction() instead, which is less precise but
still gives _something_ you can use. So in that situation, instead of
just returning false, we can fall through to the handling we use for
pre-GTK3 scroll events (which are always imprecise).
In particular, I've noticed recently that if you run GTK 3 PuTTY in
the virtual X display created by vnc4server, and connect to it using
xtightvncviewer, then scroll-wheel actions passed through from the VNC
client will cause scroll_event() to receive low-res GdkEventScroll
structures of exactly this kind. So scroll-wheel activity on the
terminal window wasn't causing a scroll in that environment, and with
this patch, it does.
(cherry picked from commit 0fd30113f1)
In the SSH-2 connection layer, an outstanding_channel_request
structure comes with a handler to be called back with the reply
packet, when the other end sends one. But sometimes it doesn't - if
the channel begins to close before the request has been replied to -
in which case the handler function is called with a NULL packet
pointer.
The common ssh2_channel_response function that handles most of the
client-side channel requests was not prepared to cope with that
pointer being null. Fixed by making it handle a null return the same
as CHANNEL_FAILURE.
(cherry picked from commit e4b6a7efd2)
