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.
Previously, we returned a valid settings_r containing a null HKEY.
That didn't actually cause trouble (I think all the registry API
functions must have spotted the null HKEY and returned a clean error
code instead of crashing), but it means the caller can't tell if the
session really existed or not. Now we return NULL in that situation,
and close_settings_r avoids crashing if we pass the NULL to it later.
My trawl of all the vtable systems in the code spotted a couple of
other function-like macros in passing, which might as well be
rewritten as inline functions too for the same reasons.
If Plink's standard output and/or standard error points at a Windows
console or a Unix tty device, and if Plink was not configured to
request a remote pty (and hence to send a terminal-type string), then
we apply the new control-character stripping facility.
The idea is to be a mild defence against malicious remote processes
sending confusing escape sequences through the standard error channel
when Plink is being used as a transport for something like git: it's
OK to have actual sensible error messages come back from the server,
but when you run a git command, you didn't really intend to give the
remote server the implicit licence to write _all over_ your local
terminal display. At the same time, in that scenario, the standard
_output_ of Plink is left completely alone, on the grounds that git
will be expecting it to be 8-bit clean. (And Plink can tell that
because it's redirected away from the console.)
For interactive login sessions using Plink, this behaviour is
disabled, on the grounds that once you've sent a terminal-type string
it's assumed that you were _expecting_ the server to use it to know
what escape sequences to send to you.
So it should be transparent for all the use cases I've so far thought
of. But in case it's not, there's a family of new command-line options
like -no-sanitise-stdout and -sanitise-stderr that you can use to
forcibly override the autodetection of whether to do it.
This all applies the same way to both Unix and Windows Plink.
Rather like isatty() on Unix, this tells you if a raw Windows HANDLE
points at a console or not. Useful to know if your standard output or
standard error is going to be shown to a user, or redirected to
something that will make automated use of it.
There's now a stdio_sink, whose write function calls fwrite on the
given FILE *; a bufchain_sink, whose write function appends to the
given bufchain; and on Windows there's a handle_sink whose write
function writes to the given 'struct handle'. (That is, not the raw
Windows HANDLE, but our event-loop-friendly wrapper on it.)
Not yet used for anything, but they're about to be.
Although I've reinstated the tedious manual mouse input, I can at
least reduce the amount of it that the user is required to provide:
the new PRNG has a hard limit on the size of its seed, so once we've
generated enough entropy to fill that up, there's no point in
collecting more, even if we're generating a particularly large key.
This reverts the policy change in 6142013ab (though not the detailed
code changes - I've kept the reorganised code layout). Now the old
mouse-based manual entropy collection is once again required when
generating a public key.
Rationale: I came across Wikipedia's page on CryptGenRandom which
mentioned that it was not a true kernel-level PRNG of the /dev/random
variety, but rather a thing running in userland, no different in
principle from PuTTY's own. So I think that makes it no longer a thing
we should rely on for all our entropy, and I'm relegating it back to
being just one entropy source among many.
Now that all the call sites are expecting a size_t instead of an int
length field, it's no longer particularly difficult to make it
actually return the pointer,length pair in the form of a ptrlen.
It would be nice to say that simplifies call sites because those
ptrlens can all be passed straight along to other ptrlen-consuming
functions. Actually almost none of the call sites are like that _yet_,
but this makes it possible to move them in that direction in future
(as part of my general aim to migrate ptrlen-wards as much as I can).
But also it's just nicer to keep the pointer and length together in
one variable, and not have to declare them both in advance with two
extra lines of boilerplate.
This is a general cleanup which has been overdue for some time: lots
of length fields are now the machine word type rather than the (in
practice) fixed 'int'.
Now we pass an error code in a separate dedicated parameter, instead
of overloading the length parameter so that a negative value means an
error code. This enables length to become unsigned without causing
trouble.
Those were a reasonable abbreviation when the code almost never had to
deal with little-endian numbers, but they've crept into enough places
now (e.g. the ECC formatting) that I think I'd now prefer that every
use of the integer read/write macros was clearly marked with its
endianness.
So all uses of GET_??BIT and PUT_??BIT are now qualified. The special
versions in x11fwd.c, which used variable endianness because so does
the X11 protocol, are suffixed _X11 to make that clear, and where that
pushed line lengths over 80 characters I've taken the opportunity to
name a local variable to remind me of what that extra parameter
actually does.
If there was still pending output data on a NetSocket's output_data
bufchain when it was closed, then we wouldn't have freed it, on either
Unix or Windows.
Similarly to my recent addition of NEON-accelerated AES, these new
implementations drop in alongside the SHA-NI ones, under a different
set of ifdefs. All the details of selection and detection are
essentially the same as they were for the AES code.
This tears out the entire previous random-pool system in sshrand.c. In
its place is a system pretty close to Ferguson and Schneier's
'Fortuna' generator, with the main difference being that I use SHA-256
instead of AES for the generation side of the system (rationale given
in comment).
The PRNG implementation lives in sshprng.c, and defines a self-
contained data type with no state stored outside the object, so you
can instantiate however many of them you like. The old sshrand.c still
exists, but in place of the previous random pool system, it's just
become a client of sshprng.c, whose job is to hold a single global
instance of the PRNG type, and manage its reference count, save file,
noise-collection timers and similar administrative business.
Advantages of this change include:
- Fortuna is designed with a more varied threat model in mind than my
old home-grown random pool. For example, after any request for
random numbers, it automatically re-seeds itself, so that if the
state of the PRNG should be leaked, it won't give enough
information to find out what past outputs _were_.
- The PRNG type can be instantiated with any hash function; the
instance used by the main tools is based on SHA-256, an improvement
on the old pool's use of SHA-1.
- The new PRNG only uses the completely standard interface to the
hash function API, instead of having to have privileged access to
the internal SHA-1 block transform function. This will make it
easier to revamp the hash code in general, and also it means that
hardware-accelerated versions of SHA-256 will automatically be used
for the PRNG as well as for everything else.
- The new PRNG can be _tested_! Because it has an actual (if not
quite explicit) specification for exactly what the output numbers
_ought_ to be derived from the hashes of, I can (and have) put
tests in cryptsuite that ensure the output really is being derived
in the way I think it is. The old pool could have been returning
any old nonsense and it would have been very hard to tell for sure.
The upcoming PRNG revamp will want to tell noise sources apart, so
that it can treat them all fairly. So I've added an extra parameter to
noise_ultralight and random_add_noise, which takes values in an
enumeration covering all the vague classes of entropy source I'm
collecting. In this commit, though, it's simply ignored.
Mostly on the Unix side: there are lots of places the Windows code was
collecting noise that the corresponding Unix/GTK code wasn't bothering
to, such as mouse movements, keystrokes and various network events.
Also, both platforms had forgotten to collect noise when reading data
from a pipe to a local proxy process, even though in that
configuration that's morally equivalent to the network packet timings
that we'd normally be collecting from.
All the hash-specific state structures, and the functions that
directly accessed them, are now local to the source files implementing
the hashes themselves. Everywhere we previously used those types or
functions, we're now using the standard ssh_hash or ssh2_mac API.
The 'simple' functions (hmacmd5_simple, SHA_Simple etc) are now a pair
of wrappers in sshauxcrypt.c, each of which takes an algorithm
structure and can do the same conceptual thing regardless of what it
is.
The refactored sshaes.c gives me a convenient slot to drop in a second
hardware-accelerated AES implementation, similar to the existing one
but using Arm NEON intrinsics in place of the x86 AES-NI ones.
This needed a minor structural change, because Arm systems are often
heterogeneous, containing more than one type of CPU which won't
necessarily all support the same set of architecture features. So you
can't test at run time for the presence of AES acceleration by
querying the CPU you're running on - even if you found a way to do it,
the answer wouldn't be reliable once the OS started migrating your
process between CPUs. Instead, you have to ask the OS itself, because
only that knows about _all_ the CPUs on the system. So that means the
aes_hw_available() mechanism has to extend a tentacle into each
platform subdirectory.
The trickiest part was the nest of ifdefs that tries to detect whether
the compiler can support the necessary parts. I had successful
test-compiles on several compilers, and was able to run the code
directly on an AArch64 tablet (so I know it passes cryptsuite), but
it's likely that at least some Arm platforms won't be able to build it
because of some path through the ifdefs that I haven't been able to
test yet.
The bulk of this commit is the changes necessary to make testcrypt
compile under Visual Studio. Unfortunately, I've had to remove my
fiddly clever uses of C99 variadic macros, because Visual Studio does
something unexpected when a variadic macro's expansion puts
__VA_ARGS__ in the argument list of a further macro invocation: the
commas don't separate further arguments. In other words, if you write
#define INNER(x,y,z) some expansion involving x, y and z
#define OUTER(...) INNER(__VA_ARGS__)
OUTER(1,2,3)
then gcc and clang will translate OUTER(1,2,3) into INNER(1,2,3) in
the obvious way, and the inner macro will be expanded with x=1, y=2
and z=3. But try this in Visual Studio, and you'll get the macro
parameter x expanding to the entire string 1,2,3 and the other two
empty (with warnings complaining that INNER didn't get the number of
arguments it expected).
It's hard to cite chapter and verse of the standard to say which of
those is _definitely_ right, though my reading leans towards the
gcc/clang behaviour. But I do know I can't depend on it in code that
has to compile under both!
So I've removed the system that allowed me to declare everything in
testcrypt.h as FUNC(ret,fn,arg,arg,arg), and now I have to use a
different macro for each arity (FUNC0, FUNC1, FUNC2 etc). Also, the
WRAPPED_NAME system is gone (because that too depended on the use of a
comma to shift macro arguments along by one), and now I put a custom C
wrapper around a function by simply re-#defining that function's own
name (and therefore the subsequent code has to be a little more
careful to _not_ pass functions' names between several macros before
stringifying them).
That's all a bit tedious, and commits me to a small amount of ongoing
annoyance because now I'll have to add an explicit argument count
every time I add something to testcrypt.h. But then again, perhaps it
will make the code less incomprehensible to someone trying to
understand it!
That's a terrible name, but winutils.c was already taken. The new
source file is intended to be to winmisc.c as the new utils.c is to
misc.c: it contains all the parts that are basically safe to link into
_any_ Windows program (even standalone test things), without tying in
to the runtime infrastructure of the main tools, referring to any
other PuTTY source module, or introducing an extra Win32 API library
dependency.
This is the commit that f3295e0fb _should_ have been. Yesterday I just
added some typedefs so that I didn't have to wear out my fingers
typing 'struct' in new code, but what I ought to have done is to move
all the typedefs into defs.h with the rest, and then go through
cleaning up the legacy 'struct's all through the existing code.
But I was mostly trying to concentrate on getting the test suite
finished, so I just did the minimum. Now it's time to come back and do
it better.
After I moved parts of misc.c into utils.c, we started getting two
versions of smemclr in the Windows builds, because utils.c didn't know
to omit its one, having not included the main putty.h.
But it was deliberate that utils.c didn't include putty.h, because I
wanted it (along with the rest of testcrypt in particular) to be
portable to unusual platforms without having to port the whole of the
code base.
So I've moved into the ubiquitous defs.h just the one decision about
whether we're on a platform that will supersede utils.c's definition
of smemclr.
(Also, in the process of moving it, I've removed the clause that
disabled the Windows smemclr in winelib mode, because it looks as if
the claim that winelib doesn't have SecureZeroMemory is now out of
date.)
misc.c has always contained a combination of things that are tied
tightly into the PuTTY code base (e.g. they use the conf system, or
work with our sockets abstraction) and things that are pure standalone
utility functions like nullstrcmp() which could quite happily be
dropped into any C program without causing a link failure.
Now the latter kind of standalone utility code lives in the new source
file utils.c, whose only external dependency is on memory.c (for snew,
sfree etc), which in turn requires the user to provide an
out_of_memory() function. So it should now be much easier to link test
programs that use PuTTY's low-level functions without also pulling in
half its bulky infrastructure.
In the process, I came across a memory allocation logging system
enabled by -DMALLOC_LOG that looks long since bit-rotted; in any case
we have much more advanced tools for that kind of thing these days,
like valgrind and Leak Sanitiser, so I've just removed it rather than
trying to transplant it somewhere sensible. (We can always pull it
back out of the version control history if really necessary, but I
haven't used it in at least a decade.)
The other slightly silly thing I did was to give bufchain a function
pointer field that points to queue_idempotent_callback(), and disallow
direct setting of the 'ic' field in favour of calling
bufchain_set_callback which will fill that pointer in too. That allows
the bufchain system to live in utils.c rather than misc.c, so that
programs can use it without also having to link in the callback system
or provide an annoying stub of that function. In fact that's just
allowed me to remove stubs of that kind from PuTTYgen and Pageant!
Taking a leaf out of the LLVM code base: this macro still includes an
assert(false) so that the message will show up in a typical build, but
it follows it up with a call to a function explicitly marked as no-
return.
So this ought to do a better job of convincing compilers that once a
code path hits this function it _really doesn't_ have to still faff
about with making up a bogus return value or filling in a variable
that 'might be used uninitialised' in the following code that won't be
reached anyway.
I've gone through the existing code looking for the assert(false) /
assert(0) idiom and replaced all the ones I found with the new macro,
which also meant I could remove a few pointless return statements and
variable initialisations that I'd already had to put in to placate
compiler front ends.
The old 'Bignum' data type is gone completely, and so is sshbn.c. In
its place is a new thing called 'mp_int', handled by an entirely new
library module mpint.c, with API differences both large and small.
The main aim of this change is that the new library should be free of
timing- and cache-related side channels. I've written the code so that
it _should_ - assuming I haven't made any mistakes - do all of its
work without either control flow or memory addressing depending on the
data words of the input numbers. (Though, being an _arbitrary_
precision library, it does have to at least depend on the sizes of the
numbers - but there's a 'formal' size that can vary separately from
the actual magnitude of the represented integer, so if you want to
keep it secret that your number is actually small, it should work fine
to have a very long mp_int and just happen to store 23 in it.) So I've
done all my conditionalisation by means of computing both answers and
doing bit-masking to swap the right one into place, and all loops over
the words of an mp_int go up to the formal size rather than the actual
size.
I haven't actually tested the constant-time property in any rigorous
way yet (I'm still considering the best way to do it). But this code
is surely at the very least a big improvement on the old version, even
if I later find a few more things to fix.
I've also completely rewritten the low-level elliptic curve arithmetic
from sshecc.c; the new ecc.c is closer to being an adjunct of mpint.c
than it is to the SSH end of the code. The new elliptic curve code
keeps all coordinates in Montgomery-multiplication transformed form to
speed up all the multiplications mod the same prime, and only converts
them back when you ask for the affine coordinates. Also, I adopted
extended coordinates for the Edwards curve implementation.
sshecc.c has also had a near-total rewrite in the course of switching
it over to the new system. While I was there, I've separated ECDSA and
EdDSA more completely - they now have separate vtables, instead of a
single vtable in which nearly every function had a big if statement in
it - and also made the externally exposed types for an ECDSA key and
an ECDH context different.
A minor new feature: since the new arithmetic code includes a modular
square root function, we can now support the compressed point
representation for the NIST curves. We seem to have been getting along
fine without that so far, but it seemed a shame not to put it in,
since it was suddenly easy.
In sshrsa.c, one major change is that I've removed the RSA blinding
step in rsa_privkey_op, in which we randomise the ciphertext before
doing the decryption. The purpose of that was to avoid timing leaks
giving away the plaintext - but the new arithmetic code should take
that in its stride in the course of also being careful enough to avoid
leaking the _private key_, which RSA blinding had no way to do
anything about in any case.
Apart from those specific points, most of the rest of the changes are
more or less mechanical, just changing type names and translating code
into the new API.
A user points out that the call to close_directory() in pscp.c's
rsource() function should have been inside rather than outside the if
statement that checks whether the directory handle is NULL. As a
result, any failed attempt to open a directory during a 'pscp -r'
recursive upload leads to a null-pointer dereference.
Moved the close_directory call to where it should be, and also
arranged to print the OS error code if the directory-open fails, by
also changing the API of open_directory to return an error string on
failure.
In the previous commit I happened to notice a %.150s in a ppl_logevent
call, which was probably an important safety precaution a couple of
decades ago when that format string was being used for an sprintf into
a fixed-size buffer, but now it's just pointless cruft.
This commit removes all printf string formatting directives with a
compile-time fixed size, with the one exception of a %.3s used to cut
out a 3-letter month name in scpserver.c. In cases where the format
string in question was already going to an arbitrary-length function
like dupprintf or ppl_logevent, that's all I've done; in cases where
there was still a fixed-size buffer, I've replaced it with a dynamic
buffer and dupprintf.
In the past, I've had a lot of macros which you call with double
parentheses, along the lines of debug(("format string", params)), so
that the inner parens protect the commas and permit the macro to treat
the whole printf-style argument list as one macro argument.
That's all very well, but it's a bit inconvenient (it doesn't leave
you any way to implement such a macro by prepending another argument
to the list), and now this code base's rules allow C99isms, I can
switch all those macros to using a single pair of parens, using the
C99 ability to say '...' in the parameter list of the #define and get
at the corresponding suffix of the arguments as __VA_ARGS__.
So I'm doing it. I've made the following printf-style macros variadic:
bpp_logevent, ppl_logevent, ppl_printf and debug.
While I'm here, I've also fixed up a collection of conditioned-out
calls to debug() in the Windows front end which were clearly expecting
a macro with a different calling syntax, because they had an integer
parameter first. If I ever have a need to condition those back in,
they should actually work now.
A long time ago, in commit 4d77b6567, I moved the generation of the
arrow-key escape sequences into a function format_arrow_key(). Mostly
the reason for that was a special purpose I had in mind at the time
which involved auto-generating the same sequences in response to
things other than a keypress, but I always thought it would be nice to
centralise a lot more of PuTTY's complicated keyboard handling in the
same way - at least the handling of the function keys and their
numerous static and dynamic config options.
In this year's general spirit of tidying up and refactoring, I think
it's finally time. So here I introduce three more centralised
functions for dealing with the numbered function keys, the small
keypad (Ins, Home, PgUp etc) and the numeric keypad. Lots of horrible
and duplicated code from the key handling functions in window.c and
gtkwin.c is now more sensibly centralised: each platform keyboard
handler concerns itself with the local format of a keyboard event and
platform-specific enumeration of key codes, and once it's decided what
the logical key press actually _is_, it hands off to the new functions
in terminal.c to generate the appropriate escape code.
Mostly this is intended to be a refactoring without functional change,
leaving the keyboard handling how it's always been. But in cases where
the Windows and GTK handlers were accidentally inconsistent, I've
fixed the inconsistency rather than carefully keeping both sides how
they were. Known consistency fixes:
- swapping the arrow keys between normal (ESC [ A) and application
(ESC O A) is now done by pressing Ctrl with them, and _not_ by
pressing Shift. That was how it was always supposed to work, and
how it's worked on GTK all along, but on Windows it's been done by
Shift as well since 2010, due to a bug at the call site of
format_arrow_key() introduced when I originally wrote that function.
- in Xterm function key mode plus application keypad mode, the /*-
keys on the numeric keypad now send ESC O {o,j,m} in place of ESC O
{Q,R,S}. That's how the Windows keyboard handler has worked all
along (it was a deliberate behaviour tweak for the Xterm-like
function key mode, because in that mode ESC O {Q,R,S} are generated
by F2-F4). But the GTK keyboard handler omitted that particular
special case and was still sending ESC O {Q,R,S} for those keys in
all application keypad modes.
- also in Xterm function key mode plus app keypad mode, we only
generates the app-keypad escape sequences if Num Lock is on; with
Num Lock off, the numeric keypad becomes arrow keys and
Home/End/etc, just as it would in non-app-keypad mode. Windows has
done this all along, but again, GTK lacked that special case.
Now they live in their own file memory.c. The advantage of this is
that you can link them into a binary without also pulling in the rest
of misc.c with its various dependencies on other parts of the code,
such as conf.c.
This is another cleanup I felt a need for while I was doing
boolification. If you define a function or variable in one .c file and
declare it extern in another, then nothing will check you haven't got
the types of the two declarations mismatched - so when you're
_changing_ the type, it's a pain to make sure you've caught all the
copies of it.
It's better to put all those extern declarations in header files, so
that the declaration in the header is also in scope for the
definition. Then the compiler will complain if they don't match, which
is what I want.
sk_startup and sk_nextaddr are entirely internal to winnet.c; nearly
all of import.c and minibidi.c's internal routines should have been
static and weren't; {read,write}_utf8 are internal to charset/utf8.c
(and didn't even need separate declarations at all); do_sftp_cleanup
is internal to psftp.c, and get_listitemheight to gtkdlg.c.
While I was editing those prototypes anyway, I've also added missing
'const' to the 'char *' passphrase parameters in import,c.
For a start, they now have different names on Windows and Unix,
reflecting their different roles: on Windows they apply escaping to
any string that's going to be used as a registry key (be it a session
name, or a host name for host key storage), whereas on Unix they're
for constructing saved-session file names in particular (and also
handle the special case of filling in "Default Settings" for NULL).
Also, they now produce output by writing to a strbuf, which simplifies
a lot of the call sites. In particular, the strbuf output idiom is
passed on to enum_settings_next, which is especially nice because its
only actual caller was doing an ad-hoc realloc loop that I can now get
rid of completely.
Thirdly, on Windows they're centralised into winmisc.c instead of
living in winstore.c, because that way Pageant can use the unescape
function too. (It was spotting the duplication there that made me
think of doing this in the first place, but once I'd started, I had to
keep unravelling the thread...)
There's one of these centralised in win_strerror() in winmisc.c, and
it doesn't seem worth keeping an earlier iteration of the same idea
entirely separate in winsock_error_string.
This removal means that non-network-specific error codes received in a
network context will no longer have "Network error:" prefixed to them.
But I think that's OK, because it seems unlikely to be critically
important that such an error was received from a network function - if
anything like that comes up then it's probably some kind of systemwide
chaos.
Jumped out at me in my trawl of the whole code base:
set_explicit_app_user_model_id is declared and defined as () rather
than (void), but this isn't C++.
My normal habit these days, in new code, is to treat int and bool as
_almost_ completely separate types. I'm still willing to use C's
implicit test for zero on an integer (e.g. 'if (!blob.len)' is fine,
no need to spell it out as blob.len != 0), but generally, if a
variable is going to be conceptually a boolean, I like to declare it
bool and assign to it using 'true' or 'false' rather than 0 or 1.
PuTTY is an exception, because it predates the C99 bool, and I've
stuck to its existing coding style even when adding new code to it.
But it's been annoying me more and more, so now that I've decided C99
bool is an acceptable thing to require from our toolchain in the first
place, here's a quite thorough trawl through the source doing
'boolification'. Many variables and function parameters are now typed
as bool rather than int; many assignments of 0 or 1 to those variables
are now spelled 'true' or 'false'.
I managed this thorough conversion with the help of a custom clang
plugin that I wrote to trawl the AST and apply heuristics to point out
where things might want changing. So I've even managed to do a decent
job on parts of the code I haven't looked at in years!
To make the plugin's work easier, I pushed platform front ends
generally in the direction of using standard 'bool' in preference to
platform-specific boolean types like Windows BOOL or GTK's gboolean;
I've left the platform booleans in places they _have_ to be for the
platform APIs to work right, but variables only used by my own code
have been converted wherever I found them.
In a few places there are int values that look very like booleans in
_most_ of the places they're used, but have a rarely-used third value,
or a distinction between different nonzero values that most users
don't care about. In these cases, I've _removed_ uses of 'true' and
'false' for the return values, to emphasise that there's something
more subtle going on than a simple boolean answer:
- the 'multisel' field in dialog.h's list box structure, for which
the GTK front end in particular recognises a difference between 1
and 2 but nearly everything else treats as boolean
- the 'urgent' parameter to plug_receive, where 1 vs 2 tells you
something about the specific location of the urgent pointer, but
most clients only care about 0 vs 'something nonzero'
- the return value of wc_match, where -1 indicates a syntax error in
the wildcard.
- the return values from SSH-1 RSA-key loading functions, which use
-1 for 'wrong passphrase' and 0 for all other failures (so any
caller which already knows it's not loading an _encrypted private_
key can treat them as boolean)
- term->esc_query, and the 'query' parameter in toggle_mode in
terminal.c, which _usually_ hold 0 for ESC[123h or 1 for ESC[?123h,
but can also hold -1 for some other intervening character that we
don't support.
In a few places there's an integer that I haven't turned into a bool
even though it really _can_ only take values 0 or 1 (and, as above,
tried to make the call sites consistent in not calling those values
true and false), on the grounds that I thought it would make it more
confusing to imply that the 0 value was in some sense 'negative' or
bad and the 1 positive or good:
- the return value of plug_accepting uses the POSIXish convention of
0=success and nonzero=error; I think if I made it bool then I'd
also want to reverse its sense, and that's a job for a separate
piece of work.
- the 'screen' parameter to lineptr() in terminal.c, where 0 and 1
represent the default and alternate screens. There's no obvious
reason why one of those should be considered 'true' or 'positive'
or 'success' - they're just indices - so I've left it as int.
ssh_scp_recv had particularly confusing semantics for its previous int
return value: its call sites used '<= 0' to check for error, but it
never actually returned a negative number, just 0 or 1. Now the
function and its call sites agree that it's a bool.
In a couple of places I've renamed variables called 'ret', because I
don't like that name any more - it's unclear whether it means the
return value (in preparation) for the _containing_ function or the
return value received from a subroutine call, and occasionally I've
accidentally used the same variable for both and introduced a bug. So
where one of those got in my way, I've renamed it to 'toret' or 'retd'
(the latter short for 'returned') in line with my usual modern
practice, but I haven't done a thorough job of finding all of them.
Finally, one amusing side effect of doing this is that I've had to
separate quite a few chained assignments. It used to be perfectly fine
to write 'a = b = c = TRUE' when a,b,c were int and TRUE was just a
the 'true' defined by stdbool.h, that idiom provokes a warning from
gcc: 'suggest parentheses around assignment used as truth value'!
I think this is the full set of things that ought logically to be
boolean.
One annoyance is that quite a few radio-button controls in config.c
address Conf fields that are now bool rather than int, which means
that the shared handler function can't just access them all with
conf_{get,set}_int. Rather than back out the rigorous separation of
int and bool in conf.c itself, I've just added a similar alternative
handler function for the bool-typed ones.
This commit includes <stdbool.h> from defs.h and deletes my
traditional definitions of TRUE and FALSE, but other than that, it's a
100% mechanical search-and-replace transforming all uses of TRUE and
FALSE into the C99-standardised lowercase spellings.
No actual types are changed in this commit; that will come next. This
is just getting the noise out of the way, so that subsequent commits
can have a higher proportion of signal.
The annoying int64.h is completely retired, since C99 guarantees a
64-bit integer type that you can actually treat like an ordinary
integer. Also, I've replaced the local typedefs uint32 and word32
(scattered through different parts of the crypto code) with the
standard uint32_t.
After the recent Seat and LogContext revamps, _nearly_ all the
remaining uses of the type 'Frontend' were in terminal.c, which needs
all sorts of interactions with the GUI window the terminal lives in,
from the obvious (actually drawing text on the window, reading and
writing the clipboard) to the obscure (minimising, maximising and
moving the window in response to particular escape sequences).
All of those functions are now provided by an abstraction called
TermWin. The few remaining uses of Frontend after _that_ are internal
to a particular platform directory, so as to spread the implementation
of that particular kind of Frontend between multiple source files; so
I've renamed all of those so that they take a more specifically named
type that refers to the particular implementation rather than the
general abstraction.
So now the name 'Frontend' no longer exists in the code base at all,
and everywhere one used to be used, it's completely clear whether it
was operating in one of Frontend's three abstract roles (and if so,
which), or whether it was specific to a particular implementation.
Another type that's disappeared is 'Context', which used to be a
typedef defined to something different on each platform, describing
whatever short-lived resources were necessary to draw on the terminal
window: the front end would provide a ready-made one when calling
term_paint, and the terminal could request one with get_ctx/free_ctx
if it wanted to do proactive window updates. Now that drawing context
lives inside the TermWin itself, because there was never any need to
have two of those contexts live at the same time.
(Another minor API change is that the window-title functions - both
reading and writing - have had a missing 'const' added to their char *
parameters / return values.)
I don't expect this change to enable any particularly interesting new
functionality (in particular, I have no plans that need more than one
implementation of TermWin in the same application). But it completes
the tidying-up that began with the Seat and LogContext rework.
In the very old days, when PuTTY was new and computers were slow, I
tried to implement a feature where scrolling the window would be
implemented using a fast rectangle-copy GDI operation, rather than an
expensive character-by-character redraw of all the changed areas.
It never quite worked right, and I ended up conditioning it out on
Windows, and never even tried to implement it on GTK. It's now been
sitting around unused for so long that I think it's no longer worth
keeping in the code at all - if I tried to put it back in, it surely
wouldn't even compile, and would need rewriting from scratch anyway.
Disturbingly, it looks as if I _tried_ to re-enable it at one point,
in that there was a '#define OPTIMISE_IS_SCROLL 1' in putty.h - but
that never had any effect, because the macro name is misspelled. All
the #ifdefs are for 'OPTIMISE_SCROLL', without the 'IS'. So despite
appearances, it really _has_ been conditioned out all along!
Previously, it returned a human-readable string suitable for log
files, which tried to say something useful about the remote end of a
socket. Now it returns a whole SocketPeerInfo structure, of which that
human-friendly log string is just one field, but also some of the same
information - remote IP address and port, in particular - is provided
in machine-readable form where it's available.
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.)
This was used by ldisc to communicate back to the front end that a key
had been pressed (or rather, that a keypress had caused a nonzero
amount of session input data). Its only nontrivial implementation was
in gtkwin.c, which used that notification to implement the Unix GUI's
"close window on keypress, if the session was already over" policy.
(Which in turn is Unix-specific, because the rationale is that
sometimes X servers don't have a functioning window manager, so it's
useful to have a way of telling any application to close without using
WM-provided facilities like a close button.)
But gtkwin.c doesn't need to be told by the ldisc that a keypress
happened - it's the one _sending_ those keypresses to ldisc in the
first place! So I've thrown away the three stub implementations of
frontend_keypress, removed the call to it in ldisc.c, and replaced it
with calls in gtkwin.c at all the points during keypress handling
that call ldisc_send.
A visible effect is that pterm's close-on-keypress behaviour will now
only trigger on an actual (input-generating) _keypress_, and not on
other input generation such as a paste action. I think that's an
improvement.
One quite recent - an unused variable in the Windows code that was
obsoleted by commit cea1329b9 last month - and one not recent at all,
namely the obsolete declaration of begin_session() in putty.h that
hasn't existed since commit 7a79df8fe replaced it with the ldisc
system in *2001*!
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.
Almost all the call sites were doing a cumbersome dupprintf-use-free
cycle to get a formatted message into an ErrorSocket anyway, so it
seems more sensible to give them an easier way of doing so.
The few call sites that were passing a constant string literal
_shouldn't_ have been - they'll be all the better for adding a
strerror suffix to the message they were previously giving!
These are things where no fix was actually necessary in the code, but
the FIXME indicated that the comment itself was either in need of a
rewrite or removal.
While grepping for FIXME comments I could get rid of easily, I came
across a completely unexplained one in puttytel.rc, and after a moment
of thought, realised that it was there because PuTTYtel sharing
PuTTY's manifest file means the manifest has the wrong application
name.
Of course I could do something a bit more clever involving having one
copy of the manifest file and templating it to multiple applications,
but I think it would be more pain than it's worth given that the
templating system would have to be compatible with all the makefiles
and run on Windows systems where no sensible scripting was available.
So I just do it the trivial way.
It's never set to anything but NULL at any call site, and there's been
a FIXME comment in uxucs.c for ages saying it should be removed. I
think it only existed in the first place because it was a facility
supported by the underlying Windows API function and we couldn't see a
reason _not_ to pass it through. But I'm cleaning up FIXMEs, so we
should get rid of it.
(It stood for 'default used', incidentally - as in 'did the function
at any point have to make use of the parameter providing a default
fallback character?'. Nothing to do with _defusing_ things :-)
Ian Jackson points out that the Linux kernel has a macro of this name
with the same purpose, and suggests that it's a good idea to use the
same name as they do, so that at least some people reading one code
base might recognise it from the other.
I never really thought very hard about what order FROMFIELD's
parameters should go in, and therefore I'm pleasantly surprised to
find that my order agrees with the kernel's, so I don't have to
permute every call site as part of making this change :-)
I don't actually know why this was ever here; it appeared in the very
first commit that invented Plug in the first place (7b0e08270) without
explanation. Perhaps Dave's original idea was that sometimes you'd
need those macros _not_ to be defined so that the same names could be
reused as the methods for a particular Plug instance? But I don't
think that ever actually happened, and the code base builds just fine
with those macros defined unconditionally just like all the other sets
of method macros we now have, so let's get rid of this piece of cruft
that was apparently unnecessary all along.
I think that means that _every_ one of my traitoids is now a struct
containing a vtable pointer as one of its fields (albeit sometimes the
only field), and never just a bare pointer.
All the main backend structures - Ssh, Telnet, Pty, Serial etc - now
describe structure types themselves rather than pointers to them. The
same goes for the codebase-wide trait types Socket and Plug, and the
supporting types SockAddr and Pinger.
All those things that were typedefed as pointers are older types; the
newer ones have the explicit * at the point of use, because that's
what I now seem to be preferring. But whichever one of those is
better, inconsistently using a mixture of the two styles is worse, so
let's make everything consistent.
A few types are still implicitly pointers, such as Bignum and some of
the GSSAPI types; generally this is either because they have to be
void *, or because they're typedefed differently on different
platforms and aren't always pointers at all. Can't be helped. But I've
got rid of the main ones, at least.
It is useful to be able to exclude the header so that the log file
can be used for realtime input to other programs such as Kst for
plotting live data from sensors.
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_.
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 should make it easier to do formatted-string based logging
outside ssh.c, because I can wrap up a local macro in any source file
I like that expands to logevent_and_free(wherever my Frontend is,
dupprintf(macro argument)).
It caused yet another stub function to be needed in testbn, but there
we go.
(Also, while I'm here, removed a redundant declaration of logevent
itself from ssh.h. The one in putty.h is all we need.)
Most of these were 'void *' because they weren't even reliably a
structure type underneath - the per-OS storage systems would directly
cast read/write/enum settings handles to and from random things like
FILE *, Unix DIR *, or Windows HKEY. So I've wrapped them in tiny
structs for the sake of having a sensible structure tag visible
elsewhere in the code.
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 commit adds the new ids and fingerprints in the keys appendix of
the manual, and moves the old ones down into the historic-keys
section. I've tweaked a few pieces of wording for ongoing use, so that
they don't imply a specific number of past key rollovers.
The -pgpfp option in all the tools now shows the new Master Key
fingerprint and the previous (2015) one. I've adjusted all the uses of
the #defines in putty.h so that future rollovers should only have to
modify the #defines themselves.
Most importantly, sign.sh bakes in the ids of the current release and
snapshot keys, so that snapshots will automatically be signed with the
new snapshot key and the -r option will invoke the new release key.
I had previously left the platform field (in line 7 of the installer
database's SummaryInformation table) set at "x86" instead of any value
you might expect such as "Arm" or "Arm64", because I found that an MSI
file with either of the latter values was rejected by WoA's msiexec as
invalid.
It turns out this is because I _also_ needed to upgrade the installer
database schema version to a higher value than I even knew existed:
apparently the problem is that those platform fields aren't present in
the older schema. A test confirms that this works.
Unfortunately, WiX 3 doesn't actually know _how_ to write MSIs with
those platform values. But that's OK, because diffing the x86 and x64
MSIs against each other suggested that there were basically no other
changes in the database tables - so I can just generate the installer
as if for x64, and then rewrite that one field after installer
construction using GNOME msitools to take apart the binary file
structure and put it back together. (Those are the same tools I'm
using as part of my system for running WiX on Linux in the first
place.)
This commit introduces a script to do that post-hoc bodging, and calls
it from Buildscr. I've also changed over the choice of Program Files
folder for the Arm installers so that it's ProgramFiles64Folder
instead of ProgramFilesFolder - so now the Windows on Arm installer
doesn't incongruously default to installing in C:\Program Files (x86)!
I just spotted it while I was looking through this module anyway. It
was using %.100s to prevent an sprintf buffer overflow, which hasn't
been necessary since I switched everything over to dupprintf, and also
it was printing the integer value of GetLastError() without using my
convenient translation wrapper win_strerror.
I heard recently that at least one third-party client of Pageant
exists, and that it's used to generate signatures to use with TLS
client certificates. Apparently the signature scheme is compatible,
but TLS tends to need signatures over more data than will fit in
AGENT_MAX_MSGLEN.
Before the BinarySink refactor in commit b6cbad89f, this was OK
because the Windows Pageant IPC didn't check the size of the _input_
message against AGENT_MAX_MSGLEN, only the output one. But then we
started checking both, so that third-party TLS client started failing.
Now we use VirtualQuery to find out the actual size of the file
mapping we've been passed, and our only requirement is that the input
and output messages should both fit in _that_. So TLS should work
again, and also, other clients should be able to retrieve longer lists
of public keys if they pass a larger file mapping.
One side effect of this change is that Pageant's reply message is now
written directly into the shared-memory region. Previously, it was
written into a separate buffer and then memcpy()ed over after
pageant_handle_msg returned, but now the buffer is variable-size, it
seems to me to make more sense to avoid that extra not-entirely
controlled malloc. So I've done one very small reordering of
statements in the cross-platform pageant_handle_msg(), which fixes the
only case I could find where that function started writing its output
before it had finished using the contents of the input buffer.
Previously, the code to recover and memory-map the file-mapping object
Pageant uses for its IPC, and the code to convey its contents to and
from the cross-platform agent code, were widely separated, with the
former living in the WM_COPYDATA handler in the window procedure, and
the latter in answer_msg.
Now all of that code lives in answer_filemapping_message; WndProc only
handles the _window message_ contents - i.e. ensures the WM_COPYDATA
message has the right dwData id and that its lpData contains an ASCIZ
string - and answer_filemapping_message goes all the way from that
file-mapping object name to calling pageant_handle_msg.
While I'm here, I've also tidied up the code so that it uses the 'goto
cleanup' idiom rather than nesting everything inconveniently deeply,
and arranged that if anything goes wrong then we at least _construct_
an error message (although as yet we don't use that for anything
unless we're compiled with DEBUG_IPC enabled).
A few variables that gcc couldn't tell I'd initialised on all the
important paths, a variable that didn't really need to be there
anyway, and yet another use of GET_WINDOWS_FUNCTION_NO_TYPECHECK.
Rather than squelching the warning, I'm actually paying attention to
the deprecation, in that I'm allowing for the possibility that the
function might stop existing or stop returning success.
The specific thing that's strange about it is that it's _not_ an error
even though the compiler is quite justified in being suspicious about
it! The MS APIs define two different structures to have identical
formats.
After Pavel Kryukov pointed out that I have to put _something_ in the
'ssh_key' structure, I thought of an actually useful thing to put
there: why not make it store a pointer to the ssh_keyalg structure?
Then ssh_key becomes a classoid - or perhaps 'traitoid' is a closer
analogy - in the same style as Socket and Plug. And just like Socket
and Plug, I've also arranged a system of wrapper macros that avoid the
need to mention the 'object' whose method you're invoking twice at
each call site.
The new vtable pointer directly replaces an existing field of struct
ec_key (which was usable by several different ssh_keyalgs, so it
already had to store a pointer to the currently active one), and also
replaces the 'alg' field of the ssh2_userkey structure that wraps up a
cryptographic key with its comment field.
I've also taken the opportunity to clean things up a bit in general:
most of the methods now have new and clearer names (e.g. you'd never
know that 'newkey' made a public-only key while 'createkey' made a
public+private key pair unless you went and looked it up, but now
they're called 'new_pub' and 'new_priv' you might be in with a
chance), and I've completely removed the openssh_private_npieces field
after realising that it was duplicating information that is actually
_more_ conveniently obtained by calling the new_priv_openssh method
(formerly openssh_createkey) and throwing away the result.
It's an SSH-1 specific function, so it should have a name reflecting
that, and it didn't. Also it had one of those outdated APIs involving
passing it a client-allocated buffer and size. Now it has a sensible
name, and internally it constructs the output string using a strbuf
and returns it dynamically allocated.
Like the corresponding rewrite of conf serialisation, this affects not
just conf_deserialise itself but also the per-platform filename and
fontspec deserialisers.
I expected this to be nightmarish because WiX 3 doesn't know about the
Windows on Arm platform at all. Fortunately, it turns out that it
doesn't have to: testing on a borrowed machine I find that Windows on
Arm's msiexec.exe is quite happy to take MSIs whose platform field in
the _SummaryInformation table says "Intel".
In fact, that seemed to be _all_ that my test machine would accept: I
tried taking the MSI apart with msidump, putting some other value in
there (e.g. "Arm64" or "Arm") and rebuilding it with msibuild, and all
I got was messages from msiexec saying "This installation package is
not supported by this processor type."
So in fact I just give WiX the same -arch x86 option that I give it
for the real 32-bit x86 Windows installer, but then I point it at the
Arm binaries, and that seems to produce a viable MSI. There is the
unfortunate effect that msiexec forcibly sets the default install
location to 'Program Files (x86)' no matter how I strive to make it
set it any other way, but that's only cosmetic: the programs _run_
just fine no matter which Program Files directory they're installed
into (and I know this won't be the first piece of software that
installs itself into the wrong one). Perhaps some day we can find a
way to do that part better.
On general principles of caution (and of not really wanting to force
Arm machines to emulate x86 code at all), the Arm versions of the
installers have the new DllOk=no flag, so they're pure MSI with no
embedded DLLs.
This arranges that we can build a completely pure MSI file, which
doesn't depend on any native code at install time. We don't lose much
by doing this - only the option to pop up the README file at the end
of installation, and validation of the install directory when you
select it from a file browser.
My immediate use for this is that I want to use it for installers that
will run on Windows on Arm. But it also seems to me like quite an
attractive property in its own right - no native code at all running
at install time would be an _especially_ good guarantee that that code
can't be hijacked by DLLs in the download directory. So I may yet
decide that the features we're losing are not critical to _any_
version of the MSI, and throw them out unconditionally.
Apparently Windows on Arm has an emulator that lets it run x86
binaries without it being obvious, which could get confusing when
people start reporting what version of what they're running where.
(Indeed, it might get confusing for _me_ during early testing.) So now
the Windows builds explicitly state 'x86' or 'Arm' as well as 32- or
64-bit.
Now I've got FROMFIELD, I can rework it so that structures providing
an implementation of the Socket or Plug trait no longer have to have
the vtable pointer as the very first thing in the structure. In
particular, this means that the ProxySocket structure can now directly
implement _both_ the Socket and Plug traits, which is always
_logically_ how it's worked, but previously it had to be implemented
via two separate structs linked to each other.
During last week's work, I made a mistake in which I got the arguments
backwards in one of the key-blob-generating functions - mistakenly
swapped the 'void *' key instance with the 'BinarySink *' output
destination - and I didn't spot the mistake until run time, because in
C you can implicitly convert both to and from void * and so there was
no compile-time failure of type checking.
Now that I've introduced the FROMFIELD macro that downcasts a pointer
to one field of a structure to retrieve a pointer to the whole
structure, I think I might start using that more widely to indicate
this kind of polymorphic subtyping. So now all the public-key
functions in the struct ssh_signkey vtable handle their data instance
in the form of a pointer to a subfield of a new zero-sized structure
type 'ssh_key', which outside the key implementations indicates 'this
is some kind of key instance but it could be of any type'; they
downcast that pointer internally using FROMFIELD in place of the
previous ordinary C cast, and return one by returning &foo->sshk for
whatever foo they've just made up.
The sshk member is not at the beginning of the structure, which means
all those FROMFIELDs and &key->sshk are actually adding and
subtracting an offset. Of course I could have put the member at the
start anyway, but I had the idea that it's actually a feature _not_ to
have the two types start at the same address, because it means you
should notice earlier rather than later if you absentmindedly cast
from one to the other directly rather than by the approved method (in
particular, if you accidentally assign one through a void * and back
without even _noticing_ you perpetrated a cast). In particular, this
enforces that you can't sfree() the thing even once without realising
you should instead of called the right freekey function. (I found
several bugs by this method during initial testing, so I think it's
already proved its worth!)
While I'm here, I've also renamed the vtable structure ssh_signkey to
ssh_keyalg, because it was a confusing name anyway - it describes the
_algorithm_ for handling all keys of that type, not a specific key. So
ssh_keyalg is the collection of code, and ssh_key is one instance of
the data it handles.
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.
There was a time, back when the USA was more vigorously against
cryptography, when we toyed with the idea of having a version of PuTTY
that outsourced its cryptographic primitives to the Microsoft optional
encryption API, which would effectively create a tool that acted like
PuTTY proper on a system with that API installed, but automatically
degraded to being PuTTYtel on a system without, and meanwhile (so went
the theory) it could be moved freely across national borders with
crypto restrictions, because it didn't _contain_ any of the actual
crypto.
I don't recall that we ever got it working at all. And certainly the
vestiges of it here and there in the current code are completely
unworkable - they refer to an 'mscrypto.c' that doesn't even exist,
and the ifdefs in the definitions of structures like RSAKey and
MD5Context are not matched by any corresponding ifdefs in the code. So
I ought to have got round to removing it long ago, in order to avoid
misleading anyone.
This gets rid of yet another huge pile of beating around the bush with
length-counting. Also, this time, the BinarySink in question is a
little more interesting than just being a strbuf every time: on
Windows, where the shared-memory Pageant IPC system imposes a hard
limit on the size of message we can return, I've written a custom
BinarySink implementation that collects up to that much data and then
gives up and sets an overflow flag rather than continue to allocate
memory.
So the main Pageant code no longer has to worry about checking
AGENT_MAX_MSGLEN all the time - and better still, the Unix version of
Pageant is no longer _limited_ by AGENT_MAX_MSGLEN in its outgoing
messages, i.e. it could store a really extra large number of keys if
it needed to. That limitation is now a local feature of Windows
Pageant rather than intrinsic to the whole code base.
(AGENT_MAX_MSGLEN is still used to check incoming agent messages for
sanity, however. Mostly that's because I feel I ought to check them
against _some_ limit, and this one seems sensible enough. Incoming
agent messages are more bounded anyway - they generally don't hold
more than _one_ private key.)
This simplifies the client code both in ssh.c and in the client side
of Pageant.
I've cheated a tiny bit by preparing agent requests in a strbuf that
has space reserved at the front for the packet frame, which makes life
easier for the code that sends them off.
This affects all the functions that generate public and private key
and signature blobs of all kinds, plus ssh_ecdhkex_getpublic. Instead
of returning a bare block of memory and taking an extra 'int *length'
parameter, all these functions now write to a BinarySink, and it's the
caller's job to have prepared an appropriate one where they want the
output to go (usually a strbuf).
The main value of this change is that those blob-generation functions
were chock full of ad-hoc length-counting and data marshalling. You
have only to look at rsa2_{public,private}_blob, for example, to see
the kind of thing I was keen to get rid of!
Now instead of iterating through conf twice in separate functions,
once to count up the size of the serialised data and once to write it
out, I just go through once and dump it all in a strbuf.
(Of course, I could still do a two-pass count-then-allocate approach
easily enough in this system; nothing would stop me writing a
BinarySink implementation that didn't actually store any data and just
counted its size, and then I could choose at each call site whether I
preferred to do it that way.)
In fact, those functions don't even exist any more. The only way to
get data into a primitive hash state is via the new put_* system. Of
course, that means put_data() is a viable replacement for every
previous call to one of the per-hash update functions - but just
mechanically doing that would have missed the opportunity to simplify
a lot of the call sites.
This centralises a few things that multiple header files were
previously defining, and were protecting against each other's
redefinition with ifdefs - small things like structs and typedefs. Now
all those things are in a defs.h which is by definition safe to
include _first_ (out of all the codebase-local headers) and only need
to be defined once.
Lots of functions had really generic names (like 'makekey'), or names
that missed out an important concept (like 'rsakey_pubblob', which
loads a public blob from a _file_ and doesn't generate it from an
in-memory representation at all). Also, the opaque 'int order' that
distinguishes the two formats of public key blob is now a mnemonic
enumeration, and while I'm at it, rsa_ssh1_public_blob takes one of
those as an extra argument.
