The function mbedtls_mpi_sub_abs first checked that A >= B and then
performed the subtraction, relying on the fact that A >= B to
guarantee that the carry propagation would stop, and not taking
advantage of the fact that the carry when subtracting two numbers can
only be 0 or 1. This made the carry propagation code a little hard to
follow.
Write an ad hoc loop for the carry propagation, checking the size of
the result. This makes termination obvious.
The initial check that A >= B is no longer needed, since the function
now checks that the carry propagation terminates, which is equivalent.
This is a slight performance gain.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
There was some confusion during review about when A->p[n] could be
nonzero. In fact, there is no need to set A->p[n]: only the
intermediate result d might need to extend to n+1 limbs, not the final
result A. So never access A->p[n]. Rework the explanation of the
calculation in a way that should be easier to follow.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
The function mpi_sub_hlp had confusing semantics: although it took a
size parameter, it accessed the limb array d beyond this size, to
propagate the carry. This made the function difficult to understand
and analyze, with a potential buffer overflow if misused (not enough
room to propagate the carry).
Change the function so that it only performs the subtraction within
the specified number of limbs, and returns the carry.
Move the carry propagation out of mpi_sub_hlp and into its caller
mbedtls_mpi_sub_abs. This makes the code of subtraction very slightly
less neat, but not significantly different.
In the one other place where mpi_sub_hlp is used, namely mpi_montmul,
this is a net win because the carry is potentially sensitive data and
the function carefully arranges to not have to propagate it.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
mpi_sub_hlp performs a subtraction A - B, but took parameters in the
order (B, A). Swap the parameters so that they match the usual
mathematical syntax.
This has the additional benefit of putting the output parameter (A)
first, which is the normal convention in this module.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Let code analyzers know that this is deliberate. For example MSVC
warns about the conversion if it's implicit.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
In mpi_montmul, an auxiliary function for modular
exponentiation (mbedtls_mpi_mod_exp) that performs Montgomery
multiplication, the last step is a conditional subtraction to force
the result into the correct range. The current implementation uses a
branch and therefore may leak information about secret data to an
adversary who can observe what branch is taken through a side channel.
Avoid this potential leak by always doing the same subtraction and
doing a contant-trace conditional assignment to set the result.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Separate out a version of mpi_safe_cond_assign that works on
equal-sized limb arrays, without worrying about allocation sizes or
signs.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This reverts commit 2cc69fffcf.
A check was added in mpi_montmul because clang-analyzer warned about a
possibly null pointer. However this was a false positive. Recent
versions of clang-analyzer no longer emit a warning (3.6 does, 6
doesn't).
Incidentally, the size check was wrong: mpi_montmul needs
T->n >= 2 * (N->n + 1), not just T->n >= N->n + 1.
Given that this is an internal function which is only used from one
public function and in a tightly controlled way, remove both the null
check (which is of low value to begin with) and the size check (which
would be slightly more valuable, but was wrong anyway). This allows
the function not to need to return an error, which makes the source
code a little easier to read and makes the object code a little
smaller.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
If Y was constructed through functions in this module, then Y->n == 0
iff Y->p == NULL. However we do not prevent filling mpi structures
manually, and zero may be represented with n=0 and p a valid pointer.
Most of the code can cope with such a representation, but for the
source of mbedtls_mpi_copy, this would cause an integer underflow.
Changing the test for zero from Y->p==NULL to Y->n==0 causes this case
to work at no extra cost.
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.
In mbedtls_mpi_exp_mod(), the limit check on wsize is never true when
MBEDTLS_MPI_WINDOW_SIZE is at least 6. Wrap in a preprocessor guard
to remove the dead code and resolve a Coverity finding from the
DEADCODE checker.
Change-Id: Ice7739031a9e8249283a04de11150565b613ae89
Fixes memory leak in mpi_miller_rabin() that occurs when the function has
failed to obtain a usable random 'A' 30 turns in a row.
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Refactor `mpi_write_hlp()` to not be recursive, to fix stack overflows.
Iterate over the `mbedtls_mpi` division of the radix requested,
until it is zero. Each iteration, put the residue in the next LSB
of the output buffer. Fixes#2190
In mbedtls_mpi_write_binary, avoid leaking the size of the number
through timing or branches, if possible. More precisely, if the number
fits in the output buffer based on its allocated size, the new code's
trace doesn't depend on the value of the number.
When a random number is generated for the Miller-Rabin primality test,
if the bit length of the random number is larger than the number being
tested, the random number is shifted right to have the same bit length.
This introduces bias, as the random number is now guaranteed to be
larger than 2^(bit length-1).
Changing this to instead zero all bits higher than the tested numbers
bit length will remove this bias and keep the random number being
uniformly generated.
The input distribution to primality testing functions is completely
different when used for generating primes and when for validating
primes. The constants used in the library are geared towards the prime
generation use case and are weak when used for validation. (Maliciously
constructed composite numbers can pass the test with high probability)
The mbedtls_mpi_is_prime() function is in the public API and although it
is not documented, it is reasonable to assume that the primary use case
is validating primes. The RSA module too uses it for validating key
material.
This commit modifies mpi_read_binary to always allocate the minimum number of
limbs required to hold the entire buffer provided to the function, regardless of
its content. Previously, leading zero bytes in the input data were detected and
used to reduce memory footprint and time, but this non-constant behavior turned
out to be non-tolerable for the cryptographic applications this function is used
for.
When provided with an empty line, mpi_read_file causes a numeric
underflow resulting in a stack underflow. This commit fixes this and
adds some documentation to mpi_read_file.
The modular inversion function hangs when provided with the modulus 1. This commit refuses this modulus with a BAD_INPUT error code. It also adds a test for this case.
Fix a buffer overflow when writting a string representation of an MPI
number to a buffer in hexadecimal. The problem occurs because hex
digits are written in pairs and this is not accounted for in the
calculation of the required buffer size when the number of digits is
odd.
