From 82823b2fe88b2a38b698a59b1645dd68326ae5f3 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 27 Jul 2023 12:25:05 +0100 Subject: [PATCH 1/6] Fix new bignum tests These tests weren't working, because they use CMake and can't pass options with CFLAGS directly. This could be mitigated by adding a CMake option, but using config.py is less invasive and it is what we normally use for setting build options anyway. Signed-off-by: Janos Follath --- library/ecp_invasive.h | 13 ++++++++----- tests/scripts/all.sh | 12 ++++++++---- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/library/ecp_invasive.h b/library/ecp_invasive.h index db9dee3f47..0c6858a40a 100644 --- a/library/ecp_invasive.h +++ b/library/ecp_invasive.h @@ -31,6 +31,14 @@ #include "bignum_mod.h" #include "mbedtls/ecp.h" +/* + * Turning this option on enables using the new bignum code in the ECC modules. + * + * WARNING: ECC implementation using the new bignum code is a work in progress, + * this option serves only development and testing purposes. + */ +//#define MBEDTLS_ECP_WITH_MPI_UINT + /* * Curve modulus types */ @@ -40,11 +48,6 @@ typedef enum { MBEDTLS_ECP_MOD_SCALAR } mbedtls_ecp_modulus_type; -/* Provide a commented-out definition so that `check_names.py` knows that - * it's not a typo. - */ -//#define MBEDTLS_ECP_WITH_MPI_UINT - typedef enum { MBEDTLS_ECP_VARIANT_NONE = 0, MBEDTLS_ECP_VARIANT_WITH_MPI_STRUCT, diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index b6f6b600c8..71e2332a7e 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -134,13 +134,14 @@ pre_initialize_variables () { CONFIG_H='include/mbedtls/mbedtls_config.h' CRYPTO_CONFIG_H='include/psa/crypto_config.h' CONFIG_TEST_DRIVER_H='tests/include/test/drivers/config_test_driver.h' + CONFIG_NEW_BIGNUM_H='library/ecp_invasive.h' # Files that are clobbered by some jobs will be backed up. Use a different # suffix from auxiliary scripts so that all.sh and auxiliary scripts can # independently decide when to remove the backup file. backup_suffix='.all.bak' # Files clobbered by config.py - files_to_back_up="$CONFIG_H $CRYPTO_CONFIG_H $CONFIG_TEST_DRIVER_H" + files_to_back_up="$CONFIG_H $CRYPTO_CONFIG_H $CONFIG_TEST_DRIVER_H $CONFIG_NEW_BIGNUM_H" # Files clobbered by in-tree cmake files_to_back_up="$files_to_back_up Makefile library/Makefile programs/Makefile tests/Makefile programs/fuzz/Makefile" @@ -1028,8 +1029,9 @@ component_test_default_cmake_gcc_asan () { component_test_default_cmake_gcc_asan_new_bignum () { msg "build: cmake, gcc, ASan" # ~ 1 min 50s + scripts/config.py -f "$CONFIG_NEW_BIGNUM_H" set MBEDTLS_ECP_WITH_MPI_UINT CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan . - make CFLAGS="-D MBEDTLS_ECP_WITH_MPI_UINT" + make msg "test: main suites (inc. selftests) (ASan build)" # ~ 50s make test @@ -1086,8 +1088,9 @@ component_test_full_cmake_gcc_asan () { component_test_full_cmake_gcc_asan_new_bignum () { msg "build: full config, cmake, gcc, ASan" scripts/config.py full + scripts/config.py -f "$CONFIG_NEW_BIGNUM_H" set MBEDTLS_ECP_WITH_MPI_UINT CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan . - make CFLAGS="-D MBEDTLS_ECP_WITH_MPI_UINT" + make msg "test: main suites (inc. selftests) (full config, ASan build)" make test @@ -1122,8 +1125,9 @@ component_test_full_cmake_gcc_asan_new_bignum_test_hooks () { msg "build: full config, cmake, gcc, ASan" scripts/config.py full scripts/config.py set MBEDTLS_TEST_HOOKS + scripts/config.py -f "$CONFIG_NEW_BIGNUM_H" set MBEDTLS_ECP_WITH_MPI_UINT CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan . - make CFLAGS="-DMBEDTLS_ECP_WITH_MPI_UINT" + make msg "test: main suites (inc. selftests) (full config, ASan build)" make test From f3135af159aca63cfcfe02ab6689975a19545e64 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 31 Jul 2023 10:07:57 +0100 Subject: [PATCH 2/6] Use config.py in all new bignum tests This previous test is correct, as it is using make. Switch to using config.py for robustness and consistency. Signed-off-by: Janos Follath --- tests/scripts/all.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 71e2332a7e..a1dcb8dfc1 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -4216,7 +4216,8 @@ component_test_have_int32_cmake_new_bignum () { scripts/config.py unset MBEDTLS_PADLOCK_C scripts/config.py unset MBEDTLS_AESCE_C scripts/config.py set MBEDTLS_TEST_HOOKS - make CC=gcc CFLAGS="$ASAN_CFLAGS -Werror -Wall -Wextra -DMBEDTLS_HAVE_INT32 -DMBEDTLS_ECP_WITH_MPI_UINT" LDFLAGS="$ASAN_CFLAGS" + scripts/config.py -f "$CONFIG_NEW_BIGNUM_H" set MBEDTLS_ECP_WITH_MPI_UINT + make CC=gcc CFLAGS="$ASAN_CFLAGS -Werror -Wall -Wextra -DMBEDTLS_HAVE_INT32" LDFLAGS="$ASAN_CFLAGS" msg "test: gcc, force 32-bit bignum limbs, new bignum interface, test hooks (ASan build)" make test From 2f04582d3799bb719889174585e1bb81d6b78d38 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 31 Jul 2023 10:57:16 +0100 Subject: [PATCH 3/6] Move MBEDTLS_ECP_WITH_MPI_UINT to mbedtls_config.h There is a precedent for having bigger and less mature options in mbedtls_config.h (MBEDTLS_USE_PSA_CRYPTO) for an extended period. Having this option in mbedtls_config.h is simpler and more robust. Signed-off-by: Janos Follath --- include/mbedtls/mbedtls_config.h | 9 +++++++++ library/ecp_invasive.h | 8 -------- tests/scripts/all.sh | 11 +++++------ 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index 30e4d13ece..c65ed92689 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -3997,4 +3997,13 @@ */ //#define MBEDTLS_P256M_EXAMPLE_DRIVER_ENABLED + +/** + * Uncomment to enable using new bignum code in the ECC modules. + * + * \warning ECC implementation using new bignum code is a work in progress, + * this option serves only development and testing purposes. + */ +//#define MBEDTLS_ECP_WITH_MPI_UINT + /** \} name SECTION: Module configuration options */ diff --git a/library/ecp_invasive.h b/library/ecp_invasive.h index 0c6858a40a..bb3b127ffe 100644 --- a/library/ecp_invasive.h +++ b/library/ecp_invasive.h @@ -31,14 +31,6 @@ #include "bignum_mod.h" #include "mbedtls/ecp.h" -/* - * Turning this option on enables using the new bignum code in the ECC modules. - * - * WARNING: ECC implementation using the new bignum code is a work in progress, - * this option serves only development and testing purposes. - */ -//#define MBEDTLS_ECP_WITH_MPI_UINT - /* * Curve modulus types */ diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index a1dcb8dfc1..c47b767aea 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -134,14 +134,13 @@ pre_initialize_variables () { CONFIG_H='include/mbedtls/mbedtls_config.h' CRYPTO_CONFIG_H='include/psa/crypto_config.h' CONFIG_TEST_DRIVER_H='tests/include/test/drivers/config_test_driver.h' - CONFIG_NEW_BIGNUM_H='library/ecp_invasive.h' # Files that are clobbered by some jobs will be backed up. Use a different # suffix from auxiliary scripts so that all.sh and auxiliary scripts can # independently decide when to remove the backup file. backup_suffix='.all.bak' # Files clobbered by config.py - files_to_back_up="$CONFIG_H $CRYPTO_CONFIG_H $CONFIG_TEST_DRIVER_H $CONFIG_NEW_BIGNUM_H" + files_to_back_up="$CONFIG_H $CRYPTO_CONFIG_H $CONFIG_TEST_DRIVER_H" # Files clobbered by in-tree cmake files_to_back_up="$files_to_back_up Makefile library/Makefile programs/Makefile tests/Makefile programs/fuzz/Makefile" @@ -1029,7 +1028,7 @@ component_test_default_cmake_gcc_asan () { component_test_default_cmake_gcc_asan_new_bignum () { msg "build: cmake, gcc, ASan" # ~ 1 min 50s - scripts/config.py -f "$CONFIG_NEW_BIGNUM_H" set MBEDTLS_ECP_WITH_MPI_UINT + scripts/config.py set MBEDTLS_ECP_WITH_MPI_UINT CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan . make @@ -1088,7 +1087,7 @@ component_test_full_cmake_gcc_asan () { component_test_full_cmake_gcc_asan_new_bignum () { msg "build: full config, cmake, gcc, ASan" scripts/config.py full - scripts/config.py -f "$CONFIG_NEW_BIGNUM_H" set MBEDTLS_ECP_WITH_MPI_UINT + scripts/config.py set MBEDTLS_ECP_WITH_MPI_UINT CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan . make @@ -1125,7 +1124,7 @@ component_test_full_cmake_gcc_asan_new_bignum_test_hooks () { msg "build: full config, cmake, gcc, ASan" scripts/config.py full scripts/config.py set MBEDTLS_TEST_HOOKS - scripts/config.py -f "$CONFIG_NEW_BIGNUM_H" set MBEDTLS_ECP_WITH_MPI_UINT + scripts/config.py set MBEDTLS_ECP_WITH_MPI_UINT CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan . make @@ -4216,7 +4215,7 @@ component_test_have_int32_cmake_new_bignum () { scripts/config.py unset MBEDTLS_PADLOCK_C scripts/config.py unset MBEDTLS_AESCE_C scripts/config.py set MBEDTLS_TEST_HOOKS - scripts/config.py -f "$CONFIG_NEW_BIGNUM_H" set MBEDTLS_ECP_WITH_MPI_UINT + scripts/config.py set MBEDTLS_ECP_WITH_MPI_UINT make CC=gcc CFLAGS="$ASAN_CFLAGS -Werror -Wall -Wextra -DMBEDTLS_HAVE_INT32" LDFLAGS="$ASAN_CFLAGS" msg "test: gcc, force 32-bit bignum limbs, new bignum interface, test hooks (ASan build)" From 3ed980d60fa376fb87839f1c528a26226d5bd12d Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 31 Jul 2023 16:13:35 +0100 Subject: [PATCH 4/6] Fix full config in config.py By default, the full configuration enables all options. But we specifically don't want to enable MBEDTLS_ECP_WITH_MPI_UINT except where it's done explicitly, because it disables the old ecp. So it needs to be added to the exceptions in config.py (EXCLUDE_FROM_FULL). Signed-off-by: Janos Follath --- scripts/config.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/config.py b/scripts/config.py index 3e957fdd22..cb42c118eb 100755 --- a/scripts/config.py +++ b/scripts/config.py @@ -217,7 +217,8 @@ EXCLUDE_FROM_FULL = frozenset([ 'MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND', # build dependency (valgrind headers) 'MBEDTLS_X509_REMOVE_INFO', # removes a feature 'MBEDTLS_SSL_RECORD_SIZE_LIMIT', # in development, currently breaks other tests - 'MBEDTLS_P256M_EXAMPLE_DRIVER_ENABLED' # influences SECP256R1 KeyGen/ECDH/ECDSA + 'MBEDTLS_P256M_EXAMPLE_DRIVER_ENABLED', # influences SECP256R1 KeyGen/ECDH/ECDSA + 'MBEDTLS_ECP_WITH_MPI_UINT' # disables the default ECP and is experimental ]) def is_seamless_alt(name): From e416f03c8f2e8b2abf42657bed23dec4e538a129 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 1 Aug 2023 08:44:40 +0100 Subject: [PATCH 5/6] Improve wording of MBEDTLS_ECP_WITH_MPI_UINT doc Use the standard "experimental" word in the description and make the wording more similar to other experimental warnings. Signed-off-by: Janos Follath --- include/mbedtls/mbedtls_config.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index c65ed92689..719bbed8fe 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -4001,8 +4001,8 @@ /** * Uncomment to enable using new bignum code in the ECC modules. * - * \warning ECC implementation using new bignum code is a work in progress, - * this option serves only development and testing purposes. + * \warning This is currently experimental, incomplete and therefore should not + * be used in production. */ //#define MBEDTLS_ECP_WITH_MPI_UINT From 5b7c38f673f97835574b4fb8f206ccfd5e9740db Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 1 Aug 2023 08:51:12 +0100 Subject: [PATCH 6/6] Sort full config exceptions in config.py The EXCLUDE_FROM_FULL list in config.py should be, and used to be, but no longer is, in alphabetical order, and with a comma on the last element. Signed-off-by: Janos Follath --- scripts/config.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/config.py b/scripts/config.py index cb42c118eb..4ff5166782 100755 --- a/scripts/config.py +++ b/scripts/config.py @@ -195,6 +195,7 @@ EXCLUDE_FROM_FULL = frozenset([ 'MBEDTLS_DEPRECATED_WARNING', # conflicts with deprecated options 'MBEDTLS_ECDH_VARIANT_EVEREST_ENABLED', # influences the use of ECDH in TLS 'MBEDTLS_ECP_NO_FALLBACK', # removes internal ECP implementation + 'MBEDTLS_ECP_WITH_MPI_UINT', # disables the default ECP and is experimental 'MBEDTLS_ENTROPY_FORCE_SHA256', # interacts with CTR_DRBG_128_BIT_KEY 'MBEDTLS_HAVE_SSE2', # hardware dependency 'MBEDTLS_MEMORY_BACKTRACE', # depends on MEMORY_BUFFER_ALLOC_C @@ -204,6 +205,7 @@ EXCLUDE_FROM_FULL = frozenset([ 'MBEDTLS_NO_DEFAULT_ENTROPY_SOURCES', # removes a feature 'MBEDTLS_NO_PLATFORM_ENTROPY', # removes a feature 'MBEDTLS_NO_UDBL_DIVISION', # influences anything that uses bignum + 'MBEDTLS_P256M_EXAMPLE_DRIVER_ENABLED', # influences SECP256R1 KeyGen/ECDH/ECDSA 'MBEDTLS_PLATFORM_NO_STD_FUNCTIONS', # removes a feature 'MBEDTLS_PSA_CRYPTO_CONFIG', # toggles old/new style PSA config 'MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG', # behavior change + build dependency @@ -213,12 +215,10 @@ EXCLUDE_FROM_FULL = frozenset([ 'MBEDTLS_RSA_NO_CRT', # influences the use of RSA in X.509 and TLS 'MBEDTLS_SHA256_USE_A64_CRYPTO_ONLY', # interacts with *_USE_A64_CRYPTO_IF_PRESENT 'MBEDTLS_SHA512_USE_A64_CRYPTO_ONLY', # interacts with *_USE_A64_CRYPTO_IF_PRESENT + 'MBEDTLS_SSL_RECORD_SIZE_LIMIT', # in development, currently breaks other tests 'MBEDTLS_TEST_CONSTANT_FLOW_MEMSAN', # build dependency (clang+memsan) 'MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND', # build dependency (valgrind headers) 'MBEDTLS_X509_REMOVE_INFO', # removes a feature - 'MBEDTLS_SSL_RECORD_SIZE_LIMIT', # in development, currently breaks other tests - 'MBEDTLS_P256M_EXAMPLE_DRIVER_ENABLED', # influences SECP256R1 KeyGen/ECDH/ECDSA - 'MBEDTLS_ECP_WITH_MPI_UINT' # disables the default ECP and is experimental ]) def is_seamless_alt(name):