These include an unused variable left over from the command-line
refactoring; an explicit referencing of the module handle for
sspicli.dll which we really do deliberately load and then don't
(directly) use; a missing pointer-type cast in the Windows handle
socket code; and two 32/64 bit integer size mismatches in the types of
functions I was importing from system API DLLs.
The last of those are a bit worrying, and suggest to me that after
going to all that trouble to add type-checking of those runtime
imports in commit 49fb598b0, I might have only checked the resulting
compiler output in a 32-bit build and not a 64-bit one. Oops!
This is another piece of long-overdue refactoring similar to the
recent commit e3796cb77. But where that one dealt with normalisation
of stuff already stored _in_ a Conf by whatever means (including, in
particular, handling a user typing 'username@host.name' into the
Hostname box of the GUI session dialog box), this one deals with
handling argv entries and putting them into the Conf.
This isn't exactly a pure no-functional-change-at-all refactoring. On
the other hand, it isn't a full-on cleanup that completely
rationalises all the user-visible behaviour as well as the code
structure. It's somewhere in between: I've preserved all the behaviour
quirks that I could imagine a reason for having intended, but taken
the opportunity to _not_ faithfully replicate anything I thought was
clearly just a bug.
So, for example, the following inconsistency is carefully preserved:
the command 'plink -load session nextword' treats 'nextword' as a host
name if the loaded session hasn't provided a hostname already, and
otherwise treats 'nextword' as the remote command to execute on the
already-specified remote host, but the same combination of arguments
to GUI PuTTY will _always_ treat 'nextword' as a hostname, overriding
a hostname (if any) in the saved session. That makes some sense to me
because of the different shapes of the overall command lines.
On the other hand, there are two behaviour changes I know of as a
result of this commit: a third argument to GUI PuTTY (after a hostname
and port) now provokes an error message instead of being silently
ignored, and in Plink, if you combine a -P option (specifying a port
number) with the historical comma-separated protocol selection prefix
on the hostname argument (which I'd completely forgotten even existed
until this piece of work), then the -P will now override the selected
protocol's default port number, whereas previously the default port
would win. For example, 'plink -P 12345 telnet,hostname' will now
connect via Telnet to port 12345 instead of to port 23.
There may be scope for removing or rethinking some of the command-
line syntax quirks in the wake of this change. If we do decide to do
anything like that, then hopefully having it all in one place will
make it easier to remove or change things consistently across the
tools.
A more or less identical piece of code to sanitise the CONF_host
string prior to session launch existed in Windows PuTTY and both
Windows and Unix Plink. It's long past time it was centralised.
While I'm here, I've added a couple of extra comments in the
centralised version, including one that - unfortunately - tries _but
fails_ to explain why a string of the form "host.name:1234" doesn't
get the suffix moved into CONF_port the way "user@host" moves the
prefix into CONF_username. Commit c1c1bc471 is the one I'm referring
to in the comment, and unfortunately it has an unexplained one-liner
log message from before I got into the habit of being usefully
verbose.
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'.
ATTR_REVERSE was being handled in the front ends, and was causing the
foreground and background colours to be switched. (I'm not completely
sure why I made that design decision; it might be purely historical,
but then again, it might also be because reverse video is one effect
on the fg and bg colours that must still be performed even in unusual
frontend-specific situations like display-driven monochrome mode.)
This affected both explicit reverse video enabled using SGR 7, and
also the transient reverse video arising from mouse selection. Thanks
to Markus Gans for reporting the bug in the latter, which when I
investigated it turned out to affect the former as well.
I've done this on a 'where possible' basis: in Windows paletted mode
(in case anyone is still using an old enough graphics card to need
that!) I simply haven't bothered, and will completely ignore the dim
flag.
Markus Gans points out that some applications which (not at all
unreasonably) don't trust $TERM to tell them the full capabilities of
their terminal will use the sequence "OSC 4 ; nn ; ? BEL" to ask for
the colour-palette value in position nn, and they may not particularly
care _what_ the results are but they will use them to decide whether
the right number of colour palette entries even exist.
Otherwise, moving the cursor (at least in active, filled-cell mode) on
to a true-coloured character cell causes it to vanish completely
because the cell's colours override the thing that differentiates the
cursor.
I'm not sure if any X11 monochrome visuals or Windows paletted display
modes are still around, but just in case they are, we shouldn't
attempt true colour on either kind of display.
I know some users don't like any colour _at all_, and we have a
separate option to turn off xterm-style 256-colour sequences, so it
seems remiss not to have an option to disable true colour as well.
This is a heavily rewritten version of a patch originally by Lorenz
Diener; it was tidied up somewhat by Christian Brabandt, and then
tidied up more by me. The basic idea is to add to the termchar
structure a pair of small structs encoding 24-bit RGB values, each
with a flag indicating whether it's turned on; if it is, it overrides
any other specification of fg or bg colour for that character cell.
I've added a test line to colours.txt containing a few example colours
from /usr/share/X11/rgb.txt. In fact it makes quite a good demo to run
the whole of rgb.txt through this treatment, with a command such as
perl -pe 's!^\s*(\d+)\s+(\d+)\s+(\d+).*$!\e[38;2;$1;$2;$3m$&\e[m!' rgb.txt
This causes PuTTY processes spawned from its system-tray menu to run
with the -restrict-acl option (or rather, the synonymous &R prefix
used by my auto-constructed command lines for easier parsing).
The previous behaviour of Pageant was never to pass -restrict-acl to
PuTTY, even when started with -restrict-acl itself; this is not
actually a silly thing to want to do, because Pageant might well have
more need of -restrict-acl than PuTTY (it stores longer-term and more
powerful secrets) and conversely PuTTY might have more need to _not_
restrict its ACL than Pageant (in that among the things enabled by an
unrestricted ACL are various kinds of accessibility software, which is
more useful on the more user-facing PuTTY than on Pageant).
But for those who want to lock everything down with every security
option possible (even though -restrict-acl is only an ad-hoc
precaution and cannot deliver any hard guarantees), this new option
should fill in the UI gap.
After a conversation this week with a user who tried to use it, it's
clear that Borland C can't build the up-to-date PuTTY without having
to make too many compromises of functionality (unsupported API
details, no 'long long' type), even above the issues that could be
worked round with extra porting ifdefs.
64-bit PuTTY should be loading gssapi64.dll, not gssapi32.dll. (In
contrast to the Windows system API DLLs, such as secur32.dll which is
also mentioned in the same source file; those keep the "32" in their
name whether we're in Win32 or Win64.)
Like every other toolchain I've tried, my Coverity scanning build has
its share of random objections to parts of my Windows API type-
checking system. I do wonder if that bright idea was worth the hassle
- but it would probably cost all the annoyance all over again to back
out now...
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.
Rather than loading some from spoolss.dll, which some MSDN documentation
suggests, but which doesn't work very well in practice.
(Also, remove an unused variable.)
I've also been experimenting recently with running Wix on Linux under
Mono, rather than running it on Windows in the obvious way. Wix under
Mono insists on forward slashes in pathnames, and it turns out that
Wix on Windows doesn't object to them either, so I can safely change
them over unconditionally and then my installer source will work in
both modes.
stdint.h is one of the set of standard C headers that has more to do
with the compiler than the library, and hence, clang provides its own
version of it, even when you're using it in clang-cl mode with Visual
Studio headers, and even when those headers are old enough not to have
a stdint.h of their own. So in clang builds, including stdint.h is
always the right way to get uintptr_t defined.
Patch due to Brian K. White; we now condition our own declaration of
DLL_DIRECTORY_COOKIE on whether the toolchain's headers had #defined
it already, rather than trying to guess the answer to that from
version-number macros.
(Apparently everything that defines DLL_DIRECTORY_COOKIE does it by
does seem to work in all my tests.)
When making select_result() return void (a2fb1d9), I removed a "return"
at the end of the FD_CLOSE case, causing a fallthrough into FD_ACCEPT
with hilarious (segfaulting) consequences. Re-instate the "return".
plug_receive() and plug_closing() return 0 or 1 depending on whether
they think the main connection has closed. It is not appropriate, as
handle_gotdata and handle_socket_unfreeze did, to treat them as
returning a backlog. In fact, plugs are unusual in PuTTY in not
reporting a backlog, but just calling into the socket to freeze and
unfreeze it as required.
It's redundant with back->connected(): only the SSH backend has a
receive function that can ever return 0, and whenever ssh_receive
returns 0 it has called ssh_do_close, which will cause future calls
to ssh_connected also to return 0. Similarly, all backend closing
functions ensure that future calls to their connected function will
return 0.
This too is not in the list of known DLLs on Windows 10. I don't know
of any actual viable hijacking attack based on it, which according to
my reading of MSDN (specifically, a rather vague hint in
https://msdn.microsoft.com/library/ff919712) _may_ be because we
mention the common controls assembly in our application manifest; but
better safe than sorry.
Now the entire list of remaining DLLs that PuTTY links against at load
time is a subset of the Win10 known DLLs list, so that _should_ mean
that everything we load before we've deployed our own defence
(SetDefaultDllDirectories) is defended against for us by Windows
itself.
The printing functions are split between winspool.drv and spoolss.dll
in a really weird way (who would have guessed that OpenPrinter and
ClosePrinter don't live in the same dynamic library?!), but _neither_
of those counts as a system 'known DLL', so linking against either one
of these at load time is again a potential DLL hijacking vector.
It's not on the default list of important system 'known DLLs' stored
at HKLM\SYSTEM\CurrentControlSet\Control\Session Manager\KnownDLLs (see
https://isc.sans.edu/forums/diary/DLL+hijacking+vulnerabilities/9445/ )
which apparently makes it exempt from Windows's standard DLL hijacking
defence, i.e. if an executable links against it in the normal way then
that executable will be vulnerable to DLL hijacking from a file called
winmm.dll in the same directory as it.
The solution is to load it dynamically _after_ we've locked down our
DLL search path, which fortunately PuTTY's code base is well used to
doing already for other DLLs.
This gives me an extra safety-check against having mistyped one of the
function prototypes that we load at run time from DLLs: we verify that
the typedef we defined based on the prototype in our source code
matches the type of the real function as declared in the Windows
headers.
This was an idea I had while adding a pile of further functions using
this mechanism. It didn't catch any errors (either in the new
functions or in the existing collection), but that's no reason not to
keep it anyway now that I've thought of it!
In VS2015, this automated type-check works for most functions, but a
couple manage to break it. SetCurrentProcessExplicitAppUserModelID in
winjump.c can't be type-checked, because including <shobjidl.h> where
that function is declared would also bring in a load of other stuff
that conflicts with the painful manual COM declarations in winjump.c.
(That stuff could probably be removed now we're on an up-to-date
Visual Studio, on the other hand, but that's a separate chore.) And
gai_strerror, used in winnet.c, does _have_ an implementation in a
DLL, but the header files like to provide an inline version with a
different calling convention, which defeats this error-checking trick.
And in the older VS2003 that we still precautionarily build with,
several more type-checks have to be #ifdeffed out because the
functions they check against just aren't there at all.
If MIT Kerberos is installed, then using GetProcAddress to extract
GetUserNameExA() from secur32.dll causes Windows to implicitly load
sspicli.dll in turn - and it does it in a search-path-unclean way.
If we load it in our own way before that happens, then Windows doesn't
need to load it again and won't do so wrongly.
[SGT: tidied up commit message from original patch]
gssapi32.dll from MIT Kerberos as well as from Heimdal both load
further DLLs from their installation directories.
[SGT: I polished the original patch a bit, in particular replacing
manual memory allocation with dup_mb_to_wc. This required a Recipe
change to link miscucs.c and winucs.c into more of the tools.]
I scrolled past it just now and decided those open braces at the ends
of the lines are just too ugly to live. They originally got that way
when I put the whole source base through GNU indent, which as far as
I'm concerned is a horrible misfeature of indent!
This is apparently the other half of what we should have done when we
called SetCurrentProcessExplicitAppUserModelID at run time: it
associates to PuTTY's Start Menu shortcut the same identifier that we
give to the running PuTTY process, so that jump lists saved under the
latter will be visible to users mousing over the former.
I've also done the same thing to the desktop shortcut, just in case
that does anything useful.
In the sentdata callback function given to handle_output_new, the
'new_backlog' parameter can be negative, and if so, it represents a
Windows error code and not a backlog size at all. handle_sentdata was
not checking for this before passing it on to plug_sent.
While I'm looking at these two dialog boxes, I notice there's another
prominent difference between PuTTY's one and these: I also never got
round to adding the button to go to PuTTY's main website. Now added.
The current About boxes are too small to fit in all the buildinfo
data, in particular the source-control commit id. Apparently I forgot
to enlarge them when I enlarged the one in PuTTY proper.
(All the same information is nonetheless *present* in the box, but
there seems to be no way to scroll a static text control, so you can
only find that out by 'Select All' and copying to the clipboard.)
Anyway. Now resized to the same dimensions as the main PuTTY About
box. (Really I should centralise more definitions into a common
resource file, but there we go.)
