From dbf230e4cf823dd6b6a3bad6d29dfad4f0ffa8fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Esser?= Date: Mon, 14 Jun 2021 23:28:28 +0200 Subject: [PATCH 1/2] libmisc/salt.c: Add comments how the minmum buffer length is computed. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the previous commit we refactored the functions converting the rounds number into a string for use with the crypt() function, to not require any static buffer anymore. Add some clarifying comments about how the minimum required buffer length is computed inside of these functions. Signed-off-by: Björn Esser --- libmisc/salt.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/libmisc/salt.c b/libmisc/salt.c index 98982ed1..e17093fc 100644 --- a/libmisc/salt.c +++ b/libmisc/salt.c @@ -216,7 +216,14 @@ static /*@observer@*/void SHA_salt_rounds_to_buf (char *buf, /*@null@*/int *pref return; } - /* Check if the result buffer is long enough. */ + /* + * Check if the result buffer is long enough. + * We are going to write a maximum of 17 bytes, + * plus one byte for the terminator. + * rounds=XXXXXXXXX$ + * 00000000011111111 + * 12345678901234567 + */ assert (GENSALT_SETTING_SIZE > buf_begin + 17); (void) snprintf (buf + buf_begin, 18, "rounds=%lu$", rounds); @@ -274,7 +281,14 @@ static /*@observer@*/void BCRYPT_salt_rounds_to_buf (char *buf, /*@null@*/int *p rounds = 19; } - /* Check if the result buffer is long enough. */ + /* + * Check if the result buffer is long enough. + * We are going to write three bytes, + * plus one byte for the terminator. + * XX$ + * 000 + * 123 + */ assert (GENSALT_SETTING_SIZE > buf_begin + 3); (void) snprintf (buf + buf_begin, 4, "%2.2lu$", rounds); @@ -308,8 +322,15 @@ static /*@observer@*/void YESCRYPT_salt_cost_to_buf (char *buf, /*@null@*/int *p cost = Y_COST_MAX; } - /* Check if the result buffer is long enough. */ - assert (GENSALT_SETTING_SIZE > buf_begin + 3); + /* + * Check if the result buffer is long enough. + * We are going to write four bytes, + * plus one byte for the terminator. + * jXX$ + * 0000 + * 1234 + */ + assert (GENSALT_SETTING_SIZE > buf_begin + 4); buf[buf_begin + 0] = 'j'; if (cost < 3) { From bc8257cf73328e450511b13cbd35e1994feccb30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Esser?= Date: Wed, 23 Jun 2021 16:06:47 +0200 Subject: [PATCH 2/2] libmisc/salt.c: Obtain random bytes from /dev/urandom. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Using the random() function to obtain pseudo-random bytes for generating salt strings is considered to be dangerous. See CWE-327. We really should use a more reliable source for obtaining pseudo-random bytes like /dev/urandom. Fixes #376. Signed-off-by: Björn Esser --- libmisc/salt.c | 45 ++++++++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/libmisc/salt.c b/libmisc/salt.c index e17093fc..af9f011f 100644 --- a/libmisc/salt.c +++ b/libmisc/salt.c @@ -11,11 +11,10 @@ #ident "$Id$" -#include -#include -#include -#include #include +#include +#include +#include #include "prototypes.h" #include "defines.h" #include "getdef.h" @@ -74,7 +73,7 @@ #define GENSALT_SETTING_SIZE 100 /* local function prototypes */ -static void seedRNG (void); +static long read_random_bytes (void); static /*@observer@*/const char *gensalt (size_t salt_size); #if defined(USE_SHA_CRYPT) || defined(USE_BCRYPT) static long shadow_random (long min, long max); @@ -125,23 +124,27 @@ static /*@observer@*/char *l64a (long value) } #endif /* !HAVE_L64A */ -static void seedRNG (void) +/* Read sizeof (long) random bytes from /dev/urandom. */ +static long read_random_bytes (void) { - struct timeval tv; - static int seeded = 0; + long randval = 0; + FILE *f = fopen ("/dev/urandom", "r"); - if (0 == seeded) { - (void) gettimeofday (&tv, NULL); - srandom (tv.tv_sec ^ tv.tv_usec ^ getpid ()); - seeded = 1; + if (fread (&randval, sizeof (randval), 1, f) != sizeof (randval)) + { + fprintf (shadow_logfd, + _("Unable to read from /dev/urandom.\n")); + + fclose(f); + exit (1); } + + fclose(f); + + return randval; } #if defined(USE_SHA_CRYPT) || defined(USE_BCRYPT) -/* It is not clear what is the maximum value of random(). - * We assume 2^31-1.*/ -#define RANDOM_MAX 0x7FFFFFFF - /* * Return a random number between min and max (both included). * @@ -151,8 +154,9 @@ static long shadow_random (long min, long max) { double drand; long ret; - seedRNG (); - drand = (double) (max - min + 1) * random () / RANDOM_MAX; + + drand = (double) (read_random_bytes () & RAND_MAX) / (double) RAND_MAX; + drand *= (double) (max - min + 1); /* On systems were this is not random() range is lower, we favor * higher numbers of salt. */ ret = (long) (max + 1 - drand); @@ -354,10 +358,9 @@ static /*@observer@*/const char *gensalt (size_t salt_size) assert (salt_size >= MIN_SALT_SIZE && salt_size <= MAX_SALT_SIZE); - seedRNG (); - strcat (salt, l64a (random())); + strcat (salt, l64a (read_random_bytes ())); do { - strcat (salt, l64a (random())); + strcat (salt, l64a (read_random_bytes ())); } while (strlen (salt) < salt_size); salt[salt_size] = '\0';