Re: OpenSSH/OpenSSL patches to stop excessive entropy consumption

Thor Lancelot Simon <[email protected]> Sat, 3 Mar 2012 23:12:23 -0500
Newsgroups gmane.os.netbsd.current,gmane.os.netbsd.devel.crypto,gmane.os.netbsd.devel.security
Message-ID <[email protected]>
Sorry, something was wrong with that diff.  This one is right.

Thor
ssl-ssh-entropy.diff (text/plain, 6.4 KB)
? ssl-ssh-entropy.diff
Index: openssh/dist/sshd.c
===================================================================
RCS file: /cvsroot/src/crypto/external/bsd/openssh/dist/sshd.c,v
retrieving revision 1.8
diff -u -r1.8 sshd.c
--- openssh/dist/sshd.c	16 Sep 2011 15:36:18 -0000	1.8
+++ openssh/dist/sshd.c	4 Mar 2012 04:10:49 -0000
@@ -55,6 +55,7 @@
 #include <sys/time.h>
 #include <sys/queue.h>
 
+#include <assert.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <netdb.h>
@@ -129,7 +130,10 @@
 #define REEXEC_DEVCRYPTO_RESERVED_FD	(STDERR_FILENO + 1)
 #define REEXEC_STARTUP_PIPE_FD		(STDERR_FILENO + 2)
 #define REEXEC_CONFIG_PASS_FD		(STDERR_FILENO + 3)
-#define REEXEC_MIN_FREE_FD		(STDERR_FILENO + 4)
+#define REEXEC_DEVURANDOM_FD		(STDERR_FILENO + 4)
+#define REEXEC_MIN_FREE_FD		(STDERR_FILENO + 5)
+
+int urandom_fd = -1;
 
 int myflag = 0;
 
@@ -582,17 +586,18 @@
 static void
 privsep_preauth_child(void)
 {
-	u_int32_t rnd[256];
+	u_int32_t rnd[32];
 	gid_t gidset[1];
 	struct passwd *pw;
 
 	/* Enable challenge-response authentication for privilege separation */
 	privsep_challenge_enable();
 
-	arc4random_stir();
-	arc4random_buf(rnd, sizeof(rnd));
+	assert((read(urandom_fd, rnd, sizeof(rnd)) == sizeof(rnd)));
 	RAND_seed(rnd, sizeof(rnd));
 
+	arc4random_stir();
+
 	/* Demote the private keys to public keys. */
 	demote_sensitive_data();
 
@@ -689,7 +694,7 @@
 static void
 privsep_postauth(Authctxt *authctxt)
 {
-	u_int32_t rnd[256];
+	u_int32_t rnd[32];
 
 	if (authctxt->pw->pw_uid == 0 || options.use_login) {
 		/* File descriptor passing is broken or root login */
@@ -720,10 +725,11 @@
 	/* Demote the private keys to public keys. */
 	demote_sensitive_data();
 
-	arc4random_stir();
-	arc4random_buf(rnd, sizeof(rnd));
+	assert((read(urandom_fd, rnd, sizeof(rnd)) == sizeof(rnd)));
 	RAND_seed(rnd, sizeof(rnd));
 
+	arc4random_stir();
+
 	/* Drop privileges */
 	do_setusercontext(authctxt->pw);
 
@@ -1091,6 +1097,7 @@
 	struct sockaddr_storage from;
 	socklen_t fromlen;
 	pid_t pid;
+	uint8_t rnd[32];
 
 	/* setup fd set for accept */
 	fdset = NULL;
@@ -1283,6 +1290,9 @@
 			 * Ensure that our random state differs
 			 * from that of the child
 			 */
+			assert(read(urandom_fd, rnd, sizeof(rnd)) ==
+			       sizeof(rnd));
+			RAND_seed(rnd, sizeof(rnd));
 			arc4random_stir();
 		}
 
@@ -1312,6 +1322,7 @@
 	mode_t new_umask;
 	Key *key;
 	Authctxt *authctxt;
+	uint8_t rnd[32];
 
 	/* Save argv. */
 	saved_argv = av;
@@ -1462,6 +1473,33 @@
 	OpenSSL_add_all_algorithms();
 
 	/*
+	 * The OpenSSL PRNG is used by key-generation functions we
+	 * rely on for security.  Seed it ourselves, so that:
+	 *
+	 *	A) it does not seed itself from somewhere questionable,
+	 *	   such as the libc arc4random or, worse, getpid().
+	 *	B) it does not reopen /dev/urandom on systems where
+	 *	   this is expensive (generator keyed on open, etc).
+	 *
+	 * Note that /dev/urandom will never return the same data to
+	 * two callers, even if they have the same dup'd reference to it.
+	 */
+	if (rexeced_flag) {
+		urandom_fd = REEXEC_DEVURANDOM_FD;
+	} else {
+		urandom_fd = open("/dev/urandom", O_RDONLY);
+		if (urandom_fd == -1) {
+			fatal("sshd requires random device");
+		}
+		/* Might as well do this here; why do it later? */
+		dup2(urandom_fd, REEXEC_DEVURANDOM_FD);
+		close(urandom_fd);
+		urandom_fd = REEXEC_DEVURANDOM_FD;
+	}
+	assert(read(urandom_fd, rnd, sizeof(rnd)) == sizeof(rnd));
+	RAND_seed(rnd, sizeof(rnd));
+
+	/*
 	 * Force logging to stderr until we have loaded the private host
 	 * key (unless started from inetd)
 	 */
@@ -1703,7 +1741,7 @@
 	/* Reinitialize the log (because of the fork above). */
 	log_init(__progname, options.log_level, options.log_facility, log_stderr);
 
-	/* Initialize the random number generator. */
+	/* Initialize the fast random number generator. */
 	arc4random_stir();
 
 	/* Chdir to the root directory so that the current disk can be
Index: openssl/dist/crypto/rand/md_rand.c
===================================================================
RCS file: /cvsroot/src/crypto/external/bsd/openssl/dist/crypto/rand/md_rand.c,v
retrieving revision 1.1.1.3
diff -u -r1.1.1.3 md_rand.c
--- openssl/dist/crypto/rand/md_rand.c	5 Jun 2011 14:59:27 -0000	1.1.1.3
+++ openssl/dist/crypto/rand/md_rand.c	4 Mar 2012 04:10:49 -0000
@@ -141,7 +141,6 @@
 static unsigned char md[MD_DIGEST_LENGTH];
 static long md_count[2]={0,0};
 static double entropy=0;
-static int initialized=0;
 
 static unsigned int crypto_lock_rand = 0; /* may be set only when a thread
                                            * holds CRYPTO_LOCK_RAND
@@ -187,7 +186,6 @@
 	md_count[0]=0;
 	md_count[1]=0;
 	entropy=0;
-	initialized=0;
 	}
 
 static void ssleay_rand_add(const void *buf, int num, double add)
@@ -389,18 +387,15 @@
 	CRYPTO_w_unlock(CRYPTO_LOCK_RAND2);
 	crypto_lock_rand = 1;
 
-	if (!initialized)
-		{
-		RAND_poll();
-		initialized = 1;
-		}
-	
 	if (!stirred_pool)
 		do_stir_pool = 1;
 	
 	ok = (entropy >= ENTROPY_NEEDED);
 	if (!ok)
 		{
+
+		RAND_poll();
+
 		/* If the PRNG state is not yet unpredictable, then seeing
 		 * the PRNG output may help attackers to determine the new
 		 * state; thus we have to decrease the entropy estimate.
@@ -571,11 +566,10 @@
 		CRYPTO_w_unlock(CRYPTO_LOCK_RAND2);
 		crypto_lock_rand = 1;
 		}
-	
-	if (!initialized)
+
+	if (entropy < ENTROPY_NEEDED)
 		{
 		RAND_poll();
-		initialized = 1;
 		}
 
 	ret = entropy >= ENTROPY_NEEDED;
Index: openssl/dist/crypto/rand/rand_unix.c
===================================================================
RCS file: /cvsroot/src/crypto/external/bsd/openssl/dist/crypto/rand/rand_unix.c,v
retrieving revision 1.2
diff -u -r1.2 rand_unix.c
--- openssl/dist/crypto/rand/rand_unix.c	19 Jul 2009 23:30:41 -0000	1.2
+++ openssl/dist/crypto/rand/rand_unix.c	4 Mar 2012 04:10:50 -0000
@@ -182,6 +182,16 @@
 	u_int32_t rnd = 0, i;
 	unsigned char buf[ENTROPY_NEEDED];
 
+	/*
+	 * XXX is this really a good idea?  It has the seemingly
+	 * XXX very undesirable eventual result of keying the CTR_DRBG
+	 * XXX generator exclusively with key material produced by
+	 * XXX the libc arc4random().  It also guarantees that even
+	 * XXX if the generator tries to use RAND_poll() to rekey
+	 * XXX itself after a call to fork() etc, it will end up with
+	 * XXX the same state, since the libc arc4 state will be the same
+	 * XXX unless explicitly updated by the application.
+	 */
 	for (i = 0; i < sizeof(buf); i++) {
 		if (i % 4 == 0)
 			rnd = arc4random();