From 140d5c77d0e16bc8e3fd3d4bd261a1c56fecb689 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Mon, 11 Sep 2023 19:05:30 +0100 Subject: [PATCH 1/7] Add single-bit difference tests Signed-off-by: Dave Rodgman --- tests/suites/test_suite_constant_time.data | 3 ++ .../suites/test_suite_constant_time.function | 41 +++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/tests/suites/test_suite_constant_time.data b/tests/suites/test_suite_constant_time.data index 1b0b964da..785bdec29 100644 --- a/tests/suites/test_suite_constant_time.data +++ b/tests/suites/test_suite_constant_time.data @@ -91,6 +91,9 @@ mbedtls_ct_memcmp:-1:17:2 mbedtls_ct_memcmp len 17 offset 3 mbedtls_ct_memcmp:-1:17:3 +mbedtls_ct_memcmp_single_bit_diff +mbedtls_ct_memcmp_single_bit_diff: + mbedtls_ct_memcpy_if len 1 offset 0 mbedtls_ct_memcpy_if:1:1:0 diff --git a/tests/suites/test_suite_constant_time.function b/tests/suites/test_suite_constant_time.function index 0e2cfdc0c..24240640a 100644 --- a/tests/suites/test_suite_constant_time.function +++ b/tests/suites/test_suite_constant_time.function @@ -172,6 +172,47 @@ exit: } /* END_CASE */ +/* BEGIN_CASE */ +void mbedtls_ct_memcmp_single_bit_diff() +{ + uint8_t *a = NULL, *b = NULL; + size_t size = 32; + TEST_CALLOC(a, size); + TEST_CALLOC(b, size); + + TEST_CF_SECRET(a, size); + TEST_CF_SECRET(b, size); + int result = mbedtls_ct_memcmp(a, b, size); + TEST_CF_PUBLIC(a, size); + TEST_CF_PUBLIC(b, size); + + TEST_EQUAL(result, 0); + + for (size_t offset = 0; offset < size; offset++) { + for (size_t bit_offset = 0; bit_offset < 8; bit_offset++) { + /* Set a single bit to be different at given offset, to test that we + detect single-bit differences */ + a[offset] = 1 << bit_offset; + + TEST_CF_SECRET(a, size); + TEST_CF_SECRET(b, size); + result = mbedtls_ct_memcmp(a, b, size); + TEST_CF_PUBLIC(a, size); + TEST_CF_PUBLIC(b, size); + + TEST_ASSERT(result != 0); + + a[offset] = 0; + } + } + + +exit: + mbedtls_free(a); + mbedtls_free(b); +} +/* END_CASE */ + /* BEGIN_CASE */ void mbedtls_ct_memcmp(int same, int size, int offset) { From 4f26770291a58b4b32ef8b80d39a3e877f940894 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Mon, 11 Sep 2023 19:05:51 +0100 Subject: [PATCH 2/7] Ensure mbedtls_ct_memcpy behaves correctly with 16-bit int Signed-off-by: Dave Rodgman --- library/constant_time.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/library/constant_time.c b/library/constant_time.c index d3c69cfa8..6fc62be8f 100644 --- a/library/constant_time.c +++ b/library/constant_time.c @@ -22,6 +22,7 @@ * might be translated to branches by some compilers on some platforms. */ +#include #include #include "common.h" @@ -120,7 +121,13 @@ int mbedtls_ct_memcmp(const void *a, diff |= x ^ y; } +#if UINT_MAX < UINT32_MAX + /* In case the only bits set are in the top 16-bits, and would be lost + * by the conversion to 16-bit int (the smallest possible size for int). */ + return (int) (diff | (diff >> 16)) +#else return (int) diff; +#endif } #if defined(MBEDTLS_PKCS1_V15) && defined(MBEDTLS_RSA_C) && !defined(MBEDTLS_RSA_ALT) From 70e022b024efc4589e62144d7346bf0936e2e33f Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Tue, 12 Sep 2023 09:29:13 +0100 Subject: [PATCH 3/7] code style Signed-off-by: Dave Rodgman --- tests/suites/test_suite_constant_time.function | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_constant_time.function b/tests/suites/test_suite_constant_time.function index 24240640a..c61fb2139 100644 --- a/tests/suites/test_suite_constant_time.function +++ b/tests/suites/test_suite_constant_time.function @@ -191,7 +191,7 @@ void mbedtls_ct_memcmp_single_bit_diff() for (size_t offset = 0; offset < size; offset++) { for (size_t bit_offset = 0; bit_offset < 8; bit_offset++) { /* Set a single bit to be different at given offset, to test that we - detect single-bit differences */ + detect single-bit differences */ a[offset] = 1 << bit_offset; TEST_CF_SECRET(a, size); From 98926d5fb173f9eca5d676f7267b3e2be486c8bb Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Tue, 12 Sep 2023 09:29:33 +0100 Subject: [PATCH 4/7] Update comment, and replace bit-twiddling with #error Signed-off-by: Dave Rodgman --- library/constant_time.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/library/constant_time.c b/library/constant_time.c index 6fc62be8f..371264347 100644 --- a/library/constant_time.c +++ b/library/constant_time.c @@ -121,10 +121,14 @@ int mbedtls_ct_memcmp(const void *a, diff |= x ^ y; } -#if UINT_MAX < UINT32_MAX - /* In case the only bits set are in the top 16-bits, and would be lost - * by the conversion to 16-bit int (the smallest possible size for int). */ - return (int) (diff | (diff >> 16)) + +#if (UINT_MAX < UINT32_MAX) + /* We don't support int smaller than 32-bits, but if someone tried to build + * with this configuration, there is a risk that, for differing data, the + * only bits set in diff are in the top 16-bits, and would be lost by a + * simple cast from uint32 to int. + * This would have significant security implications, so protect against it. */ +#error "mbedtls_ct_memcmp() requires minimum 32-bit ints" #else return (int) diff; #endif From 50b0a354942d925ea70c7a13297ddeafa9adc12a Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Tue, 12 Sep 2023 09:30:44 +0100 Subject: [PATCH 5/7] Test INT_MAX rather than UINT_MAX Signed-off-by: Dave Rodgman --- library/constant_time.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/constant_time.c b/library/constant_time.c index 371264347..d1d06e088 100644 --- a/library/constant_time.c +++ b/library/constant_time.c @@ -122,7 +122,7 @@ int mbedtls_ct_memcmp(const void *a, } -#if (UINT_MAX < UINT32_MAX) +#if (INT_MAX < INT32_MAX) /* We don't support int smaller than 32-bits, but if someone tried to build * with this configuration, there is a risk that, for differing data, the * only bits set in diff are in the top 16-bits, and would be lost by a From 49d722303686eac04ea9586da45f0db21d262ee4 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Tue, 12 Sep 2023 11:03:23 +0100 Subject: [PATCH 6/7] Fix test under memsan Signed-off-by: Dave Rodgman --- tests/suites/test_suite_constant_time.function | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/suites/test_suite_constant_time.function b/tests/suites/test_suite_constant_time.function index c61fb2139..5dbba063e 100644 --- a/tests/suites/test_suite_constant_time.function +++ b/tests/suites/test_suite_constant_time.function @@ -185,6 +185,7 @@ void mbedtls_ct_memcmp_single_bit_diff() int result = mbedtls_ct_memcmp(a, b, size); TEST_CF_PUBLIC(a, size); TEST_CF_PUBLIC(b, size); + TEST_CF_PUBLIC(&result, sizeof(result)); TEST_EQUAL(result, 0); @@ -199,6 +200,7 @@ void mbedtls_ct_memcmp_single_bit_diff() result = mbedtls_ct_memcmp(a, b, size); TEST_CF_PUBLIC(a, size); TEST_CF_PUBLIC(b, size); + TEST_CF_PUBLIC(&result, sizeof(result)); TEST_ASSERT(result != 0); From bd589442520fee75d844e8e22a1b4fe17276219b Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Tue, 12 Sep 2023 12:38:53 +0100 Subject: [PATCH 7/7] Avoid implementation defined behaviour Signed-off-by: Dave Rodgman --- library/constant_time.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/library/constant_time.c b/library/constant_time.c index d1d06e088..55e7f9435 100644 --- a/library/constant_time.c +++ b/library/constant_time.c @@ -130,7 +130,14 @@ int mbedtls_ct_memcmp(const void *a, * This would have significant security implications, so protect against it. */ #error "mbedtls_ct_memcmp() requires minimum 32-bit ints" #else - return (int) diff; + /* The bit-twiddling ensures that when we cast uint32_t to int, we are casting + * a value that is in the range 0..INT_MAX - a value larger than this would + * result in implementation defined behaviour. + * + * This ensures that the value returned by the function is non-zero iff + * diff is non-zero. + */ + return (int) ((diff & 0xffff) | (diff >> 16)); #endif }