This reads data from standard input, turns it into an SSH-2 sign
request, and writes the resulting signature blob to standard output.
I don't really anticipate many uses for this other than testing. But
it _is_ convenient for testing changes to Pageant itself: it lets me
ask for a signature without first having to construct a pointless SSH
session that will accept the relevant key.
This makes all the new deferred-decryption business actually _useful_
for the first time: you can now load an encrypted key file and then
get a prompt to decrypt it on first use, without Pageant being in the
low-usability debug mode.
Currently, the option to present runtime prompts is enabled if Pageant
is running with an X display detected, regardless of lifetime mode.
I'm not really sure why that's necessary: by my understanding of the C
standard, it shouldn't be. But my observation is that when compiling
with {Address,Leak} Sanitiser enabled, pageant --askpass can somehow
manage to exit without having actually written the passphrase to its
standard output.
This applies to both server modes ('pageant -E key.ppk [lifetime]')
and client mode ('pageant -a -E key.ppk').
I'm not completely confident that the CLI syntax is actually right
yet, but for the moment, it's enough that it _exists_. Now I don't
have to test the encrypted-key loading via manually mocked-up agent
requests.
On Windows, due to a copy-paste goof, the message that should have
read "Configuring n stop bits" instead ended with "data bits".
While I'm here, I've arranged that the "1 stop bit" case of that
message is in the singular. And then I've done the same thing again on
Unix, because I noticed that message was unconditionally plural too.
Now I've got an enum for PlugLogType, it's easier to add things to it.
We were giving a blow-by-blow account of each connection attempt, and
when it failed, saying what went wrong before we moved on to the next
candidate address, but when one finally succeeded, we never logged
_that_. Now we do.
Unix Plink, Unix Pageant in server mode, Uppity, and the post-
connection form of PSFTP's command-line reading code all had very
similar loops in them, which run a pollwrapper and mediate between
that, timers, and toplevel callbacks. It's long past time the common
code between all of those became a reusable shared routine.
So, this commit introduces uxcliloop.c, and turns all the previous
copies of basically the same loop into a call to cli_main_loop with
various callback functions to configure the parts that differ.
The sets of poll(2) events that we check in order to return SELECT_R
and SELECT_W overlap: to be precise, they have POLLERR in common. So
if an fd signals POLLERR, then pollwrap_get_fd_rwx will respond by
saying that it has both SELECT_R and SELECT_W available on it - even
if the caller had only asked for one of those.
In other words, you can get a spurious SELECT_W notification on an fd
that you never asked for SELECT_W on in the first place. This
definitely isn't what I'd meant that API to do.
In particular, if a socket in the middle of an asynchronous connect()
signals POLLERR, then Unix Plink will call select_result for it with
SELECT_R and then SELECT_W respectively. The former will notice that
it's got an error condition and call plug_closing - and _then_ the
latter will decide that it's writable and set s->connected! The plan
was to only select it for write until it was connected, but this bug
in pollwrap was defeating that plan.
Now pollwrap_get_fd_rwx should only ever return a set of rwx flags
that's a subset of the one that the client asked for via
pollwrap_add_fd_rwx.
I've often found it useful that you can make symlinks to Unix-domain
sockets, and then connect() on the symlink path will redirect to the
original socket.
This commit adds an option to Unix Pageant which will make it symlink
its socket path to a link location of your choice. My initial use case
is when running Pageant in debug mode during development: if you run a
new copy of it every few minutes after making a code change, then it's
annoying to have it change its socket path every time so you have to
keep pasting its setup command into your test shell. Not any more! Now
you can run 'pageant --debug --symlink fixed-location', and then your
test shell can point its SSH_AUTH_SOCK at the fixed location all the
time.
There are very likely other use cases too, but that's the one that
motivated me to add the option.
This is the easiest place to implement _something_ that will work as a
runtime passphrase prompt, which means I get to use it to test the
code I'm about to add to the Pageant core to make use of those
prompts. Once that's working, we can think about adding prompts for
the 'proper' usage modes.
The debug-mode passphrase prompts are implemented by simply reading
from standard input, having emitted a log message mentioning that a
prompt is impending. We put standard input into non-echoing mode, but
otherwise don't print any visible prompt (because standard output will
in general receive further log messages, which would break it anyway).
This is only just good enough for initial testing. In particular, it
won't cope if two prompts are in flight at the same time. But good
enough for initial testing is better than nothing!
This begins to head towards the goal of storing a key file encrypted
in Pageant, and decrypting it on demand via a GUI prompt the first
time a client requests a signature from it. That won't be a facility
available in all situations, so we have to be able to return failure
from the prompt.
More precisely, there are two versions of this API, one in
PageantClient and one in PageantListenerClient: the stream
implementation of PageantClient implements the former API and hands it
off to the latter. Windows Pageant has to directly implement both (but
they will end up funnelling to the same function within winpgnt.c).
NFC: for the moment, the new API functions are never called, and every
implementation of them returns failure.
In the previous trawl of this, I didn't bother with the statics in
main-program modules, on the grounds that my main aim was to avoid
'library' objects (shared between multiple programs) from polluting
the global namespace. But I think it's worth being more strict after
all, so this commit adds 'static' to a lot more file-scope variables
that aren't needed outside their own module.
Now it's no longer used, we can get rid of it, and better still, get
rid of every #define PUTTY_DO_GLOBALS in the many source files that
previously had them.
It's now a static in the main source file of each application that
uses it, and isn't accessible from any other source file unless the
main one passes it by reference.
In fact, there were almost no instances of the latter: only the
config-box functions in windlg.c were using 'conf' by virtue of its
globalness, and it's easy to make those take it as a parameter.
These global variables are only ever used by load_settings, which uses
them to vary the default protocol and port number in the absence of
any specification elsewhere. So there's no real need for them to be
universally accessible via the awkward GLOBAL mechanism: they can be
statics inside settings.c, with accessor functions that can set them.
That was the last GLOBAL in putty.h, so I've removed the definition of
the macro GLOBAL itself as well. There are still some GLOBALs in the
Windows subdirectory, though.
I haven't managed to make this one _not_ be a mutable variable, but at
least it's not global across all tools any more: it lives in cmdline.c
along with the code that decides what to set it to, and cmdline.c
exports a query method to ask for its value.
Another ugly mutable global variable gone: now, instead of this
variable being defined in cmdline.c and written to by everyone's
main(), it's defined _alongside_ everyone's main() as a constant, and
cmdline.c just refers to it.
A bonus is that now nocmdline.c doesn't have to define it anyway for
tools that don't use cmdline.c. But mostly, it didn't need to be
mutable, so better for it not to be.
While I'm at it, I've also fiddled with the bit flags that go in it,
to define their values automatically using a list macro instead of
manually specifying each one to be a different power of 2.
When I came to actually remove the global 'flags' word, I found that I
got compile failures in two functions that should never have been
accessing it at all, because they forgot to declare _local_ variables
of the same name. Yikes!
(Of course, _now_ that's harmless, because I've just removed all the
actual semantics from the global variable. But I'm about to remove the
variable too, so these bugs would become compile failures.)
It really had no need to be a global process flag at all: it's used to
signal to uxcons.c that stderr_tty_init() ought to check whether
stderr is a tty (in order to save its termios across interactive
prompts and so forth). But the only _caller_ of stderr_tty_init is
Unix Plink, which always sets that flag! So we already have a
perfectly good system for signalling to uxcons.c that you'd like
stderr_tty_init to do something: _whether you call it or not_.
The global 'int flags' has always been an ugly feature of this code
base, and I suddenly thought that perhaps it's time to start throwing
it out, one flag at a time, until it's totally unused.
My first target is FLAG_VERBOSE. This was usually set by cmdline.c
when it saw a -v option on the program's command line, except that GUI
PuTTY itself sets it unconditionally on startup. And then various bits
of the code would check it in order to decide whether to print a given
message.
In the current system of front-end abstraction traits, there's no
_one_ place that I can move it to. But there are two: every place that
checked FLAG_VERBOSE has access to either a Seat or a LogPolicy. So
now each of those traits has a query method for 'do I want verbose
messages?'.
A good effect of this is that subsidiary Seats, like the ones used in
Uppity for the main SSH server module itself and the server end of
shell channels, now get to have their own verbosity setting instead of
inheriting the one global one. In fact I don't expect any code using
those Seats to be generating any messages at all, but if that changes
later, we'll have a way to control it. (Who knows, perhaps logging in
Uppity might become a thing.)
As part of this cleanup, I've added a new flag to cmdline_tooltype,
called TOOLTYPE_NO_VERBOSE_OPTION. The unconditionally-verbose tools
now set that, and it has the effect of making cmdline.c disallow -v
completely. So where 'putty -v' would previously have been silently
ignored ("I was already verbose"), it's now an error, reminding you
that that option doesn't actually do anything.
Finally, the 'default_logpolicy' provided by uxcons.c and wincons.c
(with identical definitions) has had to move into a new file of its
own, because now it has to ask cmdline.c for the verbosity setting as
well as asking console.c for the rest of its methods. So there's a new
file clicons.c which can only be included by programs that link
against both cmdline.c _and_ one of the *cons.c, and I've renamed the
logpolicy to reflect that.
A trawl through the code with -Wmissing-prototypes and
-Wmissing-variable-declarations turned up a lot of things that should
have been internal to a particular source file, but were accidentally
global. Keep the namespace clean by making them all static.
(Also, while I'm here, a couple of them were missing a 'const': the
ONE and ZERO arrays in sshcrcda.c, and EMPTY_WINDOW_TITLE in
terminal.c.)
These are all intended to ensure that the declarations of things in
header files are in scope where the same thing is subsequently
defined, to make it harder to define it in a way that doesn't match.
(For example, the new #include in winnet.c would have caught the
just-fixed mis-definition of platform_get_x11_unix_address.)
In commit 09954a87c I introduced the portfwdmgr_connect_socket()
system, which opened a port forwarding given a callback to create the
Socket itself, with the aim of using it to make forwardings to Unix-
domain sockets and Windows named pipes (both initially for agent
forwarding).
But I forgot that a year and a bit ago, in commit 834396170, I already
introduced a similar low-level system for creating a PortForwarding
around an unusual kind of socket: the portfwd_raw_new() system, which
in place of a callback uses a two-phase setup protocol (you create the
socket in between the two setup calls, and can roll it back if the
socket can't be created).
There's really no need to have _both_ these systems! So now I'm
merging them, which is to say, I'm enhancing portfwd_raw_new to have
the one new feature it needs, and throwing away the newer system
completely. The new feature is to be able to control the initial state
of the 'ready' flag: portfwd_raw_new was always used for initiating
port forwardings in response to an incoming local connection, which
means you need to start off with ready=false and set it true when the
other end of the SSH connection sends back OPEN_CONFIRMATION. Now it's
being used for initiating port forwardings in response to a
CHANNEL_OPEN, we need to be able to start with ready=true.
This commit reverts 09954a87c2 and its
followup fix 12aa06ccc9, and simplifies
the agent_connect system down to a single trivial function that makes
a Socket given a Plug.
This is a pure refactoring: no functional change expected.
This commit introduces two new small vtable-style APIs. One is
PageantClient, which identifies a particular client of the Pageant
'core' (meaning the code that handles each individual request). This
changes pageant_handle_msg into an asynchronous operation: you pass in
an agent request message and an identifier, and at some later point,
the got_response method in your PageantClient will be called with the
answer (and the same identifier, to allow you to match requests to
responses). The trait vtable also contains a logging system.
The main importance of PageantClient, and the reason why it has to
exist instead of just passing pageant_handle_msg a bare callback
function pointer and context parameter, is that it provides robustness
if a client stops existing while a request is still pending. You call
pageant_unregister_client, and any unfinished requests associated with
that client in the Pageant core will be cleaned up, so that you're
guaranteed that after the unregister operation, no stray callbacks
will happen with a stale pointer to that client.
The WM_COPYDATA interface of Windows Pageant is a direct client of
this API. The other client is PageantListener, the system that lives
in pageant.c and handles stream-based agent connections for both Unix
Pageant and the new Windows named-pipe IPC. More specifically, each
individual connection to the listening socket is a separate
PageantClient, which means that if a socket is closed abruptly or
suffers an OS error, that client can be unregistered and any pending
requests cancelled without disrupting other connections.
Users of PageantListener have a second client vtable they can use,
called PageantListenerClient. That contains _only_ logging facilities,
and at the moment, only Unix Pageant bothers to use it (and even that
only in debugging mode).
Finally, internally to the Pageant core, there's a new trait called
PageantAsyncOp which describes an agent request in the process of
being handled. But at the moment, it has only one trivial
implementation, which is handed the full response message already
constructed, and on the next toplevel callback, passes it back to the
PageantClient.
Those chomp operations in wincons.c and uxcons.c looked ugly, and I'm
not totally convinced they couldn't underrun the buffer by 1 byte in
weird circumstances. strbuf_chomp is neater.
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.
