Eelco Chaudron <[email protected]> writes:
> 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.
I can put a NEWS entry, since users will see it. I guess we should
decide whether NAT will just not be available with a CSPRNG. See below.
>>>> + 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?
I'm worried about just logging it. Many times, people don't look at
logs right away (and these we should try to limit how often we send
them). Proceeding with insecure wouldn't be meaningfully detected by
the operator (unless they really scrape logs). I think if we want to
operate in an insecure fashion, I think a configuration flag might be
best since the local OVS admin has explicitly opted into an insecure
behavior. Actually, that may really need to be something like:
1. 'secure' - Use the CSPRNG from OpenSSL, fail without it (default)
2. 'fallback' - Allow falling back to insecure. (default if openssl is
not linked)
3. 'insecure' - Just use the xorshift implementation we have
Then we just don't use /dev/urandom directly.
>>>> + }
>>>>
>>>> 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.
I'll look closer at it. When I was first tracing the path, it looked to
me like returning false would just drop the conn key involved without
needing to do any special cleanup. However, I see there is some special
handling in there for ICMP code, and that may be something I missed.
Will address it in the next version.
> ---
>
> Typo in the comment above: "unaviable" should be "unavailable".
>
> Duplicated word in the same comment: "since since".
D'oh. Fixed.
> ---
>
>> +#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?
It's true that this is a different pattern. I must have misremembered
the order when I was writing it. Maybe it is best to just use
RAND_bytes() and check that it returns '1'. RAND_status() does
explicitly tell that RAND_bytes() will re-seed as needed, and returns an
error when it can't. Actually, makes me wonder if the check in
stream-ssl is still needed.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
