These issues were flagged by Coverity as instances where a local
variable may be used prior to being initialized. Please note that
none of these changes fixes any particular bug, this is just an attempt
to add more robustness.
Signed-off-by: Leonid Rozenboim <leonid.rozenboim@oracle.com>
Previously, ecp_add_mixed(), commputing say P+Q, would allow for the
Q parameter to have an unset Z coordinate as a shortcut for Z == 1.
This was leveraged during computation and usage of the T-table
(storing low multiples of the to-be-multiplied point on the curve).
It is a potentially error-prone corner case, though, since an MPIs
with unset data pointer coordinate and limb size 0 is also a valid
representation of the number 0.
As a first step towards removing ECP points with unset Z coordinate,
the constant time T-array getter ecp_select_comb() has previously
been modified to return 'full' mbedtls_ecp_point structures,
including a 1-initialized Z-coordinate.
Similarly, this commit ...
- Modifies ecp_normalize_jac_many() to set the Z coordinates
of the points it operates on to 1 instead of freeing them.
- Frees the Z-coordinates of the T[]-array explicitly
once the computation and normalization of the T-table has finished.
As a minimal functional difference between old and new code,
the new code also frees the Z-coordinate of T[0]=P, which the
old code did not.
- Modifies ecp_add_mixed() to no longer allow unset Z coordinates.
Except for the post-precomputation storage form of the T[] array,
the code does therefore no longer use EC points with unset Z coordinate.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
ecp_select_comb() did previously not set the Z coordinate of the target point.
Instead, callers would either set it explicitly or leave it uninitialized,
relying on the (only partly upheld) convention that sometimes an uninitialized
Z value represents 1.
This commit modifies ecp_select_comb() to always set the Z coordinate to 1.
This comes at the cost of memory for a single coordinate, which seems worth
it for the increased robustness.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
This improves readibility and prepares for further changes
like the introduction of a single double-width temporary for
ECP arithmetic.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
`ecp_add_mixed()` and `ecp_double_jac()` are the core subroutines
for elliptic curve arithmetic, and as such crucial for the performance
of ECP primitives like ECDHE and ECDSA.
This commit provides a very slight simplification and performance and
memory usage improvement to `ecp_add_mixed()` by removing the use of
three temporary MPIs used for coordinate calculations.
Where those variables were used, the code now writes directly to the
coordinate MPIs of the target elliptic curve point.
This is a valid change even if there is aliasing between input and
output, since at the time any of the coordinate MPIs in question is
written, the corresponding coordinates of both inputs are no longer
read.
(The analogous change in `ecp_double_jac()` can not be made since
this property does not hold for `ecp_double_jac()`.)
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
minor changes, such as improving the documentation for the byte reading
macros, and using MBEDTLS_PUT_UINT16_xy in place of byte reading
macro combinations
Signed-off-by: Joe Subbiani <joe.subbiani@arm.com>
byte shifting opertations throughout library/ were only replaced with
the byte reading macros when an 0xff mask was being used.
The byte reading macros are now more widley used, however they have not
been used in all cases of a byte shift operation, as it detracted from
the immediate readability or otherwise did not seem appropriate.
Signed-off-by: Joe Subbiani <joe.subbiani@arm.com>
mbedtls_ecp_read_key and mbedtls_ecp_write_key are updated to include
support for Curve448 as prescribed by RFC 7748 §5.
Test suites have been updated to validate curve448 under Montgomery
curves.
Signed-off-by: Archana <archana.madhavan@silabs.com>
These macros were moved into a header and now check-names.sh is failing.
Add an MBEDTL_ prefix to the macro names to make it pass.
Signed-off-by: Janos Follath <janos.follath@arm.com>
We were already rejecting them at the end, due to the fact that with the
usual (x, z) formulas they lead to the result (0, 0) so when we want to
normalize at the end, trying to compute the modular inverse of z will
give an error.
If we wanted to support those points, we'd a special case in
ecp_normalize_mxz(). But it's actually permitted by all sources (RFC
7748 say we MAY reject 0 as a result) and recommended by some to reject
those points (either to ensure contributory behaviour, or to protect
against timing attack when the underlying field arithmetic is not
constant-time).
Since our field arithmetic is indeed not constant-time, let's reject
those points before they get mixed with sensitive data (in
ecp_mul_mxz()), in order to avoid exploitable leaks caused by the
special cases they would trigger. (See the "May the Fourth" paper
https://eprint.iacr.org/2017/806.pdf)
Signed-off-by: Janos Follath <janos.follath@arm.com>
Clang was complaining and check-names.sh too
This only duplicates macros, so no impact on code size. In 3.0 we can
probably avoid the duplication by using an internal header under
library/ but this won't work for 2.16.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
We were already rejecting them at the end, due to the fact that with the
usual (x, z) formulas they lead to the result (0, 0) so when we want to
normalize at the end, trying to compute the modular inverse of z will
give an error.
If we wanted to support those points, we'd a special case in
ecp_normalize_mxz(). But it's actually permitted by all sources
(RFC 7748 say we MAY reject 0 as a result) and recommended by some to
reject those points (either to ensure contributory behaviour, or to
protect against timing attack when the underlying field arithmetic is
not constant-time).
Since our field arithmetic is indeed not constant-time, let's reject
those points before they get mixed with sensitive data (in
ecp_mul_mxz()), in order to avoid exploitable leaks caused by the
special cases they would trigger. (See the "May the Fourth" paper
https://eprint.iacr.org/2017/806.pdf)
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Upgrade the default list of hashes and curves allowed for TLS. The list is
now aligned with X.509 certificate verification: hashes and curves with at
least 255 bits (Curve25519 included), and RSA 2048 and above.
Remove MBEDTLS_TLS_DEFAULT_ALLOW_SHA1_IN_KEY_EXCHANGE which would no
longer do anything.
Document more precisely what is allowed by default.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
We know that Montgomery multiplication will never be called without an
RNG, so make that clear from the beginning of the function.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
It was indirectly calling ecp_mul() without an RNG. That's actually the
rare case where this should be allowed, as ecp_muladd() is typically
used on non-secret data (to verify signatures or ZKPs) and documented as
not being constant-time.
Refactor a bit in order to keep the ability to call ecp_mul() without a
RNG, but not exposed publicly (except though muladd).
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Fix trivial faulty calls in ECP test suite and ECP/ECJPAKE self-tests (by
adding a dummy RNG).
Several tests suites are not passing yet, as a couple of library
function do call ecp_mul() with a NULL RNG. The complexity of the fixes
range from "simple refactoring" to "requires API changes", so these will
be addressed in separate commits.
This makes the option MBEDTLS_ECP_NO_INTERNAL_RNG, as well as the whole
"internal RNG" code, obsolete. This will be addressed in a future
commit, after getting the test suites to pass again.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
library/ecp_alt.h (declaring individual functions of the ECP module that can
be substituted, included when building the library with
MBEDTLS_ECP_INTERNAL_ALT enabled) clashes with ecp_alt.h (not provided,
declaring types of the ECP module when the whole implementation is
substituted, included when building the library with MBEDTLS_ECP_ALT enabled).
Depending on the search path during build, this can make MBEDTLS_ECP_ALT
unusable.
Rename library/ecp_alt.h to follow the naming convention of other alt headers:
MBEDTLS_XXX_ALT corresponds to xxx_alt.h.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Instead of generating blinding values in a not-quite-uniform way
(https://github.com/ARMmbed/mbedtls/issues/4245) with copy-pasted
code, use mbedtls_mpi_random().
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Since mbedtls_mpi_random() is not specific to ECC code, move it from
the ECP module to the bignum module.
This increases the code size in builds without short Weierstrass
curves (including builds without ECC at all) that do not optimize out
unused functions.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Rename mbedtls_ecp_gen_privkey_sw to mbedtls_mpi_random since it has
no particular connection to elliptic curves beyond the fact that its
operation is defined by the deterministic ECDSA specification. This is
a generic function that generates a random MPI between 1 inclusive and
N exclusive.
Slightly generalize the function to accept a different lower bound,
which adds a negligible amount of complexity.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
