From: Wouter Wijngaards Date: Mon, 28 Jan 2008 14:34:53 +0000 (+0000) Subject: fixes for random number badness (lack of entropy and SIGFPE from RAND_cleanup X-Git-Tag: release-0.9~33 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c8f2bf0333335fc5a0a70f11c369a34fa2e42b66;p=thirdparty%2Funbound.git 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 --- diff --git a/config.h.in b/config.h.in index 1659a0fcc..19988afa8 100644 --- a/config.h.in +++ b/config.h.in @@ -76,6 +76,9 @@ /* Define to 1 if you have the header file. */ #undef HAVE_OPENSSL_ERR_H +/* Define to 1 if you have the header file. */ +#undef HAVE_OPENSSL_RAND_H + /* Define to 1 if you have the header file. */ #undef HAVE_OPENSSL_SSL_H @@ -332,6 +335,10 @@ #include #endif +#ifdef HAVE_OPENSSL_RAND_H +#include +#endif + #ifdef HAVE_ATTR_FORMAT # define ATTR_FORMAT(archetype, string_index, first_to_check) \ __attribute__ ((format (archetype, string_index, first_to_check))) diff --git a/configure b/configure index 7d404c359..4df0f2f29 100755 --- a/configure +++ b/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. diff --git a/configure.ac b/configure.ac index 3d8db09b6..bdc0df5a7 100644 --- a/configure.ac +++ b/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 #endif +#ifdef HAVE_OPENSSL_RAND_H +#include +#endif + #ifdef HAVE_ATTR_FORMAT # define ATTR_FORMAT(archetype, string_index, first_to_check) \ __attribute__ ((format (archetype, string_index, first_to_check))) diff --git a/daemon/daemon.c b/daemon/daemon.c index 12785135a..612d073c9 100644 --- a/daemon/daemon.c +++ b/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(); } diff --git a/daemon/worker.c b/daemon/worker.c index fc9407bac..7d4c41fa7 100644 --- a/daemon/worker.c +++ b/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) diff --git a/daemon/worker.h b/daemon/worker.h index 2fbdb9e5d..1b30c3391 100644 --- a/daemon/worker.h +++ b/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 */ diff --git a/doc/Changelog b/doc/Changelog index 9bc255a30..69558ddf1 100644 --- a/doc/Changelog +++ b/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. diff --git a/libunbound/context.h b/libunbound/context.h index 80b10d967..b44917001 100644 --- a/libunbound/context.h +++ b/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; diff --git a/libunbound/unbound.c b/libunbound/unbound.c index 3f7e0b189..9fd8326fe 100644 --- a/libunbound/unbound.c +++ b/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); diff --git a/libunbound/worker.c b/libunbound/worker.c index 9dd98b45e..009e08b8f 100644 --- a/libunbound/worker.c +++ b/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)) { - lock_basic_unlock(&ctx->cfglock); + if(!(w->env->rnd = ub_initstate(seed, ctx->seed_rnd))) { + if(!w->is_bg || w->is_bg_thread) { + lock_basic_unlock(&ctx->cfglock); + } seed = 0; libworker_delete(w); return NULL; diff --git a/testcode/asynclook.c b/testcode/asynclook.c index 655c7f0d1..170a1db08 100644 --- a/testcode/asynclook.c +++ b/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(strcmp(result->qname, "localhost") != 0) { - printf("%s: error result has wrong qname.\n", desc); - exit(1); - } - if(result->qtype != LDNS_RR_TYPE_A) { - printf("%s: error result has wrong qtype.\n", desc); - exit(1); - } - if(result->qclass != LDNS_RR_CLASS_IN) { - printf("%s: error result has wrong qclass.\n", desc); - exit(1); - } - if(result->data == NULL) { - printf("%s: error result->data is NULL.\n", desc); - exit(1); - } - if(result->len == NULL) { - printf("%s: error result->len is NULL.\n", desc); - exit(1); - } - if(result->rcode != 0) { - printf("%s: error result->rcode is set.\n", desc); - exit(1); - } - if(result->havedata == 0) { - printf("%s: error result->havedata is unset.\n", desc); - exit(1); - } - if(result->nxdomain != 0) { - printf("%s: error result->nxdomain is set.\n", desc); - exit(1); - } - if(result->secure || result->bogus) { - printf("%s: error result->secure or bogus is set.\n", desc); - exit(1); - } - if(result->data[0] == NULL) { - 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); - exit(1); - } - if(result->len[1] != 0 || result->data[1] != NULL) { - printf("%s: error result->data[1] or len[1] is wrong.\n", desc); - exit(1); + if(q_is_localhost) { + if(strcmp(result->qname, "localhost") != 0) { + printf("%s: error result has wrong qname.\n", desc); + exit(1); + } + if(result->qtype != LDNS_RR_TYPE_A) { + printf("%s: error result has wrong qtype.\n", desc); + exit(1); + } + if(result->qclass != LDNS_RR_CLASS_IN) { + printf("%s: error result has wrong qclass.\n", desc); + exit(1); + } + if(result->data == NULL) { + printf("%s: error result->data is NULL.\n", desc); + exit(1); + } + if(result->len == NULL) { + printf("%s: error result->len is NULL.\n", desc); + exit(1); + } + if(result->rcode != 0) { + printf("%s: error result->rcode is set.\n", desc); + exit(1); + } + if(result->havedata == 0) { + printf("%s: error result->havedata is unset.\n", desc); + exit(1); + } + if(result->nxdomain != 0) { + printf("%s: error result->nxdomain is set.\n", desc); + exit(1); + } + if(result->secure || result->bogus) { + printf("%s: error result->secure or bogus is set.\n", + desc); + exit(1); + } + if(result->data[0] == NULL) { + 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); + exit(1); + } + if(result->len[1] != 0 || result->data[1] != NULL) { + 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= 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); } /** diff --git a/testdata/05-asynclook.tpkg b/testdata/05-asynclook.tpkg index e1d7d555c..df7e08888 100644 Binary files a/testdata/05-asynclook.tpkg and b/testdata/05-asynclook.tpkg differ diff --git a/util/random.c b/util/random.c index d91e6e3a5..39fdd731e 100644 --- a/util/random.c +++ b/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) - fatal_exit("Couldn't obtain random bytes (error %ld)", - ERR_get_error()); - RC4_set_key(&s->rc4, (int)sizeof(rand_buf), rand_buf); + if (from) { + for(i=0; irc4, 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 -ub_initstate(unsigned int seed, struct ub_randstate* state, - unsigned long ATTR_UNUSED(n)) +struct ub_randstate* +ub_initstate(unsigned int seed, struct ub_randstate* from) { - state->s = calloc(1, sizeof(*state->s)); - if(!state->s) { + struct ub_randstate* s = (struct ub_randstate*)calloc(1, sizeof(*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); - return 1; + ub_arc4random_stir(s, from); + 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) { - ub_arc4random_stir(state->s); + if (s->rc4_ready <= 0) { + 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) - free(state->s); - RAND_cleanup(); + if(s) + free(s); + /* user app must do RAND_cleanup(); */ } diff --git a/util/random.h b/util/random.h index 98b8883d0..f5e3cce1d 100644 --- a/util/random.h +++ b/util/random.h @@ -42,27 +42,22 @@ * initialisation routine. */ -struct ub_hiddenstate; - /** * random state structure. */ -struct ub_randstate { - /** state hidden type. */ - struct ub_hiddenstate* s; -}; +struct ub_randstate; /** * 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 n: size of state->state. 8, 32, 64, 128, or 256 bytes. - * (ignored for arc4random). - * @return false alloc failure. + * @param from: if not NULL, the seed is taken from this random structure. + * can be used to seed random states via a parent-random-state that + * is itself seeded with entropy. + * @return new state or NULL alloc failure. */ -int ub_initstate(unsigned int seed, struct ub_randstate* state, - unsigned long n); +struct ub_randstate* ub_initstate(unsigned int seed, + struct ub_randstate* from); /** * Generate next random number from the state passed along.