This commit first changes the return convention of EccPoint_mult_safer() so
that it properly reports when faults are detected. Then all functions that
call it need to be changed to (1) follow the same return convention and (2)
properly propagate UECC_FAULT_DETECTED when it occurs.
Here's the reverse call graph from EccPoint_mult_safer() to the rest of the
library (where return values are translated to the MBEDTLS_ERR_ space) and test
functions (where expected return values are asserted explicitly).
EccPoint_mult_safer()
EccPoint_compute_public_key()
uECC_compute_public_key()
pkparse.c
tests/suites/test_suite_pkparse.function
uECC_make_key_with_d()
uECC_make_key()
ssl_cli.c
ssl_srv.c
tests/suites/test_suite_pk.function
tests/suites/test_suite_tinycrypt.function
uECC_shared_secret()
ssl_tls.c
tests/suites/test_suite_tinycrypt.function
uECC_sign_with_k()
uECC_sign()
pk.c
tests/suites/test_suite_tinycrypt.function
Note: in uECC_sign_with_k() a test for uECC_vli_isZero(p) is suppressed
because it is redundant with a more thorough test (point validity) done at the
end of EccPoint_mult_safer(). This redundancy was introduced in a previous
commit but not noticed earlier.
We don't really need a secure hash for that, something like CRC32 would
probably be enough - but we have SHA-256 handy, not CRC32, so use that for the
sake of simplicity.
By semi-internal I mean functions that are only public because they're used in
more than once compilation unit in the library (for example in ecc.c and
ecc_dsa.c) but should not really be part of the public-facing API.
Same motivation as for the other parameters. This is the last one, making the
curve structure empty, so it's left with a dummy parameter for legal reasons.
Validating the input is always a good idea. Validating the output protects
against some fault injections that would make the result invalid.
Note: valid_point() implies that the point is not zero.
Adding validation to mult_safer() makes it redundant in
compute_shared_secret().
This will make easier to add future counter-measures in a single place.
In practice this change means that:
- compute_public_key() now uses projective coordinate randomisation, which it
should as this is a protection against Template Attacks for example.
- mult_safer() now checks that the result is not the point at infinity, which
it can as the result is indeed never expected to be that
Previously it was returning 0 or 1, so flipping a single bit in the return
value reversed its meaning. Now it's returning the diff itself.
This is safe because in the two places it's used (signature verification and
point validation), invalid values will have a large number of bits differing
from the expected value, so diff will have a large Hamming weight.
An alternative would be to return for example -!(diff == 0), but the
comparison itself is prone to attacks (glitching the appropriate flag in the
CPU flags register, or the conditional branch if the comparison uses one). So
we'd need to protect the comparison, and it's simpler to just skip it and
return diff itself.
This avoids the need for each calling site to manually regularize the scalar
and randomize coordinates, which makes for simpler safe use and saves 50 bytes
of code size in the library.
Even though this is type name is purely internal to a single C file, let's
reduce the potential for clashes with other wait state types which might be
added elsewhere in the library and become visible here (for example through
platform_util.h).
Previous size was 3584 bytes which is not acceptable on constrained systems
(especially on the stack). This was a misguided attempt at minimizing the
number of calls to the RNG function in order to minimize impact on
performance, but clearly this does not justify using that much RAM and a
compromise had to be found.
While at it, loose the 'curve' argument in internal randomized functions, for
the same reasons we lost 'num_words' in uECC_vli_mult_rnd(): we only have one
curve so we don't need this, and hardcoding it saves a bit of code size and
speed, which is welcome to slightly reduce the impact of the counter-measure
on both of them.
This is a counter-measure to make horizontal attacks harder. Horizontal
attacks work with a single trace by noticing when intermediate computations
within that trace happen on the same operands.
We'll try to make that harder for an attacker to achieve that by introducing
random delays based on extra computation and extra random accesses to input in
the multi-precision multiplication (which is the dominant operation and the target of
horizontal attacks known so far). This should make it hard for the attacker to
compare two multiplications.
This first commit introduces the new function for multiplication with random
delay - future commits will ensure it is used all the way up to the top-level
scalar multiplication routine.
We called in tinycrypt in the file names, but uecc in config.h, all.sh and
other places, which could be confusing. Just use tinycrypt everywhere because
that's the name of the project and repo where we took the files.
The changes were made using the following commands (with GNU sed and zsh):
sed -i 's/uecc/tinycrypt/g' **/*.[ch] tests/scripts/all.sh
sed -i 's/MBEDTLS_USE_UECC/MBEDTLS_USE_TINYCRYPT/g' **/*.[ch] tests/scripts/all.sh scripts/config.pl
