Ignore RST received for a TCP listener

The current implementation has a bug where TCP listener does not ignore
RSTs from the peer. While handling RST+ACK from the peer, this bug can
complete handshakes that use syncookies. This results in half-open
connection delivered to the accept queue.

Fixes #6076

PiperOrigin-RevId: 376868749

1 files changed, 96 insertions, 31 deletions

diff --git a/test/packetimpact/tests/tcp_syncookie_test.go b/test/packetimpact/tests/tcp_syncookie_test.go
index 1c21c62ff..1a016bd1a 100644
--- a/test/packetimpact/tests/tcp_syncookie_test.go
+++ b/test/packetimpact/tests/tcp_syncookie_test.go
@@ -16,6 +16,8 @@ package tcp_syncookie_test
 
 import (
 	"flag"
+	"fmt"
+	"math"
 	"testing"
 	"time"
 
@@ -28,43 +30,106 @@ func init() {
 	testbench.Initialize(flag.CommandLine)
 }
 
-// TestSynCookie test if the DUT listener is replying back using syn cookies.
-// The test does not complete the handshake by not sending the ACK to SYNACK.
-// When syncookies are not used, this forces the listener to retransmit SYNACK.
-// And when syncookies are being used, there is no such retransmit.
+// TestTCPSynCookie tests for ACK handling for connections in SYNRCVD state
+// connections with and without syncookies. It verifies if the passive open
+// connection is indeed using syncookies before proceeding.
 func TestTCPSynCookie(t *testing.T) {
 	dut := testbench.NewDUT(t)
+	for _, tt := range []struct {
+		accept bool
+		flags  header.TCPFlags
+	}{
+		{accept: true, flags: header.TCPFlagAck},
+		{accept: true, flags: header.TCPFlagAck | header.TCPFlagPsh},
+		{accept: false, flags: header.TCPFlagAck | header.TCPFlagSyn},
+		{accept: true, flags: header.TCPFlagAck | header.TCPFlagFin},
+		{accept: false, flags: header.TCPFlagAck | header.TCPFlagRst},
+		{accept: false, flags: header.TCPFlagRst},
+	} {
+		t.Run(fmt.Sprintf("flags=%s", tt.flags), func(t *testing.T) {
+			// Make a copy before parallelizing the test and refer to that
+			// within the test. Otherwise, the test reference could be pointing
+			// to an incorrect variant based on how it is scheduled.
+			test := tt
 
-	// Listening endpoint accepts one more connection than the listen backlog.
-	_, remotePort := dut.CreateListener(t, unix.SOCK_STREAM, unix.IPPROTO_TCP, 1 /*backlog*/)
+			t.Parallel()
 
-	var withoutSynCookieConn testbench.TCPIPv4
-	var withSynCookieConn testbench.TCPIPv4
+			// Listening endpoint accepts one more connection than the listen
+			// backlog. Listener starts using syncookies when it sees a new SYN
+			// and has backlog size of connections in SYNRCVD state. Keep the
+			// listen backlog 1, so that the test can define 2 connections
+			// without and with using syncookies.
+			listenFD, remotePort := dut.CreateListener(t, unix.SOCK_STREAM, unix.IPPROTO_TCP, 1 /*backlog*/)
+			defer dut.Close(t, listenFD)
 
-	// Test if the DUT listener replies to more SYNs than listen backlog+1
-	for _, conn := range []*testbench.TCPIPv4{&withoutSynCookieConn, &withSynCookieConn} {
-		*conn = dut.Net.NewTCPIPv4(t, testbench.TCP{DstPort: &remotePort}, testbench.TCP{SrcPort: &remotePort})
-	}
-	defer withoutSynCookieConn.Close(t)
-	defer withSynCookieConn.Close(t)
+			var withoutSynCookieConn testbench.TCPIPv4
+			var withSynCookieConn testbench.TCPIPv4
 
-	checkSynAck := func(t *testing.T, conn *testbench.TCPIPv4, expectRetransmit bool) {
-		// Expect dut connection to have transitioned to SYN-RCVD state.
-		conn.Send(t, testbench.TCP{Flags: testbench.TCPFlags(header.TCPFlagSyn)})
-		if _, err := conn.ExpectData(t, &testbench.TCP{Flags: testbench.TCPFlags(header.TCPFlagSyn | header.TCPFlagAck)}, nil, time.Second); err != nil {
-			t.Fatalf("expected SYN-ACK, but got %s", err)
-		}
+			for _, conn := range []*testbench.TCPIPv4{&withoutSynCookieConn, &withSynCookieConn} {
+				*conn = dut.Net.NewTCPIPv4(t, testbench.TCP{DstPort: &remotePort}, testbench.TCP{SrcPort: &remotePort})
+			}
+			defer withoutSynCookieConn.Close(t)
+			defer withSynCookieConn.Close(t)
 
-		// If the DUT listener is using syn cookies, it will not retransmit SYNACK
-		got, err := conn.ExpectData(t, &testbench.TCP{SeqNum: testbench.Uint32(uint32(*conn.RemoteSeqNum(t) - 1)), Flags: testbench.TCPFlags(header.TCPFlagSyn | header.TCPFlagAck)}, nil, 2*time.Second)
-		if expectRetransmit && err != nil {
-			t.Fatalf("expected retransmitted SYN-ACK, but got %s", err)
-		}
-		if !expectRetransmit && err == nil {
-			t.Fatalf("expected no retransmitted SYN-ACK, but got %s", got)
-		}
-	}
+			// Setup the 2 connections in SYNRCVD state and verify if one of the
+			// connection is indeed using syncookies by checking for absence of
+			// SYNACK retransmits.
+			for _, c := range []struct {
+				desc             string
+				conn             *testbench.TCPIPv4
+				expectRetransmit bool
+			}{
+				{desc: "without syncookies", conn: &withoutSynCookieConn, expectRetransmit: true},
+				{desc: "with syncookies", conn: &withSynCookieConn, expectRetransmit: false},
+			} {
+				t.Run(c.desc, func(t *testing.T) {
+					// Expect dut connection to have transitioned to SYNRCVD state.
+					c.conn.Send(t, testbench.TCP{Flags: testbench.TCPFlags(header.TCPFlagSyn)})
+					if _, err := c.conn.ExpectData(t, &testbench.TCP{Flags: testbench.TCPFlags(header.TCPFlagSyn | header.TCPFlagAck)}, nil, time.Second); err != nil {
+						t.Fatalf("expected SYNACK, but got %s", err)
+					}
+
+					// If the DUT listener is using syn cookies, it will not retransmit SYNACK.
+					got, err := c.conn.ExpectData(t, &testbench.TCP{SeqNum: testbench.Uint32(uint32(*c.conn.RemoteSeqNum(t) - 1)), Flags: testbench.TCPFlags(header.TCPFlagSyn | header.TCPFlagAck)}, nil, 2*time.Second)
+					if c.expectRetransmit && err != nil {
+						t.Fatalf("expected retransmitted SYNACK, but got %s", err)
+					}
+					if !c.expectRetransmit && err == nil {
+						t.Fatalf("expected no retransmitted SYNACK, but got %s", got)
+					}
+				})
+			}
 
-	t.Run("without syncookies", func(t *testing.T) { checkSynAck(t, &withoutSynCookieConn, true /*expectRetransmit*/) })
-	t.Run("with syncookies", func(t *testing.T) { checkSynAck(t, &withSynCookieConn, false /*expectRetransmit*/) })
+			// Check whether ACKs with the given flags completes the handshake.
+			for _, c := range []struct {
+				desc string
+				conn *testbench.TCPIPv4
+			}{
+				{desc: "with syncookies", conn: &withSynCookieConn},
+				{desc: "without syncookies", conn: &withoutSynCookieConn},
+			} {
+				t.Run(c.desc, func(t *testing.T) {
+					pfds := dut.Poll(t, []unix.PollFd{{Fd: listenFD, Events: math.MaxInt16}}, 0 /*timeout*/)
+					if got, want := len(pfds), 0; got != want {
+						t.Fatalf("dut.Poll(...) = %d, want = %d", got, want)
+					}
+
+					c.conn.Send(t, testbench.TCP{Flags: testbench.TCPFlags(test.flags)})
+					pfds = dut.Poll(t, []unix.PollFd{{Fd: listenFD, Events: unix.POLLIN}}, time.Second)
+					want := 0
+					if test.accept {
+						want = 1
+					}
+					if got := len(pfds); got != want {
+						t.Fatalf("got dut.Poll(...) = %d, want = %d", got, want)
+					}
+					// Accept the connection to enable poll on any subsequent connection.
+					if test.accept {
+						fd, _ := dut.Accept(t, listenFD)
+						dut.Close(t, fd)
+					}
+				})
+			}
+		})
+	}
 }