nat: harden ICMP handling

Verify that headers are not truncated and that checksums are valid.
Correct checksum computation in translation code.

Type: fix
Change-Id: I6acfcec4661411f83c86b15aafac90cd4538c0b5
Signed-off-by: Klement Sekera <ksekera@cisco.com>

5 files changed, 114 insertions, 49 deletions

diff --git a/src/plugins/nat/nat44-ed/nat44_ed.c b/src/plugins/nat/nat44-ed/nat44_ed.c
index 41028d50a90..dcd7ae0a140 100644
--- a/src/plugins/nat/nat44-ed/nat44_ed.c
+++ b/src/plugins/nat/nat44-ed/nat44_ed.c
@@ -3401,10 +3401,9 @@ nat_6t_l3_l4_csum_calc (nat_6t_flow_t *f)
     }
 }
 
-static_always_inline int nat_6t_flow_icmp_translate (snat_main_t *sm,
-						     vlib_buffer_t *b,
-						     ip4_header_t *ip,
-						     nat_6t_flow_t *f);
+static_always_inline int
+nat_6t_flow_icmp_translate (vlib_main_t *vm, snat_main_t *sm, vlib_buffer_t *b,
+			    ip4_header_t *ip, nat_6t_flow_t *f);
 
 static_always_inline void
 nat_6t_flow_ip4_translate (snat_main_t *sm, vlib_buffer_t *b, ip4_header_t *ip,
@@ -3481,7 +3480,16 @@ nat_6t_flow_ip4_translate (snat_main_t *sm, vlib_buffer_t *b, ip4_header_t *ip,
 }
 
 static_always_inline int
-nat_6t_flow_icmp_translate (snat_main_t *sm, vlib_buffer_t *b,
+it_fits (vlib_main_t *vm, vlib_buffer_t *b, void *object, size_t size)
+{
+  int result = ((u8 *) object + size <=
+		(u8 *) vlib_buffer_get_current (b) + b->current_length) &&
+	       vlib_object_within_buffer_data (vm, b, object, size);
+  return result;
+}
+
+static_always_inline int
+nat_6t_flow_icmp_translate (vlib_main_t *vm, snat_main_t *sm, vlib_buffer_t *b,
 			    ip4_header_t *ip, nat_6t_flow_t *f)
 {
   if (IP_PROTOCOL_ICMP != ip->protocol)
@@ -3492,8 +3500,19 @@ nat_6t_flow_icmp_translate (snat_main_t *sm, vlib_buffer_t *b,
 
   if ((!vnet_buffer (b)->ip.reass.is_non_first_fragment))
     {
-      if (icmp->checksum == 0)
-	icmp->checksum = 0xffff;
+      if (!it_fits (vm, b, icmp, sizeof (*icmp)))
+	{
+	  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))
 	{
@@ -3515,7 +3534,7 @@ nat_6t_flow_icmp_translate (snat_main_t *sm, vlib_buffer_t *b,
 
 	  if (!ip4_header_checksum_is_valid (inner_ip))
 	    {
-	      return NAT_ED_TRNSL_ERR_TRANSLATION_FAILED;
+	      return NAT_ED_TRNSL_ERR_INNER_IP_CORRUPT;
 	    }
 
 	  nat_protocol_t inner_proto =
@@ -3533,6 +3552,10 @@ nat_6t_flow_icmp_translate (snat_main_t *sm, vlib_buffer_t *b,
 	    {
 	    case NAT_PROTOCOL_UDP:
 	      udp = (udp_header_t *) (inner_ip + 1);
+	      if (!it_fits (vm, b, udp, sizeof (*udp)))
+		{
+		  return NAT_ED_TRNSL_ERR_PACKET_TRUNCATED;
+		}
 	      old_udp_sum = udp->checksum;
 	      nat_6t_flow_ip4_translate (sm, b, inner_ip, f, inner_proto,
 					 1 /* is_icmp_inner_ip4 */,
@@ -3546,12 +3569,14 @@ nat_6t_flow_icmp_translate (snat_main_t *sm, vlib_buffer_t *b,
 		ip_csum_update (new_icmp_sum, old_udp_sum, udp->checksum,
 				udp_header_t, checksum);
 	      new_icmp_sum = ip_csum_fold (new_icmp_sum);
-	      if (0xffff == new_icmp_sum)
-		new_icmp_sum = 0;
 	      icmp->checksum = new_icmp_sum;
 	      break;
 	    case NAT_PROTOCOL_TCP:
 	      tcp = (tcp_header_t *) (inner_ip + 1);
+	      if (!it_fits (vm, b, tcp, sizeof (*tcp)))
+		{
+		  return NAT_ED_TRNSL_ERR_PACKET_TRUNCATED;
+		}
 	      old_tcp_sum = tcp->checksum;
 	      nat_6t_flow_ip4_translate (sm, b, inner_ip, f, inner_proto,
 					 1 /* is_icmp_inner_ip4 */,
@@ -3565,14 +3590,16 @@ nat_6t_flow_icmp_translate (snat_main_t *sm, vlib_buffer_t *b,
 		ip_csum_update (new_icmp_sum, old_tcp_sum, tcp->checksum,
 				tcp_header_t, checksum);
 	      new_icmp_sum = ip_csum_fold (new_icmp_sum);
-	      if (0xffff == new_icmp_sum)
-		new_icmp_sum = 0;
 	      icmp->checksum = new_icmp_sum;
 	      break;
 	    case NAT_PROTOCOL_ICMP:
 	      if (f->ops & NAT_FLOW_OP_ICMP_ID_REWRITE)
 		{
 		  icmp46_header_t *inner_icmp = ip4_next_header (inner_ip);
+		  if (!it_fits (vm, b, inner_icmp, sizeof (*inner_icmp)))
+		    {
+		      return NAT_ED_TRNSL_ERR_PACKET_TRUNCATED;
+		    }
 		  icmp_echo_header_t *inner_echo =
 		    (icmp_echo_header_t *) (inner_icmp + 1);
 		  if (f->rewrite.icmp_id != inner_echo->identifier)
@@ -3602,9 +3629,10 @@ nat_6t_flow_icmp_translate (snat_main_t *sm, vlib_buffer_t *b,
 }
 
 static_always_inline nat_translation_error_e
-nat_6t_flow_buf_translate (snat_main_t *sm, vlib_buffer_t *b, ip4_header_t *ip,
-			   nat_6t_flow_t *f, nat_protocol_t proto,
-			   int is_output_feature, int is_i2o)
+nat_6t_flow_buf_translate (vlib_main_t *vm, snat_main_t *sm, vlib_buffer_t *b,
+			   ip4_header_t *ip, nat_6t_flow_t *f,
+			   nat_protocol_t proto, int is_output_feature,
+			   int is_i2o)
 {
   if (!is_output_feature && f->ops & NAT_FLOW_OP_TXFIB_REWRITE)
     {
@@ -3627,7 +3655,7 @@ nat_6t_flow_buf_translate (snat_main_t *sm, vlib_buffer_t *b, ip4_header_t *ip,
 				     0 /* is_icmp_inner_ip4 */,
 				     0 /* skip_saddr_rewrite */);
 	}
-      return nat_6t_flow_icmp_translate (sm, b, ip, f);
+      return nat_6t_flow_icmp_translate (vm, sm, b, ip, f);
     }
 
   nat_6t_flow_ip4_translate (sm, b, ip, f, proto, 0 /* is_icmp_inner_ip4 */,
@@ -3637,20 +3665,22 @@ nat_6t_flow_buf_translate (snat_main_t *sm, vlib_buffer_t *b, ip4_header_t *ip,
 }
 
 nat_translation_error_e
-nat_6t_flow_buf_translate_i2o (snat_main_t *sm, vlib_buffer_t *b,
-			       ip4_header_t *ip, nat_6t_flow_t *f,
-			       nat_protocol_t proto, int is_output_feature)
+nat_6t_flow_buf_translate_i2o (vlib_main_t *vm, snat_main_t *sm,
+			       vlib_buffer_t *b, ip4_header_t *ip,
+			       nat_6t_flow_t *f, nat_protocol_t proto,
+			       int is_output_feature)
 {
-  return nat_6t_flow_buf_translate (sm, b, ip, f, proto, is_output_feature,
+  return nat_6t_flow_buf_translate (vm, sm, b, ip, f, proto, is_output_feature,
 				    1 /* is_i2o */);
 }
 
 nat_translation_error_e
-nat_6t_flow_buf_translate_o2i (snat_main_t *sm, vlib_buffer_t *b,
-			       ip4_header_t *ip, nat_6t_flow_t *f,
-			       nat_protocol_t proto, int is_output_feature)
+nat_6t_flow_buf_translate_o2i (vlib_main_t *vm, snat_main_t *sm,
+			       vlib_buffer_t *b, ip4_header_t *ip,
+			       nat_6t_flow_t *f, nat_protocol_t proto,
+			       int is_output_feature)
 {
-  return nat_6t_flow_buf_translate (sm, b, ip, f, proto, is_output_feature,
+  return nat_6t_flow_buf_translate (vm, sm, b, ip, f, proto, is_output_feature,
 				    0 /* is_i2o */);
 }
 
@@ -3683,6 +3713,15 @@ format_nat_ed_translation_error (u8 *s, va_list *args)
     case NAT_ED_TRNSL_ERR_FLOW_MISMATCH:
       s = format (s, "flow-mismatch");
       break;
+    case NAT_ED_TRNSL_ERR_PACKET_TRUNCATED:
+      s = format (s, "packet-truncated");
+      break;
+    case NAT_ED_TRNSL_ERR_INNER_IP_CORRUPT:
+      s = format (s, "inner-ip-corrupted");
+      break;
+    case NAT_ED_TRNSL_ERR_INVALID_CSUM:
+      s = format (s, "invalid-checksum");
+      break;
     }
   return s;
 }
diff --git a/src/plugins/nat/nat44-ed/nat44_ed.h b/src/plugins/nat/nat44-ed/nat44_ed.h
index 15e8e480b4e..6abdbadae43 100644
--- a/src/plugins/nat/nat44-ed/nat44_ed.h
+++ b/src/plugins/nat/nat44-ed/nat44_ed.h
@@ -125,13 +125,14 @@ typedef enum
 #undef _
 } snat_session_state_t;
 
-#define foreach_nat_in2out_ed_error                     \
-_(UNSUPPORTED_PROTOCOL, "unsupported protocol")         \
-_(OUT_OF_PORTS, "out of ports")                         \
-_(BAD_ICMP_TYPE, "unsupported ICMP type")               \
-_(MAX_SESSIONS_EXCEEDED, "maximum sessions exceeded")   \
-_(NON_SYN, "non-SYN packet try to create session")      \
-_(TCP_CLOSED, "drops due to TCP in transitory timeout")
+#define foreach_nat_in2out_ed_error                                           \
+  _ (UNSUPPORTED_PROTOCOL, "unsupported protocol")                            \
+  _ (OUT_OF_PORTS, "out of ports")                                            \
+  _ (BAD_ICMP_TYPE, "unsupported ICMP type")                                  \
+  _ (MAX_SESSIONS_EXCEEDED, "maximum sessions exceeded")                      \
+  _ (NON_SYN, "non-SYN packet try to create session")                         \
+  _ (TCP_CLOSED, "drops due to TCP in transitory timeout")                    \
+  _ (TRNSL_FAILED, "couldn't translate packet")
 
 typedef enum
 {
@@ -149,7 +150,8 @@ typedef enum
   _ (MAX_SESSIONS_EXCEEDED, "maximum sessions exceeded")                      \
   _ (NON_SYN, "non-SYN packet try to create session")                         \
   _ (TCP_CLOSED, "drops due to TCP in transitory timeout")                    \
-  _ (HASH_ADD_FAILED, "hash table add failed")
+  _ (HASH_ADD_FAILED, "hash table add failed")                                \
+  _ (TRNSL_FAILED, "couldn't translate packet")
 
 typedef enum
 {
@@ -1132,17 +1134,18 @@ typedef enum
   NAT_ED_TRNSL_ERR_SUCCESS = 0,
   NAT_ED_TRNSL_ERR_TRANSLATION_FAILED = 1,
   NAT_ED_TRNSL_ERR_FLOW_MISMATCH = 2,
+  NAT_ED_TRNSL_ERR_PACKET_TRUNCATED = 3,
+  NAT_ED_TRNSL_ERR_INNER_IP_CORRUPT = 4,
+  NAT_ED_TRNSL_ERR_INVALID_CSUM = 5,
 } nat_translation_error_e;
 
-nat_translation_error_e
-nat_6t_flow_buf_translate_i2o (snat_main_t *sm, vlib_buffer_t *b,
-			       ip4_header_t *ip, nat_6t_flow_t *f,
-			       nat_protocol_t proto, int is_output_feature);
+nat_translation_error_e nat_6t_flow_buf_translate_i2o (
+  vlib_main_t *vm, snat_main_t *sm, vlib_buffer_t *b, ip4_header_t *ip,
+  nat_6t_flow_t *f, nat_protocol_t proto, int is_output_feature);
 
-nat_translation_error_e
-nat_6t_flow_buf_translate_o2i (snat_main_t *sm, vlib_buffer_t *b,
-			       ip4_header_t *ip, nat_6t_flow_t *f,
-			       nat_protocol_t proto, int is_output_feature);
+nat_translation_error_e nat_6t_flow_buf_translate_o2i (
+  vlib_main_t *vm, snat_main_t *sm, vlib_buffer_t *b, ip4_header_t *ip,
+  nat_6t_flow_t *f, nat_protocol_t proto, int is_output_feature);
 
 void nat_6t_l3_l4_csum_calc (nat_6t_flow_t *f);
 
diff --git a/src/plugins/nat/nat44-ed/nat44_ed_in2out.c b/src/plugins/nat/nat44-ed/nat44_ed_in2out.c
index bfabdbd71d0..ead5685c6f7 100644
--- a/src/plugins/nat/nat44-ed/nat44_ed_in2out.c
+++ b/src/plugins/nat/nat44-ed/nat44_ed_in2out.c
@@ -1177,16 +1177,18 @@ nat44_ed_in2out_fast_path_node_fn_inline (vlib_main_t *vm,
 	  nat_free_session_data (sm, s0, thread_index, 0);
 	  nat_ed_session_delete (sm, s0, thread_index, 1);
 	  next[0] = NAT_NEXT_DROP;
+	  b0->error = node->errors[NAT_IN2OUT_ED_ERROR_TRNSL_FAILED];
 	  goto trace0;
 	}
 
       if (NAT_ED_TRNSL_ERR_SUCCESS !=
 	  (translation_error = nat_6t_flow_buf_translate_i2o (
-	     sm, b0, ip0, f, proto0, is_output_feature)))
+	     vm, sm, b0, ip0, f, proto0, is_output_feature)))
 	{
 	  nat_free_session_data (sm, s0, thread_index, 0);
 	  nat_ed_session_delete (sm, s0, thread_index, 1);
 	  next[0] = NAT_NEXT_DROP;
+	  b0->error = node->errors[NAT_IN2OUT_ED_ERROR_TRNSL_FAILED];
 	  goto trace0;
 	}
 
@@ -1330,8 +1332,12 @@ nat44_ed_in2out_slow_path_node_fn_inline (vlib_main_t *vm,
 	  if (NAT_NEXT_DROP != next[0] && s0 &&
 	      NAT_ED_TRNSL_ERR_SUCCESS !=
 		(translation_error = nat_6t_flow_buf_translate_i2o (
-		   sm, b0, ip0, &s0->i2o, proto0, is_output_feature)))
+		   vm, sm, b0, ip0, &s0->i2o, proto0, is_output_feature)))
 	    {
+	      nat_free_session_data (sm, s0, thread_index, 0);
+	      nat_ed_session_delete (sm, s0, thread_index, 1);
+	      next[0] = NAT_NEXT_DROP;
+	      b0->error = node->errors[NAT_IN2OUT_ED_ERROR_TRNSL_FAILED];
 	      goto trace0;
 	    }
 
@@ -1348,8 +1354,12 @@ nat44_ed_in2out_slow_path_node_fn_inline (vlib_main_t *vm,
 	  if (NAT_NEXT_DROP != next[0] && s0 &&
 	      NAT_ED_TRNSL_ERR_SUCCESS !=
 		(translation_error = nat_6t_flow_buf_translate_i2o (
-		   sm, b0, ip0, &s0->i2o, proto0, is_output_feature)))
+		   vm, sm, b0, ip0, &s0->i2o, proto0, is_output_feature)))
 	    {
+	      nat_free_session_data (sm, s0, thread_index, 0);
+	      nat_ed_session_delete (sm, s0, thread_index, 1);
+	      next[0] = NAT_NEXT_DROP;
+	      b0->error = node->errors[NAT_IN2OUT_ED_ERROR_TRNSL_FAILED];
 	      goto trace0;
 	    }
 
@@ -1425,11 +1435,12 @@ nat44_ed_in2out_slow_path_node_fn_inline (vlib_main_t *vm,
 
       if (NAT_ED_TRNSL_ERR_SUCCESS !=
 	  (translation_error = nat_6t_flow_buf_translate_i2o (
-	     sm, b0, ip0, &s0->i2o, proto0, is_output_feature)))
+	     vm, sm, b0, ip0, &s0->i2o, proto0, is_output_feature)))
 	{
 	  nat_free_session_data (sm, s0, thread_index, 0);
 	  nat_ed_session_delete (sm, s0, thread_index, 1);
-	  s0 = NULL;
+	  next[0] = NAT_NEXT_DROP;
+	  b0->error = node->errors[NAT_IN2OUT_ED_ERROR_TRNSL_FAILED];
 	  goto trace0;
 	}
 
diff --git a/src/plugins/nat/nat44-ed/nat44_ed_out2in.c b/src/plugins/nat/nat44-ed/nat44_ed_out2in.c
index eaf89937e54..124b64e29f3 100644
--- a/src/plugins/nat/nat44-ed/nat44_ed_out2in.c
+++ b/src/plugins/nat/nat44-ed/nat44_ed_out2in.c
@@ -1020,6 +1020,7 @@ nat44_ed_out2in_fast_path_node_fn_inline (vlib_main_t * vm,
 		  nat_free_session_data (sm, s0, thread_index, 0);
 		  nat_ed_session_delete (sm, s0, thread_index, 1);
 		  next[0] = NAT_NEXT_DROP;
+		  b0->error = node->errors[NAT_OUT2IN_ED_ERROR_TRNSL_FAILED];
 		  goto trace0;
 		}
 	    }
@@ -1027,9 +1028,10 @@ nat44_ed_out2in_fast_path_node_fn_inline (vlib_main_t * vm,
 
       if (NAT_ED_TRNSL_ERR_SUCCESS !=
 	  (translation_error = nat_6t_flow_buf_translate_o2i (
-	     sm, b0, ip0, f, proto0, 0 /* is_output_feature */)))
+	     vm, sm, b0, ip0, f, proto0, 0 /* is_output_feature */)))
 	{
 	  next[0] = NAT_NEXT_DROP;
+	  b0->error = node->errors[NAT_OUT2IN_ED_ERROR_TRNSL_FAILED];
 	  goto trace0;
 	}
 
@@ -1183,8 +1185,11 @@ nat44_ed_out2in_slow_path_node_fn_inline (vlib_main_t * vm,
 	  if (NAT_NEXT_DROP != next[0] && s0 &&
 	      NAT_ED_TRNSL_ERR_SUCCESS !=
 		(translation_error = nat_6t_flow_buf_translate_o2i (
-		   sm, b0, ip0, &s0->o2i, proto0, 0 /* is_output_feature */)))
+		   vm, sm, b0, ip0, &s0->o2i, proto0,
+		   0 /* is_output_feature */)))
 	    {
+	      next[0] = NAT_NEXT_DROP;
+	      b0->error = node->errors[NAT_OUT2IN_ED_ERROR_TRNSL_FAILED];
 	      goto trace0;
 	    }
 
@@ -1202,8 +1207,11 @@ nat44_ed_out2in_slow_path_node_fn_inline (vlib_main_t * vm,
 	  if (NAT_NEXT_DROP != next[0] && s0 &&
 	      NAT_ED_TRNSL_ERR_SUCCESS !=
 		(translation_error = nat_6t_flow_buf_translate_o2i (
-		   sm, b0, ip0, &s0->o2i, proto0, 0 /* is_output_feature */)))
+		   vm, sm, b0, ip0, &s0->o2i, proto0,
+		   0 /* is_output_feature */)))
 	    {
+	      next[0] = NAT_NEXT_DROP;
+	      b0->error = node->errors[NAT_OUT2IN_ED_ERROR_TRNSL_FAILED];
 	      goto trace0;
 	    }
 
@@ -1312,7 +1320,7 @@ nat44_ed_out2in_slow_path_node_fn_inline (vlib_main_t * vm,
 
       if (NAT_ED_TRNSL_ERR_SUCCESS !=
 	  (translation_error = nat_6t_flow_buf_translate_o2i (
-	     sm, b0, ip0, &s0->o2i, proto0, 0 /* is_output_feature */)))
+	     vm, sm, b0, ip0, &s0->o2i, proto0, 0 /* is_output_feature */)))
 	{
 	  next[0] = NAT_NEXT_DROP;
 	  goto trace0;
diff --git a/test/test_nat44_ed.py b/test/test_nat44_ed.py
index b8774a20d3d..ee76ce845c6 100644
--- a/test/test_nat44_ed.py
+++ b/test/test_nat44_ed.py
@@ -1215,6 +1215,7 @@ class TestNAT44ED(NAT44EDTestCase):
         self.pg_start()
         capture = self.pg1.get_capture(len(pkts))
         self.verify_capture_out(capture, ignore_port=True)
+        self.logger.debug(self.vapi.cli("show trace"))
 
         # out2in
         pkts = self.create_stream_out(self.pg1)
@@ -1223,6 +1224,7 @@ class TestNAT44ED(NAT44EDTestCase):
         self.pg_start()
         capture = self.pg0.get_capture(len(pkts))
         self.verify_capture_in(capture, self.pg0)
+        self.logger.debug(self.vapi.cli("show trace"))
 
         # in2out
         pkts = self.create_stream_in(self.pg0, self.pg1, ttl=2)
@@ -1231,6 +1233,7 @@ class TestNAT44ED(NAT44EDTestCase):
         self.pg_start()
         capture = self.pg1.get_capture(len(pkts))
         self.verify_capture_out(capture, ignore_port=True)
+        self.logger.debug(self.vapi.cli("show trace"))
 
         # out2in
         pkts = self.create_stream_out(self.pg1, ttl=2)
@@ -1239,6 +1242,7 @@ class TestNAT44ED(NAT44EDTestCase):
         self.pg_start()
         capture = self.pg0.get_capture(len(pkts))
         self.verify_capture_in(capture, self.pg0)
+        self.logger.debug(self.vapi.cli("show trace"))
 
         # in2out
         pkts = self.create_stream_in(self.pg0, self.pg1, ttl=1)