Skip to content

nstun: validate TCP acknowledgments before processing payload#279

Open
Alearner12 wants to merge 1 commit into
google:masterfrom
Alearner12:fix-nstun-tcp-ack-validation
Open

nstun: validate TCP acknowledgments before processing payload#279
Alearner12 wants to merge 1 commit into
google:masterfrom
Alearner12:fix-nstun-tcp-ack-validation

Conversation

@Alearner12

Copy link
Copy Markdown

nstun: validate TCP acknowledgments before processing payload

Summary

This change makes nstun validate guest TCP acknowledgments before using them to
mutate send-side state or deliver segment payload.

  • Classifies ACKs with wraparound-safe TCP serial-number arithmetic.
  • Rejects ACKs beyond SND.NXT before processing segment text.
  • Applies the same ACK acceptability checks in SYN-SENT.
  • Accounts for SYN, buffered data, and FIN sequence space before committing a
    cumulative ACK.
  • Uses wraparound-safe sequence comparison in the retransmission timer.
  • Makes flow destruction from push_to_guest() explicit to callers.
  • Adds focused helper and production-path regression tests.

Bug

The synchronized-state receive path previously processed segment payload before
validating SEG.ACK. A guest could therefore attach payload to an ACK beyond
SND.NXT; the payload could reach the host-facing receive path before the
invalid acknowledgment was reflected in send-side accounting.

The old ACK accounting then calculated an advance directly from the supplied
32-bit ACK and updated ack_from_guest and tx_acked_offset. An ACK outside the
current send window could consequently move the flow beyond the data actually
sent or buffered.

SYN-SENT also accepted acknowledgment state without applying the same send
window invariant. Separately, the retransmission timer compared sequence
numbers with ordinary integer ordering, which is incorrect when the sequence
space wraps at UINT32_MAX.

Fix

The receive path now classifies every ACK relative to SND.UNA and SND.NXT
before processing payload:

  • Old and duplicate ACKs leave send-side state unchanged.
  • Advancing ACKs are applied only after their SYN, data, and FIN sequence-space
    consumption is consistent with the transmit buffer.
  • Future ACKs leave the flow unchanged, drop any attached payload, and produce
    a corrective ACK.
  • Internally inconsistent buffer accounting resets and destroys the flow
    instead of partially committing state.

SYN-SENT rejects unacceptable acknowledgments before performing RST or
SYN|ACK processing. An unacceptable ACK without RST produces the reset response
specified for that state, while an acceptable inbound SYN|ACK completes the
transition to ESTABLISHED.

The defensive buffer checks mean push_to_guest() can synchronously destroy a
flow. Its return value now reports that invalidation, and callers stop using the
flow immediately. Garbage collection snapshots active flow pointers before
running callbacks so callback-driven erasure does not invalidate the map
iteration itself.

Regression coverage

tests/tcp_state_test.cc covers:

  • Old, duplicate, advancing, and future ACK classification.
  • TCP sequence-number wraparound.
  • Partial and complete cumulative ACK accounting.
  • Repeated future ACKs leaving send state and buffered data unchanged.
  • Rejection of ACK advances inconsistent with the transmit buffer.
  • SYN and FIN sequence-space accounting.

tests/tcp_process_test.cc exercises the actual TCP processing implementation
with external I/O stubbed. It verifies that:

  • A future ACK carrying ABC does not change ack_from_guest,
    seq_to_guest, tx_acked_offset, or tx_buffer.
  • The attached payload is not delivered to the host-facing receive buffer.
  • The flow remains owned and exactly one payload-free corrective ACK is sent.
  • An unacceptable ACK in SYN-SENT preserves the pending flow and emits an RST
    whose sequence number is copied from SEG.ACK.
  • A valid inbound SYN|ACK transitions the flow to ESTABLISHED and emits the
    expected final ACK.
  • The destructive push_to_guest() path reports destruction without callers
    subsequently accessing the invalidated pointer.

Verification

Local verification performed:

  • make tcp_state_test
  • make tcp_process_test
  • Both focused tests with GCC ASan/UBSan
  • Both focused tests with Clang
  • GCC object compilation of nstun/tcp.cc
  • GCC object compilation of nstun/nstun.cc
  • git diff --check

The full nsjail binary was not linked locally because this checkout does not
contain the kafel submodule and network access was unavailable. The focused
tests and directly affected translation units compile successfully.

@Alearner12 Alearner12 changed the title Fix nstun TCP ACK validation nstun: validate TCP acknowledgments before processing payload Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant