From 39188c0a2a2a5bdf27d4e047f99fd2eeca781baf Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Fri, 23 Dec 2022 12:27:04 +0000 Subject: [PATCH 01/11] Add unit tests for mbedtls_ct_memcmp and mbedtls_ct_memcpy_if_eq Signed-off-by: Dave Rodgman --- tests/suites/test_suite_constant_time.data | 108 ++++++++++++++++++ .../suites/test_suite_constant_time.function | 77 +++++++++++++ 2 files changed, 185 insertions(+) diff --git a/tests/suites/test_suite_constant_time.data b/tests/suites/test_suite_constant_time.data index 4504aa4d67..2ea3e3e964 100644 --- a/tests/suites/test_suite_constant_time.data +++ b/tests/suites/test_suite_constant_time.data @@ -9,3 +9,111 @@ ssl_cf_memcpy_offset:0:255:32 # we could get this with 255-bytes plaintext and untruncated SHA-384 Constant-flow memcpy from offset: large ssl_cf_memcpy_offset:100:339:48 + +mbedtls_ct_memcmp NULL +mbedtls_ct_memcmp_null + +mbedtls_ct_memcmp len 1 +mbedtls_ct_memcmp:1:1:0 + +mbedtls_ct_memcmp len 3 +mbedtls_ct_memcmp:1:3:0 + +mbedtls_ct_memcmp len 4 +mbedtls_ct_memcmp:1:4:0 + +mbedtls_ct_memcmp len 5 +mbedtls_ct_memcmp:1:5:0 + +mbedtls_ct_memcmp len 15 +mbedtls_ct_memcmp:1:15:0 + +mbedtls_ct_memcmp len 16 +mbedtls_ct_memcmp:1:16:0 + +mbedtls_ct_memcmp len 17 +mbedtls_ct_memcmp:1:17:0 + +mbedtls_ct_memcmp len 1 different +mbedtls_ct_memcmp:0:1:0 + +mbedtls_ct_memcmp len 17 different +mbedtls_ct_memcmp:0:17:0 + +mbedtls_ct_memcmp len 1 offset 1 different +mbedtls_ct_memcmp:0:1:1 + +mbedtls_ct_memcmp len 17 offset 1 different +mbedtls_ct_memcmp:0:17:1 + +mbedtls_ct_memcmp len 1 offset 1 +mbedtls_ct_memcmp:1:1:1 + +mbedtls_ct_memcmp len 1 offset 2 +mbedtls_ct_memcmp:1:1:2 + +mbedtls_ct_memcmp len 1 offset 3 +mbedtls_ct_memcmp:1:1:3 + +mbedtls_ct_memcmp len 5 offset 1 +mbedtls_ct_memcmp:1:5:1 + +mbedtls_ct_memcmp len 5 offset 2 +mbedtls_ct_memcmp:1:5:2 + +mbedtls_ct_memcmp len 5 offset 3 +mbedtls_ct_memcmp:1:5:3 + +mbedtls_ct_memcmp len 17 offset 1 +mbedtls_ct_memcmp:1:17:1 + +mbedtls_ct_memcmp len 17 offset 2 +mbedtls_ct_memcmp:1:17:2 + +mbedtls_ct_memcmp len 17 offset 3 +mbedtls_ct_memcmp:1:17:3 + +mbedtls_ct_memcpy_if_eq len 1 offset 0 +mbedtls_ct_memcpy_if_eq:1:1:0 + +mbedtls_ct_memcpy_if_eq len 1 offset 1 +mbedtls_ct_memcpy_if_eq:1:1:1 + +mbedtls_ct_memcpy_if_eq len 4 offset 0 +mbedtls_ct_memcpy_if_eq:1:1:0 + +mbedtls_ct_memcpy_if_eq len 4 offset 1 +mbedtls_ct_memcpy_if_eq:1:1:1 + +mbedtls_ct_memcpy_if_eq len 4 offset 2 +mbedtls_ct_memcpy_if_eq:1:1:2 + +mbedtls_ct_memcpy_if_eq len 4 offset 3 +mbedtls_ct_memcpy_if_eq:1:1:3 + +mbedtls_ct_memcpy_if_eq len 15 offset 0 +mbedtls_ct_memcpy_if_eq:1:15:0 + +mbedtls_ct_memcpy_if_eq len 15 offset 1 +mbedtls_ct_memcpy_if_eq:1:15:1 + +mbedtls_ct_memcpy_if_eq len 16 offset 0 +mbedtls_ct_memcpy_if_eq:1:16:0 + +mbedtls_ct_memcpy_if_eq len 16 offset 1 +mbedtls_ct_memcpy_if_eq:1:16:1 + +mbedtls_ct_memcpy_if_eq len 17 offset 0 +mbedtls_ct_memcpy_if_eq:1:17:0 + +mbedtls_ct_memcpy_if_eq len 17 offset 1 +mbedtls_ct_memcpy_if_eq:1:17:1 + +mbedtls_ct_memcpy_if_eq len 0 not eq +mbedtls_ct_memcpy_if_eq:0:17:0 + +mbedtls_ct_memcpy_if_eq len 5 offset 1 not eq +mbedtls_ct_memcpy_if_eq:0:5:1 + +mbedtls_ct_memcpy_if_eq len 17 offset 3 not eq +mbedtls_ct_memcpy_if_eq:0:17:3 diff --git a/tests/suites/test_suite_constant_time.function b/tests/suites/test_suite_constant_time.function index a40149ab43..211a656bb0 100644 --- a/tests/suites/test_suite_constant_time.function +++ b/tests/suites/test_suite_constant_time.function @@ -15,6 +15,83 @@ #include /* END_HEADER */ +#include + +/* BEGIN_CASE */ +void mbedtls_ct_memcmp_null() +{ + uint32_t x; + TEST_ASSERT(mbedtls_ct_memcmp(&x, NULL, 0) == 0); + TEST_ASSERT(mbedtls_ct_memcmp(NULL, &x, 0) == 0); + TEST_ASSERT(mbedtls_ct_memcmp(NULL, NULL, 0) == 0); +} +/* END_CASE */ + +/* BEGIN_CASE */ +void mbedtls_ct_memcmp(int same, int size, int offset) +{ + uint8_t *a = NULL, *b = NULL; + ASSERT_ALLOC(a, size + offset); + ASSERT_ALLOC(b, size + offset); + + TEST_CF_SECRET(a + offset, size); + TEST_CF_SECRET(b + offset, size); + + for (int i = 0; i < size + offset; i++) { + a[i] = i & 0xff; + b[i] = (i & 0xff) + (same ? 0 : 1); + } + + int reference = memcmp(a + offset, b + offset, size); + int actual = mbedtls_ct_memcmp(a + offset, b + offset, size); + TEST_CF_PUBLIC(a + offset, size); + TEST_CF_PUBLIC(b + offset, size); + + if (same != 0) { + TEST_ASSERT(reference == 0); + TEST_ASSERT(actual == 0); + } else { + TEST_ASSERT(reference != 0); + TEST_ASSERT(actual != 0); + } +exit: + mbedtls_free(a); + mbedtls_free(b); +} +/* END_CASE */ + +/* BEGIN_CASE depends_on:MBEDTLS_SSL_SOME_SUITES_USE_MAC */ +void mbedtls_ct_memcpy_if_eq(int eq, int size, int offset) +{ + uint8_t *src = NULL, *result = NULL, *expected = NULL; + ASSERT_ALLOC(src, size + offset); + ASSERT_ALLOC(result, size + offset); + ASSERT_ALLOC(expected, size + offset); + + for (int i = 0; i < size + offset; i++) { + src[i] = 1; + result[i] = 0xff; + expected[i] = eq ? 1 : 0xff; + } + + mbedtls_ct_memcpy_if_eq(result + offset, src, size, eq, 1); + ASSERT_COMPARE(expected, size, result + offset, size); + + for (int i = 0; i < size + offset; i++) { + src[i] = 1; + result[i] = 0xff; + expected[i] = eq ? 1 : 0xff; + } + mbedtls_ct_memcpy_if_eq(result, src + offset, size, eq, 1); + ASSERT_COMPARE(expected, size, result, size); + +exit: + mbedtls_free(src); + mbedtls_free(result); + mbedtls_free(expected); +} +/* END_CASE */ + /* BEGIN_CASE depends_on:MBEDTLS_SSL_SOME_SUITES_USE_TLS_CBC:MBEDTLS_TEST_HOOKS */ void ssl_cf_memcpy_offset(int offset_min, int offset_max, int len) { From cb0f2c449111a2071a70b26d27ad58c40444832b Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Fri, 23 Dec 2022 13:15:37 +0000 Subject: [PATCH 02/11] Tidy-up - move asm #define into build_info.h Signed-off-by: Dave Rodgman --- include/mbedtls/build_info.h | 7 +++++++ include/mbedtls/mbedtls_config.h | 3 +++ library/aesni.c | 6 ------ library/bn_mul.h | 4 ---- library/padlock.c | 6 ------ library/sha256.c | 6 ------ library/sha512.c | 12 ------------ 7 files changed, 10 insertions(+), 34 deletions(-) diff --git a/include/mbedtls/build_info.h b/include/mbedtls/build_info.h index bbfd5d48df..413d5c277e 100644 --- a/include/mbedtls/build_info.h +++ b/include/mbedtls/build_info.h @@ -59,6 +59,13 @@ #define inline __inline #endif +/* Define `asm` for compilers which don't define it. */ +/* *INDENT-OFF* */ +#ifndef asm +#define asm __asm__ +#endif +/* *INDENT-ON* */ + #if !defined(MBEDTLS_CONFIG_FILE) #include "mbedtls/mbedtls_config.h" #else diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index 11c3139bd4..2a2c039d76 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -48,8 +48,11 @@ * Requires support for asm() in compiler. * * Used in: + * library/aesni.h * library/aria.c * library/bn_mul.h + * library/constant_time.c + * library/padlock.h * * Required by: * MBEDTLS_AESNI_C diff --git a/library/aesni.c b/library/aesni.c index d4abb4d6cb..f6b304d157 100644 --- a/library/aesni.c +++ b/library/aesni.c @@ -37,12 +37,6 @@ #include -/* *INDENT-OFF* */ -#ifndef asm -#define asm __asm -#endif -/* *INDENT-ON* */ - #if defined(MBEDTLS_HAVE_X86_64) /* diff --git a/library/bn_mul.h b/library/bn_mul.h index 307c2418ff..ab59fbd64f 100644 --- a/library/bn_mul.h +++ b/library/bn_mul.h @@ -83,10 +83,6 @@ /* *INDENT-OFF* */ #if defined(MBEDTLS_HAVE_ASM) -#ifndef asm -#define asm __asm -#endif - /* armcc5 --gnu defines __GNUC__ but doesn't support GNU's extended asm */ #if defined(__GNUC__) && \ ( !defined(__ARMCC_VERSION) || __ARMCC_VERSION >= 6000000 ) diff --git a/library/padlock.c b/library/padlock.c index b6c6919cd1..f42c40ff93 100644 --- a/library/padlock.c +++ b/library/padlock.c @@ -31,12 +31,6 @@ #include -/* *INDENT-OFF* */ -#ifndef asm -#define asm __asm -#endif -/* *INDENT-ON* */ - #if defined(MBEDTLS_HAVE_X86) /* diff --git a/library/sha256.c b/library/sha256.c index 16fd20d8cd..cb09a71ec1 100644 --- a/library/sha256.c +++ b/library/sha256.c @@ -89,12 +89,6 @@ static int mbedtls_a64_crypto_sha256_determine_support(void) #include #include -/* *INDENT-OFF* */ -#ifndef asm -#define asm __asm__ -#endif -/* *INDENT-ON* */ - static jmp_buf return_from_sigill; /* diff --git a/library/sha512.c b/library/sha512.c index 0ea64218b2..efcbed413f 100644 --- a/library/sha512.c +++ b/library/sha512.c @@ -104,12 +104,6 @@ static int mbedtls_a64_crypto_sha512_determine_support(void) #include #include -/* *INDENT-OFF* */ -#ifndef asm -#define asm __asm__ -#endif -/* *INDENT-ON* */ - static jmp_buf return_from_sigill; /* @@ -300,12 +294,6 @@ static const uint64_t K[80] = # define mbedtls_internal_sha512_process_a64_crypto mbedtls_internal_sha512_process #endif -/* *INDENT-OFF* */ -#ifndef asm -#define asm __asm__ -#endif -/* *INDENT-ON* */ - /* Accelerated SHA-512 implementation originally written by Simon Tatham for PuTTY, * under the MIT licence; dual-licensed as Apache 2 with his kind permission. */ From 36dfc5a237a555f805de3f414a561cc61aa8e3a3 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Thu, 22 Dec 2022 15:04:43 +0000 Subject: [PATCH 03/11] Improve efficiency of some constant time functions Signed-off-by: Dave Rodgman --- library/constant_time.c | 70 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 65 insertions(+), 5 deletions(-) diff --git a/library/constant_time.c b/library/constant_time.c index 442eb0e7a2..c5b6690ddc 100644 --- a/library/constant_time.c +++ b/library/constant_time.c @@ -47,16 +47,64 @@ #include +/* + * Define MBEDTLS_EFFICIENT_UNALIGNED_ACCESS for architectures where unaligned memory + * accesses are known to be safe and efficient. + * + * This is needed because mbedtls_get_unaligned_uintXX etc don't support volatile + * memory accesses. + * + * This macro could be moved into alignment.h but for now it's only used here. + */ +#if defined(__ARM_FEATURE_UNALIGNED) +/* __ARM_FEATURE_UNALIGNED is defined by armcc, gcc 7, clang 9 and later versions. */ +#define MBEDTLS_EFFICIENT_UNALIGNED_ACCESS +#endif + +#if defined(MBEDTLS_EFFICIENT_UNALIGNED_ACCESS) && defined(MBEDTLS_HAVE_ASM) +static inline uint32_t mbedtls_get_unaligned_volatile_uint32(volatile const unsigned char *p) +{ + /* This is UB, even where it's safe: + * return *((volatile uint32_t*)p); + * so instead the same thing is expressed in assembly below. + */ + uint32_t r; +#if defined(__arm__) || defined(__thumb__) || defined(__thumb2__) + asm ("ldr %0, [%1]" : "=r" (r) : "r" (p) :); + return r; +#endif +#if defined(__aarch64__) + asm ("ldr %w0, [%1]" : "=r" (r) : "r" (p) :); + return r; +#endif + + /* Always safe, but inefficient, fall-back */ + if (MBEDTLS_IS_BIG_ENDIAN) { + return (p[0] << 24) | (p[1] << 16) | (p[2] << 8) | p[3]; + } else { + return p[0] | (p[1] << 8) | (p[2] << 16) | (p[3] << 24); + } +} +#endif /* MBEDTLS_EFFICIENT_UNALIGNED_ACCESS */ + int mbedtls_ct_memcmp(const void *a, const void *b, size_t n) { - size_t i; + size_t i = 0; volatile const unsigned char *A = (volatile const unsigned char *) a; volatile const unsigned char *B = (volatile const unsigned char *) b; - volatile unsigned char diff = 0; + volatile uint32_t diff = 0; - for (i = 0; i < n; i++) { +#if defined(MBEDTLS_EFFICIENT_UNALIGNED_ACCESS) + for (; (i + 4) <= n; i += 4) { + uint32_t x = mbedtls_get_unaligned_volatile_uint32(A + i); + uint32_t y = mbedtls_get_unaligned_volatile_uint32(B + i); + diff |= x ^ y; + } +#endif + + for (; i < n; i++) { /* Read volatile data in order before computing diff. * This avoids IAR compiler warning: * 'the order of volatile accesses is undefined ..' */ @@ -414,10 +462,22 @@ void mbedtls_ct_memcpy_if_eq(unsigned char *dest, { /* mask = c1 == c2 ? 0xff : 0x00 */ const size_t equal = mbedtls_ct_size_bool_eq(c1, c2); - const unsigned char mask = (unsigned char) mbedtls_ct_size_mask(equal); /* dest[i] = c1 == c2 ? src[i] : dest[i] */ - for (size_t i = 0; i < len; i++) { + size_t i = 0; +#if defined(MBEDTLS_EFFICIENT_UNALIGNED_ACCESS) + const uint32_t mask32 = (uint32_t) mbedtls_ct_size_mask(equal); + const unsigned char mask = (unsigned char) mask32 & 0xff; + + for (; (i + 4) <= len; i += 4) { + uint32_t a = mbedtls_get_unaligned_uint32(src + i) & mask32; + uint32_t b = mbedtls_get_unaligned_uint32(dest + i) & ~mask32; + mbedtls_put_unaligned_uint32(dest + i, a | b); + } +#else + const unsigned char mask = (unsigned char) mbedtls_ct_size_mask(equal); +#endif /* MBEDTLS_EFFICIENT_UNALIGNED_ACCESS */ + for (; i < len; i++) { dest[i] = (src[i] & mask) | (dest[i] & ~mask); } } From 051225d07abf20df3cb2d03ab4b1ee8c7024089f Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Fri, 30 Dec 2022 21:25:35 +0000 Subject: [PATCH 04/11] Address potential perf regression Ensure platforms that don't have an assembly implementation for mbedtls_get_unaligned_volatile_uint32() don't experience a performance regression. Signed-off-by: Dave Rodgman --- library/constant_time.c | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/library/constant_time.c b/library/constant_time.c index c5b6690ddc..309e11cd2b 100644 --- a/library/constant_time.c +++ b/library/constant_time.c @@ -50,18 +50,29 @@ /* * Define MBEDTLS_EFFICIENT_UNALIGNED_ACCESS for architectures where unaligned memory * accesses are known to be safe and efficient. + */ +#if defined(__ARM_FEATURE_UNALIGNED) +/* __ARM_FEATURE_UNALIGNED is defined by armcc, gcc 7, clang 9 and later versions */ +#define MBEDTLS_EFFICIENT_UNALIGNED_ACCESS +#endif + +/* + * Define MBEDTLS_EFFICIENT_UNALIGNED_VOLATILE_ACCESS where assembly is present to + * perform fast unaligned access to volatile data. * * This is needed because mbedtls_get_unaligned_uintXX etc don't support volatile * memory accesses. * - * This macro could be moved into alignment.h but for now it's only used here. + * Some of these definitions could be moved into alignment.h but for now they are + * only used here. */ -#if defined(__ARM_FEATURE_UNALIGNED) -/* __ARM_FEATURE_UNALIGNED is defined by armcc, gcc 7, clang 9 and later versions. */ -#define MBEDTLS_EFFICIENT_UNALIGNED_ACCESS +#if defined(MBEDTLS_EFFICIENT_UNALIGNED_ACCESS) && defined(MBEDTLS_HAVE_ASM) +#if defined(__arm__) || defined(__thumb__) || defined(__thumb2__) || defined(__aarch64__) +#define MBEDTLS_EFFICIENT_UNALIGNED_VOLATILE_ACCESS +#endif #endif -#if defined(MBEDTLS_EFFICIENT_UNALIGNED_ACCESS) && defined(MBEDTLS_HAVE_ASM) +#if defined(MBEDTLS_EFFICIENT_UNALIGNED_VOLATILE_ACCESS) static inline uint32_t mbedtls_get_unaligned_volatile_uint32(volatile const unsigned char *p) { /* This is UB, even where it's safe: @@ -71,21 +82,12 @@ static inline uint32_t mbedtls_get_unaligned_volatile_uint32(volatile const unsi uint32_t r; #if defined(__arm__) || defined(__thumb__) || defined(__thumb2__) asm ("ldr %0, [%1]" : "=r" (r) : "r" (p) :); - return r; -#endif -#if defined(__aarch64__) +#elif defined(__aarch64__) asm ("ldr %w0, [%1]" : "=r" (r) : "r" (p) :); - return r; #endif - - /* Always safe, but inefficient, fall-back */ - if (MBEDTLS_IS_BIG_ENDIAN) { - return (p[0] << 24) | (p[1] << 16) | (p[2] << 8) | p[3]; - } else { - return p[0] | (p[1] << 8) | (p[2] << 16) | (p[3] << 24); - } + return r; } -#endif /* MBEDTLS_EFFICIENT_UNALIGNED_ACCESS */ +#endif /* MBEDTLS_EFFICIENT_UNALIGNED_VOLATILE_ACCESS */ int mbedtls_ct_memcmp(const void *a, const void *b, @@ -96,7 +98,7 @@ int mbedtls_ct_memcmp(const void *a, volatile const unsigned char *B = (volatile const unsigned char *) b; volatile uint32_t diff = 0; -#if defined(MBEDTLS_EFFICIENT_UNALIGNED_ACCESS) +#if defined(MBEDTLS_EFFICIENT_UNALIGNED_VOLATILE_ACCESS) for (; (i + 4) <= n; i += 4) { uint32_t x = mbedtls_get_unaligned_volatile_uint32(A + i); uint32_t y = mbedtls_get_unaligned_volatile_uint32(B + i); From b9cd19bc8c6196511eb1d10094c62e58e5da6eed Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Fri, 30 Dec 2022 21:32:03 +0000 Subject: [PATCH 05/11] Prevent perf regressions in mbedtls_xor Signed-off-by: Dave Rodgman --- library/alignment.h | 13 +++++++++++++ library/common.h | 6 ++++-- library/constant_time.c | 9 --------- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/library/alignment.h b/library/alignment.h index bfc965eae1..aa4c430b96 100644 --- a/library/alignment.h +++ b/library/alignment.h @@ -29,6 +29,19 @@ #include "mbedtls/build_info.h" +/* + * Define MBEDTLS_EFFICIENT_UNALIGNED_ACCESS for architectures where unaligned memory + * accesses are known to be safe and efficient. + */ +#if defined(__ARM_FEATURE_UNALIGNED) \ + || defined(__i386__) || defined(__amd64__) || defined(__x86_64__) +/* + * __ARM_FEATURE_UNALIGNED is defined where appropriate by armcc, gcc 7, clang 9 + * (and later versions); all x86 platforms should have efficient unaligned access. + */ +#define MBEDTLS_EFFICIENT_UNALIGNED_ACCESS +#endif + /** * Read the unsigned 16 bits integer from the given address, which need not * be aligned. diff --git a/library/common.h b/library/common.h index fd3ddba48c..ae6625b9e5 100644 --- a/library/common.h +++ b/library/common.h @@ -122,11 +122,13 @@ static inline const unsigned char *mbedtls_buffer_offset_const( */ inline void mbedtls_xor(unsigned char *r, const unsigned char *a, const unsigned char *b, size_t n) { - size_t i; - for (i = 0; (i + 4) <= n; i += 4) { + size_t i = 0; +#if defined(MBEDTLS_EFFICIENT_UNALIGNED_ACCESS) + for (; (i + 4) <= n; i += 4) { uint32_t x = mbedtls_get_unaligned_uint32(a + i) ^ mbedtls_get_unaligned_uint32(b + i); mbedtls_put_unaligned_uint32(r + i, x); } +#endif for (; i < n; i++) { r[i] = a[i] ^ b[i]; } diff --git a/library/constant_time.c b/library/constant_time.c index 309e11cd2b..89778d53ca 100644 --- a/library/constant_time.c +++ b/library/constant_time.c @@ -47,15 +47,6 @@ #include -/* - * Define MBEDTLS_EFFICIENT_UNALIGNED_ACCESS for architectures where unaligned memory - * accesses are known to be safe and efficient. - */ -#if defined(__ARM_FEATURE_UNALIGNED) -/* __ARM_FEATURE_UNALIGNED is defined by armcc, gcc 7, clang 9 and later versions */ -#define MBEDTLS_EFFICIENT_UNALIGNED_ACCESS -#endif - /* * Define MBEDTLS_EFFICIENT_UNALIGNED_VOLATILE_ACCESS where assembly is present to * perform fast unaligned access to volatile data. From 7f376fa6fc2f1536aa17440ea2e4bb80ee454f26 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Thu, 5 Jan 2023 12:25:15 +0000 Subject: [PATCH 06/11] Improve documentation Signed-off-by: Dave Rodgman --- library/alignment.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/library/alignment.h b/library/alignment.h index aa4c430b96..aa09ff8569 100644 --- a/library/alignment.h +++ b/library/alignment.h @@ -31,13 +31,17 @@ /* * Define MBEDTLS_EFFICIENT_UNALIGNED_ACCESS for architectures where unaligned memory - * accesses are known to be safe and efficient. + * accesses are known to be efficient. + * + * All functions defined here will behave correctly regardless, but might be less + * efficient when this is not defined. */ #if defined(__ARM_FEATURE_UNALIGNED) \ || defined(__i386__) || defined(__amd64__) || defined(__x86_64__) /* * __ARM_FEATURE_UNALIGNED is defined where appropriate by armcc, gcc 7, clang 9 - * (and later versions); all x86 platforms should have efficient unaligned access. + * (and later versions) for Arm v7 and later; all x86 platforms should have + * efficient unaligned access. */ #define MBEDTLS_EFFICIENT_UNALIGNED_ACCESS #endif From 95ec58cc12326b5ed8270db505d8a1e366b619a7 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Thu, 5 Jan 2023 12:26:48 +0000 Subject: [PATCH 07/11] Remove not-needed stdio include from tests Signed-off-by: Dave Rodgman --- tests/suites/test_suite_alignment.function | 1 - tests/suites/test_suite_constant_time.function | 2 -- 2 files changed, 3 deletions(-) diff --git a/tests/suites/test_suite_alignment.function b/tests/suites/test_suite_alignment.function index 6c98f233af..f6703318ce 100644 --- a/tests/suites/test_suite_alignment.function +++ b/tests/suites/test_suite_alignment.function @@ -6,7 +6,6 @@ #if defined(__clang__) #pragma clang diagnostic ignored "-Wunreachable-code" #endif -#include /* * Convert a string of the form "abcd" (case-insensitive) to a uint64_t. diff --git a/tests/suites/test_suite_constant_time.function b/tests/suites/test_suite_constant_time.function index 211a656bb0..aa605d2fe7 100644 --- a/tests/suites/test_suite_constant_time.function +++ b/tests/suites/test_suite_constant_time.function @@ -15,8 +15,6 @@ #include /* END_HEADER */ -#include - /* BEGIN_CASE */ void mbedtls_ct_memcmp_null() { From fa96026a0e760fab0149b2adb08a34d3b7d51438 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Tue, 10 Jan 2023 11:14:02 +0000 Subject: [PATCH 08/11] Move definition of asm out of public header Signed-off-by: Dave Rodgman --- include/mbedtls/build_info.h | 7 ------- library/common.h | 7 +++++++ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/include/mbedtls/build_info.h b/include/mbedtls/build_info.h index 413d5c277e..bbfd5d48df 100644 --- a/include/mbedtls/build_info.h +++ b/include/mbedtls/build_info.h @@ -59,13 +59,6 @@ #define inline __inline #endif -/* Define `asm` for compilers which don't define it. */ -/* *INDENT-OFF* */ -#ifndef asm -#define asm __asm__ -#endif -/* *INDENT-ON* */ - #if !defined(MBEDTLS_CONFIG_FILE) #include "mbedtls/mbedtls_config.h" #else diff --git a/library/common.h b/library/common.h index ae6625b9e5..46af79f0da 100644 --- a/library/common.h +++ b/library/common.h @@ -142,4 +142,11 @@ inline void mbedtls_xor(unsigned char *r, const unsigned char *a, const unsigned #define /*no-check-names*/ __func__ __FUNCTION__ #endif +/* Define `asm` for compilers which don't define it. */ +/* *INDENT-OFF* */ +#ifndef asm +#define asm __asm__ +#endif +/* *INDENT-ON* */ + #endif /* MBEDTLS_LIBRARY_COMMON_H */ From 7658b633901112c5e2d9a1ae7112d69779d6fef1 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Wed, 11 Jan 2023 17:39:33 +0000 Subject: [PATCH 09/11] Remove volatile from diff; add explanatory comment Signed-off-by: Dave Rodgman --- library/constant_time.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/library/constant_time.c b/library/constant_time.c index 89778d53ca..7f4d509bc3 100644 --- a/library/constant_time.c +++ b/library/constant_time.c @@ -85,9 +85,15 @@ int mbedtls_ct_memcmp(const void *a, size_t n) { size_t i = 0; + /* + * `A` and `B` are cast to volatile to ensure that the compiler + * generates code that always fully reads both buffers. + * Otherwise it could generate a test to exit early if `diff` has all + * bits set early in the loop. + */ volatile const unsigned char *A = (volatile const unsigned char *) a; volatile const unsigned char *B = (volatile const unsigned char *) b; - volatile uint32_t diff = 0; + uint32_t diff = 0; #if defined(MBEDTLS_EFFICIENT_UNALIGNED_VOLATILE_ACCESS) for (; (i + 4) <= n; i += 4) { From 22b0d1adbfb0270ac4c244449f40252dda2112d3 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Sat, 21 Jan 2023 10:29:00 +0000 Subject: [PATCH 10/11] Test memcmp with differences starting after the first byte Signed-off-by: Dave Rodgman --- tests/suites/test_suite_constant_time.data | 74 ++++++++++++------- .../suites/test_suite_constant_time.function | 12 ++- 2 files changed, 56 insertions(+), 30 deletions(-) diff --git a/tests/suites/test_suite_constant_time.data b/tests/suites/test_suite_constant_time.data index 2ea3e3e964..91a25faccb 100644 --- a/tests/suites/test_suite_constant_time.data +++ b/tests/suites/test_suite_constant_time.data @@ -14,25 +14,25 @@ mbedtls_ct_memcmp NULL mbedtls_ct_memcmp_null mbedtls_ct_memcmp len 1 -mbedtls_ct_memcmp:1:1:0 +mbedtls_ct_memcmp:-1:1:0 mbedtls_ct_memcmp len 3 -mbedtls_ct_memcmp:1:3:0 +mbedtls_ct_memcmp:-1:3:0 mbedtls_ct_memcmp len 4 -mbedtls_ct_memcmp:1:4:0 +mbedtls_ct_memcmp:-1:4:0 mbedtls_ct_memcmp len 5 -mbedtls_ct_memcmp:1:5:0 +mbedtls_ct_memcmp:-1:5:0 mbedtls_ct_memcmp len 15 -mbedtls_ct_memcmp:1:15:0 +mbedtls_ct_memcmp:-1:15:0 mbedtls_ct_memcmp len 16 -mbedtls_ct_memcmp:1:16:0 +mbedtls_ct_memcmp:-1:16:0 mbedtls_ct_memcmp len 17 -mbedtls_ct_memcmp:1:17:0 +mbedtls_ct_memcmp:-1:17:0 mbedtls_ct_memcmp len 1 different mbedtls_ct_memcmp:0:1:0 @@ -40,38 +40,56 @@ mbedtls_ct_memcmp:0:1:0 mbedtls_ct_memcmp len 17 different mbedtls_ct_memcmp:0:17:0 +mbedtls_ct_memcmp len 17 different 1 +mbedtls_ct_memcmp:1:17:0 + +mbedtls_ct_memcmp len 17 different 4 +mbedtls_ct_memcmp:4:17:0 + +mbedtls_ct_memcmp len 17 different 10 +mbedtls_ct_memcmp:10:17:0 + +mbedtls_ct_memcmp len 17 different 16 +mbedtls_ct_memcmp:16:17:0 + mbedtls_ct_memcmp len 1 offset 1 different mbedtls_ct_memcmp:0:1:1 mbedtls_ct_memcmp len 17 offset 1 different mbedtls_ct_memcmp:0:17:1 -mbedtls_ct_memcmp len 1 offset 1 -mbedtls_ct_memcmp:1:1:1 - -mbedtls_ct_memcmp len 1 offset 2 -mbedtls_ct_memcmp:1:1:2 - -mbedtls_ct_memcmp len 1 offset 3 -mbedtls_ct_memcmp:1:1:3 - -mbedtls_ct_memcmp len 5 offset 1 -mbedtls_ct_memcmp:1:5:1 - -mbedtls_ct_memcmp len 5 offset 2 -mbedtls_ct_memcmp:1:5:2 - -mbedtls_ct_memcmp len 5 offset 3 -mbedtls_ct_memcmp:1:5:3 - -mbedtls_ct_memcmp len 17 offset 1 +mbedtls_ct_memcmp len 17 offset 1 different 1 mbedtls_ct_memcmp:1:17:1 +mbedtls_ct_memcmp len 17 offset 1 different 5 +mbedtls_ct_memcmp:5:17:1 + +mbedtls_ct_memcmp len 1 offset 1 +mbedtls_ct_memcmp:-1:1:1 + +mbedtls_ct_memcmp len 1 offset 2 +mbedtls_ct_memcmp:-1:1:2 + +mbedtls_ct_memcmp len 1 offset 3 +mbedtls_ct_memcmp:-1:1:3 + +mbedtls_ct_memcmp len 5 offset 1 +mbedtls_ct_memcmp:-1:5:1 + +mbedtls_ct_memcmp len 5 offset 2 +mbedtls_ct_memcmp:-1:5:2 + +mbedtls_ct_memcmp len 5 offset 3 +mbedtls_ct_memcmp:-1:5:3 + +mbedtls_ct_memcmp len 17 offset 1 +mbedtls_ct_memcmp:-1:17:1 + mbedtls_ct_memcmp len 17 offset 2 -mbedtls_ct_memcmp:1:17:2 +mbedtls_ct_memcmp:-1:17:2 mbedtls_ct_memcmp len 17 offset 3 -mbedtls_ct_memcmp:1:17:3 +mbedtls_ct_memcmp:-1:17:3 mbedtls_ct_memcpy_if_eq len 1 offset 0 mbedtls_ct_memcpy_if_eq:1:1:0 diff --git a/tests/suites/test_suite_constant_time.function b/tests/suites/test_suite_constant_time.function index aa605d2fe7..167962fb4a 100644 --- a/tests/suites/test_suite_constant_time.function +++ b/tests/suites/test_suite_constant_time.function @@ -35,9 +35,17 @@ void mbedtls_ct_memcmp(int same, int size, int offset) TEST_CF_SECRET(a + offset, size); TEST_CF_SECRET(b + offset, size); + /* Construct data that matches, if same == -1, otherwise + * same gives the number of bytes (after the initial offset) + * that will match; after that it will differ. + */ for (int i = 0; i < size + offset; i++) { a[i] = i & 0xff; - b[i] = (i & 0xff) + (same ? 0 : 1); + if (same == -1 || (i - offset) < same) { + b[i] = a[i]; + } else { + b[i] = (i + 1) & 0xff; + } } int reference = memcmp(a + offset, b + offset, size); @@ -45,7 +53,7 @@ void mbedtls_ct_memcmp(int same, int size, int offset) TEST_CF_PUBLIC(a + offset, size); TEST_CF_PUBLIC(b + offset, size); - if (same != 0) { + if (same == -1 || same >= size) { TEST_ASSERT(reference == 0); TEST_ASSERT(actual == 0); } else { From 58c721e89461a2f2d5c2527b4bafcfde02604189 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Sat, 21 Jan 2023 11:00:30 +0000 Subject: [PATCH 11/11] Add TEST_CF_SECRET to mbedtls_ct_memcpy_if_eq test Signed-off-by: Dave Rodgman --- .../suites/test_suite_constant_time.function | 25 ++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/tests/suites/test_suite_constant_time.function b/tests/suites/test_suite_constant_time.function index 167962fb4a..14dc8ae5cd 100644 --- a/tests/suites/test_suite_constant_time.function +++ b/tests/suites/test_suite_constant_time.function @@ -80,7 +80,17 @@ void mbedtls_ct_memcpy_if_eq(int eq, int size, int offset) expected[i] = eq ? 1 : 0xff; } - mbedtls_ct_memcpy_if_eq(result + offset, src, size, eq, 1); + int one, secret_eq; + TEST_CF_SECRET(&one, sizeof(one)); + TEST_CF_SECRET(&secret_eq, sizeof(secret_eq)); + one = 1; + secret_eq = eq; + + mbedtls_ct_memcpy_if_eq(result + offset, src, size, secret_eq, one); + + TEST_CF_PUBLIC(&one, sizeof(one)); + TEST_CF_PUBLIC(&secret_eq, sizeof(secret_eq)); + ASSERT_COMPARE(expected, size, result + offset, size); for (int i = 0; i < size + offset; i++) { @@ -88,9 +98,18 @@ void mbedtls_ct_memcpy_if_eq(int eq, int size, int offset) result[i] = 0xff; expected[i] = eq ? 1 : 0xff; } - mbedtls_ct_memcpy_if_eq(result, src + offset, size, eq, 1); - ASSERT_COMPARE(expected, size, result, size); + TEST_CF_SECRET(&one, sizeof(one)); + TEST_CF_SECRET(&secret_eq, sizeof(secret_eq)); + one = 1; + secret_eq = eq; + + mbedtls_ct_memcpy_if_eq(result, src + offset, size, secret_eq, one); + + TEST_CF_PUBLIC(&one, sizeof(one)); + TEST_CF_PUBLIC(&secret_eq, sizeof(secret_eq)); + + ASSERT_COMPARE(expected, size, result, size); exit: mbedtls_free(src); mbedtls_free(result);