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 <kris...@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
signature.asc
Description: OpenPGP digital signature
