From 115474bcf33c333d8eebad2e96d1b29463f455ba Mon Sep 17 00:00:00 2001 From: Ghanan Gowripalan Date: Tue, 16 Nov 2021 13:40:27 -0800 Subject: [PATCH] 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 | 128 +++++++++++++--------- pkg/tcpip/stack/iptables.go | 36 ++++++- pkg/tcpip/stack/iptables_test.go | 178 ++++++++++++++++++++++++------- 3 files changed, 251 insertions(+), 91 deletions(-) diff --git a/pkg/tcpip/stack/conntrack.go b/pkg/tcpip/stack/conntrack.go index 833843678..2440c34f7 100644 --- a/pkg/tcpip/stack/conntrack.go +++ b/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{} } diff --git a/pkg/tcpip/stack/iptables.go b/pkg/tcpip/stack/iptables.go index 16f539c61..09188cdaf 100644 --- a/pkg/tcpip/stack/iptables.go +++ b/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. diff --git a/pkg/tcpip/stack/iptables_test.go b/pkg/tcpip/stack/iptables_test.go index f4542de0d..32ff6a9ce 100644 --- a/pkg/tcpip/stack/iptables_test.go +++ b/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) + } + }) + } +}