From: Loup Vaillant <loup@loup-vaillant.fr>
Date: Wed, 20 Sep 2017 21:39:43 +0000 (+0200)
Subject: More robust timing tests, based on ratios.
X-Git-Url: https://git.codecow.com/?a=commitdiff_plain;h=4bf07c4c146302051f213377145a9d982bb5200e;p=Monocypher.git

More robust timing tests, based on ratios.

Fixes #25

Note: I noticed something iffy about comparing against all zeroes: for
big buffers, the timings were way off (small buffers were okay).  This
suggest they were *not* constant time, which is worrying.

The generated assembly is too big for me to review.  I can't tell
whether there's a variable time optimisation in there.  Thankfully, we
rarely use crypto_memcmp() to compare big zeroed buffers in practice.
Instead, we compare small, pseudo random data such as hashes or
authentication tags.  So I used pseudo-random data for the tests.

While we should be good in practice, I'm a bit worried.  Someone may
want to check that compilers haven't become too clever.
---

diff --git a/tests/test.c b/tests/test.c
index 74e73b2..276a867 100644
--- a/tests/test.c
+++ b/tests/test.c
@@ -12,7 +12,7 @@
 #define POLY1305_BLOCK_SIZE  16
 #define BLAKE2B_BLOCK_SIZE  128
 #define SHA_512_BLOCK_SIZE  128
-#define COMPARISON_DIFF_THRESHOLD  4
+#define COMPARISON_DIFF_THRESHOLD 0.04
 
 /////////////////
 /// Utilities ///
@@ -215,47 +215,86 @@ static int test_x25519()
 //////////////////////////////
 
 // Tests that constant-time comparison is actually constant-time.
-static int test_cmp()
+static clock_t min(clock_t a, clock_t b)
 {
-    u8 va[1024 * 64] = {0};
-    u8 vb[1024 * 64] = {0};
-    clock_t t1, t2, d;
-    int status = 0;
-
-    t1 = clock();
-    status |= (crypto_memcmp(va, vb, sizeof(va)) != 0);
-    t2 = clock();
-    d = t2 - t1;
-
-    p_random(vb, sizeof(vb));
-
-    t1 = clock();
-    status |= (crypto_memcmp(va, vb, sizeof(va)) == 0);
-    t2 = clock();
-    d = d - (t2 - t1);
-
-    d = (d < 0 ? -d : d);
-    status |= (d > COMPARISON_DIFF_THRESHOLD);
-
-    printf("%s: memcmp\n", status != 0 ? "FAILED" : "OK");
-
-    t1 = clock();
-    status |= (crypto_zerocmp(va, sizeof(va)) != 0);
-    t2 = clock();
-    d = t2 - t1;
-
-    p_random(vb, sizeof(vb));
+    return a < b ? a : b;
+}
 
-    t1 = clock();
-    status |= (crypto_zerocmp(vb, sizeof(vb)) == 0);
-    t2 = clock();
-    d = d - (t2 - t1);
+static clock_t mem_timing(u8 *va, u8 *vb, size_t size, int expected)
+{
+    clock_t start    = clock();
+    if (crypto_memcmp(va, vb, size) != expected) {
+        fprintf(stderr, "crypto_memcmp() doesn't work!\n");
+    }
+    clock_t end      = clock();
+    clock_t duration = end - start;
+    FOR (i, 0, 20) {
+        start    = clock();
+        if (crypto_memcmp(va, vb, size) != expected) {
+            fprintf(stderr, "crypto_memcmp() doesn't work!\n");
+        }
+        end      = clock();
+        duration = min(end - start, duration);
+    }
+    return duration;
+}
 
-    d = (d < 0 ? -d : d);
-    status |= (d > COMPARISON_DIFF_THRESHOLD);
+static clock_t zero_timing(u8 *va, size_t size, int expected)
+{
+    clock_t start    = clock();
+    if (crypto_zerocmp(va, size) != expected) {
+        fprintf(stderr, "crypto_zerocmp() doesn't work!\n");
+    }
+    clock_t end      = clock();
+    clock_t duration = end - start;
+    FOR (i, 0, 20) {
+        start    = clock();
+        if (crypto_zerocmp(va, size) != expected) {
+            fprintf(stderr, "crypto_zerocmp() doesn't work!\n");
+        }
+        end      = clock();
+        duration = min(end - start, duration);
+    }
+    return duration;
+}
 
-    printf("%s: zerocmp\n", status != 0 ? "FAILED" : "OK");
+#define ABS_DIFF(a, b) ((a) > (b) ? (a) - (b) : (b) - (a))
 
+static int test_cmp()
+{
+    int status = 0;
+    // Because of these static variable, don't call test_cmp() twice.
+    // It's not allocated on the stack to avoid overflow
+    static u8 va[1024 * 1024];
+    static u8 vb[1024 * 1024];
+    p_random(va, sizeof(va));
+    FOR (i, 0, sizeof(va)) {
+        vb[i] = va[i];
+    }
+    {
+        clock_t t1 = mem_timing(va, vb, sizeof(va), 0);
+        p_random(vb, sizeof(vb));
+        clock_t t2 = mem_timing(va, vb, sizeof(va), -1);
+        clock_t d  = ABS_DIFF(t1, t2);
+        double ratio = (double)d / (double)t1;
+        if (ratio > COMPARISON_DIFF_THRESHOLD) {
+            status = -1;
+        }
+    }
+    FOR (i, 0, sizeof(va)) {
+        va[i] = 0;
+    }
+    {
+        clock_t t1 = zero_timing(va, sizeof(va), 0);
+        p_random(va, sizeof(va));
+        clock_t t2 = zero_timing(va, sizeof(va), -1);
+        clock_t d  = ABS_DIFF(t1, t2);
+        double ratio = (double)d / (double)t1;
+        if (ratio > COMPARISON_DIFF_THRESHOLD) {
+            status = -1;
+        }
+    }
+    printf("%s: crypto_memcmp\n", status != 0 ? "SUSPECT TIMINGS" : "OK");
     return status;
 }
 
@@ -539,5 +578,6 @@ int main(void)
     printf("\n-------------------\n");
     status |= test_cmp();
 
+    printf("\n");
     return status;
 }