IPsec packets were passed through ip_input() a second time after

they have been decrypted.  That means that all the IP header fields
were checked twice.  Also fragment reassembly was tried twice.
At pf incoming packets in tunnel mode appeared twice on the enc0
interface, once as IP-in-IP and once as the inner packet.  In the
outgoing path pf only sees the inner packet.  Asymmetry is bad for
stateful filtering.
IPv6 shows that IPsec works without that.  After decrypting immediately
continue with local delivery.  In tunnel mode the IP-in-IP protocol
functions pass the inner header to ip6_input().  In transport mode
only pf_test() has to be called for the enc0 device.
Introduce ip_local() to avoid needless processing and cleaner pf
behavior in IPv4 IPsec.
OK mikeb@

3 files changed, 48 insertions, 44 deletions

diff --git a/sys/netinet/ip_input.c b/sys/netinet/ip_input.c
index 0cb0036968c..780190950cc 100644
--- a/sys/netinet/ip_input.c
+++ b/sys/netinet/ip_input.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ip_input.c,v 1.300 2017/05/12 14:04:09 bluhm Exp $	*/
+/*	$OpenBSD: ip_input.c,v 1.301 2017/05/12 23:05:58 bluhm Exp $	*/
 /*	$NetBSD: ip_input.c,v 1.30 1996/03/16 23:53:58 christos Exp $	*/
 
 /*
@@ -483,9 +483,6 @@ ip_ours(struct mbuf *m)
 
 	hlen = ip->ip_hl << 2;
 
-	/* pf might have modified stuff, might have to chksum */
-	in_proto_cksum_out(m, NULL);
-
 	/*
 	 * If offset or IP_MF are set, must reassemble.
 	 * Otherwise, nothing need be done.
@@ -570,11 +567,26 @@ found:
 				ip_freef(fp);
 	}
 
+	ip_local(m, hlen, ip->ip_p);
+	return;
+bad:
+	m_freem(m);
+}
+
+void
+ip_local(struct mbuf *m, int off, int nxt)
+{
+	KERNEL_ASSERT_LOCKED();
+
+	/* pf might have modified stuff, might have to chksum */
+	in_proto_cksum_out(m, NULL);
+
 #ifdef IPSEC
 	if (ipsec_in_use) {
-		if (ip_input_ipsec_ours_check(m, hlen) != 0) {
+		if (ip_input_ipsec_ours_check(m, off) != 0) {
 			ipstat_inc(ips_cantforward);
-			goto bad;
+			m_freem(m);
+			return;
 		}
 	}
 	/* Otherwise, just fall through and deliver the packet */
@@ -584,10 +596,7 @@ found:
 	 * Switch out to protocol's input routine.
 	 */
 	ipstat_inc(ips_delivered);
-	(*inetsw[ip_protox[ip->ip_p]].pr_input)(&m, &hlen, ip->ip_p, AF_INET);
-	return;
-bad:
-	m_freem(m);
+	(*inetsw[ip_protox[nxt]].pr_input)(&m, &off, nxt, AF_INET);
 }
 
 int
diff --git a/sys/netinet/ip_var.h b/sys/netinet/ip_var.h
index 09a2b0dd042..4bc5998a08b 100644
--- a/sys/netinet/ip_var.h
+++ b/sys/netinet/ip_var.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ip_var.h,v 1.72 2017/05/12 14:04:09 bluhm Exp $	*/
+/*	$OpenBSD: ip_var.h,v 1.73 2017/05/12 23:05:58 bluhm Exp $	*/
 /*	$NetBSD: ip_var.h,v 1.16 1996/02/13 23:43:20 christos Exp $	*/
 
 /*
@@ -249,6 +249,7 @@ void	 ip_savecontrol(struct inpcb *, struct mbuf **, struct ip *,
 	    struct mbuf *);
 void	 ipintr(void);
 void	 ipv4_input(struct mbuf *);
+void	 ip_local(struct mbuf *, int, int);
 void	 ip_forward(struct mbuf *, struct ifnet *, struct rtentry *, int);
 int	 ip_input_ipsec_fwd_check(struct mbuf *, int, int);
 int	 rip_ctloutput(int, struct socket *, int, int, struct mbuf *);
diff --git a/sys/netinet/ipsec_input.c b/sys/netinet/ipsec_input.c
index 9cfb4c28c01..8b275147443 100644
--- a/sys/netinet/ipsec_input.c
+++ b/sys/netinet/ipsec_input.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ipsec_input.c,v 1.150 2017/05/12 14:31:23 bluhm Exp $	*/
+/*	$OpenBSD: ipsec_input.c,v 1.151 2017/05/12 23:05:58 bluhm Exp $	*/
 /*
  * The authors of this code are John Ioannidis (ji@tla.org),
  * Angelos D. Keromytis (kermit@csd.uch.gr) and
@@ -581,44 +581,38 @@ ipsec_common_input_cb(struct mbuf *m, struct tdb *tdbp, int skip, int protoff)
 	}
 #endif
 
+#if NPF > 0
+	/*
+	 * The ip_local() shortcut avoids running through ip_input() with the
+	 * same IP header twice.  Packets in transport mode have to be be
+	 * passed to pf explicitly.  In tunnel mode the inner IP header will
+	 * run through ip_input() and pf anyway.
+	 */
+	if ((tdbp->tdb_flags & TDBF_TUNNELING) == 0) {
+		struct ifnet *ifp;
+
+		/* This is the enc0 interface unless for ipcomp. */
+		if ((ifp = if_get(m->m_pkthdr.ph_ifidx)) == NULL) {
+			m_freem(m);
+			return;
+		}
+		if (pf_test(af, PF_IN, ifp, &m) != PF_PASS) {
+			if_put(ifp);
+			m_freem(m);
+			return;
+		}
+		if_put(ifp);
+		if (m == NULL)
+			return;
+	}
+#endif
 	/* Call the appropriate IPsec transform callback. */
 	switch (af) {
 	case AF_INET:
-		if (niq_enqueue(&ipintrq, m) != 0) {
-			DPRINTF(("ipsec_common_input_cb(): dropped packet "
-			    "because of full IP queue\n"));
-			IPSEC_ISTAT(espstat.esps_qfull, ahstat.ahs_qfull,
-			    ipcompstat.ipcomps_qfull);
-		}
+		ip_local(m, skip, prot);
 		return;
 #ifdef INET6
 	case AF_INET6:
-#if NPF > 0
-		/*
-		 * In the IPv4 case all packets run through ipv4_input()
-		 * twice.  As the ip6_local() shortcut avoids this, IPv6
-		 * packets in transport mode have to be be passed to pf
-		 * explicitly.  In tunnel mode the inner IP header will
-		 * run through ip_input() and pf already.
-		 */
-		if ((tdbp->tdb_flags & TDBF_TUNNELING) == 0) {
-			struct ifnet *ifp;
-
-			/* This is the enc0 interface unless for ipcomp. */
-			if ((ifp = if_get(m->m_pkthdr.ph_ifidx)) == NULL) {
-				m_freem(m);
-				return;
-			}
-			if (pf_test(AF_INET6, PF_IN, ifp, &m) != PF_PASS) {
-				if_put(ifp);
-				m_freem(m);
-				return;
-			}
-			if_put(ifp);
-			if (m == NULL)
-				return;
-		}
-#endif
 		ip6_local(m, skip, prot);
 		return;
 #endif /* INET6 */