On 10/12/26 5:11 PM, Aaron Conole wrote:
> This commit makes the ChaCha20 CSPRNG prefer using
> OpenSSL's SIMD optimized ChaCha20 on platforms which
> are compiled with such support. This represents a
> major improvement on performance, without impacting
> the underlying CSPRNG functionality.
>
> 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: 6da48001f "random: Provide OpenSSL EVP cipher as a CSPRNG source."
────────────────────────────────────────
commit 6da48001f44958ae6d32cf7b3cfd9c36fc8401a9 ("random: Provide OpenSSL EVP
cipher as a CSPRNG source.")
Author: Aaron Conole <[email protected]>
This patch adds an OpenSSL EVP ChaCha20 backend for the cs_random CSPRNG
in lib/random.c. When HAVE_OPENSSL is defined, it replaces the in-tree
ChaCha20 implementation at compile time.
> +static void
> +csprng_seed(struct csprng *c)
> +{
> + uint8_t key[CHACHA_KEYLEN];
> + uint8_t iv[CHACHA_K_INPUT];
> +
> + get_entropy_or_die(key, sizeof key);
> + get_entropy_or_die(iv, sizeof iv);
> +
> + if (EVP_EncryptInit_ex(c->state, EVP_chacha20(), NULL, key, iv) != 1) {
> + VLOG_FATAL("cs_random: EVP reseed failure.");
> + }
What happens here when OpenSSL can't provide ChaCha20? On OpenSSL 3 with
FIPS-only default properties (RHEL in FIPS mode, for example), ChaCha20
is not an approved algorithm. The implicit fetch would fail and this
would call VLOG_FATAL on the first cs_random_bytes() call.
The call chain looks like this:
conntrack_init() or NAT tuple selection
nat_random_uint32()
cs_random_uint32()
cs_random_bytes()
csprng_seed()
EVP_EncryptInit_ex() fails -> VLOG_FATAL
On an affected host, ovs-vswitchd with a userspace datapath would exit
when the datapath is created, or at the latest on the first NAT
connection. Before this patch, cs_random_uint32() could not fail this
way.
> +struct csprng {
> +#ifndef HAVE_OPENSSL
> + uint32_t state[16]; /* ChaCha20 state:
> + * constant|key|counter|nonce. */
> +#else
> + EVP_CIPHER_CTX *state; /* EVP instance for OpenSSL. */
> +#endif
[...]
> +#ifndef HAVE_OPENSSL
The backend is picked only from HAVE_OPENSSL, which is set for TLS
support. Could the in-tree ChaCha20 stay compiled in as a runtime
fallback when EVP_EncryptInit_ex() fails? Also, there is no configure
check for EVP_chacha20(), so won't builds against an OpenSSL with
OPENSSL_NO_CHACHA fail to compile?
---
> +static void
> +csprng_refill(struct csprng *c)
> +{
> + static const uint8_t zero[CSPRNG_BUFSZ];
> + int outl;
> +
> + if (EVP_EncryptUpdate(c->state, c->buf, &outl, zero, sizeof c->buf) != 1
> ||
> + outl != sizeof c->buf) {
> + VLOG_FATAL("cs_random: EVP fill failure.");
> + }
> +
> + c->pos = 0;
> +}
This EVP csprng_refill() doesn't do the fast key erasure that the
built-in version does. The built-in version rekeys from the first 32
bytes of each refill and then wipes them:
csprng_rekey(c, c->buf);
memset(c->buf, 0, CHACHA_KEYLEN);
Here, the key and IV from csprng_seed() stay in the heap-allocated
EVP_CIPHER_CTX until the next reseed, which comes after CSPRNG_RESEED
(1600 KiB) of output. The local key[] and iv[] memsets in csprng_seed()
don't clear the copies held inside the context. Since ChaCha20 is
seekable, anyone who can read process memory (a core dump, for example)
could regenerate all output since the last reseed. That includes the
conntrack hash basis and NAT port choices. The backtracking window goes
from 4 KiB to about 1.6 MiB.
Could this rekey the EVP context from the first CHACHA_KEYLEN plus IV
bytes of each refill and skip those bytes, the same way the C path
does?
> +/* This is the EVP based CSPRNG using the OpenSSL EVP framework to create a
[...]
> + * Technically, we don't re-write the seeded variable on reinitialization,
> which
> + * represents a difference in forward secrecy between the two
> implementations,
This comment acknowledges the forward secrecy difference. The commit
message, however, says the change is made without impacting the
underlying CSPRNG functionality. Should the commit message mention
this?
> + /* 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
> + * implementation, we still skip to keep it simple,
> + * although technically there are the extra KEYLEN bytes. */
Is this comment accurate? The EVP csprng_refill() sets c->pos = 0 and
serves every byte of c->buf, so nothing is skipped. Only the assertion
bound is reduced.
---
> {
> +#ifndef HAVE_OPENSSL
> const uint32_t input[16] = {
[...]
> ovs_assert(!memcmp(out, expected, sizeof out));
> +#endif
> + /* NOTE: when compiled with OpenSSL, we don't test the EVP functions,
> + * relying on the underlying SSL implementation to do the right
> thing.
> + * Keep the message below for the library.at test. */
> printf("ok: RFC 8439 2.3.2 block function\n");
In an OpenSSL build, test_random_main() prints the "ok: RFC 8439 2.3.2
block function" line without running any check, so library.at passes
without testing anything. The EVP path has no test at all, including
the 16-byte IV layout (32-bit counter plus 96-bit nonce) passed to
EVP_chacha20() in csprng_seed(). Could a known-answer test be added for
the EVP path?
Also, lib/random.h still declares chacha20_block() unconditionally:
void chacha20_block(const uint32_t input[CHACHA_K_INPUT],
uint8_t out[CHACHA_BLOCK]);
Its definition in lib/random.c is now inside #ifndef HAVE_OPENSSL.
Wouldn't any caller fail to link in OpenSSL builds? Should the
declaration get the same guard?
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
