|
* tcp: remove bias from the randomised initial sequence number
_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.
* Masked the initial sequence number after the addition, not only before
The first line of each pair carries an explicit & 0xFFFFFFFF, which documents the
intent to keep the sequence number inside 32 bits. While the second line was |=,
that mask could not be violated. Now that it is +=, staying inside 32 bits
depends on ULONG being exactly 32 bits so the wrap does the masking implicitly.
That holds on every port today, ThreadX pinning ULONG to 32 bits everywhere, but
it is an implicit dependency where the line above states an explicit one, and the
mask on the first line would otherwise read as protecting something it no longer
protects.
Mask the result as well, at both sites. On a 32-bit ULONG this is a no-op the
compiler discards; on a hypothetical port with a wider ULONG it keeps the
invariant the first line already claims.
No behaviour change. Verified that the library builds and that three regression
tests which read nx_tcp_socket_tx_sequence still pass: netx_tcp_4_duplicate_ack_test,
netx_tcp_new_reno_algorithm_test2 and netx_tcp_overlapping_packet_test_2. A
two-million-draw model of the three lines with a 32-bit ULONG gives a mean of
2146460131 against the uniform 2147483648, so the distribution the PR set out to
fix is unaffected.
---------
Signed-off-by: Tinic Uro <[email protected]>
Co-authored-by: Frédéric Desbiens <[email protected]>
Assisted-by: Claude Code (Opus 5) <[email protected]>
|
