On 4 Sep 2026, at 16:41, Aaron Conole wrote:
> Eelco Chaudron <[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 do some testing.
>
>> //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?
>
> AFAICT, RAND_bytes doesn't take system calls unless the prng isn't
> seeded properly.
>
>>> + 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.
>
> Yes - OTOH, it is considered a security issue otherwise. Not sure how
> best to deal with not having RAND_bytes() available. We shouldn't try
> to implement our own CSPRNG (given we already support linking to
> openssl). Should we not support NAT in this case? Maybe set a flag
> that we are running in an 'insecure' configuration?
Not sure if we have any people not linking openssl. It's definitely a behaviour
change.
>>> + 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.
>
> Yes, I had AI writing a test case (and it wrote some comments). The
> test case stopped making sense (because it could take a long time to
> converge on the xorshift reversal).
>
>>> + * 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.
>
> Sure, I can change it.
>
>>> + 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?
>
> It's a good question, see above.
Maybe we should just log it (like with the fallback above) and continue?
>>> + }
>>>
>>> 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 {
PS: I was playing with some local AI and it complained about the below.
To be honest I did not look into them:
[ ... ]
> @@ -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;
> + }
When nat_get_unique_l4() reaches this block, it has already executed
at least one iteration of the FOR_EACH_PORT_IN_RANGE loop above,
which writes intermediate port values into *port (and *peer_port):
> *port = htons(curr);
> if (peer_port) {
> *peer_port = htons(curr);
> }
If nat_random_uint32() fails, the early return leaves *port (and
*peer_port) set to whichever value the last loop iteration wrote.
The normal exhaustion path, reached when attempts >= range or
attempts < 16, restores the originals:
> *port = htons(orig);
> if (peer_port) {
> *peer_port = htons(orig);
> }
> return false;
The early return skips this restore. In nat_get_unique_tuple(), the
first nat_get_unique_l4() call passes &rev_key->src.port:
> if (nat_info->nat_action & NAT_ACTION_DST_PORT) {
> found = nat_get_unique_l4(ct, rev_key, &rev_key->src.port,
> NULL, curr_dport, min_dport, max_dport);
> }
>
> if (!found) {
> found = nat_get_unique_l4(ct, rev_key, &rev_key->dst.port,
> [...]
> curr_sport, min_sport, max_sport);
> }
If that call returns false via the entropy early-return after the loop
has modified rev_key->src.port, the second call proceeds with a stale
rev_key->src.port value in its conn_lookup(). Does this leave
rev_key->src.port in an intermediate, unintended state that corrupts
the conn_lookup() in the second nat_get_unique_l4() call?
The triggering condition (OpenSSL and /dev/urandom both failing after
at least one loop iteration but only when range > max_attempts and
initial attempts >= 16) is highly unlikely in practice, but the fix
is straightforward: restore *port (and *peer_port) to htons(orig)
before the early return, matching the existing exhaustion path.
---
Typo in the comment above: "unaviable" should be "unavailable".
Duplicated word in the same comment: "since since".
---
> +#ifdef HAVE_OPENSSL
> + if (RAND_bytes((uint8_t *) r, sizeof *r) == 1 && RAND_status()) {
> + return true;
> + }
The comment above nat_random_uint32() describes this as "the same
check already used in lib/stream-ssl.c", but stream-ssl.c checks
RAND_status() *before* any RAND_bytes() call and uses it to decide
whether to reseed, which is a meaningfully different pattern. Here,
RAND_status() is checked after a successful RAND_bytes() return. In
OpenSSL 1.1.0+, RAND_bytes() returning 1 already means the output is
cryptographically strong; a post-call RAND_status() == 0 after that
would be an unusual state. Could the comment be updated to describe
what scenario this is actually guarding against, to avoid confusion
with the stream-ssl.c pattern?
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
