fixes for random number badness (lack of entropy and SIGFPE from RAND_cleanup

too early).


git-svn-id: file:///svn/unbound/trunk@907 be551aaa-1e26-0410-a405-d3ace91eadb9

config.h.in

@@ -76,6 +76,9 @@
 /* Define to 1 if you have the <openssl/err.h> header file. */
 #undef HAVE_OPENSSL_ERR_H
 
+/* Define to 1 if you have the <openssl/rand.h> header file. */
+#undef HAVE_OPENSSL_RAND_H
+
 /* Define to 1 if you have the <openssl/ssl.h> header file. */
 #undef HAVE_OPENSSL_SSL_H
 
@@ -332,6 +335,10 @@
 #include <openssl/err.h>
 #endif
 
+#ifdef HAVE_OPENSSL_RAND_H
+#include <openssl/rand.h>
+#endif
+
 #ifdef HAVE_ATTR_FORMAT
 #  define ATTR_FORMAT(archetype, string_index, first_to_check) \
     __attribute__ ((format (archetype, string_index, first_to_check)))

configure

@@ -20887,6 +20887,64 @@ fi
 done
 
 
+for ac_header in openssl/rand.h
+do
+as_ac_Header=`echo "ac_cv_header_$ac_header" | $as_tr_sh`
+{ echo "$as_me:$LINENO: checking for $ac_header" >&5
+echo $ECHO_N "checking for $ac_header... $ECHO_C" >&6; }
+if { as_var=$as_ac_Header; eval "test \"\${$as_var+set}\" = set"; }; then
+  echo $ECHO_N "(cached) $ECHO_C" >&6
+else
+  cat >conftest.$ac_ext <<_ACEOF
+/* confdefs.h.  */
+_ACEOF
+cat confdefs.h >>conftest.$ac_ext
+cat >>conftest.$ac_ext <<_ACEOF
+/* end confdefs.h.  */
+$ac_includes_default
+
+#include <$ac_header>
+_ACEOF
+rm -f conftest.$ac_objext
+if { (ac_try="$ac_compile"
+case "(($ac_try" in
+  *\"* | *\`* | *\\*) ac_try_echo=\$ac_try;;
+  *) ac_try_echo=$ac_try;;
+esac
+eval "echo \"\$as_me:$LINENO: $ac_try_echo\"") >&5
+  (eval "$ac_compile") 2>conftest.er1
+  ac_status=$?
+  grep -v '^ *+' conftest.er1 >conftest.err
+  rm -f conftest.er1
+  cat conftest.err >&5
+  echo "$as_me:$LINENO: \$? = $ac_status" >&5
+  (exit $ac_status); } && {
+	 test -z "$ac_c_werror_flag" ||
+	 test ! -s conftest.err
+       } && test -s conftest.$ac_objext; then
+  eval "$as_ac_Header=yes"
+else
+  echo "$as_me: failed program was:" >&5
+sed 's/^/| /' conftest.$ac_ext >&5
+
+	eval "$as_ac_Header=no"
+fi
+
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+fi
+ac_res=`eval echo '${'$as_ac_Header'}'`
+	       { echo "$as_me:$LINENO: result: $ac_res" >&5
+echo "${ECHO_T}$ac_res" >&6; }
+if test `eval echo '${'$as_ac_Header'}'` = yes; then
+  cat >>confdefs.h <<_ACEOF
+#define `echo "HAVE_$ac_header" | $as_tr_cpp` 1
+_ACEOF
+
+fi
+
+done
+
+
 # check for thread library.
 
 # Check whether --with-pthreads was given.

configure.ac

@@ -483,6 +483,7 @@ AC_ARG_WITH(ssl, AC_HELP_STRING([--with-ssl=pathname],
     fi
 AC_CHECK_HEADERS([openssl/ssl.h],,, [AC_INCLUDES_DEFAULT])
 AC_CHECK_HEADERS([openssl/err.h],,, [AC_INCLUDES_DEFAULT])
+AC_CHECK_HEADERS([openssl/rand.h],,, [AC_INCLUDES_DEFAULT])
 
 # check for thread library.
 AC_ARG_WITH(pthreads, AC_HELP_STRING([--with-pthreads], 
@@ -719,6 +720,10 @@ AH_BOTTOM([
 #include <openssl/err.h>
 #endif
 
+#ifdef HAVE_OPENSSL_RAND_H
+#include <openssl/rand.h>
+#endif
+
 #ifdef HAVE_ATTR_FORMAT
 #  define ATTR_FORMAT(archetype, string_index, first_to_check) \
     __attribute__ ((format (archetype, string_index, first_to_check)))

daemon/daemon.c

@@ -375,5 +375,6 @@ daemon_delete(struct daemon* daemon)
 	CRYPTO_cleanup_all_ex_data(); /* safe, no more threads right now */
 	ERR_remove_state(0);
 	ERR_free_strings();
+	RAND_cleanup();
 	checklock_stop();
 }

daemon/worker.c

@@ -906,17 +906,10 @@ worker_init(struct worker* worker, struct config_file *cfg,
 	} else { /* !do_sigs */
 		worker->comsig = 0;
 	}
-	/* init random(), large table size. */
-	if(!(worker->rndstate = (struct ub_randstate*)calloc(1,
-		sizeof(struct ub_randstate)))) {
-		log_err("malloc rndtable failed.");
-		worker_delete(worker);
-		return 0;
-	}
 	seed = (unsigned int)time(NULL) ^ (unsigned int)getpid() ^
 		(((unsigned int)worker->thread_num)<<17);
 		/* shift thread_num so it does not match out pid bits */
-	if(!ub_initstate(seed, worker->rndstate, RND_STATE_SIZE)) {
+	if(!(worker->rndstate = ub_initstate(seed, NULL))) {
 		seed = 0;
 		log_err("could not init random numbers.");
 		worker_delete(worker);
@@ -1012,7 +1005,6 @@ worker_delete(struct worker* worker)
 	comm_point_delete(worker->cmd_com);
 	comm_base_delete(worker->base);
 	ub_randfree(worker->rndstate);
-	free(worker->rndstate);
 	/* close fds after deleting commpoints, to be sure.
 	   Also epoll does not like closing fd before event_del */
 	if(worker->cmd_send_fd != -1)

daemon/worker.h

@@ -59,9 +59,6 @@ struct listen_port;
 struct ub_randstate;
 struct regional;
 
-/** size of table used for random numbers. large to be more secure. */
-#define RND_STATE_SIZE 256
-
 /** worker commands */
 enum worker_commands {
 	/** make the worker quit */

doc/Changelog

@@ -6,6 +6,9 @@
 	- made openssl entropy warning more silent for library use. Needs
 	  verbosity 1 now.
 	- fixup forgotten locks for rbtree_searches on ctx->query tree.
+	- random generator cleanup - RND_STATE_SIZE removed, and instead
+	  a super-rnd can be passed at init to chain init random states.
+	- test also does lock checks if available.
 
 25 January 2008: Wouter
 	- added tpkg for asynclook and library use. 

libunbound/context.h

@@ -103,6 +103,8 @@ struct ub_val_ctx {
 	struct module_stack mods;
 	/** local authority zones */
 	struct local_zones* local_zones;
+	/** random state used to seed new random state structures */
+	struct ub_randstate* seed_rnd;
 
 	/** next query number (to try) to use */
 	int next_querynum;

libunbound/unbound.c

@@ -52,6 +52,7 @@
 #include "util/module.h"
 #include "util/regional.h"
 #include "util/log.h"
+#include "util/random.h"
 #include "util/net_help.h"
 #include "services/modstack.h"
 #include "services/localzone.h"
@@ -62,6 +63,7 @@ struct ub_val_ctx*
 ub_val_ctx_create()
 {
 	struct ub_val_ctx* ctx = (struct ub_val_ctx*)calloc(1, sizeof(*ctx));
+	unsigned int seed;
 	if(!ctx) {
 		errno = ENOMEM;
 		return NULL;
@@ -71,7 +73,17 @@ ub_val_ctx_create()
 	verbosity = 0; /* errors only */
 	log_init(NULL, 0, NULL); /* logs to stderr */
 	alloc_init(&ctx->superalloc, NULL, 0);
+	seed = (unsigned int)time(NULL) ^ (unsigned int)getpid();
+	if(!(ctx->seed_rnd = ub_initstate(seed, NULL))) {
+		seed = 0;
+		ub_randfree(ctx->seed_rnd);
+		free(ctx);
+		errno = ENOMEM;
+		return NULL;
+	}
+	seed = 0;
 	if(socketpair(AF_UNIX, SOCK_STREAM, 0, ctx->qqpipe) == -1) {
+		ub_randfree(ctx->seed_rnd);
 		free(ctx);
 		return NULL;
 	}
@@ -79,6 +91,7 @@ ub_val_ctx_create()
 		int e = errno;
 		close(ctx->qqpipe[0]);
 		close(ctx->qqpipe[1]);
+		ub_randfree(ctx->seed_rnd);
 		free(ctx);
 		errno = e;
 		return NULL;
@@ -92,6 +105,7 @@ ub_val_ctx_create()
 		close(ctx->rrpipe[1]);
 		close(ctx->qqpipe[0]);
 		close(ctx->qqpipe[1]);
+		ub_randfree(ctx->seed_rnd);
 		free(ctx);
 		errno = e;
 		return NULL;
@@ -105,6 +119,7 @@ ub_val_ctx_create()
 		close(ctx->rrpipe[1]);
 		close(ctx->qqpipe[0]);
 		close(ctx->qqpipe[1]);
+		ub_randfree(ctx->seed_rnd);
 		free(ctx);
 		errno = ENOMEM;
 		return NULL;
@@ -116,6 +131,7 @@ ub_val_ctx_create()
 		close(ctx->qqpipe[0]);
 		close(ctx->qqpipe[1]);
 		free(ctx->env);
+		ub_randfree(ctx->seed_rnd);
 		free(ctx);
 		errno = ENOMEM;
 		return NULL;
@@ -210,6 +226,7 @@ ub_val_ctx_delete(struct ub_val_ctx* ctx)
 		config_delete(ctx->env->cfg);
 		free(ctx->env);
 	}
+	ub_randfree(ctx->seed_rnd);
 	alloc_clear(&ctx->superalloc);
 	traverse_postorder(&ctx->queries, delq, NULL);
 	free(ctx);

libunbound/worker.c

@@ -61,9 +61,6 @@
 #include "util/data/msgreply.h"
 #include "util/data/msgencode.h"
 
-/** size of table used for random numbers. large to be more secure. */
-#define RND_STATE_SIZE 256
-
 /** handle new query command for bg worker */
 static void handle_newq(struct libworker* w, uint8_t* buf, uint32_t len);
 
@@ -79,7 +76,6 @@ libworker_delete(struct libworker* w)
 		ldns_buffer_free(w->env->scratch_buffer);
 		regional_destroy(w->env->scratch);
 		ub_randfree(w->env->rnd);
-		free(w->env->rnd);
 		free(w->env);
 	}
 	outside_network_delete(w->back);
@@ -118,21 +114,17 @@ libworker_setup(struct ub_val_ctx* ctx, int is_bg)
 		libworker_delete(w);
 		return NULL;
 	}
-	w->env->rnd = (struct ub_randstate*)calloc(1, sizeof(*w->env->rnd));
-	if(!w->env->rnd) {
-		libworker_delete(w);
-		return NULL;
-	}
 	w->env->worker = (struct worker*)w;
 	seed = (unsigned int)time(NULL) ^ (unsigned int)getpid() ^
 		(((unsigned int)w->thread_num)<<17);
 	seed ^= (unsigned int)w->env->alloc->next_id;
 	if(!w->is_bg || w->is_bg_thread) {
-		/* put lock around RAND*() */
 		lock_basic_lock(&ctx->cfglock);
 	}
-	if(!ub_initstate(seed, w->env->rnd, RND_STATE_SIZE)) {
+	if(!(w->env->rnd = ub_initstate(seed, ctx->seed_rnd))) {
-		lock_basic_unlock(&ctx->cfglock);
+		if(!w->is_bg || w->is_bg_thread) {
+			lock_basic_unlock(&ctx->cfglock);
+		}
 		seed = 0;
 		libworker_delete(w);
 		return NULL;

testcode/asynclook.c

@@ -147,6 +147,9 @@ struct ext_thr_info {
 	int numq;
 };
 
+/** if true, we are testing against 'localhost' and extra checking is done */
+static int q_is_localhost = 0;
+
 /** check result structure for the 'correct' answer */
 static void
 ext_check_result(const char* desc, int err, struct ub_val_result* result)
@@ -156,54 +159,57 @@ ext_check_result(const char* desc, int err, struct ub_val_result* result)
 		printf("%s: error result is NULL.\n", desc);
 		exit(1);
 	}
-	/* DEBUG */
+	if(q_is_localhost) {
-	if(strcmp(result->qname, "localhost") != 0) {
+		if(strcmp(result->qname, "localhost") != 0) {
-		printf("%s: error result has wrong qname.\n", desc);
+			printf("%s: error result has wrong qname.\n", desc);
-		exit(1);
+			exit(1);
-	}
+		}
-	if(result->qtype != LDNS_RR_TYPE_A) {
+		if(result->qtype != LDNS_RR_TYPE_A) {
-		printf("%s: error result has wrong qtype.\n", desc);
+			printf("%s: error result has wrong qtype.\n", desc);
-		exit(1);
+			exit(1);
-	}
+		}
-	if(result->qclass != LDNS_RR_CLASS_IN) {
+		if(result->qclass != LDNS_RR_CLASS_IN) {
-		printf("%s: error result has wrong qclass.\n", desc);
+			printf("%s: error result has wrong qclass.\n", desc);
-		exit(1);
+			exit(1);
-	}
+		}
-	if(result->data == NULL) {
+		if(result->data == NULL) {
-		printf("%s: error result->data is NULL.\n", desc);
+			printf("%s: error result->data is NULL.\n", desc);
-		exit(1);
+			exit(1);
-	}
+		}
-	if(result->len == NULL) {
+		if(result->len == NULL) {
-		printf("%s: error result->len is NULL.\n", desc);
+			printf("%s: error result->len is NULL.\n", desc);
-		exit(1);
+			exit(1);
-	}
+		}
-	if(result->rcode != 0) {
+		if(result->rcode != 0) {
-		printf("%s: error result->rcode is set.\n", desc);
+			printf("%s: error result->rcode is set.\n", desc);
-		exit(1);
+			exit(1);
-	}
+		}
-	if(result->havedata == 0) {
+		if(result->havedata == 0) {
-		printf("%s: error result->havedata is unset.\n", desc);
+			printf("%s: error result->havedata is unset.\n", desc);
-		exit(1);
+			exit(1);
-	}
+		}
-	if(result->nxdomain != 0) {
+		if(result->nxdomain != 0) {
-		printf("%s: error result->nxdomain is set.\n", desc);
+			printf("%s: error result->nxdomain is set.\n", desc);
-		exit(1);
+			exit(1);
-	}
+		}
-	if(result->secure || result->bogus) {
+		if(result->secure || result->bogus) {
-		printf("%s: error result->secure or bogus is set.\n", desc);
+			printf("%s: error result->secure or bogus is set.\n", 
-		exit(1);
+				desc);
-	}
+			exit(1);
-	if(result->data[0] == NULL) {
+		}
-		printf("%s: error result->data[0] is NULL.\n", desc);
+		if(result->data[0] == NULL) {
-		exit(1);
+			printf("%s: error result->data[0] is NULL.\n", desc);
-	}
+			exit(1);
-	if(result->len[0] != 4) {
+		}
-		printf("%s: error result->len[0] is wrong.\n", desc);
+		if(result->len[0] != 4) {
-		exit(1);
+			printf("%s: error result->len[0] is wrong.\n", desc);
-	}
+			exit(1);
-	if(result->len[1] != 0 || result->data[1] != NULL) {
+		}
-		printf("%s: error result->data[1] or len[1] is wrong.\n", desc);
+		if(result->len[1] != 0 || result->data[1] != NULL) {
-		exit(1);
+			printf("%s: error result->data[1] or len[1] is "
+				"wrong.\n", desc);
+			exit(1);
+		}
 	}
 }
 
@@ -291,6 +297,8 @@ ext_test(struct ub_val_ctx* ctx, int argc, char** argv)
 {
 	struct ext_thr_info inf[NUMTHR];
 	int i;
+	if(argc == 1 && strcmp(argv[0], "localhost") == 0)
+		q_is_localhost = 1;
 	printf("extended test start (%d threads)\n", NUMTHR);
 	for(i=0; i<NUMTHR; i++) {
 		/* 0 = this, 1 = library bg worker */

testcode/unitmain.c

@@ -363,12 +363,12 @@ infra_test()
 static void
 rnd_test()
 {
-	struct ub_randstate r;
+	struct ub_randstate* r;
 	int num = 100, i;
 	long int a[100];
-	unit_assert( ub_initstate((unsigned)time(NULL), &r, 256) );
+	unit_assert( (r = ub_initstate((unsigned)time(NULL), NULL)) );
 	for(i=0; i<num; i++) {
-		a[i] = ub_random(&r);
+		a[i] = ub_random(r);
 		unit_assert(a[i] >= 0);
 		unit_assert((size_t)a[i] <= (size_t)RAND_MAX);
 		if(i > 5)
@@ -376,7 +376,7 @@ rnd_test()
 				a[i] != a[i-3] || a[i] != a[i-4] ||
 				a[i] != a[i-5] || a[i] != a[i-6]);
 	}
-	ub_randfree(&r);
+	ub_randfree(r);
 }
 
 /**

util/random.c

@@ -53,7 +53,7 @@
  * Struct with per-thread random state.
  * Keeps SSL types away from the header file.
  */
-struct ub_hiddenstate {
+struct ub_randstate {
 	/** key used for arc4random generation */
 	RC4_KEY rc4;
 	/** keeps track of key usage */
@@ -68,17 +68,22 @@ struct ub_hiddenstate {
 
 /** reseed random generator */
 static void
-ub_arc4random_stir(struct ub_hiddenstate* s)
+ub_arc4random_stir(struct ub_randstate* s, struct ub_randstate* from)
 {
 	unsigned char rand_buf[SEED_SIZE];
 	int i;
 
 	memset(&s->rc4, 0, sizeof(s->rc4));
 	memset(rand_buf, 0xc, sizeof(rand_buf));
-	if (RAND_bytes(rand_buf, (int)sizeof(rand_buf)) <= 0)
+	if (from) {
-		fatal_exit("Couldn't obtain random bytes (error %ld)",
+		for(i=0; i<SEED_SIZE; i++)
-			    ERR_get_error());
+			rand_buf[i] = (unsigned char)ub_random(from);
-	RC4_set_key(&s->rc4, (int)sizeof(rand_buf), rand_buf);
+	} else {
+		if (RAND_bytes(rand_buf, (int)sizeof(rand_buf)) <= 0)
+			fatal_exit("Couldn't obtain random bytes (error %ld)",
+				    ERR_get_error());
+	}
+	RC4_set_key(&s->rc4, SEED_SIZE, rand_buf);
 
 	/*
 	 * Discard early keystream, as per recommendations in:
@@ -92,14 +97,13 @@ ub_arc4random_stir(struct ub_hiddenstate* s)
 	s->rc4_ready = REKEY_BYTES;
 }
 
-int 
+struct ub_randstate* 
-ub_initstate(unsigned int seed, struct ub_randstate* state, 
+ub_initstate(unsigned int seed, struct ub_randstate* from)
-	unsigned long ATTR_UNUSED(n))
 {
-	state->s = calloc(1, sizeof(*state->s));
+	struct ub_randstate* s = (struct ub_randstate*)calloc(1, sizeof(*s));
-	if(!state->s) {
+	if(!s) {
 		log_err("malloc failure in random init");
-		return 0;
+		return NULL;
 	}
 
 	/* RAND_ is threadsafe, by the way */
@@ -112,37 +116,36 @@ ub_initstate(unsigned int seed, struct ub_randstate* state,
 			memmove(buf+i*sizeof(seed), &v, sizeof(seed));
 			v = v*seed + (unsigned int)i;
 		}
-		log_hex("seed with", buf, 256);
 		RAND_seed(buf, 256);
 		if(!RAND_status()) {
 			log_err("Random generator has no entropy (error %ld)",
 				ERR_get_error());
-			return 0;
+			return NULL;
 		}
 		verbose(VERB_OPS, "openssl has no entropy, seeding with time");
 	}
-	ub_arc4random_stir(state->s);
+	ub_arc4random_stir(s, from);
-	return 1;
+	return s;
 }
 
 long int 
-ub_random(struct ub_randstate* state)
+ub_random(struct ub_randstate* s)
 {
 	unsigned int r = 0;
-	if (state->s->rc4_ready <= 0) {
+	if (s->rc4_ready <= 0) {
-		ub_arc4random_stir(state->s);
+		ub_arc4random_stir(s, NULL);
 	}
 
-	RC4(&state->s->rc4, sizeof(r), 
+	RC4(&s->rc4, sizeof(r), 
 		(unsigned char *)&r, (unsigned char *)&r);
-	state->s->rc4_ready -= sizeof(r);
+	s->rc4_ready -= sizeof(r);
 	return (long int)((r) % (((unsigned)RAND_MAX + 1)));
 }
 
 void 
-ub_randfree(struct ub_randstate* state)
+ub_randfree(struct ub_randstate* s)
 {
-	if(state)
+	if(s)
-		free(state->s);
+		free(s);
-	RAND_cleanup();
+	/* user app must do RAND_cleanup(); */
 }

util/random.h

@@ -42,27 +42,22 @@
  * initialisation routine.
  */
 
-struct ub_hiddenstate;
-
 /**
  * random state structure.
  */
-struct ub_randstate {
+struct ub_randstate;
-	/** state hidden type. */
-	struct ub_hiddenstate* s;
-};
 
 /**
  * Initialize a random generator state for use 
  * @param seed: seed value to create state contents.
  *	(ignored for arc4random).
- * @param state: struct allocated by caller.
+ * @param from: if not NULL, the seed is taken from this random structure.
- * @param n: size of state->state. 8, 32, 64, 128, or 256 bytes.
+ * 	can be used to seed random states via a parent-random-state that
- *	(ignored for arc4random).
+ * 	is itself seeded with entropy.
- * @return false alloc failure.
+ * @return new state or NULL alloc failure.
  */
-int ub_initstate(unsigned int seed, struct ub_randstate* state, 
+struct ub_randstate* ub_initstate(unsigned int seed, 
-	unsigned long n);
+	struct ub_randstate* from);
 
 /**
  * Generate next random number from the state passed along.