Re: [PATCH v3 1/2] wrapper: add a helper to generate numbers from a CSPRNG

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Jan 17 2022, brian m. carlson wrote:

> @@ -234,6 +234,14 @@ all::
>  # Define NO_TRUSTABLE_FILEMODE if your filesystem may claim to support
>  # the executable mode bit, but doesn't really do so.
>  #
> +# Define CSPRNG_METHOD to "arc4random" if your system has arc4random and
> +# arc4random_buf, "libbsd" if your system has those functions from libbsd,
> +# "getrandom" if your system has getrandom, "getentropy" if your system has
> +# getentropy, "rtlgenrandom" for RtlGenRandom (Windows only), or "openssl" if
> +# you'd want to use the OpenSSL CSPRNG.  You may set multiple options with
> +# spaces, in which case a suitable option will be chosen.  If unset or set to
> +# anything else, defaults to using "/dev/urandom".
> +#
>  # Define NEEDS_MODE_TRANSLATION if your OS strays from the typical file type
>  # bits in mode values (e.g. z/OS defines I_SFMT to 0xFF000000 as opposed to the
>  # usual 0xF000).
> @@ -693,6 +701,7 @@ TEST_BUILTINS_OBJS += test-bloom.o
>  TEST_BUILTINS_OBJS += test-chmtime.o
>  TEST_BUILTINS_OBJS += test-config.o
>  TEST_BUILTINS_OBJS += test-crontab.o
> +TEST_BUILTINS_OBJS += test-csprng.o
>  TEST_BUILTINS_OBJS += test-ctype.o
>  TEST_BUILTINS_OBJS += test-date.o
>  TEST_BUILTINS_OBJS += test-delta.o
> @@ -1908,6 +1917,31 @@ ifdef HAVE_GETDELIM
>  	BASIC_CFLAGS += -DHAVE_GETDELIM
>  endif
>  
> +ifneq ($(findstring arc4random,$(CSPRNG_METHOD)),)
> +	BASIC_CFLAGS += -DHAVE_ARC4RANDOM
> +endif
> +
> +ifneq ($(findstring libbsd,$(CSPRNG_METHOD)),)
> +	BASIC_CFLAGS += -DHAVE_ARC4RANDOM_LIBBSD
> +	EXTLIBS += -lbsd
> +endif
> +
> +ifneq ($(findstring getrandom,$(CSPRNG_METHOD)),)
> +	BASIC_CFLAGS += -DHAVE_GETRANDOM
> +endif
> +
> +ifneq ($(findstring getentropy,$(CSPRNG_METHOD)),)
> +	BASIC_CFLAGS += -DHAVE_GETENTROPY
> +endif
> +
> +ifneq ($(findstring rtlgenrandom,$(CSPRNG_METHOD)),)
> +	BASIC_CFLAGS += -DHAVE_RTLGENRANDOM
> +endif
> +
> +ifneq ($(findstring openssl,$(CSPRNG_METHOD)),)
> +	BASIC_CFLAGS += -DHAVE_OPENSSL_CSPRNG
> +endif
> +

Just an implementation nit: I think:
    
    ifdef HAVE_CSPRNG_ARC4RANDOM
    endif
    ifdef HAVE_CSPRNG_GETRANDOM
    endif

etc. makes this sort of thing much simpler. It piggy-backs on make's
default "is defined?" semantics, which avoids a lot of verbosity.

The "findstring" function you're using is also designed for one-letter
flags like those used for MAKEFLAGS. See /if.*filter/ for a better
pattern for space-separated tokens if you'd like to use this "variable
takes a space separated list" pattern....

> diff --git a/config.mak.uname b/config.mak.uname
> index a3a779327f..ff0710a612 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -141,6 +141,7 @@ ifeq ($(uname_S),Darwin)
>  	HAVE_BSD_SYSCTL = YesPlease
>  	FREAD_READS_DIRECTORIES = UnfortunatelyYes
>  	HAVE_NS_GET_EXECUTABLE_PATH = YesPlease
> +	CSPRNG_METHOD = arc4random
>  
>  	# Workaround for `gettext` being keg-only and not even being linked via
>  	# `brew link --force gettext`, should be obsolete as of

..another reason to use the "defined?" pattern is that if you're using
an older version of OSX (or one of the other OS's) and this is wrong you
can just:

    HAVE_CSPRNG_WHATEVER =

But with space-separated you'll need a more verbose $(filter-out ...).

> +/*

nit: API comments with "/**" comments.

> + * Generate len bytes from the system cryptographically secure PRNG.
> + * Returns 0 on success and -1 on error, setting errno.  The inability to
> + * satisfy the full request is an error.
> + */
> +int csprng_bytes(void *buf, size_t len);
> +
>  #endif
> diff --git a/t/helper/test-csprng.c b/t/helper/test-csprng.c
> new file mode 100644
> index 0000000000..65d14973c5
> --- /dev/null
> +++ b/t/helper/test-csprng.c
> @@ -0,0 +1,29 @@
> +#include "test-tool.h"
> +#include "git-compat-util.h"
> +
> +

nit: extra line, also git-compat-util.h doesn't need to be included, test-tool.h has it.

> +int cmd__csprng(int argc, const char **argv)
> +{
> +	unsigned long count;
> +	unsigned char buf[1024];
> +
> +	if (argc > 2) {
> +		fprintf(stderr, "usage: %s [<size>]\n", argv[0]);
> +		return 2;
> +	}
> +
> +	count = (argc == 2) ? strtoul(argv[1], NULL, 0) : -1L;
> +
> +	while (count) {
> +		unsigned long chunk = count < sizeof(buf) ? count : sizeof(buf);
> +		if (csprng_bytes(buf, chunk) < 0) {
> +			perror("failed to read");
> +			return 5;
> +		}
> +		if (fwrite(buf, chunk, 1, stdout) != chunk)
> +			return 1;
> +		count -= chunk;
> +	}
> +
> +	return 0;
> +}

I know this is just a "demo", but why not add a trivial test *.sh file
for whatever low-level wrapper test we have, so we at least know it
won't segfault etc.

These return codes seem quite specific, any reason we need 2 and 5, not
just "return 1" everywhere on error?

error_errno() instead of perror()?

> +int csprng_bytes(void *buf, size_t len)
> +{
> +#if defined(HAVE_ARC4RANDOM) || defined(HAVE_ARC4RANDOM_LIBBSD)
> +	/* This function never returns an error. */
> +	arc4random_buf(buf, len);
> +	return 0;
> +#elif defined(HAVE_GETRANDOM)
> +	ssize_t res;
> +	char *p = buf;
> +	while (len) {
> +		res = getrandom(p, len, 0);
> +		if (res < 0)
> +			return -1;
> +		len -= res;
> +		p += res;
> +	}
> +	return 0;
> +#elif defined(HAVE_GETENTROPY)
> +	int res;
> +	char *p = buf;
> +	while (len) {
> +		/* getentropy has a maximum size of 256 bytes. */
> +		size_t chunk = len < 256 ? len : 256;
> +		res = getentropy(p, chunk);
> +		if (res < 0)
> +			return -1;
> +		len -= chunk;
> +		p += chunk;
> +	}
> +	return 0;
> +#elif defined(HAVE_RTLGENRANDOM)
> +	if (!RtlGenRandom(buf, len))
> +		return -1;
> +	return 0;
> +#elif defined(HAVE_OPENSSL_CSPRNG)
> +	int res = RAND_bytes(buf, len);
> +	if (res == 1)
> +		return 0;
> +	if (res == -1)
> +		errno = ENOTSUP;
> +	else
> +		errno = EIO;
> +	return -1;

Why fake up errno here instead of just returning -1? In 2/2 you call
error_errno(). This seems buggy for a function that doesn't clear errno
and doesn't guarantee that it's set on failure....

> +#else
> +	ssize_t res;
> +	char *p = buf;
> +	int fd, err;
> +	fd = open("/dev/urandom", O_RDONLY);
> +	if (fd < 0)
> +		return -1;
> +	while (len) {
> +		res = xread(fd, p, len);
> +		if (res < 0) {
> +			err = errno;
> +			close(fd);
> +			errno = err;
> +			return -1;
> +		}
> +		len -= res;
> +		p += res;
> +	}
> +	close(fd);
> +	return 0;
> +#endif
> +}

...seems better to turn it into a "int *failure_errno" parameter
instead, or just have the function itself call error_errno() in these
cases.

You can't just check "if (errno)" either due to the return value of
close() not being checked here...