Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check mbedtls_platform_zeroize() calls #8143

Merged
merged 6 commits into from
Sep 3, 2023
Merged

Check mbedtls_platform_zeroize() calls #8143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
42b02a9
Add the ability to verify mbedtls_platform_zeroize() calls with -Wsiz…
tom-cosgrove-arm Sep 1, 2023
351a391
Fix incorrect use of mbedtls_platform_zeroize() in tests
tom-cosgrove-arm Sep 1, 2023
daddf11
Add a build to all.sh to check mbedtls_platform_zeroize() calls
tom-cosgrove-arm Sep 1, 2023
7eced7d
Move zeroize-as-memset into a config file under tests/
tom-cosgrove-arm Sep 1, 2023
d9572c0
Move the description of MBEDTLS_TEST_DEFINES_ZEROIZE to before its use
tom-cosgrove-arm Sep 1, 2023
b2fafa5
config-wrapper-zeroize-memset.h should be user-config-zeroize-memset.…
tom-cosgrove-arm Sep 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions include/mbedtls/platform_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,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
*
Expand All @@ -168,6 +173,7 @@ MBEDTLS_DEPRECATED typedef int mbedtls_deprecated_numeric_constant_t;
*
*/
void mbedtls_platform_zeroize(void *buf, size_t len);
#endif

#if defined(MBEDTLS_HAVE_TIME_DATE)
/**
Expand Down
29 changes: 29 additions & 0 deletions tests/configs/user-config-zeroize-memset.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/* mbedtls_config.h modifier 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 <string.h>

/* 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could define MBEDTLS_TEST_DEFINES_ZEROIZE in this file. It's ok if defining MBEDTLS_USER_CONFIG_FILE (factor 1) to point to a file under tests (factor 2) results in an insecure build. But needing both definitions is ok too, this isn't a thing we need to do often.

* declaration. */
#define MBEDTLS_PLATFORM_ZEROIZE_ALT

#define mbedtls_platform_zeroize(buf, len) memset(buf, 0, len)
10 changes: 10 additions & 0 deletions tests/scripts/all.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5117,6 +5117,16 @@ support_build_cmake_custom_config_file () {
}


component_build_zeroize_checks () {
msg "build: check for obviously wrong calls to mbedtls_platform_zeroize()"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A side note, we've considered running clang-tidy and its bugprone-sizeof-expression looks for the same kind of problem. I don't know whether it would catch invalid calls in a normal build or if we'd need this memset replacement.

I thought we had an open issue for clang-tidy but turns out we don't, I'll file one (as part of the huge tech debt backlog, so don't hold your breath).


scripts/config.py full

# Only compile - we're looking for sizeof-pointer-memaccess warnings
make CC=gcc CFLAGS="'-DMBEDTLS_USER_CONFIG_FILE=\"../tests/configs/user-config-zeroize-memset.h\"' -DMBEDTLS_TEST_DEFINES_ZEROIZE -Werror -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
Expand Down
2 changes: 1 addition & 1 deletion tests/suites/test_suite_pkwrite.function
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,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, PSA_KEY_USAGE_EXPORT,
Expand Down