Date: Sun, 15 Mar 2015 16:29:02 +0300 From: "Andrey V. Elsukov" <bu7cher@yandex.ru> To: Kristof Provost <kristof@sigsegv.be>, freebsd-net@freebsd.org Cc: Philip Paeps <philip@freebsd.org> Subject: Re: Padded packets in ip6_input() Message-ID: <5505891E.4060109@yandex.ru> In-Reply-To: <20150315063651.GA2036@vega.codepro.be> References: <20150315063651.GA2036@vega.codepro.be>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
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@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
[-- Attachment #2 --]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQEcBAEBCAAGBQJVBYkeAAoJEAHF6gQQyKF6dTQIAITQOJ19+d7BElpTRb/klmCf
Dqic08AxBckG72MBU2WuzYo7YPgJRQinAHMJG6IJuTmf6r1ngIOQ0F73pVnpW8fp
zW9zN5CcedPkqRZZ+Z0bxiudU3hhfD3YlVfowutRFxV+GY72Z1PIQLU5OtYqKTD6
kQfJo/HaaeGNcMUXitWsfrm2Dk+4dgaCvQNSlcmkzNDDIZwMMY77qMnlJ9nqtnQ1
PWZm10diCfRw+TlfpwFG0LvH1tJ0Rm8YXqBPqH3RaoIEQWwtqF8Fxodg+ARBZd7N
qSP1h9oDeDj+jNPb3g0/GiRgDrhN5uKOKXlG7DCJWCNYWTygGSjoRrQVzv/Owns=
=9F/B
-----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5505891E.4060109>