From 59c92ed89b77786a96ca768d5c76d020b7bde0ec Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 19 Jul 2019 12:42:21 +0100 Subject: [PATCH 01/12] ECP restart: Don't calculate address of sub ctx if ctx is NULL All modules using restartable ECC operations support passing `NULL` as the restart context as a means to not use the feature. The restart contexts for ECDSA and ECP are nested, and when calling restartable ECP operations from restartable ECDSA operations, the address of the ECP restart context to use is calculated by adding the to the address of the ECDSA restart context the offset the of the ECP restart context. If the ECP restart context happens to not reside at offset `0`, this leads to a non-`NULL` pointer being passed to restartable ECP operations from restartable ECDSA-operations; those ECP operations will hence assume that the pointer points to a valid ECP restart address and likely run into a segmentation fault when trying to dereference the non-NULL but close-to-NULL address. The problem doesn't arise currently because luckily the ECP restart context has offset 0 within the ECDSA restart context, but we should not rely on it. This commit fixes the passage from restartable ECDSA to restartable ECP operations by propagating NULL as the restart context pointer. Apart from being fragile, the previous version could also lead to NULL pointer dereference failures in ASanDbg builds which dereferenced the ECDSA restart context even though it's not needed to calculate the address of the offset'ed ECP restart context. dummy --- library/ecdsa.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/ecdsa.c b/library/ecdsa.c index dc19384d6..58e1a5fce 100644 --- a/library/ecdsa.c +++ b/library/ecdsa.c @@ -172,11 +172,11 @@ static void ecdsa_restart_det_free( mbedtls_ecdsa_restart_det_ctx *ctx ) } #endif /* MBEDTLS_ECDSA_DETERMINISTIC */ -#define ECDSA_RS_ECP &rs_ctx->ecp +#define ECDSA_RS_ECP ( rs_ctx == NULL ? NULL : &rs_ctx->ecp ) /* Utility macro for checking and updating ops budget */ #define ECDSA_BUDGET( ops ) \ - MBEDTLS_MPI_CHK( mbedtls_ecp_check_budget( grp, &rs_ctx->ecp, ops ) ); + MBEDTLS_MPI_CHK( mbedtls_ecp_check_budget( grp, ECDSA_RS_ECP, ops ) ); /* Call this when entering a function that needs its own sub-context */ #define ECDSA_RS_ENTER( SUB ) do { \ From bf84d503b31a41f073e30aa6a186736248814a12 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 19 Jul 2019 12:52:08 +0100 Subject: [PATCH 02/12] Adapt ChangeLog --- ChangeLog | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ChangeLog b/ChangeLog index 37373a23c..f195d2622 100644 --- a/ChangeLog +++ b/ChangeLog @@ -79,6 +79,9 @@ Bugfix This previously limited the maximum size of DER encoded certificates in mbedtls_x509write_crt_der() to 2Kb. Reported by soccerGB in #2631. * Fix partial zeroing in x509_get_other_name. Found and fixed by ekse, #2716. + * Fix propagation of restart contexts in restartable EC operations. + This could previously lead to segmentation faults in builds using an + address-sanitizer and enabling but not using MBEDTLS_ECP_RESTARTABLE. API Changes * Extend the MBEDTLS_SSL_EXPORT_KEYS to export the handshake randbytes, From 5daa34f155c758a9f626bce88b527b92598c33b7 Mon Sep 17 00:00:00 2001 From: Aurelien Jarno Date: Sat, 3 Nov 2018 00:46:06 +0100 Subject: [PATCH 03/12] bn_mul.h: require at least ARMv6 to enable the ARM DSP code Commit 16b1bd89326e "bn_mul.h: add ARM DSP optimized MULADDC code" added some ARM DSP instructions that was assumed to always be available when __ARM_FEATURE_DSP is defined to 1. Unfortunately it appears that the ARMv5TE architecture (GCC flag -march=armv5te) supports the DSP instructions, but only in Thumb mode and not in ARM mode, despite defining __ARM_FEATURE_DSP in both cases. This patch fixes the build issue by requiring at least ARMv6 in addition to the DSP feature. --- include/mbedtls/bn_mul.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/bn_mul.h b/include/mbedtls/bn_mul.h index c33bd8d4a..748975ea5 100644 --- a/include/mbedtls/bn_mul.h +++ b/include/mbedtls/bn_mul.h @@ -642,7 +642,8 @@ "r6", "r7", "r8", "r9", "cc" \ ); -#elif defined (__ARM_FEATURE_DSP) && (__ARM_FEATURE_DSP == 1) +#elif (__ARM_ARCH >= 6) && \ + defined (__ARM_FEATURE_DSP) && (__ARM_FEATURE_DSP == 1) #define MULADDC_INIT \ asm( From a5cb7d48f35aeb8e3a78242614080c920bb21f9c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 5 Aug 2019 11:34:11 +0200 Subject: [PATCH 04/12] Add changelog entry for ARM assembly fix --- ChangeLog | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ChangeLog b/ChangeLog index 37373a23c..e1110a154 100644 --- a/ChangeLog +++ b/ChangeLog @@ -79,6 +79,9 @@ Bugfix This previously limited the maximum size of DER encoded certificates in mbedtls_x509write_crt_der() to 2Kb. Reported by soccerGB in #2631. * Fix partial zeroing in x509_get_other_name. Found and fixed by ekse, #2716. + * Fix the build on ARMv5TE in ARM mode to not use assembly instructions + that are only available in Thumb mode. Fix contributed by Aurelien Jarno + in #2169. API Changes * Extend the MBEDTLS_SSL_EXPORT_KEYS to export the handshake randbytes, From 93e4e03f9473306afe077c6631924fadf16b3f82 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 5 Aug 2019 11:34:25 +0200 Subject: [PATCH 05/12] Add a build on ARMv5TE in ARM mode Non-regression test for "bn_mul.h: require at least ARMv6 to enable the ARM DSP code" --- tests/scripts/all.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 22c81296c..d9ef302ab 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1088,6 +1088,12 @@ component_build_arm_none_eabi_gcc () { make CC=arm-none-eabi-gcc AR=arm-none-eabi-ar LD=arm-none-eabi-ld CFLAGS='-Werror -Wall -Wextra' lib } +component_build_arm_none_eabi_gcc_armel () { + msg "build: arm-none-eabi-gcc, make" # ~ 10s + scripts/config.pl baremetal + make CC=arm-none-eabi-gcc AR=arm-none-eabi-ar CFLAGS='-Werror -Wall -Wextra -march=armv5te' LDFLAGS='-march=armv5te' SHELL='sh -x' lib +} + component_build_arm_none_eabi_gcc_no_udbl_division () { msg "build: arm-none-eabi-gcc -DMBEDTLS_NO_UDBL_DIVISION, make" # ~ 10s scripts/config.pl baremetal From 8a52af9b7704b7c0e17eb7f56dc0fdfe94ec51b5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 8 Aug 2019 16:09:02 +0200 Subject: [PATCH 06/12] Switch armel build to -Os Without any -O option, the default is -O0, and then the assembly code is not used, so this would not be a non-regression test for the assembly code that doesn't build. --- tests/scripts/all.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index d9ef302ab..739f06143 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1089,9 +1089,9 @@ component_build_arm_none_eabi_gcc () { } component_build_arm_none_eabi_gcc_armel () { - msg "build: arm-none-eabi-gcc, make" # ~ 10s + msg "build: arm-none-eabi-gcc -march=arm5vte, make" # ~ 10s scripts/config.pl baremetal - make CC=arm-none-eabi-gcc AR=arm-none-eabi-ar CFLAGS='-Werror -Wall -Wextra -march=armv5te' LDFLAGS='-march=armv5te' SHELL='sh -x' lib + make CC=arm-none-eabi-gcc AR=arm-none-eabi-ar CFLAGS='-Werror -Wall -Wextra -march=armv5te -O1' LDFLAGS='-march=armv5te' SHELL='sh -x' lib } component_build_arm_none_eabi_gcc_no_udbl_division () { From 2c897d76ff6f3d6891cbfe55703f3c1dfada0fc3 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 9 Aug 2019 16:05:05 +0200 Subject: [PATCH 07/12] Document the rationale for the armel build Call the component xxx_arm5vte, because that's what it does. Explain "armel", and more generally why this component exists, in a comment. --- tests/scripts/all.sh | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 739f06143..e49277a85 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1088,9 +1088,14 @@ component_build_arm_none_eabi_gcc () { make CC=arm-none-eabi-gcc AR=arm-none-eabi-ar LD=arm-none-eabi-ld CFLAGS='-Werror -Wall -Wextra' lib } -component_build_arm_none_eabi_gcc_armel () { +component_build_arm_none_eabi_gcc_arm5vte () { msg "build: arm-none-eabi-gcc -march=arm5vte, make" # ~ 10s scripts/config.pl baremetal + # Build for a target platform that's close to what Debian uses + # for its "armel" distribution (https://wiki.debian.org/ArmEabiPort). + # See https://github.com/ARMmbed/mbedtls/pull/2169 and comments. + # It would be better to build with arm-linux-gnueabi-gcc but + # we don't have that on our CI at this time. make CC=arm-none-eabi-gcc AR=arm-none-eabi-ar CFLAGS='-Werror -Wall -Wextra -march=armv5te -O1' LDFLAGS='-march=armv5te' SHELL='sh -x' lib } From 6eece5b666a36094f5ebef93b9683df06b81c698 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sat, 10 Aug 2019 17:38:34 +0200 Subject: [PATCH 08/12] Exclude DTLS 1.2 only with older OpenSSL compat.sh used to skip OpenSSL altogether for DTLS 1.2, because older versions of OpenSSL didn't support it. But these days it is supported. We don't want to use DTLS 1.2 with OpenSSL unconditionally, because we still use legacy versions of OpenSSL to test with legacy ciphers. So check whether the version we're using supports it. --- tests/compat.sh | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/tests/compat.sh b/tests/compat.sh index 80c2d31a3..54bc0b7d1 100755 --- a/tests/compat.sh +++ b/tests/compat.sh @@ -216,14 +216,13 @@ filter_ciphersuites() G_CIPHERS=$( filter "$G_CIPHERS" ) fi - # OpenSSL 1.0.1h doesn't support DTLS 1.2 - if [ `minor_ver "$MODE"` -ge 3 ] && is_dtls "$MODE"; then + # OpenSSL <1.0.2 doesn't support DTLS 1.2. Check what OpenSSL + # supports from the s_server help. (The s_client help isn't + # accurate as of 1.0.2g: it supports DTLS 1.2 but doesn't list it. + # But the s_server help seems to be accurate.) + if ! $OPENSSL_CMD s_server -help 2>&1 | grep -q "^ *-$MODE "; then + M_CIPHERS="" O_CIPHERS="" - case "$PEER" in - [Oo]pen*) - M_CIPHERS="" - ;; - esac fi # For GnuTLS client -> mbed TLS server, From abf9b4dee8f2a321cca3f7256f9d4faede368fc8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 3 Jul 2019 20:42:16 +0200 Subject: [PATCH 09/12] Add a test of MBEDTLS_CONFIG_FILE configs/README.txt documents that you can use an alternative configuration file by defining the preprocessor symbol MBEDTLS_CONFIG_FILE. Test this. --- tests/scripts/all.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 22c81296c..89e0c7e08 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1032,6 +1032,17 @@ component_test_make_shared () { make SHARED=1 all check -j1 } +component_build_mbedtls_config_file () { + msg "build: make with MBEDTLS_CONFIG_FILE" # ~40s + # Use the full config so as to catch a maximum of places where + # the check of MBEDTLS_CONFIG_FILE might be missing. + scripts/config.pl full + sed 's!"check_config.h"!"mbedtls/check_config.h"!' <"$CONFIG_H" >full_config.h + echo '#error "MBEDTLS_CONFIG_FILE is not working"' >"$CONFIG_H" + make CFLAGS="-I '$PWD' -DMBEDTLS_CONFIG_FILE='\"full_config.h\"'" + rm -f full_config.h +} + component_test_m32_o0 () { # Build once with -O0, to compile out the i386 specific inline assembly msg "build: i386, make, gcc -O0 (ASan build)" # ~ 30s From cf74050fead788c04895f434185be993a80347d7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 3 Jul 2019 20:43:05 +0200 Subject: [PATCH 10/12] Test that the shared library build with CMake works --- tests/scripts/all.sh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 89e0c7e08..cff1f00e2 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1032,6 +1032,13 @@ component_test_make_shared () { make SHARED=1 all check -j1 } +component_test_cmake_shared () { + msg "build/test: cmake shared" # ~ 2min + cmake -DUSE_SHARED_MBEDTLS_LIBRARY=On . + make + make test +} + component_build_mbedtls_config_file () { msg "build: make with MBEDTLS_CONFIG_FILE" # ~40s # Use the full config so as to catch a maximum of places where From 56c0161b6823ad316590b8d19fcf1815a05d8403 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 3 Jul 2019 20:43:32 +0200 Subject: [PATCH 11/12] Test that a shared library build produces a dynamically linked executable --- tests/scripts/all.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index cff1f00e2..6a1d194f0 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1030,12 +1030,14 @@ component_test_platform_calloc_macro () { component_test_make_shared () { msg "build/test: make shared" # ~ 40s make SHARED=1 all check -j1 + ldd programs/util/strerror | grep libmbedcrypto } component_test_cmake_shared () { msg "build/test: cmake shared" # ~ 2min cmake -DUSE_SHARED_MBEDTLS_LIBRARY=On . make + ldd programs/util/strerror | grep libmbedcrypto make test } From 26f3e2800d53f1030782d768657ccb9b42f8b640 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 Aug 2019 18:00:02 +0200 Subject: [PATCH 12/12] Honor MBEDTLS_CONFIG_FILE in fuzz tests --- programs/fuzz/onefile.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/programs/fuzz/onefile.c b/programs/fuzz/onefile.c index 9e3986d6b..c84514963 100644 --- a/programs/fuzz/onefile.c +++ b/programs/fuzz/onefile.c @@ -1,8 +1,15 @@ #include #include #include -// Get platform-specific definition + +/* This file doesn't use any Mbed TLS function, but grab config.h anyway + * in case it contains platform-specific #defines related to malloc or + * stdio functions. */ +#if !defined(MBEDTLS_CONFIG_FILE) #include "mbedtls/config.h" +#else +#include MBEDTLS_CONFIG_FILE +#endif int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size);