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?
> > + 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.
> > + }
> >
> > 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
