I've just noticed that we call ssh1_bpp_start_compression even if the
server responded to our compression request with SSH1_SMSG_FAILURE!
Also, while I'm here, there's a potential race condition if the server
were to send an unrelated message (such as SSH1_MSG_IGNORE)
immediately after the SSH1_SMSG_SUCCESS that indicates compression
being enabled - the BPP would try to decode the compressed IGNORE
message before the SUCCESS got to the higher layer that would tell the
BPP it should have enabled compression. Fixed that by changing the
method by which we tell the BPP what's going on.
I've removed the encrypted_len fields from PktIn and PktOut, which
were used to communicate from the BPP to ssh.c how much each packet
contributed to the amount of data encrypted with a given set of cipher
keys. It seems more sensible to have the BPP itself keep that counter
- especially since only one of the three BPPs even needs to count it
at all. So now there's a new DataTransferStats structure which the BPP
updates, and ssh.c only needs to check it for overflow and reset the
limits.
Some upcoming restructuring I've got planned will need to pass output
packets back and forth on queues, as well as input ones. So here's a
change that arranges that we can have a PktInQueue and a PktOutQueue,
sharing most of their implementation via a PacketQueueBase structure
which links together the PacketQueueNode fields in the two packet
structures.
There's a tricksy bit of macro manoeuvring to get all of this
type-checked, so that I can't accidentally link a PktOut on to a
PktInQueue or vice versa. It works by having the main queue functions
wrapped by macros; when receiving a packet structure on input, they
type-check it against the queue structure and then automatically look
up its qnode field to pass to the underlying PacketQueueBase function;
on output, they translate a returned PacketQueueNode back to its
containing packet type by calling a 'get' function pointer.
Getting it out of the overgrown ssh.c is worthwhile in itself! But
there are other benefits of this reorganisation too.
One is that I get to remove ssh->current_incoming_data_fn, because now
_all_ incoming network data is handled by whatever the current BPP is.
So now we only indirect through the BPP, not through some other
preliminary function pointer _and_ the BPP.
Another is that all _outgoing_ network data is now handled centrally,
including our outgoing version string - which means that a hex dump of
that string now shows up in the raw-data log file, from which it was
previously conspicuous by its absence.
This was mildly fiddly because there's a single vtable structure that
implements two distinct interface types, one for compression and one
for decompression - and I have actually confused them before now
(commit d4304f1b7), so I think it's important to make them actually be
separate types!
This piece of tidying-up has come out particularly well in terms of
saving tedious repetition and boilerplate. I've managed to remove
three pointless methods from every MAC implementation by means of
writing them once centrally in terms of the implementation-specific
methods; another method (hmacmd5_sink) vanished because I was able to
make the interface type 'ssh2_mac' be directly usable as a BinarySink
by way of a new delegation system; and because all the method
implementations can now find their own vtable, I was even able to
merge a lot of keying and output functions that had previously
differed only in length parameters by having them look up the lengths
in whatever vtable they were passed.
This is more or less the same job as the SSH-1 case, only more
extensive, because we have a wider range of ciphers.
I'm a bit disappointed about the AES case, in particular, because I
feel as if it ought to have been possible to arrange to combine this
layer of vtable dispatch with the subsidiary one that selects between
hardware and software implementations of the underlying cipher. I may
come back later and have another try at that, in fact.
The interchangeable system of SSH-1 ciphers previously followed the
same pattern as the backends and the public-key algorithms, in that
all the clients would maintain two separate pointers, one to the
vtable and the other to the individual instance / context. Now I've
merged them, just as I did with those other two, so that you only cart
around a single pointer, which has a vtable pointer inside it and a
type distinguishing it from an instance of any of the other
interchangeable sets of algorithms.
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.
Now when we construct a packet containing sensitive data, we just set
a field saying '... and make it take up at least this much space, to
disguise its true size', and nothing in the rest of the system worries
about that flag until ssh2bpp.c acts on it.
Also, I've changed the strategy for doing the padding. Previously, we
were following the real packet with an SSH_MSG_IGNORE to make up the
size. But that was only a partial defence: it works OK against passive
traffic analysis, but an attacker proxying the TCP stream and
dribbling it out one byte at a time could still have found out the
size of the real packet by noting when the dribbled data provoked a
response. Now I put the SSH_MSG_IGNORE _first_, which should defeat
that attack.
But that in turn doesn't work when we're doing compression, because we
can't predict the compressed sizes accurately enough to make that
strategy sensible. Fortunately, compression provides an alternative
strategy anyway: if we've got zlib turned on when we send one of these
sensitive packets, then we can pad out the compressed zlib data as
much as we like by adding empty RFC1951 blocks (effectively chaining
ZLIB_PARTIAL_FLUSHes). So both strategies should now be dribble-proof.
sshbpp.h now defines a classoid that encapsulates both directions of
an SSH binary packet protocol - that is, a system for reading a
bufchain of incoming data and turning it into a stream of PktIn, and
another system for taking a PktOut and turning it into data on an
outgoing bufchain.
The state structure in each of those files contains everything that
used to be in the 'rdpkt2_state' structure and its friends, and also
quite a lot of bits and pieces like cipher and MAC states that used to
live in the main Ssh structure.
One minor effect of this layer separation is that I've had to extend
the packet dispatch table by one, because the BPP layer can no longer
directly trigger sending of SSH_MSG_UNIMPLEMENTED for a message too
short to have a type byte. Instead, I extend the PktIn type field to
use an out-of-range value to encode that, and the easiest way to make
that trigger an UNIMPLEMENTED message is to have the dispatch table
contain an entry for it.
(That's a system that may come in useful again - I was also wondering
about inventing a fake type code to indicate network EOF, so that that
could be propagated through the layers and be handled by whichever one
currently knew best how to respond.)
I've also moved the packet-censoring code into its own pair of files,
partly because I was going to want to do that anyway sooner or later,
and mostly because it's called from the BPP code, and the SSH-2
version in particular has to be called from both the main SSH-2 BPP
and the bare unencrypted protocol used for connection sharing. While I
was at it, I took the opportunity to merge the outgoing and incoming
censor functions, so that the parts that were common between them
(e.g. CHANNEL_DATA messages look the same in both directions) didn't
need to be repeated.
