In the original attempt to add RSA support to PSA crypto config was too
generic. This set of changes adds support for the following RSA features:
PSA_WANT_ALG_RSA_PKCS1V15_CRYPT, PSA_WANT_ALG_RSA_PKCS1V15_SIGN,
PSA_WANT_ALG_RSA_OAEP, PSA_WANT_ALG_RSA_PSS, PSA_WANT_KEY_TYPE_RSA_KEY_PAIR,
and PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY.
There were also some updates to ensure the proper inclusion of PSA crypto
library code when certain features are enabled. These updates were made to
address warnings and errors in builds from the new tests for these
features being added for PSA crypto configuration.
Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
The KEY_TYPE_ECC_KEY_PAIR and KEY_TYPE_ECC_PUBLIC_KEY were previously
being guarded by MBEDTLS_ECP_C in the PSA crypto library code. This change
moves it to the new MBEDTLS_PSA_BUILTIN_xxx and separates KEY_PAIR
and PUBLIC_KEY as needed. Tests have also been added to validate the new
settings.
Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
This phase adds in support for the following features being
added to the list of features that can be configured in the
include/psa/crypto_config.h header file using the PSA_WANT_ALG_xxx
macros: ECDH, HMAC, HKDF, and RSA. These changes include changes to
the PSA crypto library to use the appropriate new guards that
will allow the feature to be compiled in or out either using
new PSA_WANT_ALG_xxx or the previous MBEDTLS_xxx macros.
For HKDF and HMAC, most of the PSA library code did not have a
specific matching MBEDTLS_xxx macro for that feature, but was instead
using the generic dependent MBEDTLS_MD_C macro. The ECDH and RSA
features more closely aligned with a direct replacement with a similar
macro.
The new tests for RSA, HMAC, and HKDF would normally unset additional
dependent macros, but when attempting to implement that level of
testing it required removal of too many core features like MD_C, PK_C,
ECP_C and other low level features. This may point to additional phases of
work to complete the transition of these features to the new model.
Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
<string.h> is actually needed when MBEDTLS_ERROR_C is enabled and not
when only MBEDTLS_ERROR_STRERROR_DUMMY is enabled.
Fix#3866.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
With key usage based on key identifiers and not
key handles (openless APIs), volatile keys will
also have a key identifier. Thus, isolate key
identifier validation from key persistence
validation to clarify that key identifiers
are not specific to persistent keys.
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
* zero key buffer on failure
* readability improvements
* psa_finish_key_creation adjustment after removing import_key_into_slot
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
Use the export keys functionality, to call the public API
`mbedtls_ssl_tls_prf()`, and remove the function
`mbedtls_ssl_get_dtls_srtp_key_material()`.
Signed-off-by: Johan Pascal <johan.pascal@belledonne-communications.com>
1. Check allocation success.
2. Check parameter correctness in the use_srtp extension
in server and client.
Signed-off-by: Johan Pascal <johan.pascal@belledonne-communications.com>
Improve readability of the code:
1. move common code to `ssl_internal.h` as `static inline`.
2. Add comments.
3. Use local variables for extension size.
4. Change function signature, by adding buffer size and output length.
5. Take server srtp profile out of the loop.
Signed-off-by: Johan Pascal <johan.pascal@belledonne-communications.com>
1. Add DTLS-SRTP tests in `ssl-opts.sh`
2. Add logs for the tests to filter.
3. Add function to get the profile informations.
Signed-off-by: Johan Pascal <johan.pascal@belledonne-communications.com>
Set authmode to `MBEDTLS_SSL_VERIFY_REQUIRED` when using dtls-srtp,
in case authmode was not set. This is to support self signed certificates
received by the server, which is the case with webRTC. Certificate fingerprints
are verified outside the dtls stack, as defined in RFC 5763.
Signed-off-by: Johan Pascal <johan.pascal@belledonne-communications.com>
1. Set correct mki from the `use_srtp` extension.
2. Use mki value received from the client as the mki used by server.
3. Use `mbedtls_ssl_dtls_srtp_set_mki_value()` as a client API only.
Signed-off-by: Johan Pascal <johan.pascal@belledonne-communications.com>
Return a debg message that was removed in previous commit,
Whic is searched in the ssl-opts.sh test.
Signed-off-by: Johan Pascal <johan.pascal@belledonne-communications.com>
Enforce CertificateRequest, client and server Certificates, and
CertificateVerify messages, which are mandatory in DTLS-SRTP,
as defined in RFC 5764 section 4.1
Signed-off-by: Johan Pascal <johan.pascal@belledonne-communications.com>
Make the key material length in mbedtls_ssl_get_dtls_srtp_key_material
to be in\out, like it is done all over the library
Signed-off-by: Johan Pascal <johan.pascal@belledonne-communications.com>
1. Add check for prerequisites in check_config.h
2. Add mki value to use_srtp extension
3. address some review comments
Signed-off-by: Johan Pascal <johan.pascal@belledonne-communications.com>
In the PSA crypto library, the code for verification of ECDSA is the same for
both MBEDTLS_PSA_BUILTIN_ALG_ECDSA and
MBEDTLS_PSA_BUILTIN_ALG_DETERMINISTIC_ECDSA. So, the guards should allow for
either one to enable the code blocks. The original implementation only had
the check for ECDSA. In order to make this work, config_psa.h was updated
to ensure when MBEDTLS_CRYPTO_CONFIG is disabled, the setting for DETERMINISTIC
is only updated if MBEDTLS_ECDSA_C is also enabled.
Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
Now that there's a validate_key entry point for drivers, it becomes
much more important to separate the import action (where a key needs
to be validated) from the load action (where a key has been
previously validated, and thus re-validating it would be a waste of
time).
This also exposes why not storing the 'bits' attribute persistently
was a bad idea. The only reason there's a rather large function to
detect bit size is because loading from persistent storage requires
it.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
There's no need for calling export-and-import when the key is
guaranteed to have been stored in export representation.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
This is fix for the MBEDTLS_AES_SETKEY_DEC_ALT macro switch is including the aes xts methods
and building with a custom mbedtls_aes_setkey_dec function will disable the aes xts methods.
The fix is separating the aes xts methods and the MBEDTLS_AES_SETKEY_DEC_ALT can only
switch the presence of the mbedtls_aes_setkey_dec function.
Signed-off-by: gabor-mezei-arm <gabor.mezei@arm.com>
Mbed TLS requires users of DTLS to configure timer callbacks
needed to implement the wait-and-retransmit logic of DTLS.
Previously, the presence of these timer callbacks was checked
at every invocation of `mbedtls_ssl_fetch_input()`, so lowest
layer of the messaging stack interfacing with the underlying
transport.
This commit removes this recurring check and instead checks the
presence of timers once at the beginning of the handshake.
The main rationale for this change is that it is a step towards
separating the various layers of the messaging stack more cleanly:
datagram layer, record layer, message layer, retransmission layer.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
Migrate to new syntax where the PUBLIC attribute is explicitly defined.
Avoids issues caused sometimes where cmake does not allow the mixing of
old-style and new-style syntax
Signed-off-by: Raef Coles <raef.coles@arm.com>
Allows required targets to have prefixes added to them, so that external
projects can avoid target names clashing.
Signed-off-by: Raef Coles <raef.coles@arm.com>
The calculation of the expected key size when not using the test_size_function
was not correct. The function has now been updated to handle all cases
properly to ensure the expected key size is correct for key pairs, public
keys, and symmetric keys.
Cleaned up some comments and removed unused includes.
Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
Previous guard was using original naming and did not
get updated to the new name. Guard is now using correct
definition of TEST_DRIVER_KEY_CONTEXT_SIZE_FUNCTION.
Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
Removed TBD comment that is no longer relevant since
that portion of the code has been updated.
Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
Updated get_expected_key_size in psa_crypto_driver_wrappers to properly
handle using the new size_function from PSA crypto drivers. Created
initial infrastructure to support size_function for the PSA crypto
drivers.
Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
The psa crypto library was generically using PSA_WANT_ALG_xxx, but
should have been using the correct MBEDTLS_PSA_BUILTIN_ALG_xxx
definition since that code is the builtin version. There were also
a couple of spots that needed to ensure the code block was enabled
for either ECDSA or DETERMINISTIC_ECDSA, not just one of them.
Fixed all the new ALG_ECDSA_DETERMINISTIC names to be
ALG_DETERMINISTIC_ECDSA instead.
Fixed test to use correct definitions.
Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
The CCM specification (NIST SP 800-38C) mandates that the formatting of
the additional data length l(a) changes when it is greater _or equal_ to
2^16 - 2^8 (>= 0xFF00). Since such lengths are not supported in mbed TLS,
the operation should fail in such cases.
This commit fixes an off-by-one error which allowed encryption/decryption
to be executed when l(a) was equal to 0xFF00, resulting in an
incorrect/non-standard length format being used.
Fixes#3719.
Signed-off-by: Fredrik Strupe <fredrik.strupe@silabs.com>
* Stores bits in psa_persistent_key_storage_format.
* psa_load_persistent_key_into_slot still imports plaintext keys which
ensures that the bits value gets set.
* Updates key specification to match new implementation.
* Expands persistent store and load tests with to check for bits
attribute.
* Removes bits storage from psa_se_key_data_storage_t.
Signed-off-by: Torstein Nesse <torstein.nesse@silabs.com>
* #3742 After input of a key as SECRET in the derivation, allow the
derivation result to be used as key.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
* #3741 Allow key agreement inside derivation with a key that's allowed
for the relevant agreement.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
This change fixes the decrypt cipher setup function to return the
appropriate error code of PSA_ERROR_NOT_SUPPORTED instead of
PSA_ERROR_BAD_STATE for invalid locations when the setup call is made.
Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
This reverts commit 9c46a60e6c.
When the library is dynamically linked against Glibc (as is usually
the case with Glibc), it now requires a recent Glibc at runtime if it
was compiled with a recent Glibc. This is a loss of functionality for
no demonstrated benefit.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
In psa_generate_key_internal() for ECC keys, remove the check that the
bit-size according to Mbed TLS is equal to the requested bit-size.
This check was necessary back when the PSA API encoded curves and key
sizes independently, in order to reject combinations such as SECP256R1
with a 512-bit size. Since the curve encoding changed to specifying a
curve family and a size separately, the Mbed TLS curve id (grp_id) and
the curve data (curve_info) are now determined from the size, and
checking that (curve_info->bit_size == bits) is now only a redundant
sanity check.
This check is actually buggy, because PSA Crypto and Mbed TLS don't
have exactly the same notion of key size. PSA thinks Curve25519 is
255-bit and secp224k1 is 225-bit, but Mbed TLS thinks they're 256-bit
and 224-bit respectively. Removing the check allows key generation to
work for these curves.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
mbedtls_ecp_curve_list() now lists Curve25519 and Curve448 under the names
"x25519" and "x448". These curves support ECDH but not ECDSA.
This was meant ever since the introduction of mbedtls_ecdsa_can_do()
in 0082f9df6f, but
2c69d10bac had removed the claim
that Montgomery curves support ECDH except through Everest.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
... as opposed to PSA_ERROR_BAD_STATE.
The spec on psa_cipher_finish() states that PSA_ERROR_INVALID_ARGUMENT
should be returned when:
"The total input size passed to this operation is not valid for this
particular algorithm. For example, the algorithm is a based on block
cipher and requires a whole number of blocks, but the total input size
is not a multiple of the block size."
Currently, there is a distinction between encryption and decryption
on whether INVALID_ARGUMENT or BAD_STATE is returned, but this is not
a part of the spec.
This fix ensures that PSA_ERROR_INVALID_ARGUMENT is returned
consistently on invalid cipher input sizes.
Signed-off-by: Fredrik Strupe <fredrik.strupe@silabs.com>
With the new feature MBEDTLS_PSA_CRYPTO_CONFIG, needed to
add support that when the feature is disabled, if there
are defines like MBEDTLS_ECDSA_C defined, then the PSA_WANT_
equivalent define is also enabled. This ensures the guards in
the library psa_crypto will work properly.
Also fixed an error return code in the driver wrapper for cipher
encrypt setup so it will properly pass unit tests.
Ensured config.py full works properly with the new
MBEDTLS_PSA_CRYPTO_CONFIG, it should not be set when the full
option is used.
Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
Starting with commit 49e94e3, the do/while loop in
`rsa_prepare_blinding()` was changed to a `do...while(0)`, which
prevents retry from being effective and leaves dead code.
Restore the while condition to retry, and lift the calls to finish the
computation out of the while loop by by observing that they are
performed only when `mbedtls_mpi_inv_mod()` returns zero.
Signed-off-by: Peter Kolbus <peter.kolbus@garmin.com>
The version features library needed updating to support the new
MBEDTLS_PSA_CRYPTO_CONFIG definition.
Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
Initial changes to PSA crypto core to support configuration
of ECDSA algorithm using PSA crypto configuration mechanism.
Guards using MBEDTLS_ECDSA_C and MBEDTLS_ECDSA_DETERMINISTIC have
been changed to be based off PSA_WANT_ALG_ECDSA and
PSA_WANT_ALG_ECDSA_DETERMINISTIC. Added new tests to all.sh to
confirm new settings are working properly. Current code does not
pass the tests since built in signature verification is not in place.
Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
According to https://www.bearssl.org/ctmul.html even single-precision
multiplication is not constant-time on some older platforms.
An added benefit of the new code is that it removes the somewhat mysterious
constant 0x1ff - which was selected because at that point the maximum value of
padlen was 256. The new code is perhaps a bit more readable for that reason.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
The previous code used comparison operators >= and == that are quite likely to
be compiled to branches by some compilers on some architectures (with some
optimisation levels).
For example, take the following function:
void old_update( size_t data_len, size_t *padlen )
{
*padlen *= ( data_len >= *padlen + 1 );
}
With Clang 3.8, let's compile it for the Arm v6-M architecture:
% clang --target=arm-none-eabi -march=armv6-m -Os foo.c -S -o - |
sed -n '/^old_update:$/,/\.size/p'
old_update:
.fnstart
@ BB#0:
.save {r4, lr}
push {r4, lr}
ldr r2, [r1]
adds r4, r2, #1
movs r3, #0
cmp r4, r0
bls .LBB0_2
@ BB#1:
mov r2, r3
.LBB0_2:
str r2, [r1]
pop {r4, pc}
.Lfunc_end0:
.size old_update, .Lfunc_end0-old_update
We can see an unbalanced secret-dependant branch, resulting in a total
execution time depends on the value of the secret (here padlen) in a
straightforward way.
The new version, based on bit operations, doesn't have this issue:
new_update:
.fnstart
@ BB#0:
ldr r2, [r1]
subs r0, r0, #1
subs r0, r0, r2
asrs r0, r0, #31
bics r2, r0
str r2, [r1]
bx lr
.Lfunc_end1:
.size new_update, .Lfunc_end1-new_update
(As a bonus, it's smaller and uses less stack.)
While there's no formal guarantee that the version based on bit operations in
C won't be translated using branches by the compiler, experiments tend to show
that's the case [1], and it is commonly accepted knowledge in the practical
crypto community that if we want to sick to C, bit operations are the safest
bet [2].
[1] https://github.com/mpg/ct/blob/master/results
[2] https://github.com/veorq/cryptocoding
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
* Reworked the cipher context once again to be more robustly defined
* Removed redundant memset
* Unified behaviour on failure between driver and software in cipher_finish
* Cipher test driver setup function now also returns early when its status
is overridden, like the other test driver functions
* Removed redundant test cases
* Added bad-order checking to verify the driver doesn't get called where
the spec says it won't.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
As pointed out by Ronald. The key slot is populated using
get_key_from_slot, and after calling the driver the slot is
validated to not contain an external key, so calling
get_transparent_key is superfluous.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
Added zeroization of the wrapper context on failure/abort, and reliance on
the crypto core to not call an uninitialised wrapper.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
Once an operation has been 'accepted' by a driver, the remainder is bound
to the same driver, since driver-specific context structs cannot be shared.
This provides a pretty good gate mechanism for the fallback logic, too.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
ssl_tls1_3_keys.c exports a structure containing all labels used
in the TLS 1.3 key schedule, but the TLS 1.3 key scheduling unit
tests so far replicated those labels in the test file. In particular,
wrong label values in ssl_tls1_3_keys.c wouldn't have been caught
by the unit tests.
This commit modifies the TLS 1.3 key schedule unit tests to use
the TLS 1.3 labels as exported by ssl_tls1_3_keys.c. This not only
makes sure that those labels are correct, but also avoids hardcoding
their hex-encoding in the test file.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
Needed to make additional fixes so that when MBEDTLS_USE_PSA_CRYPTO
is defined, the depends-hashes test will succeed. There are two
versions of the ecdsa_verify_wrap() function, one with
MBEDTLS_USE_PSA_CRYPTO and when when it is not enabled. The non PSA
version is not using the md_alg parameter since it is not required.
The PSA version was using that parameter to derive a different value
it needed for PSA_ALG_ECDSA. The arguement of PSA_ALG_ECDSA is
ignored for psa_sign_hash and psa_verify_hash. It is present because
it is used and must be a valid hash, not zero, for psa_sign_hash
(but not psa_verify_hash) with PSA_ALG_DETERMINISTIC_ECDSA, and it is
needed for psa_sign_message and psa_verify_message which are not
implemented yet. The local parameter now uses PSA_ALG_ECDSA_ANY for
the verify function to avoid using the md_alg parameter and avoids
returning incorrect error codes.
Fixes#3587
Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
This is in line with how the entries of the TLS 1.3 label
structure `mbedtls_ssl_tls1_3_labels` are initialized.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
`common.h` takes care of the logic of chosing the correct
configuration file, so we don't need to replicate it in
each source file.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
This commit introduces a new file library/ssl_tls13_key.c
which will subsequently be populated with functionality relating
to the TLS 1.3 key schedule.
Those functions are expected to be internal and are documented
in the internal header library/ssl_tls13_keys.h.
The first function to be implemented is the key expansion
function `HKDF-Expand-Label`. See the documentation in
library/ssl_tls13_keys.h for more information.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
Trying to compile in the PSA accelerator test driver under MBEDTLS_TEST_HOOKS
turned out to be awkward regarding existing builds. We'll put it under a
custom (not in config.h) define instead, since it's something that only
should happen in test.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
This is a more natural place for handling the drivers belonging to the
'previous' SE driver spec. It makes for a cleaner psa_crypto.c, and
potentially an easier overview of how to migrate from the old SE driver
interface to the 'opaque accelerator' interface.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
Use macros instead of accessing directly the key identifier
fields for coding consistency and ease maintenance.
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
Fix PSA code and unit tests for the unit tests
to pass with key identifiers encoding owner
identifiers.
The changes in PSA code just make the enablement
of key identifiers encoding owner identifiers
platform independent. Previous to this commit,
such key identifiers were used only in the case
of PSA SPM platforms.
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
With PSA crypto v1.0.0, a volatile key identifier may
contain a owner identifier but no file is associated
to it. Thus rename the type psa_key_file_id_t to
mbedtls_svc_key_id_t to avoid a direct link with a
file when a key identifier involves an owner
identifier.
The new type name is prefixed by mbedtls to highlight
that the type is specific to Mbed TLS implementation
and not defined in the PSA Cryptography API
specification.
The svc in the type name stands for service as this
is the key identifier type from the point of view of
the service providing the Cryptography services.
The service can be completely provided by the present
library or partially in case of a multi-client service.
As a consequence rename as well:
. MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER to
MBEDTLS_PSA_CRYPTO_KEY_ID_ENCODES_OWNER
. PSA_KEY_ID_INIT to MBEDTLS_SVC_KEY_ID_INIT
. PSA_KEY_FILE_GET_KEY_ID to MBEDTLS_SVC_KEY_ID_GET_KEY_ID
. psa_key_file_id_make to mbedtls_svc_key_id_make
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
Define always psa_key_id_t as defined in the PSA
Cryptography API specification independently of
whether the MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER
configuration file is set or not.
As a consequence, get rid of `psa_app_key_id_t` that is
not needed anymore.
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
The purpose of this commit and the following is for
psa_key_id_t to always be as defined by the PSA
Cryptography API specification.
Currently psa_key_id_t departs from its specification
definition when MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER
configuration flag is set. In that configuration, it is set
to be equal to psa_key_file_id_t which in that configuration
encodes an owner identifier along the key identifier.
Type psa_key_file_id_t was meant to be the key identifier type
used throughout the library code. If
MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER is set it
includes both a key and owner identifier, otherwise it is
equal to psa_key_id_t.
It has not been the key identifier type throughout the
library so far because when the PSA Cryptography
specification was developped the library Doxygen
documentation was used to generate the PSA Cryptography API
specification thus the need to use psa_key_id_t and not
psa_key_file_id_t.
As this constraint does not hold anymore, move
to psa_key_file_id_t as the key identifier type throughout
the library code.
By the way, this commit updates the key identifier
initialization in the tests to be compatible with a
composit key identifier. A psa_key_id_make()
inline function is introduced to initialize key
identifiers (composit ot not) at runtime.
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
This way we can have a single call to mbedtls_platform_zeroize, which
saves a few bytes of code size.
Additionally, on my PC, I notice a significant speed improvement
(x86_64 build with MBEDTLS_AESNI_C disabled, gcc 5.4.0 -O3). I don't
have an explanation for that (I expected no measurable difference).
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Remove the zeroization of a pointer variable in the AES block
functions. The code was valid but spurious and misleading since it
looked like a mistaken attempt to zeroize the pointed-to buffer.
Reported by Antonio de la Piedra, CEA Leti, France.
Note that we do not zeroize the buffer here because these are the
round keys, and they need to stay until all the blocks are processed.
They will be zeroized in mbedtls_aes_free().
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This reduces the impact of the code size increase due to the addition
of calls to mbedtls_platform_zeroize.
Signed-off-by: gabor-mezei-arm <gabor.mezei@arm.com>
Zeroising of local buffers and variables which are used for calculations in
mbedtls_internal_md*_process() and mbedtls_internal_ripemd160_process()
functions to erase sensitive data from memory.
Checked all function for possible missing zeroisation in MD.
Signed-off-by: gabor-mezei-arm <gabor.mezei@arm.com>
Zeroising of local buffers and variables which are used for calculations in
mbedtls_pkcs5_pbkdf2_hmac() and mbedtls_internal_sha*_process() functions
to erase sensitive data from memory.
Checked all function for possible missing zeroisation in PKCS and SHA.
Signed-off-by: gabor-mezei-arm <gabor.mezei@arm.com>
Minor updates to changelog for more concise wording and fixed styling
in other files as needed.
Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
Add guards in pk_wrap.c to ensure if ECDSA is not defined, errors
are returned.
Remove warnings in pk.c for unused variables.
Add new test (test_depends_pkalgs_psa) to all.sh to confirm
when USE_PSA_CRYPTO is defined that features are working properly.
Fix#3294
Signed-off-by: John Durkop <john.durkop@fermatsoftware.com>
Probably the `W[2 << MBEDTLS_MPI_WINDOW_SIZE]` notation is based on a transcription of 2**MBEDTLS_MPI_WINDOW_SIZE.
Signed-off-by: Daniel Otte <d.otte@wut.de>
Wrap multi-line string literals in parentheses
to prevent a Clang 12 -Wstring-concatenation warning
(activated by -Wall), which caused the build to fail.
Fixes https://github.com/ARMmbed/mbedtls/issues/3586
Signed-off-by: Guido Vranken <guidovranken@gmail.com>
Update copyright notices to newly added files since merge of original
PR #3546 "Update copyright notices to use Linux Foundation guidance".
Generated using the same script.
Signed-off-by: Dan Handley <dan.handley@arm.com>
* development:
Update copyright notices to use Linux Foundation guidance
Undef ASSERT before defining it to ensure that no previous definition has sneaked in through included files.
Add ChangeLog entry for X.509 CN-type vulnerability
Improve documentation of cn in x509_crt_verify()
Fix comparison between different name types
Add test: DNS names should not match IP addresses
Remove obsolete buildbot reference in compat.sh
Fix misuse of printf in shell script
Fix added proxy command when IPv6 is used
Simplify test syntax
Fix logic error in setting client port
ssl-opt.sh: include test name in log files
ssl-opt.sh: remove old buildbot-specific condition
ssl-opt.sh: add proxy to all DTLS tests
Signed-off-by: Dan Handley <dan.handley@arm.com>
Currently the new component in all.sh fails because
mbedtls_ssl_cf_memcpy_offset() is not actually constant flow - this is on
purpose to be able to verify that the new test works.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
mbedtls_md_setup() allocates a hash-specific context and then, if
requested, an extra HMAC context. If the second allocation failed, the
hash context was not freed.
Fix this by ensuring that the mbedtls_md_context_t object is always in
a consistent state, in particular, that the md_info field is always
set. For robustness, ensure that the object is in a consistent state
even on errors (other than BAD_INPUT_DATA if the object was not in a
consistent state on entry).
Fix#3486
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
The tests are supposed to be failing now (in all.sh component
test_memsan_constant_flow), but they don't as apparently MemSan doesn't
complain when the src argument of memcpy() is uninitialized, see
https://github.com/google/sanitizers/issues/1296
The next commit will add an option to test constant flow with valgrind, which
will hopefully correctly flag the current non-constant-flow implementation.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
This paves the way for a constant-flow implementation of HMAC checking, by
making sure that the comparison happens at a constant address. The missing
step is obviously to copy the HMAC from the secret offset to this temporary
buffer with constant flow, which will be done in the next few commits.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
As a result, the copyright of contributors other than Arm is now
acknowledged, and the years of publishing are no longer tracked in the
source files.
Also remove the now-redundant lines declaring that the files are part of
MbedTLS.
This commit was generated using the following script:
# ========================
#!/bin/sh
# Find files
find '(' -path './.git' -o -path './3rdparty' ')' -prune -o -type f -print | xargs sed -bi '
# Replace copyright attribution line
s/Copyright.*Arm.*/Copyright The Mbed TLS Contributors/I
# Remove redundant declaration and the preceding line
$!N
/This file is part of Mbed TLS/Id
P
D
'
# ========================
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
RFC5280 does not state that the `revocationDate` should be checked.
In addition, when no time source is available (i.e., when MBEDTLS_HAVE_TIME_DATE is not defined), `mbedtls_x509_time_is_past` always returns 0. This results in the CRL not being checked at all.
https://tools.ietf.org/html/rfc5280
Signed-off-by: Raoul Strackx <raoul.strackx@fortanix.com>
The client previously reproted the offered ciphersuites through
their numerical identifier only, while the server reported them
through their name.
This commit modifies the debug output on client and server to
both use the format `ID (NAME)` for the ciphersuites.
* development: (55 commits)
Log change as bugfix
Add changelog entry
Clarify updates to the persistent state in storage
With multiple applicable transparent drivers, the order is unspecified
Minor clarifications
Give some examples of purpsoses of pure-software transparent driver
Fix typos
Add a link to the PSA API specification
Explain locations vs lifetimes
Initialize key pointer in ecdh to NULL
Add buffer zeroization when ecp_write_key fails
Simplified key slot deletion
Style fixes
Use arc4random_buf instead of rand on NetBSD
Apply review feedback
Update open question section about public key storage
Remove the paragraph about declaring application needs
Change driver persistent data to a callback interface
Rework and expand key management in opaque drivers
Fix typos and copypasta
...
Since it is being dereferenced by free on exit it should be inited to NULL.
Also added a small test that would trigger the issue.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
* return is treated as a function call
* space between opening and closing parentheses
* remove whiteline between assignment and checking of same variable
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
* No need to check for NULL before free'ing
* No need to reset variables that weren't touched
* Set output buffer to zero if key output fails
* Document internal functions and rearrange order of input arguments to
better match other functions.
* Clean up Montgomery fix to be less verbose code
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
PSA Crypto was checking the byte length of a to-be-imported public ECP key
against the expected length for Weierstrass keys, forgetting that
Curve25519/Curve448 exists.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
Avoids stack-allocating a key slot during ECDH, and mock-attaching a
key to a key slot during key import.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
* No null-check before calling free
* Close memory leak
* No need for double check of privkey validity
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
* Allocate internal representation contexts on the heap (i.e. don't change
where they're being allocated)
* Unify load_xxx_representation in terms of allocation and init behaviour
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
* Updated wording
* Split out buffer allocation to a convenience function
* Moved variable declarations to beginning of their code block
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
Now that both ECP and RSA keys are represented in export representation,
they can be treated more uniformly.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
Change to on-demand loading of the internal representation when required
in order to call an mbed TLS cryptography API.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
Change to on-demand loading of the internal representation when required
in order to call an mbed TLS cryptography API.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
In preparation for the implementation of the accelerator APIs. This is
ramping up to the goal of only storing the export representation in the
key slot, and not keeping the crypto implementation-specific representations
around.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
Run some self-test both for a short Weierstrass curve and for a
Montgomery curve, if the build-time configuration includes a curve of
both types. Run both because there are significant differences in the
implementation.
The test data is suitable for Curve25519.
Signed-off-by: Gilles Peskine <gilles.peskine@arm.com>
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>
For some curves (semi-coincidentally, short Weierstrass curves), the
ECP module calculates some group parameters dynamically. Build the
code to calculate the parameters only if a relevant curve is enabled.
This fixes an unused function warning when building with only
Montgomery curves.
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>
In the entries (mbedtls_x509_crl_entry values) on the list constructed
by mbedtls_x509_crl_parse_der(), set entry->raw.tag to
(SEQUENCE | CONSTRUCTED) rather than to the tag of the first ASN.1
element of the entry (which happens to be the tag of the serial
number, so INTEGER or INTEGER | CONTEXT_SPECIFIC). This is doesn't
really matter in practice (and in particular the value is never used
in Mbed TLS itself), and isn't documented, but at least it's
consistent with how mbedtls_x509_buf is normally used.
The primary importance of this change is that the old code tried to
access the tag of the first element of the entry even when the entry
happened to be empty. If the entry was empty and not followed by
anything else in the CRL, this could cause a read 1 byte after the end
of the buffer containing the CRL.
The test case "X509 CRL ASN1 (TBSCertList, single empty entry at end)"
hit the problematic buffer overflow, which is detected with ASan.
Credit to OSS-Fuzz for detecting the problem.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This option allows to test the constant-flow nature of selected code, using
MemSan and the fundamental observation behind ctgrind that the set of
operations allowed on undefined memory by dynamic analysers is the same as the
set of operations allowed on secret data to avoid leaking it to a local
attacker via side channels, namely, any operation except branching and
dereferencing.
(This isn't the full story, as on some CPUs some instructions have variable
execution depending on the inputs, most notably division and on some cores
multiplication. However, testing that no branch or memory access depends on
secret data is already a good start.)
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
The test function now depends on MBEDTLS_TEST_HOOKS, which is enabled by
config.py full, and since there are already components in all.sh exercising
the full config, this test function is sill exercised even with this new
dependency.
Since this is the first time a test function depends on MBEDTLS_TEST_HOOKS,
fix a bug in check-names.sh that wasn't apparent so far: headers from
library/*.h were not considered when looking for macro definitions. This
became apparent because MBEDTLS_STATIC_TESTABLE is defined in library/common.h
and started being used in library/ssl_msg.c, so was flagged as a likely typo.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Just move code from ssl_decrypt_buf() to the new cf_hmac() function and then
call cf_hmac() from there.
This makes the new cf_hmac() function used, opening the door for making it
static in the next commit. It also validates that its interface works for
using it in ssl_decrypt_buf().
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
The dummy implementation is not constant-flow at all for now, it's just
here as a starting point and a support for developing the tests and putting
the infrastructure in place.
Depending on the implementation strategy, there might be various corner cases
depending on where the lengths fall relative to block boundaries. So it seems
safer to just test all possible lengths in a given range than to use only a
few randomly-chosen values.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
The condition is a complex and repeated a few times. There were already some
inconsistencies in the repetitions as some of them forgot about DES.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Address remaining PR comments for #2118
- Add ChangeLog.d/x509write_csr_heap_alloc.txt.
- Fix parameter alignment per Gille's recommendation.
- Update comments to more explicitly describe the manipulation of buf.
- Replace use of `MBEDTLS_MPI_MAX_SIZE` as `sig` buffer size for
call to `x509write_csr_der_internal()` with more intuitive
`MBEDTLS_PK_SIGNATURE_MAX_SIZE`.
- Update `mbedtls_x509write_csr_der()` to return
`MBEDTLS_ERR_X509_ALLOC_FAILED` on mbedtls_calloc error.
Signed-off-by: Simon Leet <simon.leet@microsoft.com>
Using a stack-buffer with a size > 2K could easily produce a stack
overflow for an embedded device which has a limited stack size.
This commit dynamically allocates the large CSR buffer.
This commit avoids using a temporary buffer for storing the OIDs.
A single buffer is used:
a) OIDs are written backwards starting with the end of the buffer;
b) OIDs are memmove'd to the beginning of the buffer;
c) signature over this OIDs is computed and written backwards from the
end of the buffer;
d) the two memory regions are compacted.
Signed-off-by: Doru Gucea <doru-cristian.gucea@nxp.com>
PSA_ALG_ECB_NO_PADDING came in to the PSA Crypto API spec v1.0.0, but
was not implemented yet in the mbed TLS implementation.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>
Changed PSA core (and PKWrite) from reaching into MPI to using the proper
ecp function to fetch a private key.
Added changelog.
Signed-off-by: Steven Cooreman <steven.cooreman@silabs.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>
Rename PSA_ECC_CURVE_xxx to PSA_ECC_FAMILY_xxx, also rename
PSA_KEY_TYPE_GET_CURVE to PSA_KEY_TYPE_ECC_GET_FAMILY and rename
psa_ecc_curve_t to psa_ecc_family_t. Old defines are provided in
include/crypto_compat.h for backward compatibility.
Signed-off-by: Paul Elliott <paul.elliott@arm.com>
"Include the library directory for the sake of 3rdparty" did the job
for Make and Visual Studio. This commit does the job for CMake.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
All libraries (should) rely on the same directory structure. Instead of
repeating the same clauses 6 times (3 libraries times 2 build modes), set
the include paths, compile definitions and install instructions with a
single piece of code.
Include the 3rdparty directory for all libraries, not just crypto. It's
currently only needed for crypto, but that's just happenstance.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.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>
When compiling library files under `3rdparty/`, the directory containing
the `.c` file that is being compiled is not the current directory, so
headers from the `library/` directory are not found. Fix this by
adding `.` to the include path.
This was not detected until now because as of this commit, no 3rdparty
source file requires a header under `library/`.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Executed "./scripts/bump_version.sh --version 2.23.0 --so-crypto 5"
A symbol has been removed from the mbedcrypto library since the last
release:
mbedtls_ecc_group_to_psa ( enum mbedtls_ecp_group_id grpid,
size_t* bits )
This is an ABI break and we need to increase the SO version.
Signed-off-by: Janos Follath <janos.follath@arm.com>
inv_mod() already returns a specific error code if the value is not
invertible, so no need to check in advance that it is. Also, this is a
preparation for blinding the call to inv_mod(), which is made easier by
avoiding the redundancy (otherwise the call to gcd() would need to be blinded
too).
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
In the next commit, we'll need to draw a second random value, in order to
blind modular inversion. Having a function for that will avoid repetition.
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
* development: (87 commits)
entropy: Adjust parameter type of internal function to avoid a cast
entropy: Avoid arithmetic on void pointer
add comment about potential future extension
Adjust comments about SEED synchronisation
entropy: Rename sysctl_wrapper to sysctl_arnd_wrapper
test_suite_x509parse.function improvement
Pass "certificate policies" extension to callback
Update iv and len context pointers manually when reallocating buffers
Add Apache-2.0 headers to all source files
Remove Dangerous Parameter Passing
Add Apache-2.0 headers to all scripts
Add missing copyright dates to scripts and sources
Show failure in ssl-opts.sh when key export fails
Add changelog entry
tests: Reformating due to rnd_* renaming
tests: Add mbedtls_test_ prefix to rnd_* symbols
tests: Reformating due to hexcmp() renaming
tests: Add mbedtls_test_ prefix to hexcmp()
tests: Reformating due to unhexify_alloc() renaming
tests: Add mbedtls_test_ prefix to unhexify_alloc()
...
Don't define anymore globally third party include
directories and compile definitions. Declare them within the
scope of the crypto library target as per the third party
source files.
Note that targets linking to the crypto library inherit from
the third party public include directories.
Signed-off-by: Ronald Cron <ronald.cron@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>
Pass the "certificate policies" extension to the callback supplied to
mbedtls_x509_crt_parse_der_with_ext_cb() if it contains unsupported
policies. This allows the callback to fully replicate the behaviour
of the deprecated MBEDTLS_X509_ALLOW_UNSUPPORTED_CRITICAL_EXTENSION
configuration.
Signed-off-by: Nicola Di Lieto <nicola.dilieto@gmail.com>
These fields might be shifted accordingly in `ssl_parse_record_header()`
when receiving a connection with CID, so they require a manual update
after calling the generic `mbedtls_ssl_reset_in_out_pointers()`.
This commit also adds a regression test which is run by all.sh.
Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>
Also normalize the first line of the copyright headers.
This commit was generated using the following script:
# ========================
#!/bin/sh
# Find scripts
find -path './.git' -prune -o '(' -name '*.c' -o -name '*.cpp' -o -name '*.fmt' -o -name '*.h' ')' -print | xargs sed -i '
# Normalize the first line of the copyright headers (no text on the first line of a block comment)
/^\/\*.*Copyright.*Arm/I {
i\
/*
s/^\// /
}
/Copyright.*Arm/I {
# Print copyright declaration
p
# Read the two lines immediately following the copyright declaration
N
N
# Insert Apache header if it is missing
/SPDX/! i\
* SPDX-License-Identifier: Apache-2.0\
*\
* Licensed under the Apache License, Version 2.0 (the "License"); you may\
* not use this file except in compliance with the License.\
* You may obtain a copy of the License at\
*\
* http://www.apache.org/licenses/LICENSE-2.0\
*\
* Unless required by applicable law or agreed to in writing, software\
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT\
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.\
* See the License for the specific language governing permissions and\
* limitations under the License.
# Clear copyright declaration from buffer
D
}
'
# ========================
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
This is basically the same as reading from /dev/urandom on supported
systems, only it has a limit of 256 bytes per call, and does not require
an open file descriptor (so it can be used in chroots, when resource
limits are in place, or are otherwise exhausted).
It's functionally equivalent to the comparable function getentropy(),
but has been around for longer. It's actually used to implement
getentropy in FreeBSD's libc. Discussions about adding getrandom or
getentropy to NetBSD are still ongoing.
It's present in all supported versions of FreeBSD and NetBSD.
It's not present in DragonFly or OpenBSD.
Documentation: https://netbsd.gw.com/cgi-bin/man-cgi?sysctl+7
Comparable code in OpenSSL:
ddec332f32/crypto/rand/rand_unix.c (L208)
Signed-off-by: nia <nia@netbsd.org>
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>
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>
No effect so far, except on dependency checking, as the feature it's meant to
disable isn't implemented yet (so the descriptions in config.h and the
ChangeLog entry are anticipation for now).
Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@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>
The previous version attempted to write the explicit IV from
the destination buffer before it has been written there.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
Invasive testing strategy
Create a new header `common.h`.
Introduce a configuration option `MBEDTLS_TEST_HOOKS` for test-specific code, to be used in accordance with the invasive testing strategy.
This is to avoid confusion with the class of macros
MBEDTLS_SSL_PROTO_TLS1_X
which have an underscore between major and minor version number.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
TLS 1.3 record protection allows the addition of an arbitrary amount
of padding.
This commit introduces a configuration option
```
MBEDTLS_SSL_TLS13_PADDING_GRANULARITY
```
The semantics of this option is that padding is chosen in a minimal
way so that the padded plaintext has a length which is a multiple of
MBEDTLS_SSL_TLS13_PADDING_GRANULARITY.
For example, setting MBEDTLS_SSL_TLS13_PADDING_GRANULARITY to 1024
means that padded plaintexts will have length 1024, 2048, ..., while
setting it to 1 means that no padding will be used.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
This commit adds an error condition for bad user configurations
and updates the number of SSL module errors in error.h.
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
This commit uses the previously defined macro to uniformize
bounds checks in several places. It also adds bounds checks to
the ClientHello writing function that were previously missing.
Also, the functions adding extensions to the ClientHello message
can now fail if the buffer is too small or a different error
condition occurs, and moreover they take an additional buffer
end parameter to free them from the assumption that one is
writing to the default output buffer.
Signed-off-by: Ronald Cron <ronald.cron@arm.com>
new name: mbedtls_x509_crt_parse_der_with_ext_cb
Co-authored-by: Gilles Peskine <gilles.peskine@arm.com>
Signed-off-by: Nicola Di Lieto <nicola.dilieto@gmail.com>
The structure `mbedtls_ssl_transform` representing record protection
transformations should ideally be used through a function-based
interface only, as this will ease change of implementation as well
as the addition of new record protection routines in the future.
This commit makes a step in that direction by introducing the
helper function `ssl_transform_get_explicit_iv_len()` which
returns the size of the pre-expansion during record encryption
due to the potential addition of an explicit IV.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
This commit simplifies nonce derivation for AEAD based record protection
routines in the following way.
So far, code distinguished between the cases of GCM+CCM and ChachaPoly:
- In the case of GCM+CCM, the AEAD nonce is the concatentation
of a 4-byte Fixed IV and a dynamically chosen 8-byte IV which is prepended
to the record. In Mbed TLS, this is always chosen to be the record sequence
number, but it need not to.
- In the case of ChaChaPoly, the AEAD nonce is derived as
`( 12-byte Fixed IV ) XOR ( 0 || 8-byte dynamic IV == record seq nr )`
and the dynamically chosen IV is no longer prepended to the record.
This commit removes this distinction by always computing the record nonce
via the formula
`IV == ( Fixed IV || 0 ) XOR ( 0 || Dynamic IV )`
The ChaChaPoly case is recovered in case `Len(Fixed IV) == Len(IV)`, and
GCM+CCM is recovered when `Len(IV) == Len(Fixed IV) + Len(Dynamic IV)`.
Moreover, a getter stub `ssl_transform_aead_dynamic_iv_is_explicit()`
is introduced which infers from a transform whether the dynamically
chosen part of the IV is explicit, which in the current implementation
of `mbedtls_ssl_transform` can be derived from the helper field
`mbedtls_ssl_transform::fixed_ivlen`.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
The computation of the per-record nonce for AEAD record protection
varies with the AEAD algorithm and the TLS version in use.
This commit introduces a helper function for the nonce computation
to ease readability of the quite monolithic record encrytion routine.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
The previous record protection code added the explicit part of the
record nonce prior to encrypting the record. This temporarily leaves
the record structure in the undesireable state that the data outsie
of the interval `rec->data_offset, .., rec->data_offset + rec->data_len`
has already been written.
This commit moves the addition of the explicit IV past record encryption.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
The internal functions
`ssl_cid_{build/parse}_inner_plaintext()`
implement the TLSInnerPlaintext mechanism used by DTLS 1.2 + CID
in order to allow for flexible length padding and to protect the
true content type of a record.
This feature is also present in TLS 1.3 support for which is under
development. As a preparatory step towards sharing the code between
the case of DTLS 1.2 + CID and TLS 1.3, this commit renames
`ssl_cid_{build/parse}_inner_plaintext()`
to
`ssl_{build/parse}_inner_plaintext()`.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
Now that lifetimes have structures and secure element drivers handle
all the lifetimes with a certain location, update driver registration
to take a location as argument rather than a lifetime.
This commit updates the Mbed TLS implementation.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
