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

["René Scharfe" <l.s.r@web.de>]

Am 18.01.22 um 10:45 schrieb Ævar Arnfjörð Bjarmason:
>
> On Mon, Jan 17 2022, brian m. carlson wrote:
>
>> +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....

Clearing errno is unnecessary as long as it's always set on error and
the return code indicates whether to look at it.  Translating the return
codes of RAND_bytes to suitable errno values makes sense if the goal is
to consistently report errors that way on all platforms.

arc4random_buf never fails, getrandom and getentropy do set errno, so no
translation is needed for them.  RAND_bytes doesn't set errno according
to [1], and the translation above looks sensible.

RtlGenRandom also doesn't set errno according to [2], but a translation
is missing above.  Should it set errno to EIO in the error case?

[1] https://www.openssl.org/docs/manmaster/man3/RAND_bytes.html
[2] https://docs.microsoft.com/en-us/windows/win32/api/ntsecapi/nf-ntsecapi-rtlgenrandom

>
>> +#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...

If the last close(2) call fails for some reason then callers of
csprng_bytes() won't notice due to the return code being zero, nor do
they care -- they got their random data and they cannot do anything
about the file descriptor that now hangs in limbo.  So this code looks
OK to me.

René