RE: [PATCH 2/2] reftable/stack: accept insecure random bytes
- From
rsbecker@nexbridge.com <rsbecker@nexbridge.com>
- Date
- Jan 7, 2025, 15:37 UTC
- Message-ID
- <005d01db611a$19ed55f0$4dc801d0$@nexbridge.com>
- In-Reply-To
- <20250107-b4-pks-reftable-csprng-v1-2-6109a54a8756@pks.im>
On January 7, 2025 10:27 AM, Patrick Steinhardt wrote:
Show 71 quoted lines
>The reftable library uses randomness in two call paths:
>
> - When reading a stack in case some of the referenced tables
> disappears. The randomness is used to delay the next read by a
> couple of milliseconds.
>
> - When writing a new table, where the randomness gets appended to the
> table name (e.g. "0x000000000001-0x000000000002-0b1d8ddf.ref").
>
>In neither of these cases do we need strong randomness.
>
>Unfortunately though, we have observed test failures caused by the former case. In
>t0610 we have a test that spawns a 100 processes at once, all of which try to write
>a new table to the stack. And given that all of the processes will require
>randomness, it can happen that these processes make the entropy pool run dry,
>which will then cause us to
>die:
>
> + test_seq 100
> + printf %s commit\trefs/heads/branch-%s\n
> 68d032e9edd3481ac96382786ececc37ec28709e 1
> + printf %s commit\trefs/heads/branch-%s\n
> 68d032e9edd3481ac96382786ececc37ec28709e 2
> ...
> + git update-ref refs/heads/branch-98 HEAD
> + git update-ref refs/heads/branch-97 HEAD
> + git update-ref refs/heads/branch-99 HEAD
> + git update-ref refs/heads/branch-100 HEAD
> fatal: unable to get random bytes
> fatal: unable to get random bytes
> fatal: unable to get random bytes
> fatal: unable to get random bytes
> fatal: unable to get random bytes
> fatal: unable to get random bytes
> fatal: unable to get random bytes
>
>The report was for NonStop, which uses OpenSSL as the backend for randomness.
>In the preceding commit we have adapted that backend to also return randomness
>in case the entropy pool is empty and the caller passes the
>`CSPRNG_BYTES_INSECURE` flag. Do so to fix the issue.
>
>Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
>Signed-off-by: Patrick Steinhardt <ps@pks.im>
>---
> reftable/stack.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/reftable/stack.c b/reftable/stack.c index
>6d0aa774e7e29d5366ed55df19725944f8eef792..572a74e00f9ed6040534e060
>652e72c26641749d 100644
>--- a/reftable/stack.c
>+++ b/reftable/stack.c
>@@ -493,7 +493,7 @@ static int reftable_stack_reload_maybe_reuse(struct
>reftable_stack *st,
> close(fd);
> fd = -1;
>
>- delay = delay + (delay * rand()) / RAND_MAX + 1;
>+ delay = delay + (delay * git_rand(CSPRNG_BYTES_INSECURE)) /
>+UINT32_MAX + 1;
> sleep_millisec(delay);
> }
>
>@@ -659,7 +659,7 @@ int reftable_stack_add(struct reftable_stack *st, static int
>format_name(struct reftable_buf *dest, uint64_t min, uint64_t max) {
> char buf[100];
>- uint32_t rnd = (uint32_t)git_rand(0);
>+ uint32_t rnd = git_rand(CSPRNG_BYTES_INSECURE);
> snprintf(buf, sizeof(buf), "0x%012" PRIx64 "-0x%012" PRIx64 "-%08x",
> min, max, rnd);
> reftable_buf_reset(dest);I think this is a good catch. We are seeing other tests fail similarly through this path. I will retest rc3 on both platforms since we cannot exhaust randomness on x86 using the hardware randomizer inside OpenSSL.