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

From: René Scharfe <l.s.r@web.de>
Date: Jan 18, 2022, 13:31 UTC
Message-ID: <fdab40cf-572c-d5a1-3dc0-8afe736c38af@web.de>
In-Reply-To: <220118.86v8yhpdmr.gmgdl@evledraar.gmail.com>

Am 18.01.22 um 10:45 schrieb Ævar Arnfjörð Bjarmason:

Show 50 quoted lines

>
> 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

Show 30 quoted lines

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