From 7e1df5182c989e56927fe304d15e28ed06a5240d Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Mon, 6 Nov 2017 10:44:37 -0800 Subject: [PATCH] [master] isc_rng_randombytes() 4807. [cleanup] isc_rng_randombytes() returns a specified number of bytes from the PRNG; this is now used instead of calling isc_rng_random() multiple times. [RT #46230] --- CHANGES | 4 ++ bin/named/controlconf.c | 5 +- bin/named/server.c | 14 +---- lib/dns/dispatch.c | 2 +- lib/isc/include/isc/random.h | 12 +++- lib/isc/random.c | 77 +++++++++++++++++--------- lib/isc/tests/random_test.c | 103 +++++++++++++++++++++++++++-------- lib/isc/win32/libisc.def.in | 1 + lib/ns/client.c | 4 +- 9 files changed, 155 insertions(+), 67 deletions(-) diff --git a/CHANGES b/CHANGES index 1a03576f03..0daae05930 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +4807. [cleanup] isc_rng_randombytes() returns a specified number of + bytes from the PRNG; this is now used instead of + calling isc_rng_random() multiple times. [RT #46230] + --- 9.12.0b2 released --- 4806. [func] Log messages related to loading of zones are now diff --git a/bin/named/controlconf.c b/bin/named/controlconf.c index adc0585818..040a62dd7d 100644 --- a/bin/named/controlconf.c +++ b/bin/named/controlconf.c @@ -456,9 +456,8 @@ control_recvmessage(isc_task_t *task, isc_event_t *event) { */ if (conn->nonce == 0) { while (conn->nonce == 0) { - isc_uint16_t r1 = isc_rng_random(server->sctx->rngctx); - isc_uint16_t r2 = isc_rng_random(server->sctx->rngctx); - conn->nonce = (r1 << 16) | r2; + isc_rng_randombytes(server->sctx->rngctx, &conn->nonce, + sizeof(conn->nonce)); } eresult = ISC_R_SUCCESS; } else diff --git a/bin/named/server.c b/bin/named/server.c index 83b9e7a412..54d86f6bbb 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -13484,11 +13484,6 @@ newzone_cfgctx_destroy(void **cfgp) { static isc_result_t generate_salt(unsigned char *salt, size_t saltlen) { - size_t i, n; - union { - unsigned char rnd[256]; - isc_uint16_t rnd16[128]; - } rnd; unsigned char text[512 + 1]; isc_region_t r; isc_buffer_t buf; @@ -13497,14 +13492,9 @@ generate_salt(unsigned char *salt, size_t saltlen) { if (saltlen > 256U) return (ISC_R_RANGE); - n = (saltlen + sizeof(isc_uint16_t) - 1) / sizeof(isc_uint16_t); - for (i = 0; i < n; i++) { - rnd.rnd16[i] = isc_rng_random(named_g_server->sctx->rngctx); - } + isc_rng_randombytes(named_g_server->sctx->rngctx, salt, saltlen); - memmove(salt, rnd.rnd, saltlen); - - r.base = rnd.rnd; + r.base = salt; r.length = (unsigned int) saltlen; isc_buffer_init(&buf, text, sizeof(text)); diff --git a/lib/dns/dispatch.c b/lib/dns/dispatch.c index 607a730395..da40fa6263 100644 --- a/lib/dns/dispatch.c +++ b/lib/dns/dispatch.c @@ -3281,7 +3281,7 @@ dns_dispatch_addresponse3(dns_dispatch_t *disp, unsigned int options, if ((options & DNS_DISPATCHOPT_FIXEDID) != 0) id = *idp; else - id = (dns_messageid_t)isc_rng_random(DISP_RNGCTX(disp)); + isc_rng_randombytes(DISP_RNGCTX(disp), &id, sizeof(id)); ok = ISC_FALSE; i = 0; do { diff --git a/lib/isc/include/isc/random.h b/lib/isc/include/isc/random.h index 4020a0cad9..c60e56d203 100644 --- a/lib/isc/include/isc/random.h +++ b/lib/isc/include/isc/random.h @@ -14,6 +14,7 @@ #include #include #include +#include /*! \file isc/random.h * \brief Implements pseudo random number generators. @@ -111,10 +112,19 @@ isc_rng_detach(isc_rng_t **rngp); * \li rngp != NULL the RNG struct to decrement reference for */ +void +isc_rng_randombytes(isc_rng_t *rngctx, void *output, size_t length); +/*%< + * Returns a pseudo random sequence of length octets in output. + */ + isc_uint16_t -isc_rng_random(isc_rng_t *rngctx); +isc_rng_random(isc_rng_t *rngctx) ISC_DEPRECATED; /*%< * Returns a pseudo random 16-bit unsigned integer. + * + * This function is deprecated. You should use `isc_rng_randombytes()` + * instead. */ isc_uint16_t diff --git a/lib/isc/random.c b/lib/isc/random.c index dcc386dd95..91441b22ff 100644 --- a/lib/isc/random.c +++ b/lib/isc/random.c @@ -60,6 +60,13 @@ #define CHACHA_IVSIZE 8U #define CHACHA_BLOCKSIZE 64 #define CHACHA_BUFFERSIZE (16 * CHACHA_BLOCKSIZE) +#define CHACHA_MAXHAVE (CHACHA_BUFFERSIZE - CHACHA_KEYSIZE - CHACHA_IVSIZE) +/* + * Derived from OpenBSD's implementation. The rationale is not clear, + * but should be conservative enough in safety, and reasonably large for + * efficiency. + */ +#define CHACHA_MAXLENGTH 1600000 /* ChaCha RNG state */ struct isc_rng { @@ -296,26 +303,29 @@ chacha_rekey(isc_rng_t *rng, u_char *dat, size_t datlen) { chacha_reinit(rng, rng->buffer, CHACHA_KEYSIZE + CHACHA_IVSIZE); memset(rng->buffer, 0, CHACHA_KEYSIZE + CHACHA_IVSIZE); - rng->have = CHACHA_BUFFERSIZE - CHACHA_KEYSIZE - CHACHA_IVSIZE; + rng->have = CHACHA_MAXHAVE; } -static inline isc_uint16_t -chacha_getuint16(isc_rng_t *rng) { - isc_uint16_t val; - +static void +chacha_getbytes(isc_rng_t *rng, isc_uint8_t *output, size_t length) { REQUIRE(VALID_RNG(rng)); - if (rng->have < sizeof(val)) + while (ISC_UNLIKELY(length > CHACHA_MAXHAVE)) { + chacha_rekey(rng, NULL, 0); + memmove(output, rng->buffer + CHACHA_BUFFERSIZE - rng->have, + CHACHA_MAXHAVE); + output += CHACHA_MAXHAVE; + length -= CHACHA_MAXHAVE; + rng->have = 0; + } + + if (rng->have < length) chacha_rekey(rng, NULL, 0); - memmove(&val, rng->buffer + CHACHA_BUFFERSIZE - rng->have, - sizeof(val)); + memmove(output, rng->buffer + CHACHA_BUFFERSIZE - rng->have, length); /* Clear the copied region. */ - memset(rng->buffer + CHACHA_BUFFERSIZE - rng->have, - 0, sizeof(val)); - rng->have -= sizeof(val); - - return (val); + memset(rng->buffer + CHACHA_BUFFERSIZE - rng->have, 0, length); + rng->have -= length; } static void @@ -354,23 +364,40 @@ chacha_stir(isc_rng_t *rng) { * but should be conservative enough in safety, and reasonably large * for efficiency. */ - rng->count = 1600000; + rng->count = CHACHA_MAXLENGTH; +} + +void +isc_rng_randombytes(isc_rng_t *rng, void *output, size_t length) { + isc_uint8_t *ptr = output; + + REQUIRE(VALID_RNG(rng)); + REQUIRE(output != NULL && length > 0); + + LOCK(&rng->lock); + + while (ISC_UNLIKELY(length > CHACHA_MAXLENGTH)) { + chacha_stir(rng); + chacha_getbytes(rng, ptr, CHACHA_MAXLENGTH); + ptr += CHACHA_MAXLENGTH; + length -= CHACHA_MAXLENGTH; + rng->count = 0; + } + + rng->count -= length; + if (rng->count <= 0) + chacha_stir(rng); + + chacha_getbytes(rng, ptr, length); + + UNLOCK(&rng->lock); } isc_uint16_t isc_rng_random(isc_rng_t *rng) { isc_uint16_t result; - REQUIRE(VALID_RNG(rng)); - - LOCK(&rng->lock); - - rng->count -= sizeof(isc_uint16_t); - if (rng->count <= 0) - chacha_stir(rng); - result = chacha_getuint16(rng); - - UNLOCK(&rng->lock); + isc_rng_randombytes(rng, &result, sizeof(result)); return (result); } @@ -401,7 +428,7 @@ isc_rng_uniformrandom(isc_rng_t *rng, isc_uint16_t upper_bound) { * to re-roll. */ for (;;) { - r = isc_rng_random(rng); + isc_rng_randombytes(rng, &r, sizeof(r)); if (r >= min) break; } diff --git a/lib/isc/tests/random_test.c b/lib/isc/tests/random_test.c index fe9c7b8863..81782fad0d 100644 --- a/lib/isc/tests/random_test.c +++ b/lib/isc/tests/random_test.c @@ -238,7 +238,7 @@ matrix_binaryrank(isc_uint32_t *bits, ssize_t rows, ssize_t cols) { } static void -random_test(pvalue_func_t *func) { +random_test(pvalue_func_t *func, isc_boolean_t word_sized) { isc_mem_t *mctx = NULL; isc_result_t result; isc_rng_t *rng; @@ -272,8 +272,13 @@ random_test(pvalue_func_t *func) { isc_uint16_t values[128000]; double p_value; - for (i = 0; i < 128000; i++) - values[i] = isc_rng_random(rng); + if (word_sized) { + for (i = 0; i < 128000; i++) + isc_rng_randombytes(rng, &values[i], + sizeof(values[i])); + } else { + isc_rng_randombytes(rng, values, sizeof(values)); + } p_value = (*func)(mctx, values, 128000); if (p_value >= 0.01) @@ -596,41 +601,41 @@ binarymatrixrank(isc_mem_t *mctx, isc_uint16_t *values, size_t length) { return (p_value); } -ATF_TC(isc_rng_monobit); -ATF_TC_HEAD(isc_rng_monobit, tc) { +ATF_TC(isc_rng_monobit_16); +ATF_TC_HEAD(isc_rng_monobit_16, tc) { atf_tc_set_md_var(tc, "descr", "Monobit test for the RNG"); } -ATF_TC_BODY(isc_rng_monobit, tc) { +ATF_TC_BODY(isc_rng_monobit_16, tc) { UNUSED(tc); - random_test(monobit); + random_test(monobit, ISC_TRUE); } -ATF_TC(isc_rng_runs); -ATF_TC_HEAD(isc_rng_runs, tc) { +ATF_TC(isc_rng_runs_16); +ATF_TC_HEAD(isc_rng_runs_16, tc) { atf_tc_set_md_var(tc, "descr", "Runs test for the RNG"); } -ATF_TC_BODY(isc_rng_runs, tc) { +ATF_TC_BODY(isc_rng_runs_16, tc) { UNUSED(tc); - random_test(runs); + random_test(runs, ISC_TRUE); } -ATF_TC(isc_rng_blockfrequency); -ATF_TC_HEAD(isc_rng_blockfrequency, tc) { +ATF_TC(isc_rng_blockfrequency_16); +ATF_TC_HEAD(isc_rng_blockfrequency_16, tc) { atf_tc_set_md_var(tc, "descr", "Block frequency test for the RNG"); } -ATF_TC_BODY(isc_rng_blockfrequency, tc) { +ATF_TC_BODY(isc_rng_blockfrequency_16, tc) { UNUSED(tc); - random_test(blockfrequency); + random_test(blockfrequency, ISC_TRUE); } -ATF_TC(isc_rng_binarymatrixrank); -ATF_TC_HEAD(isc_rng_binarymatrixrank, tc) { +ATF_TC(isc_rng_binarymatrixrank_16); +ATF_TC_HEAD(isc_rng_binarymatrixrank_16, tc) { atf_tc_set_md_var(tc, "descr", "Binary matrix rank test for the RNG"); } @@ -638,20 +643,72 @@ ATF_TC_HEAD(isc_rng_binarymatrixrank, tc) { * This is the binary matrix rank test taken from the NIST SP 800-22 RNG * test suite. */ -ATF_TC_BODY(isc_rng_binarymatrixrank, tc) { +ATF_TC_BODY(isc_rng_binarymatrixrank_16, tc) { UNUSED(tc); - random_test(binarymatrixrank); + random_test(binarymatrixrank, ISC_TRUE); +} + +ATF_TC(isc_rng_monobit_bytes); +ATF_TC_HEAD(isc_rng_monobit_bytes, tc) { + atf_tc_set_md_var(tc, "descr", "Monobit test for the RNG"); +} + +ATF_TC_BODY(isc_rng_monobit_bytes, tc) { + UNUSED(tc); + + random_test(monobit, ISC_FALSE); +} + +ATF_TC(isc_rng_runs_bytes); +ATF_TC_HEAD(isc_rng_runs_bytes, tc) { + atf_tc_set_md_var(tc, "descr", "Runs test for the RNG"); +} + +ATF_TC_BODY(isc_rng_runs_bytes, tc) { + UNUSED(tc); + + random_test(runs, ISC_FALSE); +} + +ATF_TC(isc_rng_blockfrequency_bytes); +ATF_TC_HEAD(isc_rng_blockfrequency_bytes, tc) { + atf_tc_set_md_var(tc, "descr", "Block frequency test for the RNG"); +} + +ATF_TC_BODY(isc_rng_blockfrequency_bytes, tc) { + UNUSED(tc); + + random_test(blockfrequency, ISC_FALSE); +} + +ATF_TC(isc_rng_binarymatrixrank_bytes); +ATF_TC_HEAD(isc_rng_binarymatrixrank_bytes, tc) { + atf_tc_set_md_var(tc, "descr", "Binary matrix rank test for the RNG"); +} + +/* + * This is the binary matrix rank test taken from the NIST SP 800-22 RNG + * test suite. + */ +ATF_TC_BODY(isc_rng_binarymatrixrank_bytes, tc) { + UNUSED(tc); + + random_test(binarymatrixrank, ISC_FALSE); } /* * Main */ ATF_TP_ADD_TCS(tp) { - ATF_TP_ADD_TC(tp, isc_rng_monobit); - ATF_TP_ADD_TC(tp, isc_rng_runs); - ATF_TP_ADD_TC(tp, isc_rng_blockfrequency); - ATF_TP_ADD_TC(tp, isc_rng_binarymatrixrank); + ATF_TP_ADD_TC(tp, isc_rng_monobit_16); + ATF_TP_ADD_TC(tp, isc_rng_runs_16); + ATF_TP_ADD_TC(tp, isc_rng_blockfrequency_16); + ATF_TP_ADD_TC(tp, isc_rng_binarymatrixrank_16); + ATF_TP_ADD_TC(tp, isc_rng_monobit_bytes); + ATF_TP_ADD_TC(tp, isc_rng_runs_bytes); + ATF_TP_ADD_TC(tp, isc_rng_blockfrequency_bytes); + ATF_TP_ADD_TC(tp, isc_rng_binarymatrixrank_bytes); return (atf_no_error()); } diff --git a/lib/isc/win32/libisc.def.in b/lib/isc/win32/libisc.def.in index 83ab3854c2..cf61fc0f7d 100644 --- a/lib/isc/win32/libisc.def.in +++ b/lib/isc/win32/libisc.def.in @@ -555,6 +555,7 @@ isc_rng_attach isc_rng_create isc_rng_detach isc_rng_random +isc_rng_randombytes isc_rng_uniformrandom isc_rwlock_destroy isc_rwlock_downgrade diff --git a/lib/ns/client.c b/lib/ns/client.c index 54db1d93ed..26e37a06d5 100644 --- a/lib/ns/client.c +++ b/lib/ns/client.c @@ -1648,8 +1648,8 @@ ns_client_addopt(ns_client_t *client, dns_message_t *message, isc_buffer_init(&buf, cookie, sizeof(cookie)); isc_stdtime_get(&now); - nonce = ((isc_rng_random(client->sctx->rngctx) << 16) | - isc_rng_random(client->sctx->rngctx)); + isc_rng_randombytes(client->sctx->rngctx, + &nonce, sizeof(nonce)); compute_cookie(client, now, nonce, client->sctx->secret, &buf);