Ilya Maximets <[email protected]> writes:
> 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.
Well, the HAVE_OPENSSL check isn't in this patch. :-/
>> +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.
Yes, it can. We could be more relaxed about it, but basically, would
have to reserve some seed from a previous run. But if /dev/urandom
isn't available for read() then there is a bigger issue.
> 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:
Yes, the benchmark is a different utility. I will update with this
version if that seems more appropriate.
>> + 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?
I can do that, but above the loop count is for a test-bench file I have,
not the committed benchmark. The test-bench is a bit too synthetic
because it has a bunch of implementations bundled together. That made
some strange performance data that didn't hold up when I ported things
to ovs code base. I can re-run with the ovstest benchmark if needed.
> 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?
Addressed when responding to Ilya's licensing concern.
> 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.
Yes, my mistake.
> 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"?
This got some weird hallucination. That hunk is weird. Still, I can
make the description a bit cleaner, probably.
> ---
>
>> + 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.
This looks wrong. l == 4064 won't abort when the ovs_assert runs. And
the csprng_refill() won't abort unless get_entropy_or_die fails.
> Also, the prototype in random.h documents no maximum length:
I can document the length. But if someone is really needing to pull 4k
of csprng data in OVS somewhere, we could be in trouble.
>> +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?
Hrrm.. this is a real problem - we may need something like (untested):
void ovs_memzero(uint8_t *ptr, size_t l)
{
size_t s;
for (s = 0; s < l; s++) {
volatile uint8_t *p = (volatile uint8_t *)(ptr + s);
*p = 0;
}
}
That should force the compiler NOT to remove the assignment to 'p'.
> 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?
chacha20_block may leave around data. I can erase this.
> 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?
csprng_seed() leaving the buffer around isn't an issue. Either the
buffer is still csrandom data, or is uninitialized data.
> ---
>
>> +#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?
no It isn't intended for anywhere else.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
