Use constant time memcmp() when comparing packet HMACs.

This eliminates a timing side channel vulnerability, which could
potentially allow an attacker to compute a valid HMAC, and insert arbitrary
ciphertext data into the connection. If an attacker also identifies packets
with a plaintext it can guess (e.g. small packets every 1s are probably
pings), the attacker can xor the ciphertext to mangle the packet to
arbitrary plaintext. Although this type of attack is rarely seen in the
wild, it is generally considered technically viable.

Signed-off-by: Steffan Karger <steffan@karger.me>

diff --git a/src/net_packet.c b/src/net_packet.c
index 905b944cd4eebd8327bcca77d3e374da7d5fa0c0..4341cf01f76c4754cb7ff5f74d019a38510abad1 100644 (file)
--- a/src/net_packet.c
+++ b/src/net_packet.c
@@ -269,7 +269,7 @@ static bool try_mac(const node_t *n, const vpn_packet_t *inpkt) {
 
        HMAC(n->indigest, n->inkey, n->inkeylength, (unsigned char *) &inpkt->seqno, inpkt->len - n->inmaclength, (unsigned char *)hmac, NULL);
 
-       return !memcmp(hmac, (char *) &inpkt->seqno + inpkt->len - n->inmaclength, n->inmaclength);
+       return !memcmp_constant_time(hmac, (char *) &inpkt->seqno + inpkt->len - n->inmaclength, n->inmaclength);
 }
 
 static void receive_udppacket(node_t *n, vpn_packet_t *inpkt) {
@@ -302,7 +302,7 @@ static void receive_udppacket(node_t *n, vpn_packet_t *inpkt) {
                HMAC(n->indigest, n->inkey, n->inkeylength,
                         (unsigned char *) &inpkt->seqno, inpkt->len, (unsigned char *)hmac, NULL);
 
-               if(memcmp(hmac, (char *) &inpkt->seqno + inpkt->len, n->inmaclength)) {
+               if(memcmp_constant_time(hmac, (char *) &inpkt->seqno + inpkt->len, n->inmaclength)) {
                        ifdebug(TRAFFIC) logger(LOG_DEBUG, "Got unauthenticated packet from %s (%s)",
                                           n->name, n->hostname);
                        return;

diff --git a/src/utils.c b/src/utils.c
index 3b221f59f0ef731f62b0369314de5964bdf855fd..c629ae0b50cd94a5b0169ca868e6f2c55357a525 100644 (file)
--- a/src/utils.c
+++ b/src/utils.c
@@ -78,3 +78,18 @@ unsigned int bitfield_to_int(const void *bitfield, size_t size) {
        memcpy(&value, bitfield, size);
        return value;
 }
+
+/**
+ * As memcmp(), but constant-time.
+ * Returns 0 when data is equal, non-zero otherwise.
+ */
+int memcmp_constant_time (const void *a, const void *b, size_t size) {
+  const uint8_t *a1 = a, *b1 = b;
+  int ret = 0;
+  size_t i;
+
+  for (i = 0; i < size; i++)
+      ret |= *a1++ ^ *b1++;
+
+  return ret;
+}

diff --git a/src/utils.h b/src/utils.h
index 2e4fcf86083154b54399b8e34773d294ca1c9fd2..7ab29d2ced15f91a706e3e1c755c8dda4c4357f7 100644 (file)
--- a/src/utils.h
+++ b/src/utils.h
@@ -42,4 +42,6 @@ extern const char *winerror(int);
 
 extern unsigned int bitfield_to_int(const void *bitfield, size_t size);
 
+int memcmp_constant_time (const void *a, const void *b, size_t size);
+
 #endif                                                 /* __TINC_UTILS_H__ */