From 43210b56f349bcb330e04756aa83c13125293b53 Mon Sep 17 00:00:00 2001 From: Tom Cosgrove Date: Fri, 1 Sep 2023 09:53:42 +0100 Subject: [PATCH 1/6] Add the ability to verify mbedtls_platform_zeroize() calls with -Wsizeof-pointer-memaccess Signed-off-by: Tom Cosgrove --- include/mbedtls/platform_util.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/include/mbedtls/platform_util.h b/include/mbedtls/platform_util.h index 55fc43113..55eb77dc7 100644 --- a/include/mbedtls/platform_util.h +++ b/include/mbedtls/platform_util.h @@ -243,7 +243,28 @@ MBEDTLS_DEPRECATED typedef int mbedtls_deprecated_numeric_constant_t; * \param len Length of the buffer in bytes * */ +#if defined(MBEDTLS_PLATFORM_ZEROIZE_CHECK_UNSAFE) +#define MBEDTLS_PLATFORM_ZEROIZE_ALT +#define mbedtls_platform_zeroize(buf, len) memset(buf, 0, len) +#include +#else void mbedtls_platform_zeroize(void *buf, size_t len); +#endif + +/* MBEDTLS_PLATFORM_ZEROIZE_CHECK_UNSAFE + * + * Replaces calls to mbedtls_platform_zeroize() with calls to memset(), + * to allow compiler analysis to check for invalid length arguments (e.g. + * specifying sizeof(pointer) rather than sizeof(pointee)). + * + * Note that this option is meant for internal use only and must not be used + * in production builds, because that would lead to zeroization calls being + * optimised out by the compiler. + * + * It is only intended to be used in CFLAGS, with -Wsizeof-pointer-memaccess, + * to check for those incorrect calls to mbedtls_platform_zeroize(). + */ +//#define MBEDTLS_PLATFORM_ZEROIZE_CHECK_UNSAFE #if defined(MBEDTLS_HAVE_TIME_DATE) /** From f7829b099d4c9a348f02d3236957924ad6738f7b Mon Sep 17 00:00:00 2001 From: Tom Cosgrove Date: Fri, 1 Sep 2023 09:54:04 +0100 Subject: [PATCH 2/6] Fix incorrect use of mbedtls_platform_zeroize() in tests Signed-off-by: Tom Cosgrove --- tests/suites/test_suite_pkwrite.function | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_pkwrite.function b/tests/suites/test_suite_pkwrite.function index c5391ba39..fb71d347e 100644 --- a/tests/suites/test_suite_pkwrite.function +++ b/tests/suites/test_suite_pkwrite.function @@ -154,7 +154,7 @@ void pk_write_public_from_private(char *priv_key_file, char *pub_key_file) pub_key_raw, pub_key_len); #if defined(MBEDTLS_USE_PSA_CRYPTO) - mbedtls_platform_zeroize(derived_key_raw, sizeof(derived_key_raw)); + mbedtls_platform_zeroize(derived_key_raw, derived_key_len); TEST_EQUAL(mbedtls_pk_wrap_as_opaque(&priv_key, &opaque_key_id, PSA_ALG_NONE), 0); From 5117062bb619345f81ac40e1f91626f8887a0d0c Mon Sep 17 00:00:00 2001 From: Tom Cosgrove Date: Fri, 1 Sep 2023 10:40:15 +0100 Subject: [PATCH 3/6] Add a build to all.sh to check mbedtls_platform_zeroize() calls Signed-off-by: Tom Cosgrove --- tests/scripts/all.sh | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 5cbf19ed6..f57895700 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -3536,6 +3536,16 @@ support_build_cmake_custom_config_file () { } +component_build_zeroize_checks () { + msg "build: check for obviously wrong calls to mbedtls_platform_zeroize()" + + scripts/config.py full + + # Only compile - we're looking for sizeof-pointer-memaccess warnings + make CC=gcc CFLAGS='-Werror -DMBEDTLS_PLATFORM_ZEROIZE_CHECK_UNSAFE -Wsizeof-pointer-memaccess' +} + + component_test_zeroize () { # Test that the function mbedtls_platform_zeroize() is not optimized away by # different combinations of compilers and optimization flags by using an From 7f18f440536021e9867d5e4dacba3f05c4d56c56 Mon Sep 17 00:00:00 2001 From: Tom Cosgrove Date: Fri, 1 Sep 2023 13:55:39 +0100 Subject: [PATCH 4/6] Move zeroize-as-memset into a config file under tests/ Signed-off-by: Tom Cosgrove --- include/mbedtls/platform_util.h | 23 ++++---------- tests/configs/config-wrapper-zeroize-memset.h | 31 +++++++++++++++++++ tests/scripts/all.sh | 2 +- 3 files changed, 38 insertions(+), 18 deletions(-) create mode 100644 tests/configs/config-wrapper-zeroize-memset.h diff --git a/include/mbedtls/platform_util.h b/include/mbedtls/platform_util.h index 55eb77dc7..70dc51a8b 100644 --- a/include/mbedtls/platform_util.h +++ b/include/mbedtls/platform_util.h @@ -243,28 +243,17 @@ MBEDTLS_DEPRECATED typedef int mbedtls_deprecated_numeric_constant_t; * \param len Length of the buffer in bytes * */ -#if defined(MBEDTLS_PLATFORM_ZEROIZE_CHECK_UNSAFE) -#define MBEDTLS_PLATFORM_ZEROIZE_ALT -#define mbedtls_platform_zeroize(buf, len) memset(buf, 0, len) -#include -#else +#if !defined(MBEDTLS_TEST_DEFINES_ZEROIZE) void mbedtls_platform_zeroize(void *buf, size_t len); #endif -/* MBEDTLS_PLATFORM_ZEROIZE_CHECK_UNSAFE +/* MBEDTLS_TEST_DEFINES_ZEROIZE * - * Replaces calls to mbedtls_platform_zeroize() with calls to memset(), - * to allow compiler analysis to check for invalid length arguments (e.g. - * specifying sizeof(pointer) rather than sizeof(pointee)). - * - * Note that this option is meant for internal use only and must not be used - * in production builds, because that would lead to zeroization calls being - * optimised out by the compiler. - * - * It is only intended to be used in CFLAGS, with -Wsizeof-pointer-memaccess, - * to check for those incorrect calls to mbedtls_platform_zeroize(). + * Indicates that the library is being built by the test framework, and the + * framework is going to provide a replacement mbedtls_platform_zeroize() + * using a pre-processor macro, so the function declaration should be omitted. */ -//#define MBEDTLS_PLATFORM_ZEROIZE_CHECK_UNSAFE +//#define MBEDTLS_TEST_DEFINES_ZEROIZE #if defined(MBEDTLS_HAVE_TIME_DATE) /** diff --git a/tests/configs/config-wrapper-zeroize-memset.h b/tests/configs/config-wrapper-zeroize-memset.h new file mode 100644 index 000000000..d1bfa1717 --- /dev/null +++ b/tests/configs/config-wrapper-zeroize-memset.h @@ -0,0 +1,31 @@ +/* mbedtls_config.h wrapper that defines mbedtls_platform_zeroize() to be + * memset(), so that the compile can check arguments for us. + * Used for testing. + */ +/* + * Copyright The Mbed TLS Contributors + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "mbedtls/mbedtls_config.h" + +#include + +/* Define _ALT so we don't get the built-in implementation. The test code will + * also need to define MBEDTLS_TEST_DEFINES_ZEROIZE so we don't get the + * declaration. */ +#define MBEDTLS_PLATFORM_ZEROIZE_ALT + +#define mbedtls_platform_zeroize(buf, len) memset(buf, 0, len) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index f57895700..0232711a5 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -3542,7 +3542,7 @@ component_build_zeroize_checks () { scripts/config.py full # Only compile - we're looking for sizeof-pointer-memaccess warnings - make CC=gcc CFLAGS='-Werror -DMBEDTLS_PLATFORM_ZEROIZE_CHECK_UNSAFE -Wsizeof-pointer-memaccess' + make CC=gcc CFLAGS="'-DMBEDTLS_USER_CONFIG_FILE=\"../tests/configs/config-wrapper-zeroize-memset.h\"' -DMBEDTLS_TEST_DEFINES_ZEROIZE -Werror -Wsizeof-pointer-memaccess" } From 95b5d79cbfde05ea51e4c1ae715a419700cc1f39 Mon Sep 17 00:00:00 2001 From: Tom Cosgrove Date: Fri, 1 Sep 2023 14:34:37 +0100 Subject: [PATCH 5/6] Move the description of MBEDTLS_TEST_DEFINES_ZEROIZE to before its use Signed-off-by: Tom Cosgrove --- include/mbedtls/platform_util.h | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/include/mbedtls/platform_util.h b/include/mbedtls/platform_util.h index 70dc51a8b..62f6d7038 100644 --- a/include/mbedtls/platform_util.h +++ b/include/mbedtls/platform_util.h @@ -221,6 +221,11 @@ MBEDTLS_DEPRECATED typedef int mbedtls_deprecated_numeric_constant_t; #define MBEDTLS_IGNORE_RETURN(result) ((void) !(result)) #endif +/* If the following macro is defined, the library is being built by the test + * framework, and the framework is going to provide a replacement + * mbedtls_platform_zeroize() using a preprocessor macro, so the function + * declaration should be omitted. */ +#if !defined(MBEDTLS_TEST_DEFINES_ZEROIZE) //no-check-names /** * \brief Securely zeroize a buffer * @@ -243,18 +248,9 @@ MBEDTLS_DEPRECATED typedef int mbedtls_deprecated_numeric_constant_t; * \param len Length of the buffer in bytes * */ -#if !defined(MBEDTLS_TEST_DEFINES_ZEROIZE) void mbedtls_platform_zeroize(void *buf, size_t len); #endif -/* MBEDTLS_TEST_DEFINES_ZEROIZE - * - * Indicates that the library is being built by the test framework, and the - * framework is going to provide a replacement mbedtls_platform_zeroize() - * using a pre-processor macro, so the function declaration should be omitted. - */ -//#define MBEDTLS_TEST_DEFINES_ZEROIZE - #if defined(MBEDTLS_HAVE_TIME_DATE) /** * \brief Platform-specific implementation of gmtime_r() From 5ffb19741d27b0fe62bf30483940a8425e2b5a12 Mon Sep 17 00:00:00 2001 From: Tom Cosgrove Date: Fri, 1 Sep 2023 14:40:21 +0100 Subject: [PATCH 6/6] config-wrapper-zeroize-memset.h should be user-config-zeroize-memset.h and not include mbedtls_config.h Signed-off-by: Tom Cosgrove --- ...-wrapper-zeroize-memset.h => user-config-zeroize-memset.h} | 4 +--- tests/scripts/all.sh | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) rename tests/configs/{config-wrapper-zeroize-memset.h => user-config-zeroize-memset.h} (90%) diff --git a/tests/configs/config-wrapper-zeroize-memset.h b/tests/configs/user-config-zeroize-memset.h similarity index 90% rename from tests/configs/config-wrapper-zeroize-memset.h rename to tests/configs/user-config-zeroize-memset.h index d1bfa1717..fcdd1f099 100644 --- a/tests/configs/config-wrapper-zeroize-memset.h +++ b/tests/configs/user-config-zeroize-memset.h @@ -1,4 +1,4 @@ -/* mbedtls_config.h wrapper that defines mbedtls_platform_zeroize() to be +/* mbedtls_config.h modifier that defines mbedtls_platform_zeroize() to be * memset(), so that the compile can check arguments for us. * Used for testing. */ @@ -19,8 +19,6 @@ * limitations under the License. */ -#include "mbedtls/mbedtls_config.h" - #include /* Define _ALT so we don't get the built-in implementation. The test code will diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 0232711a5..4ed1949a2 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -3542,7 +3542,7 @@ component_build_zeroize_checks () { scripts/config.py full # Only compile - we're looking for sizeof-pointer-memaccess warnings - make CC=gcc CFLAGS="'-DMBEDTLS_USER_CONFIG_FILE=\"../tests/configs/config-wrapper-zeroize-memset.h\"' -DMBEDTLS_TEST_DEFINES_ZEROIZE -Werror -Wsizeof-pointer-memaccess" + make CC=gcc CFLAGS="'-DMBEDTLS_USER_CONFIG_FILE=\"../tests/configs/user-config-zeroize-memset.h\"' -DMBEDTLS_TEST_DEFINES_ZEROIZE -Werror -Wsizeof-pointer-memaccess" }