The comparison functions between an mp_int and an integer worked by
walking along the mp_int, comparing each of its words to the
corresponding word of the integer. When they ran out of mp_int, they'd
stop.
But this overlooks the possibility that they might not have run out of
_integer_ yet! If BIGNUM_INT_BITS is defined to be less than the size
of a uintmax_t, then comparing (say) the uintmax_t 0x8000000000000001
against a one-word mp_int containing 0x0001 would return equality,
because it would never get as far as spotting the high bit of the
integer.
Fixed by iterating up to the max of the number of BignumInts in the
mp_int and the number that cover a uintmax_t. That means we have to
use mp_word() instead of a direct array lookup to get the mp_int words
to compare against, since now the word indices might be out of range.
Functions like mp_copy_integer_into, mp_add_integer_into and
mp_hs_integer all take an ordinary C integer in the form of a
uintmax_t, and perform an operation between that and an mp_int. In
order to do that, they have to break it up into some number of
BignumInt, via bit shifts.
But in C, shifting by an amount equal to or greater than the width of
the type is undefined behaviour, and you risk the compiler generating
nonsense or complaining at compile time. I did various dodges in those
functions to try to avoid that, but didn't manage to use the same
idiom everywhere. Sometimes I'd leave the integer in its original form
and shift it right by increasing multiples of BIGNUM_INT_BITS;
sometimes I'd shift it down in place every time. And mostly I'd do the
conditional shift by checking against sizeof(n), but once I did it by
shifting by half the word and then the other half.
Now refactored so that there's a pair of functions to shift a
uintmax_t left or right by BIGNUM_INT_BITS in what I hope is a UB-safe
manner, and changed all the code I could find to use them.
This is another application of the existing mp_bezout_into, which
needed a tweak or two to cope with the numbers not necessarily being
coprime, plus a wrapper function to deal with shared factors of 2.
It reindents the entire second half of mp_bezout_into, so the patch is
best viewed with whitespace differences ignored.
This is a third random-number generation function, with an API in
between the too-specific mp_random_bits and the too-general
mp_random_in_range. Now you can generate a value between 0 and n
without having to either make n a power of 2, or tediously allocate a
zero mp_int to be the lower limit for mp_random_in_range.
Implementation is done by sawing the existing mp_random_in_range into
two pieces and exposing the API between them.
This is a version of mp_lshift_fixed_into() which allocates the output
number, which it can do because you know the size of the original
number and are allowed to treat the shift count as non-secret.
(By contrast, mp_lshift_safe() would be a nonsensical function - if
you're trying to keep the shift count secret, you _can't_ use it as a
parameter of memory allocation! In that situation you have no choice
but to allocate memory based on a fixed upper bound.)
There was previously no safe left shift at all, which is an omission.
And rshift_safe_into was an odd thing to be missing, so while I'm
here, I've added it on the basis that it will probably be useful
sooner or later.
Unlike the ones in mpint.c proper, these are not intended to respect
the constant-time guarantees. They're going to be the kind of thing
you use in key generation, which is too random to be constant-time in
any case.
I've arranged several precautions to try to make sure these functions
don't accidentally get linked into the main SSH application, only into
key generators:
- declare them in a separate header with "unsafe" in the name
- put "unsafe" in the name of every actual function
- don't even link the mpunsafe.c translation unit into PuTTY proper
- in fact, define global symbols of the same name in that file and
the SSH client code, so that there will be a link failure if we
ever try to do it by accident
The initial contents of the new source file consist of the subroutine
mp_mod_short() that previously lived in sshprime.c (and was not in
mpint.c proper precisely because it was unsafe). While I'm here, I've
turned it into mp_unsafe_mod_integer() and let it take a modulus of up
to 32 bits instead of 16.
Also added some obviously useful functions to shrink an mpint to the
smallest physical size that can hold the contained number (rather like
bn_restore_invariant in the old Bignum system), which I expect to be
using shortly.
While looking over the code for other reasons, I happened to notice
that the internal function mp_add_masked_integer_into was using a
totally wrong condition to check whether it was about to do an
out-of-range right shift: it was comparing a shift count measured in
bits against BIGNUM_INT_BYTES.
The resulting bug hasn't shown up in the code so far, which I assume
is just because no caller is passing any RHS to mp_add_integer_into
bigger than about 1 or 2. And it doesn't show up in the test suite
because I hadn't tested those functions. Now I am testing them, and
the newly added test fails when built for 16-bit BignumInt if you back
out the actual fix in this commit.
These were just too footling for even me to bother splitting up into
multiple commits:
- a couple of int -> size_t changes left out of the big-bang commit
0cda34c6f
- a few 'const' added to pointer-type casts that are only going to be
read from (leaving out the const provokes a warning if the pointer
was const _before_ the cast)
- a couple of 'return' statements trying to pass the void return of
one function through to another.
- another missing (void) in a declaration in putty.h (but this one
didn't cause any knock-on confusion).
- a few tweaks to macros, to arrange that they eat a semicolon after
the macro call (extra do ... while (0) wrappers, mostly, and one
case where I had to do it another way because the macro included a
variable declaration intended to remain in scope)
- reworked key_type_to_str to stop putting an unreachable 'break'
statement after every 'return'
- removed yet another type-check of a function loaded from a Windows
system DLL
- and finally, a totally spurious semicolon right after an open brace
in mainchan.c.
A trawl through the code with -Wmissing-prototypes and
-Wmissing-variable-declarations turned up a lot of things that should
have been internal to a particular source file, but were accidentally
global. Keep the namespace clean by making them all static.
(Also, while I'm here, a couple of them were missing a 'const': the
ONE and ZERO arrays in sshcrcda.c, and EMPTY_WINDOW_TITLE in
terminal.c.)
MD5_Simple and SHA512_Simple are superseded by the general
hash_simple() in sshauxcrypt.c. The SHA-1 and SHA-256 analogues of
those functions were already removed as a side effect of reorganising
their source files for hardware acceleration, but the hashes I didn't
rewrite managed to keep their 'Simple' APIs.
mp_neg was never used in the first place. I wrote it as an obvious
companion to mp_neg_into, but since I never put either of those in the
header file, none of the rest of the code ever called them; and
internally to mpint.c, only mp_neg_into is used out of the two.
(Which makes sense, because mp_int is fundamentally an unsigned type,
so mp_neg has weird semantics - it will give you not -x but 2^k-x for
some not-entirely-specified k. mp_neg_into is actually more coherent,
because _you_ get to control k by your choice of the target mp_int.)
The number of people has been steadily increasing who read our source
code with an editor that thinks tab stops are 4 spaces apart, as
opposed to the traditional tty-derived 8 that the PuTTY code expects.
So I've been wondering for ages about just fixing it, and switching to
a spaces-only policy throughout the code. And I recently found out
about 'git blame -w', which should make this change not too disruptive
for the purposes of source-control archaeology; so perhaps now is the
time.
While I'm at it, I've also taken the opportunity to remove all the
trailing spaces from source lines (on the basis that git dislikes
them, and is the only thing that seems to have a strong opinion one
way or the other).
Apologies to anyone downstream of this code who has complicated patch
sets to rebase past this change. I don't intend it to be needed again.
With this change, my new side-channel test system gets a 100% pass
rate when compiled with clang -O3 on Ubuntu 18.10. Previously, it had
three failing tests (namely the three ECC multiply functions), all due
to inconsistent control flow inside mp_cond_swap.
I admit I don't really understand whether this is really necessary or
not, so I'm playing it safe. The problem _seems_ to be that clang has
generated one version of the cond_swap loop using integer arithmetic
and another using MMX vectors, so the obvious suspect is alignment -
probably mp_cond_swap is processing an iteration of the loop up front
until its pointer is 16-byte aligned and then switching over to the
vectorised version. But on the other hand, when I experimentally tried
forcing allocations to be 16- or even 32-byte aligned, it didn't make
a difference. And I don't speak x86 vector instructions very well (in
fact barely at all), so I'm not even completely sure of whether the
code I was reading did what I thought it did; so I'm more comfortable
with simply applying brute force to get some code generation that the
automated test is genuinely happy with.
Previously, I checked by assertion that the base was less than the
modulus. There were two things wrong with this policy. Firstly, it's
perfectly _meaningful_ to want to raise a large number to a power mod
a smaller number, even if it doesn't come up often in cryptography;
secondly, I didn't do it right, because the check was based on the
formal sizes (nw fields) of the mp_ints, which meant that it was
possible to have a failure of the assertion even in the case where the
numerical value of the base _was_ less than the modulus.
In particular, this could come up in Diffie-Hellman with a fixed
group, because the fixed group modulus was decoded from an MP_LITERAL
in sshdh.c which gave a minimal value of nw, but the base was the
public value sent by the other end of the connection, which would
sometimes be sent with the leading zero byte required by the SSH-2
mpint encoding, and would cause a value of nw one larger, failing the
assertion.
Fixed by simply using mp_modmul in monty_import, replacing the
previous clever-but-restricted strategy that I wrote when I thought I
could get away without having to write a general division-based
modular reduction at all.
Some functions got confused if given one as input (particularly
mp_get_decimal, which assumed it could safely write at least one word
into the inv5 value it makes internally), and I've decided it's easier
to stop them ever being created than to teach everything to handle
them correctly. So now mp_make_sized enforces nw != 0 by assertion,
and I've added a max at any call site that looked as if it might
violate that precondition.
mp_from_hex("") could generate one of these, in particular, so now
I've fixed it, I've added a test to make sure it continues doing
something sensible.
This is in preparation for a PRNG revamp which will want to have a
well defined boundary for any given request-for-randomness, so that it
can destroy the evidence afterwards. So no more looping round calling
random_byte() and then stopping when we feel like it: now you say up
front how many random bytes you want, and call random_read() which
gives you that many in one go.
Most of the call sites that had to be fixed are fairly mechanical, and
quite a few ended up more concise afterwards. A few became more
cumbersome, such as mp_random_bits, in which the new API doesn't let
me load the random bytes directly into the target integer without
triggering undefined behaviour, so instead I have to allocate a
separate temporary buffer.
The _most_ interesting call site was in the PKCS#1 v1.5 padding code
in sshrsa.c (used in SSH-1), in which you need a stream of _nonzero_
random bytes. The previous code just looped on random_byte, retrying
if it got a zero. Now I'm doing a much more interesting thing with an
mpint, essentially scaling a binary fraction repeatedly to extract a
number in the range [0,255) and then adding 1 to it.
I've decided not to trust register-controlled shift operations to be
time-constant after all. They're surely fine on nice fast machines
where everything simple takes one cycle, but stranger machines,
perhaps not. In which case, I should avoid using them in the mpint
shift operation that's supposed not to reveal the shift count.
I had forgotten that my VS implementation of BignumADC expected the
carry parameter to be a literal carry _flag_, i.e. a boolean, rather
than a full word of extra data to be added to the sum of the main
input BignumInts a,b. So in one place where I didn't need a separate
carry I had passed one of the data words in the carry slot, which
worked fine on gcc and clang, but VS normalised that argument to 1.
That looks like the only VS bug, though: now I get a clean run of
cryptsuite.py even if it's talking to a VS-built testcrypt.exe.
This makes the test suite pass if I compile with -DBIGNUM_OVERRIDE=4
to fall back to 16-bit BignumInt. In that mode, BignumInt is smaller
than 'int', which means default promotion keeps causing things to get
promoted to 'int' unexpectedly, so I had to add some casts back down.
I got the maximum shift count _completely_ wrong when trying to work
out whether each word should be compared against part of the input
uintmax_t: I measured it in bytes rather than bits _and_ applied it to
the wrong type. Ahem.
A major advantage of the new testcrypt system _not_ being written as a
native-code Python module in the usual way is that it makes it very
easy to recompile testcrypt in a non-default way, such as with -m32,
and still run the same tests via the same Python module.
But I hadn't actually _done_ that until now, and now that I do, the
test suite has picked up a couple of bugs. When computing the initial
reciprocal approximation in mp_divmod_into, I did a lot of work on
explicit uint64_t, but did it in a way that used BIGNUM_INT_BITS as
the number's bit size instead of the constant 64, and cast several
things absentmindedly to BignumInt. And because I'd only tested on a
platform where those are the same type anyway, I didn't spot it.
When I was originally designing my knockoff of Stein's algorithm, I
simplified it for my own understanding by replacing the step that
turns a into (a-b)/2 with a step that simply turned it into a-b, on
the basis that the next step would do the division by 2 in any case.
This made it easier to get my head round in the first place, and in
the initial Python prototype of the algorithm, it looked more sensible
to have two different kinds of simple step rather than one simple and
one complicated.
But actually, when it's rewritten under the constraints of time
invariance, the standard way is better, because we had to do the
computation for both kinds of step _anyway_, and this way we sometimes
make both of them useful at once instead of only ever using one.
So I've put it back to the more standard version of Stein, which is a
big improvement, because now we can run in at most 2n iterations
instead of 3n _and_ the code implementing each step is simpler. A
quick timing test suggests that modular inversion is now faster by a
factor of about 1.75.
Also, since I went to the effort of thinking up and commenting a pair
of worst-case inputs for the iteration count of Stein's algorithm, it
seems like an omission not to have made sure they were in the test
suite! Added extra tests that include 2^128-1 as a modulus and 2^127
as a value to invert.
The new testcrypt system made it easy to write a tiny Python program
that does a lot of multiplications of various large sizes, run it
against versions of the testcrypt binary built with lots of different
threshold settings, and time the output by running the Python program
with PUTTY_TESTCRYPT="command time -f %U ./testcrypt".
When I tried that I found that lots of values in the 20-30 range
looked about as good as each other. 24 was an unusually low dip which
could well have just been a random outlier, but it's a nice round
number so I picked it anyway.
I wrote it for the sake of a test-system design I had in mind at the
time, but that design changed after I committed, and now I think
_even_ my upcoming test application won't need to copy MontyContexts.
So I'll remove the function now, so as not to have to pointlessly
write tests for it :-)
If it had to negate x-y to make it positive for mp_mod, but the answer
comes out as zero after that, then after re-negating it this is the
one case where we _shouldn't_ add the modulus afterwards. Result was
that, for example, mp_modsub(0, 0, 5) would return 5 instead of the
obvious 0.
It correctly masked off bits in the partial word, but then left all
higher words _unchanged_ rather than zeroing them.
Apparently its use in mp_invert_mod_2to was in restricted enough
circumstances not to cause a failure there!
My test for whether x has a square root was based on testing whether a
large power of x was congruent to 1 mod p, which is a fine test
provided x is in the multiplicative group of p, but would give a false
negative on the one possible input value that _isn't_ - namely zero.
The actual number returned from the function is fine (because that too
is a large power of the input, and when the input is 0 that's
foolproof). So I just needed to add a special case for the returned
'success' flag.
I had not considered what would happen if the input and output mp_ints
aliased each other. What happens is that because I wrote the output
starting from the low end, input words would get corrupted before I'd
finished with them. Easily fixed by reversing the order of the loop.
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!
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.
