From 76776aad8b3c3de915e9307a8c436af0cdbcfa21 Mon Sep 17 00:00:00 2001 From: Ghanan Gowripalan Date: Mon, 27 Dec 2021 11:17:22 -0800 Subject: [PATCH] Perform Output hook for ICMPv4 Reply Before this change, locally generated ICMPv4 replies would not perform the Output hook so NAT will not be performed for locally generated ICMPv4 replies. This change fixes that bug. PiperOrigin-RevId: 418513563 --- pkg/tcpip/network/ipv4/icmp.go | 28 ++++- pkg/tcpip/network/ipv4/ipv4.go | 22 ++-- pkg/tcpip/tests/integration/iptables_test.go | 104 ++++++++++++++----- 3 files changed, 113 insertions(+), 41 deletions(-) diff --git a/pkg/tcpip/network/ipv4/icmp.go b/pkg/tcpip/network/ipv4/icmp.go index 2a024fe38..ce8ec96ab 100644 --- a/pkg/tcpip/network/ipv4/icmp.go +++ b/pkg/tcpip/network/ipv4/icmp.go @@ -20,6 +20,7 @@ import ( "gvisor.dev/gvisor/pkg/tcpip" "gvisor.dev/gvisor/pkg/tcpip/buffer" "gvisor.dev/gvisor/pkg/tcpip/header" + "gvisor.dev/gvisor/pkg/tcpip/header/parse" "gvisor.dev/gvisor/pkg/tcpip/stack" ) @@ -255,6 +256,12 @@ func (e *endpoint) handleICMP(pkt *stack.PacketBuffer) { e.dispatcher.DeliverTransportPacket(header.ICMPv4ProtocolNumber, pkt) pkt = nil + sent := e.stats.icmp.packetsSent + if !e.protocol.allowICMPReply(header.ICMPv4EchoReply, header.ICMPv4UnusedCode) { + sent.rateLimited.Increment() + return + } + // Take the base of the incoming request IP header but replace the options. replyHeaderLength := uint8(header.IPv4MinimumSize + len(newOptions)) replyIPHdr := header.IPv4(append(iph[:header.IPv4MinimumSize:header.IPv4MinimumSize], newOptions...)) @@ -275,9 +282,10 @@ func (e *endpoint) handleICMP(pkt *stack.PacketBuffer) { } defer r.Release() - sent := e.stats.icmp.packetsSent - if !e.protocol.allowICMPReply(header.ICMPv4EchoReply, header.ICMPv4UnusedCode) { - sent.rateLimited.Increment() + outgoingEP, ok := e.protocol.getEndpointForNIC(r.NICID()) + if !ok { + // The outgoing NIC went away. + sent.dropped.Increment() return } @@ -308,6 +316,9 @@ func (e *endpoint) handleICMP(pkt *stack.PacketBuffer) { replyIPHdr.SetSourceAddress(r.LocalAddress()) replyIPHdr.SetDestinationAddress(r.RemoteAddress()) replyIPHdr.SetTTL(r.DefaultTTL()) + replyIPHdr.SetTotalLength(uint16(len(replyIPHdr) + len(replyData))) + replyIPHdr.SetChecksum(0) + replyIPHdr.SetChecksum(^replyIPHdr.CalculateChecksum()) replyICMPHdr := header.ICMPv4(replyData) replyICMPHdr.SetType(header.ICMPv4EchoReply) @@ -321,9 +332,16 @@ func (e *endpoint) handleICMP(pkt *stack.PacketBuffer) { Data: replyVV, }) defer replyPkt.DecRef() - replyPkt.TransportProtocolNumber = header.ICMPv4ProtocolNumber + // Populate the network/transport headers in the packet buffer so the + // ICMP packet goes through IPTables. + if ok := parse.IPv4(replyPkt); !ok { + panic("expected to parse IPv4 header we just created") + } + if ok := parse.ICMPv4(replyPkt); !ok { + panic("expected to parse ICMPv4 header we just created") + } - if err := r.WriteHeaderIncludedPacket(replyPkt); err != nil { + if err := outgoingEP.writePacket(r, replyPkt); err != nil { sent.dropped.Increment() return } diff --git a/pkg/tcpip/network/ipv4/ipv4.go b/pkg/tcpip/network/ipv4/ipv4.go index 8afb52a99..475bb82cd 100644 --- a/pkg/tcpip/network/ipv4/ipv4.go +++ b/pkg/tcpip/network/ipv4/ipv4.go @@ -420,11 +420,17 @@ func (e *endpoint) handleFragments(_ *stack.Route, networkMTU uint32, pkt *stack // WritePacket writes a packet to the given destination address and protocol. func (e *endpoint) WritePacket(r *stack.Route, params stack.NetworkHeaderParams, pkt *stack.PacketBuffer) tcpip.Error { - dstAddr := r.RemoteAddress() - if err := e.addIPHeader(r.LocalAddress(), dstAddr, pkt, params, nil /* options */); err != nil { + if err := e.addIPHeader(r.LocalAddress(), r.RemoteAddress(), pkt, params, nil /* options */); err != nil { return err } + return e.writePacket(r, pkt) +} + +func (e *endpoint) writePacket(r *stack.Route, pkt *stack.PacketBuffer) tcpip.Error { + netHeader := header.IPv4(pkt.NetworkHeader().View()) + dstAddr := netHeader.DestinationAddress() + // iptables filtering. All packets that reach here are locally // generated. outNicName := e.protocol.stack.FindNICNameFromID(e.nic.ID()) @@ -441,8 +447,8 @@ func (e *endpoint) WritePacket(r *stack.Route, params stack.NetworkHeaderParams, // We should do this for every packet, rather than only DNATted packets, but // removing this check short circuits broadcasts before they are sent out to // other hosts. - if netHeader := header.IPv4(pkt.NetworkHeader().View()); dstAddr != netHeader.DestinationAddress() { - if ep := e.protocol.findEndpointWithAddress(netHeader.DestinationAddress()); ep != nil { + if newDstAddr := netHeader.DestinationAddress(); dstAddr != newDstAddr { + if ep := e.protocol.findEndpointWithAddress(newDstAddr); ep != nil { // Since we rewrote the packet but it is being routed back to us, we // can safely assume the checksum is valid. ep.handleLocalPacket(pkt, true /* canSkipRXChecksum */) @@ -450,10 +456,10 @@ func (e *endpoint) WritePacket(r *stack.Route, params stack.NetworkHeaderParams, } } - return e.writePacket(r, pkt, false /* headerIncluded */) + return e.writePacketPostRouting(r, pkt, false /* headerIncluded */) } -func (e *endpoint) writePacket(r *stack.Route, pkt *stack.PacketBuffer, headerIncluded bool) tcpip.Error { +func (e *endpoint) writePacketPostRouting(r *stack.Route, pkt *stack.PacketBuffer, headerIncluded bool) tcpip.Error { if r.Loop()&stack.PacketLoop != 0 { // If the packet was generated by the stack (not a raw/packet endpoint // where a packet may be written with the header included), then we can @@ -561,7 +567,7 @@ func (e *endpoint) WriteHeaderIncludedPacket(r *stack.Route, pkt *stack.PacketBu return &tcpip.ErrMalformedHeader{} } - return e.writePacket(r, pkt, true /* headerIncluded */) + return e.writePacketPostRouting(r, pkt, true /* headerIncluded */) } // forwardPacket attempts to forward a packet to its final destination. @@ -691,7 +697,7 @@ func (e *endpoint) forwardPacket(pkt *stack.PacketBuffer) ip.ForwardingError { return &ip.ErrOther{Err: &tcpip.ErrUnknownDevice{}} } - switch err := forwardToEp.writePacket(r, newPkt, true /* headerIncluded */); err.(type) { + switch err := forwardToEp.writePacketPostRouting(r, newPkt, true /* headerIncluded */); err.(type) { case nil: return nil case *tcpip.ErrMessageTooLong: diff --git a/pkg/tcpip/tests/integration/iptables_test.go b/pkg/tcpip/tests/integration/iptables_test.go index ccdc1f877..e84193de9 100644 --- a/pkg/tcpip/tests/integration/iptables_test.go +++ b/pkg/tcpip/tests/integration/iptables_test.go @@ -971,7 +971,7 @@ func TestForwardingHook(t *testing.T) { } } -func TestInputHookWithLocalForwarding(t *testing.T) { +func TestFilteringEchoPacketsWithLocalForwarding(t *testing.T) { const ( nicID1 = 1 nicID2 = 2 @@ -1018,37 +1018,58 @@ func TestInputHookWithLocalForwarding(t *testing.T) { }, } + type droppedEcho int + const ( + _ droppedEcho = iota + noneDropped + echoRequestDroppedAtInput + echoRequestDroppedAtForward + echoReplyDropped + ) + subTests := []struct { - name string - setupFilter func(*testing.T, *stack.Stack, tcpip.NetworkProtocolNumber) - expectDrop bool + name string + setupFilter func(*testing.T, *stack.Stack, tcpip.NetworkProtocolNumber) + expectResult droppedEcho }{ { - name: "Accept", - setupFilter: func(*testing.T, *stack.Stack, tcpip.NetworkProtocolNumber) { /* no filter */ }, - expectDrop: false, + name: "Accept", + setupFilter: func(*testing.T, *stack.Stack, tcpip.NetworkProtocolNumber) { /* no filter */ }, + expectResult: noneDropped, }, { - name: "Drop", - setupFilter: setupDropFilter(stack.Input, stack.IPHeaderFilter{}), - expectDrop: true, + name: "Input Drop", + setupFilter: setupDropFilter(stack.Input, stack.IPHeaderFilter{}), + expectResult: echoRequestDroppedAtInput, }, { - name: "Drop with input NIC filtering on arrival NIC", - setupFilter: setupDropFilter(stack.Input, stack.IPHeaderFilter{InputInterface: nic1Name}), - expectDrop: true, + name: "Input Drop with input NIC filtering on arrival NIC", + setupFilter: setupDropFilter(stack.Input, stack.IPHeaderFilter{InputInterface: nic1Name}), + expectResult: echoRequestDroppedAtInput, }, { - name: "Drop with input NIC filtering on delivered NIC", - setupFilter: setupDropFilter(stack.Input, stack.IPHeaderFilter{InputInterface: nic2Name}), - expectDrop: false, + name: "Input Drop with input NIC filtering on delivered NIC", + setupFilter: setupDropFilter(stack.Input, stack.IPHeaderFilter{InputInterface: nic2Name}), + expectResult: noneDropped, }, { - name: "Drop with input NIC filtering on other NIC", - setupFilter: setupDropFilter(stack.Input, stack.IPHeaderFilter{InputInterface: otherNICName}), - expectDrop: false, + name: "Input Drop with input NIC filtering on other NIC", + setupFilter: setupDropFilter(stack.Input, stack.IPHeaderFilter{InputInterface: otherNICName}), + expectResult: noneDropped, + }, + + { + name: "Forward Drop", + setupFilter: setupDropFilter(stack.Forward, stack.IPHeaderFilter{}), + expectResult: echoRequestDroppedAtForward, + }, + + { + name: "Output Drop", + setupFilter: setupDropFilter(stack.Output, stack.IPHeaderFilter{}), + expectResult: echoReplyDropped, }, } @@ -1121,8 +1142,34 @@ func TestInputHookWithLocalForwarding(t *testing.T) { if got := ip1Stats.ValidPacketsReceived.Value(); got != 1 { t.Errorf("got ip1Stats.ValidPacketsReceived.Value() = %d, want = 1", got) } - if got, want := ip1Stats.PacketsSent.Value(), boolToInt(!subTest.expectDrop); got != want { - t.Errorf("got ip1Stats.PacketsSent.Value() = %d, want = %d", got, want) + + expectedIP1StatIPTablesForawrdDropped := uint64(0) + expectedIP1StatIPTablesOutputDropped := uint64(0) + expectedIP1StatPacketsSent := uint64(0) + expectedIP2StatValidPacketsReceived := uint64(1) + expectedIP2StatIPTablesInputDropped := uint64(0) + switch subTest.expectResult { + case noneDropped: + expectedIP1StatPacketsSent = 1 + case echoRequestDroppedAtInput: + expectedIP2StatIPTablesInputDropped = 1 + case echoRequestDroppedAtForward: + expectedIP1StatIPTablesForawrdDropped = 1 + expectedIP2StatValidPacketsReceived = 0 + case echoReplyDropped: + expectedIP1StatIPTablesOutputDropped = 1 + default: + t.Fatalf("unhandled expectResult = %d", subTest.expectResult) + } + + if got := ip1Stats.IPTablesForwardDropped.Value(); got != expectedIP1StatIPTablesForawrdDropped { + t.Errorf("got ip1Stats.IPTablesForwardDropped.Value() = %d, want = %d", got, expectedIP1StatIPTablesForawrdDropped) + } + if got := ip1Stats.IPTablesOutputDropped.Value(); got != expectedIP1StatIPTablesOutputDropped { + t.Errorf("got ip1Stats.IPTablesOutputDropped.Value() = %d, want = %d", got, expectedIP1StatIPTablesOutputDropped) + } + if got := ip1Stats.PacketsSent.Value(); got != expectedIP1StatPacketsSent { + t.Errorf("got ip1Stats.PacketsSent.Value() = %d, want = %d", got, expectedIP1StatPacketsSent) } ep2, err := s.GetNetworkEndpoint(nicID2, test.netProto) @@ -1138,19 +1185,20 @@ func TestInputHookWithLocalForwarding(t *testing.T) { if got := ip2Stats.PacketsReceived.Value(); got != 0 { t.Errorf("got ip2Stats.PacketsReceived.Value() = %d, want = 0", got) } - if got := ip2Stats.ValidPacketsReceived.Value(); got != 1 { - t.Errorf("got ip2Stats.ValidPacketsReceived.Value() = %d, want = 1", got) + if got := ip2Stats.ValidPacketsReceived.Value(); got != expectedIP2StatValidPacketsReceived { + t.Errorf("got ip2Stats.ValidPacketsReceived.Value() = %d, want = %d", got, expectedIP2StatValidPacketsReceived) } - if got, want := ip2Stats.IPTablesInputDropped.Value(), boolToInt(subTest.expectDrop); got != want { - t.Errorf("got ip2Stats.IPTablesInputDropped.Value() = %d, want = %d", got, want) + if got := ip2Stats.IPTablesInputDropped.Value(); got != expectedIP2StatIPTablesInputDropped { + t.Errorf("got ip2Stats.IPTablesInputDropped.Value() = %d, want = %d", got, expectedIP2StatIPTablesInputDropped) } if got := ip2Stats.PacketsSent.Value(); got != 0 { t.Errorf("got ip2Stats.PacketsSent.Value() = %d, want = 0", got) } - if p := e1.Read(); (p != nil) == subTest.expectDrop { - t.Errorf("got e1.Read() = %#v, want = (_ == nil) = %t", p, !subTest.expectDrop) - } else if !subTest.expectDrop { + expectPacket := subTest.expectResult == noneDropped + if p := e1.Read(); (p != nil) != expectPacket { + t.Errorf("got e1.Read() = %#v, want = (_ == nil) = %t", p, expectPacket) + } else if expectPacket { test.checker(t, stack.PayloadSince(p.NetworkHeader())) } if p := e2.Read(); p != nil {