The constants used in the test worked with every supported curve
except secp192k1. For secp192k1, the "N-1" exponent was too large.
Signed-off-by: Gilles Peskine <gilles.peskine@arm.com>
Replace the now-redundant internal curve type macros ECP_xxx by the
macros MBEDTLS_ECP__xxx_ENABLED which are declared in ecp.h.
Signed-off-by: Gilles Peskine <gilles.peskine@arm.com>
Document that mbedtls_ecp_muladd and mbedtls_ecp_muladd_restartable
are only implemented on short Weierstrass curves.
Exclude these functions at build time if no short Weierstrass curve
is included in the build. Before, these functions failed to compile in
such a configuration.
Signed-off-by: Gilles Peskine <gilles.peskine@arm.com>
mbedtls_ecp_write_key is a mirror function to mbedtls_ecp_read_key, which
writes a private key back into a byte buffer in the correct format.
This is a helpful convenience function, since the byte order is defined
differently between Montgomery and Weierstrass curves. Since this difference
is accounted for in mbedtls_ecp_read_key, it made sense to add
mbedtls_ecp_write_key for the purpose of abstracting this away such that
psa_export_key doesn't need to take byte order into account.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
In library source files, include "common.h", which takes care of
including "mbedtls/config.h" (or the alternative MBEDTLS_CONFIG_FILE)
and other things that are used throughout the library.
FROM=$'#if !defined(MBEDTLS_CONFIG_FILE)\n#include "mbedtls/config.h"\n#else\n#include MBEDTLS_CONFIG_FILE\n#endif' perl -i -0777 -pe 's~\Q$ENV{FROM}~#include "common.h"~' library/*.c 3rdparty/*/library/*.c scripts/data_files/error.fmt scripts/data_files/version_features.fmt
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
While this is a static function, so right now we know we don't need the check,
things may change in the future, so better be on the safe side.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
CTR-DRBG and HMAC-DRBG may used the seed differently depending on its length.
To avoid leaks, pass them a constant-length seed.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Checking the budget only after the randomization is done means sometimes we
were randomizing first, then noticing we ran out of budget, return, come back
and randomize again before we finally normalize.
While this is fine from a correctness and security perspective, it's a minor
inefficiency, and can also be disconcerting while debugging, so we might as
well avoid it.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
It results in smaller code than using CTR_DRBG (64 bytes smaller on ARMv6-M
with arm-none-eabi-gcc 7.3.1), so let's use this by default when both are
available.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Unless MBEDTLS_ECP_NO_INTERNAL_RNG is defined, it's no longer possible for
f_rng to be NULL at the places that randomize coordinates.
Eliminate the NULL check in this case:
- it makes it clearer to reviewers that randomization always happens (unless
the user opted out at compile time)
- a NULL check in a place where it's easy to prove the value is never NULL
might upset or confuse static analyzers (including humans)
- removing the check saves a bit of code size
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Currently we draw pseudo-random numbers at the beginning and end of the main
loop. With ECP_RESTARTABLE, it's possible that between those two occasions we
returned from the multiplication function, hence lost our internal DRBG
context that lives in this function's stack frame. This would result in the
same pseudo-random numbers being used for blinding in multiple places. While
it's not immediately clear that this would give rise to an attack, it's also
absolutely not clear that it doesn't. So let's avoid that by using a DRBG
context that lives inside the restart context and persists across
return/resume cycles. That way the RESTARTABLE case uses exactly the
same pseudo-random numbers as the non-restartable case.
Testing and compile-time options:
- The case ECP_RESTARTABLE && !ECP_NO_INTERNAL_RNG is already tested by
component_test_no_use_psa_crypto_full_cmake_asan.
- The case ECP_RESTARTABLE && ECP_NO_INTERNAL_RNG didn't have a pre-existing
test so a component is added.
Testing and runtime options: when ECP_RESTARTABLE is enabled, the test suites
already contain cases where restart happens and cases where it doesn't
(because the operation is short enough or because restart is disabled (NULL
restart context)).
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
While it seems cleaner and more convenient to set it in the top-level
mbedtls_ecp_mul() function, the existence of the restartable option changes
things - when it's enabled the drbg context needs to be saved in the restart
context (more precisely in the restart_mul sub-context), which can only be
done when it's allocated, which is in the curve-specific mul function.
This commit only internal drbg management from mbedtls_ecp_mul() to
ecp_mul_mxz() and ecp_mul_comb(), without modifying behaviour (even internal),
and a future commit will modify the ecp_mul_comb() version to handle restart
properly.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
The case of MBEDTLS_ECP_RESTARTABLE isn't handled correctly yet: in that case
the DRBG instance should persist when resuming the operation. This will be
addressed in the next commit.
When both CTR_DRBG and HMAC_DRBG are available, CTR_DRBG is preferred since
both are suitable but CTR_DRBG tends to be faster and I needed a tie-breaker.
There are currently three possible cases to test:
- NO_INTERNAL_RNG is set -> tested in test_ecp_no_internal_rng
- it's unset and CTR_DRBG is available -> tested in the default config
- it's unset and CTR_DRBG is disabled -> tested in
test_ecp_internal_rng_no_ctr_drbg
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
ecp_double_add_mxz wrongly does an MPI addition followed by a call to
MOD_MUL instead of MOD_ADD. This is more visible since the
mbedtls_mpi_xxx_mod functions have been added in commit 3b3b34f608
("Replace some macros by functions").
Fix that by using mbedtls_mpi_add_mod instead. The testsuite still
passes after that change.
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
See the comments in the code for how an attack would go, and the ChangeLog
entry for an impact assessment. (For ECDSA, leaking a few bits of the scalar
over several signatures translates to full private key recovery using a
lattice attack.)
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
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.
Replace some frequently-used macros by inline functions: instead of
calling MOD_{ADD,SUB,MUL} after the mbedtls_mpi_{add,sub,mul}_mpi,
call a function mbedtls_mpi_xxx_mod that does the same.
In the baremetal config, with "gcc -Os -mthumb -mcpu=cortex-m0plus",
ecp.o goes down from 13878 bytes to 12234.
No noticeable performance change for benchmarks on x86_64 with either
"gcc -O2" or "gcc -Os".
The window size variable in ecp_pick_window_size() can take values
4, 5 or 6, but we clamp it not to exceed the value of
MBEDTLS_ECP_WINDOW_SIZE. If that is 6 (default) or higher, the
static analyzer will point out that the test:
w > MBEDTLS_ECP_WINDOW_SIZE always evaluates to false.
This commit removes the test for the cases of the window size
large enough to fit all the potential values of the variable.
Resolve merge conflicts by performing the following actions:
- Reject changes to deleted files.
- Reject changes to generate_errors.pl and generate_visualc_files.pl.
Don't add an 'include-crypto' option which would attempt to use the
non-existent crypto submodule.
- list-identifiers.sh had the `--internal` option added to it, which
lists identifiers only in internal headers. Add PSA-specific internal
headers to list-identifiers.sh.
* origin/development: (40 commits)
Document the scripts behaviour further
Use check_output instead of Popen
all.sh: Require i686-w64-mingw32-gcc version >= 6
generate_visualc_files.pl: add mbedtls source shadowing by crypto
generate_errors.pl: refactor and simplify the code
Start unused variable with underscore
Correct documentation
generate_errors.pl: typo fix
revert changes to generate_features.pl and generate_query_config.pl
Check that the report directory is a directory
Use namespaces instead of full classes
Fix pylint issues
Don't put abi dumps in subfolders
Add verbose switch to silence all output except the final report
Fetch the remote crypto branch, rather than cloning it
Prefix internal functions with underscore
Add RepoVersion class to make handling of many arguments easier
Reduce indentation levels
Improve documentation
Use optional arguments for setting repositories
...
Resolve conflicts actions:
- Reject path changes to config.h
- Reject submodule-related changes in build scripts (Makefile,
CMakeLists.txt)
- Add oid test suite to list of tests in tests/CMakeLists.txt,
rejecting any test filtering related changes (which TLS uses to avoid
duplicating crypto tests)
- Add legacy ECDH test to all.sh without including
all.sh tests that depend on SSL
The ecp_get_type function comes handy in higher level modules and tests
as well. It is not inline anymore, to enable alternative implementations
to implement it for themselves.
mbedtls_ecp_read_key() module returned without an error even when
importing keys corresponding to the requested group was not
implemented.
We change this and return an error when the requested group is not
supported and make the remaining import/export functions more robust.
Renamed the tests because they are explicitly testing Curve25519 and
nothing else. Improved test coverage, test documentation and extended
in-code documentation with a specific reference to the standard as well.
The library is able to perform computations and cryptographic schemes on
curves with x coordinate ladder representation. Here we add the
capability to export such points.
The private keys used in ECDH differ in the case of Weierstrass and
Montgomery curves. They have different constraints, the former is based
on big endian, the latter little endian byte order. The fundamental
approach is different too:
- Weierstrass keys have to be in the right interval, otherwise they are
rejected.
- Any byte array of the right size is a valid Montgomery key and it
needs to be masked before interpreting it as a number.
Historically it was sufficient to use mbedtls_mpi_read_binary() to read
private keys, but as a preparation to improve support for Montgomery
curves we add mbedtls_ecp_read_key() to enable uniform treatment of EC
keys.
For the masking the `mbedtls_mpi_set_bit()` function is used. This is
suboptimal but seems to provide the best trade-off at this time.
Alternatives considered:
- Making a copy of the input buffer (less efficient)
- removing the `const` constraint from the input buffer (breaks the api
and makes it less user friendly)
- applying the mask directly to the limbs (violates the api between the
modules and creates and unwanted dependency)
The library is able to perform computations and cryptographic schemes on
curves with x coordinate ladder representation. Here we add the
capability to import such points.
This commit improves hygiene and formatting of macro definitions
throughout the library. Specifically:
- It adds brackets around parameters to avoid unintended
interpretation of arguments, e.g. due to operator precedence.
- It adds uses of the `do { ... } while( 0 )` idiom for macros that
can be used as commands.
Additional changes to temporarily enable running tests:
ssl_srv.c and test_suite_ecdh use mbedtls_ecp_group_load instead of
mbedtls_ecdh_setup
test_suite_ctr_drbg uses mbedtls_ctr_drbg_update instead of
mbedtls_ctr_drbg_update_ret
Function calls to alternative implementations have to follow certain
rules in order to preserve correct functionality. To avoid accidentally
breaking these rules we state them explicitly in the ECP module for
ourselves and every contributor to see.
We initialized the ECC hardware before calling
mbedtls_ecp_mul_shortcuts(). This in turn calls
mbedtls_ecp_mul_restartable(), which initializes and frees the hardware
too. This issue has been introduced by recent changes and caused some
accelerators to hang.
We move the initialization after the mbedtle_ecp_mul_shortcuts() calls
to avoid double initialization.
`mbedtls_ecp_tls_read_group()` both parses the group ID and loads the
group into the structure provided. We want to support alternative
implementations of ECDH in the future and for that we need to parse the
group ID without populating an `mbedtls_ecp_group` structure (because
alternative implementations might not use that).
This commit moves the part that parses the group ID to a new function.
There is no need to test the new function directly, because the tests
for `mbedtls_ecp_tls_read_group()` are already implicitly testing it.
There is no intended change in behaviour in this commit.
This commit modifies a bounds check in `mbedtls_ecp_check_budget()` to
be correct even if the requested number of ECC operations would overflow
the operation counter.
* development-restricted: (578 commits)
Update library version number to 2.13.1
Don't define _POSIX_C_SOURCE in header file
Don't declare and define gmtime()-mutex on Windows platforms
Correct preprocessor guards determining use of gmtime()
Correct documentation of mbedtls_platform_gmtime_r()
Correct typo in documentation of mbedtls_platform_gmtime_r()
Correct POSIX version check to determine presence of gmtime_r()
Improve documentation of mbedtls_platform_gmtime_r()
platform_utils.{c/h} -> platform_util.{c/h}
Don't include platform_time.h if !MBEDTLS_HAVE_TIME
Improve wording of documentation of MBEDTLS_PLATFORM_GMTIME_R_ALT
Fix typo in documentation of MBEDTLS_PLATFORM_GMTIME_R_ALT
Replace 'thread safe' by 'thread-safe' in the documentation
Improve documentation of MBEDTLS_HAVE_TIME_DATE
ChangeLog: Add missing renamings gmtime -> gmtime_r
Improve documentation of MBEDTLS_HAVE_TIME_DATE
Minor documentation improvements
Style: Add missing period in documentation in threading.h
Rename mbedtls_platform_gmtime() to mbedtls_platform_gmtime_r()
Guard decl and use of gmtime mutex by HAVE_TIME_DATE and !GMTIME_ALT
...
In ecp_mul_comb(), if (!p_eq_g && grp->T == NULL) and then ecp_precompute_comb() fails (which can
happen due to OOM), then the new array of points T will be leaked (as it's newly allocated, but
hasn't been asigned to grp->T yet).
Symptom was a memory leak in ECDHE key exchange under low memory conditions.
The IAR compiler doesn't like it when we assign an int to an enum variable.
"C:\builds\ws\mbedtls-restricted-pr\library\ecp.c",509 Error[Pe188]:
enumerated type mixed with another type
Summary of merge conflicts:
include/mbedtls/ecdh.h -> documentation style
include/mbedtls/ecdsa.h -> documentation style
include/mbedtls/ecp.h -> alt style, new error codes, documentation style
include/mbedtls/error.h -> new error codes
library/error.c -> new error codes (generated anyway)
library/ecp.c:
- code of an extracted function was changed
library/ssl_cli.c:
- code addition on one side near code change on the other side
(ciphersuite validation)
library/x509_crt.c -> various things
- top fo file: helper structure added near old zeroize removed
- documentation of find_parent_in()'s signature: improved on one side,
added arguments on the other side
- documentation of find_parent()'s signature: same as above
- verify_chain(): variables initialised later to give compiler an
opportunity to warn us if not initialised on a code path
- find_parent(): funcion structure completely changed, for some reason git
tried to insert a paragraph of the old structure...
- merge_flags_with_cb(): data structure changed, one line was fixed with a
cast to keep MSVC happy, this cast is already in the new version
- in verify_restratable(): adjacent independent changes (function
signature on one line, variable type on the next)
programs/ssl/ssl_client2.c:
- testing for IN_PROGRESS return code near idle() (event-driven):
don't wait for data in the the socket if ECP_IN_PROGRESS
tests/data_files/Makefile: adjacent independent additions
tests/suites/test_suite_ecdsa.data: adjacent independent additions
tests/suites/test_suite_x509parse.data: adjacent independent additions
* development: (1059 commits)
Change symlink to hardlink to avoid permission issues
Fix out-of-tree testing symlinks on Windows
Updated version number to 2.10.0 for release
Add a disabled CMAC define in the no-entropy configuration
Adapt the ARIA test cases for new ECB function
Fix file permissions for ssl.h
Add ChangeLog entry for PR#1651
Fix MicroBlaze register typo.
Fix typo in doc and copy missing warning
Fix edit mistake in cipher_wrap.c
Update CTR doc for the 64-bit block cipher
Update CTR doc for other 128-bit block ciphers
Slightly tune ARIA CTR documentation
Remove double declaration of mbedtls_ssl_list_ciphersuites
Update CTR documentation
Use zeroize function from new platform_util
Move to new header style for ALT implementations
Add ifdef for selftest in header file
Fix typo in comments
Use more appropriate type for local variable
...
This commit removes all the static occurrencies of the function
mbedtls_zeroize() in each of the individual .c modules. Instead the
function has been moved to utils.h that is included in each of the
modules.
The function mbedtls_ecp_gen_keypair_base did not wipe the stack buffer used to
hold the private exponent before returning. This commit fixes this by not using
a stack buffer in the first place but instead calling mpi_fill_random directly
to acquire the necessary random MPI.
If rsm != NULL then the curve type has to be Short Weierstrass, as we don't
implement restartable Montgomery now. If and when we do, then it's better to
check for the subcontext only, and not for the curve type.
Exactly one of three ways will be used, so make that clear by using an
if 1 else if 2 else 3 structure.
While at it, don't initialize variables at declaration, just to make extra
sure they're properly initialized afterwards in all code paths.
Systematically assign state just before the next operation that may return,
rather that just after the previous one. This makes things more local. (For
example, previously precompute_comb() has to handle a state reset for
mul_comb_core(), a kind of coupling that's best avoided.)
Note that this change doesn't move the location of state updates relative
to any potential return point, which is all that matters.
Incrementing the state is error-prone as we can end up doing it too many times
(loops) or not enough (skipped branches), or just make programming mistakes
(eg. the state was incremented twice at the end, so it ended up with a value
not in the enum...)
This is the first step of the rework, the next one will rationalize where the
state assignments are done.
The call would anyway check for pointer equality and return early, but it
doesn't hurt to save a function call, and also this follows more uniformly the
pattern that those two lines go together:
#if defined(MBEDTLS_ECP_RESTARTBLE)
if( rs_ctx != NULL && ...
(Unrelated to restartable work, just noticed while staring at the code.)
Checking at the end is inefficient as we might give up when we just generated
a valid signature or key.
Otherwise code that uses these functions in other modules will have to do:
#if defined(MBEDTLS_ECP_RESTARTABLE)
ret = do_stuff( there, may, be, many, args );
#else
ret = do_stuff( their, may, be, namy, args, rs_ctx );
#fi
and there is a risk that the arg list will differ when code is updated, and
this might not be caught immediately by tests because this depends on a
config.h compile-time option which are harder to test.
Always declaring the restartable variants of the API functions avoids this
problem; the cost in ROM size should be negligible.
This will be useful for restartable ECDH and ECDSA. Currently they call
mbedtls_ecp_gen_keypair(); one could make that one restartable, but that means
adding its own sub-context, while ECDH and ECDSA (will) have their own
contexts already, so switching to this saves one extra context.
