NAT source ports for locally generated traffic when necessary

Currently, when a packet hits the NAT table and does not hit any NAT targets,
its connection is assigned "no-op NAT", which basically means that the
connection will not be NATed. However, this causes a problem when, for example,
locally-generated traffic chooses a local port that has already been used by
forwarded traffic that the stack is NATing. In this case, the locally-generated
traffic will simply be dropped when the connection is finalized due to a tuple
conflict.

Instead of doing nothing, the netstack must implicitly perform SNAT for the
locally-generated traffic to remap its source port to prevent that traffic from
being dropped.

Also, when setting up NAT for a connection, the netstack checks if the
connections' reply tuple is unique to see whether it needs to rewrite the
transport-layer port/ID. This logic currently doesn't account for self-connected
sockets, where the original and reply tuples are identical and point to the same
connection; add logic handling that scenario, such that the reply tuple can be
non-unique if it refers to the same connection as the original tuple.

PiperOrigin-RevId: 612969884

pkg/tcpip/stack/conntrack.go

@@ -695,20 +695,41 @@ func (cn *conn) finalize() bool {
 	}
 }
 
-func (cn *conn) maybePerformNoopNAT(dnat bool) {
+// If NAT has not been configured for this connection, either mark the
+// connection as configured for "no-op NAT", in the case of DNAT, or, in the
+// case of SNAT, perform source port remapping so that source ports used by
+// locally-generated traffic do not conflict with ports occupied by existing NAT
+// bindings.
+//
+// Note that in the typical case this is also a no-op, because `snatAction`
+// will do nothing if the original tuple is already unique.
+func (cn *conn) maybePerformNoopNAT(pkt *PacketBuffer, hook Hook, r *Route, 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
+	if *manip != manipNotPerformed {
+		cn.mu.Unlock()
+		_ = cn.handlePacket(pkt, hook, r)
+		return
 	}
+	if dnat {
+		*manip = manipPerformedNoop
+		cn.mu.Unlock()
+		_ = cn.handlePacket(pkt, hook, r)
+		return
+	}
+	cn.mu.Unlock()
+
+	// At this point, we know that NAT has not yet been performed on this
+	// connection, and the DNAT case has been handled with a no-op. For SNAT, we
+	// simply perform source port remapping to ensure that source ports for
+	// locally generated traffic do not clash with ports used by existing NAT
+	// bindings.
+	_, _ = snatAction(pkt, hook, r, 0, tcpip.Address{}, true /* changePort */, false /* changeAddress */)
 }
 
 type portOrIdentRange struct {
@@ -774,7 +795,12 @@ func (cn *conn) performNAT(pkt *PacketBuffer, hook Hook, r *Route, portsOrIdents
 	// Does the current port/ident fit in the range?
 	if portsOrIdents.start <= *portOrIdent && *portOrIdent <= lastPortOrIdent {
 		// Yes, is the current reply tuple unique?
-		if other := cn.ct.connForTID(cn.reply.tupleID); other == nil {
+		//
+		// Or, does the reply tuple refer to the same connection as the current one that
+		// we are NATing? This would apply, for example, to a self-connected socket,
+		// where the original and reply tuples are identical.
+		other := cn.ct.connForTID(cn.reply.tupleID)
+		if other == nil || other.conn == cn {
 			// Yes! No need to change the port.
 			return
 		}

pkg/tcpip/stack/iptables.go

@@ -561,8 +561,7 @@ func (it *IPTables) checkNAT(table Table, hook Hook, pkt *PacketBuffer, r *Route
 	//
 	// If the packet was already NATed, the connection must be NATed.
 	if !natDone {
-		t.conn.maybePerformNoopNAT(dnat)
-		_ = t.conn.handlePacket(pkt, hook, r)
+		t.conn.maybePerformNoopNAT(pkt, hook, r, dnat)
 	}
 
 	return true

pkg/tcpip/stack/iptables_test.go

@@ -237,7 +237,9 @@ func TestNATedConnectionReap(t *testing.T) {
 }
 
 // TestNATAlwaysPerformed tests that a connection will have a noop-NAT
-// performed on it when no rule matches its associated packet.
+// performed on it when no rule matches its associated packet. (Note that SNAT
+// is performed on all connections to ensure that ports used by locally
+// generated traffic do not clash with ports used by forwarded traffic.
 func TestNATAlwaysPerformed(t *testing.T) {
 	tests := []struct {
 		name     string
@@ -317,8 +319,8 @@ func TestNATAlwaysPerformed(t *testing.T) {
 			conn.mu.RLock()
 			srcManip := conn.sourceManip
 			conn.mu.RUnlock()
-			if srcManip != manipPerformedNoop {
-				t.Errorf("got destManip = %d, want = %d", destManip, manipPerformedNoop)
+			if srcManip != manipPerformed {
+				t.Errorf("got srcManip = %d, want = %d", srcManip, manipPerformed)
 			}
 		})
 	}

pkg/tcpip/tests/integration/iptables_test.go

@@ -2928,6 +2928,208 @@ func TestSNATHandlePortOrIdentConflicts(t *testing.T) {
 	}
 }
 
+func TestSNATLocallyGeneratedTrafficPorts(t *testing.T) {
+	s := stack.New(stack.Options{
+		NetworkProtocols:   []stack.NetworkProtocolFactory{ipv4.NewProtocol, ipv6.NewProtocol},
+		TransportProtocols: []stack.TransportProtocolFactory{udp.NewProtocol},
+	})
+	defer s.Destroy()
+
+	ep1 := channel.New(1, header.IPv4MinimumMTU, "")
+	ep2 := channel.New(1, header.IPv4MinimumMTU, "")
+	utils.SetupRouterStack(t, s, ep1, ep2)
+
+	// Configure Masquerade NAT on the router stack.
+	ipt := s.IPTables()
+	table := stack.Table{
+		Rules: []stack.Rule{
+			// Prerouting
+			{
+				Target: &stack.AcceptTarget{},
+			},
+
+			// Input
+			{
+				Target: &stack.AcceptTarget{},
+			},
+
+			// Forward
+			{
+				Target: &stack.AcceptTarget{},
+			},
+
+			// Output
+			{
+				Target: &stack.AcceptTarget{},
+			},
+
+			// Postrouting
+			{
+				Filter: stack.IPHeaderFilter{
+					Protocol:        udp.ProtocolNumber,
+					CheckProtocol:   true,
+					OutputInterface: utils.RouterNIC2Name,
+				},
+				Target: &stack.MasqueradeTarget{NetworkProtocol: ipv4.ProtocolNumber},
+			},
+			{
+				Target: &stack.AcceptTarget{},
+			},
+		},
+		BuiltinChains: [stack.NumHooks]int{
+			stack.Prerouting:  0,
+			stack.Input:       1,
+			stack.Forward:     2,
+			stack.Output:      3,
+			stack.Postrouting: 4,
+		},
+	}
+	ipt.ForceReplaceTable(stack.NATID, table, false /* ipv6 */)
+
+	routerNIC2Addr := utils.RouterNIC2IPv4Addr.AddressWithPrefix.Address
+	ep1Addr := utils.Host1IPv4Addr.AddressWithPrefix.Address
+	var ep1Port uint16 = 1234
+	ep2Addr := utils.Host2IPv4Addr.AddressWithPrefix.Address
+	var ep2Port uint16 = 2345
+
+	// Inject an incoming packet on NIC1 destined to an address that will be
+	// routed out of NIC2. Expect that we can read the packet on ep2 coming from
+	// the stack's address assigned on NIC2, because it should have performed
+	// Masquerade NAT on the forwarded traffic.
+	ep1.InjectInbound(ipv4.ProtocolNumber, stack.NewPacketBuffer(stack.PacketBufferOptions{
+		Payload: buffer.MakeWithData(udpv4Packet(ep1Addr, ep2Addr, ep1Port, ep2Port, 0 /* dataSize */)),
+	}))
+	pkt := ep2.Read()
+	if pkt.IsNil() {
+		t.Fatal("expected to read a packet on ep2")
+	}
+	pktView := stack.PayloadSince(pkt.NetworkHeader())
+	defer pktView.Release()
+	pkt.DecRef()
+	checker.IPv4(t, pktView,
+		checker.SrcAddr(routerNIC2Addr),
+		checker.DstAddr(ep2Addr),
+		checker.UDP(
+			checker.SrcPort(ep1Port),
+			checker.DstPort(ep2Port),
+		),
+	)
+
+	// Now bind a UDP socket on the stack itself to the same port used by the
+	// previous packet, and send a packet to the same address.
+	var wq waiter.Queue
+	we, ch := waiter.NewChannelEntry(waiter.ReadableEvents)
+	wq.EventRegister(&we)
+	defer wq.EventUnregister(&we)
+
+	ep, err := s.NewEndpoint(udp.ProtocolNumber, ipv4.ProtocolNumber, &wq)
+	if err != nil {
+		t.Fatalf("s.NewEndpoint(%d, %d, _): %s", udp.ProtocolNumber, ipv4.ProtocolNumber, err)
+	}
+	defer ep.Close()
+
+	srcAddr := tcpip.FullAddress{Addr: routerNIC2Addr, Port: ep1Port}
+	if err := ep.Bind(srcAddr); err != nil {
+		t.Fatalf("ep.Bind(%#v): %s", srcAddr, err)
+	}
+	dstAddr := tcpip.FullAddress{Addr: ep2Addr, Port: ep2Port}
+	if err := ep.Connect(dstAddr); err != nil {
+		t.Fatalf("ep.Connect(%#v): %s", dstAddr, err)
+	}
+
+	data := []byte{1, 2, 3, 4}
+	var r bytes.Reader
+	r.Reset(data)
+	var wOpts tcpip.WriteOptions
+	n, err := ep.Write(&r, wOpts)
+	if err != nil {
+		t.Fatalf("ep.Write(_, %#v): %s", wOpts, err)
+	}
+	if want := int64(len(data)); n != want {
+		t.Fatalf("got ep.Write(_, %#v) = (%d, _), want = (%d, _)", wOpts, n, want)
+	}
+
+	// The router should perform source port remapping for the locally generated
+	// traffic so that it does not conflict with the existing conntrack entry, so
+	// ep2 should observe the traffic as coming from the router's address, but
+	// *not* from the same port as the traffic from ep1 before.
+	pkt = ep2.Read()
+	if pkt.IsNil() {
+		t.Fatal("expected to read a packet on ep2")
+	}
+	pktView = stack.PayloadSince(pkt.NetworkHeader())
+	defer pktView.Release()
+	pkt.DecRef()
+	checker.IPv4(t, pktView,
+		checker.SrcAddr(routerNIC2Addr),
+		checker.DstAddr(ep2Addr),
+		checker.UDP(
+			checker.DstPort(ep2Port),
+			checker.Payload(data),
+		),
+	)
+	gotPort := header.UDP(header.IPv4(pktView.AsSlice()).Payload()).SourcePort()
+	if gotPort == ep1Port {
+		t.Errorf("got src port == ep1Port (%d), should be remapped to avoid conflict", gotPort)
+	}
+
+	// We should also be able to reply on either connection, by injecting inbound
+	// traffic on ep2 destined to the router.
+	//
+	// Traffic destined to the port originally used in the traffic injected on ep1
+	// should go to ep1.
+	ep2.InjectInbound(ipv4.ProtocolNumber, stack.NewPacketBuffer(stack.PacketBufferOptions{
+		Payload: buffer.MakeWithData(udpv4Packet(ep2Addr, routerNIC2Addr, ep2Port, ep1Port, 0 /* dataSize */)),
+	}))
+	pkt = ep1.Read()
+	if pkt.IsNil() {
+		t.Fatal("expected to read a packet on ep2")
+	}
+	pktView = stack.PayloadSince(pkt.NetworkHeader())
+	defer pktView.Release()
+	pkt.DecRef()
+	checker.IPv4(t, pktView,
+		checker.SrcAddr(ep2Addr),
+		checker.DstAddr(ep1Addr),
+		checker.UDP(
+			checker.SrcPort(ep2Port),
+			checker.DstPort(ep1Port),
+		),
+	)
+
+	// And traffic destined to the remapped source port chosen by conntrack for
+	// the socket bound on the stack should go to the socket.
+	reply := udpv4Packet(ep2Addr, routerNIC2Addr, ep2Port, gotPort, 0 /* dataSize */)
+	reply = append(reply, data...)
+	ep2.InjectInbound(ipv4.ProtocolNumber, stack.NewPacketBuffer(stack.PacketBufferOptions{
+		Payload: buffer.MakeWithData(reply),
+	}))
+	var buf bytes.Buffer
+	var res tcpip.ReadResult
+	for {
+		var err tcpip.Error
+		res, err = ep.Read(&buf, tcpip.ReadOptions{})
+		if _, ok := err.(*tcpip.ErrWouldBlock); ok {
+			<-ch
+			continue
+		}
+		if err != nil {
+			t.Fatalf("ep.Read(_, {}): %s", err)
+		}
+		break
+	}
+	if diff := cmp.Diff(
+		tcpip.ReadResult{
+			Count: 0,
+			Total: 0,
+		},
+		res,
+		checker.IgnoreCmpPath("ControlMessages"),
+	); diff != "" {
+		t.Errorf("ep.Read: unexpected result (-want +got):\n%s", diff)
+	}
+}
+
 func TestLocallyRoutedPackets(t *testing.T) {
 	const nicID = 1