Merge pull request #1073 from Mbed-TLS/better-ct-memcmp

More consistent use of mbedtls_ct_memcmp

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 @@ static int mbedtls_ccm_compare_tags(const unsigned char *tag1,
                                     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;

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 mbedtls_chachapoly_auth_decrypt(mbedtls_chachapoly_context *ctx,
 {
     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 @@ int mbedtls_chachapoly_auth_decrypt(mbedtls_chachapoly_context *ctx,
     }
 
     /* 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);

library/constant_time.c

@@ -141,6 +141,36 @@ int mbedtls_ct_memcmp(const void *a,
 #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)

library/constant_time_internal.h

@@ -492,6 +492,37 @@ void mbedtls_ct_memcpy_offset(unsigned char *dest,
                          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"
 

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 mbedtls_gcm_auth_decrypt(mbedtls_gcm_context *ctx,
 {
     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 @@ int mbedtls_gcm_auth_decrypt(mbedtls_gcm_context *ctx,
     }
 
     /* 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);

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 @@ int mbedtls_nist_kw_unwrap(mbedtls_nist_kw_context *ctx,
                            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 @@ int mbedtls_nist_kw_unwrap(mbedtls_nist_kw_context *ctx,
          * 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 @@ cleanup:
         *out_len = 0;
     }
 
-    mbedtls_platform_zeroize(&bad_padding, sizeof(bad_padding));
     mbedtls_platform_zeroize(&diff, sizeof(diff));
     mbedtls_platform_zeroize(A, sizeof(A));
 

library/rsa.c

@@ -1541,7 +1541,8 @@ int mbedtls_rsa_rsaes_oaep_decrypt(mbedtls_rsa_context *ctx,
 {
     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 @@ int mbedtls_rsa_rsaes_oaep_decrypt(mbedtls_rsa_context *ctx,
     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) */

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)
 

tests/suites/test_suite_constant_time.data

@@ -702,3 +702,69 @@ mbedtls_ct_memmove_left:16:15
 
 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

tests/suites/test_suite_constant_time.function

@@ -259,6 +259,55 @@ exit:
 }
 /* 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)
 {