From d1fba7cdf0ffcc3fb3d5c2cd37728b2de7ed133a Mon Sep 17 00:00:00 2001 From: Valerio Setti Date: Fri, 28 Jul 2023 16:42:58 +0200 Subject: [PATCH 01/24] pk: return PK_USE_PSA_EC_DATA to pk.h Signed-off-by: Valerio Setti --- include/mbedtls/pk.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/include/mbedtls/pk.h b/include/mbedtls/pk.h index 41e980d627..28ec29d6ba 100644 --- a/include/mbedtls/pk.h +++ b/include/mbedtls/pk.h @@ -222,6 +222,28 @@ typedef struct mbedtls_pk_rsassa_pss_options { #define MBEDTLS_PK_USE_PSA_EC_DATA #endif /* MBEDTLS_USE_PSA_CRYPTO && !MBEDTLS_ECP_C */ +/* Internal helper to define which fields in the pk_context structure below + * should be used for EC keys: legacy ecp_keypair or the raw (PSA friendly) + * format. It should be noticed that this only affect how data is stored, not + * which functions are used for various operations. The overall picture looks + * like this: + * - if USE_PSA is not defined and ECP_C is then use ecp_keypair data structure + * and legacy functions + * - if USE_PSA is defined and + * - if ECP_C then use ecp_keypair structure, convert data to a PSA friendly + * format and use PSA functions + * - if !ECP_C then use new raw data and PSA functions directly. + * + * The main reason for the "intermediate" (USE_PSA + ECP_C) above is that as long + * as ECP_C is defined mbedtls_pk_ec() gives the user a read/write access to the + * ecp_keypair structure inside the pk_context so he/she can modify it using + * ECP functions which are not under PK module's control. + */ +#if defined(MBEDTLS_USE_PSA_CRYPTO) && defined(PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY) && \ + !defined(MBEDTLS_ECP_C) +#define MBEDTLS_PK_USE_PSA_EC_DATA +#endif /* MBEDTLS_USE_PSA_CRYPTO && !MBEDTLS_ECP_C */ + /** * \brief Types for interfacing with the debug module */ From c5c4bd225e6168fa328f1bdfd096653de0e94351 Mon Sep 17 00:00:00 2001 From: Valerio Setti Date: Thu, 3 Aug 2023 14:28:20 +0200 Subject: [PATCH 02/24] test: add component testing TFM configuration and P256M driver Signed-off-by: Valerio Setti --- tests/scripts/all.sh | 71 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 2afc18166f..55ce05c4f9 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -2780,6 +2780,77 @@ component_test_psa_crypto_config_reference_ecc_no_bignum () { #tests/ssl-opt.sh } +component_test_tfm_config_p256m_driver_accel_ec () { + msg "build: TFM config + p256m driver + accel ECDH(E)/ECDSA" + + # Replace "mbedtls_config.h" and "cypto_config.h" with TFM ones + cp configs/tfm_mbedcrypto_config_profile_medium.h "$CONFIG_H" + cp configs/crypto_config_profile_medium.h "$CRYPTO_CONFIG_H" + + # Create a fake "mbedtls_entropy_nv_seed_config.h" which is required by + # the TFM configuration. It will be deleted on exit. + touch "include/mbedtls/mbedtls_entropy_nv_seed_config.h" + + # Disable all the features that auto-enable ECP_LIGHT (see build_info.h) + scripts/config.py -f "$CRYPTO_CONFIG_H" unset PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_DERIVE + + # Unset PSA_CRYPTO_SPM because test and sample programs aren't equipped + # for the modified names used when MBEDTLS_PSA_CRYPTO_SPM is active. + scripts/config.py unset MBEDTLS_PSA_CRYPTO_SPM + # Use our implementation of AES + scripts/config.py unset MBEDTLS_AES_DECRYPT_ALT + scripts/config.py unset MBEDTLS_AES_SETKEY_DEC_ALT + # Configure entropy support + scripts/config.py unset MBEDTLS_NO_PLATFORM_ENTROPY + scripts/config.py set MBEDTLS_ENTROPY_NV_SEED + # Enable crypto storage + scripts/config.py set MBEDTLS_PSA_CRYPTO_STORAGE_C + + # Set the list of accelerated components in order to remove them from + # builtin support. We don't set IMPORT and EXPORT because P256M does not + # support these operations. + loc_accel_list="ALG_ECDSA \ + ALG_ECDH \ + KEY_TYPE_ECC_KEY_PAIR_BASIC \ + KEY_TYPE_ECC_KEY_PAIR_GENERATE \ + KEY_TYPE_ECC_PUBLIC_KEY" + loc_accel_flags="$( echo "$loc_accel_list" | sed 's/[^ ]* */-DMBEDTLS_PSA_ACCEL_&/g' )" + # Add missing symbols from "tfm_mbedcrypto_config_profile_medium.h". All of + # them fix missing items: + # - USE_PSA_CRYPTO for PK_HAVE_ECC_KEYS + # - FS_IO for the plaform + entropy modules + # - ECP_C and BIGNUM because P256M does not have support for import and export + # of keys so we need the builtin support for that + # - ASN1_[PARSE/WRITE]_C and OID_C found by check_config.h + cflags="-DMBEDTLS_USE_PSA_CRYPTO \ + -DMBEDTLS_ASN1_PARSE_C \ + -DMBEDTLS_ASN1_WRITE_C \ + -DMBEDTLS_OID_C \ + -DMBEDTLS_FS_IO \ + -DMBEDTLS_ECP_C \ + -DMBEDTLS_BIGNUM_C \ + -DMBEDTLS_PSA_ITS_FILE_C" + # Build crypto library specifying we want to use P256M code for EC operations + make CFLAGS="$cflags $loc_accel_flags -DMBEDTLS_P256M_EXAMPLE_DRIVER_ENABLED -O0 -g" + + # Make sure any built-in EC alg was not re-enabled by accident (additive config) + #not grep mbedtls_ecdsa_ library/ecdsa.o # this is needed for deterministic ECDSA + not grep mbedtls_ecdh_ library/ecdh.o + not grep mbedtls_ecjpake_ library/ecjpake.o + # Also ensure that ECP, RSA, DHM or BIGNUM modules were not re-enabled + #not grep mbedtls_ecp_ library/ecp.o # this is needed for import/export EC keys (explained above) + not grep mbedtls_rsa_ library/rsa.o + not grep mbedtls_dhm_ library/dhm.o + #not grep mbedtls_mpi_ library/bignum.o # this is needed from ECP module + + # Run the tests + msg "test: TFM config + p256m driver + accel ECDH(E)/ECDSA" + make test + + # Remove unnecessary files generated for the test + rm "include/mbedtls/mbedtls_entropy_nv_seed_config.h" +} + # Helper function used in: # - component_test_psa_crypto_config_accel_all_curves_except_p192 # - component_test_psa_crypto_config_accel_all_curves_except_x25519 From 983923c914a9ee23a820d8d86bdec29055e8b3d1 Mon Sep 17 00:00:00 2001 From: Valerio Setti Date: Thu, 3 Aug 2023 15:33:24 +0200 Subject: [PATCH 03/24] p256m: minor fixes to the driver interface Signed-off-by: Valerio Setti --- 3rdparty/p256-m/p256-m_driver_entrypoints.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/3rdparty/p256-m/p256-m_driver_entrypoints.c b/3rdparty/p256-m/p256-m_driver_entrypoints.c index 8828909189..90dce1e18c 100644 --- a/3rdparty/p256-m/p256-m_driver_entrypoints.c +++ b/3rdparty/p256-m/p256-m_driver_entrypoints.c @@ -114,7 +114,7 @@ psa_status_t p256_transparent_key_agreement( /* * Check that private key = 32 bytes, peer public key = 65 bytes, * and that the shared secret buffer is big enough. */ - psa_status_t status = PSA_ERROR_NOT_SUPPORTED; + psa_status_t status = PSA_ERROR_INVALID_ARGUMENT; if (key_buffer_size != 32 || shared_secret_size < 32 || peer_key_length != 65) { return status; @@ -150,7 +150,7 @@ psa_status_t p256_transparent_sign_hash( (void) alg; psa_status_t status = PSA_ERROR_NOT_SUPPORTED; - if (key_buffer_size != 32 || signature_size != 64) { + if (key_buffer_size != 32 || signature_size < 64) { return status; } From 52ba0e37183b7f27ac886393b65c179334126369 Mon Sep 17 00:00:00 2001 From: Valerio Setti Date: Fri, 4 Aug 2023 12:43:03 +0200 Subject: [PATCH 04/24] test: improve accelerated TFM configuration test and add reference Signed-off-by: Valerio Setti --- tests/scripts/all.sh | 91 ++++++++++++++++++++++++++------------------ 1 file changed, 54 insertions(+), 37 deletions(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 55ce05c4f9..6a94cc6217 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -2780,31 +2780,55 @@ component_test_psa_crypto_config_reference_ecc_no_bignum () { #tests/ssl-opt.sh } -component_test_tfm_config_p256m_driver_accel_ec () { - msg "build: TFM config + p256m driver + accel ECDH(E)/ECDSA" - - # Replace "mbedtls_config.h" and "cypto_config.h" with TFM ones +common_tfm_config () { + # Enable TF-M config cp configs/tfm_mbedcrypto_config_profile_medium.h "$CONFIG_H" cp configs/crypto_config_profile_medium.h "$CRYPTO_CONFIG_H" - # Create a fake "mbedtls_entropy_nv_seed_config.h" which is required by - # the TFM configuration. It will be deleted on exit. - touch "include/mbedtls/mbedtls_entropy_nv_seed_config.h" + # Adjust for the fact that we're building outside the TF-M environment. + # + # TF-M has separation, our build doesn't + scripts/config.py unset MBEDTLS_PSA_CRYPTO_SPM + scripts/config.py unset MBEDTLS_PSA_CRYPTO_KEY_ID_ENCODES_OWNER + # TF-M provdes its own (dummy) implemenation, from their tree + scripts/config.py unset MBEDTLS_AES_DECRYPT_ALT + scripts/config.py unset MBEDTLS_AES_SETKEY_DEC_ALT + # We have an OS that provides entropy, use it + scripts/config.py unset MBEDTLS_NO_PLATFORM_ENTROPY + + # Other config adjustments to make the tests pass. + # Those are a surprise and should be investigated and fixed. + # + # pkparse.c and pkwrite.c fail to link without this + echo "#define MBEDTLS_OID_C" >> "$CONFIG_H" + + # Config adjustements for better test coverage in our environment. + # These are not needed just to build and pass tests. + # + # Enable filesystem I/O for the benefit of PK parse/write tests. + echo "#define MBEDTLS_FS_IO" >> "$CONFIG_H" +} + +component_test_tfm_config_p256m_driver_accel_ec () { + msg "build: TF-M config + p256m driver + accel ECDH(E)/ECDSA" + + common_tfm_config # Disable all the features that auto-enable ECP_LIGHT (see build_info.h) scripts/config.py -f "$CRYPTO_CONFIG_H" unset PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_DERIVE - # Unset PSA_CRYPTO_SPM because test and sample programs aren't equipped - # for the modified names used when MBEDTLS_PSA_CRYPTO_SPM is active. - scripts/config.py unset MBEDTLS_PSA_CRYPTO_SPM - # Use our implementation of AES - scripts/config.py unset MBEDTLS_AES_DECRYPT_ALT - scripts/config.py unset MBEDTLS_AES_SETKEY_DEC_ALT - # Configure entropy support - scripts/config.py unset MBEDTLS_NO_PLATFORM_ENTROPY - scripts/config.py set MBEDTLS_ENTROPY_NV_SEED - # Enable crypto storage - scripts/config.py set MBEDTLS_PSA_CRYPTO_STORAGE_C + # Add missing symbols from "tfm_mbedcrypto_config_profile_medium.h" + # + # - USE_PSA_CRYPTO for PK_HAVE_ECC_KEYS + echo "#define MBEDTLS_USE_PSA_CRYPTO" >> "$CONFIG_H" + # - ECP_C and BIGNUM because P256M does not have support for import and export + # of keys so we need the builtin support for that + echo "#define MBEDTLS_ECP_C" >> "$CONFIG_H" + echo "#define MBEDTLS_BIGNUM_C" >> "$CONFIG_H" + # - ASN1_[PARSE/WRITE]_C and OID_C found by check_config.h + echo "#define MBEDTLS_ASN1_PARSE_C" >> "$CONFIG_H" + echo "#define MBEDTLS_ASN1_WRITE_C" >> "$CONFIG_H" + echo "#define MBEDTLS_OID_C" >> "$CONFIG_H" # Set the list of accelerated components in order to remove them from # builtin support. We don't set IMPORT and EXPORT because P256M does not @@ -2815,23 +2839,9 @@ component_test_tfm_config_p256m_driver_accel_ec () { KEY_TYPE_ECC_KEY_PAIR_GENERATE \ KEY_TYPE_ECC_PUBLIC_KEY" loc_accel_flags="$( echo "$loc_accel_list" | sed 's/[^ ]* */-DMBEDTLS_PSA_ACCEL_&/g' )" - # Add missing symbols from "tfm_mbedcrypto_config_profile_medium.h". All of - # them fix missing items: - # - USE_PSA_CRYPTO for PK_HAVE_ECC_KEYS - # - FS_IO for the plaform + entropy modules - # - ECP_C and BIGNUM because P256M does not have support for import and export - # of keys so we need the builtin support for that - # - ASN1_[PARSE/WRITE]_C and OID_C found by check_config.h - cflags="-DMBEDTLS_USE_PSA_CRYPTO \ - -DMBEDTLS_ASN1_PARSE_C \ - -DMBEDTLS_ASN1_WRITE_C \ - -DMBEDTLS_OID_C \ - -DMBEDTLS_FS_IO \ - -DMBEDTLS_ECP_C \ - -DMBEDTLS_BIGNUM_C \ - -DMBEDTLS_PSA_ITS_FILE_C" + # Build crypto library specifying we want to use P256M code for EC operations - make CFLAGS="$cflags $loc_accel_flags -DMBEDTLS_P256M_EXAMPLE_DRIVER_ENABLED -O0 -g" + make CFLAGS="$loc_accel_flags -DMBEDTLS_P256M_EXAMPLE_DRIVER_ENABLED" # Make sure any built-in EC alg was not re-enabled by accident (additive config) #not grep mbedtls_ecdsa_ library/ecdsa.o # this is needed for deterministic ECDSA @@ -2844,11 +2854,18 @@ component_test_tfm_config_p256m_driver_accel_ec () { #not grep mbedtls_mpi_ library/bignum.o # this is needed from ECP module # Run the tests - msg "test: TFM config + p256m driver + accel ECDH(E)/ECDSA" + msg "test: TF-M config + p256m driver + accel ECDH(E)/ECDSA" make test +} - # Remove unnecessary files generated for the test - rm "include/mbedtls/mbedtls_entropy_nv_seed_config.h" +component_test_tfm_config() { + common_tfm_config + + msg "build: TF-M config" + make tests + + msg "test: TF-M config" + make test } # Helper function used in: From ac6d35f793eb138d4f501d08c718c763d770ec98 Mon Sep 17 00:00:00 2001 From: Valerio Setti Date: Fri, 4 Aug 2023 12:49:11 +0200 Subject: [PATCH 05/24] test: update components' descriptions Signed-off-by: Valerio Setti --- tests/scripts/all.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 6a94cc6217..255eefcc50 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -2780,6 +2780,9 @@ component_test_psa_crypto_config_reference_ecc_no_bignum () { #tests/ssl-opt.sh } +# Helper for setting common configurations between: +# - component_test_tfm_config_p256m_driver_accel_ec() +# - component_test_tfm_config() common_tfm_config () { # Enable TF-M config cp configs/tfm_mbedcrypto_config_profile_medium.h "$CONFIG_H" @@ -2809,6 +2812,8 @@ common_tfm_config () { echo "#define MBEDTLS_FS_IO" >> "$CONFIG_H" } +# Keep this in sync with component_test_tfm_config() as they are both meant +# to be used in analyze_outcomes.py for driver's coverage analysis. component_test_tfm_config_p256m_driver_accel_ec () { msg "build: TF-M config + p256m driver + accel ECDH(E)/ECDSA" @@ -2858,6 +2863,9 @@ component_test_tfm_config_p256m_driver_accel_ec () { make test } +# Keep this in sync with component_test_tfm_config_p256m_driver_accel_ec() as +# they are both meant to be used in analyze_outcomes.py for driver's coverage +# analysis. component_test_tfm_config() { common_tfm_config From f01d6486770505b1884c85034500f98195e07929 Mon Sep 17 00:00:00 2001 From: Valerio Setti Date: Fri, 4 Aug 2023 13:51:18 +0200 Subject: [PATCH 06/24] analyze_outcome: add new check for parity for TFM configuration Signed-off-by: Valerio Setti --- tests/scripts/analyze_outcomes.py | 60 +++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/scripts/analyze_outcomes.py b/tests/scripts/analyze_outcomes.py index ee51513b73..ac97a99dd9 100755 --- a/tests/scripts/analyze_outcomes.py +++ b/tests/scripts/analyze_outcomes.py @@ -402,6 +402,66 @@ TASKS = { 'ignored_tests': {} } }, + 'analyze_driver_vs_reference_tfm_config': { + 'test_function': do_analyze_driver_vs_reference, + 'args': { + 'component_ref': 'test_tfm_config', + 'component_driver': 'test_tfm_config_p256m_driver_accel_ec', + # Ignore test suites for the modules that are disabled in the + # accelerated test case. + 'ignored_suites': ['ecdh', 'ecdsa'], + 'ignored_tests': { + # Ignore all tests that require DERIVE support which is disabled + # in the driver version + 'test_suite_psa_crypto': [ + 'PSA key agreement setup: ECDH + HKDF-SHA-256: good', + ('PSA key agreement setup: ECDH + HKDF-SHA-256: good, key algorithm broader ' + 'than required'), + 'PSA key agreement setup: ECDH + HKDF-SHA-256: public key not on curve', + 'PSA key agreement setup: KDF instead of a key agreement algorithm', + 'PSA key agreement setup: bad key agreement algorithm', + 'PSA key agreement: ECDH SECP256R1 (RFC 5903) + HKDF-SHA-256: capacity=8160', + 'PSA key agreement: ECDH SECP256R1 (RFC 5903) + HKDF-SHA-256: read 0+32', + 'PSA key agreement: ECDH SECP256R1 (RFC 5903) + HKDF-SHA-256: read 1+31', + 'PSA key agreement: ECDH SECP256R1 (RFC 5903) + HKDF-SHA-256: read 31+1', + 'PSA key agreement: ECDH SECP256R1 (RFC 5903) + HKDF-SHA-256: read 32+0', + 'PSA key agreement: ECDH SECP256R1 (RFC 5903) + HKDF-SHA-256: read 32+32', + 'PSA key agreement: ECDH SECP256R1 (RFC 5903) + HKDF-SHA-256: read 64+0', + 'PSA key derivation: ECDH on P256 with HKDF-SHA256, info first', + 'PSA key derivation: ECDH on P256 with HKDF-SHA256, key output', + 'PSA key derivation: ECDH on P256 with HKDF-SHA256, missing info', + 'PSA key derivation: ECDH on P256 with HKDF-SHA256, omitted salt', + 'PSA key derivation: ECDH on P256 with HKDF-SHA256, raw output', + 'PSA key derivation: ECDH on P256 with HKDF-SHA256, salt after secret', + 'PSA key derivation: ECDH with TLS 1.2 PRF SHA-256, good case', + 'PSA key derivation: ECDH with TLS 1.2 PRF SHA-256, missing label', + 'PSA key derivation: ECDH with TLS 1.2 PRF SHA-256, missing label and secret', + 'PSA key derivation: ECDH with TLS 1.2 PRF SHA-256, no inputs', + 'PSA key derivation: HKDF-SHA-256 -> ECC secp256r1', + 'PSA key derivation: HKDF-SHA-256 -> ECC secp256r1 (1 redraw)', + 'PSA key derivation: HKDF-SHA-256 -> ECC secp256r1, exercise ECDSA', + 'PSA key derivation: TLS 1.2 Mix-PSK-to-MS, SHA-256, 0+48, ka', + 'PSA key derivation: TLS 1.2 Mix-PSK-to-MS, SHA-256, 24+24, ka', + 'PSA key derivation: TLS 1.2 Mix-PSK-to-MS, SHA-256, 48+0, ka', + 'PSA key derivation: TLS 1.2 Mix-PSK-to-MS, bad state #1, ka', + 'PSA key derivation: TLS 1.2 Mix-PSK-to-MS, bad state #3, ka', + 'PSA key derivation: TLS 1.2 Mix-PSK-to-MS, bad state #4, ka', + 'PSA key derivation: bits=7 invalid for ECC BRAINPOOL_P_R1 (ECC enabled)', + 'PSA key derivation: bits=7 invalid for ECC MONTGOMERY (ECC enabled)', + 'PSA key derivation: bits=7 invalid for ECC SECP_K1 (ECC enabled)', + 'PSA key derivation: bits=7 invalid for ECC SECP_R1 (ECC enabled)', + 'PSA key derivation: bits=7 invalid for ECC SECP_R2 (ECC enabled)', + 'PSA key derivation: bits=7 invalid for ECC SECT_K1 (ECC enabled)', + 'PSA key derivation: bits=7 invalid for ECC SECT_R1 (ECC enabled)', + 'PSA key derivation: bits=7 invalid for ECC SECT_R2 (ECC enabled)', + 'PSA raw key agreement: ECDH SECP256R1 (RFC 5903)', + ], + 'test_suite_psa_crypto_pake': [ + 'PSA PAKE: ecjpake size macros', + ] + } + } + } } def main(): From 132240f01a9e7b1d725d3525b3fc35be488cf5b4 Mon Sep 17 00:00:00 2001 From: Valerio Setti Date: Mon, 7 Aug 2023 14:34:53 +0200 Subject: [PATCH 07/24] test: use ASAN flags for testing the accelerated TFM configuration Signed-off-by: Valerio Setti --- tests/scripts/all.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 255eefcc50..3482df33a5 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -2798,6 +2798,8 @@ common_tfm_config () { scripts/config.py unset MBEDTLS_AES_SETKEY_DEC_ALT # We have an OS that provides entropy, use it scripts/config.py unset MBEDTLS_NO_PLATFORM_ENTROPY + # Disable buffer allocator and use dynamic memory for calloc/free + scripts/config.py unset MBEDTLS_MEMORY_BUFFER_ALLOC_C # Other config adjustments to make the tests pass. # Those are a surprise and should be investigated and fixed. @@ -2846,7 +2848,7 @@ component_test_tfm_config_p256m_driver_accel_ec () { loc_accel_flags="$( echo "$loc_accel_list" | sed 's/[^ ]* */-DMBEDTLS_PSA_ACCEL_&/g' )" # Build crypto library specifying we want to use P256M code for EC operations - make CFLAGS="$loc_accel_flags -DMBEDTLS_P256M_EXAMPLE_DRIVER_ENABLED" + make CFLAGS="$ASAN_CFLAGS $loc_accel_flags -DMBEDTLS_P256M_EXAMPLE_DRIVER_ENABLED" LDFLAGS="$ASAN_CFLAGS" # Make sure any built-in EC alg was not re-enabled by accident (additive config) #not grep mbedtls_ecdsa_ library/ecdsa.o # this is needed for deterministic ECDSA From bac6d9a703ad7e96db9f7bf8eb39e5ec6f82d335 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 7 Aug 2023 09:59:14 +0200 Subject: [PATCH 08/24] Add key management utilities to p256-m MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Those will be needed in order for the driver to implement all the PSA key management entry points (currently only implements key generation). Signed-off-by: Manuel Pégourié-Gonnard --- 3rdparty/p256-m/p256-m/p256-m.c | 45 +++++++++++++++++++++++++++++++++ 3rdparty/p256-m/p256-m/p256-m.h | 39 ++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+) diff --git a/3rdparty/p256-m/p256-m/p256-m.c b/3rdparty/p256-m/p256-m/p256-m.c index 53d306f638..de550cf8e5 100644 --- a/3rdparty/p256-m/p256-m/p256-m.c +++ b/3rdparty/p256-m/p256-m/p256-m.c @@ -1468,4 +1468,49 @@ int p256_ecdsa_verify(const uint8_t sig[64], const uint8_t pub[64], return P256_INVALID_SIGNATURE; } +/********************************************************************** + * + * Key management utilities + * + **********************************************************************/ + +int p256_validate_pubkey(const uint8_t pub[64]) +{ + uint32_t x[8], y[8]; + int ret = point_from_bytes(x, y, pub); + + return ret == 0 ? P256_SUCCESS : P256_INVALID_PUBKEY; +} + +int p256_validate_privkey(const uint8_t priv[32]) +{ + uint32_t s[8]; + int ret = scalar_from_bytes(s, priv); + zeroize(s, sizeof(s)); + + return ret == 0 ? P256_SUCCESS : P256_INVALID_PRIVKEY; +} + +int p256_public_from_private(uint8_t pub[64], const uint8_t priv[32]) +{ + int ret; + uint32_t s[8]; + + ret = scalar_from_bytes(s, priv); + if (ret != 0) + return P256_INVALID_PRIVKEY; + + /* compute and ouput the associated public key */ + uint32_t x[8], y[8]; + scalar_mult(x, y, p256_gx, p256_gy, s); + + /* the associated public key is not a secret, the scalar was */ + CT_UNPOISON(x, 32); + CT_UNPOISON(y, 32); + zeroize(s, sizeof(s)); + + point_to_bytes(pub, x, y); + return P256_SUCCESS; +} + #endif diff --git a/3rdparty/p256-m/p256-m/p256-m.h b/3rdparty/p256-m/p256-m/p256-m.h index 398c8469f0..d4b053ad0d 100644 --- a/3rdparty/p256-m/p256-m/p256-m.h +++ b/3rdparty/p256-m/p256-m/p256-m.h @@ -89,6 +89,45 @@ int p256_ecdsa_sign(uint8_t sig[64], const uint8_t priv[32], int p256_ecdsa_verify(const uint8_t sig[64], const uint8_t pub[64], const uint8_t *hash, size_t hlen); +/* + * Public key validation + * + * Note: you never need to call this function, as all other function always + * validate their input; however it's availabe if you want to validate the key + * without performing an operation. + * + * [in] pub: the public key, as two big-endian integers + * + * return: P256_SUCCESS if the key is valid + * P256_INVALID_PUBKEY if pub is invalid + */ +int p256_validate_pubkey(const uint8_t pub[64]); + +/* + * Private key validation + * + * Note: you never need to call this function, as all other function always + * validate their input; however it's availabe if you want to validate the key + * without performing an operation. + * + * [in] priv: the private key, as a big-endian integer + * + * return: P256_SUCCESS if the key is valid + * P256_INVALID_PRIVKEY if priv is invalid + */ +int p256_validate_privkey(const uint8_t priv[32]); + +/* + * Compute public key from private key + * + * [out] pub: the associated public key, as two big-endian integers + * [in] priv: the private key, as a big-endian integer + * + * return: P256_SUCCESS on success + * P256_INVALID_PRIVKEY if priv is invalid + */ +int p256_public_from_private(uint8_t pub[64], const uint8_t priv[32]); + #ifdef __cplusplus } #endif From 5424cf2e406b0b5f34953ae00806cbf4056e9106 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 7 Aug 2023 10:56:12 +0200 Subject: [PATCH 09/24] Add import_key entry point to p256-m driver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- 3rdparty/p256-m/p256-m_driver_entrypoints.c | 45 +++++++++++++++++++++ 3rdparty/p256-m/p256-m_driver_entrypoints.h | 33 +++++++++++++++ 2 files changed, 78 insertions(+) diff --git a/3rdparty/p256-m/p256-m_driver_entrypoints.c b/3rdparty/p256-m/p256-m_driver_entrypoints.c index 90dce1e18c..e48b4e8b10 100644 --- a/3rdparty/p256-m/p256-m_driver_entrypoints.c +++ b/3rdparty/p256-m/p256-m_driver_entrypoints.c @@ -24,6 +24,7 @@ #include "psa/crypto.h" #include "psa_crypto_driver_wrappers.h" #include +#include #if defined(MBEDTLS_P256M_EXAMPLE_DRIVER_ENABLED) @@ -59,6 +60,50 @@ static psa_status_t p256_to_psa_error(int ret) } } +psa_status_t p256_transparent_import_key(const psa_key_attributes_t *attributes, + const uint8_t *data, + size_t data_length, + uint8_t *key_buffer, + size_t key_buffer_size, + size_t *key_buffer_length, + size_t *bits) +{ + /* Check the key size */ + if (*bits != 0 && *bits != 256) { + return PSA_ERROR_NOT_SUPPORTED; + } + + /* Validate the key (and its type and size) */ + psa_key_type_t type = psa_get_key_type(attributes); + if (type == PSA_KEY_TYPE_ECC_PUBLIC_KEY(PSA_ECC_FAMILY_SECP_R1)) { + if (data_length != 65) { + return *bits == 0 ? PSA_ERROR_NOT_SUPPORTED : PSA_ERROR_INVALID_ARGUMENT; + } + if (p256_validate_pubkey(data + 1) != P256_SUCCESS) { + return PSA_ERROR_INVALID_ARGUMENT; + } + } else if (type == PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_FAMILY_SECP_R1)) { + if (data_length != 32) { + return *bits == 0 ? PSA_ERROR_NOT_SUPPORTED : PSA_ERROR_INVALID_ARGUMENT; + } + if (p256_validate_privkey(data) != P256_SUCCESS) { + return PSA_ERROR_INVALID_ARGUMENT; + } + } else { + return PSA_ERROR_NOT_SUPPORTED; + } + *bits = 256; + + /* We only support the export format for input, so just copy. */ + if (key_buffer_size < data_length) { + return PSA_ERROR_BUFFER_TOO_SMALL; + } + memcpy(key_buffer, data, data_length); + *key_buffer_length = data_length; + + return PSA_SUCCESS; +} + psa_status_t p256_transparent_generate_key( const psa_key_attributes_t *attributes, uint8_t *key_buffer, diff --git a/3rdparty/p256-m/p256-m_driver_entrypoints.h b/3rdparty/p256-m/p256-m_driver_entrypoints.h index 18c677a891..6eacbc3e65 100644 --- a/3rdparty/p256-m/p256-m_driver_entrypoints.h +++ b/3rdparty/p256-m/p256-m_driver_entrypoints.h @@ -29,6 +29,39 @@ #include "psa/crypto_types.h" +/** Import SECP256R1 key. + * + * \param[in] attributes The attributes of the key to use for the + * operation. + * \param[in] data The raw key material. For private keys + * this must be a big-endian integer of 32 + * bytes; for public key this must be an + * uncompressed ECPoint (65 bytes). + * \param[in] data_length The size of the raw key material. + * \param[out] key_buffer The buffer to contain the key data in + * output format upon successful return. + * \param[in] key_buffer_size Size of the \p key_buffer buffer in bytes. + * \param[out] key_buffer_length The length of the data written in \p + * key_buffer in bytes. + * \param[out] bits The bitsize of the key. + * + * \retval #PSA_SUCCESS + * Success. Keypair generated and stored in buffer. + * \retval #PSA_ERROR_NOT_SUPPORTED + * The input is not supported by this driver (not SECP256R1). + * \retval #PSA_ERROR_INVALID_ARGUMENT + * The input is invalid. + * \retval #PSA_ERROR_BUFFER_TOO_SMALL + * \p key_buffer_size is too small. + */ +psa_status_t p256_transparent_import_key(const psa_key_attributes_t *attributes, + const uint8_t *data, + size_t data_length, + uint8_t *key_buffer, + size_t key_buffer_size, + size_t *key_buffer_length, + size_t *bits); + /** Generate SECP256R1 ECC Key Pair. * Interface function which calls the p256-m key generation function and * places it in the key buffer provided by the caller (mbed TLS) in the From 18d7142efd0a186275c9dd10f6de2de128dde3be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 7 Aug 2023 11:18:05 +0200 Subject: [PATCH 10/24] Add export_public_key entry point to p256-m driver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- 3rdparty/p256-m/p256-m_driver_entrypoints.c | 34 +++++++++++++++++++++ 3rdparty/p256-m/p256-m_driver_entrypoints.h | 27 ++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/3rdparty/p256-m/p256-m_driver_entrypoints.c b/3rdparty/p256-m/p256-m_driver_entrypoints.c index e48b4e8b10..6131daec29 100644 --- a/3rdparty/p256-m/p256-m_driver_entrypoints.c +++ b/3rdparty/p256-m/p256-m_driver_entrypoints.c @@ -104,6 +104,40 @@ psa_status_t p256_transparent_import_key(const psa_key_attributes_t *attributes, return PSA_SUCCESS; } +psa_status_t p256_transparent_export_public_key(const psa_key_attributes_t *attributes, + const uint8_t *key_buffer, + size_t key_buffer_size, + uint8_t *data, + size_t data_size, + size_t *data_length) +{ + /* Is this the right curve? */ + size_t bits = psa_get_key_bits(attributes); + psa_key_type_t type = psa_get_key_type(attributes); + if (bits != 256 || type != PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_FAMILY_SECP_R1)) { + return PSA_ERROR_NOT_SUPPORTED; + } + + /* Validate input and output sizes */ + if (key_buffer_size != 32) { + return PSA_ERROR_INVALID_ARGUMENT; + } + if (data_size < 65) { + return PSA_ERROR_BUFFER_TOO_SMALL; + } + + /* Output public key in the PSA export format */ + data[0] = 0x04; + int ret = p256_public_from_private(data + 1, key_buffer); + if (ret != P256_SUCCESS) { + /* The only possible error is the private key was invalid */ + return PSA_ERROR_INVALID_ARGUMENT; + } + *data_length = 65; + + return PSA_SUCCESS; +} + psa_status_t p256_transparent_generate_key( const psa_key_attributes_t *attributes, uint8_t *key_buffer, diff --git a/3rdparty/p256-m/p256-m_driver_entrypoints.h b/3rdparty/p256-m/p256-m_driver_entrypoints.h index 6eacbc3e65..42e51d975a 100644 --- a/3rdparty/p256-m/p256-m_driver_entrypoints.h +++ b/3rdparty/p256-m/p256-m_driver_entrypoints.h @@ -62,6 +62,33 @@ psa_status_t p256_transparent_import_key(const psa_key_attributes_t *attributes, size_t *key_buffer_length, size_t *bits); +/** Export SECP256R1 public key, from the private key. + * + * \param[in] attributes The attributes of the key to use for the + * operation. + * \param[in] key_buffer The private key in the export format. + * \param[in] key_buffer_size The size of the private key in bytes. + * \param[out] data The buffer to contain the public key in + * the export format upon successful return. + * \param[in] data_size The size of the \p data buffer in bytes. + * \param[out] data_length The length written to \p data in bytes. + * + * \retval #PSA_SUCCESS + * Success. Keypair generated and stored in buffer. + * \retval #PSA_ERROR_NOT_SUPPORTED + * The input is not supported by this driver (not SECP256R1). + * \retval #PSA_ERROR_INVALID_ARGUMENT + * The input is invalid. + * \retval #PSA_ERROR_BUFFER_TOO_SMALL + * \p key_buffer_size is too small. + */ +psa_status_t p256_transparent_export_public_key(const psa_key_attributes_t *attributes, + const uint8_t *key_buffer, + size_t key_buffer_size, + uint8_t *data, + size_t data_size, + size_t *data_length); + /** Generate SECP256R1 ECC Key Pair. * Interface function which calls the p256-m key generation function and * places it in the key buffer provided by the caller (mbed TLS) in the From 92a386f24ccef8ca8b3549c8783962273a90a0f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 7 Aug 2023 12:53:33 +0200 Subject: [PATCH 11/24] Add JSON file for p256-m driver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- .../data_files/driver_jsons/driverlist.json | 2 +- .../driver_jsons/p256_transparent_driver.json | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) create mode 100644 scripts/data_files/driver_jsons/p256_transparent_driver.json diff --git a/scripts/data_files/driver_jsons/driverlist.json b/scripts/data_files/driver_jsons/driverlist.json index 50ad81604a..42c186adb5 100644 --- a/scripts/data_files/driver_jsons/driverlist.json +++ b/scripts/data_files/driver_jsons/driverlist.json @@ -1 +1 @@ -["mbedtls_test_opaque_driver.json","mbedtls_test_transparent_driver.json"] +["mbedtls_test_opaque_driver.json","mbedtls_test_transparent_driver.json","p256_transparent_driver.json"] diff --git a/scripts/data_files/driver_jsons/p256_transparent_driver.json b/scripts/data_files/driver_jsons/p256_transparent_driver.json new file mode 100644 index 0000000000..7ab1f6c847 --- /dev/null +++ b/scripts/data_files/driver_jsons/p256_transparent_driver.json @@ -0,0 +1,19 @@ +{ + "prefix": "p256", + "type": "transparent", + "mbedtls/h_condition": "defined(MBEDTLS_P256M_EXAMPLE_DRIVER_ENABLED)", + "headers": ["../3rdparty/p256-m/p256-m_driver_entrypoints.h"], + "capabilities": [ + { + "mbedtls/c_condition": "defined(MBEDTLS_P256M_EXAMPLE_DRIVER_ENABLED)", + "entry_points": ["import_key", "export_public_key"], + "algorithms": ["PSA_ALG_ECDH", "PSA_ALG_ECDSA(PSA_ALG_ANY_HASH)"], + "key_types": [ + "PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_FAMILY_SECP_R1)", + "PSA_KEY_TYPE_ECC_PUBLIC_KEY(PSA_ECC_FAMILY_SECP_R1)" + ], + "key_sizes": [256], + "fallback": false + } + ] +} From 25b45db3d8eb782096028e13282948ee82466de9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 8 Aug 2023 11:06:21 +0200 Subject: [PATCH 12/24] Disable ECP_C in component with p256-m driver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Builds, but 20 test cases failing in test_suite_psa_crypto, to be addressed in future commits. Signed-off-by: Manuel Pégourié-Gonnard --- tests/scripts/all.sh | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 3482df33a5..a9efa5108b 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -2823,19 +2823,18 @@ component_test_tfm_config_p256m_driver_accel_ec () { # Disable all the features that auto-enable ECP_LIGHT (see build_info.h) scripts/config.py -f "$CRYPTO_CONFIG_H" unset PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_DERIVE + # Disable deterministic ECDSA as p256-m only does randomized + scripts/config.py -f "$CRYPTO_CONFIG_H" unset PSA_WANT_ALG_DETERMINISTIC_ECDSA # Add missing symbols from "tfm_mbedcrypto_config_profile_medium.h" # # - USE_PSA_CRYPTO for PK_HAVE_ECC_KEYS echo "#define MBEDTLS_USE_PSA_CRYPTO" >> "$CONFIG_H" - # - ECP_C and BIGNUM because P256M does not have support for import and export - # of keys so we need the builtin support for that - echo "#define MBEDTLS_ECP_C" >> "$CONFIG_H" - echo "#define MBEDTLS_BIGNUM_C" >> "$CONFIG_H" - # - ASN1_[PARSE/WRITE]_C and OID_C found by check_config.h + # - ASN1_[PARSE/WRITE]_C found by check_config.h for pkparse/pkwrite echo "#define MBEDTLS_ASN1_PARSE_C" >> "$CONFIG_H" echo "#define MBEDTLS_ASN1_WRITE_C" >> "$CONFIG_H" - echo "#define MBEDTLS_OID_C" >> "$CONFIG_H" + # - MD_C for HKDF_C + echo "#define MBEDTLS_MD_C" >> "$CONFIG_H" # Set the list of accelerated components in order to remove them from # builtin support. We don't set IMPORT and EXPORT because P256M does not @@ -2843,6 +2842,8 @@ component_test_tfm_config_p256m_driver_accel_ec () { loc_accel_list="ALG_ECDSA \ ALG_ECDH \ KEY_TYPE_ECC_KEY_PAIR_BASIC \ + KEY_TYPE_ECC_KEY_PAIR_IMPORT \ + KEY_TYPE_ECC_KEY_PAIR_EXPORT \ KEY_TYPE_ECC_KEY_PAIR_GENERATE \ KEY_TYPE_ECC_PUBLIC_KEY" loc_accel_flags="$( echo "$loc_accel_list" | sed 's/[^ ]* */-DMBEDTLS_PSA_ACCEL_&/g' )" From f0251e0824f173c18ddb9183f34731908eac50d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 8 Aug 2023 12:23:42 +0200 Subject: [PATCH 13/24] Improve error codes in p256-m driver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix 19 out of 20 errors in test_suite_psa_crypto mentioned in the previous commit. The remaining error will be fix in the next commit. Signed-off-by: Manuel Pégourié-Gonnard --- 3rdparty/p256-m/p256-m_driver_entrypoints.c | 77 ++++++++++----------- 1 file changed, 36 insertions(+), 41 deletions(-) diff --git a/3rdparty/p256-m/p256-m_driver_entrypoints.c b/3rdparty/p256-m/p256-m_driver_entrypoints.c index 6131daec29..8c8f85cb53 100644 --- a/3rdparty/p256-m/p256-m_driver_entrypoints.c +++ b/3rdparty/p256-m/p256-m_driver_entrypoints.c @@ -118,9 +118,9 @@ psa_status_t p256_transparent_export_public_key(const psa_key_attributes_t *attr return PSA_ERROR_NOT_SUPPORTED; } - /* Validate input and output sizes */ + /* Validate sizes, as p256-m expects fixed-size buffers */ if (key_buffer_size != 32) { - return PSA_ERROR_INVALID_ARGUMENT; + return PSA_ERROR_CORRUPTION_DETECTED; } if (data_size < 65) { return PSA_ERROR_BUFFER_TOO_SMALL; @@ -129,13 +129,11 @@ psa_status_t p256_transparent_export_public_key(const psa_key_attributes_t *attr /* Output public key in the PSA export format */ data[0] = 0x04; int ret = p256_public_from_private(data + 1, key_buffer); - if (ret != P256_SUCCESS) { - /* The only possible error is the private key was invalid */ - return PSA_ERROR_INVALID_ARGUMENT; + if (ret == P256_SUCCESS) { + *data_length = 65; } - *data_length = 65; - return PSA_SUCCESS; + return p256_to_psa_error(ret); } psa_status_t p256_transparent_generate_key( @@ -148,13 +146,9 @@ psa_status_t p256_transparent_generate_key( * of driver entry-points. (void) used to avoid compiler warning. */ (void) attributes; - psa_status_t status = PSA_ERROR_NOT_SUPPORTED; - - /* - * p256-m generates a 32 byte private key, and expects to write to a buffer - * that is of that size. */ + /* Validate sizes, as p256-m expects fixed-size buffers */ if (key_buffer_size != 32) { - return status; + return PSA_ERROR_BUFFER_TOO_SMALL; } /* @@ -164,13 +158,12 @@ psa_status_t p256_transparent_generate_key( * function as an argument. */ uint8_t public_key_buffer[64]; - status = p256_to_psa_error( - p256_gen_keypair(key_buffer, public_key_buffer)); - if (status == PSA_SUCCESS) { + int ret = p256_gen_keypair(key_buffer, public_key_buffer); + if (ret == P256_SUCCESS) { *key_buffer_length = 32; } - return status; + return p256_to_psa_error(ret); } psa_status_t p256_transparent_key_agreement( @@ -190,25 +183,23 @@ psa_status_t p256_transparent_key_agreement( (void) attributes; (void) alg; - /* - * Check that private key = 32 bytes, peer public key = 65 bytes, - * and that the shared secret buffer is big enough. */ - psa_status_t status = PSA_ERROR_INVALID_ARGUMENT; - if (key_buffer_size != 32 || shared_secret_size < 32 || - peer_key_length != 65) { - return status; + /* Validate sizes, as p256-m expects fixed-size buffers */ + if (key_buffer_size != 32 || peer_key_length != 65) { + return PSA_ERROR_INVALID_ARGUMENT; + } + if (shared_secret_size < 32) { + return PSA_ERROR_BUFFER_TOO_SMALL; } /* We add 1 to peer_key pointer to omit the leading byte of the public key * representation (0x04). See information about PSA key formats at the top * of the file. */ - status = p256_to_psa_error( - p256_ecdh_shared_secret(shared_secret, key_buffer, peer_key+1)); - if (status == PSA_SUCCESS) { + int ret = p256_ecdh_shared_secret(shared_secret, key_buffer, peer_key + 1); + if (ret == P256_SUCCESS) { *shared_secret_length = 32; } - return status; + return p256_to_psa_error(ret); } psa_status_t p256_transparent_sign_hash( @@ -228,18 +219,20 @@ psa_status_t p256_transparent_sign_hash( (void) attributes; (void) alg; - psa_status_t status = PSA_ERROR_NOT_SUPPORTED; - if (key_buffer_size != 32 || signature_size < 64) { - return status; + /* Validate sizes, as p256-m expects fixed-size buffers */ + if (key_buffer_size != 32) { + return PSA_ERROR_CORRUPTION_DETECTED; + } + if (signature_size < 64) { + return PSA_ERROR_BUFFER_TOO_SMALL; } - status = p256_to_psa_error( - p256_ecdsa_sign(signature, key_buffer, hash, hash_length)); - if (status == PSA_SUCCESS) { + int ret = p256_ecdsa_sign(signature, key_buffer, hash, hash_length); + if (ret == P256_SUCCESS) { *signature_length = 64; } - return status; + return p256_to_psa_error(ret); } /* This function expects the key buffer to contain a 65 byte public key, @@ -252,19 +245,21 @@ static psa_status_t p256_verify_hash_with_public_key( const uint8_t *signature, size_t signature_length) { - psa_status_t status = PSA_ERROR_NOT_SUPPORTED; - if (key_buffer_size != 65 || signature_length != 64 || *key_buffer != 0x04) { - return status; + /* Validate sizes, as p256-m expects fixed-size buffers */ + if (key_buffer_size != 65 || *key_buffer != 0x04) { + return PSA_ERROR_CORRUPTION_DETECTED; + } + if (signature_length != 64) { + return PSA_ERROR_INVALID_SIGNATURE; } /* We add 1 to public_key_buffer pointer to omit the leading byte of the * public key representation (0x04). See information about PSA key formats * at the top of the file. */ const uint8_t *public_key_buffer = key_buffer + 1; - status = p256_to_psa_error( - p256_ecdsa_verify(signature, public_key_buffer, hash, hash_length)); + int ret = p256_ecdsa_verify(signature, public_key_buffer, hash, hash_length); - return status; + return p256_to_psa_error(ret); } psa_status_t p256_transparent_verify_hash( From 0509b5878ca57f818b45f7f439b803b96690927d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 8 Aug 2023 12:47:56 +0200 Subject: [PATCH 14/24] Fix INVALID vs NOT_SUPPORTED issue in test suite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes the last remaining failure. Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_psa_crypto.function | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/tests/suites/test_suite_psa_crypto.function b/tests/suites/test_suite_psa_crypto.function index 2396590b2d..02cc5efe3d 100644 --- a/tests/suites/test_suite_psa_crypto.function +++ b/tests/suites/test_suite_psa_crypto.function @@ -1366,7 +1366,21 @@ void import_with_data(data_t *data, int type_arg, psa_set_key_bits(&attributes, attr_bits); status = psa_import_key(&attributes, data->x, data->len, &key); - TEST_EQUAL(status, expected_status); + /* When expecting INVALID_ARGUMENT, also accept NOT_SUPPORTED. + * + * This can happen with a type supported only by a driver: + * - the driver sees the invalid data (for example wrong size) and thinks + * "well perhaps this is a key size I don't support" so it returns + * NOT_SUPPORTED which is correct at this point; + * - we fallback to built-ins, which don't support this type, so return + * NOT_SUPPORTED which again is correct at this point. + */ + if (expected_status == PSA_ERROR_INVALID_ARGUMENT && + status == PSA_ERROR_NOT_SUPPORTED) { + ; // OK + } else { + TEST_EQUAL(status, expected_status); + } if (status != PSA_SUCCESS) { goto exit; } From 96839e745009e441ec5faf0099cf6289e3d43ba9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 8 Aug 2023 13:01:29 +0200 Subject: [PATCH 15/24] Move common things to common function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These should be shared between ref and accel, for meaningful coverage comparison. Signed-off-by: Manuel Pégourié-Gonnard --- tests/scripts/all.sh | 43 ++++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index a9efa5108b..c0811933e4 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -2798,20 +2798,36 @@ common_tfm_config () { scripts/config.py unset MBEDTLS_AES_SETKEY_DEC_ALT # We have an OS that provides entropy, use it scripts/config.py unset MBEDTLS_NO_PLATFORM_ENTROPY - # Disable buffer allocator and use dynamic memory for calloc/free - scripts/config.py unset MBEDTLS_MEMORY_BUFFER_ALLOC_C # Other config adjustments to make the tests pass. - # Those are a surprise and should be investigated and fixed. + # Those should probably be adopted upstream. # + # - USE_PSA_CRYPTO for PK_HAVE_ECC_KEYS + echo "#define MBEDTLS_USE_PSA_CRYPTO" >> "$CONFIG_H" # pkparse.c and pkwrite.c fail to link without this echo "#define MBEDTLS_OID_C" >> "$CONFIG_H" + # - ASN1_[PARSE/WRITE]_C found by check_config.h for pkparse/pkwrite + echo "#define MBEDTLS_ASN1_PARSE_C" >> "$CONFIG_H" + echo "#define MBEDTLS_ASN1_WRITE_C" >> "$CONFIG_H" + # - MD_C for HKDF_C + echo "#define MBEDTLS_MD_C" >> "$CONFIG_H" # Config adjustements for better test coverage in our environment. # These are not needed just to build and pass tests. # # Enable filesystem I/O for the benefit of PK parse/write tests. echo "#define MBEDTLS_FS_IO" >> "$CONFIG_H" + # Disable this for maximal ASan efficiency + scripts/config.py unset MBEDTLS_MEMORY_BUFFER_ALLOC_C + + # Config adjustements for features that are not supported + # when using only drivers / by p256-m + # + # Disable all the features that auto-enable ECP_LIGHT (see build_info.h) + scripts/config.py -f "$CRYPTO_CONFIG_H" unset PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_DERIVE + # Disable deterministic ECDSA as p256-m only does randomized + scripts/config.py -f "$CRYPTO_CONFIG_H" unset PSA_WANT_ALG_DETERMINISTIC_ECDSA + } # Keep this in sync with component_test_tfm_config() as they are both meant @@ -2821,21 +2837,6 @@ component_test_tfm_config_p256m_driver_accel_ec () { common_tfm_config - # Disable all the features that auto-enable ECP_LIGHT (see build_info.h) - scripts/config.py -f "$CRYPTO_CONFIG_H" unset PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_DERIVE - # Disable deterministic ECDSA as p256-m only does randomized - scripts/config.py -f "$CRYPTO_CONFIG_H" unset PSA_WANT_ALG_DETERMINISTIC_ECDSA - - # Add missing symbols from "tfm_mbedcrypto_config_profile_medium.h" - # - # - USE_PSA_CRYPTO for PK_HAVE_ECC_KEYS - echo "#define MBEDTLS_USE_PSA_CRYPTO" >> "$CONFIG_H" - # - ASN1_[PARSE/WRITE]_C found by check_config.h for pkparse/pkwrite - echo "#define MBEDTLS_ASN1_PARSE_C" >> "$CONFIG_H" - echo "#define MBEDTLS_ASN1_WRITE_C" >> "$CONFIG_H" - # - MD_C for HKDF_C - echo "#define MBEDTLS_MD_C" >> "$CONFIG_H" - # Set the list of accelerated components in order to remove them from # builtin support. We don't set IMPORT and EXPORT because P256M does not # support these operations. @@ -2852,14 +2853,14 @@ component_test_tfm_config_p256m_driver_accel_ec () { make CFLAGS="$ASAN_CFLAGS $loc_accel_flags -DMBEDTLS_P256M_EXAMPLE_DRIVER_ENABLED" LDFLAGS="$ASAN_CFLAGS" # Make sure any built-in EC alg was not re-enabled by accident (additive config) - #not grep mbedtls_ecdsa_ library/ecdsa.o # this is needed for deterministic ECDSA + not grep mbedtls_ecdsa_ library/ecdsa.o not grep mbedtls_ecdh_ library/ecdh.o not grep mbedtls_ecjpake_ library/ecjpake.o # Also ensure that ECP, RSA, DHM or BIGNUM modules were not re-enabled - #not grep mbedtls_ecp_ library/ecp.o # this is needed for import/export EC keys (explained above) + not grep mbedtls_ecp_ library/ecp.o not grep mbedtls_rsa_ library/rsa.o not grep mbedtls_dhm_ library/dhm.o - #not grep mbedtls_mpi_ library/bignum.o # this is needed from ECP module + not grep mbedtls_mpi_ library/bignum.o # Run the tests msg "test: TF-M config + p256m driver + accel ECDH(E)/ECDSA" From e9d97976b26d8669ad93b1224245563704c92383 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 8 Aug 2023 18:34:47 +0200 Subject: [PATCH 16/24] Update list of ignored tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- tests/scripts/analyze_outcomes.py | 44 ++++++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 4 deletions(-) diff --git a/tests/scripts/analyze_outcomes.py b/tests/scripts/analyze_outcomes.py index ac97a99dd9..8648aec950 100755 --- a/tests/scripts/analyze_outcomes.py +++ b/tests/scripts/analyze_outcomes.py @@ -407,9 +407,20 @@ TASKS = { 'args': { 'component_ref': 'test_tfm_config', 'component_driver': 'test_tfm_config_p256m_driver_accel_ec', - # Ignore test suites for the modules that are disabled in the - # accelerated test case. - 'ignored_suites': ['ecdh', 'ecdsa'], + 'ignored_suites': [ + # Ignore test suites for the modules that are disabled in the + # accelerated test case. + 'ecp', + 'ecdsa', + 'ecdh', + 'ecjpake', + 'bignum_core', + 'bignum_random', + 'bignum_mod', + 'bignum_mod_raw', + 'bignum.generated', + 'bignum.misc', + ], 'ignored_tests': { # Ignore all tests that require DERIVE support which is disabled # in the driver version @@ -456,9 +467,34 @@ TASKS = { 'PSA key derivation: bits=7 invalid for ECC SECT_R2 (ECC enabled)', 'PSA raw key agreement: ECDH SECP256R1 (RFC 5903)', ], + 'test_suite_random': [ + 'PSA classic wrapper: ECDSA signature (SECP256R1)', + ], 'test_suite_psa_crypto_pake': [ 'PSA PAKE: ecjpake size macros', - ] + ], + 'test_suite_asn1parse': [ + # This test depends on BIGNUM_C + 'INTEGER too large for mpi', + ], + 'test_suite_asn1write': [ + # Following tests depends on BIGNUM_C + 'ASN.1 Write mpi 0 (1 limb)', + 'ASN.1 Write mpi 0 (null)', + 'ASN.1 Write mpi 0x100', + 'ASN.1 Write mpi 0x7f', + 'ASN.1 Write mpi 0x7f with leading 0 limb', + 'ASN.1 Write mpi 0x80', + 'ASN.1 Write mpi 0x80 with leading 0 limb', + 'ASN.1 Write mpi 0xff', + 'ASN.1 Write mpi 1', + 'ASN.1 Write mpi, 127*8 bits', + 'ASN.1 Write mpi, 127*8+1 bits', + 'ASN.1 Write mpi, 127*8-1 bits', + 'ASN.1 Write mpi, 255*8 bits', + 'ASN.1 Write mpi, 255*8-1 bits', + 'ASN.1 Write mpi, 256*8-1 bits', + ], } } } From 138bdb6b17a644ec326b21d34e5cf40ab5611cc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 9 Aug 2023 11:18:49 +0200 Subject: [PATCH 17/24] Add comment to p256-m driver JSON file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- scripts/data_files/driver_jsons/p256_transparent_driver.json | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/data_files/driver_jsons/p256_transparent_driver.json b/scripts/data_files/driver_jsons/p256_transparent_driver.json index 7ab1f6c847..97c11f97bd 100644 --- a/scripts/data_files/driver_jsons/p256_transparent_driver.json +++ b/scripts/data_files/driver_jsons/p256_transparent_driver.json @@ -6,6 +6,7 @@ "capabilities": [ { "mbedtls/c_condition": "defined(MBEDTLS_P256M_EXAMPLE_DRIVER_ENABLED)", + "_comment_entry_points": "This is not the complete list of entry points supported by this driver, only those that are currently supported in JSON. See docs/psa-driver-example-and-guide.md", "entry_points": ["import_key", "export_public_key"], "algorithms": ["PSA_ALG_ECDH", "PSA_ALG_ECDSA(PSA_ALG_ANY_HASH)"], "key_types": [ From ba63e0ce34086fb439960478e4611a03bc9b73a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 9 Aug 2023 11:53:09 +0200 Subject: [PATCH 18/24] Use macros for sizes in p256-m driver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- 3rdparty/p256-m/p256-m_driver_entrypoints.c | 58 ++++++++++++--------- 1 file changed, 34 insertions(+), 24 deletions(-) diff --git a/3rdparty/p256-m/p256-m_driver_entrypoints.c b/3rdparty/p256-m/p256-m_driver_entrypoints.c index 8c8f85cb53..0ca5583c9a 100644 --- a/3rdparty/p256-m/p256-m_driver_entrypoints.c +++ b/3rdparty/p256-m/p256-m_driver_entrypoints.c @@ -41,7 +41,17 @@ * keys is only 64 bytes; the same as PSA but without the leading byte (0x04). * Hence, when passing public keys from PSA to p256-m, the leading byte is * removed. + * + * Shared secret and signature have the same format between PSA and p256-m. */ +#define PSA_PUBKEY_SIZE 65 +#define PSA_PUBKEY_HEADER_BYTE 0x04 +#define P256_PUBKEY_SIZE 64 +#define PRIVKEY_SIZE 32 +#define SHARED_SECRET_SIZE 32 +#define SIGNATURE_SIZE 64 + +#define CURVE_BITS 256 /* Convert between p256-m and PSA error codes */ static psa_status_t p256_to_psa_error(int ret) @@ -69,21 +79,21 @@ psa_status_t p256_transparent_import_key(const psa_key_attributes_t *attributes, size_t *bits) { /* Check the key size */ - if (*bits != 0 && *bits != 256) { + if (*bits != 0 && *bits != CURVE_BITS) { return PSA_ERROR_NOT_SUPPORTED; } /* Validate the key (and its type and size) */ psa_key_type_t type = psa_get_key_type(attributes); if (type == PSA_KEY_TYPE_ECC_PUBLIC_KEY(PSA_ECC_FAMILY_SECP_R1)) { - if (data_length != 65) { + if (data_length != PSA_PUBKEY_SIZE) { return *bits == 0 ? PSA_ERROR_NOT_SUPPORTED : PSA_ERROR_INVALID_ARGUMENT; } if (p256_validate_pubkey(data + 1) != P256_SUCCESS) { return PSA_ERROR_INVALID_ARGUMENT; } } else if (type == PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_FAMILY_SECP_R1)) { - if (data_length != 32) { + if (data_length != PRIVKEY_SIZE) { return *bits == 0 ? PSA_ERROR_NOT_SUPPORTED : PSA_ERROR_INVALID_ARGUMENT; } if (p256_validate_privkey(data) != P256_SUCCESS) { @@ -92,7 +102,7 @@ psa_status_t p256_transparent_import_key(const psa_key_attributes_t *attributes, } else { return PSA_ERROR_NOT_SUPPORTED; } - *bits = 256; + *bits = CURVE_BITS; /* We only support the export format for input, so just copy. */ if (key_buffer_size < data_length) { @@ -114,23 +124,23 @@ psa_status_t p256_transparent_export_public_key(const psa_key_attributes_t *attr /* Is this the right curve? */ size_t bits = psa_get_key_bits(attributes); psa_key_type_t type = psa_get_key_type(attributes); - if (bits != 256 || type != PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_FAMILY_SECP_R1)) { + if (bits != CURVE_BITS || type != PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_FAMILY_SECP_R1)) { return PSA_ERROR_NOT_SUPPORTED; } /* Validate sizes, as p256-m expects fixed-size buffers */ - if (key_buffer_size != 32) { + if (key_buffer_size != PRIVKEY_SIZE) { return PSA_ERROR_CORRUPTION_DETECTED; } - if (data_size < 65) { + if (data_size < PSA_PUBKEY_SIZE) { return PSA_ERROR_BUFFER_TOO_SMALL; } /* Output public key in the PSA export format */ - data[0] = 0x04; + data[0] = PSA_PUBKEY_HEADER_BYTE; int ret = p256_public_from_private(data + 1, key_buffer); if (ret == P256_SUCCESS) { - *data_length = 65; + *data_length = PSA_PUBKEY_SIZE; } return p256_to_psa_error(ret); @@ -147,7 +157,7 @@ psa_status_t p256_transparent_generate_key( (void) attributes; /* Validate sizes, as p256-m expects fixed-size buffers */ - if (key_buffer_size != 32) { + if (key_buffer_size != PRIVKEY_SIZE) { return PSA_ERROR_BUFFER_TOO_SMALL; } @@ -156,11 +166,11 @@ psa_status_t p256_transparent_generate_key( * keys. Allocate a buffer to which the public key will be written. The * private key will be written to key_buffer, which is passed to this * function as an argument. */ - uint8_t public_key_buffer[64]; + uint8_t public_key_buffer[P256_PUBKEY_SIZE]; int ret = p256_gen_keypair(key_buffer, public_key_buffer); if (ret == P256_SUCCESS) { - *key_buffer_length = 32; + *key_buffer_length = PRIVKEY_SIZE; } return p256_to_psa_error(ret); @@ -184,10 +194,10 @@ psa_status_t p256_transparent_key_agreement( (void) alg; /* Validate sizes, as p256-m expects fixed-size buffers */ - if (key_buffer_size != 32 || peer_key_length != 65) { + if (key_buffer_size != PRIVKEY_SIZE || peer_key_length != PSA_PUBKEY_SIZE) { return PSA_ERROR_INVALID_ARGUMENT; } - if (shared_secret_size < 32) { + if (shared_secret_size < SHARED_SECRET_SIZE) { return PSA_ERROR_BUFFER_TOO_SMALL; } @@ -196,7 +206,7 @@ psa_status_t p256_transparent_key_agreement( * of the file. */ int ret = p256_ecdh_shared_secret(shared_secret, key_buffer, peer_key + 1); if (ret == P256_SUCCESS) { - *shared_secret_length = 32; + *shared_secret_length = SHARED_SECRET_SIZE; } return p256_to_psa_error(ret); @@ -220,22 +230,22 @@ psa_status_t p256_transparent_sign_hash( (void) alg; /* Validate sizes, as p256-m expects fixed-size buffers */ - if (key_buffer_size != 32) { + if (key_buffer_size != PRIVKEY_SIZE) { return PSA_ERROR_CORRUPTION_DETECTED; } - if (signature_size < 64) { + if (signature_size < SIGNATURE_SIZE) { return PSA_ERROR_BUFFER_TOO_SMALL; } int ret = p256_ecdsa_sign(signature, key_buffer, hash, hash_length); if (ret == P256_SUCCESS) { - *signature_length = 64; + *signature_length = SIGNATURE_SIZE; } return p256_to_psa_error(ret); } -/* This function expects the key buffer to contain a 65 byte public key, +/* This function expects the key buffer to contain a PSA public key, * as exported by psa_export_public_key() */ static psa_status_t p256_verify_hash_with_public_key( const uint8_t *key_buffer, @@ -246,10 +256,10 @@ static psa_status_t p256_verify_hash_with_public_key( size_t signature_length) { /* Validate sizes, as p256-m expects fixed-size buffers */ - if (key_buffer_size != 65 || *key_buffer != 0x04) { + if (key_buffer_size != PSA_PUBKEY_SIZE || *key_buffer != PSA_PUBKEY_HEADER_BYTE) { return PSA_ERROR_CORRUPTION_DETECTED; } - if (signature_length != 64) { + if (signature_length != SIGNATURE_SIZE) { return PSA_ERROR_INVALID_SIGNATURE; } @@ -277,10 +287,10 @@ psa_status_t p256_transparent_verify_hash( (void) alg; psa_status_t status; - uint8_t public_key_buffer[65]; - size_t public_key_buffer_size = 65; + uint8_t public_key_buffer[PSA_PUBKEY_SIZE]; + size_t public_key_buffer_size = PSA_PUBKEY_SIZE; - size_t public_key_length = 65; + size_t public_key_length = PSA_PUBKEY_SIZE; /* As p256-m doesn't require dynamic allocation, we want to avoid it in * the entrypoint functions as well. psa_driver_wrapper_export_public_key() * requires size_t*, so we use a pointer to a stack variable. */ From f7298cd3974a181d8f76463b13f682ddbf63474c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 18 Sep 2023 09:55:24 +0200 Subject: [PATCH 19/24] Fix some issues in comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ranging from typos to outdated comment contradicting the code. Signed-off-by: Manuel Pégourié-Gonnard --- 3rdparty/p256-m/p256-m/p256-m.h | 4 ++-- include/mbedtls/pk.h | 24 ++++++++++++------------ tests/scripts/all.sh | 7 +++---- 3 files changed, 17 insertions(+), 18 deletions(-) diff --git a/3rdparty/p256-m/p256-m/p256-m.h b/3rdparty/p256-m/p256-m/p256-m.h index d4b053ad0d..28d319f394 100644 --- a/3rdparty/p256-m/p256-m/p256-m.h +++ b/3rdparty/p256-m/p256-m/p256-m.h @@ -92,7 +92,7 @@ int p256_ecdsa_verify(const uint8_t sig[64], const uint8_t pub[64], /* * Public key validation * - * Note: you never need to call this function, as all other function always + * Note: you never need to call this function, as all other functions always * validate their input; however it's availabe if you want to validate the key * without performing an operation. * @@ -106,7 +106,7 @@ int p256_validate_pubkey(const uint8_t pub[64]); /* * Private key validation * - * Note: you never need to call this function, as all other function always + * Note: you never need to call this function, as all other functions always * validate their input; however it's availabe if you want to validate the key * without performing an operation. * diff --git a/include/mbedtls/pk.h b/include/mbedtls/pk.h index 28ec29d6ba..48e331a5bc 100644 --- a/include/mbedtls/pk.h +++ b/include/mbedtls/pk.h @@ -173,11 +173,11 @@ typedef struct mbedtls_pk_rsassa_pss_options { /* Internal helper to define which fields in the pk_context structure below * should be used for EC keys: legacy ecp_keypair or the raw (PSA friendly) - * format. It should be noticed that this only affects how data is stored, not + * format. It should be noted that this only affects how data is stored, not * which functions are used for various operations. The overall picture looks * like this: - * - if USE_PSA is not defined and ECP_C is then use ecp_keypair data structure - * and legacy functions + * - if USE_PSA is not defined and ECP_C is defined then use ecp_keypair data + * structure and legacy functions * - if USE_PSA is defined and * - if ECP_C then use ecp_keypair structure, convert data to a PSA friendly * format and use PSA functions @@ -185,13 +185,13 @@ typedef struct mbedtls_pk_rsassa_pss_options { * * The main reason for the "intermediate" (USE_PSA + ECP_C) above is that as long * as ECP_C is defined mbedtls_pk_ec() gives the user a read/write access to the - * ecp_keypair structure inside the pk_context so he/she can modify it using + * ecp_keypair structure inside the pk_context so they can modify it using * ECP functions which are not under PK module's control. */ #if defined(MBEDTLS_USE_PSA_CRYPTO) && defined(PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY) && \ !defined(MBEDTLS_ECP_C) #define MBEDTLS_PK_USE_PSA_EC_DATA -#endif /* MBEDTLS_USE_PSA_CRYPTO && !MBEDTLS_ECP_C */ +#endif /* Helper symbol to state that the PK module has support for EC keys. This * can either be provided through the legacy ECP solution or through the @@ -202,11 +202,11 @@ typedef struct mbedtls_pk_rsassa_pss_options { /* Internal helper to define which fields in the pk_context structure below * should be used for EC keys: legacy ecp_keypair or the raw (PSA friendly) - * format. It should be noted that this only affect how data is stored, not + * format. It should be noted that this only affects how data is stored, not * which functions are used for various operations. The overall picture looks * like this: - * - if USE_PSA is not defined and ECP_C is then use ecp_keypair data structure - * and legacy functions + * - if USE_PSA is not defined and ECP_C is defined then use ecp_keypair data + * structure and legacy functions * - if USE_PSA is defined and * - if ECP_C then use ecp_keypair structure, convert data to a PSA friendly * format and use PSA functions @@ -220,11 +220,11 @@ typedef struct mbedtls_pk_rsassa_pss_options { #if defined(MBEDTLS_USE_PSA_CRYPTO) && defined(PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY) && \ !defined(MBEDTLS_ECP_C) #define MBEDTLS_PK_USE_PSA_EC_DATA -#endif /* MBEDTLS_USE_PSA_CRYPTO && !MBEDTLS_ECP_C */ +#endif /* Internal helper to define which fields in the pk_context structure below * should be used for EC keys: legacy ecp_keypair or the raw (PSA friendly) - * format. It should be noticed that this only affect how data is stored, not + * format. It should be noted that this only affects how data is stored, not * which functions are used for various operations. The overall picture looks * like this: * - if USE_PSA is not defined and ECP_C is then use ecp_keypair data structure @@ -236,13 +236,13 @@ typedef struct mbedtls_pk_rsassa_pss_options { * * The main reason for the "intermediate" (USE_PSA + ECP_C) above is that as long * as ECP_C is defined mbedtls_pk_ec() gives the user a read/write access to the - * ecp_keypair structure inside the pk_context so he/she can modify it using + * ecp_keypair structure inside the pk_context so they can modify it using * ECP functions which are not under PK module's control. */ #if defined(MBEDTLS_USE_PSA_CRYPTO) && defined(PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY) && \ !defined(MBEDTLS_ECP_C) #define MBEDTLS_PK_USE_PSA_EC_DATA -#endif /* MBEDTLS_USE_PSA_CRYPTO && !MBEDTLS_ECP_C */ +#endif /** * \brief Types for interfacing with the debug module diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index c0811933e4..c1fd50c29f 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -2812,7 +2812,7 @@ common_tfm_config () { # - MD_C for HKDF_C echo "#define MBEDTLS_MD_C" >> "$CONFIG_H" - # Config adjustements for better test coverage in our environment. + # Config adjustments for better test coverage in our environment. # These are not needed just to build and pass tests. # # Enable filesystem I/O for the benefit of PK parse/write tests. @@ -2820,7 +2820,7 @@ common_tfm_config () { # Disable this for maximal ASan efficiency scripts/config.py unset MBEDTLS_MEMORY_BUFFER_ALLOC_C - # Config adjustements for features that are not supported + # Config adjustments for features that are not supported # when using only drivers / by p256-m # # Disable all the features that auto-enable ECP_LIGHT (see build_info.h) @@ -2838,8 +2838,7 @@ component_test_tfm_config_p256m_driver_accel_ec () { common_tfm_config # Set the list of accelerated components in order to remove them from - # builtin support. We don't set IMPORT and EXPORT because P256M does not - # support these operations. + # builtin support. loc_accel_list="ALG_ECDSA \ ALG_ECDH \ KEY_TYPE_ECC_KEY_PAIR_BASIC \ From 4f119b8f211abd6a8feba0e560281afd14092304 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 18 Sep 2023 09:57:04 +0200 Subject: [PATCH 20/24] Remove extra copies of a block of comment/define MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Not sure how it happened, but this block was not just duplicated, but triplicated. Keep only the first copy: the one before the code that uses the macro being defined. Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/pk.h | 44 -------------------------------------------- 1 file changed, 44 deletions(-) diff --git a/include/mbedtls/pk.h b/include/mbedtls/pk.h index 48e331a5bc..aea602be79 100644 --- a/include/mbedtls/pk.h +++ b/include/mbedtls/pk.h @@ -200,50 +200,6 @@ typedef struct mbedtls_pk_rsassa_pss_options { #define MBEDTLS_PK_HAVE_ECC_KEYS #endif /* MBEDTLS_PK_USE_PSA_EC_DATA || MBEDTLS_ECP_C */ -/* Internal helper to define which fields in the pk_context structure below - * should be used for EC keys: legacy ecp_keypair or the raw (PSA friendly) - * format. It should be noted that this only affects how data is stored, not - * which functions are used for various operations. The overall picture looks - * like this: - * - if USE_PSA is not defined and ECP_C is defined then use ecp_keypair data - * structure and legacy functions - * - if USE_PSA is defined and - * - if ECP_C then use ecp_keypair structure, convert data to a PSA friendly - * format and use PSA functions - * - if !ECP_C then use new raw data and PSA functions directly. - * - * The main reason for the "intermediate" (USE_PSA + ECP_C) above is that as long - * as ECP_C is defined mbedtls_pk_ec() gives the user read/write access to the - * ecp_keypair structure inside the pk_context so they can modify it using - * ECP functions which are not under the PK module's control. - */ -#if defined(MBEDTLS_USE_PSA_CRYPTO) && defined(PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY) && \ - !defined(MBEDTLS_ECP_C) -#define MBEDTLS_PK_USE_PSA_EC_DATA -#endif - -/* Internal helper to define which fields in the pk_context structure below - * should be used for EC keys: legacy ecp_keypair or the raw (PSA friendly) - * format. It should be noted that this only affects how data is stored, not - * which functions are used for various operations. The overall picture looks - * like this: - * - if USE_PSA is not defined and ECP_C is then use ecp_keypair data structure - * and legacy functions - * - if USE_PSA is defined and - * - if ECP_C then use ecp_keypair structure, convert data to a PSA friendly - * format and use PSA functions - * - if !ECP_C then use new raw data and PSA functions directly. - * - * The main reason for the "intermediate" (USE_PSA + ECP_C) above is that as long - * as ECP_C is defined mbedtls_pk_ec() gives the user a read/write access to the - * ecp_keypair structure inside the pk_context so they can modify it using - * ECP functions which are not under PK module's control. - */ -#if defined(MBEDTLS_USE_PSA_CRYPTO) && defined(PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY) && \ - !defined(MBEDTLS_ECP_C) -#define MBEDTLS_PK_USE_PSA_EC_DATA -#endif - /** * \brief Types for interfacing with the debug module */ From fbea9d2e7d004dfdd5f457d9a3f0d8d6581f4405 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 20 Sep 2023 09:22:29 +0200 Subject: [PATCH 21/24] Improve return code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CORRUPTION_DETECTED should be reserved for cases that are impossible, short of physical corruption during execution or a major bug in the code. We shouldn't use this for the kind of mistakes that can happen during configuration or integration, such as calling a driver on a key type that it doesn't support. Signed-off-by: Manuel Pégourié-Gonnard --- 3rdparty/p256-m/p256-m_driver_entrypoints.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/3rdparty/p256-m/p256-m_driver_entrypoints.c b/3rdparty/p256-m/p256-m_driver_entrypoints.c index 0ca5583c9a..b75c06c669 100644 --- a/3rdparty/p256-m/p256-m_driver_entrypoints.c +++ b/3rdparty/p256-m/p256-m_driver_entrypoints.c @@ -130,7 +130,7 @@ psa_status_t p256_transparent_export_public_key(const psa_key_attributes_t *attr /* Validate sizes, as p256-m expects fixed-size buffers */ if (key_buffer_size != PRIVKEY_SIZE) { - return PSA_ERROR_CORRUPTION_DETECTED; + return PSA_ERROR_INVALID_ARGUMENT; } if (data_size < PSA_PUBKEY_SIZE) { return PSA_ERROR_BUFFER_TOO_SMALL; @@ -231,7 +231,7 @@ psa_status_t p256_transparent_sign_hash( /* Validate sizes, as p256-m expects fixed-size buffers */ if (key_buffer_size != PRIVKEY_SIZE) { - return PSA_ERROR_CORRUPTION_DETECTED; + return PSA_ERROR_INVALID_ARGUMENT; } if (signature_size < SIGNATURE_SIZE) { return PSA_ERROR_BUFFER_TOO_SMALL; @@ -257,7 +257,7 @@ static psa_status_t p256_verify_hash_with_public_key( { /* Validate sizes, as p256-m expects fixed-size buffers */ if (key_buffer_size != PSA_PUBKEY_SIZE || *key_buffer != PSA_PUBKEY_HEADER_BYTE) { - return PSA_ERROR_CORRUPTION_DETECTED; + return PSA_ERROR_INVALID_ARGUMENT; } if (signature_length != SIGNATURE_SIZE) { return PSA_ERROR_INVALID_SIGNATURE; From 5ca69349b50ad4d0c8d9d47bbcaf849f189c9f80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 20 Sep 2023 09:28:02 +0200 Subject: [PATCH 22/24] Improve comments on key formats MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- 3rdparty/p256-m/p256-m_driver_entrypoints.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/3rdparty/p256-m/p256-m_driver_entrypoints.c b/3rdparty/p256-m/p256-m_driver_entrypoints.c index b75c06c669..bcbb651c7c 100644 --- a/3rdparty/p256-m/p256-m_driver_entrypoints.c +++ b/3rdparty/p256-m/p256-m_driver_entrypoints.c @@ -38,7 +38,7 @@ * total of 65 bytes. * * p256-m's internal format for private keys matches PSA. Its format for public - * keys is only 64 bytes; the same as PSA but without the leading byte (0x04). + * keys is only 64 bytes: the same as PSA but without the leading byte (0x04). * Hence, when passing public keys from PSA to p256-m, the leading byte is * removed. * @@ -89,6 +89,7 @@ psa_status_t p256_transparent_import_key(const psa_key_attributes_t *attributes, if (data_length != PSA_PUBKEY_SIZE) { return *bits == 0 ? PSA_ERROR_NOT_SUPPORTED : PSA_ERROR_INVALID_ARGUMENT; } + /* See INFORMATION ON PSA KEY EXPORT FORMATS near top of file */ if (p256_validate_pubkey(data + 1) != P256_SUCCESS) { return PSA_ERROR_INVALID_ARGUMENT; } @@ -136,7 +137,7 @@ psa_status_t p256_transparent_export_public_key(const psa_key_attributes_t *attr return PSA_ERROR_BUFFER_TOO_SMALL; } - /* Output public key in the PSA export format */ + /* See INFORMATION ON PSA KEY EXPORT FORMATS near top of file */ data[0] = PSA_PUBKEY_HEADER_BYTE; int ret = p256_public_from_private(data + 1, key_buffer); if (ret == P256_SUCCESS) { @@ -201,10 +202,9 @@ psa_status_t p256_transparent_key_agreement( return PSA_ERROR_BUFFER_TOO_SMALL; } - /* We add 1 to peer_key pointer to omit the leading byte of the public key - * representation (0x04). See information about PSA key formats at the top - * of the file. */ - int ret = p256_ecdh_shared_secret(shared_secret, key_buffer, peer_key + 1); + /* See INFORMATION ON PSA KEY EXPORT FORMATS near top of file */ + const uint8_t peer_key_p256m = peer_key + 1; + int ret = p256_ecdh_shared_secret(shared_secret, key_buffer, peer_key_p256m); if (ret == P256_SUCCESS) { *shared_secret_length = SHARED_SECRET_SIZE; } @@ -263,11 +263,9 @@ static psa_status_t p256_verify_hash_with_public_key( return PSA_ERROR_INVALID_SIGNATURE; } - /* We add 1 to public_key_buffer pointer to omit the leading byte of the - * public key representation (0x04). See information about PSA key formats - * at the top of the file. */ - const uint8_t *public_key_buffer = key_buffer + 1; - int ret = p256_ecdsa_verify(signature, public_key_buffer, hash, hash_length); + /* See INFORMATION ON PSA KEY EXPORT FORMATS near top of file */ + const uint8_t *public_key_p256m = key_buffer + 1; + int ret = p256_ecdsa_verify(signature, public_key_p256m, hash, hash_length); return p256_to_psa_error(ret); } From f25189473b24b50603926ab682eb5ceae20fd564 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 20 Sep 2023 09:42:55 +0200 Subject: [PATCH 23/24] Fix documentation of error codes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- 3rdparty/p256-m/p256-m_driver_entrypoints.h | 39 +++++++++++++-------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/3rdparty/p256-m/p256-m_driver_entrypoints.h b/3rdparty/p256-m/p256-m_driver_entrypoints.h index 42e51d975a..0e85237e5e 100644 --- a/3rdparty/p256-m/p256-m_driver_entrypoints.h +++ b/3rdparty/p256-m/p256-m_driver_entrypoints.h @@ -104,9 +104,10 @@ psa_status_t p256_transparent_export_public_key(const psa_key_attributes_t *attr * * \retval #PSA_SUCCESS * Success. Keypair generated and stored in buffer. - * \retval #PSA_ERROR_NOT_SUPPORTED - * \retval #PSA_ERROR_GENERIC_ERROR - * \retval #PSA_ERROR_INSUFFICIENT_MEMORY + * \retval #PSA_ERROR_BUFFER_TOO_SMALL + * \p key_buffer_size is too small. + * \retval #PSA_ERROR_GENERIC_ERROR + * The internal RNG failed. */ psa_status_t p256_transparent_generate_key( const psa_key_attributes_t *attributes, @@ -132,9 +133,12 @@ psa_status_t p256_transparent_generate_key( * bytes. * \param[out] shared_secret_length On success, the number of bytes that * make up the returned shared secret. - * \retval #PSA_SUCCESS - * Success. Shared secret successfully calculated. - * \retval #PSA_ERROR_NOT_SUPPORTED + * \retval #PSA_SUCCESS + * Success. Shared secret successfully calculated. + * \retval #PSA_ERROR_INVALID_ARGUMENT + * The input is invalid. + * \retval #PSA_ERROR_BUFFER_TOO_SMALL + * \p shared_secret_size is too small. */ psa_status_t p256_transparent_key_agreement( const psa_key_attributes_t *attributes, @@ -163,10 +167,14 @@ psa_status_t p256_transparent_key_agreement( * \param[out] signature_length On success, the number of bytes * that make up the returned signature value. * - * \retval #PSA_SUCCESS + * \retval #PSA_SUCCESS * Success. Hash was signed successfully. - * respectively of the key. - * \retval #PSA_ERROR_NOT_SUPPORTED + * \retval #PSA_ERROR_INVALID_ARGUMENT + * The input is invalid. + * \retval #PSA_ERROR_BUFFER_TOO_SMALL + * \p signature_size is too small. + * \retval #PSA_ERROR_GENERIC_ERROR + * The internal RNG failed. */ psa_status_t p256_transparent_sign_hash( const psa_key_attributes_t *attributes, @@ -202,12 +210,13 @@ psa_status_t p256_transparent_sign_hash( * \param[in] signature Buffer containing the signature to verify. * \param[in] signature_length Size of the \p signature buffer in bytes. * - * \retval #PSA_SUCCESS - * The signature is valid. - * \retval #PSA_ERROR_INVALID_SIGNATURE - * The calculation was performed successfully, but the passed - * signature is not a valid signature. - * \retval #PSA_ERROR_NOT_SUPPORTED + * \retval #PSA_SUCCESS + * The signature is valid. + * \retval #PSA_ERROR_INVALID_SIGNATURE + * The calculation was performed successfully, but the passed + * signature is not a valid signature. + * \retval #PSA_ERROR_INVALID_ARGUMENT + * The input is invalid. */ psa_status_t p256_transparent_verify_hash( const psa_key_attributes_t *attributes, From 3ec976c42c229b39858559ee45627a294293bebd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 20 Sep 2023 16:12:46 +0200 Subject: [PATCH 24/24] Fix typo in variable declaration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- 3rdparty/p256-m/p256-m_driver_entrypoints.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/3rdparty/p256-m/p256-m_driver_entrypoints.c b/3rdparty/p256-m/p256-m_driver_entrypoints.c index bcbb651c7c..b2236e4877 100644 --- a/3rdparty/p256-m/p256-m_driver_entrypoints.c +++ b/3rdparty/p256-m/p256-m_driver_entrypoints.c @@ -203,7 +203,7 @@ psa_status_t p256_transparent_key_agreement( } /* See INFORMATION ON PSA KEY EXPORT FORMATS near top of file */ - const uint8_t peer_key_p256m = peer_key + 1; + const uint8_t *peer_key_p256m = peer_key + 1; int ret = p256_ecdh_shared_secret(shared_secret, key_buffer, peer_key_p256m); if (ret == P256_SUCCESS) { *shared_secret_length = SHARED_SECRET_SIZE;