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.)
The idea of these is that they centralise the common idiom along the
lines of
if (logical_array_len >= physical_array_size) {
physical_array_size = logical_array_len * 5 / 4 + 256;
array = sresize(array, physical_array_size, ElementType);
}
which happens at a zillion call sites throughout this code base, with
different random choices of the geometric factor and additive
constant, sometimes forgetting them completely, and generally doing a
lot of repeated work.
The new macro sgrowarray(array,size,n) has the semantics: here are the
array pointer and its physical size for you to modify, now please
ensure that the nth element exists, so I can write into it. And
sgrowarrayn(array,size,n,m) is the same except that it ensures that
the array has size at least n+m (so sgrowarray is just the special
case where m=1).
Now that this is a single centralised implementation that will be used
everywhere, I've also gone to more effort in the implementation, with
careful overflow checks that would have been painful to put at all the
previous call sites.
This commit also switches over every use of sresize(), apart from a
few where I really didn't think it would gain anything. A consequence
of that is that a lot of array-size variables have to have their types
changed to size_t, because the macros require that (they address-take
the size to pass to the underlying function).
I haven't tried compiling with /DMINEFIELD in a while, and when I just
did, I found that the declarations in winstuff.h weren't actually
being included by memory.c where they're needed.
I've just noticed that the MSDN docs for WinSock gethostname()
guarantee that a size-256 buffer is large enough. That seems a lot
simpler than the previous faff.
pageant.c and sshshare.c each had an extra copy of crBegin and
crFinishV, dating from when the main versions were kept in ssh.c where
they couldn't be conveniently #included by other modules. Now they're
in sshcr.h, where they can be, so there's no need to have extra copies
of them anywhere.
(But I've left the crGetChar macro in each of those files, because
those really are specific to the particular context, referring to an
extra variable that clients of the more general sshcr.h macros won't
all have.)
The live versions of the dmemdump macros had a trailing semicolon in
the expansion, which would cause them to break if used in the wrong
syntactic context (e.g. between if and else with the natural semicolon
at the call site). The conditioned-out versions of those and of
debug() itself expanded to the empty string in place of the more usual
((void)0). And SECOND_PASS_ONLY in gtkmain.c's command-line handling
should have had the standard do ... while(0) wrapper to make it
reliably a single statement.
Mostly noticed in passing while using Address / Leak Sanitiser to
check over the previous commit. One highlight here is freeing of the
previous iqmp value in rsa_verify, which was actually a potentially
sensitive leak, introduced in the mp_int rewrite (commit 25b034ee3).
I've fixed a handful of these where I found them in passing, but when
I went systematically looking, there were a lot more that I hadn't
found!
A particular highlight of this collection is the code that formats
Windows clipboard data in RTF, which was absolutely crying out for
strbuf_catf, and now it's got it.
If the user clicks 'ok' to a prompt such as 'should we carry on even
though the server only supports diffie-hellman-stage-whisper-sha0',
then we've done our duty to warn them about weak crypto, and shouldn't
nag them with the same confirmation prompt again and again in
subsequent rekeys. So now we keep a tree234 of all the algorithms the
user has consented to, so as to ask about each one at most once.
