From ecc3d01d181a6ae6d3cc72531542d9ea5fe3e376 Mon Sep 17 00:00:00 2001 From: Ghanan Gowripalan Date: Thu, 2 Apr 2020 15:58:38 -0700 Subject: [PATCH] Increment NDP message RX stats before validation Tests: - ipv6_test.TestHopLimitValidation - ipv6_test.TestRouterAdvertValidation PiperOrigin-RevId: 304495723 --- pkg/tcpip/network/ipv6/icmp.go | 57 +++++++++++++----------------- pkg/tcpip/network/ipv6/ndp_test.go | 49 ++++++++++++------------- 2 files changed, 49 insertions(+), 57 deletions(-) diff --git a/pkg/tcpip/network/ipv6/icmp.go b/pkg/tcpip/network/ipv6/icmp.go index e0dd5afd3..81e6f4d67 100644 --- a/pkg/tcpip/network/ipv6/icmp.go +++ b/pkg/tcpip/network/ipv6/icmp.go @@ -86,25 +86,12 @@ func (e *endpoint) handleICMP(r *stack.Route, netHeader buffer.View, pkt stack.P return } - // As per RFC 4861 sections 4.1 - 4.5, 6.1.1, 6.1.2, 7.1.1, 7.1.2 and - // 8.1, nodes MUST silently drop NDP packets where the Hop Limit field - // in the IPv6 header is not set to 255, or the ICMPv6 Code field is not - // set to 0. - switch h.Type() { - case header.ICMPv6NeighborSolicit, - header.ICMPv6NeighborAdvert, - header.ICMPv6RouterSolicit, - header.ICMPv6RouterAdvert, - header.ICMPv6RedirectMsg: - if iph.HopLimit() != header.NDPHopLimit { - received.Invalid.Increment() - return - } - - if h.Code() != 0 { - received.Invalid.Increment() - return - } + isNDPValid := func() bool { + // As per RFC 4861 sections 4.1 - 4.5, 6.1.1, 6.1.2, 7.1.1, 7.1.2 and + // 8.1, nodes MUST silently drop NDP packets where the Hop Limit field + // in the IPv6 header is not set to 255, or the ICMPv6 Code field is not + // set to 0. + return iph.HopLimit() == header.NDPHopLimit && h.Code() == 0 } // TODO(b/112892170): Meaningfully handle all ICMP types. @@ -133,7 +120,7 @@ func (e *endpoint) handleICMP(r *stack.Route, netHeader buffer.View, pkt stack.P case header.ICMPv6NeighborSolicit: received.NeighborSolicit.Increment() - if len(v) < header.ICMPv6NeighborSolicitMinimumSize { + if len(v) < header.ICMPv6NeighborSolicitMinimumSize || !isNDPValid() { received.Invalid.Increment() return } @@ -253,7 +240,7 @@ func (e *endpoint) handleICMP(r *stack.Route, netHeader buffer.View, pkt stack.P case header.ICMPv6NeighborAdvert: received.NeighborAdvert.Increment() - if len(v) < header.ICMPv6NeighborAdvertSize { + if len(v) < header.ICMPv6NeighborAdvertSize || !isNDPValid() { received.Invalid.Increment() return } @@ -355,8 +342,20 @@ func (e *endpoint) handleICMP(r *stack.Route, netHeader buffer.View, pkt stack.P case header.ICMPv6RouterSolicit: received.RouterSolicit.Increment() + if !isNDPValid() { + received.Invalid.Increment() + return + } case header.ICMPv6RouterAdvert: + received.RouterAdvert.Increment() + + p := h.NDPPayload() + if len(p) < header.NDPRAMinimumSize || !isNDPValid() { + received.Invalid.Increment() + return + } + routerAddr := iph.SourceAddress() // @@ -370,16 +369,6 @@ func (e *endpoint) handleICMP(r *stack.Route, netHeader buffer.View, pkt stack.P return } - p := h.NDPPayload() - - // Is the NDP payload of sufficient size to hold a Router - // Advertisement? - if len(p) < header.NDPRAMinimumSize { - // ...No, silently drop the packet. - received.Invalid.Increment() - return - } - ra := header.NDPRouterAdvert(p) opts := ra.Options() @@ -395,8 +384,6 @@ func (e *endpoint) handleICMP(r *stack.Route, netHeader buffer.View, pkt stack.P // as RFC 4861 section 6.1.2 is concerned. // - received.RouterAdvert.Increment() - // Tell the NIC to handle the RA. stack := r.Stack() rxNICID := r.NICID() @@ -404,6 +391,10 @@ func (e *endpoint) handleICMP(r *stack.Route, netHeader buffer.View, pkt stack.P case header.ICMPv6RedirectMsg: received.RedirectMsg.Increment() + if !isNDPValid() { + received.Invalid.Increment() + return + } default: received.Invalid.Increment() diff --git a/pkg/tcpip/network/ipv6/ndp_test.go b/pkg/tcpip/network/ipv6/ndp_test.go index f924ed9e1..3b05e8062 100644 --- a/pkg/tcpip/network/ipv6/ndp_test.go +++ b/pkg/tcpip/network/ipv6/ndp_test.go @@ -381,44 +381,48 @@ func TestHopLimitValidation(t *testing.T) { pkt.SetType(typ.typ) pkt.SetChecksum(header.ICMPv6Checksum(pkt, r.LocalAddress, r.RemoteAddress, extraData.ToVectorisedView())) + // Rx count of the NDP message should initially be 0. + if got := typStat.Value(); got != 0 { + t.Errorf("got %s = %d, want = 0", typ.name, got) + } + // Invalid count should initially be 0. if got := invalid.Value(); got != 0 { - t.Fatalf("got invalid = %d, want = 0", got) + t.Errorf("got invalid = %d, want = 0", got) } - // Should not have received any ICMPv6 packets with - // type = typ.typ. - if got := typStat.Value(); got != 0 { - t.Fatalf("got %s = %d, want = 0", typ.name, got) + if t.Failed() { + t.FailNow() } - // Receive the NDP packet with an invalid hop limit - // value. + // Receive the NDP packet with an invalid hop limit. handleIPv6Payload(hdr, header.NDPHopLimit-1, ep, &r) + // Rx count of the NDP packet should have increased. + if got := typStat.Value(); got != 1 { + t.Errorf("got %s = %d, want = 1", typ.name, got) + } + // Invalid count should have increased. if got := invalid.Value(); got != 1 { - t.Fatalf("got invalid = %d, want = 1", got) + t.Errorf("got invalid = %d, want = 1", got) } - // Rx count of NDP packet of type typ.typ should not - // have increased. - if got := typStat.Value(); got != 0 { - t.Fatalf("got %s = %d, want = 0", typ.name, got) + if t.Failed() { + t.FailNow() } // Receive the NDP packet with a valid hop limit value. handleIPv6Payload(hdr, header.NDPHopLimit, ep, &r) - // Rx count of NDP packet of type typ.typ should have - // increased. - if got := typStat.Value(); got != 1 { - t.Fatalf("got %s = %d, want = 1", typ.name, got) + // Rx count of the NDP packet should have increased. + if got := typStat.Value(); got != 2 { + t.Errorf("got %s = %d, want = 2", typ.name, got) } // Invalid count should not have increased again. if got := invalid.Value(); got != 1 { - t.Fatalf("got invalid = %d, want = 1", got) + t.Errorf("got invalid = %d, want = 1", got) } }) } @@ -592,21 +596,18 @@ func TestRouterAdvertValidation(t *testing.T) { Data: hdr.View().ToVectorisedView(), }) + if got := rxRA.Value(); got != 1 { + t.Fatalf("got rxRA = %d, want = 1", got) + } + if test.expectedSuccess { if got := invalid.Value(); got != 0 { t.Fatalf("got invalid = %d, want = 0", got) } - if got := rxRA.Value(); got != 1 { - t.Fatalf("got rxRA = %d, want = 1", got) - } - } else { if got := invalid.Value(); got != 1 { t.Fatalf("got invalid = %d, want = 1", got) } - if got := rxRA.Value(); got != 0 { - t.Fatalf("got rxRA = %d, want = 0", got) - } } }) }