Merge pull request #1059 from daverodgman/ct_memcmp_fix

Constant time memcmp check for 16-bit int

library/constant_time.c

@@ -22,6 +22,7 @@
  * might be translated to branches by some compilers on some platforms.
  */
 
+#include <stdint.h>
 #include <limits.h>
 
 #include "common.h"
@@ -120,7 +121,24 @@ int mbedtls_ct_memcmp(const void *a,
         diff |= x ^ y;
     }
 
-    return (int) diff;
+
+#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
+     * 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
+    /* 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
 }
 
 #if defined(MBEDTLS_PKCS1_V15) && defined(MBEDTLS_RSA_C) && !defined(MBEDTLS_RSA_ALT)

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
 

tests/suites/test_suite_constant_time.function

@@ -172,6 +172,49 @@ 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_CF_PUBLIC(&result, sizeof(result));
+
+    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_CF_PUBLIC(&result, sizeof(result));
+
+            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)
 {