From: Miroslav Lichvar <mlichvar@redhat.com>
Date: Wed, 2 Apr 2025 13:34:43 +0000 (+0200)
Subject: keys: compare MACs in constant time
X-Git-Tag: 4.7-pre1~22
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d115449e854728662647b0edd95559b8a09c2b8c;p=thirdparty%2Fchrony.git

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 9225e6cd..c86685f9 100644
--- 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 47d907b5..f85bb205 100644
--- 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();
 }