Skip to content

Removed bias from the randomised TCP initial sequence number #408

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fdesbiens merged 2 commits into from
Aug 13, 2026
Merged

Removed bias from the randomised TCP initial sequence number #408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f829a5c
tcp: remove bias from the randomised initial sequence number
tinic Jul 26, 2026
2382dcb
Masked the initial sequence number after the addition, not only before
fdesbiens Aug 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion common/src/nx_tcp_server_socket_accept.c
View file
Open in desktop

@fdesbiens fdesbiens Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 fdesbiens Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,8 @@ NX_IP *ip_ptr;
if (socket_ptr -> nx_tcp_socket_tx_sequence == 0)
{
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();
socket_ptr -> nx_tcp_socket_tx_sequence += (ULONG)NX_RAND();

@fdesbiens fdesbiens Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

socket_ptr -> nx_tcp_socket_tx_sequence &= 0xFFFFFFFF;
}
else
{
Expand Down
3 changes: 2 additions & 1 deletion common/src/nxd_tcp_client_socket_connect.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,8 @@ ULONG ip_address_log = 0;
if (socket_ptr -> nx_tcp_socket_tx_sequence == 0)
{
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();
socket_ptr -> nx_tcp_socket_tx_sequence += (ULONG)NX_RAND();

@fdesbiens fdesbiens Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 fdesbiens Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented my preferred style in 2382dcb.

socket_ptr -> nx_tcp_socket_tx_sequence &= 0xFFFFFFFF;
}
else
{
Expand Down