acl-plugin: make the IPv4/IPv6 non-first fragment handling in line with ACL (VPP-682)

This fixes the previously-implicit "drop all non-first fragments" behavior
to be more in line with security rules: a non-first fragment is treated
for the purposes of matching the ACL as a packet with the port
match succeeding. This allows to change the behavior to permit
the fragmented packets for the default "permit specific rules"
ruleset, but also gives the flexibility to block the non-initial
fragments by inserting into the begining a bogus rule
which would deny the L4 traffic.

Also, add a knob which allows to potentially turn this behavior off
in case of a dire need (and revert to dropping all non-initial fragments),
via a debug CLI.

Change-Id: I546b372b65ff2157d9c68b1d32f9e644f1dd71b4
Signed-off-by: Andrew Yourtchenko <ayourtch@gmail.com>
(cherry picked from commit 9fc0c26c6b28fd6c8b8142ea52f52eafa7e8c7ac)

6 files changed, 168 insertions, 22 deletions

diff --git a/src/plugins/acl/acl.c b/src/plugins/acl/acl.c
index 6657d370e63..98c74b9b358 100644
--- a/src/plugins/acl/acl.c
+++ b/src/plugins/acl/acl.c
@@ -2008,6 +2008,11 @@ acl_set_aclplugin_fn (vlib_main_t * vm,
     }
     goto done;
   }
+  if (unformat (input, "l4-match-nonfirst-fragment %u", &val))
+    {
+      am->l4_match_nonfirst_fragment = (val != 0);
+      goto done;
+    }
   if (unformat (input, "session")) {
     if (unformat (input, "clear")) {
       acl_main_t *am = &acl_main;
@@ -2205,6 +2210,8 @@ acl_init (vlib_main_t * vm)
   foreach_acl_eh
 #undef _
 
+  am->l4_match_nonfirst_fragment = 1;
+
   return error;
 }
 
diff --git a/src/plugins/acl/acl.h b/src/plugins/acl/acl.h
index f5a1fe0f934..d708c521dfa 100644
--- a/src/plugins/acl/acl.h
+++ b/src/plugins/acl/acl.h
@@ -181,6 +181,9 @@ typedef struct {
   /* EH values that we can skip over */
   uword *fa_ipv6_known_eh_bitmap;
 
+  /* whether to match L4 ACEs with ports on the non-initial fragment */
+  int l4_match_nonfirst_fragment;
+
   /* conn table per-interface conn table parameters */
   u32 fa_conn_table_hash_num_buckets;
   uword fa_conn_table_hash_memory_size;
@@ -235,6 +238,7 @@ typedef struct {
    _(HOPBYHOP , 0  , "IPv6ExtHdrHopByHop")                      \
    _(ROUTING  , 43 , "IPv6ExtHdrRouting")                       \
    _(DESTOPT  , 60 , "IPv6ExtHdrDestOpt")                       \
+   _(FRAGMENT , 44 , "IPv6ExtHdrFragment")                      \
    _(MOBILITY , 135, "Mobility Header")                         \
    _(HIP      , 139, "Experimental use Host Identity Protocol") \
    _(SHIM6    , 140, "Shim6 Protocol")                          \
@@ -247,7 +251,6 @@ typedef struct {
  Also, Fragment header needs special processing.
 
    _(NONEXT   , 59 , "NoNextHdr")                               \
-   _(FRAGMENT , 44 , "IPv6ExtHdrFragment")                      \
 
 
 ESP is hiding its internal format, so no point in trying to go past it.
diff --git a/src/plugins/acl/fa_node.c b/src/plugins/acl/fa_node.c
index 1f9117a6321..e12cbaa731d 100644
--- a/src/plugins/acl/fa_node.c
+++ b/src/plugins/acl/fa_node.c
@@ -191,7 +191,21 @@ acl_match_5tuple (acl_main_t * am, u32 acl_index, fa_5tuple_t * pkt_5tuple,
 	{
 	  if (pkt_5tuple->l4.proto != r->proto)
 	    continue;
-	  /* A sanity check just to ensure what we jave just matched was a valid L4 extracted from the packet */
+
+          if (PREDICT_FALSE (pkt_5tuple->pkt.is_nonfirst_fragment &&
+                     am->l4_match_nonfirst_fragment))
+          {
+            /* non-initial fragment with frag match configured - match this rule */
+            *trace_bitmap |= 0x80000000;
+            *r_action = r->is_permit;
+            if (r_acl_match_p)
+	      *r_acl_match_p = acl_index;
+            if (r_rule_match_p)
+	      *r_rule_match_p = i;
+            return 1;
+          }
+
+	  /* A sanity check just to ensure we are about to match the ports extracted from the packet */
 	  if (PREDICT_FALSE (!pkt_5tuple->pkt.l4_valid))
 	    continue;
 
@@ -312,6 +326,10 @@ acl_fill_5tuple (acl_main_t * am, vlib_buffer_t * b0, int is_ip6,
       l3_offset = 0;
     }
 
+  /* key[0..3] contains src/dst address and is cleared/set below */
+  /* Remainder of the key and per-packet non-key data */
+  p5tuple_pkt->kv.key[4] = 0;
+  p5tuple_pkt->kv.value = 0;
 
   if (is_ip6)
     {
@@ -333,12 +351,33 @@ acl_fill_5tuple (acl_main_t * am, vlib_buffer_t * b0, int is_ip6,
       int need_skip_eh = clib_bitmap_get (am->fa_ipv6_known_eh_bitmap, proto);
       if (PREDICT_FALSE (need_skip_eh))
 	{
-	  /* FIXME: add fragment header special handling. Currently causes treated as unknown header. */
 	  while (need_skip_eh && offset_within_packet (b0, l4_offset))
 	    {
-	      u8 nwords = *(u8 *) get_ptr_to_offset (b0, 1 + l4_offset);
-	      proto = *(u8 *) get_ptr_to_offset (b0, l4_offset);
-	      l4_offset += 8 * (1 + (u16) nwords);
+	      /* Fragment header needs special handling */
+	      if (PREDICT_FALSE(ACL_EH_FRAGMENT == proto))
+	        {
+	          proto = *(u8 *) get_ptr_to_offset (b0, l4_offset);
+		  u16 frag_offset;
+		  clib_memcpy (&frag_offset, get_ptr_to_offset (b0, 2 + l4_offset), sizeof(frag_offset));
+		  frag_offset = ntohs(frag_offset) >> 3;
+		  if (frag_offset)
+		    {
+                      p5tuple_pkt->pkt.is_nonfirst_fragment = 1;
+                      /* invalidate L4 offset so we don't try to find L4 info */
+                      l4_offset += b0->current_length;
+		    }
+		  else
+		    {
+		      /* First fragment: skip the frag header and move on. */
+		      l4_offset += 8;
+		    }
+		}
+              else
+                {
+	          u8 nwords = *(u8 *) get_ptr_to_offset (b0, 1 + l4_offset);
+	          proto = *(u8 *) get_ptr_to_offset (b0, l4_offset);
+	          l4_offset += 8 * (1 + (u16) nwords);
+                }
 #ifdef FA_NODE_VERBOSE_DEBUG
 	      clib_warning ("ACL_FA_NODE_DBG: new proto: %d, new offset: %d",
 			    proto, l4_offset);
@@ -369,13 +408,26 @@ acl_fill_5tuple (acl_main_t * am, vlib_buffer_t * b0, int is_ip6,
 				   offsetof (ip4_header_t,
 					     protocol) + l3_offset);
       l4_offset = l3_offset + sizeof (ip4_header_t);
+      u16 flags_and_fragment_offset;
+      clib_memcpy (&flags_and_fragment_offset,
+                   get_ptr_to_offset (b0,
+                                      offsetof (ip4_header_t,
+                                                flags_and_fragment_offset)) + l3_offset,
+                                                sizeof(flags_and_fragment_offset));
+      flags_and_fragment_offset = ntohs (flags_and_fragment_offset);
+
+      /* non-initial fragments have non-zero offset */
+      if ((PREDICT_FALSE(0xfff & flags_and_fragment_offset)))
+        {
+          p5tuple_pkt->pkt.is_nonfirst_fragment = 1;
+          /* invalidate L4 offset so we don't try to find L4 info */
+          l4_offset += b0->current_length;
+        }
+
     }
-  /* Remainder of the key and per-packet non-key data */
-  p5tuple_pkt->kv.key[4] = 0;
-  p5tuple_pkt->kv.value = 0;
+  p5tuple_pkt->l4.proto = proto;
   if (PREDICT_TRUE (offset_within_packet (b0, l4_offset)))
     {
-      p5tuple_pkt->l4.proto = proto;
       p5tuple_pkt->pkt.l4_valid = 1;
       if (icmp_protos[is_ip6] == proto)
 	{
diff --git a/src/plugins/acl/fa_node.h b/src/plugins/acl/fa_node.h
index 76a40a38486..8edd0069217 100644
--- a/src/plugins/acl/fa_node.h
+++ b/src/plugins/acl/fa_node.h
@@ -22,10 +22,12 @@
 typedef union {
   u64 as_u64;
   struct {
-    u8 tcp_flags_valid;
     u8 tcp_flags;
-    u8 is_input;
-    u8 l4_valid;
+    u8 tcp_flags_valid:1;
+    u8 is_input:1;
+    u8 l4_valid:1;
+    u8 is_nonfirst_fragment:1;
+    u8 flags_reserved:4;
   };
 } fa_packet_info_t;
 
diff --git a/test/test_acl_plugin.py b/test/test_acl_plugin.py
index 2bbebe85b1b..b051d457824 100644
--- a/test/test_acl_plugin.py
+++ b/test/test_acl_plugin.py
@@ -9,6 +9,7 @@ from scapy.packet import Raw
 from scapy.layers.l2 import Ether
 from scapy.layers.inet import IP, TCP, UDP, ICMP
 from scapy.layers.inet6 import IPv6, ICMPv6EchoRequest
+from scapy.layers.inet6 import IPv6ExtHdrFragment
 from framework import VppTestCase, VppTestRunner
 from util import Host, ppp
 
@@ -229,7 +230,7 @@ class TestACLplugin(VppTestCase):
         return ''
 
     def create_stream(self, src_if, packet_sizes, traffic_type=0, ipv6=0,
-                      proto=-1, ports=0):
+                      proto=-1, ports=0, fragments=False):
         """
         Create input packet stream for defined interface using hosts or
         deleted_hosts list.
@@ -263,8 +264,14 @@ class TestACLplugin(VppTestCase):
                     p = Ether(dst=dst_host.mac, src=src_host.mac)
                     if pkt_info.ip:
                         p /= IPv6(dst=dst_host.ip6, src=src_host.ip6)
+                        if fragments:
+                            p /= IPv6ExtHdrFragment(offset=64, m=1)
                     else:
-                        p /= IP(src=src_host.ip4, dst=dst_host.ip4)
+                        if fragments:
+                            p /= IP(src=src_host.ip4, dst=dst_host.ip4,
+                                    flags=1, frag=64)
+                        else:
+                            p /= IP(src=src_host.ip4, dst=dst_host.ip4)
                     if traffic_type == self.ICMP:
                         if pkt_info.ip:
                             p /= ICMPv6EchoRequest(type=self.icmp6_type,
@@ -381,14 +388,16 @@ class TestACLplugin(VppTestCase):
         self.pg_enable_capture(self.pg_interfaces)
         self.pg_start()
 
-    def run_verify_test(self, traffic_type=0, ip_type=0, proto=-1, ports=0):
+    def run_verify_test(self, traffic_type=0, ip_type=0, proto=-1, ports=0,
+                        frags=False):
         # Test
         # Create incoming packet streams for packet-generator interfaces
         pkts_cnt = 0
         for i in self.pg_interfaces:
             if self.flows.__contains__(i):
                 pkts = self.create_stream(i, self.pg_if_packet_sizes,
-                                          traffic_type, ip_type, proto, ports)
+                                          traffic_type, ip_type, proto, ports,
+                                          frags)
                 if len(pkts) > 0:
                     i.add_stream(pkts)
                     pkts_cnt += len(pkts)
@@ -408,13 +417,14 @@ class TestACLplugin(VppTestCase):
                     self.verify_capture(dst_if, capture, traffic_type, ip_type)
 
     def run_verify_negat_test(self, traffic_type=0, ip_type=0, proto=-1,
-                              ports=0):
+                              ports=0, frags=False):
         # Test
         self.reset_packet_infos()
         for i in self.pg_interfaces:
             if self.flows.__contains__(i):
                 pkts = self.create_stream(i, self.pg_if_packet_sizes,
-                                          traffic_type, ip_type, proto, ports)
+                                          traffic_type, ip_type, proto, ports,
+                                          frags)
                 if len(pkts) > 0:
                     i.add_stream(pkts)
 
@@ -1011,5 +1021,32 @@ class TestACLplugin(VppTestCase):
 
         self.logger.info("ACLP_TEST_FINISH_0020")
 
+    def test_0021_udp_deny_port_verify_fragment_deny(self):
+        """ deny single UDPv4/v6, permit ip any, verify non-initial fragment blocked
+        """
+        self.logger.info("ACLP_TEST_START_0021")
+
+        port = random.randint(0, 65535)
+        # Add an ACL
+        rules = []
+        rules.append(self.create_rule(self.IPV4, self.DENY, port,
+                                      self.proto[self.IP][self.UDP]))
+        rules.append(self.create_rule(self.IPV6, self.DENY, port,
+                                      self.proto[self.IP][self.UDP]))
+        # deny ip any any in the end
+        rules.append(self.create_rule(self.IPV4, self.PERMIT,
+                                      self.PORTS_ALL, 0))
+        rules.append(self.create_rule(self.IPV6, self.PERMIT,
+                                      self.PORTS_ALL, 0))
+
+        # Apply rules
+        self.apply_rules(rules, "deny ip4/ip6 udp "+str(port))
+
+        # Traffic should not pass
+        self.run_verify_negat_test(self.IP, self.IPRANDOM,
+                                   self.proto[self.IP][self.UDP], port, True)
+
+        self.logger.info("ACLP_TEST_FINISH_0021")
+
 if __name__ == '__main__':
     unittest.main(testRunner=VppTestRunner)
diff --git a/test/test_acl_plugin_l2l3.py b/test/test_acl_plugin_l2l3.py
index 346825fceec..32abf184bb9 100644
--- a/test/test_acl_plugin_l2l3.py
+++ b/test/test_acl_plugin_l2l3.py
@@ -33,6 +33,7 @@ from scapy.layers.l2 import Ether
 from scapy.layers.inet import IP, UDP, ICMP, TCP
 from scapy.layers.inet6 import IPv6, ICMPv6Unknown, ICMPv6EchoRequest
 from scapy.layers.inet6 import ICMPv6EchoReply, IPv6ExtHdrRouting
+from scapy.layers.inet6 import IPv6ExtHdrFragment
 
 from framework import VppTestCase, VppTestRunner
 import time
@@ -203,7 +204,7 @@ class TestIpIrb(VppTestCase):
                     if add_extension_header:
                         # prepend some extension headers
                         ulp = (IPv6ExtHdrRouting() / IPv6ExtHdrRouting() /
-                               IPv6ExtHdrRouting() / ulp_l4)
+                               IPv6ExtHdrFragment(offset=0, m=1) / ulp_l4)
                         # uncomment below to test invalid ones
                         # ulp = IPv6ExtHdrRouting(len = 200) / ulp_l4
                     else:
@@ -214,10 +215,12 @@ class TestIpIrb(VppTestCase):
                          Raw(payload))
                 else:
                     ulp_l4 = UDP(sport=src_l4, dport=dst_l4)
-                    # IPv4 does not allow extension headers
+                    # IPv4 does not allow extension headers,
+                    # but we rather make it a first fragment
+                    flags = 1 if add_extension_header else 0
                     ulp = ulp_l4
                     p = (Ether(dst=dst_mac, src=src_mac) /
-                         IP(src=src_ip4, dst=dst_ip4) /
+                         IP(src=src_ip4, dst=dst_ip4, frag=0, flags=flags) /
                          ulp /
                          Raw(payload))
             elif modulo == 1:
@@ -670,6 +673,48 @@ class TestIpIrb(VppTestCase):
         self.run_test_ip46_bridged_to_routed_and_back(False, True,
                                                       self.WITH_EH)
 
+    # IPv4 with "MF" bit set
+
+    def test_1201_ip6_irb_1(self):
+        """ ACL IPv4+MF routed -> bridged, L2 ACL deny"""
+        self.run_test_ip46_routed_to_bridged(True, False, False,
+                                             self.WITH_EH)
+
+    def test_1202_ip6_irb_1(self):
+        """ ACL IPv4+MF routed -> bridged, L3 ACL deny"""
+        self.run_test_ip46_routed_to_bridged(False, False, False,
+                                             self.WITH_EH)
+
+    def test_1205_ip6_irb_1(self):
+        """ ACL IPv4+MF bridged -> routed, L2 ACL deny """
+        self.run_test_ip46_bridged_to_routed(True, False, False,
+                                             self.WITH_EH)
+
+    def test_1206_ip6_irb_1(self):
+        """ ACL IPv4+MF bridged -> routed, L3 ACL deny """
+        self.run_test_ip46_bridged_to_routed(False, False, False,
+                                             self.WITH_EH)
+
+    def test_1301_ip6_irb_1(self):
+        """ ACL IPv4+MF routed -> bridged, L2 ACL permit+reflect"""
+        self.run_test_ip46_routed_to_bridged_and_back(True, False,
+                                                      self.WITH_EH)
+
+    def test_1302_ip6_irb_1(self):
+        """ ACL IPv4+MF bridged -> routed, L2 ACL permit+reflect"""
+        self.run_test_ip46_bridged_to_routed_and_back(True, False,
+                                                      self.WITH_EH)
+
+    def test_1311_ip6_irb_1(self):
+        """ ACL IPv4+MF routed -> bridged, L3 ACL permit+reflect"""
+        self.run_test_ip46_routed_to_bridged_and_back(False, False,
+                                                      self.WITH_EH)
+
+    def test_1312_ip6_irb_1(self):
+        """ ACL IPv4+MF bridged -> routed, L3 ACL permit+reflect"""
+        self.run_test_ip46_bridged_to_routed_and_back(False, False,
+                                                      self.WITH_EH)
+
     # Old datapath group
     def test_8900_ip6_irb_1(self):
         """ ACL plugin set old L2 datapath"""