From f35069a82df6080b02eea3d8b278d85675f2a83a Mon Sep 17 00:00:00 2001 From: John Durkop Date: Mon, 17 Aug 2020 22:05:14 -0700 Subject: [PATCH 1/9] Fix undefined ref error when ECDSA not defined 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 --- library/pk.c | 3 +++ library/pk_wrap.c | 18 +++++++++++++++++- tests/scripts/all.sh | 6 ++++++ 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/library/pk.c b/library/pk.c index 631415cca4..6706344cc2 100644 --- a/library/pk.c +++ b/library/pk.c @@ -593,6 +593,9 @@ int mbedtls_pk_wrap_as_opaque( mbedtls_pk_context *pk, psa_algorithm_t hash_alg ) { #if !defined(MBEDTLS_ECP_C) + ((void) pk); + ((void) handle); + ((void) hash_alg); return( MBEDTLS_ERR_PK_TYPE_MISMATCH ); #else const mbedtls_ecp_keypair *ec; diff --git a/library/pk_wrap.c b/library/pk_wrap.c index 0c6d5a590e..fd4a87509f 100644 --- a/library/pk_wrap.c +++ b/library/pk_wrap.c @@ -34,7 +34,7 @@ #include "mbedtls/ecp.h" #endif -#if defined(MBEDTLS_ECDSA_C) +#if defined(MBEDTLS_ECDSA_C) || defined(MBEDTLS_USE_PSA_CRYPTO) #include "mbedtls/ecdsa.h" #endif @@ -912,6 +912,8 @@ static int pk_opaque_can_do( mbedtls_pk_type_t type ) type == MBEDTLS_PK_ECDSA ); } +#if defined(MBEDTLS_ECDSA_C) + /* * Simultaneously convert and move raw MPI from the beginning of a buffer * to an ASN.1 MPI at the end of the buffer. @@ -994,11 +996,24 @@ static int pk_ecdsa_sig_asn1_from_psa( unsigned char *sig, size_t *sig_len, return( 0 ); } +#endif + static int pk_opaque_sign_wrap( void *ctx, mbedtls_md_type_t md_alg, const unsigned char *hash, size_t hash_len, unsigned char *sig, size_t *sig_len, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) { +#if !defined(MBEDTLS_ECDSA_C) + ((void) ctx); + ((void) md_alg); + ((void) hash); + ((void) hash_len); + ((void) sig); + ((void) sig_len); + ((void) f_rng); + ((void) p_rng); + return( PSA_ERROR_NOT_SUPPORTED ); +#else const psa_key_handle_t *key = (const psa_key_handle_t *) ctx; psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT; psa_algorithm_t alg = PSA_ALG_ECDSA( mbedtls_psa_translate_md( md_alg ) ); @@ -1029,6 +1044,7 @@ static int pk_opaque_sign_wrap( void *ctx, mbedtls_md_type_t md_alg, /* transcode it to ASN.1 sequence */ return( pk_ecdsa_sig_asn1_from_psa( sig, sig_len, buf_len ) ); +#endif } const mbedtls_pk_info_t mbedtls_pk_opaque_info = { diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 558016d040..f95d8cf919 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1184,6 +1184,12 @@ component_test_depends_hashes () { record_status tests/scripts/depends-hashes.pl } +component_test_depends_pkalgs_psa () { + msg "test/build: depends-pkalgs.pl with MBEDTLS_USE_PSA_CRYPTO defined (gcc)" + scripts/config.py set MBEDTLS_USE_PSA_CRYPTO + record_status tests/scripts/depends-pkalgs.pl +} + component_test_depends_pkalgs () { msg "test/build: depends-pkalgs.pl (gcc)" # ~ 2 min record_status tests/scripts/depends-pkalgs.pl From bc5a754f28381a2a6ccb07cb4f840a8cdf0e548b Mon Sep 17 00:00:00 2001 From: John Durkop Date: Tue, 18 Aug 2020 05:23:36 -0700 Subject: [PATCH 2/9] Add change log description for Fix #3294 Signed-off-by: John Durkop --- ChangeLog.d/bugfix_PR3294.txt | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 ChangeLog.d/bugfix_PR3294.txt diff --git a/ChangeLog.d/bugfix_PR3294.txt b/ChangeLog.d/bugfix_PR3294.txt new file mode 100644 index 0000000000..9a5bbc445f --- /dev/null +++ b/ChangeLog.d/bugfix_PR3294.txt @@ -0,0 +1,8 @@ +Bugfix + * 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. Fixes issue reported in + #3294 where undefined reference errors occur when using USE_PSA_CRYPTO + and removing ECDSA support. + From c14be901eb1bc0c065690b2d6bc458e8530bf90f Mon Sep 17 00:00:00 2001 From: John Durkop Date: Thu, 20 Aug 2020 06:16:41 -0700 Subject: [PATCH 3/9] Add new test_depends_curves_psa to all.sh Add new test (test_depends_curves_psa) to all.sh to confirm that test is passing when MBEDTLS_USE_PSA_CRYPTO is defined. Fix #3294 Signed-off-by: John Durkop --- tests/scripts/all.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index f95d8cf919..1c67a9ac97 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1179,6 +1179,12 @@ component_test_depends_curves () { record_status tests/scripts/curves.pl } +component_test_depends_curves_psa () { + msg "test/build: curves.pl with MBEDTLS_USE_PSA_CRYPTO defined (gcc)" + scripts/config.py set MBEDTLS_USE_PSA_CRYPTO + record_status tests/scripts/curves.pl +} + component_test_depends_hashes () { msg "test/build: depends-hashes.pl (gcc)" # ~ 2 min record_status tests/scripts/depends-hashes.pl From af5363c24e822960f1c8ff7af66f76d05ddc29eb Mon Sep 17 00:00:00 2001 From: John Durkop Date: Mon, 24 Aug 2020 08:29:39 -0700 Subject: [PATCH 4/9] Updates to cleanup fixes for #3294 Minor updates to changelog for more concise wording and fixed styling in other files as needed. Signed-off-by: John Durkop --- ChangeLog.d/bugfix_PR3294.txt | 8 ++------ library/pk_wrap.c | 8 ++++---- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/ChangeLog.d/bugfix_PR3294.txt b/ChangeLog.d/bugfix_PR3294.txt index 9a5bbc445f..a6ea75e05c 100644 --- a/ChangeLog.d/bugfix_PR3294.txt +++ b/ChangeLog.d/bugfix_PR3294.txt @@ -1,8 +1,4 @@ Bugfix - * 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. Fixes issue reported in - #3294 where undefined reference errors occur when using USE_PSA_CRYPTO - and removing ECDSA support. + * Fix build failure in configurations where MBEDTLS_USE_PSA_CRYPTO is + enabled but ECDSA is disabled. Contributed by jdurkop. Fixes #3294. diff --git a/library/pk_wrap.c b/library/pk_wrap.c index fd4a87509f..33253a4d25 100644 --- a/library/pk_wrap.c +++ b/library/pk_wrap.c @@ -34,7 +34,7 @@ #include "mbedtls/ecp.h" #endif -#if defined(MBEDTLS_ECDSA_C) || defined(MBEDTLS_USE_PSA_CRYPTO) +#if defined(MBEDTLS_ECDSA_C) #include "mbedtls/ecdsa.h" #endif @@ -1012,8 +1012,8 @@ static int pk_opaque_sign_wrap( void *ctx, mbedtls_md_type_t md_alg, ((void) sig_len); ((void) f_rng); ((void) p_rng); - return( PSA_ERROR_NOT_SUPPORTED ); -#else + return( MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE ); +#else /* !MBEDTLS_ECDSA_C */ const psa_key_handle_t *key = (const psa_key_handle_t *) ctx; psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT; psa_algorithm_t alg = PSA_ALG_ECDSA( mbedtls_psa_translate_md( md_alg ) ); @@ -1044,7 +1044,7 @@ static int pk_opaque_sign_wrap( void *ctx, mbedtls_md_type_t md_alg, /* transcode it to ASN.1 sequence */ return( pk_ecdsa_sig_asn1_from_psa( sig, sig_len, buf_len ) ); -#endif +#endif /* !MBEDTLS_ECDSA_C */ } const mbedtls_pk_info_t mbedtls_pk_opaque_info = { From d46ede0d37cbe9148fadf782e2ca85f067bdc08a Mon Sep 17 00:00:00 2001 From: John Durkop Date: Mon, 24 Aug 2020 09:51:00 -0700 Subject: [PATCH 5/9] Fix missing label for guard Fixes #3294 Signed-off-by: John Durkop --- library/pk_wrap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/pk_wrap.c b/library/pk_wrap.c index 33253a4d25..6983d14752 100644 --- a/library/pk_wrap.c +++ b/library/pk_wrap.c @@ -996,7 +996,7 @@ static int pk_ecdsa_sig_asn1_from_psa( unsigned char *sig, size_t *sig_len, return( 0 ); } -#endif +#endif /* MBEDTLS_ECDSA_C */ static int pk_opaque_sign_wrap( void *ctx, mbedtls_md_type_t md_alg, const unsigned char *hash, size_t hash_len, From 2ec2eaac312f782f3ccdf5b69d01e64d40aa2c90 Mon Sep 17 00:00:00 2001 From: John Durkop Date: Mon, 24 Aug 2020 18:29:15 -0700 Subject: [PATCH 6/9] Fix test issues with depends-hashes 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 --- library/pk_wrap.c | 8 ++------ tests/scripts/all.sh | 6 ++++++ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/library/pk_wrap.c b/library/pk_wrap.c index 6983d14752..103842c02b 100644 --- a/library/pk_wrap.c +++ b/library/pk_wrap.c @@ -553,11 +553,12 @@ static int ecdsa_verify_wrap( void *ctx_arg, mbedtls_md_type_t md_alg, unsigned char buf[30 + 2 * MBEDTLS_ECP_MAX_BYTES]; unsigned char *p; mbedtls_pk_info_t pk_info = mbedtls_eckey_info; - psa_algorithm_t psa_sig_md, psa_md; + psa_algorithm_t psa_sig_md = PSA_ALG_ECDSA_ANY; size_t curve_bits; psa_ecc_family_t curve = mbedtls_ecc_group_to_psa( ctx->grp.id, &curve_bits ); const size_t signature_part_size = ( ctx->grp.nbits + 7 ) / 8; + ((void) md_alg); if( curve == 0 ) return( MBEDTLS_ERR_PK_BAD_INPUT_DATA ); @@ -571,11 +572,6 @@ static int ecdsa_verify_wrap( void *ctx_arg, mbedtls_md_type_t md_alg, if( key_len <= 0 ) return( MBEDTLS_ERR_PK_BAD_INPUT_DATA ); - psa_md = mbedtls_psa_translate_md( md_alg ); - if( psa_md == 0 ) - return( MBEDTLS_ERR_PK_BAD_INPUT_DATA ); - psa_sig_md = PSA_ALG_ECDSA( psa_md ); - psa_set_key_type( &attributes, PSA_KEY_TYPE_ECC_PUBLIC_KEY( curve ) ); psa_set_key_usage_flags( &attributes, PSA_KEY_USAGE_VERIFY_HASH ); psa_set_key_algorithm( &attributes, psa_sig_md ); diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 1c67a9ac97..15d8a1decc 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1190,6 +1190,12 @@ component_test_depends_hashes () { record_status tests/scripts/depends-hashes.pl } +component_test_depends_hashes_psa () { + msg "test/build: depends-hashes.pl with MBEDTLS_USE_PSA_CRYPTO defined (gcc)" + scripts/config.py set MBEDTLS_USE_PSA_CRYPTO + record_status tests/scripts/depends-hashes.pl +} + component_test_depends_pkalgs_psa () { msg "test/build: depends-pkalgs.pl with MBEDTLS_USE_PSA_CRYPTO defined (gcc)" scripts/config.py set MBEDTLS_USE_PSA_CRYPTO From d4efa8d0ac41a20baa346c634154fc1c6eb75d16 Mon Sep 17 00:00:00 2001 From: John Durkop Date: Tue, 8 Sep 2020 05:58:28 -0700 Subject: [PATCH 7/9] Fix pk_ec_test_vec() to use MBEDTLS_MD_NONE The pk_ec_test_vec() was incorrectly using MBEDTLS_MD_SHA1 for the parameter to mbedtls_pk_verify(). It should use MBEDTLS_MD_NONE since that parameter is ignored for this test case. Signed-off-by: John Durkop --- tests/suites/test_suite_pk.function | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_pk.function b/tests/suites/test_suite_pk.function index 43b4914739..c6041b249f 100644 --- a/tests/suites/test_suite_pk.function +++ b/tests/suites/test_suite_pk.function @@ -775,8 +775,8 @@ void pk_ec_test_vec( int type, int id, data_t * key, data_t * hash, TEST_ASSERT( mbedtls_ecp_point_read_binary( &eckey->grp, &eckey->Q, key->x, key->len ) == 0 ); - // MBEDTLS_MD_SHA1 is a dummy - it is ignored, but has to be other than MBEDTLS_MD_NONE. - TEST_ASSERT( mbedtls_pk_verify( &pk, MBEDTLS_MD_SHA1, + // MBEDTLS_MD_NONE is used since it will be ignored. + TEST_ASSERT( mbedtls_pk_verify( &pk, MBEDTLS_MD_NONE, hash->x, hash->len, sig->x, sig->len ) == ret ); exit: From 619e09e70534e7a95ad16c7dccf9c480fdcae1f6 Mon Sep 17 00:00:00 2001 From: John Durkop Date: Tue, 8 Sep 2020 22:19:56 -0700 Subject: [PATCH 8/9] Minor update to all.sh to change test order Moved the new component_test_depends_pkalgs_psa to after the component_test_depends_pkalgs test to be more consistent. Signed-off-by: John Durkop --- tests/scripts/all.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 15d8a1decc..bd5d5a18e3 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1196,14 +1196,14 @@ component_test_depends_hashes_psa () { record_status tests/scripts/depends-hashes.pl } -component_test_depends_pkalgs_psa () { - msg "test/build: depends-pkalgs.pl with MBEDTLS_USE_PSA_CRYPTO defined (gcc)" - scripts/config.py set MBEDTLS_USE_PSA_CRYPTO +component_test_depends_pkalgs () { + msg "test/build: depends-pkalgs.pl (gcc)" # ~ 2 min record_status tests/scripts/depends-pkalgs.pl } -component_test_depends_pkalgs () { - msg "test/build: depends-pkalgs.pl (gcc)" # ~ 2 min +component_test_depends_pkalgs_psa () { + msg "test/build: depends-pkalgs.pl with MBEDTLS_USE_PSA_CRYPTO defined (gcc)" + scripts/config.py set MBEDTLS_USE_PSA_CRYPTO record_status tests/scripts/depends-pkalgs.pl } From d61712233e7d33edaa200094201febec3fb5ada7 Mon Sep 17 00:00:00 2001 From: John Durkop Date: Wed, 9 Sep 2020 05:18:51 -0700 Subject: [PATCH 9/9] Remove check compilation guards from travis build With the increase in depends testing for PSA changes introduced here the Travis builds are now taking too long. The check for compilation guards will only be run on Jenkins now. See this comment for further details. https://github.com/ARMmbed/mbedtls/pull/3585#discussion_r485189748 Signed-off-by: John Durkop --- .travis.yml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/.travis.yml b/.travis.yml index c67c0cd33c..76cb1c5372 100644 --- a/.travis.yml +++ b/.travis.yml @@ -28,10 +28,6 @@ jobs: script: - tests/scripts/all.sh -k test_full_cmake_gcc_asan - - name: check compilation guards - script: - - tests/scripts/all.sh -k 'test_depends_*' 'build_key_exchanges' - - name: macOS os: osx compiler: clang