Padded packets in ip6_input()

[Andrey V. Elsukov]

On 15.03.2015 09:36, Kristof Provost wrote:
> Hi,
> 
> While having a quick look at PR 169630 I wound up looking at what
> happens with short IP and IPv6 packets in their input paths.
> 
> On Ethernet frames have to have a minimum size and for both legacy and
> modern IP it's possible to have shorter packets. In that case the sender
> just adds some padding after the packet so it can be transported over
> Ethernet.
> 
> The way we deal with that is different for IPv4 and IPv6. In v4 we check
> packet size (is the packet big enough for what IP claims the size is)
> and remove the trailing bytes very early in the processing. In
> ip6_input() that isn't done until after the pfil() hook and nexthop
> processing.
> 
> I think it's risky to wait with the check and trim (as in PR 169360:
> it's possible firewalls would reassemble and include the padding. That'd
> be a bug in the firewall of course, but this would ensure it couldn't
> happen at all).
> On the flip size, I see no downside of doing the size check earlier. We
> have to check the packet size anyway.
> 
> Below is a patch which does just that.
> 
> commit 04efb7e62ab6ae2d3bdac362b1da8a1de9f0a531
> Author: Kristof Provost <kristof at sigsegv.be>
> Date:   Sun Mar 15 05:25:00 2015 +0100
> 
>     Check ip6 packet size and trim before the firewall
>     
>     In the ip6 input path we don't check the packet size ("Is the entire IP
>     frame there?") or trim it down (e.g. on Ethernet where short packets get
>     padding at the tail) until after the pfil() hook.
>     That means that the firewall can get packets with unwanted trailing
>     bytes. This could cause issues with careless reassembly code.
>     There's no reason to wait with this check so align with the ip4 input
>     path and do the check before the pfil() hook.
>     
>     Note that we re-read the plen after the pfil() hook, just in case the
>     firewall code does something to the packet length. This may or may not
>     be required.
> 
> diff --git a/sys/netinet6/ip6_input.c b/sys/netinet6/ip6_input.c
> index 78e8ef3..d7b20fa 100644
> --- a/sys/netinet6/ip6_input.c
> +++ b/sys/netinet6/ip6_input.c
> @@ -563,6 +563,26 @@ ip6_input(struct mbuf *m)
>  		in6_ifstat_inc(m->m_pkthdr.rcvif, ifs6_in_addrerr);
>  		goto bad;
>  	}
> +
> +	/*
> +	 * Check that the amount of data in the buffers
> +	 * is as at least much as the IPv6 header would have us expect.
> +	 * Trim mbufs if longer than we expect.
> +	 * Drop packet if shorter than we expect.
> +	 */
> +	plen = (u_int32_t)ntohs(ip6->ip6_plen);
> +	if (m->m_pkthdr.len - sizeof(struct ip6_hdr) < plen) {
> +		IP6STAT_INC(ip6s_tooshort);
> +		in6_ifstat_inc(m->m_pkthdr.rcvif, ifs6_in_truncated);
> +		goto bad;
> +	}

This is very rare case, I think, but plen can be zero in case, when
jumbo payload option is present. Probably this is the reason why this
check is done after hop-by-hop options parsing.


-- 
WBR, Andrey V. Elsukov

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 538 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freebsd.org/pipermail/freebsd-net/attachments/20150315/7d8f73a9/attachment.sig>