svn commit: r345377 - in stable/12: sys/netpfil/pf tests/sys/netpfil/pf

[Kristof Provost]

Author: kp
Date: Thu Mar 21 14:17:10 2019
New Revision: 345377
URL: https://svnweb.freebsd.org/changeset/base/345377

Log:
  MFC r345366:
  
  pf: Ensure that IP addresses match in ICMP error packets
  
  States in pf(4) let ICMP and ICMP6 packets pass if they have a
  packet in their payload that matches an exiting connection.  It was
  not checked whether the outer ICMP packet has the same destination
  IP as the source IP of the inner protocol packet.  Enforce that
  these addresses match, to prevent ICMP packets that do not make
  sense.
  
  Reported by:	Nicolas Collignon, Corentin Bayet, Eloi Vanderbeken, Luca Moro at Synacktiv
  Obtained from:	OpenBSD
  Security:	CVE-2019-5598

Modified:
  stable/12/sys/netpfil/pf/pf.c
  stable/12/tests/sys/netpfil/pf/pft_ping.py
Directory Properties:
  stable/12/   (props changed)

Modified: stable/12/sys/netpfil/pf/pf.c
==============================================================================
--- stable/12/sys/netpfil/pf/pf.c	Thu Mar 21 13:30:48 2019	(r345376)
+++ stable/12/sys/netpfil/pf/pf.c	Thu Mar 21 14:17:10 2019	(r345377)
@@ -4596,7 +4596,7 @@ pf_test_state_icmp(struct pf_state **state, int direct
 {
 	struct pf_addr  *saddr = pd->src, *daddr = pd->dst;
 	u_int16_t	 icmpid = 0, *icmpsum;
-	u_int8_t	 icmptype;
+	u_int8_t	 icmptype, icmpcode;
 	int		 state_icmp = 0;
 	struct pf_state_key_cmp key;
 
@@ -4605,6 +4605,7 @@ pf_test_state_icmp(struct pf_state **state, int direct
 #ifdef INET
 	case IPPROTO_ICMP:
 		icmptype = pd->hdr.icmp->icmp_type;
+		icmpcode = pd->hdr.icmp->icmp_code;
 		icmpid = pd->hdr.icmp->icmp_id;
 		icmpsum = &pd->hdr.icmp->icmp_cksum;
 
@@ -4619,6 +4620,7 @@ pf_test_state_icmp(struct pf_state **state, int direct
 #ifdef INET6
 	case IPPROTO_ICMPV6:
 		icmptype = pd->hdr.icmp6->icmp6_type;
+		icmpcode = pd->hdr.icmp6->icmp6_code;
 		icmpid = pd->hdr.icmp6->icmp6_id;
 		icmpsum = &pd->hdr.icmp6->icmp6_cksum;
 
@@ -4817,6 +4819,23 @@ pf_test_state_icmp(struct pf_state **state, int direct
 #endif /* INET6 */
 		}
 
+		if (PF_ANEQ(pd->dst, pd2.src, pd->af)) {
+			if (V_pf_status.debug >= PF_DEBUG_MISC) {
+				printf("pf: BAD ICMP %d:%d outer dst: ",
+				    icmptype, icmpcode);
+				pf_print_host(pd->src, 0, pd->af);
+				printf(" -> ");
+				pf_print_host(pd->dst, 0, pd->af);
+				printf(" inner src: ");
+				pf_print_host(pd2.src, 0, pd2.af);
+				printf(" -> ");
+				pf_print_host(pd2.dst, 0, pd2.af);
+				printf("\n");
+			}
+			REASON_SET(reason, PFRES_BADSTATE);
+			return (PF_DROP);
+		}
+
 		switch (pd2.proto) {
 		case IPPROTO_TCP: {
 			struct tcphdr		 th;
@@ -4873,7 +4892,7 @@ pf_test_state_icmp(struct pf_state **state, int direct
 			    !SEQ_GEQ(seq, src->seqlo - (dst->max_win << dws)))) {
 				if (V_pf_status.debug >= PF_DEBUG_MISC) {
 					printf("pf: BAD ICMP %d:%d ",
-					    icmptype, pd->hdr.icmp->icmp_code);
+					    icmptype, icmpcode);
 					pf_print_host(pd->src, 0, pd->af);
 					printf(" -> ");
 					pf_print_host(pd->dst, 0, pd->af);
@@ -4886,7 +4905,7 @@ pf_test_state_icmp(struct pf_state **state, int direct
 			} else {
 				if (V_pf_status.debug >= PF_DEBUG_MISC) {
 					printf("pf: OK ICMP %d:%d ",
-					    icmptype, pd->hdr.icmp->icmp_code);
+					    icmptype, icmpcode);
 					pf_print_host(pd->src, 0, pd->af);
 					printf(" -> ");
 					pf_print_host(pd->dst, 0, pd->af);

Modified: stable/12/tests/sys/netpfil/pf/pft_ping.py
==============================================================================
--- stable/12/tests/sys/netpfil/pf/pft_ping.py	Thu Mar 21 13:30:48 2019	(r345376)
+++ stable/12/tests/sys/netpfil/pf/pft_ping.py	Thu Mar 21 14:17:10 2019	(r345377)
@@ -8,26 +8,38 @@ import threading
 PAYLOAD_MAGIC = 0x42c0ffee
 
 class Sniffer(threading.Thread):
-	def __init__(self, recvif):
+	def __init__(self, args, check_function):
 		threading.Thread.__init__(self)
 
-		self._recvif = recvif
+		self._args = args
+		self._recvif = args.recvif[0]
+		self._check_function = check_function
+		self.foundCorrectPacket = False
 
 		self.start()
 
+	def _checkPacket(self, packet):
+		ret = self._check_function(self._args, packet)
+		if ret:
+			self.foundCorrectPacket = True
+		return ret
+
 	def run(self):
-		self.packets = sp.sniff(iface=self._recvif, timeout=3)
+		self.packets = sp.sniff(iface=self._recvif,
+				stop_filter=self._checkPacket, timeout=3)
 
-def check_ping_request(packet, dst_ip, args):
+def check_ping_request(args, packet):
 	if args.ip6:
-		return check_ping6_request(packet, dst_ip, args)
+		return check_ping6_request(args, packet)
 	else:
-		return check_ping4_request(packet, dst_ip, args)
+		return check_ping4_request(args, packet)
 
-def check_ping4_request(packet, dst_ip, args):
+def check_ping4_request(args, packet):
 	"""
 	Verify that the packet matches what we'd have sent
 	"""
+	dst_ip = args.to[0]
+
 	ip = packet.getlayer(sp.IP)
 	if not ip:
 		return False
@@ -54,13 +66,14 @@ def check_ping4_request(packet, dst_ip, args):
 				% (ip.tos, args.expect_tos[0])
 			return False
 
-
 	return True
 
-def check_ping6_request(packet, dst_ip, args):
+def check_ping6_request(args, packet):
 	"""
 	Verify that the packet matches what we'd have sent
 	"""
+	dst_ip = args.to[0]
+
 	ip = packet.getlayer(sp.IPv6)
 	if not ip:
 		return False
@@ -124,7 +137,7 @@ def main():
 
 	sniffer = None
 	if not args.recvif is None:
-		sniffer = Sniffer(args.recvif[0])
+		sniffer = Sniffer(args, check_ping_request)
 
 	if args.ip6:
 		ping6(args.sendif[0], args.to[0], args)
@@ -134,12 +147,10 @@ def main():
 	if sniffer:
 		sniffer.join()
 
-		for packet in sniffer.packets:
-			if check_ping_request(packet, args.to[0], args):
-				sys.exit(0)
-
-		# We did not get the packet we expected
-		sys.exit(1)
+		if sniffer.foundCorrectPacket:
+			sys.exit(0)
+		else:
+			sys.exit(1)
 
 if __name__ == '__main__':
 	main()