From eab43c4653d750a08a77317d3be200e7dfd372f2 Mon Sep 17 00:00:00 2001 From: Pat Whittingslow Date: Fri, 27 Feb 2026 16:38:46 +0100 Subject: [PATCH] tcp: Reject SYN on synchronized connections; fix maxSend underflow (#44) * tcp: add test replicating bugs and 32bit panic with GOARCH=386 * tcp: reject SYN on synchronized connections; fix maxSend underflow --- internal/tap.go | 2 +- tcp/control.go | 21 +++- tcp/control_rcvhandlers.go | 1 - tcp/seqsbug_test.go | 224 +++++++++++++++++++++++++++++++++++++ 4 files changed, 242 insertions(+), 6 deletions(-) create mode 100644 tcp/seqsbug_test.go diff --git a/internal/tap.go b/internal/tap.go index 6923f9b..1e51193 100644 --- a/internal/tap.go +++ b/internal/tap.go @@ -1,4 +1,4 @@ -//go:build linux && !tinygo +//go:build linux && !tinygo && amd64 package internal diff --git a/tcp/control.go b/tcp/control.go index 72183d4..78d52fd 100644 --- a/tcp/control.go +++ b/tcp/control.go @@ -134,7 +134,12 @@ func (snd *sendSpace) inFlight() Size { // maxSend returns maximum segment datalength receivable by remote peer. func (snd *sendSpace) maxSend() Size { - return snd.WND - snd.inFlight() + if inf := snd.inFlight(); inf >= snd.WND { + // Guard uint32 underflow when window shrinks below inflight. + return 0 + } else { + return snd.WND - inf + } } // recvSpace contains Receive Sequence Space data. Its sequence numbers correspond to remote data. @@ -252,6 +257,13 @@ func (tcb *ControlBlock) Recv(seg Segment) (err error) { return err } + // RFC 9293 §3.10.7.4: SYN on synchronized connection → challenge ACK. + if seg.Flags.HasAny(FlagSYN) && !tcb._state.IsPreestablished() { + tcb.challengeAck = true + tcb.pending[0] |= FlagACK + return errDropSegment + } + prevNxt := tcb.snd.NXT var pending Flags switch tcb._state { @@ -402,10 +414,11 @@ func (tcb *ControlBlock) validateOutgoingSegment(seg Segment) (err error) { func (tcb *ControlBlock) validateIncomingSegment(seg Segment) (err error) { flags := seg.Flags hasAck := flags.HasAll(FlagACK) - // Short circuit SEQ checks if SYN present since the incoming segment initialize1s connection. - checkSEQ := !flags.HasAny(FlagSYN) - established := tcb._state == StateEstablished + // Short circuit SEQ checks if SYN present in pre-established states only. + // In synchronized states SYN must pass normal SEQ validation (RFC 9293 §3.10.7.4). preestablished := tcb._state.IsPreestablished() + checkSEQ := !flags.HasAny(FlagSYN) || !preestablished + established := tcb._state == StateEstablished acksOld := hasAck && !tcb.snd.UNA.LessThan(seg.ACK) acksUnsentData := hasAck && !seg.ACK.LessThanEq(tcb.snd.NXT) ctlOrDataSegment := established && (seg.DATALEN > 0 || flags.HasAny(FlagFIN|FlagRST)) diff --git a/tcp/control_rcvhandlers.go b/tcp/control_rcvhandlers.go index 1358f38..9e7560b 100644 --- a/tcp/control_rcvhandlers.go +++ b/tcp/control_rcvhandlers.go @@ -62,7 +62,6 @@ func (tcb *ControlBlock) rcvSynRcvd(seg Segment) (pending Flags, err error) { func (tcb *ControlBlock) rcvEstablished(seg Segment) (pending Flags, err error) { flags := seg.Flags - dataToAck := seg.DATALEN > 0 hasFin := flags.HasAny(FlagFIN) if dataToAck || hasFin { diff --git a/tcp/seqsbug_test.go b/tcp/seqsbug_test.go new file mode 100644 index 0000000..1daf0b6 --- /dev/null +++ b/tcp/seqsbug_test.go @@ -0,0 +1,224 @@ +package tcp + +// Tests for the "seqs: bad calculation" panic bug caused by a retransmitted SYN +// being accepted on an ESTABLISHED connection. +// +// Bug 1: validateIncomingSegment skips SEQ checks for any SYN, even in synchronized states. +// Bug 2: rcvEstablished silently accepts bare SYN segments (no data, no FIN). +// Combined: SYN overwrites snd.WND with a small value while data is in flight, causing +// uint32 underflow in maxSend() → panic in PendingSegment. + +import "testing" + +// TestSYNOnEstablished_PanicRegression replicates the exact crash sequence from the bug report: +// a retransmitted SYN arrives on an ESTABLISHED connection with 1200 bytes in flight, +// clobbers snd.WND to 1025, and triggers "seqs: bad calculation" in PendingSegment. +// To trigger run with GOARCH=386 +func TestSYNOnEstablished_PanicRegression(t *testing.T) { + // Values from the bug report timeline. + const ( + serverISS Value = 155000 + clientISS Value = 4189094524 // The SYN's sequence number from the log. + serverWND Size = 1024 + clientWND Size = 1025 // The SYN's small window. + payloadSent Size = 1200 // HTTP response bytes in flight. + ) + + // Recv must reject the SYN on an ESTABLISHED connection. + t.Run("Recv_rejects_SYN", func(t *testing.T) { + var tcb ControlBlock + tcb.HelperInitState(StateEstablished, serverISS, serverISS+1+Value(payloadSent), serverWND) + tcb.HelperInitRcv(clientISS, clientISS+1, 65535) + tcb.snd.UNA = serverISS + 1 + + syn := Segment{ + SEQ: clientISS, + ACK: 0, + Flags: FlagSYN, + WND: clientWND, + } + + err := tcb.Recv(syn) + if err == nil { + t.Fatal("Recv accepted SYN on ESTABLISHED connection; expected error") + } + if tcb.State() != StateEstablished { + t.Fatalf("state changed to %s; want ESTABLISHED", tcb.State()) + } + if tcb.snd.WND == clientWND { + t.Fatalf("snd.WND was clobbered to %d by the SYN segment", clientWND) + } + }) + + // Directly simulate the corrupted state to trigger the panic in PendingSegment. + // This is the actual crash path: inFlight=1200, snd.WND=1025 → maxSend() underflows. + // On 32-bit targets int(underflowedUint32) wraps negative → panic. + t.Run("PendingSegment_panic", func(t *testing.T) { + var tcb ControlBlock + tcb.HelperInitState(StateEstablished, serverISS, serverISS+1+Value(payloadSent), serverWND) + tcb.HelperInitRcv(clientISS, clientISS+1, 65535) + tcb.snd.UNA = serverISS + 1 + + // Simulate what Recv does when the SYN is accepted (the bug): + // snd.WND gets overwritten with the SYN's small window. + tcb.snd.WND = clientWND // inFlight=1200 > snd.WND=1025 → underflow. + + defer func() { + if r := recover(); r != nil { + t.Errorf("confirmed panic: %v", r) + } + }() + tcb.PendingSegment(1) // Any payload > 0 triggers the panic. + }) +} + +// TestSYNOnEstablished_SEQValidation verifies that SYN segments are subject to +// sequence number validation in synchronized states (Bug 1 from the report). +func TestSYNOnEstablished_SEQValidation(t *testing.T) { + const ( + iss Value = 100 + remoteISS Value = 5000 + window Size = 1000 + ) + var tcb ControlBlock + tcb.HelperInitState(StateEstablished, iss, iss, window) + tcb.HelperInitRcv(remoteISS, remoteISS, window) + + // SYN with SEQ outside the receive window should be rejected. + outOfWindowSYN := Segment{ + SEQ: remoteISS - 2000, // Way before rcv.NXT. + Flags: FlagSYN, + WND: 1025, + } + + err := tcb.Recv(outOfWindowSYN) + if err == nil { + t.Fatal("SYN with out-of-window SEQ was accepted in ESTABLISHED state") + } + + // SYN with SEQ inside the window but not equal to rcv.NXT should also be rejected + // (we require sequential segments per SHLD-31). + inWindowSYN := Segment{ + SEQ: remoteISS + 1, // In window but not NXT. + Flags: FlagSYN, + WND: 1025, + } + + err = tcb.Recv(inWindowSYN) + if err == nil { + t.Fatal("SYN with in-window non-NXT SEQ was accepted in ESTABLISHED state") + } +} + +// TestSYNOnEstablished_ChallengeACK verifies that per RFC 9293 §3.10.7.4, +// receiving a SYN on an ESTABLISHED connection results in a challenge ACK +// rather than silently accepting or resetting (Bug 2 from the report). +func TestSYNOnEstablished_ChallengeACK(t *testing.T) { + const ( + issA Value = 100 + issB Value = 300 + window Size = 1000 + ) + var tcb ControlBlock + tcb.HelperInitState(StateEstablished, issA, issA, window) + tcb.HelperInitRcv(issB, issB, window) + + // SYN at exact rcv.NXT (in-window). + syn := Segment{ + SEQ: issB, + Flags: FlagSYN, + WND: 512, + } + + err := tcb.Recv(syn) + // Must be rejected (either via validation or rcvEstablished). + if err == nil { + t.Fatal("SYN at rcv.NXT was accepted in ESTABLISHED state; expected rejection") + } + + // Connection must stay ESTABLISHED. + if tcb.State() != StateEstablished { + t.Fatalf("state = %s; want ESTABLISHED", tcb.State()) + } + + // snd.WND must be preserved. + if tcb.snd.WND == 512 { + t.Fatal("snd.WND was overwritten by the SYN's window value") + } + + // A challenge ACK should be pending. + seg, ok := tcb.PendingSegment(0) + if !ok { + t.Fatal("no pending segment after SYN on ESTABLISHED; expected challenge ACK") + } + if seg.Flags != FlagACK { + t.Errorf("pending flags = %s; want ACK (challenge ACK)", seg.Flags) + } + if seg.SEQ != issA { + t.Errorf("challenge ACK SEQ = %d; want snd.NXT=%d", seg.SEQ, issA) + } + if seg.ACK != issB { + t.Errorf("challenge ACK ACK = %d; want rcv.NXT=%d", seg.ACK, issB) + } +} + +// TestSYNOnClosingStates verifies SYN rejection in other synchronized states +// beyond ESTABLISHED (FIN-WAIT-1, FIN-WAIT-2, CLOSE-WAIT). +func TestSYNOnClosingStates(t *testing.T) { + states := []State{StateFinWait1, StateFinWait2, StateCloseWait} + for _, state := range states { + t.Run(state.String(), func(t *testing.T) { + const iss, remoteISS, window Size = 100, 300, 1000 + var tcb ControlBlock + tcb.HelperInitState(state, Value(iss), Value(iss), window) + tcb.HelperInitRcv(Value(remoteISS), Value(remoteISS), window) + + syn := Segment{ + SEQ: Value(remoteISS), + Flags: FlagSYN, + WND: 512, + } + + err := tcb.Recv(syn) + if err == nil { + t.Fatalf("SYN accepted in %s state; expected rejection", state) + } + if tcb.State() != state { + t.Fatalf("state changed from %s to %s after SYN", state, tcb.State()) + } + }) + } +} + +// TestSYNPreestablished_StillAllowed ensures the fix doesn't break normal SYN +// processing in pre-established states (LISTEN, SYN-SENT, SYN-RCVD). +func TestSYNPreestablished_StillAllowed(t *testing.T) { + const issA, issB, windowA, windowB = 100, 300, 1000, 1000 + + t.Run("LISTEN", func(t *testing.T) { + var tcb ControlBlock + tcb.HelperInitState(StateListen, issA, issA, windowA) + syn := Segment{SEQ: issB, Flags: FlagSYN, WND: windowB} + err := tcb.Recv(syn) + if err != nil { + t.Fatalf("SYN rejected in LISTEN state: %v", err) + } + if tcb.State() != StateSynRcvd { + t.Fatalf("state = %s; want SYN-RCVD", tcb.State()) + } + }) + + t.Run("SYN-SENT", func(t *testing.T) { + var tcb ControlBlock + tcb.HelperInitState(StateSynSent, issA, issA, windowA) + // Simultaneous open: receive SYN from peer. + syn := Segment{SEQ: issB, Flags: FlagSYN, WND: windowB} + err := tcb.Recv(syn) + if err != nil { + t.Fatalf("SYN rejected in SYN-SENT state: %v", err) + } + if tcb.State() != StateSynRcvd { + t.Fatalf("state = %s; want SYN-RCVD", tcb.State()) + } + }) +}