In the case of *ret we might need to preserve a 0 value throughout the
loop and therefore we need an extra condition to protect it from being
overwritten.
The value of done is always 1 after *ret has been set and does not need
to be protected from overwriting. Therefore in this case the extra
condition can be removed.
The code relied on the assumptions that CHAR_BIT is 8 and that unsigned
does not have padding bits.
In the Bignum module we already assume that the sign of an MPI is either
-1 or 1. Using this, we eliminate the above mentioned dependency.
The signature of mbedtls_mpi_cmp_mpi_ct() meant to support using it in
place of mbedtls_mpi_cmp_mpi(). This meant full comparison functionality
and a signed result.
To make the function more universal and friendly to constant time
coding, we change the result type to unsigned. Theoretically, we could
encode the comparison result in an unsigned value, but it would be less
intuitive.
Therefore we won't be able to represent the result as unsigned anymore
and the functionality will be constrained to checking if the first
operand is less than the second. This is sufficient to support the
current use case and to check any relationship between MPIs.
The only drawback is that we need to call the function twice when
checking for equality, but this can be optimised later if an when it is
needed.
Multiplication is known to have measurable timing variations based on
the operands. For example it typically is much faster if one of the
operands is zero. Remove them from constant time code.
You can't reuse a CTR_DRBG context without free()ing it and
re-init()ing it. This generally happened to work, but was never
guaranteed. It could have failed with alternative implementations of
the AES module because mbedtls_ctr_drbg_seed() calls
mbedtls_aes_init() on a context which is already initialized if
mbedtls_ctr_drbg_seed() hasn't been called before, plausibly causing a
memory leak.
Calling free() and seed() with no intervening init fails when
MBEDTLS_THREADING_C is enabled and all-bits-zero is not a valid mutex
representation.
You can't reuse a CTR_DRBG context without free()ing it and
re-init()ing. This generally happened to work, but was never
guaranteed. It could have failed with alternative implementations of
the AES module because mbedtls_ctr_drbg_seed() calls
mbedtls_aes_init() on a context which is already initialized if
mbedtls_ctr_drbg_seed() hasn't been called before, plausibly causing a
memory leak. Calling free() and seed() with no intervening init fails
when MBEDTLS_THREADING_C is enabled and all-bits-zero is not a valid
mutex representation. So add the missing free() and init().
The blinding applied to the scalar before modular inversion is
inadequate. Bignum is not constant time/constant trace, side channel
attacks can retrieve the blinded value, factor it (it is smaller than
RSA keys and not guaranteed to have only large prime factors). Then the
key can be recovered by brute force.
Reducing the blinded value makes factoring useless because the adversary
can only recover pk*t+z*N instead of pk*t.
mbedtls_ctr_drbg_seed() always set the entropy length to the default,
so a call to mbedtls_ctr_drbg_set_entropy_len() before seed() had no
effect. Change this to the more intuitive behavior that
set_entropy_len() sets the entropy length and seed() respects that and
only uses the default entropy length if there was no call to
set_entropy_len().
The former test-only function mbedtls_ctr_drbg_seed_entropy_len() is
no longer used, but keep it for strict ABI compatibility.
Move the definitions of mbedtls_ctr_drbg_seed_entropy_len() and
mbedtls_ctr_drbg_seed() to after they are used. This makes the code
easier to read and to maintain.
mbedtls_hmac_drbg_seed() always set the entropy length to the default,
so a call to mbedtls_hmac_drbg_set_entropy_len() before seed() had no
effect. Change this to the more intuitive behavior that
set_entropy_len() sets the entropy length and seed() respects that and
only uses the default entropy length if there was no call to
set_entropy_len().
When running 'make test' with GNU make, if a test suite program
displays "PASSED", this was automatically counted as a pass. This
would in particular count as passing:
* A test suite with the substring "PASSED" in a test description.
* A test suite where all the test cases succeeded, but the final
cleanup failed, in particular if a sanitizer reported a memory leak.
Use the test executable's return status instead to determine whether
the test suite passed. It's always 0 on PASSED unless the executable's
cleanup code fails, and it's never 0 on any failure.
FixARMmbed/mbed-crypto#303
Some sanitizers default to displaying an error message and recovering.
This could result in a test being recorded as passing despite a
complaint from the sanitizer. Turn off sanitizer recovery to avoid
this risk.
The documentation of HMAC_DRBG erroneously claimed that
mbedtls_hmac_drbg_set_entropy_len() had an impact on the initial
seeding. This is in fact not the case: mbedtls_hmac_drbg_seed() forces
the entropy length to its chosen value. Fix the documentation.
The documentation of CTR_DRBG erroneously claimed that
mbedtls_ctr_drbg_set_entropy_len() had an impact on the initial
seeding. This is in fact not the case: mbedtls_ctr_drbg_seed() forces
the initial seeding to grab MBEDTLS_CTR_DRBG_ENTROPY_LEN bytes of
entropy. Fix the documentation and rewrite the discussion of the
entropy length and the security strength accordingly.
Explain how MBEDTLS_CTR_DRBG_ENTROPY_LEN is set next to the security
strength statement, rather than giving a partial explanation (current
setting only) in the documentation of MBEDTLS_CTR_DRBG_ENTROPY_LEN.
NIST and many other sources call it a "personalization string", and
certainly not "device-specific identifiers" which is actually somewhat
misleading since this is just one of many things that might go into a
personalization string.
Improve the formatting and writing of the documentation based on what
had been done for CTR_DRBG.
Document the maximum size and nullability of some buffer parameters.
Exercise the library functions with calloc returning NULL for a size
of 0. Make this a separate job with UBSan (and ASan) to detect
places where we try to dereference the result of calloc(0) or to do
things like
buf = calloc(size, 1);
if (buf == NULL && size != 0) return INSUFFICIENT_MEMORY;
memcpy(buf, source, size);
which has undefined behavior when buf is NULL at the memcpy call even
if size is 0.
This is needed because other test components jobs either use the system
malloc which returns non-NULL on Linux and FreeBSD, or the
memory_buffer_alloc malloc which returns NULL but does not give as
useful feedback with ASan (because the whole heap is a single C
object).
Add a very basic test of calloc to the selftest program. The selftest
program acts in its capacity as a platform compatibility checker rather
than in its capacity as a test of the library.
The main objective is to report whether calloc returns NULL for a size
of 0. Also observe whether a free/alloc sequence returns the address
that was just freed and whether a size overflow is properly detected.
