Always perform NAT in NAT-supported hooks

This avoids a race condition when a packet is being written and the NAT
table is being updated at the same time.

Previously, NAT will only be skipped if either the connection has been
finalized or the hook's relevant NAT (DNAT for Prerouting/Output; SNAT
for Input/Postrouting) has been performed. However, it is possible for
the following sequence of events to occur:

  1) A packet performs DNAT related hooks in Prerouting or Output
     but not perform DNAT as no rule matched the packet.
  2) The NAT table updates such that a DNAT rule now will be performed
     on packets matching the packet's tuple from (1).
  3) A second packet matching the original packet's tuple performs
     the Prerouting or Output hook, now having performed DNAT and
     updating the connection.
  4) Either packet goes through the other hooks and finalizes the
     connection.

Here we would have 2 packets that have the same original tuple but have
different destination address/ports after performing all the hooks.
Later packets will look like the second packet in the example but the
first packet may trigger a response that the connection table will not
recognize, potentially leading to an ICMP error or TCP RST.

A similar race exists for SNAT.

To avoid the race, this change guarantees that {D,S}NAT is always
performed on a connection before leaving the relevant hook. This
way we make sure that all packets that are associated with a
connection will have the same tuple, per direction.

PiperOrigin-RevId: 410338441

pkg/tcpip/stack/conntrack.go

@@ -95,6 +95,19 @@ func (ti tupleID) reply() tupleID {
 	}
 }
 
+type manipType int
+
+const (
+	// manipNotPerformed indicates that NAT has not been performed.
+	manipNotPerformed manipType = iota
+
+	// manipPerformed indicates that NAT was performed.
+	manipPerformed
+
+	// manipPerformedNoop indicates that NAT was performed but it was a no-op.
+	manipPerformedNoop
+)
+
 // conn is a tracked connection.
 //
 // +stateify savable
@@ -112,14 +125,14 @@ type conn struct {
 	//
 	// +checklocks:mu
 	finalized bool
-	// sourceManip indicates the packet's source is manipulated.
+	// sourceManip indicates the source manipulation type.
 	//
 	// +checklocks:mu
-	sourceManip bool
-	// destinationManip indicates the packet's destination is manipulated.
+	sourceManip manipType
+	// destinationManip indicates the destination's manipulation type.
 	//
 	// +checklocks:mu
-	destinationManip bool
+	destinationManip manipType
 
 	stateMu sync.RWMutex `state:"nosave"`
 	// tcb is TCB control block. It is used to keep track of states
@@ -539,13 +552,20 @@ func (cn *conn) finalize() {
 	cn.ct.finalize(cn)
 }
 
-// performNAT setups up the connection for the specified NAT.
-//
-// Generally, only the first packet of a connection reaches this method; other
-// other packets will be manipulated without needing to modify the connection.
-func (cn *conn) performNAT(pkt *PacketBuffer, hook Hook, r *Route, ports portRange, address tcpip.Address, dnat bool) {
-	cn.performNATIfNoop(ports, address, dnat)
-	cn.handlePacket(pkt, hook, r)
+func (cn *conn) maybePerformNoopNAT(dnat bool) {
+	cn.mu.Lock()
+	defer cn.mu.Unlock()
+
+	var manip *manipType
+	if dnat {
+		manip = &cn.destinationManip
+	} else {
+		manip = &cn.sourceManip
+	}
+
+	if *manip == manipNotPerformed {
+		*manip = manipPerformedNoop
+	}
 }
 
 type portRange struct {
@@ -553,36 +573,49 @@ type portRange struct {
 	size  uint16
 }
 
-func (cn *conn) performNATIfNoop(ports portRange, address tcpip.Address, dnat bool) {
+// performNAT setups up the connection for the specified NAT and rewrites the
+// packet.
+//
+// If NAT has already been performed on the connection, then the packet will
+// be rewritten with the NAT performed on the connection, ignoring the passed
+// address and port range.
+//
+// Generally, only the first packet of a connection reaches this method; other
+// packets will be manipulated without needing to modify the connection.
+func (cn *conn) performNAT(pkt *PacketBuffer, hook Hook, r *Route, ports portRange, natAddress tcpip.Address, dnat bool) {
+	// Make sure the packet is re-written after performing NAT.
+	defer func() {
+		// handlePacket returns true if the packet may skip the NAT table as the
+		// connection is already NATed, but if we reach this point we must be in the
+		// NAT table, so the return value is useless for us.
+		_ = cn.handlePacket(pkt, hook, r)
+	}()
+
 	cn.mu.Lock()
 	defer cn.mu.Unlock()
 
-	if cn.finalized {
-		return
-	}
-
 	cn.reply.mu.Lock()
 	defer cn.reply.mu.Unlock()
 
+	var manip *manipType
+	var address *tcpip.Address
 	var port *uint16
 	if dnat {
-		if cn.destinationManip {
-			return
-		}
-		cn.destinationManip = true
-
-		cn.reply.tupleID.srcAddr = address
+		manip = &cn.destinationManip
+		address = &cn.reply.tupleID.srcAddr
 		port = &cn.reply.tupleID.srcPort
 	} else {
-		if cn.sourceManip {
-			return
-		}
-		cn.sourceManip = true
-
-		cn.reply.tupleID.dstAddr = address
+		manip = &cn.sourceManip
+		address = &cn.reply.tupleID.dstAddr
 		port = &cn.reply.tupleID.dstPort
 	}
 
+	if *manip != manipNotPerformed {
+		return
+	}
+	*manip = manipPerformed
+	*address = natAddress
+
 	// Does the current port fit in the range?
 	if end := ports.start + ports.size - 1; *port >= ports.start && *port <= end {
 		// Yes, is the current reply tuple unique?
@@ -686,37 +719,34 @@ func (cn *conn) handlePacket(pkt *PacketBuffer, hook Hook, rt *Route) bool {
 
 	reply := pkt.tuple.reply
 
-	tid, performManip := func() (tupleID, bool) {
+	tid, manip := func() (tupleID, manipType) {
 		cn.mu.RLock()
 		defer cn.mu.RUnlock()
 
-		var tuple *tuple
 		if reply {
-			if dnat {
-				if !cn.sourceManip {
-					return tupleID{}, false
-				}
-			} else if !cn.destinationManip {
-				return tupleID{}, false
-			}
+			tid := cn.original.id()
 
-			tuple = &cn.original
-		} else {
 			if dnat {
-				if !cn.destinationManip {
-					return tupleID{}, false
-				}
-			} else if !cn.sourceManip {
-				return tupleID{}, false
+				return tid, cn.sourceManip
 			}
-
-			tuple = &cn.reply
+			return tid, cn.destinationManip
 		}
 
-		return tuple.id(), true
+		tid := cn.reply.id()
+		if dnat {
+			return tid, cn.destinationManip
+		}
+		return tid, cn.sourceManip
 	}()
-	if !performManip {
+	switch manip {
+	case manipNotPerformed:
 		return false
+	case manipPerformedNoop:
+		*natDone = true
+		return true
+	case manipPerformed:
+	default:
+		panic(fmt.Sprintf("unhandled manip = %d", manip))
 	}
 
 	newPort := tid.dstPort
@@ -939,7 +969,7 @@ func (ct *ConnTrack) originalDst(epID TransportEndpointID, netProto tcpip.Networ
 
 	t.conn.mu.RLock()
 	defer t.conn.mu.RUnlock()
-	if !t.conn.destinationManip {
+	if t.conn.destinationManip == manipNotPerformed {
 		// Unmanipulated destination.
 		return "", 0, &tcpip.ErrInvalidOptionValue{}
 	}

pkg/tcpip/stack/iptables.go

@@ -425,11 +425,43 @@ func (it *IPTables) checkMangleRLocked(hook Hook, pkt *PacketBuffer, r *Route, a
 //
 // +checklocksread:it.mu
 func (it *IPTables) checkNATRLocked(hook Hook, pkt *PacketBuffer, r *Route, addressEP AddressableEndpoint, inNicName, outNicName string) bool {
-	if t := pkt.tuple; t != nil && t.conn.handlePacket(pkt, hook, r) {
+	t := pkt.tuple
+	if t != nil && t.conn.handlePacket(pkt, hook, r) {
 		return true
 	}
 
-	return it.checkRLocked(NATID, hook, pkt, r, addressEP, inNicName, outNicName)
+	if !it.checkRLocked(NATID, hook, pkt, r, addressEP, inNicName, outNicName) {
+		return false
+	}
+
+	if t == nil {
+		return true
+	}
+
+	var dnat bool
+	var natDone *bool
+	switch hook {
+	case Prerouting, Output:
+		dnat = true
+		natDone = &pkt.DNATDone
+	case Input, Postrouting:
+		dnat = false
+		natDone = &pkt.SNATDone
+	case Forward:
+		panic("should not attempt NAT in forwarding")
+	default:
+		panic(fmt.Sprintf("unhandled hook = %d", hook))
+	}
+
+	// Make sure the connection is NATed.
+	//
+	// If the packet was already NATed, the connection must be NATed.
+	if !*natDone {
+		t.conn.maybePerformNoopNAT(dnat)
+		_ = t.conn.handlePacket(pkt, hook, r)
+	}
+
+	return true
 }
 
 // checkFilterRLocked runs the packet through the filter table.

pkg/tcpip/stack/iptables_test.go

@@ -18,27 +18,57 @@ import (
 	"math/rand"
 	"testing"
 
-	"gvisor.dev/gvisor/pkg/tcpip"
 	"gvisor.dev/gvisor/pkg/tcpip/faketime"
 	"gvisor.dev/gvisor/pkg/tcpip/header"
+	"gvisor.dev/gvisor/pkg/tcpip/testutil"
 )
 
+const (
+	nattedDstPort = 1
+	srcPort       = 2
+	dstPort       = 3
+
+	// The network protocol used for these tests doesn't matter as the tests are
+	// not targetting anything protocol specific.
+	ipv6     = true
+	netProto = header.IPv6ProtocolNumber
+)
+
+var (
+	nattedDstAddr = testutil.MustParse6("a::1")
+	srcAddr       = testutil.MustParse6("b::2")
+	dstAddr       = testutil.MustParse6("c::3")
+)
+
+func v6PacketBuffer() *PacketBuffer {
+	pkt := NewPacketBuffer(PacketBufferOptions{
+		ReserveHeaderBytes: header.IPv6MinimumSize + header.UDPMinimumSize,
+	})
+	udp := header.UDP(pkt.TransportHeader().Push(header.UDPMinimumSize))
+	udp.SetSourcePort(srcPort)
+	udp.SetDestinationPort(dstPort)
+	udp.SetChecksum(0)
+	udp.SetChecksum(^udp.CalculateChecksum(header.PseudoHeaderChecksum(
+		header.UDPProtocolNumber,
+		srcAddr,
+		dstAddr,
+		uint16(len(udp)),
+	)))
+	pkt.TransportProtocolNumber = header.UDPProtocolNumber
+	ip := header.IPv6(pkt.NetworkHeader().Push(header.IPv6MinimumSize))
+	ip.Encode(&header.IPv6Fields{
+		PayloadLength:     uint16(len(udp)),
+		TransportProtocol: header.UDPProtocolNumber,
+		HopLimit:          64,
+		SrcAddr:           srcAddr,
+		DstAddr:           dstAddr,
+	})
+	pkt.NetworkProtocolNumber = header.IPv6ProtocolNumber
+	return pkt
+}
+
 // TestNATedConnectionReap tests that NATed connections are properly reaped.
 func TestNATedConnectionReap(t *testing.T) {
-	// Note that the network protocol used for this test doesn't matter as this
-	// test focuses on reaping, not anything related to a specific network
-	// protocol.
-
-	const (
-		nattedDstPort = 1
-		srcPort       = 2
-		dstPort       = 3
-
-		nattedDstAddr = tcpip.Address("\x0a\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01")
-		srcAddr       = tcpip.Address("\x0b\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02")
-		dstAddr       = tcpip.Address("\x0c\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x03")
-	)
-
 	clock := faketime.NewManualClock()
 	iptables := DefaultTables(clock, rand.New(rand.NewSource(0 /* seed */)))
 
@@ -46,7 +76,7 @@ func TestNATedConnectionReap(t *testing.T) {
 		Rules: []Rule{
 			// Prerouting
 			{
-				Target: &DNATTarget{NetworkProtocol: header.IPv6ProtocolNumber, Addr: nattedDstAddr, Port: nattedDstPort},
+				Target: &DNATTarget{NetworkProtocol: netProto, Addr: nattedDstAddr, Port: nattedDstPort},
 			},
 			{
 				Target: &AcceptTarget{},
@@ -80,7 +110,7 @@ func TestNATedConnectionReap(t *testing.T) {
 			Postrouting: 5,
 		},
 	}
-	if err := iptables.ReplaceTable(NATID, table, true /* ipv6 */); err != nil {
+	if err := iptables.ReplaceTable(NATID, table, ipv6); err != nil {
 		t.Fatalf("ipt.ReplaceTable(%d, _, true): %s", NATID, err)
 	}
 
@@ -88,29 +118,7 @@ func TestNATedConnectionReap(t *testing.T) {
 	// on the first change to IPTables.
 	iptables.reaperDone <- struct{}{}
 
-	pkt := NewPacketBuffer(PacketBufferOptions{
-		ReserveHeaderBytes: header.IPv6MinimumSize + header.UDPMinimumSize,
-	})
-	udp := header.UDP(pkt.TransportHeader().Push(header.UDPMinimumSize))
-	udp.SetSourcePort(srcPort)
-	udp.SetDestinationPort(dstPort)
-	udp.SetChecksum(0)
-	udp.SetChecksum(^udp.CalculateChecksum(header.PseudoHeaderChecksum(
-		header.UDPProtocolNumber,
-		srcAddr,
-		dstAddr,
-		uint16(len(udp)),
-	)))
-	pkt.TransportProtocolNumber = header.UDPProtocolNumber
-	ip := header.IPv6(pkt.NetworkHeader().Push(header.IPv6MinimumSize))
-	ip.Encode(&header.IPv6Fields{
-		PayloadLength:     uint16(len(udp)),
-		TransportProtocol: header.UDPProtocolNumber,
-		HopLimit:          64,
-		SrcAddr:           srcAddr,
-		DstAddr:           dstAddr,
-	})
-	pkt.NetworkProtocolNumber = header.IPv6ProtocolNumber
+	pkt := v6PacketBuffer()
 
 	originalTID, _, ok := getTupleID(pkt)
 	if !ok {
@@ -219,3 +227,93 @@ func TestNATedConnectionReap(t *testing.T) {
 	checkNoTupleInBucket(originalBkt, originalTID, false /* reply */)
 	checkNoTupleInBucket(replyBkt, replyTID, true /* reply */)
 }
+
+// TestNATAlwaysPerformed tests that a connection will have a noop-NAT
+// performed on it when no rule matches its associated packet.
+func TestNATAlwaysPerformed(t *testing.T) {
+	tests := []struct {
+		name     string
+		dnatHook func(*testing.T, *IPTables, *PacketBuffer)
+		snatHook func(*testing.T, *IPTables, *PacketBuffer)
+	}{
+		{
+			name: "Prerouting and Input",
+			dnatHook: func(t *testing.T, iptables *IPTables, pkt *PacketBuffer) {
+				t.Helper()
+
+				if !iptables.CheckPrerouting(pkt, nil /* addressEP */, "" /* inNicName */) {
+					t.Fatal("got iptables.CheckPrerouting(...) = false, want = true")
+				}
+			},
+			snatHook: func(t *testing.T, iptables *IPTables, pkt *PacketBuffer) {
+				t.Helper()
+
+				if !iptables.CheckInput(pkt, "" /* inNicName */) {
+					t.Fatal("got iptables.CheckInput(...) = false, want = true")
+				}
+			},
+		},
+		{
+			name: "Output and Postrouting",
+			dnatHook: func(t *testing.T, iptables *IPTables, pkt *PacketBuffer) {
+				t.Helper()
+
+				// Output hook depends on a route but if the route is local, we don't
+				// need anything else from it.
+				r := Route{
+					routeInfo: routeInfo{
+						Loop: PacketLoop,
+					},
+				}
+				if !iptables.CheckOutput(pkt, &r, "" /* outNicName */) {
+					t.Fatal("got iptables.CheckOutput(...) = false, want = true")
+				}
+			},
+			snatHook: func(t *testing.T, iptables *IPTables, pkt *PacketBuffer) {
+				t.Helper()
+
+				// Postrouting hook depends on a route but if the route is local, we
+				// don't need anything else from it.
+				r := Route{
+					routeInfo: routeInfo{
+						Loop: PacketLoop,
+					},
+				}
+				if !iptables.CheckPostrouting(pkt, &r, nil /* addressEP */, "" /* outNicName */) {
+					t.Fatal("got iptables.CheckPostrouting(...) = false, want = true")
+				}
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			clock := faketime.NewManualClock()
+			iptables := DefaultTables(clock, rand.New(rand.NewSource(0 /* seed */)))
+
+			// Just to make sure the iptables is not short circuited.
+			if err := iptables.ReplaceTable(NATID, iptables.GetTable(NATID, ipv6), ipv6); err != nil {
+				t.Fatalf("ipt.ReplaceTable(%d, _, true): %s", NATID, err)
+			}
+
+			pkt := v6PacketBuffer()
+
+			test.dnatHook(t, iptables, pkt)
+			conn := pkt.tuple.conn
+			conn.mu.RLock()
+			destManip := conn.destinationManip
+			conn.mu.RUnlock()
+			if destManip != manipPerformedNoop {
+				t.Errorf("got destManip = %d, want = %d", destManip, manipPerformedNoop)
+			}
+
+			test.snatHook(t, iptables, pkt)
+			conn.mu.RLock()
+			srcManip := conn.sourceManip
+			conn.mu.RUnlock()
+			if srcManip != manipPerformedNoop {
+				t.Errorf("got destManip = %d, want = %d", destManip, manipPerformedNoop)
+			}
+		})
+	}
+}