From 5bf902b187899ebb603849590d06161053988f97 Mon Sep 17 00:00:00 2001 From: wiire-a Date: Thu, 30 Nov 2017 15:48:46 +0100 Subject: [PATCH] Fixed UB in the new PRNG due to int overflow --- src/random/glibc_random.c | 40 +++++++++++++++++++--------------- src/random/glibc_random_lazy.c | 39 ++++++++++++++++++++------------- 2 files changed, 46 insertions(+), 33 deletions(-) diff --git a/src/random/glibc_random.c b/src/random/glibc_random.c index 46c99d0..739860c 100644 --- a/src/random/glibc_random.c +++ b/src/random/glibc_random.c @@ -3,14 +3,18 @@ * Reference: http://www.mathstat.dal.ca/~selinger/random/ * * The original code was modified to achieve better speed + * + * Note that in the original code two signed integers are added together + * which results in undefined behavior if the sum overflows the content + * of a signed integer while trying to preserve the sign. + * + * To avoid this, we exploit the 2's complement, thus using only + * unsigned integers. Note that INT_MAX + INT_MAX <= UINT_MAX and that + * adding two unsigned integers which sum exceeds UINT_MAX is not + * undefined behavior, it causes the value to wrap around. */ -#define GLIBC_PRNG_SIZE 344 -#define GLIBC_FAST_MAX_GEN 3 - -#if GLIBC_MAX_GEN < GLIBC_FAST_MAX_GEN - #error "GLIBC_MAX_GEN must be >= GLIBC_FAST_MAX_GEN" -#endif +#include /* * The +1 is used to keep the index inside the array after the increment, @@ -18,39 +22,39 @@ */ struct glibc_prng { int index; - int state[GLIBC_PRNG_SIZE + GLIBC_MAX_GEN - GLIBC_FAST_MAX_GEN + 1]; + uint32_t state[344 + GLIBC_MAX_GEN + 1]; }; /* * If only 3 numbers are generated then there's no need to store new values */ -static unsigned int glibc_rand_fast(struct glibc_prng *prng) +static uint32_t glibc_rand_fast(struct glibc_prng *prng) { - const int *state = prng->state; + const uint32_t *state = prng->state; const int i = prng->index++; - return ((unsigned int)(state[i - 31] + state[i - 3])) >> 1; + return (state[i - 31] + state[i - 3]) >> 1; } /* * There are no checks of bounds (GLIBC_MAX_GEN is the maximum number of times it can be called) */ -static unsigned int glibc_rand(struct glibc_prng *prng) +static uint32_t glibc_rand(struct glibc_prng *prng) { - int *state = prng->state; + uint32_t *state = prng->state; const int i = prng->index++; - state[i] = state[i - 31] + state[i - 3]; - return (unsigned int)state[i] >> 1; + state[i] = (uint32_t)(state[i - 31] + state[i - 3]); + return state[i] >> 1; } -static void glibc_seed(struct glibc_prng *prng, int seed) +static void glibc_seed(struct glibc_prng *prng, uint32_t seed) { int i = 0; - int *state = prng->state; - prng->index = GLIBC_PRNG_SIZE; + uint32_t *state = prng->state; + prng->index = 344; state[i++] = seed; for ( ; i < 31; i++) { state[i] = (16807LL * state[i - 1]) % 2147483647; - if (state[i] < 0) + if (state[i] & 0x80000000) /* < 0 */ state[i] += 2147483647; } for (i = 31; i < 34; i++) state[i] = state[i - 31]; diff --git a/src/random/glibc_random_lazy.c b/src/random/glibc_random_lazy.c index 45e9150..055ff1a 100644 --- a/src/random/glibc_random_lazy.c +++ b/src/random/glibc_random_lazy.c @@ -3,48 +3,57 @@ * Reference: http://www.mathstat.dal.ca/~selinger/random/ * * The original code was modified to achieve better speed + * + * Note that in the original code two signed integers are added together + * which results in undefined behavior if the sum overflows the content + * of a signed integer while trying to preserve the sign. + * + * To avoid this, we exploit the 2's complement, thus using only + * unsigned integers. Note that INT_MAX + INT_MAX <= UINT_MAX and that + * adding two unsigned integers which sum exceeds UINT_MAX is not + * undefined behavior, it causes the value to wrap around. */ #include struct glibc_lazyprng { - int state[344]; + uint32_t state[344]; }; /* * Return 1st generated element only */ -static unsigned int glibc_rand1(struct glibc_lazyprng *prng) +static uint32_t glibc_rand1(struct glibc_lazyprng *prng) { - const int *state = prng->state; - return ((unsigned int)(state[344 - 31] + state[344 - 3])) >> 1; + const uint32_t *state = prng->state; + return (state[344 - 31] + state[344 - 3]) >> 1; } /* * Fill a 4 elements array (to use with memcmp) */ -static int *glibc_randfill(struct glibc_lazyprng *prng, uint32_t *arr) +static uint32_t *glibc_randfill(struct glibc_lazyprng *prng, uint32_t *arr) { - int *state = prng->state; - int const first = state[344 - 31] + state[344 - 3]; - arr[0] = ((unsigned int)first) >> 1; - arr[1] = ((unsigned int)(state[344 - 31 + 1] + state[342 - 31] + state[342 - 3])) >> 1; - arr[2] = ((unsigned int)(state[344 - 31 + 2] + state[343 - 31] + state[343 - 3])) >> 1; - arr[3] = ((unsigned int)(state[344 - 31 + 3] + first)) >> 1; + uint32_t *state = prng->state; + const uint32_t first = state[344 - 31] + state[344 - 3]; + arr[0] = first >> 1; + arr[1] = (state[344 - 31 + 1] + state[342 - 31] + state[342 - 3]) >> 1; + arr[2] = (state[344 - 31 + 2] + state[343 - 31] + state[343 - 3]) >> 1; + arr[3] = (state[344 - 31 + 3] + first) >> 1; return arr; } /* * Lazy seeding (stay 2 shorter) */ -static void glibc_lazyseed(struct glibc_lazyprng *prng, int seed) +static void glibc_lazyseed(struct glibc_lazyprng *prng, uint32_t seed) { - int *state = prng->state; - int i = 0; + uint32_t *state = prng->state; + uint32_t i = 0; state[i++] = seed; for ( ; i < 31; i++) { state[i] = (16807LL * state[i - 1]) % 2147483647; - if (state[i] < 0) + if (state[i] & 0x80000000) /* < 0 */ state[i] += 2147483647; } for (i = 31; i < 34; i++) state[i] = state[i - 31];