| Message ID | 18a669995a73fefd70e179e6bc11b74e397e56ad.1595594449.git.sd@queasysnail.net |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | [RFC,ipsec] xfrm: don't pass too short packets to userspace with ESPINUDP encap | expand |
On Fri, Jul 24, 2020 at 04:46:07PM +0200, Sabrina Dubroca wrote: > Currently, any UDP-encapsulated packet of 8 bytes or less will be > passed to userspace, whether it starts with the non-ESP prefix or > not (except keepalives). This includes: > - messages of 1, 2, 3 bytes > - messages of 4 to 8 bytes not starting with 00 00 00 00 > > This patch changes that behavior, so that only properly-formed non-ESP > messages are passed to userspace. Messages of 8 bytes or less that > don't contain a full non-ESP prefix followed by some data (at least > one byte) will be dropped and counted as XfrmInHdrError. I'm ok with that change. But it affects userspace, so the *swan people have to tell if that's ok for them.
>> Currently, any UDP-encapsulated packet of 8 bytes or less will be >> passed to userspace, whether it starts with the non-ESP prefix or >> not (except keepalives). This includes: >> - messages of 1, 2, 3 bytes >> - messages of 4 to 8 bytes not starting with 00 00 00 00 >> >> This patch changes that behavior, so that only properly-formed non-ESP >> messages are passed to userspace. Messages of 8 bytes or less that >> don't contain a full non-ESP prefix followed by some data (at least >> one byte) will be dropped and counted as XfrmInHdrError. > > I'm ok with that change. But it affects userspace, so the *swan > people have to tell if that's ok for them. Yes, no problem from strongSwan's side. Packets shorter than 4 bytes are immediately dropped anyway, the others when attempting to parse as IKE messages (already the initiator IKE SPI, with which they start, is 8 bytes long). Regards, Tobias
On Jul 27, 2020, at 05:28, Steffen Klassert <steffen.klassert@secunet.com> wrote: > > >> >> This patch changes that behavior, so that only properly-formed non-ESP >> messages are passed to userspace. Messages of 8 bytes or less that >> don't contain a full non-ESP prefix followed by some data (at least >> one byte) will be dropped and counted as XfrmInHdrError. > > I'm ok with that change. But it affects userspace, so the *swan > people have to tell if that's ok for them. Libreswan is okay with this, we actually discussed this with Sabrina as a result of the TCP work where she noticed the difference. Paul
diff --git a/net/ipv4/xfrm4_input.c b/net/ipv4/xfrm4_input.c index ad2afeef4f10..2a2bb38ac798 100644 --- a/net/ipv4/xfrm4_input.c +++ b/net/ipv4/xfrm4_input.c @@ -114,9 +114,14 @@ int xfrm4_udp_encap_rcv(struct sock *sk, struct sk_buff *skb) } else if (len > sizeof(struct ip_esp_hdr) && udpdata32[0] != 0) { /* ESP Packet without Non-ESP header */ len = sizeof(struct udphdr); - } else - /* Must be an IKE packet.. pass it through */ + } else if (len > 4 && udpdata32[0] == 0) { + /* IKE packet: pass it through */ return 1; + } else { + /* incomplete packet, drop */ + XFRM_INC_STATS(dev_net(skb->dev), LINUX_MIB_XFRMINHDRERROR); + goto drop; + } break; case UDP_ENCAP_ESPINUDP_NON_IKE: /* Check if this is a keepalive packet. If so, eat it. */ diff --git a/net/ipv6/xfrm6_input.c b/net/ipv6/xfrm6_input.c index 04cbeefd8982..7e14d59d55cb 100644 --- a/net/ipv6/xfrm6_input.c +++ b/net/ipv6/xfrm6_input.c @@ -110,9 +110,14 @@ int xfrm6_udp_encap_rcv(struct sock *sk, struct sk_buff *skb) } else if (len > sizeof(struct ip_esp_hdr) && udpdata32[0] != 0) { /* ESP Packet without Non-ESP header */ len = sizeof(struct udphdr); - } else - /* Must be an IKE packet.. pass it through */ + } else if (len > 4 && udpdata32[0] == 0) { + /* IKE packet: pass it through */ return 1; + } else { + /* incomplete packet, drop */ + XFRM_INC_STATS(dev_net(skb->dev), LINUX_MIB_XFRMINHDRERROR); + goto drop; + } break; case UDP_ENCAP_ESPINUDP_NON_IKE: /* Check if this is a keepalive packet. If so, eat it. */
Currently, any UDP-encapsulated packet of 8 bytes or less will be passed to userspace, whether it starts with the non-ESP prefix or not (except keepalives). This includes: - messages of 1, 2, 3 bytes - messages of 4 to 8 bytes not starting with 00 00 00 00 This patch changes that behavior, so that only properly-formed non-ESP messages are passed to userspace. Messages of 8 bytes or less that don't contain a full non-ESP prefix followed by some data (at least one byte) will be dropped and counted as XfrmInHdrError. Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> --- net/ipv4/xfrm4_input.c | 9 +++++++-- net/ipv6/xfrm6_input.c | 9 +++++++-- 2 files changed, 14 insertions(+), 4 deletions(-)