This goes with the existing 'to_server' flag (indicating whether the
values typed by the user are going to be sent over the wire or remain
local), to indicate whether the _text of the prompts_ has come over
the wire or is originated locally.
Like to_server, nothing yet uses this. It's a hedge against the
possibility of maybe having an option for all the auth prompts to work
via GUI dialog boxes.
The banner text sent by the server was already being run through a
StripCtrlChars. Now it's run through one in line-limiting mode, and
surrounded by header and footer lines long enough that the line-length
limit wouldn't allow the server to counterfeit one. So it should now
be reliably possible to tell what is banner text sent by the server,
and what is not.
Now it can optionally check that output lines don't go beyond a
certain length (measured in terminal columns, via wcwidth, rather than
bytes or characters). In this mode, lines are prefixed with a
distinctive character (namely '|'), and if a line is too long, then it
is broken and the continuation line gets a different prefix ('>').
When StripCtrlChars is targeting a terminal, it asks the terminal to
call wcwidth on its behalf, so it can be sure to use the same idea as
the real terminal about which characters are wide (i.e. depending on
the configuration of ambiguous characters).
This mode isn't yet used anywhere.
The previous unlimited system was nicely general, but unfortunately
meant you could easily DoS a PuTTY-based terminal by sending a
printing character followed by an endless stream of identical
combining chars. (In fact, due to accidentally-quadratic linked list
management, you'd DoS it by using up all the CPU even before you got
the point of making it allocate all the RAM.)
The new limit is chosen to be 32, more or less arbitrarily. Overlong
sequences of combining characters are signalled by turning the whole
character cell into U+FFFD REPLACEMENT CHARACTER.
In both the Weierstrass and Montgomery forms, we now check that the
provided curve point isn't a silly one, like the identity or a torsion
point, which will give little or no variation in the possible outputs
of key exchange.
This checks that the public Diffie-Hellman value sent by the server is
not an obviously silly one like 1 or -1 (mod p). We already had the
validation function, and were using it in standard DH key exchange,
but the parallel code in the GSSAPI case missed it out.
If the terminal is one column wide, it's not possible to print a
double-width CJK character at all - it won't fit. Replace it with
U+FFFD to indicate that impossibility.
The previous behaviour was to notice that we're in the rightmost
column of the terminal, and invoke the LATTR_WRAPPED2 special case to
wrap to the leftmost column on the next line. But in a width-1
terminal, the rightmost column _is_ the leftmost column, so this would
leave us no better off, and we would have fallen through into the next
case while in exactly the situation we'd tried to rule out.
When we're displaying double-width text as a result of the VT100 ESC#6
escape sequence or its friends, and the terminal width is an odd
number of columns, we divide by 2 the number of characters we'll even
try to display, and round _down_: if there's a rightmost odd column,
it stays blank, and doesn't show the left half of a double-width char.
In the GTK redraw function, that rounding-down can set the 'len'
variable to zero. But when we're displaying a character with Unicode
combining chars on top, that fails an assertion that len == 1, because
at the top of the function we set it to 1.
The fix is just to return early if len is reduced to zero by that
rounding: if we're not displaying any characters, then we don't have
to do anything at all.
The REP escape (ESC [ nnn b) causes the previously printed graphic
character to be repeated another nnn times. So if it's sent as the
very first thing in a terminal session, when there _is_ no previously
printed graphic character, there's nothing sensible it can do.
In fact, in that situation, it does something decidedly _not_
sensible: it takes the uninitialised value term->last_graphic_char and
sends it directly to term_display_graphic_char, with undesirable
results if it's not actually a printing character. In particular, the
value 0 is treated as a combining char (because it has zero wcwidth),
leading to a knock-on assertion failure when compressing the
scrollback lines (which uses \0 as a terminating value for sequences
of combining characters, precisely because it expects it never to show
up in an actual cc slot!).
I completely forgot to check that the server had actually sent a key
of at least MINKLEN bits, as RFC 4432 clearly says that it MUST.
Without this restriction, not only can a server trick the client into
using a shared secret with inadequate entropy, but it can send a key
so short that the client attempts to generate a secret integer of
negative length, with integer-overflowing results.
I've always thought poll was more hassle to set up, because if you
want to reuse part of your pollfds list between calls then you have to
index every fd by its position in the list as well as the fd number
itself, which gives you twice as many indices to keep track of than if
the fd is always its own key.
But the problem is that select is fundamentally limited to the range
of fds that can fit in an fd_set, which is not the range of fds that
can _exist_, so I've had a change of heart and now have to go with
poll.
For the moment, I've surrounded it with a 'pollwrapper' structure that
lets me treat it more or less like select, containing a tree234 that
maps each fd to its location in the list, and also translating between
the simple select r/w/x classification and the richer poll flags.
That's let me do the migration with minimal disruption to the call
sites.
In future perhaps I can start using poll more directly, and/or using
the richer flag system (though the latter might be fiddly because of
sometimes being constrained to use the glib event loop). But this will
do for now.
Those hashes aren't directly needed for authenticating downloaded
files (the installer itself is checksummed, which covers all the files
it will unpack from itself). But they'll surely come in useful for
other purposes sooner or later, so we should arrange to keep them
somewhere easy to find.
The executables were already ignoring it.
This is a minimal change; PUTTY.HLP can still be built, and there's
still all the context IDs lying around.
Buildscr changes are untested.
With this change, we stop expecting to find putty.chm alongside the
executable file. That was a security hazard comparable to DLL
hijacking, because of the risk that a malicious CHM file could be
dropped into the same directory as putty.exe (e.g. if someone ran
PuTTY from their browser's download dir)..
Instead, the standalone putty.exe (and other binaries needing help)
embed the proper CHM file within themselves, as a Windows resource,
and if called on to display the help then they write the file out to a
temporary location. This has the advantage that if you download and
run the standalone putty.exe then you actually _get_ help, which
previously didn't happen!
The versions of the binaries in the installer don't each contain a
copy of the help file; that would be extravagant. Instead, the
installer itself writes a registry entry pointing at the proper help
file, and the executables will look there.
Another effect of this commit is that I've withdrawn support for the
older .HLP format completely. It's now entirely outdated, and
supporting it through this security fix would have been a huge pain.
My previous dodge to make the GTK 1 headers work with modern compilers
was to manually reset to -std=gnu89, which changed the semantics of
'inline' back to what glib.h was expecting. But that doesn't work now
the PuTTY code base expects to be able to use the rest of C99, so
instead I have to manually override the specific #defines that glib.h
uses to know how 'inline' works.
Also, moved that code in configure.ac out of the fallback branch that
manually detects GTK1, so that it will fire even if autoconf is run on
a system that still has the genuine GTK1 detection code. (Amazingly,
one still exists that I have access to!)
With that fixed, there's one more problem: the nethack_mode and
app_keypad_mode flags in gtkwin.c's key_event() are only used in the
GTK >= 2 branch of the ifdefs, so they should only be declared and set
in that branch as well, on pain of a -Wunused complaint.
A user reports that under OpenWatcom these are defined in <winnls.h>,
in which case it's redundant to redefine them ourselves and provokes a
compiler diagnostic.
I carefully made it return a bool to indicate that the whole PPL had
been freed, and then never actually checked that return value, so any
kind of connection-fatal event inside filter_queue (such as reporting
a DISCONNECT message) would cause a reference to freed memory on
return.
There was a race between toplevel callbacks: if we read enough data to
receive an SSH_MSG_DISCONNECT, and then returned, then whether we
reported the DISCONNECT message or the followup EOF would depend on
whether the BPP or the master PPL got called back first. Now the BPP
politely waits its turn (i.e. waits to see if it even gets called back
at all) before reporting EOF.
Turns out that my assertion that term->cols == line->cols can
sometimes fail, because if the window is shrunk, scrlineptr()
deliberately _doesn't_ shrink the line (so that the columns on the
right can be recovered if the window is then resized larger again). So
clear_line() should _make_ the line the right width, instead of
asserting that it already is.
I've factored out clear_line() (wipe out everything on a terminal line
including its line attrs) and also line_cols() (determine how many
columns are on this particular line, taking into account
LATTR_WRAPPED2 which reduces it by one).
Also, newline() and freeline() were badly named. Now they're called
newtermline() and freetermline(), which include the full actual type
name they deal with, and also means that now neither of them is named
the same as a control character!
Now instead of taking raw arguments to configure the output
StripCtrlChars with, it takes an enumerated value giving the context
of what's being sanitised, and allows the seat to decide what the
output parameters for that context should be.
The only context currently used is SIC_BANNER (SSH login banners).
I've also added a not-yet-used one for keyboard-interactive prompts.
Now instead of making a StripCtrlChars just for that function call, it
uses an existing one, pointing it at the output strbuf via
stripctrl_retarget.
This adds flexibility (now you can use the same convenient string-
sanitising function with a StripCtrl configured in any way you like)
and also saves pointless setting-up and tearing-down of identical sccs
all the time.
The existing call sites in PSCP and PSFTP now use a static
StripCtrlChars instance that was made at program startup.
stripctrl_retarget() points the StripCtrlChars at a new BinarySink, to
avoid having to pointlessly throw it away and make a new one all the
time.
Since that probably means the same scc is going to be reused for
processing a fresh data stream, we also don't want any character-set
conversion state hanging over from the previous stream, so we also
reset the state in the process. Just in case it's needed,
stripctrl_reset() is also provided to do that operation on its own.
Now if a pathname ends with a slash already, we detect that (using the
shiny new ptrlen_endswith), and don't bother putting another one in.
No functional change, but this should improve the occasional error
message, e.g. 'pscp remote:some.filename /' will now say it can't
create /some.filename instead of //some.filename.
I'd never even heard of these before. We don't need to do anything
unusual with these when passing them to our own getaddrinfo, but
host_strduptrim considered them a violation of its expectations about
what an IPv6 literal looked like, and hence wasn't stripping square
brackets off one. So a port-forwarding command-line option such as
'-L 12345:[fe80::%eth0]:22' would cause the address string in the
direct-tcpip CHANNEL_OPEN packet to be "[fe80::%eth0]" instead of the
correct "fe80::%eth0", leading to getaddrinfo failure on the SSH
server side.
The previous commit removed its last use, so now we can garbage-
collect it, including its long-standing FIXME comment which is now
fulfilled by the new StripCtrlChars system.
Local functions in uxcons.c and wincons.c were calling the old
simplistic sanitise_term_data to print console-based prompts. Now they
use the same new system as everything else.
This removes the last use of the ASCII-centric sanitise_term_data.
Now the banner can contain printable Unicode while still having escape
characters removed, in a way that works in both console and GUI
applications.
In the case of SSH banners, ssh2userauth.c does the sanitisation
itself, so it has to use the new Seat method to get an appropriately
configured StripCtrlChars.
(This wasn't an issue for the prompts_t system, because sanitisation
of prompt text is devolved to the local implementation of the prompt
system, in case a local implementation wants to present prompts in a
way that doesn't need sanitisation in any case, e.g. a dialog box.)
If centralised code like the SSH implementation wants to sanitise
escape sequences out of a piece of server-provided text, it will need
to do it by making a locale-based StripCtrlChars if it's running in a
console context, or a Terminal-based one if it's in a GUI terminal-
window application.
All the other changes of behaviour needed between those two contexts
are handled by providing reconfigurable methods in the Seat vtable;
this one is no different. So now there's a new method in the Seat
vtable that will construct a StripCtrlChars appropriate to that kind
of seat. Terminal-window seats (gtkwin.c, window.c) implement it by
calling the new stripctrl_new_term(), and console ones use the locale-
based stripctrl_new().
SSH authentication prompts (passwords, passphrases and keyboard-
interactive) were previously sanitised to remove escape sequences by
the simplistic sanitise_term_data() in utils.c. Now they're fed
through the new mode of StripCtrlChars instead, which means they
should permit printable Unicode (if the terminal is in UTF-8 mode)
while still disallowing escape sequences. Hopefully this will be a
usability improvement to everyone whose login prompts are in a
language not representable in plain ASCII.
If you use the new stripctrl_new_term() to construct a StripCtrlChars
instead of the existing stripctrl_new(), then the resulting object
will align itself with the character-set configuration of the Terminal
object you point it at. (In fact, it'll reuse the same actual
translation code, courtesy of the last few refactoring commits.) So it
will interpret things as control characters precisely if that Terminal
would also have done so.
The previous locale-based sanitisation is appropriate if you're
sending the sanitised output to an OS terminal device managed outside
this process - the LC_CTYPE setting has the best chance of knowing how
that terminal device will interpret a byte stream. But I want to start
using the same sanitisation system for data intended for PuTTY's own
internal terminal emulator, in which case there's no reason why
LC_CTYPE should be expected to match that terminal's configuration,
and no reason to need it to either since we can check the internal
terminal configuration directly.
One small bodge: stripctrl_new_term() is actually a macro, which
passes in the function pointer term_translate() to the underlying real
constructor. That's just so that console-only tools can link in
stripctrl.c without acquiring a dependency on terminal.c (similarly to
how we pass random_read in to the mp_random functions).
Also, instead of insisting on modifying the UTF-8 decoding state
inside the Terminal structure, it now takes a separate pointer to a
small struct containing that decode state. The idea is that if a
separate module wants to decode characters the same way the real
terminal would, it can pass its own mutable state structure, but the
same main Terminal pointer.
When ssh2_connection_filter_queue is _receiving_ messages about a
channel from the other end, it carefully checks if the channel
referred to is half-open. But we weren't exercising the same caution
before beginning to _send_ channel data, and we should, because in
that situation important fields like c->remwinsize aren't even
initialised yet.
This can come up, for example, due to typeahead in the main session
window before the server has sent OPEN_CONFIRMATION.
If an Ssh structure is destroyed while its IdempotentCallback
ssh->ic_out_raw is prnding, then the latter will stay on callback.c's
list pointing at the freed memory.
It's defined in the C standard to return an int, not a size_t, and we
should honour that since the subsequent code checks it for <0.
A knock-on effect is that I reorganise the addends in one of the
sgrowarrays, to be extra careful about overflow when adding something
to that int.
The _nm strategy is slower, so I don't want to just change everything
over no matter what its contents. In this pass I've tried to catch
everything that holds the _really_ sensitive things like passwords,
private keys and session keys.
These versions, distinguished by the _nm suffix on their names, avoid
using realloc to grow the array, in case it moves the block and leaves
a copy of the data in the freed memory at the old address. (The suffix
'nm' stands for 'no moving'.) Instead, the array is grown by making a
new allocation, manually copying the data over, and carefully clearing
the old block before freeing it.
(An alternative would be to give this code base its own custom heap in
which the ordinary realloc takes care about this kind of thing, but I
don't really feel like going to that much effort!)
A minor precaution against leaving secrets visible in process memory:
bufchains are used all over the place and probably _something_ in one
of them would be better wiped out of RAM.
If a proxy command jabbers on standard error in a way that doesn't
involve any newline characters, we now won't keep buffering data for
ever.
(Not that I've heard of it happening, but I noticed the theoretical
possibility on the way past in a recent cleanup pass.)
