Merge pull request #1073 from Mbed-TLS/better-ct-memcmp
More consistent use of mbedtls_ct_memcmp
diff --git a/library/ccm.c b/library/ccm.c
index cd689c8..7b29775 100644
--- a/library/ccm.c
+++ b/library/ccm.c
@@ -33,6 +33,7 @@
#include "mbedtls/ccm.h"
#include "mbedtls/platform_util.h"
#include "mbedtls/error.h"
+#include "mbedtls/constant_time.h"
#include <string.h>
@@ -533,13 +534,8 @@
const unsigned char *tag2,
size_t tag_len)
{
- unsigned char i;
- int diff;
-
/* Check tag in "constant-time" */
- for (diff = 0, i = 0; i < tag_len; i++) {
- diff |= tag1[i] ^ tag2[i];
- }
+ int diff = mbedtls_ct_memcmp(tag1, tag2, tag_len);
if (diff != 0) {
return MBEDTLS_ERR_CCM_AUTH_FAILED;
diff --git a/library/chachapoly.c b/library/chachapoly.c
index 0124d75..aebc646 100644
--- a/library/chachapoly.c
+++ b/library/chachapoly.c
@@ -25,6 +25,7 @@
#include "mbedtls/chachapoly.h"
#include "mbedtls/platform_util.h"
#include "mbedtls/error.h"
+#include "mbedtls/constant_time.h"
#include <string.h>
@@ -310,7 +311,6 @@
{
int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
unsigned char check_tag[16];
- size_t i;
int diff;
if ((ret = chachapoly_crypt_and_tag(ctx,
@@ -320,9 +320,7 @@
}
/* Check tag in "constant-time" */
- for (diff = 0, i = 0; i < sizeof(check_tag); i++) {
- diff |= tag[i] ^ check_tag[i];
- }
+ diff = mbedtls_ct_memcmp(tag, check_tag, sizeof(check_tag));
if (diff != 0) {
mbedtls_platform_zeroize(output, length);
diff --git a/library/constant_time.c b/library/constant_time.c
index 55e7f94..8b41aed 100644
--- a/library/constant_time.c
+++ b/library/constant_time.c
@@ -141,6 +141,36 @@
#endif
}
+#if defined(MBEDTLS_NIST_KW_C)
+
+int mbedtls_ct_memcmp_partial(const void *a,
+ const void *b,
+ size_t n,
+ size_t skip_head,
+ size_t skip_tail)
+{
+ unsigned int diff = 0;
+
+ volatile const unsigned char *A = (volatile const unsigned char *) a;
+ volatile const unsigned char *B = (volatile const unsigned char *) b;
+
+ size_t valid_end = n - skip_tail;
+
+ for (size_t i = 0; i < n; i++) {
+ unsigned char x = A[i], y = B[i];
+ unsigned int d = x ^ y;
+ mbedtls_ct_condition_t valid = mbedtls_ct_bool_and(mbedtls_ct_uint_ge(i, skip_head),
+ mbedtls_ct_uint_lt(i, valid_end));
+ diff |= mbedtls_ct_uint_if_else_0(valid, d);
+ }
+
+ /* Since we go byte-by-byte, the only bits set will be in the bottom 8 bits, so the
+ * cast from uint to int is safe. */
+ return (int) diff;
+}
+
+#endif
+
#if defined(MBEDTLS_PKCS1_V15) && defined(MBEDTLS_RSA_C) && !defined(MBEDTLS_RSA_ALT)
void mbedtls_ct_memmove_left(void *start, size_t total, size_t offset)
diff --git a/library/constant_time_internal.h b/library/constant_time_internal.h
index 44b74ae..323edef 100644
--- a/library/constant_time_internal.h
+++ b/library/constant_time_internal.h
@@ -492,6 +492,37 @@
size_t n);
*/
+#if defined(MBEDTLS_NIST_KW_C)
+
+/** Constant-time buffer comparison without branches.
+ *
+ * Similar to mbedtls_ct_memcmp, except that the result only depends on part of
+ * the input data - differences in the head or tail are ignored. Functionally equivalent to:
+ *
+ * memcmp(a + skip_head, b + skip_head, size - skip_head - skip_tail)
+ *
+ * Time taken depends on \p n, but not on \p skip_head or \p skip_tail .
+ *
+ * Behaviour is undefined if ( \p skip_head + \p skip_tail) > \p n.
+ *
+ * \param a Secret. Pointer to the first buffer, containing at least \p n bytes. May not be NULL.
+ * \param b Secret. Pointer to the second buffer, containing at least \p n bytes. May not be NULL.
+ * \param n The number of bytes to examine (total size of the buffers).
+ * \param skip_head Secret. The number of bytes to treat as non-significant at the start of the buffer.
+ * These bytes will still be read.
+ * \param skip_tail Secret. The number of bytes to treat as non-significant at the end of the buffer.
+ * These bytes will still be read.
+ *
+ * \return Zero if the contents of the two buffers are the same, otherwise non-zero.
+ */
+int mbedtls_ct_memcmp_partial(const void *a,
+ const void *b,
+ size_t n,
+ size_t skip_head,
+ size_t skip_tail);
+
+#endif
+
/* Include the implementation of static inline functions above. */
#include "constant_time_impl.h"
diff --git a/library/gcm.c b/library/gcm.c
index 786290f..b773529 100644
--- a/library/gcm.c
+++ b/library/gcm.c
@@ -35,6 +35,7 @@
#include "mbedtls/platform.h"
#include "mbedtls/platform_util.h"
#include "mbedtls/error.h"
+#include "mbedtls/constant_time.h"
#include <string.h>
@@ -601,7 +602,6 @@
{
int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
unsigned char check_tag[16];
- size_t i;
int diff;
if ((ret = mbedtls_gcm_crypt_and_tag(ctx, MBEDTLS_GCM_DECRYPT, length,
@@ -611,9 +611,7 @@
}
/* Check tag in "constant-time" */
- for (diff = 0, i = 0; i < tag_len; i++) {
- diff |= tag[i] ^ check_tag[i];
- }
+ diff = mbedtls_ct_memcmp(tag, check_tag, tag_len);
if (diff != 0) {
mbedtls_platform_zeroize(output, length);
diff --git a/library/nist_kw.c b/library/nist_kw.c
index fbd7221..d73e82f 100644
--- a/library/nist_kw.c
+++ b/library/nist_kw.c
@@ -35,6 +35,7 @@
#include "mbedtls/platform_util.h"
#include "mbedtls/error.h"
#include "mbedtls/constant_time.h"
+#include "constant_time_internal.h"
#include <stdint.h>
#include <string.h>
@@ -333,9 +334,9 @@
unsigned char *output, size_t *out_len, size_t out_size)
{
int ret = 0;
- size_t i, olen;
+ size_t olen;
unsigned char A[KW_SEMIBLOCK_LENGTH];
- unsigned char diff, bad_padding = 0;
+ int diff;
*out_len = 0;
if (out_size < in_len - KW_SEMIBLOCK_LENGTH) {
@@ -420,19 +421,15 @@
* larger than 8, because of the type wrap around.
*/
padlen = in_len - KW_SEMIBLOCK_LENGTH - Plen;
- if (padlen > 7) {
- padlen &= 7;
- ret = MBEDTLS_ERR_CIPHER_AUTH_FAILED;
- }
+ ret = -((int) mbedtls_ct_uint_if(mbedtls_ct_uint_gt(padlen, 7),
+ -MBEDTLS_ERR_CIPHER_AUTH_FAILED, -ret));
+ padlen &= 7;
/* Check padding in "constant-time" */
- for (diff = 0, i = 0; i < KW_SEMIBLOCK_LENGTH; i++) {
- if (i >= KW_SEMIBLOCK_LENGTH - padlen) {
- diff |= output[*out_len - KW_SEMIBLOCK_LENGTH + i];
- } else {
- bad_padding |= output[*out_len - KW_SEMIBLOCK_LENGTH + i];
- }
- }
+ const uint8_t zero[KW_SEMIBLOCK_LENGTH] = { 0 };
+ diff = mbedtls_ct_memcmp_partial(
+ &output[*out_len - KW_SEMIBLOCK_LENGTH], zero,
+ KW_SEMIBLOCK_LENGTH, KW_SEMIBLOCK_LENGTH - padlen, 0);
if (diff != 0) {
ret = MBEDTLS_ERR_CIPHER_AUTH_FAILED;
@@ -454,7 +451,6 @@
*out_len = 0;
}
- mbedtls_platform_zeroize(&bad_padding, sizeof(bad_padding));
mbedtls_platform_zeroize(&diff, sizeof(diff));
mbedtls_platform_zeroize(A, sizeof(A));
diff --git a/library/rsa.c b/library/rsa.c
index d0782f5..02626b3 100644
--- a/library/rsa.c
+++ b/library/rsa.c
@@ -1541,7 +1541,8 @@
{
int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
size_t ilen, i, pad_len;
- unsigned char *p, bad, pad_done;
+ unsigned char *p, pad_done;
+ int bad;
unsigned char buf[MBEDTLS_MPI_MAX_SIZE];
unsigned char lhash[MBEDTLS_MD_MAX_SIZE];
unsigned int hlen;
@@ -1608,9 +1609,8 @@
p += hlen; /* Skip seed */
/* Check lHash */
- for (i = 0; i < hlen; i++) {
- bad |= lhash[i] ^ *p++;
- }
+ bad |= mbedtls_ct_memcmp(lhash, p, hlen);
+ p += hlen;
/* Get zero-padding len, but always read till end of buffer
* (minus one, for the 01 byte) */
diff --git a/tests/include/test/macros.h b/tests/include/test/macros.h
index 7edc991..3bfbe33 100644
--- a/tests/include/test/macros.h
+++ b/tests/include/test/macros.h
@@ -143,6 +143,38 @@
} \
} while (0)
+/** Allocate memory dynamically and fail the test case if this fails.
+ * The allocated memory will be filled with zeros.
+ *
+ * You must set \p pointer to \c NULL before calling this macro and
+ * put `mbedtls_free(pointer)` in the test's cleanup code.
+ *
+ * If \p item_count is zero, the resulting \p pointer will not be \c NULL.
+ *
+ * This macro expands to an instruction, not an expression.
+ * It may jump to the \c exit label.
+ *
+ * \param pointer An lvalue where the address of the allocated buffer
+ * will be stored.
+ * This expression may be evaluated multiple times.
+ * \param item_count Number of elements to allocate.
+ * This expression may be evaluated multiple times.
+ *
+ * Note: if passing size 0, mbedtls_calloc may return NULL. In this case,
+ * we reattempt to allocate with the smallest possible buffer to assure a
+ * non-NULL pointer.
+ */
+#define TEST_CALLOC_NONNULL(pointer, item_count) \
+ do { \
+ TEST_ASSERT((pointer) == NULL); \
+ (pointer) = mbedtls_calloc(sizeof(*(pointer)), \
+ (item_count)); \
+ if (((pointer) == NULL) && ((item_count) == 0)) { \
+ (pointer) = mbedtls_calloc(1, 1); \
+ } \
+ TEST_ASSERT((pointer) != NULL); \
+ } while (0)
+
/* For backwards compatibility */
#define ASSERT_ALLOC(pointer, item_count) TEST_CALLOC(pointer, item_count)
diff --git a/tests/suites/test_suite_constant_time.data b/tests/suites/test_suite_constant_time.data
index 785bdec..82ee869 100644
--- a/tests/suites/test_suite_constant_time.data
+++ b/tests/suites/test_suite_constant_time.data
@@ -702,3 +702,69 @@
mbedtls_ct_memmove_left 16 16
mbedtls_ct_memmove_left:16:16
+
+mbedtls_ct_memcmp_partial -1 0 0 0
+mbedtls_ct_memcmp_partial:-1:0:0:0
+
+mbedtls_ct_memcmp_partial 0 1 0 0
+mbedtls_ct_memcmp_partial:0:1:0:0
+
+mbedtls_ct_memcmp_partial 0 1 1 0
+mbedtls_ct_memcmp_partial:0:1:1:0
+
+mbedtls_ct_memcmp_partial 0 1 0 1
+mbedtls_ct_memcmp_partial:0:1:0:1
+
+mbedtls_ct_memcmp_partial -1 1 0 0
+mbedtls_ct_memcmp_partial:-1:1:0:0
+
+mbedtls_ct_memcmp_partial 0 2 0 1
+mbedtls_ct_memcmp_partial:0:2:0:1
+
+mbedtls_ct_memcmp_partial 0 2 1 0
+mbedtls_ct_memcmp_partial:0:2:1:0
+
+mbedtls_ct_memcmp_partial 0 16 4 4
+mbedtls_ct_memcmp_partial:0:16:4:4
+
+mbedtls_ct_memcmp_partial 2 16 4 4
+mbedtls_ct_memcmp_partial:2:16:4:4
+
+mbedtls_ct_memcmp_partial 3 16 4 4
+mbedtls_ct_memcmp_partial:3:16:4:4
+
+mbedtls_ct_memcmp_partial 4 16 4 4
+mbedtls_ct_memcmp_partial:4:16:4:4
+
+mbedtls_ct_memcmp_partial 7 16 4 4
+mbedtls_ct_memcmp_partial:7:16:4:4
+
+mbedtls_ct_memcmp_partial 11 16 4 4
+mbedtls_ct_memcmp_partial:11:16:4:4
+
+mbedtls_ct_memcmp_partial 12 16 4 4
+mbedtls_ct_memcmp_partial:12:16:4:4
+
+mbedtls_ct_memcmp_partial 15 16 4 4
+mbedtls_ct_memcmp_partial:15:16:4:4
+
+mbedtls_ct_memcmp_partial 15 16 4 0
+mbedtls_ct_memcmp_partial:15:16:4:0
+
+mbedtls_ct_memcmp_partial 15 16 0 4
+mbedtls_ct_memcmp_partial:15:16:0:4
+
+mbedtls_ct_memcmp_partial 0 16 0 0
+mbedtls_ct_memcmp_partial:0:16:0:0
+
+mbedtls_ct_memcmp_partial 15 16 0 0
+mbedtls_ct_memcmp_partial:15:16:0:0
+
+mbedtls_ct_memcmp_partial -1 16 0 0
+mbedtls_ct_memcmp_partial:-1:16:0:0
+
+mbedtls_ct_memcmp_partial -1 16 12 4
+mbedtls_ct_memcmp_partial:-1:16:12:4
+
+mbedtls_ct_memcmp_partial -1 16 8 8
+mbedtls_ct_memcmp_partial:-1:16:8:8
diff --git a/tests/suites/test_suite_constant_time.function b/tests/suites/test_suite_constant_time.function
index 5dbba06..087ccdf 100644
--- a/tests/suites/test_suite_constant_time.function
+++ b/tests/suites/test_suite_constant_time.function
@@ -259,6 +259,55 @@
}
/* END_CASE */
+/* BEGIN_CASE depends_on:MBEDTLS_NIST_KW_C */
+
+/**
+ * Generate two arrays of the given size, and test mbedtls_ct_memcmp_partial
+ * over them. The arrays will be identical, except that one byte may be specified
+ * to be different.
+ *
+ * \p diff Index of byte that differs (if out of range, the arrays will match).
+ * \p size Size of arrays to compare
+ * \p skip_head Leading bytes to skip, as per mbedtls_ct_memcmp_partial
+ * \p skip_tail Trailing bytes to skip, as per mbedtls_ct_memcmp_partial
+ */
+void mbedtls_ct_memcmp_partial(int diff, int size, int skip_head, int skip_tail)
+{
+ uint8_t *a = NULL, *b = NULL;
+
+ TEST_CALLOC_NONNULL(a, size);
+ TEST_CALLOC_NONNULL(b, size);
+
+ TEST_ASSERT((skip_head + skip_tail) <= size);
+
+ /* Construct data that matches, except for specified byte (if in range). */
+ for (int i = 0; i < size; i++) {
+ a[i] = i & 0xff;
+ b[i] = a[i];
+ if (i == diff) {
+ // modify the specified byte
+ b[i] ^= 1;
+ }
+ }
+
+ int reference = memcmp(a + skip_head, b + skip_head, size - skip_head - skip_tail);
+
+ TEST_CF_SECRET(a, size);
+ TEST_CF_SECRET(b, size);
+
+ int actual = mbedtls_ct_memcmp_partial(a, b, size, skip_head, skip_tail);
+
+ TEST_CF_PUBLIC(a, size);
+ TEST_CF_PUBLIC(b, size);
+ TEST_CF_PUBLIC(&actual, sizeof(actual));
+
+ TEST_EQUAL(!!reference, !!actual);
+exit:
+ mbedtls_free(a);
+ mbedtls_free(b);
+}
+/* END_CASE */
+
/* BEGIN_CASE */
void mbedtls_ct_memcpy_if(int eq, int size, int offset)
{