That's more directly useful in uxpty.c (which is currently the only
actual client of the function), and also matches the data that SSH
clients send in "pty-req". Also, it makes that method behave more like
the GUI query function get_window_pixels used by terminal.c (with the
sole exception that unlike g_w_p it's allowed to return failure), so
it becomes even more trivial to implement in the GUI front ends.
This is a new vtable-based abstraction which is passed to a backend in
place of Frontend, and it implements only the subset of the Frontend
functions needed by a backend. (Many other Frontend functions still
exist, notably the wide range of things called by terminal.c providing
platform-independent operations on the GUI terminal window.)
The purpose of making it a vtable is that this opens up the
possibility of creating a backend as an internal implementation detail
of some other activity, by providing just that one backend with a
custom Seat that implements the methods differently.
For example, this refactoring should make it feasible to directly
implement an SSH proxy type, aka the 'jump host' feature supported by
OpenSSH, aka 'open a secondary SSH session in MAINCHAN_DIRECT_TCP
mode, and then expose the main channel of that as the Socket for the
primary connection'. (Which of course you can already do by spawning
'plink -nc' as a separate proxy process, but this would permit it in
the _same_ process without anything getting confused.)
I've centralised a full set of stub methods in misc.c for the new
abstraction, which allows me to get rid of several annoying stubs in
the previous code. Also, while I'm here, I've moved a lot of
duplicated modalfatalbox() type functions from application main
program files into wincons.c / uxcons.c, which I think saves
duplication overall. (A minor visible effect is that the prefixes on
those console-based fatal error messages will now be more consistent
between applications.)
LogContext is now the owner of the logevent() function that back ends
and so forth are constantly calling. Previously, logevent was owned by
the Frontend, which would store the message into its list for the GUI
Event Log dialog (or print it to standard error, or whatever) and then
pass it _back_ to LogContext to write to the currently open log file.
Now it's the other way round: LogContext gets the message from the
back end first, writes it to its log file if it feels so inclined, and
communicates it back to the front end.
This means that lots of parts of the back end system no longer need to
have a pointer to a full-on Frontend; the only thing they needed it
for was logging, so now they just have a LogContext (which many of
them had to have anyway, e.g. for logging SSH packets or session
traffic).
LogContext itself also doesn't get a full Frontend pointer any more:
it now talks back to the front end via a little vtable of its own
called LogPolicy, which contains the method that passes Event Log
entries through, the old askappend() function that decides whether to
truncate a pre-existing log file, and an emergency function for
printing an especially prominent message if the log file can't be
created. One minor nice effect of this is that console and GUI apps
can implement that last function subtly differently, so that Unix
console apps can write it with a plain \n instead of the \r\n
(harmless but inelegant) that the old centralised implementation
generated.
One other consequence of this is that the LogContext has to be
provided to backend_init() so that it's available to backends from the
instant of creation, rather than being provided via a separate API
call a couple of function calls later, because backends have typically
started doing things that need logging (like making network
connections) before the call to backend_provide_logctx. Fortunately,
there's no case in the whole code base where we don't already have
logctx by the time we make a backend (so I don't actually remember why
I ever delayed providing one). So that shortens the backend API by one
function, which is always nice.
While I'm tidying up, I've also moved the printf-style logeventf() and
the handy logevent_and_free() into logging.c, instead of having copies
of them scattered around other places. This has also let me remove
some stub functions from a couple of outlying applications like
Pageant. Finally, I've removed the pointless "_tag" at the end of
LogContext's official struct name.
It's not used anywhere, but this would make it one step easier to add
a mode argument to PSFTP's mkdir command, if anyone needs it. Mostly
the point is to get rid of the FIXME comment in fxp_mkdir_send itself.
GUI feedback mode was last seen in 2006 (removed in commit 33b7caa59),
so quite what a conditioned-out piece of online help text for it was
doing still around here 12 years later, I have no idea.
(Especially since it had been under #if 0 since 2001, and also since
then its containing source file had ceased to be Windows-only so it
would have been extra-wrong to reinstate it.)
Another mistake in commit 54b300f15 was to introduce a new flag
'progress_bar_displayed', when in fact we were already storing an
indication of whether a set of live transfer statistics were currently
on the display, in the form of prev_stats_len (which is also used to
make sure each stats display overwrites all of the previous one).
Removed that redundancy, and while I'm at it, renamed the new
abandon_progress_bar() to match the rest of the code's general
convention of calling that status display 'statistics' or 'transfer
statistics' rather than a 'progress bar'.
In commit 54b300f15, I managed to set the progress_bar_displayed flag
just _after_, rather than before, the call to abandon_progress_bar
that moves to the new line once the file has finished copying. So in
the case where a file is so small that the very first displaying of
the transfer statistics is already at 100% completion, the flag
wouldn't be set when abandon_progress_bar checked for it, and a
newline still wouldn't be printed.
In order to list cross-certifiable host keys in the GUI specials menu,
the SSH backend has been inventing new values on the end of the
Telnet_Special enumeration, starting from the value TS_LOCALSTART.
This is inelegant, and also makes it awkward to break up special
handlers (e.g. to dispatch different specials to different SSH
layers), since if all you know about a special is that it's somewhere
in the TS_LOCALSTART+n space, you can't tell what _general kind_ of
thing it is. Also, if I ever need another open-ended set of specials
in future, I'll have to remember which TS_LOCALSTART+n codes are in
which set.
So here's a revamp that causes every special to take an extra integer
argument. For all previously numbered specials, this argument is
passed as zero and ignored, but there's a new main special code for
SSH host key cross-certification, in which the integer argument is an
index into the backend's list of available keys. TS_LOCALSTART is now
a thing of the past: if I need any other open-ended sets of specials
in future, I can add a new top-level code with a nicely separated
space of arguments.
While I'm at it, I've removed the legacy misnomer 'Telnet_Special'
from the code completely; the enum is now SessionSpecialCode, the
struct containing full details of a menu entry is SessionSpecial, and
the enum values now start SS_ rather than TS_.
Pavel Kryukov reports that gcc 8 didn't like that buffer being the
same size as the one from which I was sprintf("%s")ing into it. The
easiest fix is to stop trying to guess buffer sizes and use dupprintf.
Originally, it controlled whether ssh.c should send terminal messages
(such as login and password prompts) to terminal.c or to stderr. But
we've had the from_backend() abstraction for ages now, which even has
an existing flag to indicate that the data is stderr rather than
stdout data; applications which set FLAG_STDERR are precisely those
that link against uxcons or wincons, so from_backend will do the
expected thing anyway with data sent to it with that flag set. So
there's no reason ssh.c can't just unconditionally pass everything
through that, and remove the special case.
FLAG_STDERR was also used by winproxy and uxproxy to decide whether to
capture standard error from a local proxy command, or whether to let
the proxy command send its diagnostics directly to the usual standard
error. On reflection, I think it's better to unconditionally capture
the proxy's stderr, for three reasons. Firstly, it means proxy
diagnostics are prefixed with 'proxy:' so that you can tell them apart
from any other stderr spew (which used to be particularly confusing if
both the main application and the proxy command were instances of
Plink); secondly, proxy diagnostics are now reliably copied to packet
log files along with all the other Event Log entries, even by
command-line tools; and thirdly, this means the option to suppress
proxy command diagnostics after the main session starts will actually
_work_ in the command-line tools, which it previously couldn't.
A more minor structure change is that copying of Event Log messages to
stderr in verbose mode is now done by wincons/uxcons, instead of
centrally in logging.c (since logging.c can now no longer check
FLAG_STDERR to decide whether to do it). The total amount of code to
do this is considerably smaller than the defensive-sounding comment in
logevent.c explaining why I did it the other way instead :-)
Now there's a centralised routine in misc.c to do the sanitisation,
which copies data on to an outgoing bufchain. This allows me to remove
from_backend_untrusted() completely from the frontend API, simplifying
code in several places.
Two use cases for untrusted-terminal-data sanitisation were in the
terminal.c prompts handler, and in the collection of SSH-2 userauth
banners. Both of those were writing output to a bufchain anyway, so
it was very convenient to just replace a bufchain_add with
sanitise_term_data and then not have to worry about it again.
There was also a simplistic sanitiser in uxcons.c, which I've now
replaced with a call to the good one - and in wincons.c there was a
FIXME saying I ought to get round to that, which now I have!
This is another major source of unexplained 'void *' parameters
throughout the code.
In particular, the currently unused testback.c actually gave the wrong
pointer type to its internal store of the frontend handle - it cast
the input void * to a Terminal *, from which it got implicitly cast
back again when calling from_backend, and nobody noticed. Now it uses
the right type internally as well as externally.
Nearly every part of the code that ever handles a full backend
structure has historically done it using a pair of pointer variables,
one pointing at a constant struct full of function pointers, and the
other pointing to a 'void *' state object that's passed to each of
those.
While I'm modernising the rest of the code, this seems like a good
time to turn that into the same more or less type-safe and less
cumbersome system as I'm using for other parts of the code, such as
Socket, Plug, BinaryPacketProtocol and so forth: the Backend structure
contains a vtable pointer, and a system of macro wrappers handles
dispatching through that vtable.
Same principle again - the more of these structures have globally
visible tags (even if the structure contents are still opaque in most
places), the fewer of them I can mistake for each other.
That's one fewer anonymous 'void *' which might be accidentally
confused with some other pointer type if I misremember the order of
function arguments.
While I'm here, I've made its pointer-nature explicit - that is,
'Ldisc' is now a typedef for the structure type itself rather than a
pointer to it. A stylistic change only, but it feels more natural to
me these days for a thing you're going to eventually pass to a 'free'
function.
This is a cleanup I started to notice a need for during the BinarySink
work. It removes a lot of faffing about casting things to char * or
unsigned char * so that some API will accept them, even though lots of
such APIs really take a plain 'block of raw binary data' argument and
don't care what C thinks the signedness of that data might be - they
may well reinterpret it back and forth internally.
So I've tried to arrange for all the function call APIs that ought to
have a void * (or const void *) to have one, and those that need to do
pointer arithmetic on the parameter internally can cast it back at the
top of the function. That saves endless ad-hoc casts at the call
sites.
It's an incoherent concept! There should not be any such thing as an
error box that terminates the entire program but is not modal. If it's
bad enough to terminate the whole program, i.e. _all_ currently live
connections, then there's no point in permitting progress to continue
in windows other than the affected one, because all windows are
affected anyway.
So all previous uses of fatalbox() have become modalfatalbox(), except
those which looked to me as if they shouldn't have been fatal in the
first place, e.g. lingering pieces of error handling in winnet.c which
ought to have had the severity of 'give up on this particular Socket
and close it' rather than 'give up on the ENTIRE UNIVERSE'.
Ilya Shipitsin sent me a list of errors reported by a tool 'cppcheck',
which I hadn't seen before, together with some fixes for things
already taken off that list. This change picks out all the things from
the remaining list that I could quickly identify as actual errors,
which it turns out are all format-string goofs along the lines of
using a %d with an unsigned int, or a %u with a signed int, or (in the
cases in charset/utf8.c) an actual _size_ mismatch which could in
principle have caused trouble on a big-endian target.
We responded to a local error writing out the destination file by
going into a mode where we just looped round receiving and throwing
away data until our count of the file size reached the file size we
expected - but once we were in that mode, we never actually
incremented the count!
xfer_download_data could return actuallen as either 0 or -1 to
indicate EOF. Now it's always 0, and scp_recv_filedata actually checks
for that case and reports an error.
When called with -V to ask for our version, return 0 rather than 1.
This is the usual behaviour observed by ssh(1) and other Unix commands.
Also use exit() rather than cleanup_exit() in pscp.c and psftp.c ; at
this point we have nothing to cleanup!
Partly to reassure the user that they got what they asked for, and
partly so that's a clue for us in the logs when we get bug reports.
This involved repurposing platform_psftp_post_option_setup() (no longer
used since e22120fe) as platform_psftp_pre_conn_setup(), and moving it
to after logging is set up.
This shows the build platform (32- vs 64-bit in particular, and also
whether Unix GTK builds were compiled with or without the X11 pieces),
what compiler was used to build the binary, and any interesting build
options that might have been set on the make command line (especially,
but not limited to, the security-damaging ones like NO_SECURITY or
UNPROTECT). This will probably be useful all over the place, but in
particular it should allow the different Windows binaries to be told
apart!
Commits 21101c739 and 2eb952ca3 laid the groundwork for this, by
allowing the various About boxes to contain free text and also
ensuring they could be copied and pasted easily as part of a bug
report.
Formerly PuTTY's SFTP code would transmit (or buffer) a megabyte of data
before even starting to look for acknowledgements, but wouldn't allow
there to be more than a megabyte of unacknowledged data at a time. Now,
instead, it pays attention to whether the transmit path is blocked, and
transmits iff it isn't.
This should mean that SFTP goes faster over long fat pipes, and also
doesn't end up buffering so much over thin ones.
I practice, I tend to run into other performance limitations (such as
TCP or SSH-2 windows) before this enhancement looks particularly good,
but with an artificial lag of 250 ms on the loopback interface this
patch almost doubles my upload speed, so I think it's worthwhile.
Protecting our processes from outside interference need not be limited
to just PuTTY: there's no reason why the other SSH-speaking tools
shouldn't have the same treatment (PSFTP, PSCP, Plink), and PuTTYgen
and Pageant which handle private key material.
There was a rogue sscanf("%s") with no field width limit, targeting a
stack-based buffer, and scanning a string containing untrusted data.
It occurs in the 'sink' side of the protocol, i.e. when downloading
files *from* the server.
Our own bug id for this vulnerability is 'vuln-pscp-sink-sscanf'.
TOOLTYPE_NONNETWORK (i.e. pterm) already has "-log" (as does Unix
PuTTY), so there's no sense suppressing the synonym "-sessionlog".
Undocumented lacunae that remain:
plink accepts -sessionlog, but does nothing with it. Arguably it should.
puttytel accepts -sshlog/-sshrawlog (and happily logs e.g. Telnet
negotiation, as does PuTTY proper).
(cherry picked from commit a454399ec8)
Conflicts:
unix/uxplink.c
windows/winplink.c
(cherry-picker's notes: the conflict was only contextual, in the Plink
help output)
TOOLTYPE_NONNETWORK (i.e. pterm) already has "-log" (as does Unix
PuTTY), so there's no sense suppressing the synonym "-sessionlog".
Undocumented lacunae that remain:
plink accepts -sessionlog, but does nothing with it. Arguably it should.
puttytel accepts -sshlog/-sshrawlog (and happily logs e.g. Telnet
negotiation, as does PuTTY proper).
I noticed that Unix PSCP was unwantedly renaming downloaded files
which had a backslash in their names, because pscp.c's stripslashes()
treated \ as a path component separator, since it hadn't been modified
since PSCP ran on Windows only.
It also turns out that pscp.c, psftp.c and winsftp.c all had a
stripslashes(), and they didn't all have quite the same prototype. So
now there's one in winsftp.c and one in uxsftp.c, with appropriate
OS-dependent behaviour, and the ones in pscp.c and psftp.c are gone.
Having found a lot of unfixed constness issues in recent development,
I thought perhaps it was time to get proactive, so I compiled the
whole codebase with -Wwrite-strings. That turned up a huge load of
const problems, which I've fixed in this commit: the Unix build now
goes cleanly through with -Wwrite-strings, and the Windows build is as
close as I could get it (there are some lingering issues due to
occasional Windows API functions like AcquireCredentialsHandle not
having the right constness).
Notable fallout beyond the purely mechanical changing of types:
- the stuff saved by cmdline_save_param() is now explicitly
dupstr()ed, and freed in cmdline_run_saved.
- I couldn't make both string arguments to cmdline_process_param()
const, because it intentionally writes to one of them in the case
where it's the argument to -pw (in the vain hope of being at least
slightly friendly to 'ps'), so elsewhere I had to temporarily
dupstr() something for the sake of passing it to that function
- I had to invent a silly parallel version of const_cmp() so I could
pass const string literals in to lookup functions.
- stripslashes() in pscp.c and psftp.c has the annoying strchr nature
I'm not actually sure why we've always had back ends notify ldisc of
changes to echo/edit settings by giving ldisc_send(ldisc,NULL,0,0) a
special meaning, instead of by having a separate dedicated notify
function with its own prototype and parameter set. Coverity's recent
observation that the two kinds of call don't even have the same
requirements on the ldisc (particularly, whether ldisc->term can be
NULL) makes me realise that it's really high time I separated the two
conceptually different operations into actually different functions.
While I'm here, I've renamed the confusing ldisc_update() function
which that special operation ends up feeding to, because it's not
actually a function applying to an ldisc - it applies to a front end.
So ldisc_send(ldisc,NULL,0,0) is now ldisc_echoedit_update(ldisc), and
that in turn figures out the current echo/edit settings before passing
them on to frontend_echoedit_update(). I think that should be clearer.
I've gone through everywhere we handle host names / addresses (on
command lines, in PuTTY config, in port forwarding, in X display
names, in host key storage...) and tried to make them handle IPv6
literals sensibly, by using the host_str* functions I introduced in my
previous commit. Generally it's now OK to use a bracketed IPv6 literal
anywhere a hostname might have been valid; in a few cases where no
ambiguity exists (e.g. no :port suffix is permitted anyway)
unbracketed IPv6 literals are also acceptable.
[originally from svn r10120]
The basic strategy is described at the top of the new source file
sshshare.c. In very brief: an 'upstream' PuTTY opens a Unix-domain
socket or Windows named pipe, and listens for connections from other
PuTTYs wanting to run sessions on the same server. The protocol spoken
down that socket/pipe is essentially the bare ssh-connection protocol,
using a trivial binary packet protocol with no encryption, and the
upstream has to do some fiddly transformations that I've been
referring to as 'channel-number NAT' to avoid resource clashes between
the sessions it's managing.
This is quite different from OpenSSH's approach of using the Unix-
domain socket as a means of passing file descriptors around; the main
reason for that is that fd-passing is Unix-specific but this system
has to work on Windows too. However, there are additional advantages,
such as making it easy for each downstream PuTTY to run its own
independent set of port and X11 forwardings (though the method for
making the latter work is quite painful).
Sharing is off by default, but configuration is intended to be very
easy in the normal case - just tick one box in the SSH config panel
and everything else happens automatically.
[originally from svn r10083]
pscp.c when I did the big revamp in r9279: I assumed that in any SCP
connection we would be the first to send EOF, but in fact this isn't
true - doing downloads with old-SCP, EOF is initiated by the server,
so we were spuriously reporting an error for 'unexpected' EOF when
everything had gone fine. Thanks to Nathan Phelan for the report.
[originally from svn r10016]
[r9279 == 947962e0b9]
within the same string that destfname points to the start of, so
freeing it causes at best a double-free of destfname and more likely a
free of something that isn't even the start of an allocated block.
[originally from svn r9974]
[r9916 == cc4f38df14]
that the user really ought to know but that are not actually fatal to
continued operation of PuTTY or a single network connection.
[originally from svn r9932]
effect of handling it, but they do not free it if it isn't a packet
they recognise as part of their upload/download. Invent a return value
that specifically signals this, and consistently free pktin at every
call site if that return value comes back. Also, ensure that that
return value also always comes with something meaningful in fxp_error.
[originally from svn r9915]
own newline before the one tell_user puts on the end anyway. Also,
while I'm here, make up my mind about whether to prefix messages with
"scp:" or "pscp:" - I choose the latter.
[originally from svn r9914]
array pointer, _even_ if you're asking it to sort zero elements so
that in principle it should never dereference that pointer. Fix the
four instances in PSCP/PSFTP where this was previously occurring.
[originally from svn r9912]
Simon Tatham
Owen Dunn
Jacob Nevins
Ben Harris