On 10/12/26 5:11 PM, Aaron Conole wrote:
> Until now, utilities wishing access to a quality source of
> cryptographically secure random numbers would have to either
> forego security and just use the xorshift, or use one of a
> few very slow entropy options like get_entropy_or_die or
> OpenSSL's RAND_bytes() calls. Both of these are quality
> tested mechanisms, but the drawback is a very long time to
> execute (measured 10m iterations of rand_bytes taking
> roughly 5.14s to complete).
>
> This change adds a ChaCha20 based CSPRNG, which is known
> to be a high quality CSPRNG. The benefits are very fast
> execution time (the same 10m iterations only take 0.059s
> vs. the PRNG taking 0.018s).
>
> The implementation heavily relies on referencing the
> following:
> * RFC 8439, "ChaCha20 and Poly1305 for IETF Protocols"
> * libevent/arc4random.c, "Arc4 random number generator for OpenBSD."
> * glibc/stdlib/arc4random.c, "Psuedo random number generator"
> * Ben Tasker, "Writing (and backdooring) a ChaCha20 based CSPRNG"
>
> Signed-off-by: Aaron Conole <[email protected]>
> ---
A different LLM had the following to say about this patch
(some parts seem reasonable to a human posting this, but not
fully validated):
────────────────────────────────────────
REVIEW: 5b2647092 "random: Implement a ChaCha20 based CSPRNG."
────────────────────────────────────────
commit 5b26470924cfe117d22c7fee5115e2b3e3fdd66c ("random: Implement a ChaCha20
based CSPRNG.")
Author: Aaron Conole <[email protected]>
This patch adds a per-thread ChaCha20-based CSPRNG to lib/random.c and
exposes it as cs_random_bytes() and cs_random_uint32(). When OpenSSL is
available it uses EVP instead. It also adds an RFC 8439 block selftest
and a benchmark to tests/test-random.c.
> +void
> +cs_random_bytes(void *b, size_t l)
> +{
> + struct csprng *c = cs_state_get();
[...]
> + if (!c->initialized || c->served >= CSPRNG_RESEED) {
> + csprng_seed(c);
> + }
> +static void
> +csprng_seed(struct csprng *c)
> +{
> + uint8_t key[CHACHA_KEYLEN];
> +
> + get_entropy_or_die(key, sizeof key);
Can this take down the process at runtime? Every CSPRNG_RESEED bytes
(about 409600 cs_random_uint32() calls per thread), and on first use in
each new thread, cs_random_bytes() calls csprng_seed(). That calls
get_entropy_or_die(). In lib/entropy.c, get_entropy() opens
/dev/urandom on each call, and get_entropy_or_die() does VLOG_FATAL on
any error.
The call chain is:
cs_random_uint32()
cs_random_bytes()
csprng_seed()
get_entropy_or_die()
get_entropy() -> open("/dev/urandom") fails -> VLOG_FATAL
Before this patch, get_entropy_or_die() was only reached from one-time
initialization. With the follow-up conntrack NAT change, datapath
threads reach this path while handling packets. A transient EMFILE or
ENFILE at reseed time would then exit ovs-vswitchd.
Since csprng_refill() already rekeys for forward secrecy, could a
failed reseed keep the current key and log a warning instead?
---
The commit message quotes timings for "10m iterations", but the
benchmark loop runs N_ROUNDS * N_ROUNDS times:
> + enum { N_ROUNDS = 10000 };
[...]
> + for (i = 0; i < N_ROUNDS * N_ROUNDS; i++) {
> + x += cs_random_uint32();
> + }
That is 100,000,000 iterations, not 10M. Should the commit message or
the loop count be updated so the figures match?
The header comment in lib/random.c also cites a source that the commit
message's reference list leaves out:
> +/* This is the 32-bit CSPRNG adapted from the ChaCha20 algorithm from
> + * arc4random(3) and from the Linux kernel's get_random_u32().
Should the Linux kernel be added to the references in the commit
message?
There is also an unused include in tests/test-random.c, since only
ovs_assert() is called:
> #include "random.h"
> +#include <assert.h>
---
> +#define CHACHA_K_INPUT 16 /* ChaCha20 inblock size in bytes. */
> +void chacha20_block(const uint32_t input[CHACHA_K_INPUT],
> + uint8_t out[CHACHA_BLOCK]);
Is the comment on CHACHA_K_INPUT correct? chacha20_block() uses it as
a count of 16 uint32_t words, which is 64 bytes. A caller who trusts
the header comment and passes a 16-byte buffer would have it read past
the end.
The same macro sizes a string here:
> +static const uint8_t chacha_sigma[CHACHA_K_INPUT + 1] = "expand 32-byte k";
That only works because the string happens to be 16 characters long.
The comment in cs_random_bytes() also seems to misdescribe the
reserved bytes:
> + /* c->buf isn't fully available for reading due to
> + * the initial 32-bytes from the block being used as a
> + * seeding counter. In the case of the OpenSSL EVP
csprng_refill() uses those 32 bytes as the next key via
csprng_rekey(c, c->buf). The counter is in state[12..13]. Should this
say "next key" instead of "seeding counter"?
---
> + ovs_assert(l < sizeof c->buf - CHACHA_KEYLEN);
Is this bound off by one? After csprng_refill(), c->pos is 32. With
l == 4064, the check c->pos + l > sizeof c->buf is 4096 > 4096, which
is false, so the memcpy() of buf[32..4095] stays in bounds. The assert
still aborts.
Also, the prototype in random.h documents no maximum length:
> +void cs_random_bytes(void *, size_t);
random_bytes() accepts any length. Would it be better to loop over
refills for larger requests, or at least document the cap?
---
> + get_entropy_or_die(key, sizeof key);
> + csprng_rekey(c, key);
> + memset(key, 0, sizeof key);
key is never read after this memset(), so the compiler is free to
remove it as a dead store. Is the key actually scrubbed from the stack
here?
Similarly, chacha20_block() leaves its working array on the stack:
> + uint32_t x[CHACHA_K_INPUT];
[...]
> + ovs_store_le32(out + i * 4, x[i] + input[i]);
Because out = x + input, anyone holding both x[] and the output block
can recover the key words input[4..11]. Should x[] be cleared before
returning?
On reseed, csprng_seed() sets c->pos = sizeof c->buf but leaves the old
unserved keystream in c->buf until the next refill. Should that be
cleared too, given the forward secrecy goal stated in the
csprng_refill() comment?
---
> +#define QUARTERROUND(a, b, c, d) \
> + a += b; d ^= a; d = ROTL32(d, 16); \
> + c += d; b ^= c; b = ROTL32(b, 12); \
> + a += b; d ^= a; d = ROTL32(d, 8); \
> + c += d; b ^= c; b = ROTL32(b, 7)
This is a minor point. The macro expands to several statements with no
do { } while (0) wrapper. All current uses are inside the braced loop
in chacha20_block(), but if it were used as the body of an unbraced
if, only "a += b" would be conditional. Would wrapping it be worth it?
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
