Check mbedtls_platform_zeroize() calls #8143

tom-cosgrove-arm · 2023-09-01T08:58:43Z

Description

Add the ability to verify mbedtls_platform_zeroize() calls with -Wsizeof-pointer-memaccess, and fix a problem it finds in the tests.

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

changelog not required - this is internal only
backport Backport 2.28: Check mbedtls_platform_zeroize() calls #8145
tests present - now that Fix TLS pad buffer zeroization #8068 is merged this can be merged too

tom-cosgrove-arm · 2023-09-01T10:31:55Z

Great, this catches the expected failure:

[2023-09-01T10:19:29.832Z] In file included from ../include/mbedtls/ssl.h:24:0,
[2023-09-01T10:19:29.832Z]                  from ssl_tls.c:30:
[2023-09-01T10:19:29.832Z] ssl_tls.c: In function 'ssl_calc_finished_tls_generic':
[2023-09-01T10:19:29.832Z] ssl_tls.c:7725:44: error: argument to 'sizeof' in 'memset' call is the same expression as the destination; did you mean to provide an explicit length? [-Werror=sizeof-pointer-memaccess]
[2023-09-01T10:19:29.832Z]      mbedtls_platform_zeroize(padbuf, sizeof(padbuf));
[2023-09-01T10:19:29.832Z]                                             ^
[2023-09-01T10:19:29.832Z] ../include/mbedtls/platform_util.h:172:59: note: in definition of macro 'mbedtls_platform_zeroize'
[2023-09-01T10:19:29.832Z]  #define mbedtls_platform_zeroize(buf, len) memset(buf, 0, len)
[2023-09-01T10:19:29.832Z]                                                            ^
[2023-09-01T10:19:29.832Z]   CC    ssl_tls12_client.c
[2023-09-01T10:19:30.091Z] cc1: all warnings being treated as errors
[2023-09-01T10:19:30.091Z] Makefile:308: recipe for target 'ssl_tls.o' failed
[2023-09-01T10:19:30.091Z] make[1]: *** [ssl_tls.o] Error 1
[2023-09-01T10:19:30.091Z] make[1]: *** Waiting for unfinished jobs....
[2023-09-01T10:19:30.091Z] Makefile:18: recipe for target 'lib' failed
[2023-09-01T10:19:30.091Z] make: *** [lib] Error 2
[2023-09-01T10:19:30.091Z] ^^^^build_zeroize_checks: build: check for obviously wrong calls to mbedtls_platform_zeroize(): make CC=gcc CFLAGS='-Werror -DMBEDTLS_PLATFORM_ZEROIZE_CHECK_UNSAFE -Wsizeof-pointer-memaccess' -> 2^^^^

gilles-peskine-arm

The test configuration is leaking into the library.

gilles-peskine-arm · 2023-09-01T11:00:09Z

include/mbedtls/platform_util.h

+ * 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


I really don't like having test-only code that reduces security in the library. Our normal policy for this is a two-tier system: enable a compilation option that by itself doesn't change the runtime behavior, and provide or run additional code that has to be deliberately written.

Here, we can already achieve the effect by activating MBEDTLS_PLATFORM_ZEROIZE_ALT and building with an implementation of mbedtls_platform_zeroize, which lives in the tests/src directory, that just calls memset. (Similar example: we can build with a deterministic, non-cryptographic RNG by activating MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG and linking with the external RNG from tests/src/fake_external_rng_for_test.c.) See tests/configs/user-config-for-test.h and tests/configs/config-wrapper-malloc-0-null.h for examples doing something similar.

We actually need the compiler to see the calls to mbedtls_platform_zeroize() as memset(), rather to have a call to mbedtls_platform_zeroize() be implemented as memset(), since it's at the mbedtls_platform_zeroize() call site that the information is available about the types of parameters, which is what need to be checked.

This can only be done with a macro, so the choice is between having it in the header permanently, like this, or having the test use a perl (or sed, I suppose) script to make these changes when we run the test build. However, that would be fragile.

Also, I disagree that this is test-only code: rather, it is enabling static analysis. If there was an annotation comparable to the __attribute__ ((format (...))) one for printf-style function then we would apply it to this function definition at this point; this is very similar to that, so I think this is the right way to do this.

See comment below - we can add a check for MBEDTLS_TEST_HOOKS to limit the risk?

It's annoying that this can't be done with the existing option. (I checked, and indeed even compiling with -O3 and a static inline wrapper with __attribute__((access(write_only, 1, 2))) is not enough to get the warning.) Food for thought when we simplify the platform interface: this is a thing you can't do with a function replacement, only if you
can define a macro, the blocking thing being the function prototype. (If we manage to skip the function prototype then we can define a macro and all is good.)

So this does require a patch to platform_util.h. But (unless we actually patch the file before building, which I'd rather avoid), this patch should still not, by itself, make an insecure build as easy as -D. Gating by MBEDTLS_TEST_HOOKS doesn't help: there's already a compile-time option to gate the insecurity (which should be called MBEDTLS_TEST_something), but our policy is a two-tier system to make it a genuine hurdle to make an insecure build: an insecure build should need to either write your own insecure code or compile with something under tests.

The minimum patch we need in platform_util.h is to skip the prototype. So I propose to have a symbol (not referenced outside platform_util.h and test code) that does just that. The #define mbedtls_platform_zeroize … should be in a file under tests.

daverodgman · 2023-09-01T11:47:13Z

include/mbedtls/platform_util.h

@@ -167,7 +167,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)


Suggested change

#if defined(MBEDTLS_PLATFORM_ZEROIZE_CHECK_UNSAFE)

#if defined(MBEDTLS_PLATFORM_ZEROIZE_CHECK_UNSAFE) && defined(MBEDTLS_TEST_HOOKS)

Maybe this can help reduce the risk of this accidentally getting enabled?

Well, MBEDTLS_TEST_HOOKS, is a documented config option in mbedtls_config.h. It has to be manually enabled in all.sh if wanted. The name has got "TEST" in the name, indicating that it might not necessarily be wanted in production.

MBEDTLS_PLATFORM_ZEROIZE_CHECK_UNSAFE is only documented just under where it is used (to placate check-names, otherwise I would have not put it in). It needs to be manually enabled when wanted, which it is in the test in all.sh, and to enable it in the configuration file requires typing it in, rather than removing comments. The name has got "UNSAFE" in it, indicating that it really shouldn't be used in production. It's not in mbedtls_config.h, so someone running through enabling everything without careful reading is not going to get it.

So: this would seem to be the equivalent of needing to turn two keys before launching a missile, rather than just one.

I'm happy to add it if you think it really necessary, but I think MBEDTLS_TEST_HOOKS is more likely to be accidentally enabled than MBEDTLS_PLATFORM_ZEROIZE_CHECK_UNSAFE is, so I don't know if it's actually worth it.

So: this would seem to be the equivalent of needing to turn two keys before launching a missile, rather than just one.

Yes, which is why our policy is that it takes more than doing some key turning to launch a missile. MBEDTLS_TEST_HOOKS by itself must not enable any insecure behavior, only the possibility of injecting some code (not from the library) which itself may be insecure.

daverodgman

LGTM

gilles-peskine-arm

The overall design looks good now.

include/mbedtls/platform_util.h

tests/configs/config-wrapper-zeroize-memset.h

gilles-peskine-arm

LGTM

gilles-peskine-arm · 2023-09-01T14:11:58Z

tests/configs/user-config-zeroize-memset.h

+#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


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.

gilles-peskine-arm · 2023-09-01T14:17:09Z

tests/scripts/all.sh

@@ -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()"


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).

tom-cosgrove-arm · 2023-09-01T19:32:05Z

The all_u16-build_zeroize_checks failures are expected until #8068 is merged.

The other build failure, all_u18-build_mingw, is unrelated to this PR:

[2023-09-01T16:29:48.075Z]  > git fetch --no-tags --force --progress -- https://github.com/Mbed-TLS/mbedtls.git +refs/pull/8143/head:refs/remotes/origin/PR-8143-head # timeout=10
[2023-09-01T16:30:04.013Z] ERROR: Error cloning remote repo 'origin'
[2023-09-01T16:30:04.013Z] hudson.plugins.git.GitException: Command "git fetch --no-tags --force --progress -- https://github.com/Mbed-TLS/mbedtls.git +refs/pull/8143/head:refs/remotes/origin/PR-8143-head" returned status code 128:
[2023-09-01T16:30:04.013Z] stdout: 
[2023-09-01T16:30:04.013Z] stderr: remote: Internal Server Error
[2023-09-01T16:30:04.013Z] fatal: unable to access 'https://github.com/Mbed-TLS/mbedtls.git/': The requested URL returned error: 500

tom-cosgrove-arm · 2023-09-02T18:20:31Z

Editing the description seems to have re-triggered the CI, good

tom-cosgrove-arm · 2023-09-02T18:21:18Z

Ah - except I need to rebase onto development first, damn

…eof-pointer-memaccess Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>

Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>

…h and not include mbedtls_config.h Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>

tom-cosgrove-arm · 2023-09-02T18:24:26Z

Rebased on top of latest development with no changes so that the tests will pass, now that #8068 is in

$ git rebase development
First, rewinding head to replay your work on top of it...
Applying: Add the ability to verify mbedtls_platform_zeroize() calls with -Wsizeof-pointer-memaccess
Applying: Fix incorrect use of mbedtls_platform_zeroize() in tests
Applying: Add a build to all.sh to check mbedtls_platform_zeroize() calls
Applying: Move zeroize-as-memset into a config file under tests/
Applying: Move the description of MBEDTLS_TEST_DEFINES_ZEROIZE to before its use
Applying: config-wrapper-zeroize-memset.h should be user-config-zeroize-memset.h and not include mbedtls_config.h

tom-cosgrove-arm mentioned this pull request Sep 1, 2023

Fix TLS pad buffer zeroization #8068

Merged

3 tasks

tom-cosgrove-arm added needs-preceding-pr Requires another PR to be merged first needs-backports Backports are missing or are pending review and approval. labels Sep 1, 2023

tom-cosgrove-arm mentioned this pull request Sep 1, 2023

Backport 2.28: Check mbedtls_platform_zeroize() calls #8145

Merged

3 tasks

tom-cosgrove-arm requested a review from paul-elliott-arm September 1, 2023 10:35

gilles-peskine-arm self-requested a review September 1, 2023 10:49

gilles-peskine-arm removed the needs-reviewer This PR needs someone to pick it up for review label Sep 1, 2023

gilles-peskine-arm requested changes Sep 1, 2023

View reviewed changes

gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Sep 1, 2023

daverodgman self-requested a review September 1, 2023 11:06

daverodgman reviewed Sep 1, 2023

View reviewed changes

daverodgman previously approved these changes Sep 1, 2023

View reviewed changes

gilles-peskine-arm requested changes Sep 1, 2023

View reviewed changes

include/mbedtls/platform_util.h Outdated Show resolved Hide resolved

include/mbedtls/platform_util.h Outdated Show resolved Hide resolved

tests/configs/config-wrapper-zeroize-memset.h Outdated Show resolved Hide resolved

tom-cosgrove-arm dismissed daverodgman’s stale review via bef6eef September 1, 2023 13:35

tom-cosgrove-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Sep 1, 2023

gilles-peskine-arm approved these changes Sep 1, 2023

View reviewed changes

tom-cosgrove-arm requested a review from daverodgman September 1, 2023 14:22

daverodgman approved these changes Sep 1, 2023

View reviewed changes

daverodgman added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Sep 1, 2023

gilles-peskine-arm mentioned this pull request Sep 1, 2023

Investigate clang-tidy on the CI #8148

Open

daverodgman removed the needs-preceding-pr Requires another PR to be merged first label Sep 2, 2023

tom-cosgrove-arm added 6 commits September 2, 2023 19:22

Add the ability to verify mbedtls_platform_zeroize() calls with -Wsiz…

42b02a9

…eof-pointer-memaccess Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>

Fix incorrect use of mbedtls_platform_zeroize() in tests

351a391

Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>

Add a build to all.sh to check mbedtls_platform_zeroize() calls

daddf11

Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>

Move zeroize-as-memset into a config file under tests/

7eced7d

Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>

Move the description of MBEDTLS_TEST_DEFINES_ZEROIZE to before its use

d9572c0

Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>

config-wrapper-zeroize-memset.h should be user-config-zeroize-memset.…

b2fafa5

…h and not include mbedtls_config.h Signed-off-by: Tom Cosgrove <tom.cosgrove@arm.com>

tom-cosgrove-arm force-pushed the check-mbedtls_platform_zeroize-calls branch from 42a7e99 to b2fafa5 Compare September 2, 2023 18:23

daverodgman added this pull request to the merge queue Sep 3, 2023

Merged via the queue into Mbed-TLS:development with commit 8595984 Sep 3, 2023

gilles-peskine-arm mentioned this pull request Jul 30, 2024

Get rid of MBEDTLS_TEST_DEFINES_ZEROIZE #9434

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check mbedtls_platform_zeroize() calls #8143

Check mbedtls_platform_zeroize() calls #8143

tom-cosgrove-arm commented Sep 1, 2023 •

edited

Loading

tom-cosgrove-arm commented Sep 1, 2023

gilles-peskine-arm left a comment

gilles-peskine-arm Sep 1, 2023

tom-cosgrove-arm Sep 1, 2023 •

edited

Loading

daverodgman Sep 1, 2023

gilles-peskine-arm Sep 1, 2023

daverodgman Sep 1, 2023

tom-cosgrove-arm Sep 1, 2023 •

edited

Loading

gilles-peskine-arm Sep 1, 2023

daverodgman left a comment

gilles-peskine-arm left a comment

gilles-peskine-arm left a comment

gilles-peskine-arm Sep 1, 2023

gilles-peskine-arm Sep 1, 2023

tom-cosgrove-arm commented Sep 1, 2023

tom-cosgrove-arm commented Sep 2, 2023

tom-cosgrove-arm commented Sep 2, 2023

tom-cosgrove-arm commented Sep 2, 2023

	#if defined(MBEDTLS_PLATFORM_ZEROIZE_CHECK_UNSAFE)
	#if defined(MBEDTLS_PLATFORM_ZEROIZE_CHECK_UNSAFE) && defined(MBEDTLS_TEST_HOOKS)

Check mbedtls_platform_zeroize() calls #8143

Check mbedtls_platform_zeroize() calls #8143

Conversation

tom-cosgrove-arm commented Sep 1, 2023 • edited Loading

Description

PR checklist

tom-cosgrove-arm commented Sep 1, 2023

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Sep 1, 2023

Choose a reason for hiding this comment

tom-cosgrove-arm Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

daverodgman Sep 1, 2023

Choose a reason for hiding this comment

gilles-peskine-arm Sep 1, 2023

Choose a reason for hiding this comment

daverodgman Sep 1, 2023

Choose a reason for hiding this comment

tom-cosgrove-arm Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

gilles-peskine-arm Sep 1, 2023

Choose a reason for hiding this comment

daverodgman left a comment

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Sep 1, 2023

Choose a reason for hiding this comment

gilles-peskine-arm Sep 1, 2023

Choose a reason for hiding this comment

tom-cosgrove-arm commented Sep 1, 2023

tom-cosgrove-arm commented Sep 2, 2023

tom-cosgrove-arm commented Sep 2, 2023

tom-cosgrove-arm commented Sep 2, 2023

tom-cosgrove-arm commented Sep 1, 2023 •

edited

Loading

tom-cosgrove-arm Sep 1, 2023 •

edited

Loading

tom-cosgrove-arm Sep 1, 2023 •

edited

Loading