From 26d3ccdfc5500a8afdbd54938677ef4ef985caaa Mon Sep 17 00:00:00 2001
From: Simon Tatham <anakin@pobox.com>
Date: Sun, 26 Apr 2015 23:31:11 +0100
Subject: [PATCH] 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.

(cherry picked from commit 9d5a16402168f82ba1bd695c3e95bb4812ccd0a9)

Cherry-picker's notes: the above commit comment isn't really true on
this branch, since the ETM packet protocol changes haven't been
cherry-picked. But it seemed silly to deliberately leave out even a
small safety measure.
---
 misc.c     | 16 ++++++++++++++++
 misc.h     | 12 ++++++++++++
 sshmd5.c   |  4 ++--
 sshsh256.c |  4 ++--
 sshsha.c   |  8 ++++----
 5 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/misc.c b/misc.c
index 63de9d11..94b5ac8a 100644
--- 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 a361d706..c38266d0 100644
--- 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 2fdb5900..493db12c 100644
--- 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 60c5217f..85bfeee2 100644
--- 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 a5b3a60c..31d6daa9 100644
--- 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,
-- 
2.45.2