Use a timing-safe memory compare to verify MACs.

Now that we have modes in which the MAC verification happens before
any other crypto operation and hence will be the only thing seen by an
attacker, it seems like about time we got round to doing it in a
cautious way that tries to prevent the attacker from using our memcmp
as a timing oracle.

So, here's an smemeq() function which has the semantics of !memcmp but
attempts to run in time dependent only on the length parameter. All
the MAC implementations now use this in place of !memcmp to verify the
MAC on input data.

diff --git a/misc.c b/misc.c
index 63de9d11a83072edc32286e471d0f4defb418ecc..94b5ac8ab986e240a3a768996399b294dbe7016f 100644 (file)
--- a/misc.c
+++ b/misc.c
@@ -1019,3 +1019,19 @@ int validate_manual_hostkey(char *key)
 
     return FALSE;
 }
+
+int smemeq(const void *av, const void *bv, size_t len)
+{
+    const unsigned char *a = (const unsigned char *)av;
+    const unsigned char *b = (const unsigned char *)bv;
+    unsigned val = 0;
+
+    while (len-- > 0) {
+        val |= *a++ ^ *b++;
+    }
+    /* Now val is 0 iff we want to return 1, and in the range
+     * 0x01..0xFF iff we want to return 0. So subtracting from 0x100
+     * will clear bit 8 iff we want to return 0, and leave it set iff
+     * we want to return 1, so then we can just shift down. */
+    return (0x100 - val) >> 8;
+}

diff --git a/misc.h b/misc.h
index a361d706b45d40fd8c7908a97fb17520c67f7dc3..c38266d04e4ea1dcb51d48a1f1bc07f82fe34231 100644 (file)
--- a/misc.h
+++ b/misc.h
@@ -64,8 +64,20 @@ int validate_manual_hostkey(char *key);
 
 struct tm ltime(void);
 
+/* Wipe sensitive data out of memory that's about to be freed. Simpler
+ * than memset because we don't need the fill char parameter; also
+ * attempts (by fiddly use of volatile) to inhibit the compiler from
+ * over-cleverly trying to optimise the memset away because it knows
+ * the variable is going out of scope. */
 void smemclr(void *b, size_t len);
 
+/* Compare two fixed-length chunks of memory for equality, without
+ * data-dependent control flow (so an attacker with a very accurate
+ * stopwatch can't try to guess where the first mismatching byte was).
+ * Returns 0 for mismatch or 1 for equality (unlike memcmp), hinted at
+ * by the 'eq' in the name. */
+int smemeq(const void *av, const void *bv, size_t len);
+
 /*
  * Debugging functions.
  *

diff --git a/sshmd5.c b/sshmd5.c
index af139690121af1123fbe556747d2dded037256b7..b7a03b5e13367d820854c72fd0d08d05a4ddf09a 100644 (file)
--- a/sshmd5.c
+++ b/sshmd5.c
@@ -287,7 +287,7 @@ static int hmacmd5_verresult(void *handle, unsigned char const *hmac)
 {
     unsigned char correct[16];
     hmacmd5_genresult(handle, correct);
-    return !memcmp(correct, hmac, 16);
+    return smemeq(correct, hmac, 16);
 }
 
 static void hmacmd5_do_hmac_internal(void *handle,
@@ -327,7 +327,7 @@ static int hmacmd5_verify(void *handle, unsigned char *blk, int len,
 {
     unsigned char correct[16];
     hmacmd5_do_hmac_ssh(handle, blk, len, seq, correct);
-    return !memcmp(correct, blk + len, 16);
+    return smemeq(correct, blk + len, 16);
 }
 
 const struct ssh_mac ssh_hmac_md5 = {

diff --git a/sshsh256.c b/sshsh256.c
index 7ea25fbe1193c7ed1d062c798b63bc41c3fd2d77..fdc8a3a52b7537ccbc78b30f56b54ff9a3849fea 100644 (file)
--- a/sshsh256.c
+++ b/sshsh256.c
@@ -307,7 +307,7 @@ static int hmacsha256_verresult(void *handle, unsigned char const *hmac)
 {
     unsigned char correct[32];
     hmacsha256_genresult(handle, correct);
-    return !memcmp(correct, hmac, 32);
+    return smemeq(correct, hmac, 32);
 }
 
 static int sha256_verify(void *handle, unsigned char *blk, int len,
@@ -315,7 +315,7 @@ static int sha256_verify(void *handle, unsigned char *blk, int len,
 {
     unsigned char correct[32];
     sha256_do_hmac(handle, blk, len, seq, correct);
-    return !memcmp(correct, blk + len, 32);
+    return smemeq(correct, blk + len, 32);
 }
 
 const struct ssh_mac ssh_hmac_sha256 = {

diff --git a/sshsha.c b/sshsha.c
index f22035fe187070c782a20a60cff45723782d9517..579bf4dea17f0d269e6e71b31dbcd24df0f76ba4 100644 (file)
--- a/sshsha.c
+++ b/sshsha.c
@@ -342,7 +342,7 @@ static int hmacsha1_verresult(void *handle, unsigned char const *hmac)
 {
     unsigned char correct[20];
     hmacsha1_genresult(handle, correct);
-    return !memcmp(correct, hmac, 20);
+    return smemeq(correct, hmac, 20);
 }
 
 static int sha1_verify(void *handle, unsigned char *blk, int len,
@@ -350,7 +350,7 @@ static int sha1_verify(void *handle, unsigned char *blk, int len,
 {
     unsigned char correct[20];
     sha1_do_hmac(handle, blk, len, seq, correct);
-    return !memcmp(correct, blk + len, 20);
+    return smemeq(correct, blk + len, 20);
 }
 
 static void hmacsha1_96_genresult(void *handle, unsigned char *hmac)
@@ -372,7 +372,7 @@ static int hmacsha1_96_verresult(void *handle, unsigned char const *hmac)
 {
     unsigned char correct[20];
     hmacsha1_genresult(handle, correct);
-    return !memcmp(correct, hmac, 12);
+    return smemeq(correct, hmac, 12);
 }
 
 static int sha1_96_verify(void *handle, unsigned char *blk, int len,
@@ -380,7 +380,7 @@ static int sha1_96_verify(void *handle, unsigned char *blk, int len,
 {
     unsigned char correct[20];
     sha1_do_hmac(handle, blk, len, seq, correct);
-    return !memcmp(correct, blk + len, 12);
+    return smemeq(correct, blk + len, 12);
 }
 
 void hmac_sha1_simple(void *key, int keylen, void *data, int datalen,