From 1cca983d1bb1daccc62e83498b3dc3f64d78aef0 Mon Sep 17 00:00:00 2001 From: ae Date: Thu, 11 Dec 2014 14:43:44 +0000 Subject: Remove check for presence of PACKET_TAG_IPSEC_PENDING_TDB and PACKET_TAG_IPSEC_OUT_CRYPTO_NEEDED mbuf tags. They aren't used in FreeBSD. Instead check presence of PACKET_TAG_IPSEC_OUT_DONE mbuf tag. If it is found, bypass security policy lookup as described in the comment. PACKET_TAG_IPSEC_OUT_DONE tag added to mbuf when IPSEC code finishes ESP/AH processing. Since it was already finished, this means the security policy placed in the tdb_ident was already checked. And there is no reason to check it again here. Obtained from: Yandex LLC Sponsored by: Yandex LLC --- sys/netinet/ip_ipsec.c | 55 ++++++-------------------------------------------- 1 file changed, 6 insertions(+), 49 deletions(-) (limited to 'sys/netinet/ip_ipsec.c') diff --git a/sys/netinet/ip_ipsec.c b/sys/netinet/ip_ipsec.c index c1ca94a..e4efe1c 100644 --- a/sys/netinet/ip_ipsec.c +++ b/sys/netinet/ip_ipsec.c @@ -217,9 +217,7 @@ ip_ipsec_mtu(struct mbuf *m, int mtu) int ip_ipsec_output(struct mbuf **m, struct inpcb *inp, int *flags, int *error) { - struct secpolicy *sp = NULL; - struct tdb_ident *tdbi; - struct m_tag *mtag; + struct secpolicy *sp; /* * Check the security policy (SP) for the packet and, if * required, do IPsec-related processing. There are two @@ -229,17 +227,11 @@ ip_ipsec_output(struct mbuf **m, struct inpcb *inp, int *flags, int *error) * AH, ESP, etc. processing), there will be a tag to bypass * the lookup and related policy checking. */ - mtag = m_tag_find(*m, PACKET_TAG_IPSEC_PENDING_TDB, NULL); - if (mtag != NULL) { - tdbi = (struct tdb_ident *)(mtag + 1); - sp = ipsec_getpolicy(tdbi, IPSEC_DIR_OUTBOUND); - if (sp == NULL) - *error = -EINVAL; /* force silent drop */ - m_tag_delete(*m, mtag); - } else { - sp = ipsec4_checkpolicy(*m, IPSEC_DIR_OUTBOUND, *flags, - error, inp); + if (m_tag_find(*m, PACKET_TAG_IPSEC_OUT_DONE, NULL) != NULL) { + *error = 0; + return (0); } + sp = ipsec4_checkpolicy(*m, IPSEC_DIR_OUTBOUND, *flags, error, inp); /* * There are four return cases: * sp != NULL apply IPsec policy @@ -248,40 +240,6 @@ ip_ipsec_output(struct mbuf **m, struct inpcb *inp, int *flags, int *error) * sp == NULL, error != 0 discard packet, report error */ if (sp != NULL) { - /* Loop detection, check if ipsec processing already done */ - KASSERT(sp->req != NULL, ("ip_output: no ipsec request")); - for (mtag = m_tag_first(*m); mtag != NULL; - mtag = m_tag_next(*m, mtag)) { - if (mtag->m_tag_cookie != MTAG_ABI_COMPAT) - continue; - if (mtag->m_tag_id != PACKET_TAG_IPSEC_OUT_DONE && - mtag->m_tag_id != PACKET_TAG_IPSEC_OUT_CRYPTO_NEEDED) - continue; - /* - * Check if policy has an SA associated with it. - * This can happen when an SP has yet to acquire - * an SA; e.g. on first reference. If it occurs, - * then we let ipsec4_process_packet do its thing. - */ - if (sp->req->sav == NULL) - break; - tdbi = (struct tdb_ident *)(mtag + 1); - if (tdbi->spi == sp->req->sav->spi && - tdbi->proto == sp->req->sav->sah->saidx.proto && - bcmp(&tdbi->dst, &sp->req->sav->sah->saidx.dst, - sizeof (union sockaddr_union)) == 0) { - /* - * No IPsec processing is needed, free - * reference to SP. - * - * NB: null pointer to avoid free at - * done: below. - */ - KEY_FREESP(&sp), sp = NULL; - goto done; - } - } - /* * Do delayed checksums now because we send before * this is done in the normal processing path. @@ -332,9 +290,8 @@ ip_ipsec_output(struct mbuf **m, struct inpcb *inp, int *flags, int *error) if (*error == -EINVAL) *error = 0; goto bad; - } else { - /* No IPsec processing for this packet. */ } + /* No IPsec processing for this packet. */ } done: if (sp != NULL) -- cgit v1.1 From 8e6349d4bcea839f004de921402559ba85da1a5e Mon Sep 17 00:00:00 2001 From: ae Date: Thu, 11 Dec 2014 14:58:55 +0000 Subject: Remove PACKET_TAG_IPSEC_IN_DONE mbuf tag lookup and usage of its security policy. The changed block of code in ip*_ipsec_input() is called when packet has ESP/AH header. Presence of PACKET_TAG_IPSEC_IN_DONE mbuf tag in the same time means that packet was already handled by IPSEC and reinjected in the netisr, and it has another ESP/AH headers (encrypted twice?). Since it was already processed by IPSEC code, the AH/ESP headers was already stripped (and probably outer IP header was stripped too) and security policy from the tdb_ident was applied to those headers. It is incorrect to apply this security policy to current headers. Also make ip_ipsec_input() prototype similar to ip6_ipsec_input(). Obtained from: Yandex LLC Sponsored by: Yandex LLC --- sys/netinet/ip_ipsec.c | 30 +++++++----------------------- 1 file changed, 7 insertions(+), 23 deletions(-) (limited to 'sys/netinet/ip_ipsec.c') diff --git a/sys/netinet/ip_ipsec.c b/sys/netinet/ip_ipsec.c index e4efe1c..ad6dec2 100644 --- a/sys/netinet/ip_ipsec.c +++ b/sys/netinet/ip_ipsec.c @@ -146,11 +146,8 @@ ip_ipsec_fwd(struct mbuf *m) * 1 = drop packet, 0 = continue processing packet. */ int -ip_ipsec_input(struct mbuf *m) +ip_ipsec_input(struct mbuf *m, int nxt) { - struct ip *ip = mtod(m, struct ip *); - struct m_tag *mtag; - struct tdb_ident *tdbi; struct secpolicy *sp; int error; /* @@ -158,21 +155,9 @@ ip_ipsec_input(struct mbuf *m) * note that we do not visit this with protocols with pcb layer * code - like udp/tcp/raw ip. */ - if ((inetsw[ip_protox[ip->ip_p]].pr_flags & PR_LASTHDR) != 0) { - /* - * Check if the packet has already had IPsec processing - * done. If so, then just pass it along. This tag gets - * set during AH, ESP, etc. input handling, before the - * packet is returned to the ip input queue for delivery. - */ - mtag = m_tag_find(m, PACKET_TAG_IPSEC_IN_DONE, NULL); - if (mtag != NULL) { - tdbi = (struct tdb_ident *)(mtag + 1); - sp = ipsec_getpolicy(tdbi, IPSEC_DIR_INBOUND); - } else { - sp = ipsec_getpolicybyaddr(m, IPSEC_DIR_INBOUND, - IP_FORWARDING, &error); - } + if ((inetsw[ip_protox[nxt]].pr_flags & PR_LASTHDR) != 0) { + sp = ipsec_getpolicybyaddr(m, IPSEC_DIR_INBOUND, + IP_FORWARDING, &error); if (sp != NULL) { /* * Check security policy against packet attributes. @@ -183,12 +168,11 @@ ip_ipsec_input(struct mbuf *m) /* XXX error stat??? */ error = EINVAL; DPRINTF(("ip_input: no SP, packet discarded\n"));/*XXX*/ - return 1; } - if (error) - return 1; + if (error != 0) + return (1); } - return 0; + return (0); } /* -- cgit v1.1 From 4b1c09e9090e2f67bf353f6450ba6a3c26bf23c0 Mon Sep 17 00:00:00 2001 From: ae Date: Thu, 11 Dec 2014 16:53:29 +0000 Subject: Move ip_ipsec_fwd() from ip_input() into ip_forward(). Remove check for presence PACKET_TAG_IPSEC_IN_DONE mbuf tag from ip_ipsec_fwd(). PACKET_TAG_IPSEC_IN_DONE tag means that packet is already handled by IPSEC code. This means that before IPSEC processing it was destined to our address and security policy was checked in the ip_ipsec_input(). After IPSEC processing packet has new IP addresses and destination address isn't our own. So, anyway we can't check security policy from the mbuf tag, because it corresponds to different addresses. We should check security policy that corresponds to packet attributes in both cases - when it has a mbuf tag and when it has not. Obtained from: Yandex LLC Sponsored by: Yandex LLC --- sys/netinet/ip_ipsec.c | 38 ++++++++++++-------------------------- 1 file changed, 12 insertions(+), 26 deletions(-) (limited to 'sys/netinet/ip_ipsec.c') diff --git a/sys/netinet/ip_ipsec.c b/sys/netinet/ip_ipsec.c index ad6dec2..22bbba3 100644 --- a/sys/netinet/ip_ipsec.c +++ b/sys/netinet/ip_ipsec.c @@ -101,41 +101,27 @@ ip_ipsec_filtertunnel(struct mbuf *m) /* * Check if this packet has an active SA and needs to be dropped instead * of forwarded. - * Called from ip_input(). + * Called from ip_forward(). * 1 = drop packet, 0 = forward packet. */ int ip_ipsec_fwd(struct mbuf *m) { - struct m_tag *mtag; - struct tdb_ident *tdbi; struct secpolicy *sp; int error; - mtag = m_tag_find(m, PACKET_TAG_IPSEC_IN_DONE, NULL); - if (mtag != NULL) { - tdbi = (struct tdb_ident *)(mtag + 1); - sp = ipsec_getpolicy(tdbi, IPSEC_DIR_INBOUND); - } else { - sp = ipsec_getpolicybyaddr(m, IPSEC_DIR_INBOUND, - IP_FORWARDING, &error); - } - if (sp == NULL) { /* NB: can happen if error */ - /*XXX error stat???*/ - DPRINTF(("ip_input: no SP for forwarding\n")); /*XXX*/ - return 1; - } - - /* - * Check security policy against packet attributes. - */ - error = ipsec_in_reject(sp, m); - KEY_FREESP(&sp); - if (error) { - IPSTAT_INC(ips_cantforward); - return 1; + sp = ipsec_getpolicybyaddr(m, IPSEC_DIR_INBOUND, + IP_FORWARDING, &error); + if (sp != NULL) { + /* + * Check security policy against packet attributes. + */ + error = ipsec_in_reject(sp, m); + KEY_FREESP(&sp); } - return 0; + if (error != 0) + return (1); + return (0); } /* -- cgit v1.1 From 8eff9f6e5d0a5dea65eadef6d0b12629560eeb30 Mon Sep 17 00:00:00 2001 From: ae Date: Thu, 11 Dec 2014 17:34:49 +0000 Subject: Remove flags and tunalready arguments from ipsec4_process_packet() and make its prototype similar to ipsec6_process_packet. The flags argument isn't used here, tunalready is always zero. Obtained from: Yandex LLC Sponsored by: Yandex LLC --- sys/netinet/ip_ipsec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'sys/netinet/ip_ipsec.c') diff --git a/sys/netinet/ip_ipsec.c b/sys/netinet/ip_ipsec.c index 22bbba3..2452ec3 100644 --- a/sys/netinet/ip_ipsec.c +++ b/sys/netinet/ip_ipsec.c @@ -228,7 +228,7 @@ ip_ipsec_output(struct mbuf **m, struct inpcb *inp, int *flags, int *error) #endif /* NB: callee frees mbuf */ - *error = ipsec4_process_packet(*m, sp->req, *flags, 0); + *error = ipsec4_process_packet(*m, sp->req); if (*error == EJUSTRETURN) { /* * We had a SP with a level of 'use' and no SA. We -- cgit v1.1 From 3665df88dc5fa7d75826afe5bafe27b796b1425b Mon Sep 17 00:00:00 2001 From: ae Date: Thu, 11 Dec 2014 18:35:34 +0000 Subject: Remove flag/flags argument from the following functions: ipsec_getpolicybyaddr() ipsec4_checkpolicy() ip_ipsec_output() ip6_ipsec_output() The only flag used here was IP_FORWARDING. Obtained from: Yandex LLC Sponsored by: Yandex LLC --- sys/netinet/ip_ipsec.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) (limited to 'sys/netinet/ip_ipsec.c') diff --git a/sys/netinet/ip_ipsec.c b/sys/netinet/ip_ipsec.c index 2452ec3..f086f34 100644 --- a/sys/netinet/ip_ipsec.c +++ b/sys/netinet/ip_ipsec.c @@ -110,8 +110,7 @@ ip_ipsec_fwd(struct mbuf *m) struct secpolicy *sp; int error; - sp = ipsec_getpolicybyaddr(m, IPSEC_DIR_INBOUND, - IP_FORWARDING, &error); + sp = ipsec_getpolicybyaddr(m, IPSEC_DIR_INBOUND, &error); if (sp != NULL) { /* * Check security policy against packet attributes. @@ -142,8 +141,7 @@ ip_ipsec_input(struct mbuf *m, int nxt) * code - like udp/tcp/raw ip. */ if ((inetsw[ip_protox[nxt]].pr_flags & PR_LASTHDR) != 0) { - sp = ipsec_getpolicybyaddr(m, IPSEC_DIR_INBOUND, - IP_FORWARDING, &error); + sp = ipsec_getpolicybyaddr(m, IPSEC_DIR_INBOUND, &error); if (sp != NULL) { /* * Check security policy against packet attributes. @@ -185,7 +183,7 @@ ip_ipsec_mtu(struct mbuf *m, int mtu) * -1 = packet was reinjected and stop processing packet */ int -ip_ipsec_output(struct mbuf **m, struct inpcb *inp, int *flags, int *error) +ip_ipsec_output(struct mbuf **m, struct inpcb *inp, int *error) { struct secpolicy *sp; /* @@ -201,7 +199,7 @@ ip_ipsec_output(struct mbuf **m, struct inpcb *inp, int *flags, int *error) *error = 0; return (0); } - sp = ipsec4_checkpolicy(*m, IPSEC_DIR_OUTBOUND, *flags, error, inp); + sp = ipsec4_checkpolicy(*m, IPSEC_DIR_OUTBOUND, error, inp); /* * There are four return cases: * sp != NULL apply IPsec policy -- cgit v1.1 From 763607f988f01b1466a279f442857914e00b15b9 Mon Sep 17 00:00:00 2001 From: ae Date: Thu, 11 Dec 2014 18:55:54 +0000 Subject: Use ipsec4_in_reject() to simplify ip_ipsec_fwd() and ip_ipsec_input(). ipsec4_in_reject() does the same things, also it counts policy violation errors. Obtained from: Yandex LLC Sponsored by: Yandex LLC --- sys/netinet/ip_ipsec.c | 34 +++------------------------------- 1 file changed, 3 insertions(+), 31 deletions(-) (limited to 'sys/netinet/ip_ipsec.c') diff --git a/sys/netinet/ip_ipsec.c b/sys/netinet/ip_ipsec.c index f086f34..1a643fb 100644 --- a/sys/netinet/ip_ipsec.c +++ b/sys/netinet/ip_ipsec.c @@ -107,20 +107,8 @@ ip_ipsec_filtertunnel(struct mbuf *m) int ip_ipsec_fwd(struct mbuf *m) { - struct secpolicy *sp; - int error; - sp = ipsec_getpolicybyaddr(m, IPSEC_DIR_INBOUND, &error); - if (sp != NULL) { - /* - * Check security policy against packet attributes. - */ - error = ipsec_in_reject(sp, m); - KEY_FREESP(&sp); - } - if (error != 0) - return (1); - return (0); + return (ipsec4_in_reject(m, NULL)); } /* @@ -133,29 +121,13 @@ ip_ipsec_fwd(struct mbuf *m) int ip_ipsec_input(struct mbuf *m, int nxt) { - struct secpolicy *sp; - int error; /* * enforce IPsec policy checking if we are seeing last header. * note that we do not visit this with protocols with pcb layer * code - like udp/tcp/raw ip. */ - if ((inetsw[ip_protox[nxt]].pr_flags & PR_LASTHDR) != 0) { - sp = ipsec_getpolicybyaddr(m, IPSEC_DIR_INBOUND, &error); - if (sp != NULL) { - /* - * Check security policy against packet attributes. - */ - error = ipsec_in_reject(sp, m); - KEY_FREESP(&sp); - } else { - /* XXX error stat??? */ - error = EINVAL; - DPRINTF(("ip_input: no SP, packet discarded\n"));/*XXX*/ - } - if (error != 0) - return (1); - } + if ((inetsw[ip_protox[nxt]].pr_flags & PR_LASTHDR) != 0) + return (ipsec4_in_reject(m, NULL)); return (0); } -- cgit v1.1