Eelco Chaudron via dev <[email protected]> writes:
> On 27 Aug 2026, at 17:23, Aaron Conole via dev wrote:
>
>> random_uint32() is a xorshift32 PRNG (see lib/random.c): it holds many
>> properties including uniformity and reversibility, so recovering the
>> internal state from any single observed output lets an attacker
>> compute every other output of that (per-thread) stream, forward and
>> backward. Using it for anything security-relevant is documented in
>> random.c itself as inappropriate.
>>
>> Most NAT tuple selection does not call random_uint32() directly.
>> Instead, nat_get_unique_tuple() picks both the NAT address
>> (get_addr_in_range()) and, in the common case, the NAT port
>> (set_sport_range()/set_dport_range()) via nat_range_hash(), a
>> deterministic hash of the flow tuple mixed with a single 32-bit value:
>> ct->hash_basis. That basis was previously drawn once from
>> random_uint32() at conntrack_init() time. Because it is a single,
>> long-lived value on which every NAT address and default-port choice
>> depends, an attacker able to recover the xorshift state feeding that
>> one random_uint32() call--e.g. by observing any other output drawn
>> from the same per-thread PRNG stream elsewhere in the process--could
>> predict every NAT tuple ovs-vswitchd will assign. In effect, it was
>> derived from a shared, reversible state.
>>
>> Two narrower paths draw directly from random_uint32() per new
>> connection rather than through the hash: the `nat(...,random)` port
>> path, and the retry offset used when the hash-selected port collides
>> with an existing connection under range congestion. These are
>> directly observable per-connection outputs.
>>
>> Fix this by drawing ct->hash_basis, and the two per-connection port
>> paths above, from a cryptographic source: OpenSSL's PRNG when
>> compiled against OpenSSL 1.1.0+ (checking RAND_status() to catch the
>> case where it has not been seeded properly despite RAND_bytes()
>> succeeding, the same check already used in lib/stream-ssl.c), falling
>> back to the system entropy pool.
>>
>> If neither source can provide randomness, do not silently downgrade
>> to the non-cryptographic PRNG. nat_random_uint32() instead reports
>> failure, and its callers propagate that as NAT tuple exhaustion:
>> nat_get_unique_l4() and nat_get_unique_tuple() both return false, so
>> the connection attempt is failed the same way as any other allocation
>> exhaustion (see the nat_res_exhaustion path in conn_update_state()),
>> and each occurrence increments the new conntrack_entropy_failed
>> coverage counter so operators can see it happening. There is also a
>> new point for ovs-vswitchd failure - at the conntrack initialization
>> if sufficient entropy cannot be pulled for ct->hash_basis.
>>
>> Signed-off-by: Aaron Conole <[email protected]>
>
> Hi Aaron,
>
> Thanks for the patch. See some comments below. As Ilya mentioned offline,
> it might be good to get some performance numbers with this change to see
> the impact of the RAND_bytes() usage.
I'll give a quick update on this (and why the respin took so long).
I've benchmarked the random_uint32() vs my proposed rand_bytes() version
and the numbers are far worse that I expected. RAND_bytes is
*excessively* slow to the point that it probably isn't usable:
for 10m iterations of calls:
random_uint32() --> 0.02s
RAND_bytes() version --> 5.14s
QnD ChaCha20 --> 0.16s
So, I'm looking at adapting my quick and dirty ChaCha20 and will submit
that as the next version.
> //Eelco
>
>> ---
>> lib/conntrack.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 72 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>> index f84cdd216a..d7fa6bd746 100644
>> --- a/lib/conntrack.c
>> +++ b/lib/conntrack.c
>> @@ -22,6 +22,10 @@
>> #include <netinet/icmp6.h>
>> #include <string.h>
>>
>> +#ifdef HAVE_OPENSSL
>> +#include <openssl/rand.h>
>> +#endif
>> +
>> #include "conntrack.h"
>> #include "conntrack-private.h"
>> #include "conntrack-tp.h"
>> @@ -30,6 +34,7 @@
>> #include "csum.h"
>> #include "ct-dpif.h"
>> #include "dp-packet.h"
>> +#include "entropy.h"
>> #include "flow.h"
>> #include "netdev.h"
>> #include "odp-netlink.h"
>> @@ -47,6 +52,45 @@
>>
>> VLOG_DEFINE_THIS_MODULE(conntrack);
>>
>> +/* Counts calls to nat_random_uint32() that could not obtain any
>> + * cryptographic randomness at all (neither OpenSSL nor the system
>> + * entropy pool). Each occurrence corresponds to one failed NAT tuple
>> + * allocation attempt. */
>> +COVERAGE_DEFINE(conntrack_entropy_failed);
>> +
>> +/* NAT tuple selection (both the address/port hash basis and the
>> + * fully-random port paths) must not be predictable from an observed
>> + * random_uint32() output, so it draws from a cryptographic source here
>> + * instead of the general-purpose xorshift32 PRNG in random.c.
>> + *
>> + * When compiled against OpenSSL 1.1.0+, this uses OpenSSL's PRNG. If
>> + * that is unavailable, or RAND_status() reports that it is not properly
>> + * seeded (see the same check in lib/stream-ssl.c), it falls back to the
>> + * system entropy pool. If neither source can provide randomness, this
>> + * returns false rather than falling back to the non-cryptographic PRNG:
>> + * callers must fail the NAT tuple allocation (and, transitively, the
>> + * connection attempt) instead of silently downgrading its quality. */
>> +static bool
>> +nat_random_uint32(uint32_t *r)
>> +{
>> +#ifdef HAVE_OPENSSL
>> + if (RAND_bytes((uint8_t *) r, sizeof *r) == 1 && RAND_status()) {
>
> No idea how expensive this is, and/or if it takes systemcalls.
> This is used in the fast path, so maybe we should do some benchmarking?
>
>> + return true;
>> + }
>> +
>> + static struct vlog_rate_limit rl1 = VLOG_RATE_LIMIT_INIT(1, 5);
>> + VLOG_WARN_RL(&rl1, "RAND_bytes unreliable, falling back to system "
>> + "entropy pool for NAT tuple selection");
>> +#endif
>> +
>> + if (!get_entropy(r, sizeof *r)) {
>
> The fallback is expensive; it does open/read/close() on /dev/urandom.
>
>> + return true;
>> + }
>> +
>> + COVERAGE_INC(conntrack_entropy_failed);
>> + return false;
>> +}
>> +
>> COVERAGE_DEFINE(conntrack_full);
>> COVERAGE_DEFINE(conntrack_l3csum_checked);
>> COVERAGE_DEFINE(conntrack_l3csum_err);
>> @@ -251,8 +295,16 @@ conntrack_init(void)
>>
>> /* This value can be used during init (e.g. timeout_policy_init()),
>> * set it first to ensure it is available.
>> - */
>> - ct->hash_basis = random_uint32();
>> + *
>> + * It is also the basis that nat_range_hash() mixes into every NAT
>> + * address and (non-random) NAT port choice, so it must come from
>> + * nat_random_uint32() rather than the predictable general-purpose
>> + * PRNG--otherwise all NAT tuples derived from it are only as
>
> The -- looks odd here, maybe an AI text cut/paste. Guess a '; ' would be
> better here.
>
>> + * unpredictable as that single 32-bit xorshift output. */
>
> Also, to a non-native speaker, the 'unpredictable' reads odd. Maybe something
> like;
>
> * PRNG; otherwise, all NAT tuples derived from it would be
> * predictable from that PRNG output.
>
>
>> + if (!nat_random_uint32(&ct->hash_basis)) {
>> + VLOG_FATAL("conntrack: unable to obtain cryptographic randomness "
>> + "to initialize the NAT hash basis");
>
> Are we ok with not having a backup?
>
>> + }
>>
>> ovs_rwlock_init(&ct->resources_lock);
>> ovs_rwlock_wrlock(&ct->resources_lock);
>> @@ -2556,8 +2608,19 @@ another_round:
>> }
>>
>> if (attempts < range && attempts >= 16) {
>> + uint32_t r;
>> +
>> + if (!nat_random_uint32(&r)) {
>> + /* CPRNG wasn't available, return false in this case. It is
>> + * possible that the entropy pool is only temporarily unaviable,
>> + * but bailing on this connection attempt should be okay since
>> + * since we don't want to waste cpu cycles for an event that may
>> + * take a while. */
>> + return false;
>> + }
>> +
>> attempts /= 2;
>> - curr = min + (random_uint32() % range);
>> + curr = min + (r % range);
>> goto another_round;
>> }
>>
>> @@ -2613,7 +2676,12 @@ nat_get_unique_tuple(struct conntrack *ct, struct
>> conn *conn,
>> hash = nat_range_hash(fwd_key, basis, nat_info);
>>
>> if (nat_info->nat_flags & NAT_RANGE_RANDOM) {
>> - port_off = random_uint32();
>> + if (!nat_random_uint32(&port_off)) {
>> + /* The entropy failure here will reflect that we're resource
>> + * exhausted. It is a bit confusing because we're out of
>> + * entropy rather than out of actual NAT range. */
>> + return false;
>> + }
>> } else if (basis) {
>> port_off = hash;
>> } else {
>> --
>> 2.55.0
>>
>> _______________________________________________
>> dev mailing list
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
