[[ NOTE: need to fix up my INBOX detection a bit. I may have sent from
my personal address. ]]
Ilya Maximets <[email protected]> writes:
> 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.
Okay, this is a good catch. We should keep the chacha20 fallback in the
case that OpenSSL EVP can't provide that stream.
>> +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?
See above, we should check for OPENSSL_NO_CHACHA.
> ---
>
>> +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?
I can have an additional internal seed() function that will allow
pulling from the stream buffer. I didn't benchmark it, but it could
actually be 'faster' by reducing the syscalls.
>> +/* 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?
I can mention it. It really isn't such a huge deal, since
reconstruction would still only be for bytes in the buf[] in flight. If
I update the seed()/refill() functions as above, then I think it
probably could even qualify.
>> + /* 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.
It may be confusing. The skip isn't a good wording here, so I'll fix it
up. We simply treat the buffer smaller - but again with the above
changes, it won't matter.
> ---
>
>> {
>> +#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?
It seems silly to add code for this.
> 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?
Since we'll implement the chacha20 fallback, then this won't have a
guard, and the other internal functions won't either.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
