solved unused variable error when KEY_EVENT_DIAGNOSTICS defined but
DEBUG not defined
although we intend to always define DEBUG when KEY_EVENT_DIAGNOSTICS
is going to be defined.
The idea of the especially large RNG that we use in key generation is
that it should have as much actual entropy as possible. The reason I
based it on SHA-512 previously was that that was the hash function in
our collection with the largest output. But that's no longer true!
Among the SHA-3 family that I added for Ed448 purposes, we have a
ready-made variant of SHAKE-256 that outputs a whopping 114 bytes of
hash. I see no reason not to upgrade to that from SHA-512's 64 bytes.
(I could probably extend it even further by manually making another
SHA-3 variant specially for the purpose, but I don't know that it
would be worth it. This is a one-line change which I think is already
a positive step.)
A user mentioned having found this confusing recently, and fair
enough, because it's done in a way that doesn't quite match the
built-in OO system of any language I know about. But after the
rewriting in recent years, I think pretty much everything in PuTTY
that has a system of interchangeable implementations of the same
abstract type is now done basically the same way, so this seems like a
good moment to document the idiom we use and explain all its ins and
outs.
It referred to the global variable 'flags' as an example. But 'flags'
was retired (and good riddance) nearly a year ago, in commit
4ea811a0bf. So we should be using a different example now!
sysctlbyname() turns out to be a new library function, so we can't
assume it's present just because defined __APPLE__. Add an autoconf
check to see if it's really there, before trying to call it.
An obvious goof - in SHA-384, you don't want to write out the last of
the four state vectors! Fortunately I spotted it only a couple of
hours after introducing it.
MD5 is structurally very similar to all the SHA-1 and SHA-2 hashes
(with the main difference being that the message schedule consists of
just repeating the 32-bit words of the message four times in different
permutations, instead of transforming them via an LFSR-style process).
So it helps legibility and maintainability if all the implementations
of these hashes are coded in a similar style - for example, that way,
the next time I need to make a change to the ssh_hash API, I can do it
the same way in all these modules without having to think everything
out again.
After the SHA-512 rewrite earlier today, all the hashes in that family
had been updated to a consistent new style as a side effect of adding
optional hardware acceleration, except for MD5, because there's no
hardware-accelerated version of it. (And not much chance of anyone
ever needing one, I hope!)
So this is a purely stylistic update which reworks MD5 so that it
looks just like all the SHA-1 and SHA-2 hash implementations. No
functional change.
The NEON support for SHA-512 acceleration looks very like SHA-256,
with a pair of chained instructions to generate a 128-bit vector
register full of message schedule, and another pair to update the hash
state based on those. But since SHA-512 is twice as big in all
dimensions, those four instructions between them only account for two
rounds of it, in place of four rounds of SHA-256.
Also, it's a tighter squeeze to fit all the data needed by those
instructions into their limited number of register operands. The NEON
SHA-256 implementation was able to keep its hash state and message
schedule stored as 128-bit vectors and then pass combinations of those
vectors directly to the instructions that did the work; for SHA-512,
in several places you have to make one of the input operands to the
main instruction by combining two halves of different vectors from
your existing state. But that operation is a quick single EXT
instruction, so no trouble.
The only other problem I've found is that clang - in particular the
version on M1 macOS, but as far as I can tell, even on current trunk -
doesn't seem to implement the NEON intrinsics for the SHA-512
extension. So I had to bodge my own versions with inline assembler in
order to get my implementation to compile under clang. Hopefully at
some point in the future the gap might be filled and I can relegate
that to a backwards-compatibility hack!
This commit adds the same kind of switching mechanism for SHA-512 that
we already had for SHA-256, SHA-1 and AES, and as with all of those,
plumbs it through to testcrypt so that you can explicitly ask for the
hardware or software version of SHA-512. So the test suite can run the
standard test vectors against both implementations in turn.
On M1 macOS, I'm testing at run time for the presence of SHA-512 by
checking a sysctl setting. You can perform the same test on the
command line by running "sysctl hw.optional.armv8_2_sha512".
As far as I can tell, on Windows there is not yet any flag to test for
this CPU feature, so for the moment, the new accelerated SHA-512 is
turned off unconditionally on Windows.
This builds on the previous refactoring by reworking the SHA-512
vtables and block layer to look more like the SHA-256 version, in
which the block and padding structure is a subroutine of the top-level
vtable methods instead of an owning layer around them.
This also organises the code in a way that makes it easy to drop in
hardware-accelerated versions alongside it: the block layer and the
big arrays of constants are now nicely separate from the inner
block-transform part.
It was written in an awkward roundabout way involving all the
arithmetic being done in horrible macros looking like assembler
instructions, and lots of explicit temp variables. That's because,
when I originally wrote it, I needed it to compile on platforms
without a 64-bit integer type.
In commit a647f2ba11 I switched it over to using uint64_t, but I did
it in a way that made minimal change to the code structure, by
rewriting the insides of those macros to contain ordinary uint64_t
arithmetic instead of faffing about with 32-bit halves. So it worked,
but it still looked disgusting.
Now I've reworked it so that individual arithmetic operations are
written directly in the sensible way, and the more complicated
SHA-specific operations are written as inline functions instead of
macros.
The M1 chip in the new range of Macs includes the crypto extension
that permits AES, SHA-1 and SHA-256 acceleration. But you can't find
that out by querying the ELF aux vector, because macOS isn't even
ELF-based at all, so there isn't an ELF aux vector, and no web search
I've tried has turned up any MachO thing obviously analogous to it.
Running 'sysctl -a' does show some flags indicating CPU architecture
extensions, but they're more advanced ones than this. So I think we
have to assume that if we're on the new M1 macOS at all, then we have
the basic crypto extension available.
Accordingly, I've added a special case to all the query functions that
simply returns true if defined __APPLE__.
If the autoconf/ifdef system ends up taking the trivial branch through
all the Arm-architecture ifdefs, then we define the always-fail
version of getauxval as a 'static inline' function, and then (because
none of our desired HWCAP_FOO values is defined at all) never call it.
This leads to a compiler warning because we defined a static function
and never called it - i.e. at the default -Werror, a build failure.
Of course it's perfectly sensible to define a static inline function
that never gets called! Header files do it all the time, and nobody is
expected to ensure that if they include a header file then they take
care to refer to every static inline function it defines.
But if the definition is in the _source_ file rather than a header
file, then clang (in particular on macOS) will give a warning. So the
easy solution is to move the inline definitions of getauxval into a
header file, which suppresses the warning without requiring me to faff
about with further ifdefs to make the definitions conditional on at
least one use.
With the new --open-unconditional-agent-socket option, every time
Uppity receives an SSH connection, it will immediately open a Unix-
domain socket and attempt to do agent forwarding on it, in the sense
that any connection to that socket will be turned into an
"auth-agent@openssh.com" CHANNEL_OPEN request on whichever SSH
connection it was associated with.
That connection-global socket is independent of any that are created
as part of setting up a session channel. The pathname of the socket
file is written to the server's event log (there being no other
sensible place to send it).
The aim is that this allows me to test the behaviour of an SSH client
if the server tries to open an agent-forwarding channel outside the
usual context. In particular, it allows me to test the change I just
made in the previous commit, that if you enable agent forwarding in
the client configuration, then auth-agent channels opened by the
server are accepted even if no session channel opened by the client
has sent an auth-agent-req. More importantly, it allows me to check
that I _haven't_ accidentally arranged that those channels are
accepted even when agent forwarding is _not_ permitted by the client
configuration!
Implementation details: the agent forwarding socket was previously
implemented as part of the internal sesschan structure. I've moved it
out into a little sub-struct of its own which can be created
independently of a sesschan.
Previously, the instant at which we send to the server a request to
enable agent forwarding (the "auth-agent-req@openssh.com" channel
request, or SSH1_CMSG_AGENT_REQUEST_FORWARDING) was also the instant
at which we set a flag indicating that we're prepared to accept
attempts from the server to open a channel to talk to the forwarded
agent. If the server attempts that when we haven't sent a forwarding
request, we treat it with suspicion, and reject it.
But it turns out that at least one SSH server does this, for what
seems to be a _somewhat_ sensible purpose, and OpenSSH accepts it. So,
on the basis that the @openssh.com domain suffix makes them the
arbiters of this part of the spec, I'm following their practice. I've
removed the 'agent_fwd_enabled' flag from both connection layer
implementations, together with the ConnectionLayer method that sets
it; now agent-forwarding CHANNEL_OPENs are gated only on the questions
of whether agent forwarding was permitted in the configuration and
whether an agent actually exists to talk to, and not also whether we
had previously sent a message to the server announcing it.
(The change to this condition is also applied in the SSH-1 agent
forwarding code, mostly for the sake of keeping things parallel where
possible. I think it doesn't actually make a difference in SSH-1,
because in SSH-1, it's not _possible_ for the server to try to open an
agent channel before the main channel is set up, due to the entirely
separate setup phase of the protocol.)
The use case is a proxy host which makes a secondary SSH connection to
a real destination host. A user has run into one of these recently,
announcing a version banner of "SSH-2.0-FudoSSH", which relies on
agent forwarding to authenticate the secondary connection. You connect
to the proxy host and authenticate with a username string of the form
"realusername#real.destination.host", and then, at the start of the
connection protocol, the server immediately opens a channel back to
your SSH agent which it uses to authenticate to the destination host.
And it delays answering any CHANNEL_OPEN requests from the client
until that's all done. For example (seen from the client's POV,
although the server's CHANNEL_OPEN may well have been _sent_ up front
rather than in response to the client's):
client: SSH2_MSG_CHANNEL_OPEN "session"
server: SSH2_MSG_CHANNEL_OPEN "auth-agent@openssh.com"
client: SSH2_MSG_CHANNEL_OPEN_CONFIRMATION to the auth-agent request
<- data is exchanged on the agent channel; proxy host uses
that signature to log in to the destination host ->
server: SSH2_MSG_CHANNEL_OPEN_CONFIRMATION to the session request
With PuTTY, this wasn't working, because at the point when the server
sends the auth-agent CHANNEL_OPEN, we had not yet had any opportunity
to send auth-agent-req (because that has to wait until we've had a
CHANNEL_OPEN_CONFIRMATION). So we were rejecting the server's
CHANNEL_OPEN, which broke this workflow:
client: SSH2_MSG_CHANNEL_OPEN "session"
server: SSH2_MSG_CHANNEL_OPEN "auth-agent@openssh.com"
client: SSH2_MSG_CHANNEL_OPEN_FAILURE to the auth-agent request
(hey, I haven't told you you can do that yet!)
server: SSH2_MSG_CHANNEL_OPEN_FAILURE to the session request
(in that case, no shell session for you!)
When we invent a movzx instruction as part of shift-count logging on
x86, we apparently need to set its 'translation' field to point at a
pre-existing instruction that it's logically related to. Later
versions of DynamoRIO than I was running with will complain if this
isn't done.
The callback function to pageant_enum_keys now takes a flags
parameter, which receives the flags word from the extended key list
request, if available. (If not, then the flags word is passed as
zero.)
The only callback that uses this parameter is the one for printing
text output from 'pageant -l', which uses it to print a suffix on each
line, indicating whether the key is stored encrypted only (so it will
need a passphrase on next use), or whether it's stored both encrypted
_and_ unencrypted (so that 'pageant -R' will be able to return it to
the former state).
Now, if you have a given key stored encrypted in your agent and you
say 'pageant -a [same key]' (without -E), Pageant will notice (via the
new extended key list request) that the key is currently encrypted in
the agent, and that you're trying to add it unencrypted. In this
situation it won't abort the attempt, and will try to add the key
anyway, so that it becomes decrypted in your agent.
Now, if you send SSH2_AGENTC_ADD_IDENTITY with a cleartext private key
blob, and the agent already contains an encrypted-only version of the
same key, it will drop the cleartext version in alongside it,
effectively decrypting the key as if the passphrase had been typed.
This is an extended version of SSH2_AGENTC_REQUEST_IDENTITIES, which
augments each entry in the returned key list with an extra field
containing additional data about the key.
The initial contents of that extra field are a pair of flags
indicating whether the key is currently stored in the agent encrypted,
decrypted or both.
The idea is that this will permit a Pageant-aware client to make
decisions based on that. For a start, the output key list can mention
it to the user; also, if you try to add a key unencrypted when it's
already present, we can discriminate based on whether it's already
present _unencrypted_
Somehow it had acquired a lot of internal 2-space indentation, which
is out of step with the rest of this code base's style. Before I get
into making more changes in here, let's clean it up.
When I added the psusan man page, I noticed that they've all got
impenetrable names like 'man-pl.but' to fit within 8.3 naming. But
this source base hasn't had to worry about 8.3 naming conventions in a
long time, so I think I can safely rename all those files to ones
whose purpose is more obvious.
I've been collecting actual examples of things I've used psusan for,
and now I think I have enough of them to make some kind of case for
why it's a useful tool. So I've written a man page, and dumped all my
collected examples in there.
In some applications of psusan, it's useful to establish a fixed
listening endpoint on a Unix-domain socket. You can make this happen
using an external helper program (effectively behaving like a
specialised inetd), but it's more convenient to have it built in to
psusan itself, and not really very difficult since Uppity had all the
necessary code already.
I've also added the --listen-once option from Uppity, and for good
measure, the --verbose option (so that psusan in listening mode can
show connections and disconnections on its original standard error).
'Uppity' is the name of a program that's only useful for debugging, so
I'd rather not have its name reused by psusan which I'm polishing up
to be actually useful to end users (if rather specialist ones).
So SshServerConfig now has an 'application name' field which is used
as the application name in the SSH banner, and Uppity sets it to
"Uppity" while psusan sets it to "PSUSAN".
This occurred to me recently as a (very small) hole in the logging
strategy: if the size of an allocated memory block depended on some
secret data, it certainly would change the control flow and memory
access pattern inside malloc, but since we disable logging inside
malloc, the log file from this test suite would never see the
difference.
Easily fixed by printing the size of each block in the code that
intercepts malloc and realloc. As expected, no test actually fails as
a result of filling in this gap.
On AArch64, there are unexpectedly malloc and free functions in ld.so,
so the module-load function finds them there, wraps them, and then
misses the real versions in libc.
This allows my side-channel test system to at least _compile_ on other
architectures without failing for the lack of OP_xxx enum constants,
although it now won't log all the things it needs to be a proper test.
Apart from being pointless, it also triggers a bug in OpenSSH pre-8.1
that causes it to send a repeat EXT_INFO after the rekey concludes,
which trips our quite draconian check for whether EXT_INFO has been
sent at the right time.
The OpenSSH bug: https://bugzilla.mindrot.org/show_bug.cgi?id=2929
Again in the GDK Broadway backend, where we never get a non-Unicode
translation of any keystroke: when we come to the code that handles
Ctrl+letter (and other symbols), we were basing the Ctrl transform on
output[1], that is, the non-Unicode translation we had so far. But we
didn't have one.
Now we check the use_ucsoutput flag to decide which of output and
ucsoutput to start from, and as a result, Ctrl+keys works again on
Broadway.
When you run using the GDK Broadway backend, this turns out to happen,
and it's new in my experience - I was cheerfully iterating over
event->string and calling strlen on it without ever checking it for
NULL.
I must have not recompiled with debug printouts enabled since updating
the internal printf functions to have the gcc printf attribute, or
these warnings would surely have come up before.
If it's worth having command-line options to _specify_ a log file,
it's also worth having options to avoid having to answer an
interactive prompt _about_ that log file every time.
(Particularly useful when debugging, in which I often want to run a
zillion instances of the same quite temporary command line that
involves writing a log file.)
This adds the framework to be able to send it in both client _and_
server (in the post-NEWKEYS slot); it's just that currently only the
server has anything it wants to put in it.
Uppity now announces its public key type list, which is enough by
itself to allow it to accept RFC 8332 rsa-sha2-* signatures during
userauth. (Because the key verification code receives an ssh-rsa host
key and validates it against the SHA2-based key algorithm structure
derived from the id string that was sent separately.)
The information was already centralised in find_pubkey_alg, but that
had a query-based API that couldn't enumerate the key types. Now I
expose an underlying array so that it's possible to iterate over them.
Also, I'd forgotten to add the two new rsa-sha2-* algorithms to
find_pubkey_alg. That's also done as part of this commit.
As with the userauth keys, there's a localised bodge when sending
algorithm names, where I just write a couple of extra entries into the
list when I notice that a key is RSA-typed. Then I arrange that the
selection of those entries sets the new variable s->hkflags to the
right value to pass to ssh_key_sign.
We parse enough of EXT_INFO to spot when the server advertises support
for them, and if it does, we upgrade the key algorithm name from
"ssh-rsa" to one of the other two, and set appropriate signing flags.
This doesn't actually end up using the ssh_rsa_sha256 / ssh_rsa_sha512
vtables I set up two commits ago, because it's easier to just vary the
flags word we pass to ssh_key_sign.
The upgrade is done by ad-hoc special-case code in ssh2userauth.c. I
could have done it by introducing a new ssh_keyalg vtable method for
'please upgrade to your favourite version of yourself according to
some set of flags from the BPP', but it just didn't seem like a good
idea at this stage, because it presupposes that quirks in the
algorithm selection are going to follow a consistent pattern, and I
think it's much more likely that the next weird thing in this area
will be something totally different. So I've left it as a localised
bodge for now, and we can always refactor it into something nicer once
we have more information and know what the nicer thing _is_.
We now add the appropriate advertisement to our KEXINIT that indicates
a willingness to receive EXT_INFO. Code in the BPP enforces that it
must appear in one of the permitted locations in the protocol (in
particular, this ensures a pre-key-exchange MITM can't get away with
inserting it into the initial cleartext segment of the protocol). And
when we receive it, we look through it for extension names we know
about.
No functional change (except for the advertisement in KEXINIT): we
don't yet actually do anything in response to any extension reported
in EXT_INFO.
This is the cleanest part of the RFC 8332 support: I simply add two
more RSA-based SSH-2 key algorithm vtables, both almost identical to
the existing one, with different ssh_id strings and signature flags.
Adding those to the HOSTKEY_ALGORITHMS list macro is enough to ensure
that we advertise support for the new identifiers in our client
KEXINIT, select the appropriate algorithm if the server announces one
or both of them too, and use the right version of the signature
validation.
