tcp: remove bias from the randomised initial sequence number

[tinic]

_nxd_tcp_client_socket_connect() and _nx_tcp_server_socket_accept() compose a fresh ISN from two random draws with a bitwise OR:

socket_ptr -> nx_tcp_socket_tx_sequence =  (((ULONG)NX_RAND()) << NX_SHIFT_BY_16) & 0xFFFFFFFF;
socket_ptr -> nx_tcp_socket_tx_sequence |= (ULONG)NX_RAND();

The two draws overlap in bits 16-30, and an OR of two random bits is 1 with probability 3/4. rand() returns 0..RAND_MAX, so with the usual RAND_MAX of 0x7FFFFFFF the ISN has 17 bits at P(1) = 1/2 and 15 bits at P(1) = 3/4. That is 29.2 bits of Shannon entropy and 17 + 15*log2(4/3) = 23.2 bits of min-entropy: the likeliest ISN comes up 2^(32-23.2) = 438 times more often than under a uniform distribution, and an attacker who enumerates the dense upper values first faces 2^23 rather than 2^32.

This is a property of the composition, not of the generator. A port that points NX_RAND at a CSPRNG loses the same nine bits, because the loss is entirely in the overlap between the two draws.

Adding instead of ORing removes it, and matches the else branch immediately below -- the path every reused socket takes, which already adds. With a 31-bit draw the sum is exactly uniform over the whole 32-bit space: (a << 16) is uniform over the 65536 multiples of 65536, and adding an independent uniform value over [0, 2^31) leaves every 32-bit residue equally likely. Measured over 2e7 samples of a 31-bit uniform source:

           Shannon   min-entropy   likeliest ISN vs uniform
with |      29.168        23.220                      440x
with +      32.000        31.991                        1x

The consequence is blind in-window injection: an off-path attacker needs a sequence number inside the receive window to land a spoofed RST or segment, and nine bits of bias is nine bits fewer guesses. RFC 6528 asks that an ISN not be predictable. This does not implement 6528's four-tuple hash; it only removes a bias in a value the code already meant to be random.

Found while auditing SYN traces from an m68k AmigaOS port: bits 16-30 were set in 35 of 45 SYNs (0.78, predicted 0.75) before the change and 69 of 135 (0.51, predicted 0.50) after.

[fdesbiens]

Thank you — this is an unusually well-argued two-line change, and it is right. I checked the analysis rather than taking it on trust, and everything lines up.

The bit-level model is exact. With RAND_MAX = 0x7FFFFFFF the first draw contributes bits 16-31 and the second bits 0-30, so bits 0-15 come from the second draw alone, bit 31 from the first alone, and bits 16-30 are an OR of two independent bits. Because each output bit draws on a distinct pair of source bits, all 32 output bits are independent, which makes the closed form exact rather than approximate:

                 Shannon   min-entropy   likeliest ISN vs uniform
  OR   analytic   29.169        23.226                      438x
  ADD  analytic   32.000        32.000                        1x
  PR (2e7 samp)   29.168        23.220                      440x

Your measured figures differ from the closed form only in the last digit or two, which is what sampling error at 2e7 draws should look like.

And the uniformity claim for + is exactly right, for a reason worth putting in the commit message. (a << 16) & 0xFFFFFFFF is uniform over the 65536 multiples of 65536 — that is, uniform on a subgroup of the additive group mod 2^32. Adding an independent value whose residue mod 2^16 is uniform therefore lands uniformly on the whole group. That is why the result is exactly uniform and not merely closer to uniform, and it holds whenever RAND_MAX + 1 is a multiple of 2^16.

Empirically confirmed on 4M draws from a 31-bit source, bucketed 4096 ways. Chi-square against an expectation of ~4096: OR gives 4.2e7 (emphatically non-uniform), + gives 4029 (indistinguishable from uniform). Sampled overlap bits came out at P(1) = 0.7499 to 0.7500, matching the predicted 3/4.

Three more things I verified so they are on the record:

Your consistency argument is real. The else branch at nxd_tcp_client_socket_connect.c:418 is tx_sequence + 0x10000 + NX_RAND(), so the reuse path does already add. Making the fresh path agree is the right kind of change.

These are the only two instances. I swept common/src, addons and nx_secure/src for the two-draw OR pattern and found nothing else. The superficially similar line at nx_ip_header_add.c:149 ORs a random IP identification field with the fragment bits, which occupy disjoint halves — a correct use of OR, not the same bug.

No test depends on the composition. Eight tests under test/regression/netxduo_test reference nx_tcp_socket_tx_sequence, and all of them read the socket's value at runtime and use it relatively — comparing it, echoing it back in an ACK, or offsetting by one. None hardcodes an expected ISN, so none is sensitive to this.

I also went looking for an overflow problem and did not find one, which is worth stating because the pre-existing & 0xFFFFFFFF on the preceding line invites the worry: if ULONG were ever 64 bits, += could produce a value above 0xFFFFFFFF where |= could not. It cannot happen — ThreadX pins ULONG to 32 bits on every port, including an explicit #if defined(__x86_64__) special case in the Linux port that keeps it as unsigned int under LP64. So the addition wraps modulo 2^32, which is precisely what the uniformity argument requires.

I will merge this, but please address my detailed comments if possible.

[fdesbiens]

Your analysis assumes RAND_MAX = 0x7FFFFFFF, which is right for glibc and for the AmigaOS port you audited. It is worth noting what happens on ports where it is not, because a reader elsewhere may look for the improvement in their own traces and not find it.

(Also, as an aside, I am genuinely intrigued to see AmigaOS mentioned here. This is likely a first! :-) I never owned an Amiga but, as any software gray/bald head, I have great respect for the platform.)

The C standard only guarantees RAND_MAX >= 32767, and 0x7FFF is what MSVC and a good number of embedded libcs return. With a 15-bit draw the two values occupy bits 16-30 and bits 0-14 — they do not overlap at all. No overlap means no OR bias, and it also means no carries, so | and + produce bit-identical results. I confirmed that over 200000 draws: identical in 200000 of 200000.

So the honest summary is that this change is a strict improvement wherever rand() yields more than 16 bits, a no-op where it yields 16 or fewer, and a regression nowhere. That is a stronger claim than the PR currently makes and costs one sentence.

Worth flagging separately, and explicitly not something I want you to fix here: on those same 15-bit ports the ISN spans only 30 bits, with bits 15 and 31 permanently zero. That is a larger weakness than the OR bias and neither form addresses it — it needs a wider draw, which is a different change with different portability arguments. If you would like to open an issue for it, I would be glad to have it recorded, but it should not be attached to this PR.

[fdesbiens]

Purely stylistic, and I am happy either way.

The first line carries an explicit & 0xFFFFFFFF, which documents an intent to keep the value inside 32 bits. With |= the second line could not violate that. With += it relies on ULONG being exactly 32 bits so that the wrap does the masking implicitly — which, as noted above, is true on every port today.

Two ways to make the invariant visible rather than implicit: mask the result as well,

    socket_ptr -> nx_tcp_socket_tx_sequence += (ULONG)NX_RAND();
    socket_ptr -> nx_tcp_socket_tx_sequence &= 0xFFFFFFFF;

or drop the mask from the first line too, on the grounds that it was already redundant. I lean slightly towards the first, because it survives someone later introducing a port with a wider ULONG, and because the mask on line one will otherwise read as protecting something it no longer protects. But it is genuinely a matter of taste and I would not hold the PR for it.

[fdesbiens]

Noting this only because it is a real consequence of moving to a uniform distribution, and I would rather it be written down than discovered later. I am not asking for a change.

nx_tcp_socket_tx_sequence == 0 is the sentinel that distinguishes a fresh socket from a reused one, tested immediately above at :106 and at :411 in the client path. Under the OR composition an ISN of exactly zero required all 17 half-probability bits and all 15 three-quarter bits to come up zero, so P was 2^-47 — effectively never. Under a uniform distribution it is 2^-32.

The consequence if it ever happens is benign: the next connect on that socket takes the fresh-ISN branch instead of the + 0x10000 + rand() increment branch, so it gets a freshly randomised ISN rather than an incremented one. That is arguably the better of the two outcomes, and at 2^-32 per connection it is not worth code to avoid. It is simply no longer impossible.

[fdesbiens]

I have asked for regression tests on most of the PRs I have reviewed this week, so it is worth explaining why not here rather than leaving an apparent inconsistency.

The property this change fixes is statistical, not functional. A test that draws ISNs and asserts uniformity has to pick a significance threshold, and at any threshold loose enough to avoid flaking it would also pass the biased version for small sample counts — so it would either be flaky or vacuous. A test that pins specific ISN values would fix the composition in place and make future improvements harder, which is the opposite of useful.

The right artefact for a change like this is the analysis, and yours is better than a test would be.