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.)
There's no reason to pass a 'void *' through and then cast it back to
a packet. All those functions are really doing is serialising a pile
of output on to an arbitrary receiver, so it's nicer to use the new
abstract BinarySink type, and then the do_mode function doesn't have
to cast it at all.
This also removes one lingering ugliness from the previous commit, in
the form of the variable 'stringstart' I introduced in ssh2_setup_pty
to work around the removal of the addstring_start system. Now that's
done the sensible way, by constructing the subsidiary terminal-modes
string in a strbuf (taking advantage of the new type-genericity
meaning I can pass that to the underlying functions just as easily as
a struct Packet), and then putting that into the final packet
afterwards.
All previous uses of them are replaced by the new put_* family.
This is a fairly mechanical transformation, which doesn't take full
advantage of the new ways things can be done more usefully. I'll come
back and clean parts of it up in separate patches later; muddling that
together with this main search-and-replace part would make (even more
of) a giant unreadable monolith.
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.
We can now simply call the centralised functions to put uint32s and
mpints into hash states, so there's no need to have duplicate local
copies doing the same things less type-generically.
I've finally got tired of all the code throughout PuTTY that repeats
the same logic about how to format the SSH binary primitives like
uint32, string, mpint. We've got reasonably organised code in ssh.c
that appends things like that to 'struct Packet'; something similar in
sftp.c which repeats a lot of the work; utility functions in various
places to format an mpint to feed to one or another hash function; and
no end of totally ad-hoc stuff in functions like public key blob
formatters which actually have to _count up_ the size of data
painstakingly, then malloc exactly that much and mess about with
PUT_32BIT.
It's time to bring all of that into one place, and stop repeating
myself in error-prone ways everywhere. The new marshal.h defines a
system in which I centralise all the actual marshalling functions, and
then layer a touch of C macro trickery on top to allow me to (look as
if I) pass a wide range of different types to those functions, as long
as the target type has been set up in the right way to have a write()
function.
This commit adds the new header and source file, and sets up some
general centralised types (strbuf and the various hash-function
contexts like SHA_State), but doesn't use the new calls for anything
yet.
(I've also renamed some internal functions in import.c which were
using the same names that I've just defined macros over. That won't
last long - those functions are going to go away soon, so the changed
names are strictly temporary.)
Now, instead of being a black box that you shovel strings into and
eventually extract a final answer, it exposes enough structure fields
to the world that you can append things to it _and_ look inside its
current contents. For convenience, it exports its internal pointer as
both a char * and an unsigned char *.
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.
In the SSH1_AGENTC_ADD_RSA_IDENTITY message, the multiplicative
inverse integer is the inverse of the first prime mod the second one.
In our notation, that means we should send iqmp, then q, then p, which
is also how the Pageant server side expects to receive them.
Unfortunately, we were sending iqmp, p, q instead, which I think must
be a confusion resulting from the SSH 1.5 document naming the primes
the other way round (and talking about the auxiliary value 'inverse of
p mod q').
If we're called on to generate the one-line OpenSSH public key format
for a key that we don't have a comment field for, we were mistakenly
testing this by checking if '*comment' rather than 'comment' was zero,
i.e. if comment was NULL we'd dereference it by mistake.
If name resolution fails, pfd_connect tries to sfree(dummy_realhost)
when it's completely uninitialised - the failed resolution didn't
write to it, and we also didn't precautionarily initialise it to NULL
first. Now we do the latter.
I never noticed before because it only comes up in the case of an
agent sending back one particular kind of corrupt data, but if the
last-minute check that there's no trailing junk on the end of the
agent's SSH-2 key list fails, it prints an error message erroneously
mentioning SSH-1.
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.
If you forget the '+' at the start of a continuation line with only
one word on it, then Perl would test $_[1] before checking that it
even existed to test. The test would give the right answer anyway, but
the warning was annoying.
Added a missing pq_push_front (see commit a41eefff4), without which
the SSH_MSG_USERAUTH_FAILURE was being dropped from the userauth
packet queue before returning to the top of the loop which waits for
it to arrive.
This seems to be a knock-on effect of my recent reworking of the SSH
code to be based around queues and callbacks. The loop iteration
function in uxsftp.c (ssh_sftp_do_select) would keep going round its
select loop until something had happened on one of its file
descriptors, and then return to the caller in the assumption that the
resulting data might have triggered whatever condition the caller was
waiting for - and if not, then the caller checks, finds nothing
interesting has happened, and resumes looping with no harm done.
But now, when something happens on an fd, it doesn't _synchronously_
trigger the follow-up condition PSFTP was waiting for (which, at
startup time, happens to be back->sendok() starting to return TRUE).
Instead, it schedules a callback, which will schedule a callback,
which ... ends up setting that flag. But by that time, the loop
function has already returned, the caller has found nothing
interesting and resumed looping, and _now_ the interesting thing
happens but it's too late because ssh_sftp_do_select will wait until
the next file descriptor activity before it next returns.
Solution: give run_toplevel_callbacks a return value which says
whether it's actually done something, and if so, return immediately in
case that was the droid the caller was looking for. As it were.
In commit 528513dde I absentmindedly replaced a write to the local
variable 'need_size' of drawing_area_setup with a write to
inst->drawing_area_setup_needed, imagining that they had the same
effect. But actually, need_size was doing two jobs and I only replaced
one of them: it was also the variable that indicated that the logical
terminal size had changed and so we had to call term_size() to make
the terminal.c data structures resize themselves appropriately. The
loss of that call also inhibited generation of SIGWINCH.
This causes the previous graphic character to be displayed another Pn
times (defaulting to 1, as usual). I just found out about it because
Ubuntu 18.04's ncurses expects it to be honoured.
According to all-escapes, REP is only supposed to be used when the
thing immediately preceding it in the terminal data stream _is_ a
printing character, and if not, then the behaviour is undefined. But
'undefined' is good enough for me to do the simple thing of just
remembering the last graphic character no matter whether anything else
has intervened since then.
To avoid DoS attacks using this escape sequence with a really huge Pn,
I clamp the value at the total size of the screen. There might be ways
to do that with more finesse (e.g. reduce it mod the width so that the
screen ends up looking the way it should even for huge parameters, or
reduce it even further if we notice the terminal isn't in wrapping
modes), but this will do for now.
I'm about to want to implement an escape sequence that causes a
graphic character to be printed, which means I'll need the code that
does so to be in a separate routine that I can call easily, instead of
buried a few loops deep in the middle of the main state machine.
Another piece of fallout from this morning's patch series, which I
didn't notice until I left a session running for more than an hour:
once do_ssh2_transport is told to begin a rekey, it has no way of
knowing _not_ to immediately do another one, and another, and so on.
Added a value RK_NONE to the rekey class enumeration, and set
rekey_class to that immediately after a key exchange completes. Then a
new one won't start until some code actually sets rekey_class to a
nonzero value again.
Returning from the coroutine every time we finish putting together a
packet is wrong, because it means that any further data in the
incoming queue won't get processed until something triggers another
call to the coroutine. In ssh2_rdpkt I got this right - the only
crReturn that _isn't_ due to running out of data is the special one
immediately after a NEWKEYS - but I forgot to fix it the same way in
ssh1_rdpkt.
ssh->current_user_input_fn was not set to NULL when the ssh structure
was initially set up, which meant that with sufficiently eager
typeahead it could accidentally be called while still full of garbage.
And ssh->connshare is freed in more than one place (by ssh_free and
also by do_ssh_close), but only one of those places nulls it out to
stop the other one trying to free it a second time.
This fixes further embarrassing hangs in the wake of the big
restructuring, in which we'd assign a new function pointer into
ssh->current_user_input_fn but not immediately call it to let it
process whatever might already be waiting for it in the user input
queue.
A large part of the purpose of the 20-patch series just past is to
arrange that user input is never dropped on the floor: if you type it
at a moment where the active protocol coroutine has nothing it can
usefully do with it, the default action will now be to leave it on the
user_input queue where it will eventually be picked up by some later
coroutine, or later phase of this one, that does.
And did I _test_ this feature, end to end, just once, before pushing
the giant patch series?
I did not.
It doesn't work, and the reason why it doesn't work is because various
loops that spin round alternating a crReturn with a check of the input
queue do the first crReturn _before_ the first queue check. So if
there's already data in the queue, well, it won't be _dropped_, but it
also won't be passed on immediately to where it needs to be - instead,
it will sit in the queue until you press _another_ key, at which point
a queue check will happen and all your backed-up typeahead data will
come out.
Fixed by restructuring those loops to do a queue check first. This
applies to the final loop in do_ssh2_connection, and all the little
loops during userauth that prompt for usernames, passwords,
passphrases etc via get_userpass_input.
do_ssh2_authconn is no more! In its place are two separate functions
do_ssh2_userauth and do_ssh2_connection, each handling just one of the
three main protocol components of SSH-2, and each with its own
separate packet queue feeding from pq_full via the dispatch table.
This should be entirely NFC: no bugs are fixed as a side effect of
this change. It's mostly done in the hope that further refactoring in
the future might finally be able to split up ssh.c into smaller source
files, for which it's useful to have these two routines separate,
since each one will be talking to a completely different set of
supporting machinery.
This is not as trivial as it sounds, because although
do_ssh2_transport never uses user input, its in/inlen parameter pair
were nonetheless actually doing something, because by setting inlen to
special values -1 or -2 they doubled as a way to initiate a rekey with
a stated textual reason. So _that_ use of in and inlen has to be
replaced. (Good thing too - that was a horribly ugly API!)
I've replaced them with a new pair of fields in the main ssh
structure, a textual "rekey_reason" for the Event Log and an
enumeration "rekey_class" for discrimination within the code. In
particular, that allows me to get rid of the ugly strcmp that was
checking the textual rekey reason to find out whether the rekey was
due to a GSSAPI credentials update - now there's a separate enum value
for that kind of rekey and we can test for that more sensibly.
Even with my love of verbose commit messages, I don't think there's
anything interesting to say about this commit that the previous few
similar ones haven't mentioned already. This is just more of the same
transformations.
The connection protocol didn't have to do anything too complicated
with user input - just remember to check for it and turn it into
SSH1_CMSG_STDIN_DATA - so this is a much less involved change than the
previous user input conversions.
The userauth loop has always had a rather awkward feature whereby some
of its branches have _already_ received an SSH_MSG_USERAUTH_SUCCESS or
SSH_MSG_USERAUTH_FAILURE packet (e.g. because they have to wait for a
packet that might be one of those _or_ a continuation packet of some
kind), whereas other branches go back round to the top of the loop at
the moment that they know one of those will be the next packet to
arrive. So we had a flag 's->gotit' which tells the start of the loop
whether it's already sitting on the success or failure message, or
whether the first thing it should do is to crWait for one.
Now that the packets are coming from a linked list, there's a simpler
way to handle this: the top of the userauth loop _always_ expects to
find a success/failure message as the first thing in the queue, and if
any branch of the auth code finds it's already removed such a message
from the queue, it can simply put it back on the front again before
going back round.
Just like do_ssh1_login, do_ssh2_authconn is now no longer receiving
its packets via function arguments; instead, it's pulling them off a
dedicated PacketQueue populated by particular dispatch table entries,
with the same reference-counting system to stop them being freed when
the pq_full processing function regains control.
This eliminates further ugly bombout()s from the code while waiting
for authentication responses from the user or the SSH agent.
As I mentioned in the previous commit, I've had to keep
ssh2_authconn_input separate from do_ssh2_authconn, and this time the
reason why is thoroughly annoying: it's because do_ssh2_authconn has
changed its prototype to take the Ssh structure as a void * so that it
can be called from the idempotent callback system, whereas
ssh2_authconn_input has to have the type of ssh->current_user_input_fn
which takes the more sensible pointer type 'Ssh'.
Just as I did to do_ssh1_login, I'm removing the 'in' and 'inlen'
parameters from the combined SSH-2 userauth + connection coroutine, in
favour of it reading directly from ssh->user_input, and in particular
also allowing get_userpass_input to do so on its behalf.
Similarly to the previous case, I've completely emptied the wrapper
function ssh2_authconn_input(), and again, there is a reason I can't
quite get away with removing it...
This introduces the first of several filtered PacketQueues that
receive subsets of pq_full destined for a particular coroutine. The
wrapper function ssh1_coro_wrapper_initial, whose purpose I just
removed in the previous commit, now gains the replacement purpose of
accepting a packet as a function argument and putting it on the new
queue for do_ssh1_login to handle when it's ready. That wrapper in
turn is called from the packet-type dispatch table, meaning that the
new pq_ssh1_login will be filtered down to only the packets destined
for this coroutine.
This is the point where I finally start using the reference counting
system that I added to 'struct Packet' about a dozen commits ago. The
general packet handling calls ssh_unref_packet for everything that
it's just pulled off pq_full and handed to a dispatch-table function -
so _this_ dispatch-table function, which needs the packet not to be
freed because it's going to go on to another queue and wait to be
handled there, can arrange that by incrementing its ref count.
This completes the transformation of do_ssh1_login into a function
with a trivial argument list, whose job is to read from a pair of
input queues (one for user keyboard input and one for SSH packets) and
respond by taking action directly rather than returning a value to its
caller to request action.
It also lets me get rid of a big pile of those really annoying
bombout() calls that I used to work around the old coroutine system's
inability to deal with receiving an SSH packet when the control flow
was in the middle of waiting for some other kind of input. That was
always the weakness of the coroutine structure of this code, which I
accepted as the price for the convenience of coroutines the rest of
the time - but now I think I've got the benefits without that cost :-)
The one remaining argument to do_ssh1_login is the main Ssh structure,
although I've had to turn it into a void * to make the function's type
compatible with the idempotent callback mechanism, since that will be
calling do_ssh1_login directly when its input queue needs looking at.
Now it does its post-completion work itself instead of telling the
callee to do the same. So its caller, ssh1_coro_wrapper_initial, is
now a _completely_ trivial wrapper - but I'm not taking the
opportunity to fold the two functions together completely, because the
wrapper is going to acquire a new purpose in the next commit :-)
This is the first refactoring of a major coroutine enabled by adding
the ssh->user_input queue. Now, instead of receiving a fixed block of
parameter data, do_ssh1_login reads directly from the user input
bufchain.
In particular, I can get rid of all the temporary bufchains I
constructed to pass to get_userpass_input (or rather, the ones in this
particular function), because now we can let get_userpass_input read
directly from the main user_input bufchain, and it will read only as
much data as it has an interest in, and leave the rest as type-ahead
for future prompts or the main session.
After the last few commits, neither incoming SSH packets nor incoming
user input goes through those functions any more - each of those
directions of data goes into a queue and from there to a callback
specifically processing that queue. So the centralised top-level
protocol switching functions have nothing left to switch, and can go.
This change introduces a new bufchain ssh->user_input, into which we
put every byte received via back->send() (i.e. keyboard input from the
GUI PuTTY window, data read by Plink from standard input, or outgoing
SCP/SFTP protocol data made up by the file transfer utilities).
Just like ssh->incoming_data, there's also a function pointer
ssh->current_user_input_fn which says who currently has responsibility
for pulling data back off that bufchain and processing it. So that can
be changed over when the connection moves into a different major phase.
At the moment, nothing very interesting is being done with this
bufchain: each phase of the connection has its own little function
that pulls chunks back out of it with bufchain_prefix and passes them
to the unchanged main protocol coroutines. But this is groundwork for
being able to switch over each of those coroutines in turn to read
directly from ssh->user_input, with the aim of eliminating a
collection of annoying bugs in which typed-ahead data is accidentally
discarded at an SSH phase transition.
This flag was used to indicate that ssh1_protocol (or, as of the
previous commit, ssh1_coro_wrapper) should stop passing packets to
do_ssh1_login and start passing them to do_ssh1_connection.
Now, instead of using a flag, we simply have two separate versions of
ssh1_coro_wrapper for the two phases, and indicate the change by
rewriting all the entries in the dispatch table. So now we _just_ have
a function-pointer dereference per packet, rather than one of those
and then a flag check.
After the previous two refactorings, there's no longer any need to
pass packets to ssh1_protocol or ssh2_protocol so that each one can do
its own thing with them, because now the handling is the same in both
cases: first call the general type-independent packet processing code
(if any), and then call the dispatch table entry for the packet type
(which now always exists).
In SSH-2, every possible packet type code has a non-NULL entry in the
dispatch table, even if most of them are just ssh2_msg_unimplemented.
In SSH-1, some dispatch table entries are NULL, which means that the
code processing the dispatch table has to have some SSH-1 specific
fallback logic.
Now I've put the fallback logic in a separate function, and replaced
the NULL table entries with pointers to that function, so that another
pointless difference between the SSH-1 and SSH-2 code is removed.
NFC: I'm just moving a small piece of code out into a separate
function, which does processing on incoming SSH-2 packets that is
completely independent of the packet type. (Specifically, we count up
the total amount of data so far transferred, and use it to trigger a
rekey when we get over the per-session-key data limit.)
The aim is that I'll be able to call this function from a central
location that's not SSH-2 specific, by using a function pointer that
points to this function in SSH-2 mode or is null in SSH-1 mode.
I've completely removed the top-level coroutine ssh_gotdata(), and
replaced it with a system in which ssh_receive (which is a plug
function, i.e. called directly from the network code) simply adds the
incoming data to a new bufchain called ssh->incoming_data, and then
queues an idempotent callback to ensure that whatever function is
currently responsible for the top-level handling of wire data will be
invoked in the near future.
So the decisions that ssh_gotdata was previously making are now made
by changing which function is invoked by that idempotent callback:
when we finish doing SSH greeting exchange and move on to the packet-
structured main phase of the protocol, we just change
ssh->current_incoming_data_fn and ensure that the new function gets
called to take over anything still outstanding in the queue.
This simplifies the _other_ end of the API of the rdpkt functions. In
the previous commit, they stopped returning their 'struct Packet'
directly, and instead put it on a queue; in this commit, they're no
longer receiving a (data, length) pair in their parameter list, and
instead, they're just reading from ssh->incoming_data. So now, API-
wise, they take no arguments at all except the main 'ssh' state
structure.
It's not just the rdpkt functions that needed to change, of course.
The SSH greeting handlers have also had to switch to reading from
ssh->incoming_data, and are quite substantially rewritten as a result.
(I think they look simpler in the new style, personally.)
This new bufchain takes over from the previous queued_incoming_data,
which was only used at all in cases where we throttled the entire SSH
connection. Now, data is unconditionally left on the new bufchain
whether we're throttled or not, and the only question is whether we're
currently bothering to read it; so all the special-purpose code to
read data from a bufchain and pass it to rdpkt can go away, because
rdpkt itself already knows how to do that job.
One slightly fiddly point is that we now have to defer processing of
EOF from the SSH server: if we have data already in the incoming
bufchain and then the server slams the connection shut, we want to
process the data we've got _before_ reacting to the remote EOF, just
in case that data gives us some reason to change our mind about how we
react to the EOF, or a last-minute important piece of data we might
need to log.
Each of the coroutines that parses the incoming wire data into a
stream of 'struct Packet' now delivers those packets to a PacketQueue
called ssh->pq_full (containing the full, unfiltered stream of all
packets received on the SSH connection), replacing the old API in
which each coroutine would directly return a 'struct Packet *' to its
caller, or NULL if it didn't have one ready yet.
This simplifies the function-call API of the rdpkt coroutines (they
now return void). It increases the complexity at the other end,
because we've now got a function ssh_process_pq_full (scheduled as an
idempotent callback whenever rdpkt appends anything to the queue)
which pulls things out of the queue and passes them to ssh->protocol.
But that's only a temporary complexity increase; by the time I finish
the upcoming stream of refactorings, there won't be two chained
functions there any more.
One small workaround I had to add in this commit is a flag called
'pending_newkeys', which ssh2_rdpkt sets when it's just returned an
SSH_MSG_NEWKEYS packet, and then waits for the transport layer to
process the NEWKEYS and set up the new encryption context before
processing any more wire data. This wasn't necessary before, because
the old architecture was naturally synchronous - ssh2_rdpkt would
return a NEWKEYS, which would be immediately passed to
do_ssh2_transport, which would finish processing it immediately, and
by the time ssh2_rdpkt was next called, the keys would already be in
place.
This change adds a big while loop around the whole of each rdpkt
function, so it's easiest to read it as a whitespace-ignored diff.
NFC for the moment, because the bufchain is always specially
constructed to hold exactly the same data that would have been passed
in to the function as a (pointer,length) pair. But this API change
allows get_userpass_input to express the idea that it consumed some
but not all of the data in the bufchain, which means that later on
I'll be able to point the same function at a longer-lived bufchain
containing the full stream of keyboard input and avoid dropping
keystrokes that arrive too quickly after the end of an interactive
password prompt.
The crWaitUntil macros have do-while type semantics, i.e. they always
crReturn _at least_ once, and then perhaps more times if their
termination condition is still not met. But sometimes a coroutine will
want to wait for a condition that may _already_ be true - the key
examples being non-emptiness of a bufchain or a PacketQueue, which may
already be non-empty in spite of you having just removed something
from its head.
In that situation, it's obviously more convenient not to bother with a
crReturn in the first place than to do one anyway and have to fiddle
about with toplevel callbacks to make sure we resume later. So here's
a new pair of macros crMaybeWaitUntil{,V}, which have the semantics of
while rather than do-while, i.e. they test the condition _first_ and
don't return at all if it's already met.
This is a set of convenience wrappers around the existing toplevel
callback function, which arranges to avoid scheduling a second call to
a callback function if one is already in the queue.
Just like the last few commits, this is a piece of infrastructure that
nothing is yet using. But it will.
This is another piece of not-yet-used infrastructure, which later on
will simplify my life when I start processing PacketQueues and adding
some of their packets to other PacketQueues, because this way the code
can unref every packet removed from the source queue in the same way,
whether or not the packet is actually finished with.
bufchain_fetch_consume is a one-stop function that moves a given
number of bytes out of the head of a bufchain into an output buffer,
removing them from the bufchain in the process.
That function will fail an assertion (just like bufchain_fetch) if the
bufchain doesn't actually _have_ at least that many bytes to read, so
I also provide bufchain_try_fetch_consume which will return a success
or failure status.
Nothing uses these functions yet, but they will.
Simon Tatham
Jacob Nevins