summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAlexander Bluhm <bluhm@cvs.openbsd.org>2017-05-12 23:05:59 +0000
committerAlexander Bluhm <bluhm@cvs.openbsd.org>2017-05-12 23:05:59 +0000
commit664a998e8b54a61e434fa72282229da0faf625da (patch)
tree49d7f063ada71eec68d17669083bd3d628e00b6d
parent48f28640af9ee122465351110d8a5c85c20a3d2a (diff)
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@
-rw-r--r--sys/netinet/ip_input.c29
-rw-r--r--sys/netinet/ip_var.h3
-rw-r--r--sys/netinet/ipsec_input.c60
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 */