From: Klement Sekera <ksekera@cisco.com>
Date: Tue, 27 Jul 2021 11:33:51 +0000 (+0200)
Subject: nat: fix ICMP checksum validation
X-Git-Tag: v22.02-rc0~163
X-Git-Url: https://gerrit.fd.io/r/gitweb?a=commitdiff_plain;h=254c803612c0c9ec5dfc1b90de6efb23ec5bedd5;p=vpp.git

nat: fix ICMP checksum validation

Handle case where extra data is present in buffer which is not part of
IP/ICMP headers.

Type: fix
Fixes: 05b5a5b3b4b04823776feed6403b5a99b2e06d76
Change-Id: Icfef811470056d38c60fc45cc302139ed7594385
Signed-off-by: Klement Sekera <ksekera@cisco.com>
---

diff --git a/src/plugins/nat/nat44-ed/nat44_ed.c b/src/plugins/nat/nat44-ed/nat44_ed.c
index 4e13907a9d8..8ad971decea 100644
--- a/src/plugins/nat/nat44-ed/nat44_ed.c
+++ b/src/plugins/nat/nat44-ed/nat44_ed.c
@@ -3694,15 +3694,6 @@ nat_6t_flow_icmp_translate (vlib_main_t *vm, snat_main_t *sm, vlib_buffer_t *b,
 	  return NAT_ED_TRNSL_ERR_PACKET_TRUNCATED;
 	}
 
-      ssize_t icmp_offset = (u8 *) icmp - (u8 *) vlib_buffer_get_current (b);
-      ip_csum_t sum =
-	ip_incremental_checksum (0, icmp, b->current_length - icmp_offset);
-      sum = (u16) ~ip_csum_fold (sum);
-      if (sum != 0)
-	{
-	  return NAT_ED_TRNSL_ERR_INVALID_CSUM;
-	}
-
       if (!icmp_type_is_error_message (icmp->type))
 	{
 	  if ((f->ops & NAT_FLOW_OP_ICMP_ID_REWRITE) &&
@@ -3718,6 +3709,15 @@ nat_6t_flow_icmp_translate (vlib_main_t *vm, snat_main_t *sm, vlib_buffer_t *b,
 	}
       else
 	{
+	  ip_csum_t sum = ip_incremental_checksum (
+	    0, icmp,
+	    clib_net_to_host_u16 (ip->length) - ip4_header_bytes (ip));
+	  sum = (u16) ~ip_csum_fold (sum);
+	  if (sum != 0)
+	    {
+	      return NAT_ED_TRNSL_ERR_INVALID_CSUM;
+	    }
+
 	  // errors are not fragmented
 	  ip4_header_t *inner_ip = (ip4_header_t *) (echo + 1);
 
diff --git a/test/test_nat44_ed.py b/test/test_nat44_ed.py
index ee76ce845c6..b11e3c03cde 100644
--- a/test/test_nat44_ed.py
+++ b/test/test_nat44_ed.py
@@ -949,6 +949,50 @@ class TestNAT44ED(NAT44EDTestCase):
         self.logger.info(ppp("p2 packet:", p2))
         self.logger.info(ppp("capture packet:", capture))
 
+    def test_icmp_echo_reply_trailer(self):
+        """ ICMP echo reply with ethernet trailer"""
+
+        self.nat_add_address(self.nat_addr)
+        self.nat_add_inside_interface(self.pg0)
+        self.nat_add_outside_interface(self.pg1)
+
+        # in2out
+        p1 = (Ether(src=self.pg0.remote_mac, dst=self.pg0.local_mac) /
+              IP(src=self.pg0.remote_ip4, dst=self.pg1.remote_ip4) /
+              ICMP(type=8, id=0xabcd, seq=0))
+
+        self.pg0.add_stream(p1)
+        self.pg_enable_capture(self.pg_interfaces)
+        self.pg_start()
+        c = self.pg1.get_capture(1)[0]
+
+        self.logger.debug(self.vapi.cli("show trace"))
+
+        # out2in
+        p2 = (Ether(src=self.pg1.remote_mac, dst=self.pg1.local_mac) /
+              IP(src=self.pg1.remote_ip4, dst=self.nat_addr, id=0xee59) /
+              ICMP(type=0, id=c[ICMP].id, seq=0))
+
+        # force checksum calculation
+        p2 = p2.__class__(bytes(p2))
+
+        self.logger.debug(ppp("Packet before modification:", p2))
+
+        # hex representation of vss monitoring ethernet trailer
+        # this seems to be just added to end of packet without modifying
+        # IP or ICMP lengths / checksums
+        p2 = p2 / Raw("\x00\x00\x52\x54\x00\x46\xab\x04\x84\x18")
+        # change it so that IP/ICMP is unaffected
+        p2[IP].len = 28
+
+        self.logger.debug(ppp("Packet with added trailer:", p2))
+
+        self.pg1.add_stream(p2)
+        self.pg_enable_capture(self.pg_interfaces)
+        self.pg_start()
+
+        self.pg0.get_capture(1)
+
     def test_users_dump(self):
         """ NAT44ED API test - nat44_user_dump """