UBsan pointed out another memcpy from NULL (again with length 0) in
the prompts_t system. When I looked at it, I realised that firstly
prompt_ensure_result_size was an early not-so-good implementation of
sgrowarray_nm that would benefit from being replaced with a call to
the real one, and secondly, the whole system for storing prompt
results should really have been replaced with strbufs with the no-move
option, because that's doing all the same jobs better.
So, now each prompt_t holds a strbuf in place of its previous manually
managed string. prompt_ensure_result_size is gone (the console
prompt-reading functions use strbuf_append, and everything else just
adds to the strbuf in the usual marshal.c way). New functions exist to
retrieve a prompt_t's result, either by reference or copied.
These are better than my previous approach of just assigning to
sb->len, because firstly they check by assertion that the new length
is within range, and secondly they preserve the invariant that the
byte stored in the buffer just after the length runs out is \0.
Switched to using the new functions everywhere a grep could turn up
opportunities.
This bug applies to both the new stream-based agent forwarding, and
ordinary remote->local TCP port forwardings, because it was introduced
by the preliminary infrastructure in commit 09954a87c.
new_connection() and sk_new() accept a SockAddr *, and take ownership
of it. So it's a mistake to make an address, connect to it, and then
sk_addr_free() it: the free will decrement its reference count to
zero, and then the Socket made by the connection will be holding a
stale pointer. But that's exactly what I was doing in the version of
portfwdmgr_connect() that I rewrote in that refactoring. And then I
made the same error again in commit ae1148267 in the Unix stream-based
agent forwarding.
Now both fixed. Rather than remove the sk_addr_free() to make the code
look more like it used to, I've instead solved the problem by adding
an sk_addr_dup() at the point of making the connection. The idea is
that that should be more robust, in that it will still do the right
thing if portfwdmgr_connect_socket should later change so as not to
call its connect helper function at all.
The new Windows stream-based agent forwarding is unaffected by this
bug, because it calls new_named_pipe_client() with a pathname in
string format, without first wrapping it into a SockAddr.
Now they have names that are more consistent (no more userkey_this but
that_userkey); a bit shorter; and, most importantly, all the current
functions end in _f to indicate that they deal with keys stored in
disk files. I'm about to add a second set of entry points that deal
with keys via the more general BinarySource / BinarySink interface,
which will sit alongside these with a different suffix.
Historically, because of the way Windows Pageant's IPC works, PuTTY's
agent forwarding has always been message-oriented. The channel
implementation in agentf.c deals with receiving a data stream from the
remote agent client and breaking it up into messages, and then it
passes each message individually to agent_query().
On Unix, this is more work than is really needed, and I've always
meant to get round to doing the more obvious thing: making an agent
forwarding channel into simply a stream-oriented proxy, passing raw
data back and forth between the SSH channel and the local AF_UNIX
socket without having to know or care about the message boundaries in
the stream.
The portfwdmgr_connect_socket() facility introduced by the previous
commit is the missing piece of infrastructure to make that possible.
Now, the agent client module provides an API that includes a callback
you can pass to portfwdmgr_connect_socket() to open a streamed agent
connection, and the agent forwarding setup function tries to use that
where possible, only falling back to the message-based agentf.c system
if it can't be done. On Windows, the new piece of agent-client API
returns failure, so we still fall back to agentf.c there.
There are two benefits to doing it this way. One is that it's just
simpler and more robust: if PuTTY isn't trying to parse the agent
connection, then it has less work to do and fewer places to introduce
bugs. The other is that it's futureproof against changes in the agent
protocol: if any kind of extension is ever introduced that requires
keeping state within a single agent connection, or that changes the
protocol itself so that agentf's message-boundary detection stops
working, then this forwarding system will still work.
In commit 105672e32 I added the pty_backend_exit_signame() function,
which constructs the SSH wire name of the signal that terminated the
pty session process. I did it by having a sequence of inline #ifdefs
for all the translatable signal names, each one guarding the code that
expected that signal to be defined in <signal.h>.
But there was no need to write all that out longhand, because in the
preceding commit 72eca76d2, I made a central list of signal names in
sshsignals.h, so all I should have needed to do was to set up the
macros that header expects, and let _it_ do the iteration over the
locally defined subset of signal ids! So now I do that instead.
When I'm declaring a local instance of some context structure type to
pass to a function which will pass it in turn to a callback, I've
tended to use a declaration of the form
struct context actx, *ctx = &actx;
so that the outermost caller can initialise the context, and/or read
out fields of it afterwards, by the same syntax 'ctx->foo' that the
callback function will be using. So you get visual consistency between
the two functions that share this context.
It only just occurred to me that there's a much neater way to declare
a context struct of this kind, which still makes 'ctx' behave like a
pointer in the owning function, and doesn't need all that weird
verbiage or a spare variable name:
struct context ctx[1];
That's much nicer! I've switched to doing that in all existing cases I
could find, and also in a couple of cases where I hadn't previously
bothered to do the previous more cumbersome idiom.
When I reworked the 'selparams' array in commit e790adec4 to contain
pointers to 'struct selparam' rather than directly containing
structures, I missed this one case where I should have removed an &.
As a result the GTK1 signal handler that deals with clicks on the
config-pane selection treeview was getting a pointer to a pointer and
treating it as a pointer to an object. Nothing good happened.
I was testing some upcoming new GTK code against all GTK versions,
which for once was interesting enough to make nontrivial use of
g_object_ref_sink, and I found that I hadn't implemented the GTK1
fallback version right. GTK1 has no ref_sink call, but it does have
ref and sink, so the right thing seems to be to just call them in
succession.
If you go to Change Settings in Unix PuTTY or pterm, and change the
'Gap between text and window edge' setting but not the width and
height, then change_settings_menuitem() correctly sets the physical
window to a new size, but drawing_area_setup() was not recreating the
backing surface / pixmap in the same way, because it hadn't spotted
that the border size might be relevant.
Now I unconditionally work out what the exact size of the backing
surface _ought_ to be, before reaching the potential early exit path,
and never take the early exit if the backing area needs resizing for
any reason at all.
(I think this probably ought to have been part of commit 528513dde.)
I'm about to rearrange this function, and the patch that actually does
work will be easier to read if mass reindentation isn't combined with
it.
The braces I've just removed were necessary when we hadn't yet
committed to requiring (most of) C99 from all our build platforms. Now
they aren't.
Up until now, it's been a variadic _function_, whose argument list
consists of 'const char *' ASCIZ strings to concatenate, terminated by
one containing a null pointer. Now, that function is dupcat_fn(), and
it's wrapped by a C99 variadic _macro_ called dupcat(), which
automatically suffixes the null-pointer terminating argument.
This has three benefits. Firstly, it's just less effort at every call
site. Secondly, it protects against the risk of accidentally leaving
off the NULL, causing arbitrary words of stack memory to be
dereferenced as char pointers. And thirdly, it protects against the
more subtle risk of writing a bare 'NULL' as the terminating argument,
instead of casting it explicitly to a pointer. That last one is
necessary because C permits the macro NULL to expand to an integer
constant such as 0, so NULL by itself may not have pointer type, and
worse, it may not be marshalled in a variadic argument list in the
same way as a pointer. (For example, on a 64-bit machine it might only
occupy 32 bits. And yet, on another 64-bit platform, it might work
just fine, so that you don't notice the mistake!)
I was inspired to do this by happening to notice one of those bare
NULL terminators, and thinking I'd better check if there were any
more. Turned out there were quite a few. Now there are none.
When I introduced the unreachable() macro in commit 0112936ef, I
searched the source code for assert(0) and assert(false), together
with their variant form assert(0 && "explanatory text"). But I didn't
search for assert(!"explanatory text"), which is the form I used to
use before finding that assert(0 && "text") seemed to be preferred in
other code bases.
So, here's a belated replacement of all the assert(!"stuff") macros
with further instances of unreachable().
The number of people has been steadily increasing who read our source
code with an editor that thinks tab stops are 4 spaces apart, as
opposed to the traditional tty-derived 8 that the PuTTY code expects.
So I've been wondering for ages about just fixing it, and switching to
a spaces-only policy throughout the code. And I recently found out
about 'git blame -w', which should make this change not too disruptive
for the purposes of source-control archaeology; so perhaps now is the
time.
While I'm at it, I've also taken the opportunity to remove all the
trailing spaces from source lines (on the basis that git dislikes
them, and is the only thing that seems to have a strong opinion one
way or the other).
Apologies to anyone downstream of this code who has complicated patch
sets to rebase past this change. I don't intend it to be needed again.
It's silly to use the higher-level name_lookup(), because the way in
which they differ is that name_lookup() allows DNS lookups to be
deferred to the other side of a network proxy over which your
connection is travelling - and when you're trying to find out your own
host name for use as a database key, there's no _connection_ involved
at all, and no potential proxy either.
I've only just noticed that it doesn't do anything at all!
Almost every implementation of the Socket vtable provides a flush()
method which does nothing, optionally with a comment explaining why
it's OK to do nothing. The sole exception is the wrapper Proxy_Socket,
which implements the method during its setup phase by setting a
pending_flush flag, so that when its sub-socket is later created, it
can call sk_flush on that. But since the sub-socket's sk_flush will do
nothing, even that is completely pointless!
Source control history says that sk_flush was introduced by Dave
Hinton in 2001 (commit 7b0e08270), who was going to use it for some
purpose involving the SSL Telnet support he was working on at the
time. That SSL support was never finished, and its vestigial
declarations in network.h were removed in 2015 (commit 42334b65b). So
sk_flush is just another vestige of that abandoned work, which I
should have removed in the latter commit but overlooked.
At Coverity's urging, I put in an instance of the Fortuna PRNG in
place of the use of rand() to select which drawing area to light up
next on a keypress. But Coverity turns out to be unhappy that I'm
still using rand() to select the _initial_ area, before the user
enters any input at all.
It's even harder to imagine this being a genuine information leak than
the previous complaint. But I don't really see a reason _not_ to
switch it over, now it's been pointed out.
Introduced by 61f3e3e29, as part of my periodic efforts to make the
GTK front end work usefully on OS X: the 'Unable to open connection to
[host]: [error]' message box is accidentally passed through two layers
of printf format-string parsing, the second of which has no argument
list. So if you pass in a host name like '%s' on the command line,
bad things will happen when that error message is constructed.
This happened because in that commit I changed a call to
fatal_message_box() into a call to connection_fatal(), without
noticing that the latter was printf-style variadic and the former
wasn't. On the plus side, that means now I can remove the explicit
dupprintf/free around the error message.
If you select an entry in the saved sessions list box, but without
double-clicking to actually load it, and then you hit OK, the config-
box code will automatically load it. That just saves one click in a
common situation.
But in order to load that session, the config-box system first has to
ask the list-box control _which_ session is selected. On Windows, this
causes an assertion failure if the user has switched away from the
Session panel to some other panel of the config box, because when the
list box isn't on screen, its Windows control object is actually
destroyed.
I think a sensible answer is that we shouldn't be doing that implicit
load behaviour in any case if the list box isn't _visible_: silently
loading and launching a session someone selected a lot of UI actions
ago wasn't really the point. So now I make that behaviour only happen
when the list box (i.e. the Session panel) _is_ visible. That should
prevent the assertion failure on Windows, but the UI effect is cross-
platform, applying even on GTK where the control objects for invisible
panels persist and so the assertion failure didn't happen. I think
it's a reasonable UI change to make globally.
In order to implement it, I've had to invent a new query function so
that config.c can tell whether a given control is visible. In order to
do that on GTK, I had to give each control a pointer to the 'selparam'
structure describing its config-box pane, so that query function could
check it against the current one - and in order to do _that_, I had to
first arrange that those 'selparam' structures have stable addresses
from the moment they're first created, which meant adding a layer of
indirection so that the array of them in the top-level dlgparam
structure is now an array of _pointers_ rather than of actual structs.
(That way, realloc half way through config box creation can't
invalidate the important pointer values.)
Having decided that the terminal's local echo setting shouldn't be
allowed to propagate through to termios, I think the local edit
setting shouldn't either. Also, no other terminal emulator I know
seems to implement this sequence, and if you enable it, things get
very confused in general. I think it's generally better off absent; if
somebody turns out to have been using it, then we'll at least be able
to find out what it's good for.
The functions that previously lived in it now live in terminal.c
itself; they've been renamed term_keyinput and term_keyinputw, and
their function is to add data to the terminal's user input buffer from
a char or wchar_t string respectively.
They sit more comfortably in terminal.c anyway, because their whole
point is to translate into the character encoding that the terminal is
currently configured to use. Also, making them part of the terminal
code means they can also take care of calling term_seen_key_event(),
which simplifies most of the call sites in the GTK and Windows front
ends.
Generation of text _inside_ terminal.c, from responses to query escape
sequences, is therefore not done by calling those external entry
points: we send those responses directly to the ldisc, so that they
don't count as keypresses for all the user-facing purposes like bell
overload handling and scrollback reset. To make _that_ convenient,
I've arranged that most of the code that previously lived in
lpage_send and luni_send is now in separate translation functions, so
those can still be called from situations where you're not going to do
the default thing with the translated data.
(However, pasted data _does_ still count as close enough to a keypress
to call term_seen_key_event - but it clears the 'interactive' flag
when the data is passed on to the line discipline, which tweaks a
minor detail of control-char handling in line ending mode but mostly
just means pastes aren't interrupted.)
If the user closes the Change Settings dialog box using the close
button provided by the window manager (or some analogous thing that
generates the same X11 event) instead of using the Cancel button
within the dialog itself, then after_change_settings_dialog() gets
called with retval < 0, which triggers an early return path in which
we forget to call unregister_dialog(), and as a result, assertions
fail all over the place the _next_ time you try to put up a Change
Settings dialog.
Also, the early return causes ctx.newconf to be memory-leaked. So
rather than just moving the unregister_dialog() call to above the
early return, a better fix is to remove the early return completely,
and simply treat retval<0 the same as retval==0: it doesn't matter
_how_ the user closed the config box without committing the changes,
it only matters that they did.
When I start Uppity in listening mode, it's useful to have it
acknowledge that it _has_ started up in that mode, and isn't (for
example) stuck somewhere in my local wrapper script.
Coverity complained that it was wrong to use rand() in a security
context, and although in this case it's _very_ marginal, I can't
actually disagree that the choice of which light to light up to avoid
giving information about passphrase length is a security context.
So, no more rand(); instead we instantiate a shiny Fortuna PRNG
instance, seed it in more or less the usual way, and use that as an
overkill-level method of choosing which light to light up next.
(Acknowledging that this is a slightly unusual application and less
critical than most, I don't actually put the passphrase characters
themselves into the PRNG, and I don't use a random-seed file.)
It's identical in uxnoise and winnoise, being written entirely in
terms of existing cross-platform functions. Might as well centralise
it into sshrand.c.
We have to use the file name we just failed to open to format an error
message _before_ freeing it, not after. If that use-after-free managed
not to cause a crash, we'd also leak the file descriptor 'outfd'.
Both spotted by Coverity (which is probably the first thing in years
to look seriously at any of the code designed for Ben's AFL exercise).
The recent rewriting in both the GTK and Windows keyboard handlers
left the keypad 'Enter' key in a bad state, when no override is
enabled that causes it to generate an escape sequence.
On Windows, a series of fallbacks was causing it to generate \r
regardless of configuration, whereas in Telnet mode it should default
to generating the special Telnet new-line sequence, and in response to
ESC[20h (enabling term->cr_lf_return) it should generate \r\n.
On GTK, it wasn't generating anything _at all_, and also, I can't see
any evidence that the GTK keyboard handler had ever remembered to
implement the cr_lf_return mode.
Now Keypad Enter in non-escape-sequence mode should behave just like
Return, on both platforms.
With this and the ciphers, I think we've now got the full range of
SSH-1 config options (such as they are) that correspond to varying the
KEXINIT strings in SSH-2.
Despite the name, AuthPolicy in uxserver.c was also holding the state
of the current connection, including in particular how far through the
multi-step test keyboard-interactive interaction we are. But now that
Uppity can handle multiple connections in the same run, we need to
reset that state between connections. So the tree234s of acceptable
user keys now live in an AuthPolicyShared structure, and AuthPolicy
proper is a field of server_instance.
All my instincts expect the shell subprocesses to start off in ~, so
it's confusing if they start off in some random PuTTY checkout
directory. So now we default to $HOME, and if I really do want the
latter, I can use the new config option to reselect '.'.
My helper scripts for invoking Uppity have been manually unsetting
things like XAUTHORITY and SSH_AUTH_SOCK, to avoid accidentally
passing them through from my primary login session, so that I don't
get confused about whether agent forwarding is happening, or end up
with one DISPLAY going with a different XAUTHORITY.
Now I clear these within Uppity itself, so the wrapping script won't
have to.
In some contexts (namely pterm on a pure Wayland system, and Uppity),
seat_get_x_display() will return NULL. In that situation uxpty.c was
cheerfully passing it to dupprintf regardless, which in principle is
undefined behaviour and in practice was causing it to construct the
silly environment string "DISPLAY=(null)".
Now we handle that case by unsetenv("DISPLAY") instead.
I've been busily adding new options, and forgot to document them all,
which will annoy me the next time I haven't used it for a week or two
if I don't write them all up now.
As and when I make this SSH server into a test suite, I'm not going to
want to wait for a gratuitous RSA key generation in every test run. So
now you can provide one in advance.
It has to be in SSH-1 format, because that's the format for which I
happen to already have internal API routines that return an RSAKey
instead of an opaque ssh_key. But since you also have to store it
without a passphrase, that doesn't really matter anyway.
I remembered to strbuf_free(realhost) on the IPv4-only error exit path
if gethostbyname() returns failure, but not on the _default_ one if
getaddrinfo() does.
There was no way to enable it for testing purposes at all until now.
Overriding the server KEX string to mention it doesn't help when it
was prevented from getting into the list that scan_kexinit_lists will
go through afterwards to find pointers to algorithm structures.
Uppity is not secure enough to listen on a TCP port as if it was a
normal SSH server. Until now, I've been using it by means of a local
proxy command, i.e. PuTTY invokes Uppity in the same way it might
invoke 'plink -nc'. This rigorously prevents any hostile user from
connecting to my utterly insecure test server, but it's a thundering
inconvenience as soon as you want to attach a debugger to the Uppity
process itself - you have to stick a gdbserver somewhere in the middle
of your already complicated shell pipeline, and then find a way to
connect back to it from a gdb in a terminal window.
So I've added an option to make Uppity listen on a TCP port in the
normal way - but it's protected using that /proc/net/tcp trick I just
added in the previous commit.
This is a Linux-specific trick that I'm quite fond of: I've used it
before in 'agedu' and a lot of my unpublished personal scriptery.
Suppose you want to run a listening network server in such a way that
it can only accept connections from processes under your own control.
Often it's not convenient to do this by adding an authentication step
to the protocol itself (either because the password management gets
hairy or because the protocol is already well defined). The 'right'
answer is to switch from TCP to Unix-domain sockets, because then you
can use the file permissions on the path leading to the socket inode
to ensure that no other user id can connect to it - but that's often
inconvenient as well, because if any _client_ of the server is not
already prepared to speak AF_UNIX your control then you can only trick
it into connecting to an AF_UNIX socket instead of TCP by applying a
downstream patch or resorting to LD_PRELOAD shenanigans.
But on Linux, there's an alternative shenanigan available, in the form
of /proc/net/tcp (or tcp6), which lists every currently active TCP
endpoint known to the kernel, and for each one, lists an owning uid.
Listen on localhost only. Then, when a connection comes in, look up
the far end of it in that file and see if the owning uid is the right
one!
I've always vaguely wondered if there would be uses for this trick in
PuTTY. One potentially useful one might be to protect the listening
sockets created by local-to-remote port forwarding. But for the
moment, I'm only planning to use it for a less security-critical
purpose, which will appear in the next commit.
This is an obviously useful test feature, since if nothing else it
will let me exercise every individual crypto primitive, even the ones
that the client-side configuration is too coarse-grained to describe
in detail (such as the difference between CBC and CTR mode versions of
the same cipher).
This mimics a bug in some old SSH servers for which PuTTY contains
compensation code (parsing an incoming "exit-signal" two ways and
seeing which one worked). I completely rewrote that code in commit
7535f645a, as part of the BinarySource rework. Now I can finally test
it sensibly.
Replaces a couple of existing strcmp, and does just slightly better
because as well as matching "--verbose" (say) it also matches
"--verbose=foo" and gives a comprehensible error message about it.
