On 9/8/26 2:27 PM, Aaron Conole wrote: > 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. I'd say we should not over-complicate things. All distributions I know of build with OpenSSL. People who care about performance will build with OpenSSL because it also affects database performance and a few other things. People using userspace datapath usually do that for performance reasons, so they will have OpenSSL linked in vast majority of cases. Also, a large portion of deployments do not have multi-tenancy or not exposed to any external networks, so they couldn't care less about security of this NAT interface. And with the main vector here being DNS, people who care about DNS security should use DoH/T/etc. anyway. So, I'd say just fall back to want we have today. Warn once in the log. IMO, that should be enough.
Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
