keys: compare MACs in constant time

Switch from memcmp() to the new constant-time function to compare the
received and expected authentication data generated with a symmetric key
(NTP MAC or AES CMAC).

While this doesn't seem to be strictly necessary with the current
code, it is a recommended practice to prevent timing attacks. If
memcmp() compared the MACs one byte at a time (a typical memcmp()
implementation works with wider integers for better performance) and
chronyd as an NTP client/server/peer was leaking the timing of the
comparison (e.g. in the monitoring protocol), an attacker might be able
for a given NTP request or response find in a sequence the individual
bytes of the MAC by observing differences in the timing over a large
number of attempts. However, this process would likely be so slow the
authenticated request or response would not be useful in a MITM attack
as the expected origin timestamp is changing with each poll.

Extend the keys unit test to compare the time the function takes to
compare two identical MACs and MACs differing in the first byte
(maximizing the timing difference). It should fail if the compiler's
optimizations figure out the function can return early. The test is not
included in the util unit test to avoid compile-time optimizations with
the function and its caller together. The test can be disabled by
setting NO_TIMING_TESTS environment variable if it turns out to be
unreliable.

diff --git a/keys.c b/keys.c
index 9225e6cdd5ba6872185059fec6641b70db0bf9ef..c86685f94260fb89ce3c8def07d0fcff4f4df2c0 100644 (file)
--- a/keys.c
+++ b/keys.c
@@ -405,7 +405,7 @@ check_auth(Key *key, const void *data, int data_len,
 
   hash_len = generate_auth(key, data, data_len, buf, sizeof (buf));
 
-  return MIN(hash_len, trunc_len) == auth_len && !memcmp(buf, auth, auth_len);
+  return MIN(hash_len, trunc_len) == auth_len && UTI_IsMemoryEqual(buf, auth, auth_len);
 }
 
 /* ================================================== */

diff --git a/test/unit/keys.c b/test/unit/keys.c
index 47d907b56370b3243328605e64e34ea62f4df37d..f85bb2059915ec86d6c5bc9436899b5c4f9a73fe 100644 (file)
--- a/test/unit/keys.c
+++ b/test/unit/keys.c
@@ -19,12 +19,14 @@
  */
 
 #include <config.h>
+#include <local.h>
 #include "test.h"
 
 #include <keys.c>
 
 #define KEYS 100
 #define KEYFILE "keys.test-keys"
+#define MIN_TIMING_INTERVAL 1.0e-3
 
 static
 uint32_t write_random_key(FILE *f)
@@ -97,9 +99,11 @@ generate_key_file(const char *name, uint32_t *keys)
 void
 test_unit(void)
 {
-  int i, j, data_len, auth_len, type, bits;
+  int i, j, data_len, auth_len, type, bits, s, timing_fails, timing_iters;
   uint32_t keys[KEYS], key;
-  unsigned char data[100], auth[MAX_HASH_LENGTH];
+  unsigned char data[100], auth[MAX_HASH_LENGTH], auth2[MAX_HASH_LENGTH];
+  struct timespec ts1, ts2;
+  double diff1, diff2;
   char conf[][100] = {
     "keyfile "KEYFILE
   };
@@ -107,6 +111,7 @@ test_unit(void)
   CNF_Initialise(0, 0);
   for (i = 0; i < sizeof conf / sizeof conf[0]; i++)
     CNF_ParseLine(NULL, i + 1, conf[i]);
+  LCL_Initialise();
 
   generate_key_file(KEYFILE, keys);
   KEY_Initialise();
@@ -156,9 +161,63 @@ test_unit(void)
     }
   }
 
+  if (!getenv("NO_TIMING_TESTS") &&
+      LCL_GetSysPrecisionAsQuantum() < MIN_TIMING_INTERVAL / 100.0) {
+    auth_len = sizeof (auth);
+    UTI_GetRandomBytes(auth, auth_len);
+    memcpy(auth2, auth, auth_len);
+
+    timing_fails = 0;
+    timing_iters = 1000;
+
+    i = 0;
+    for (i = 0; i < 100; i++) {
+      int d = random() % 2;
+
+      auth2[0] = auth[0] + d;
+
+      for (j = s = 0; j < timing_iters; j++) {
+        if (j == 100)
+          LCL_ReadRawTime(&ts1);
+        s += UTI_IsMemoryEqual(auth, auth2, auth_len);
+      }
+      LCL_ReadRawTime(&ts2);
+      TEST_CHECK(s == (d + 1) % 2 * timing_iters);
+      diff1 = UTI_DiffTimespecsToDouble(&ts2, &ts1);
+
+      auth2[0] = auth[0] + (d + 1) % 2;
+
+      for (j = s = 0; j < timing_iters; j++) {
+        if (j == 100)
+          LCL_ReadRawTime(&ts1);
+        s += UTI_IsMemoryEqual(auth, auth2, auth_len);
+      }
+      LCL_ReadRawTime(&ts2);
+      TEST_CHECK(s == d * timing_iters);
+      diff2 = UTI_DiffTimespecsToDouble(&ts2, &ts1);
+
+      DEBUG_LOG("d=%d diff1=%e diff2=%e iters=%d", d, diff1, diff2, timing_iters);
+
+      if (diff1 < MIN_TIMING_INTERVAL && diff2 < MIN_TIMING_INTERVAL) {
+        if (timing_iters >= INT_MAX / 2)
+          break;
+        timing_iters *= 2;
+        i--;
+        continue;
+      }
+
+      if ((d == 0 && 0.8 * diff1 > diff2) || (d == 1 && diff1 < 0.8 * diff2))
+        timing_fails++;
+    }
+
+    DEBUG_LOG("timing fails %d/%d", timing_fails, i);
+    TEST_CHECK(timing_fails < i / 2);
+  }
+
   unlink(KEYFILE);
 
   KEY_Finalise();
+  LCL_Finalise();
   CNF_Finalise();
   HSH_Finalise();
 }