- djm@cvs.openbsd.org 2010/07/13 11:52:06

     [auth-rsa.c channels.c jpake.c key.c misc.c misc.h monitor.c]
     [packet.c ssh-rsa.c]
     implement a timing_safe_cmp() function to compare memory without leaking
     timing information by short-circuiting like memcmp() and use it for
     some of the more sensitive comparisons (though nothing high-value was
     readily attackable anyway); "looks ok" markus@

10 files changed, 45 insertions, 23 deletions

diff --git a/ChangeLog b/ChangeLog
index f652e618..d650f63f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -22,6 +22,13 @@
         Hostname %h.example.org
      
      "I like it" markus@
+   - djm@cvs.openbsd.org 2010/07/13 11:52:06
+     [auth-rsa.c channels.c jpake.c key.c misc.c misc.h monitor.c]
+     [packet.c ssh-rsa.c]
+     implement a timing_safe_cmp() function to compare memory without leaking
+     timing information by short-circuiting like memcmp() and use it for
+     some of the more sensitive comparisons (though nothing high-value was
+     readily attackable anyway); "looks ok" markus@
 
 20100714
  - (tim) [contrib/redhat/openssh.spec] Bug 1796: Test for skip_x11_askpass
diff --git a/auth-rsa.c b/auth-rsa.c
index ef6767bf..71abadf6 100644
--- a/auth-rsa.c
+++ b/auth-rsa.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: auth-rsa.c,v 1.76 2010/05/11 02:58:04 djm Exp $ */
+/* $OpenBSD: auth-rsa.c,v 1.77 2010/07/13 11:52:06 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -116,7 +116,7 @@ auth_rsa_verify_response(Key *key, BIGNUM *challenge, u_char response[16])
 	MD5_Final(mdbuf, &md);
 
 	/* Verify that the response is the original challenge. */
-	if (memcmp(response, mdbuf, 16) != 0) {
+	if (timing_safe_cmp(response, mdbuf, 16) != 0) {
 		/* Wrong answer. */
 		return (0);
 	}
diff --git a/channels.c b/channels.c
index fe08257d..e52946bc 100644
--- a/channels.c
+++ b/channels.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: channels.c,v 1.306 2010/06/25 07:20:04 djm Exp $ */
+/* $OpenBSD: channels.c,v 1.307 2010/07/13 11:52:06 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -917,7 +917,7 @@ x11_open_helper(Buffer *b)
 	}
 	/* Check if authentication data matches our fake data. */
 	if (data_len != x11_fake_data_len ||
-	    memcmp(ucp + 12 + ((proto_len + 3) & ~3),
+	    timing_safe_cmp(ucp + 12 + ((proto_len + 3) & ~3),
 		x11_fake_data, x11_fake_data_len) != 0) {
 		debug2("X11 auth data does not match fake data.");
 		return -1;
diff --git a/jpake.c b/jpake.c
index 13066106..50cf5c82 100644
--- a/jpake.c
+++ b/jpake.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: jpake.c,v 1.2 2009/03/05 07:18:19 djm Exp $ */
+/* $OpenBSD: jpake.c,v 1.3 2010/07/13 11:52:06 djm Exp $ */
 /*
  * Copyright (c) 2008 Damien Miller.  All rights reserved.
  *
@@ -434,7 +434,7 @@ jpake_check_confirm(const BIGNUM *k,
 	if (peer_confirm_hash_len != expected_confirm_hash_len)
 		error("%s: confirmation length mismatch (my %u them %u)",
 		    __func__, expected_confirm_hash_len, peer_confirm_hash_len);
-	else if (memcmp(peer_confirm_hash, expected_confirm_hash,
+	else if (timing_safe_cmp(peer_confirm_hash, expected_confirm_hash,
 	    expected_confirm_hash_len) == 0)
 		success = 1;
 	bzero(expected_confirm_hash, expected_confirm_hash_len);
diff --git a/key.c b/key.c
index c4d9d577..4e31c84e 100644
--- a/key.c
+++ b/key.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: key.c,v 1.88 2010/05/07 11:30:29 djm Exp $ */
+/* $OpenBSD: key.c,v 1.89 2010/07/13 11:52:06 djm Exp $ */
 /*
  * read_bignum():
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -52,6 +52,7 @@
 #include "uuencode.h"
 #include "buffer.h"
 #include "log.h"
+#include "misc.h"
 #include "ssh2.h"
 
 static struct KeyCert *
@@ -227,7 +228,7 @@ cert_compare(struct KeyCert *a, struct KeyCert *b)
 		return 0;
 	if (buffer_len(&a->certblob) != buffer_len(&b->certblob))
 		return 0;
-	if (memcmp(buffer_ptr(&a->certblob), buffer_ptr(&b->certblob),
+	if (timing_safe_cmp(buffer_ptr(&a->certblob), buffer_ptr(&b->certblob),
 	    buffer_len(&a->certblob)) != 0)
 		return 0;
 	return 1;
diff --git a/misc.c b/misc.c
index 4500b7a3..3b98e3fc 100644
--- a/misc.c
+++ b/misc.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: misc.c,v 1.77 2010/07/02 04:32:44 djm Exp $ */
+/* $OpenBSD: misc.c,v 1.78 2010/07/13 11:52:06 djm Exp $ */
 /*
  * Copyright (c) 2000 Markus Friedl.  All rights reserved.
  * Copyright (c) 2005,2006 Damien Miller.  All rights reserved.
@@ -850,6 +850,18 @@ ms_to_timeval(struct timeval *tv, int ms)
 	tv->tv_usec = (ms % 1000) * 1000;
 }
 
+int
+timing_safe_cmp(const void *_s1, const void *_s2, size_t n)
+{
+	u_char *s1 = (u_char *)_s1;
+	u_char *s2 = (u_char *)_s2;
+	int ret = 0;
+
+	for (; n > 0; n--, s1++, s2++)
+		ret |= *s1 ^ *s2;
+	return ret;
+}
+
 void
 sock_set_v6only(int s)
 {
diff --git a/misc.h b/misc.h
index 32073acd..7a02a03a 100644
--- a/misc.h
+++ b/misc.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: misc.h,v 1.41 2010/01/09 23:04:13 dtucker Exp $ */
+/* $OpenBSD: misc.h,v 1.42 2010/07/13 11:52:06 djm Exp $ */
 
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
@@ -36,6 +36,7 @@ void	 sanitise_stdfd(void);
 void	 ms_subtract_diff(struct timeval *, int *);
 void	 ms_to_timeval(struct timeval *, int);
 void	 sock_set_v6only(int);
+int	 timing_safe_cmp(const void *, const void *, size_t);
 
 struct passwd *pwcopy(struct passwd *);
 const char *ssh_gai_strerror(int);
diff --git a/monitor.c b/monitor.c
index 334aedde..92072839 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: monitor.c,v 1.106 2010/03/07 11:57:13 dtucker Exp $ */
+/* $OpenBSD: monitor.c,v 1.107 2010/07/13 11:52:06 djm Exp $ */
 /*
  * Copyright 2002 Niels Provos <provos@citi.umich.edu>
  * Copyright 2002 Markus Friedl <markus@openbsd.org>
@@ -518,7 +518,7 @@ monitor_allowed_key(u_char *blob, u_int bloblen)
 {
 	/* make sure key is allowed */
 	if (key_blob == NULL || key_bloblen != bloblen ||
-	    memcmp(key_blob, blob, key_bloblen))
+	    timing_safe_cmp(key_blob, blob, key_bloblen))
 		return (0);
 	return (1);
 }
@@ -1103,14 +1103,14 @@ monitor_valid_userblob(u_char *data, u_int datalen)
 		len = buffer_len(&b);
 		if ((session_id2 == NULL) ||
 		    (len < session_id2_len) ||
-		    (memcmp(p, session_id2, session_id2_len) != 0))
+		    (timing_safe_cmp(p, session_id2, session_id2_len) != 0))
 			fail++;
 		buffer_consume(&b, session_id2_len);
 	} else {
 		p = buffer_get_string(&b, &len);
 		if ((session_id2 == NULL) ||
 		    (len != session_id2_len) ||
-		    (memcmp(p, session_id2, session_id2_len) != 0))
+		    (timing_safe_cmp(p, session_id2, session_id2_len) != 0))
 			fail++;
 		xfree(p);
 	}
@@ -1158,7 +1158,7 @@ monitor_valid_hostbasedblob(u_char *data, u_int datalen, char *cuser,
 	p = buffer_get_string(&b, &len);
 	if ((session_id2 == NULL) ||
 	    (len != session_id2_len) ||
-	    (memcmp(p, session_id2, session_id2_len) != 0))
+	    (timing_safe_cmp(p, session_id2, session_id2_len) != 0))
 		fail++;
 	xfree(p);
 
@@ -1682,9 +1682,9 @@ mm_get_kex(Buffer *m)
 
 	kex = xcalloc(1, sizeof(*kex));
 	kex->session_id = buffer_get_string(m, &kex->session_id_len);
-	if ((session_id2 == NULL) ||
-	    (kex->session_id_len != session_id2_len) ||
-	    (memcmp(kex->session_id, session_id2, session_id2_len) != 0))
+	if (session_id2 == NULL ||
+	    kex->session_id_len != session_id2_len ||
+	    timing_safe_cmp(kex->session_id, session_id2, session_id2_len) != 0)
 		fatal("mm_get_get: internal error: bad session id");
 	kex->we_need = buffer_get_int(m);
 	kex->kex[KEX_DH_GRP1_SHA1] = kexdh_server;
diff --git a/packet.c b/packet.c
index 994e35b6..5c7ec2b5 100644
--- a/packet.c
+++ b/packet.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: packet.c,v 1.166 2009/06/27 09:29:06 andreas Exp $ */
+/* $OpenBSD: packet.c,v 1.167 2010/07/13 11:52:06 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -1307,7 +1307,7 @@ packet_read_poll2(u_int32_t *seqnr_p)
 		macbuf = mac_compute(mac, active_state->p_read.seqnr,
 		    buffer_ptr(&active_state->incoming_packet),
 		    buffer_len(&active_state->incoming_packet));
-		if (memcmp(macbuf, buffer_ptr(&active_state->input),
+		if (timing_safe_cmp(macbuf, buffer_ptr(&active_state->input),
 		    mac->mac_len) != 0) {
 			logit("Corrupted MAC on input.");
 			if (need > PACKET_MAX_SIZE)
diff --git a/ssh-rsa.c b/ssh-rsa.c
index bb9cc8e2..01f27f52 100644
--- a/ssh-rsa.c
+++ b/ssh-rsa.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssh-rsa.c,v 1.41 2010/04/16 01:47:26 djm Exp $ */
+/* $OpenBSD: ssh-rsa.c,v 1.42 2010/07/13 11:52:06 djm Exp $ */
 /*
  * Copyright (c) 2000, 2003 Markus Friedl <markus@openbsd.org>
  *
@@ -30,6 +30,7 @@
 #include "buffer.h"
 #include "key.h"
 #include "compat.h"
+#include "misc.h"
 #include "ssh.h"
 
 static int openssh_RSA_verify(int, u_char *, u_int, u_char *, u_int, RSA *);
@@ -249,11 +250,11 @@ openssh_RSA_verify(int type, u_char *hash, u_int hashlen,
 		error("bad decrypted len: %d != %d + %d", len, hlen, oidlen);
 		goto done;
 	}
-	if (memcmp(decrypted, oid, oidlen) != 0) {
+	if (timing_safe_cmp(decrypted, oid, oidlen) != 0) {
 		error("oid mismatch");
 		goto done;
 	}
-	if (memcmp(decrypted + oidlen, hash, hlen) != 0) {
+	if (timing_safe_cmp(decrypted + oidlen, hash, hlen) != 0) {
 		error("hash mismatch");
 		goto done;
 	}