There aren't quite as many of these as there are on Unix, but Windows
Plink and PSFTP still share some suspiciously similar-looking code.
Now they're both clients of wincliloop.c.
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.
We now have no remaining things in header files that switch from being
a declaration to a definition depending on an awkward #define at the
point of including that header. There are still a few mutable
variables with external linkage, but at least now each one is defined
in a specific source file file appropriate to its purpose and context.
The remaining globals as of this commit were:
- 'logctx' and 'term', which never needed to be globals in the first
place, because they were never actually shared between source
files. Now 'term' is just a static in window.c, and 'logctx' is a
static in each of that and winplink.c.
- 'hinst', which still has external linkage, but is now defined
separately in each source file that sets it up (i.e. those with a
WinMain)
- osMajorVersion, osMinorVersion and osPlatformId, whose definitions
now live in winmisc.c alongside the code which sets them up.
(Actually they were defined there all along, it turns out, but
every toolchain I've built with has commoned them together with the
version defined by the GLOBAL in the header.)
- 'hwnd', which nothing was actually _using_ any more after previous
commits, so all this commit had to do was delete it.
The declarations in header files now use ordinary 'extern'. That means
I have to arrange to put definitions matching those declarations in
the appropriate modules; so I've made a macro DEFINE_WINDOWS_FUNCTION
which performs a definition matching a prior DECLARE_WINDOWS_FUNCTION
(and reusing the typedef made by the latter).
This applies not only to the batch of functions that were marked
GLOBAL in winstuff.h, but also the auxiliary sets marked
WINCAPI_GLOBAL and WINSECUR_GLOBAL in wincapi.h and winsecur.h
respectively.
This was the difficult part of cleaning up that global variable. The
main Windows PuTTY GUI is split between source files, so that _does_
actually need to refer to the main window from multiple places.
But all the places where windlg.c needed to use 'hwnd' are seat
methods, so they were already receiving a Seat pointer as a parameter.
In other words, the methods of the Windows GUI Seat were already split
between source files. So it seems only fair that they should be able
to share knowledge of the seat's data as well.
Hence, I've created a small 'WinGuiSeat' structure which both window.c
and windlg.c can see the layout of, and put the main terminal window
handle in there. Then the seat methods implemented in windlg.c, like
win_seat_verify_ssh_host_key, can use container_of to turn the Seat
pointer parameter back into the address of that structure, just as the
methods in window.c can do (even though they currently don't need to).
(Who knows: now that it _exists_, perhaps that structure can be
gradually expanded in future to turn it into a proper encapsulation of
all the Windows frontend's state, like we should have had all
along...)
I've also moved the Windows GUI LogPolicy implementation into the same
object (i.e. WinGuiSeat implements both traits at once). That allows
win_gui_logging_error to recover the same WinGuiSeat from its input
LogPolicy pointer, which means it can get from there to the Seat facet
of the same object, so that I don't need the extern variable
'win_seat' any more either.
Windows Pageant doesn't really have a 'main window' any more, ever
since I separated the roles of system-tray management and IPC receiver
into two different hidden windows managed by different threads. So it
was already silly to be storing one of them in the global 'HWND hwnd'
variable, because it's no longer obvious which it should be.
So there's now a static variable 'traywindow' within winpgnt.c which
it uses in place of the global 'hwnd'.
The GUI version of pgp_fingerprints() is now a differently named
function that takes a parent HWND as a parameter, and so does my
help-enabled wrapper around MessageBox.
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.
It's silly to set it at each call site of restrict_process_acl() if
that function returns success! More sensible to have it be a flag in
the same source file as restrict_process_acl(), set as an automatic
_side effect_ of success.
I've renamed the variable itself, and the global name 'restricted_acl'
is now a query function that asks winsecur.c whether that operation
has been (successfully) performed.
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.
This is another piece of the old 2003 attempt at async agent requests.
Nothing ever calls this function (in particular, the new working
version of async-agent doesn't need it). Remove it completely, and all
its special-window-message implementations too.
(If we _were_ still using this function, then it would surely be
possible to fold it into the more recently introduced general
toplevel-callback system, and get rid of all this single-use special
code. But we're not, so removing it completely is even easier.)
In particular, this system was the only reason why Windows Plink paid
any attention to its message queue. So now I can make it call plain
WaitForMultipleObjects instead of MsgWaitForMultipleObjects.
This was the easiest flag to remove: nothing ever checks it at all!
It was part of an abandoned early attempt to make Pageant requests
asynchronous. The flag was added in commit 135abf244 (April 2003); the
code that used it was #ifdef-ed out in commit 98d735fde (January 2004),
and removed completely in commit f864265e3 (January 2017).
We now have an actually working system for async agent requests on
Windows, via the new named-pipe IPC. And we also have a perfectly good
way to force a particular agent request to work synchronously: just
pass NULL as the callback function pointer. All of that works just
fine, without ever using this flag. So begone!
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.
These were just too footling for even me to bother splitting up into
multiple commits:
- a couple of int -> size_t changes left out of the big-bang commit
0cda34c6f
- a few 'const' added to pointer-type casts that are only going to be
read from (leaving out the const provokes a warning if the pointer
was const _before_ the cast)
- a couple of 'return' statements trying to pass the void return of
one function through to another.
- another missing (void) in a declaration in putty.h (but this one
didn't cause any knock-on confusion).
- a few tweaks to macros, to arrange that they eat a semicolon after
the macro call (extra do ... while (0) wrappers, mostly, and one
case where I had to do it another way because the macro included a
variable declaration intended to remain in scope)
- reworked key_type_to_str to stop putting an unreachable 'break'
statement after every 'return'
- removed yet another type-check of a function loaded from a Windows
system DLL
- and finally, a totally spurious semicolon right after an open brace
in mainchan.c.
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.)
Similarly to the previous commit, this function had an inconsistent
parameter list between Unix and Windows, because the Windows source
file that defines it (winnet.c) didn't include ssh.h where its
prototype lives, so the compiler never checked.
Luckily, the discrepancy was that the Windows version of the function
was declared as taking an extra parameter which it ignored, so the fix
is very easy.
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 was pointed out as a compiler warning when I test-built with
up-to-date clang-cl. It looks as if it would cause the IDM_FULLSCREEN
item on the system menu to be wrongly greyed/ungreyed, but in fact I
think it's benign, because MF_BYCOMMAND == 0. So it's _just_ a
warning fix, luckily!
A user reports that Visual Studio 2013 and earlier have printf
implementations in their C library that don't support the 'z' modifier
to indicate that an integer argument is size_t. The 'I' modifier
apparently works in place of it.
To avoid littering ifdefs everywhere, I've invented my own inttypes.h
style macros to wrap size_t formatting directives, which are defined
to %zu and %zx normally, or %Iu and %Ix in old-VS mode. Those are in
defs.h, and they're used everywhere that a %z might otherwise get into
the Windows build.
In an early draft of commit de38a4d82 I used 'void *' as the reqid
type, and then I thought better of it and made it a special type of
its own, in keeping with my usual idea that it's better to have your
casts somewhat checked than totally unchecked. One remnant of the
'void *' version got past me. Now fixed.
A user reports that the ReadFile call in console_get_userpass_input
fails with ERROR_NOT_ENOUGH_MEMORY on Windows 7, and further reports
that this problem only happens if you tell ReadFile to read more than
31366 bytes in a single call.
That seems to be a thing that other people have found as well: I
turned up a similar workaround in Ruby's Win32 support module, except
that there it's for WriteConsole. So I'm reducing my arbitrary read
size of 64K to 16K, which is well under that limit.
This issue became noticeable in PuTTY as of the recent commit
cd6bc14f0, which reworked console_get_userpass_input to use strbufs.
Previously we were trying to read an amount proportional to the
existing size of the buffer, so as to grow the buffer exponentially to
save quadratic-time reallocation. That was OK in practice, since the
initial read size was nice and small. But in principle, the same bug
was present in that version of the code, just latent - if we'd ever
been called on to read a _really large_ amount of data, then
_eventually_ the input size parameter to ReadFile would have grown
beyond that mysterious limit!
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.
This is preparation to allow Pageant to be able to return to its GUI
main loop in the middle of handling a request (e.g. present a dialog
box to the user related to that particular request, and wait for the
user's response). In order to do that, we need the main thread's
Windows message loop to never be blocked by a WM_COPYDATA agent
request.
So I've split Pageant's previous hidden window into two hidden
windows, each with a subset of the original roles, and created in
different threads so that they get independent message loops. The one
in the main thread receives messages relating to Pageant's system tray
icon; the one in the subthread has the identity known to (old) Pageant
clients, and receives WM_COPYDATA messages only. Each WM_COPYDATA is
handled by passing the request back to the main thread via an event
object integrated into the Pageant main loop, and then waiting for a
second event object that the main thread will signal when the answer
comes back, and not returning from the WndProc handler until the
response arrives.
Hence, if an agent request received via WM_COPYDATA requires GUI
activity, then the main thread's GUI message loop will be able to do
that in parallel with all Pageant's other activity, including other
GUI activity (like the key list dialog box) and including responding
to other requests via named pipe.
I can't stop WM_COPYDATA requests from blocking _each other_, but this
allows them not to block anything else. And named-pipe requests block
nothing at all, so as clients switch over to the new IPC, even that
blockage will become less and less common.
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.
In setting up the ECC tests for cmdgen, I noticed that OpenSSH and
PuTTYgen disagree on the bit length to put in a key fingerprint for an
ed25519 key: we think 255, they think 256.
On reflection, I think 255 is more accurate, which is why I bodged
get_fp() in the test suite to ignore that difference when checking our
key fingerprint against OpenSSH's. But having done that, it now seems
silly that if you unnecessarily specify a bit count at ed25519
generation time, cmdgen will insist that it be 256!
255 is now permitted everywhere an ed25519 bit count is input. 256 is
also still allowed for backwards compatibility but 255 is preferred by
the error message if you give any other value.
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.
As in the previous commit, this means that agent_query() is now able
to operate in an asynchronous mode, so that if Pageant takes time to
answer a request, the GUI of the PuTTY instance making the request
won't be blocked.
Also as in the previous commit, we still fall back to the WM_COPYDATA
protocol if the new named pipe protocol isn't available.
Now that Pageant runs a named-pipe server as well as a WM_COPYDATA
server, we prefer the former (if available) for agent forwarding, for
the same reasons as on Unix: it lets us establish a simple raw-data
streaming connection instead of agentf.c's complicated message
boundary detection and buffer management, and if agent connections
ever become stateful, this technique will cope.
On Windows, another advantage of this change is that forwarded agent
requests can now be asynchronous: if the agent takes time to respond
to a request for any reason, then the rest of PuTTY's GUI and SSH
connection are not blocked, and you can carry on working while the
agent is thinking about the request.
(I didn't list that as a benefit of doing the same thing for Unix in
commit ae1148267, because on Unix, agent_query() could _already_ run
asynchronously. It's only on Windows that that's new.)
This reuses all the named-pipe IPC code I set up for connection
sharing a few years ago, to set up a named pipe with a predictable
name and speak the stream-oriented SSH agent protocol over it.
In this commit, we just set up the server, and there's no code that
speaks the client end of the new IPC yet. But my plan is that clients
should switch over to using this interface if possible, because it's
generally better: it doesn't have to be handled synchronously in the
middle of a GUI event loop (either in Pageant itself _or_ in its
client), and it's a better fit to the connection-oriented nature of
forwarded agent connections (so if any features ever appear in the
agent protocol that require state within a connection, we'll now be
able to support them).
This contains most of the guts of the previously monolithic function
new_named_pipe_client(), but it directly returns the HANDLE to the
opened pipe, or a string error message on failure.
new_named_pipe_client() is now a thin veneer on top of that, which
returns a Socket * by wrapping up the HANDLE into a HandleSocket or
the error message into an ErrorSocket as appropriate.
So it's now possible to connect to a named pipe, using all our usual
infrastructure (including in particular the ownership check of the
server, to defend against spoofing attacks), without having to have a
Socket-capable event loop in progress.
Windows Plink and PSFTP had very similar implementations, and now they
share one that lives in a new file winselcli.c. I've similarly moved
GUI PuTTY's implementation out of window.c into winselgui.c, where
other GUI programs wanting to do networking will be able to access
that too.
In the spirit of centralisation, I've also taken the opportunity to
make both functions use the reasonably complete winsock_error_string()
rather than (for some historical reason) each inlining a minimal
version that reports most errors as 'unknown'.
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.
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.
The do_select function is called with a boolean parameter indicating
whether we're supposed to start or stop paying attention to network
activity on a given socket. So if we freeze and unfreeze the socket in
mid-session because of backlog, we'll call do_select(s, false) to
freeze it, and do_select(s, true) to unfreeze it.
But the implementation of do_select in the Windows SFTP code predated
the rigorous handling of socket backlogs, so it assumed that
do_select(s, true) would only be called at initialisation time, i.e.
only once, and therefore that it was safe to use that flag as a cue to
set up the Windows event object to associate with socket activity.
Hence, every time the socket was frozen and unfrozen, we would create
a new netevent at unfreeze time, leaking the old one.
I think perhaps part of the reason why that was hard to figure out was
that the boolean parameter was called 'startup' rather than 'enable'.
To make it less confusing the next time I read this code, I've also
renamed it, and while I was at it, adjusted another related comment.
This commit switches as many ssh_hash_free / ssh_hash_new pairs as
possible to reuse the previous hash object via ssh_hash_reset. Also a
few other cleanups: use the wrapper function hash_simple() where
possible, and I've also introduced ssh_hash_digest_nondestructive()
and switched to that where possible as well.
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.
Thanks to Patrick Stekovic for pointing out that, unlike sensible IP
stacks, Windows requires a non-default socket option to prevent a
second application from binding to a port you were already listening
on, causing some of your incoming connections to be diverted.
This replaces the previous setsockopt that enabled SO_REUSEADDR, which
I put there a long time ago in order to fix an annoying behaviour if
you used the same listening socket twice in rapid succession (e.g. for
successive PuTTYs forwarding the same port) and the second one failed
to bind the listening port because a left-over connection from the
first one was still in TIME_WAIT and causing the port number to be
marked as used.
As far as I can see, SO_EXCLUSIVEADDRUSE and SO_REUSEADDR are mutually
exclusive - if I try to set both, either way round, then setsockopt
returns failure on the second one - so if I have to set the former
then I _can't_ set the latter. And fortunately, re-testing on Windows
10, the TIME_WAIT problem that SO_REUSEADDR was supposed to solve
doesn't seem to exist any more: I deliberately tried listening on a
port that had a TIME_WAIT connection sitting on it, and it worked for
me even without SO_REUSEADDR.
(I can't remember now whether I definitely confirmed the TIME_WAIT
problem on a previous version of Windows, or whether I just assumed it
would happen on Windows in the same way as Linux, where I definitely
do remember observing it.)
While I'm changing that setsockopt call, I've also fixed its 'on'
parameter so that it's a BOOL rather than an int, in accordance with
the docs for WinSock setsockopt.
The message "Reusing a shared connection to this server" is sent to
the seat's output method during the call to ssh_init. In Windows
Plink, that output method wants to talk to the BinarySink stderr_bs
(or stdout_bs, but for this particular message, stderr). So we have to
have already set up stderr_bs by the time the backend init function is
called.
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.
The RESIZE_EITHER resizing mode responds to a window resize by
changing the logical terminal size if the window is shown normally, or
by changing the font size to keep the terminal size the same if the
resize is a transition between normal and maximised state.
But a user pointed out that it's also possible for a window to receive
a WM_SIZE message while _remaining_ in maximised state, and that
PuTTY's resize logic didn't allow for that possibility. It occurs when
there's a change in the amount of available screen space for the
window to be maximised _in_: e.g. when the video resolution is
reconfigured, or when you reconnect to a Remote Desktop session using
a client window of a different size, or even when you toggle the
'Automatically hide the taskbar' option in the Windows taskbar settings.
In that situation, the right thing seems to be for PuTTY to continue
to go with the policy of changing the font size rather than the
logical terminal size. In other words, we prefer to change the font
size when the resize is _from_ maximised state, _to_ maximised state,
_or both_.
That's easily implemented by removing the check of the 'was_zoomed'
flag, in the case where we've received a WM_SIZE message with the
state SIZE_MAXIMIZED: once we know the transition is _to_ maximised
state, it doesn't matter whether or not it was also _from_ it. (But we
still set the was_zoomed flag to the most recent maximised status, so
that we can recognise transitions _out_ of maximised mode.)
Commit f2e61275f converted the integer casts in cmpforsearch to
uintptr_t from unsigned long. But it left the companion function
cmpfortree alone, presumably on the grounds that the compiler didn't
report a warning for that one.
But those two functions (cmpfortree and cmpforsearch) are used with
the same tree234, so they're supposed to implement the same sorting
criterion. And the thing they're actually comparing, namely the
Windows API typedef SOCKET, is a pointer-sized integer. So there was a
latent bug here in which cmpforsearch was comparing all 64 bits of the
pointer, while cmpfortree was only comparing the low-order 32.
Set the initial buffer size to MAX_PATH + 1 (261). Increment e->i before
the function returns instead of incrementing it in the call to
RegEnumKey.
The initial buffer size was too small to fit a subkey with a 256
characters long name plus '\0', the first call to RegEnumKey would fail
with ERROR_MORE_DATA, sgrowarray would grow the buffer, and RegEnumKey
would be called again.
However, because e->i was incremented in the first RegEnumKey call, the
second call would get the next subkey and the subkey with the long name
would be skipped.
Saving a session with a 256 characters long name would trigger this
problem. The session would be saved in the registry, but Putty would not
be able to display it in the saved sessions list.
Pageant didn't have this problem since it uses a different function to get
the saved sessions and the size of the buffer used is MAX_PATH + 1. Pageant
and Putty would display slightly different lists of saved sessions.
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.
If the agent sent a response whose length field describes an interval
of memory larger than the file-mapping object the message is supposed
to be stored in, we shouldn't return that message to the client as if
nothing is wrong. Treat that the same as a failure to receive any
response at all.
Looking over this function today, I spotted several questionable uses
of strcat to concatenate "\PUTTY.RND" to the end of a pathname,
without having checked whether the pathname had filled up the static
fixed-size buffer already.
I don't think this is exploitable (because you'd have to be in control
of the local account already to control any of the data sources used
to fill those buffers). But it's horrible anyway, of course. Now all
of those are replaced with sensible dupcats.
(This patch re-indents a lot of the function, to give variables
tighter scopes. So the diff is best viewed with whitespace ignored.)
This assertion was supposed to be checking for the buffer overrun
fixed by the previous commit, but because it checks the buffer index
just _after_ writing into the buffer, it would have permitted a
one-byte overrun before failing the assertion.
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.)
Normally I never notice warnings in this build, because it runs inside
bob and dumps all the warnings in a part of the build log I never look
at. But I've had these fixes lying around for a while and should
commit them.
They're benign: all we need is an explicit declaration of strtoumax to
replace the one that stdlib.h doesn't provide, and a couple more of
those annoying NO_TYPECHECK modifiers on GET_WINDOWS_FUNCTION calls.
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.)
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.
Having explicitly _stated_ in commit 4dcc0fddf the principle that if
you ever queue a toplevel callback on a freeable object then you
should also call delete_callbacks_for_context on that object before
freeing it, I realised I'd never actually gone through and checked
methodically at every call site of queue_toplevel_callback. So I did,
and naturally, I found several missing ones.
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.
TranslateKey() on Windows passed all numeric-keypad key events to this
function in terminal.c, and accepted whatever it gave back. That
included the handling for the trivial case of the numeric keypad, when
Num Lock is on and application keypad mode hasn't overridden it, so
that the keypad should be returning actual digits. In that case,
format_numeric_keypad_key() itself was returning the same ASCII
character I had passed in to it as a keypad identifier, and
TranslateKey was returning that in turn as the final translation.
Unfortunately, that means that with Num Lock on, the numeric keypad
translates into what _I_ used as the logical keypad codes inside the
source code, not what the local keyboard layout thinks are the right
codes. In particular, the key I identified as keypad '.' would render
as '.' even on a German keyboard where it ought to produce ','.
Fixed by removing the fallback case in format_numeric_keypad_key()
itself, so now it returns the empty string if it didn't produce an
escape sequence as its translation. Instead, the special case is in
window.c, which checks for a zero-length output string and handles it
by falling through to the keyboard-layout specific ToUnicode code
further down TranslateKey().
On the GTK side, no change is needed here: the GTK keyboard handler
does things in the opposite order, by trying the local input method
_first_ (unless it can see a reason up front to override it), and only
calling format_numeric_keypad_key() if that didn't provide a
translation. So the fallback ASCII translation in the latter was
already not used.
Re-consider the icon in light of the font size, so that we pick the icon
whose size mostly closely matches the terminal font, rather than always
scaling the default icon.
Remove the 'winhelp-topic' IDs from the Halibut source, and from the
code. Now we have one fewer name to think of every time we add a
setting.
I've left the HELPCTX system in place, with the vague notion that it
might be a useful layer of indirection for some future help system on a
platform like Mac OS X.
(I've left the putty.hlp target in doc/Makefile, if nothing else because
this is a convenient test case for Halibut's WinHelp support. But the
resulting help file will no longer support context help.)
GetDlgItemText_alloc is often used to get passwords from text fields,
so the memory should be freed and erased properly. Otherwise parts
of passwords might leak in memory.
Signed-off-by: Sven Strickroth <email@cs-ware.de>
In my eagerness to make sure we didn't _accidentally_ change the
seat's trust status back to trusted at any point, I forgot to do it on
purpose if a second SSH login phase is legitimately run in the same
terminal after the first session has ended.
Fill in all the function pointers for 3rd party Windows GSS DLLs, not
just some of them. These were missed out when GSS key exchange was added
in d515e4f1a3.
At the point when we change over the seat's trust status to untrusted
for the last time, to finish authentication, Plink will now present a
final interactive prompt saying 'Press Return to begin session'. This
is a hint that anything after that that resembles an auth prompt
should be treated with suspicion, because _PuTTY_ thinks it's finished
authenticating.
This is of course an annoying inconvenience for interactive users, so
I've tried to reduce its impact as much as I can. It doesn't happen in
GUI PuTTY at all (because the trust sigil system is used instead); it
doesn't happen if you use plink -batch (because then the user already
knows that they _never_ expect an interactive prompt); and it doesn't
happen if Plink's standard input is being redirected from anywhere
other than the terminal / console (because then it would be pointless
for the server to try to scam passphrases out of the user anyway,
since the user isn't in a position to enter one in response to a spoof
prompt). So it should only happen to people who are using Plink in a
terminal for interactive login purposes, and that's not _really_ what
I ever intended Plink to be used for (which is why it's never had any
out-of-band control UI like OpenSSH's ~ system).
If anyone _still_ doesn't like this new prompt, it can also be turned
off using the new -no-antispoof flag, if the user is willing to
knowingly assume the risk.
In terminal-based GUI applications, this is passed through to
term_set_trust_status, to toggle whether lines are prefixed with the
new trust sigil. In console applications, the function returns false,
indicating to the backend that it should employ some other technique
for spoofing protection.
This is not yet used by anything, but the idea is that it'll be a
graphic in the terminal window that can't be replicated by a server
sending escape sequences, and hence can be used as a reliable
indication that the text on a particular terminal line is generated by
PuTTY itself and not passed through from the server. This will make it
possible to detect a malicious server trying to mimic local prompts to
trick you out of information that shouldn't be sent over the wire
(such as private-key passphrases).
The trust sigil I've picked is a small copy of the PuTTY icon, which
is thematically nice (it can be read as if the PuTTY icon is the name
of the speaker in a dialogue) and also convenient because we had that
graphic available already on all platforms. (Though the contortions I
had to go through to make the GTK 1 code draw it were quite annoying.)
The trust sigil has the same dimensions as a CJK double-width
character, i.e. it's 2 character cells wide by 1 high.
Now, instead of each seat's prompt-handling function doing the
control-char sanitisation of prompt text, the SSH code does it. This
means we can do it differently depending on the prompt.
In particular, prompts _we_ generate (e.g. a genuine request for your
private key's passphrase) are not sanitised; but prompts coming from
the server (in keyboard-interactive mode, or its more restricted SSH-1
analogues, TIS and CryptoCard) are not only sanitised but also
line-length limited and surrounded by uncounterfeitable headers, like
I've just done to the authentication banners.
This should mean that if a malicious server tries to fake the local
passphrase prompt (perhaps because it's somehow already got a copy of
your _encrypted_ private key), you can tell the difference.
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.
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 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.
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.
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().
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.
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.
