This parameter returned a substring of the input, which was used for
two purposes. Firstly, it was used to hash the host and server keys
during the initial SSH-1 key setup phase; secondly, it was used to
check the keys in Pageant against the public key blob of a key
specified on the command line.
Unfortunately, those two purposes didn't agree! The first one needs
just the bare key modulus bytes (without even the SSH-1 mpint length
header); the second needs the entire key blob. So, actually, it seems
to have never worked in SSH-1 to say 'putty -i keyfile' and have PuTTY
find that key in Pageant and not have to ask for the passphrase to
decrypt the version on disk.
Fixed by removing that parameter completely, which simplifies all the
_other_ call sites, and replacing it by custom code in those two
places that each does the actually right thing.
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.
Another piece of half-finished machinery that I can't have tested
properly when I set up connection sharing: I had the function
ssh_alloc_sharing_rportfwd which is how sshshare.c asks ssh.c to start
sending it channel-open requests for a given remote forwarded port,
but I had no companion function that removes one of those requests
again when a downstream remote port forwarding goes away (either by
mid-session cancel-tcpip-forward or by the whole downstream
disconnecting).
As a result, the _second_ attempt to set up the same remote port
forwarding, after a sharing downstream had done so once and then
stopped, would quietly fail.
This is another bug that must have been around since connection
sharing was introduced, and nobody noticed until I did some unusually
thorough testing yesterday.
When a sharing downstream asks to set up a remote port forwarding, we
pass through the "tcpip-forward" global request, and we also intercept
the reply so that we know that the forwarding has been set up (and
hence that we should be passing "forwarded-tcpip" channel opens for
that port to this downstream). To do that, we set the want-reply flag
in the version of the packet we pass to the server, even if it was
clear in downstream's version; and we also put an item on a queue
local to sshshare.c which reminds us what to do about the reply when
it comes back.
But when the downstream _cancels_ one of those forwardings, I wrote
the code for all parts of that process except adding that queue item.
I even wrote the code to _consume_ the queue item, but somehow I
completely forgot to generate one in the first place! So the enum
value GLOBREQ_CANCEL_TCPIP_FORWARD was declared, tested for, but never
actually assigned to anything.
This must have been a bug introduced during the SSH-2 connection
sharing rework. Apparently nobody's ever re-tested SSH-1 X forwarding
since then - until I did so yesterday in the course of testing my
enormous refactor of the packet unmarshalling code.
I must have introduced this bug yesterday when I rewrote the packet
censoring functions using BinarySource. The base pointer passed to
log_packet was pointing at the right place, but the accompanying
length was the gross rather than net one, as it were - it counted the
extra header data we're about to insert at the _start_ of the packet,
so log_packet() was trying to print that many extra bytes at the _end_
and overrunning its buffer.
If the server sends an SSH_MSG_DISCONNECT, then we call
connection_fatal(). But if the server closes the network connection,
then we call connection_fatal(). In situations where the former
happens, the latter happens too.
Currently, calling connection_fatal twice is especially bad on GTK
because all dialogs are now non-modal and an assertion fails in the
GTK front end when two fatal message boxes try to exist at the same
time (the register_dialog system finds that slot is already occupied).
But regardless of that, we'd rather not even _try_ to print two fatal
boxes, because even if the front end doesn't fail an assertion,
there's no guarantee that the _more useful_ one of the messages will
end up being displayed. So a better fix is to have ssh.c make a
sensible decision about which message is the helpful one - in this
case, the actual error message out of the SSH_MSG_DISCONNECT, rather
than the predictable fact of the connection having been slammed shut
immediately afterwards - and only pass that one to the front end in
the first place.
There are several old functions that the previous commits have removed
all, or nearly all, of the references to. match_ssh_id is superseded
by ptrlen_eq_string; get_ssh_{string,uint32} is yet another replicated
set of decode functions (this time _partly_ centralised into misc.c);
the old APIs for the SSH-1 RSA decode functions are gone (together
with their last couple of holdout clients), as are
ssh{1,2}_{read,write}_bignum and ssh{1,2}_bignum_length.
Particularly odd was the use of ssh1_{read,write}_bignum in the SSH-2
Diffie-Hellman implementation. I'd completely forgotten I did that!
Now replaced with a raw bignum_from_bytes, which is simpler anyway.
I've also replaced the entire SOCKS state machine whose states were
barely-documented literal integers with one that uses an actual enum.
I think the result is a great deal clearer.
In the course of this rewrite I noticed that PuTTY's dynamic port
forwarding had never got round to supporting the SOCKS5 IPv6 address
format - though there was a FIXME comment saying it ought to. So now
it does: if a SOCKS5 client provides a binary IPv6 address (which
PuTTY's _own_ SOCKS5 client, in proxy.c, is quite capable of doing!),
then that will be translated into the usual IPv6 hex literal
representation to put in the "direct-tcpip" channel open request.
Quite a few of the function pointers in the ssh_keyalg vtable now take
ptrlen arguments in place of separate pointer and length pairs.
Meanwhile, the various key types' implementations of those functions
now work by initialising a BinarySource with the input ptrlen and
using the new decode functions to walk along it.
One exception is the openssh_createkey method which reads a private
key in the wire format used by OpenSSH's SSH-2 agent protocol, which
has to consume a prefix of a larger data stream, and tell the caller
how much of that data was the private key. That function now takes an
actual BinarySource, and passes that directly to the decode functions,
so that on return the caller finds that the BinarySource's read
pointer has been advanced exactly past the private key.
This let me throw away _several_ reimplementations of mpint-reading
functions, one in each of sshrsa, sshdss.c and sshecc.c. Worse still,
they didn't all have exactly the SSH-2 semantics, because the thing in
sshrsa.c whose name suggested it was an mpint-reading function
actually tolerated the wrong number of leading zero bytes, which it
had to be able to do to cope with the "ssh-rsa" signature format which
contains a thing that isn't quite an SSH-2 mpint. Now that deviation
is clearly commented!
Another set of localised decoding routines get thrown away here. Also,
I've changed the APIs of a couple of helper functions in x11fwd.c to
take ptrlens in place of zero-terminated C strings, because that's the
format in which they come back from the decode, and it saves mallocing
a zero-terminated version of each one just to pass to those helpers.
The OpenSSH PEM reader is the most interesting conversion out of
these: it was using a standalone function called get_ber_id_len(),
which only skipped over the header of an ASN.1 BER data item and left
the current position at the start of the payload. That's been replaced
by a get_ber() function more in the spirit of the new API, which
consumes the entire BER element, returning its header details and also
a ptrlen pointing at its payload.
(That function could easily be promoted out of import.c to somewhere
more central, if we ever had a need to handle ASN.1 on a larger scale
- e.g. X.509 certificates would find the same function useful. For the
moment, though, it can stay where it is.)
Other than that, this is a fairly mechanical API translation.
Like the corresponding rewrite of conf serialisation, this affects not
just conf_deserialise itself but also the per-platform filename and
fontspec deserialisers.
pageant_handle_msg was _particularly_ full of painful manual packet
decoding with error checks at every stage, so it's a great relief to
throw it all away and replace it with short sequences of calls to the
shiny new API!
This is the function that breaks apart a signature blob (generated
locally or received from an SSH agent) and adds leading zero bytes in
front of the signature integer, if we think we're talking to a server
that will incorrectly insist on that. The breaking-apart process is
just another instance of SSH-style data unmarshalling, so it should be
done by the new centralised routines.
The 'savedpos' field in 'struct Packet', which was already unused on
the output side after I threw away ssh_pkt_addstring_start, is now
unused on the input side too because a BinarySource implementation has
taken over. So it's now completely gone.
This is the first major piece of code converted to the new
unmarshalling system, and allows me to remove all the sftp_pkt_get*
functions in sftp.c that were previously duplicating standard decode
logic.
The SSH-1 RSA key reading functions now have BinarySource-shaped get_*
forms, although for the moment I'm still supporting the old API as a
wrapper on the new one, because I haven't switched over the client
code yet. Also, rsa_public_blob_len uses the new system internally,
although its API is unchanged.
This is the companion to the BinarySink system I introduced a couple
of weeks ago, and provides the same type-genericity which will let me
use the same get_* routines on an SSH packet, an SFTP packet or
anything else that chooses to include an implementing substructure.
However, unlike BinarySink which contained a (one-function) vtable,
BinarySource contains only mutable data fields - so another thing you
might very well want to do is to simply instantiate a bare one without
any containing object at all. I couldn't quite coerce C into letting
me use the same setup macro in both cases, so I've arranged a
BinarySource_INIT you can use on larger implementing objects and a
BinarySource_BARE_INIT you can use on a BinarySource not contained in
anything.
The API follows the general principle that even if decoding fails, the
decode functions will always return _some_ kind of value, with the
same dynamically-allocated-ness they would have used for a completely
successful value. But they also set an error flag in the BinarySource
which can be tested later. So instead of having to decode a 10-field
packet by means of 10 separate 'if (!get_foo(src)) throw error'
clauses, you can just write 10 'variable = get_foo(src)' statements
followed by a single check of get_err(src), and if the error check
fails, you have to do exactly the same set of frees you would have
after a successful decode.
This wraps up a (pointer, length) pair into a convenient struct that
lets me return it by value from a function, and also pass it through
to other functions in one go.
Ideally quite a lot of this code base could be switched over to using
ptrlen in place of separate pointer and length variables or function
parameters. (In fact, in my personal ideal conception of C, the usual
string type would be of this form, and all the string.h functions
would operate on ptrlens instead of zero-terminated 'char *'.)
For the moment, I'm just introducing it to make some upcoming
refactoring less inconvenient. Bulk migration of existing code to
ptrlen is a project for another time.
Along with the type itself, I've provided a convenient system of
including the contents of a ptrlen in a printf; a constructor function
that wraps up a pointer and length so you can make a ptrlen on the fly
in mid-expression; a function to compare a ptrlen against an ordinary
C string (which I mostly expect to use with string literals); and a
function 'mkstr' to make a dynamically allocated C string out of one.
That last function replaces a function of the same name in sftp.c,
which I'm promoting to a whole-codebase facility and adjusting its
API.
It's annoying that what ought to be a zero-cost type-safety measure
takes up space at run time, but it can't be helped - if it's undefined
behaviour then it's undefined behaviour.
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.
When our Windows make commands were serial, 'make all cleantestprogs'
was a nice shorthand for 'first build all the binaries, then delete
the ones we don't want to ship'. Now they're using -j, that doesn't
work so well - last night's snapshot build log shows that the command
'rm -f testbn.exe' from the cleantestprogs target happened _before_
the lld-link command that created testbn.exe in the first place, so
that file got shipped into the download directory by mistake.
Easily fixed, of course - just run two separate make commands per
build directory.
I had a one-off 'Strange packet received' error yesterday for an
SSH_MSG_GLOBAL_REQUEST at connection startup. I wasn't able to
reproduce, but examining the packet logs from the successful retry
suggested that the global request in question was an OpenSSH 'here are
my hostkeys' message.
My belief is that in the failing connection that request packet must
have arrived exceptionally promptly, and been added to pq_full before
the preceding SSH_MSG_USERAUTH_SUCCESS had been processed. So the code
that loops along pq_full feeding everything to the dispatch table
would have moved the USERAUTH_SUCCESS on to pq_ssh2_userauth and
scheduled a callback to handle it, and then moved on to the
GLOBAL_REQUEST which would have gone straight to the dispatch table
handler for 'help, we weren't expecting this message'. The userauth
callback would later on have installed a more sensible dispatch table
handler for global requests, but by then, it was too late.
Solution: make a special case during pq_full processing, so that when
we see USERAUTH_SUCCESS we _immediately_ - before continuing to
traverse pq_full - install the initial dispatch table entries for the
ssh-connection protocol. That way, even if connection messages are
already in the queue, we won't get confused by them.
This fixes an oversight in commit 0fc2d3b45: if a key creation
function returns a null 'ssh_key *', then adjusting the pointer's
address using FROMFIELD is a mistake, both in technical C terms
(undefined behaviour) and practically speaking because it will foil
the subsequent check against NULL. Instead, if we're going to check a
pointer against NULL, we must do it _before_ applying this kind of
address-adjusting type conversion.
Now we're building four rather than two sets of Windows binaries, the
build time has gone up rather painfully. I've just added a feature to
bob where it will invent a sensible value to use in 'make -j' and the
like, so let's start using it.
I build both 32- and 64-bit versions of the .exe files, code-sign
them, and create the same .zip file as I do for x86 Windows. I don't
yet have a method of building Arm MSI installers, though.
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 we don't have to worry about which windres we're using (or whether
another target architecture's windres will do just as well), this is
very easy - just test for a couple of extra values of $(Platform).
To build on Arm with VS2017 includes and libraries, various blog posts
and websites explain that you have to #define a cumbersome macro
called _ARM_WINAPI_PARTITION_DESKTOP_SDK_AVAILABLE, without which the
headers will #error at you. But if you do that, then everything seems
to compile fine and I actually tested it on an Arm Windows machine
today.
Also, I had to disable stack protection (/GS-), because clang-cl
doesn't yet support the particular form of it for which the VS2017 Arm
C library provides the runtime support. Unfortunate in a security
application, of course, but there we go.
Previously I was using clang-cl as my compiler, lld as my linker, and
GNU windres as my resource compiler, which made a confusing hybrid of
the LLVM and GNU toolchains. This was because llvm-rc had about four
missing features that stopped it being able to handle PuTTY's resource
files. (Some dialog control types; dialog class names; handling
preprocessor output without getting confused by line markers and
snippets of stray C; not complaining about the DISCARDABLE keyword.
Although admittedly I could have dealt with the last of those by just
removing it from my .rc files, because it's pointless anyway.)
In the past month, the llvm-rc developers have been hard at work, and
now it has _all_ those features! So now I can switch over to a purely
LLVM-based toolchain for my Windows builds, which is easier to set up
(and easier to tell other people how to set up, since they get it for
free with the rest of LLVM); doesn't have a nominal architecture
dependency (windres has to built against a particular flavour of
binutils); and produces bit-identical output to Visual Studio's
resource compiler as far as I can see (whereas windres is more in the
'close enough' area).
This needed a small makefile restructuring, because unlike windres,
llvm-rc doesn't have a built-in option to run the resource file
through the preprocessor. So now Makefile.clangcl has separate rules
to preprocess $tool.rc into $tool.rcpp and then compile the latter
into $tool.res.
In the course of reworking the socket vtable system, I noticed that
both sshshare.c and x11fwd.c independently invented the idea of a Plug
none of whose methods do anything. We don't need more than one of
those, so let's centralise the idea to somewhere it can be easily
reused.
It wasn't freeing the key comment along with the key data, probably
because I originally based the code on the SSH-1 analogue and forgot
that freersakey() *does* free the comment.
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.
In Friday's testing of the BinarySink work, I noticed that if you
accidentally add a mathematically invalid RSA1 key to Pageant, it will
accept it, getting into a state where it can fail assertions when
asked to use the key later. Added a call to rsa_verify(), triggering
an SSH_AGENT_FAILURE response if it doesn't agree the key is good.
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.
Similarly to commit 1c4f12252, this was one of those errors that C
can't catch because of the const-removing prototype of memchr. But
when you memchr() a const string, the value you get back is
_semantically_ a pointer to const, and should be assigned into a
variable of that type.
This removes a lot of pointless duplications of those constants.
Of course, _ideally_, I should upgrade to C99 bool throughout the code
base, replacing TRUE and FALSE with true and false and tagging
variables explicitly as bool when that's what they semantically are.
But that's a much bigger piece of work, and shouldn't block this
trivial cleanup!
