From owner-freebsd-net@FreeBSD.ORG Sun Mar 15 06:36:57 2015 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 2A65B58D; Sun, 15 Mar 2015 06:36:57 +0000 (UTC) Received: from venus.codepro.be (venus.codepro.be [IPv6:2a01:4f8:162:1127::2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "*.codepro.be", Issuer "Gandi Standard SSL CA" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id AB2C33E2; Sun, 15 Mar 2015 06:36:56 +0000 (UTC) Received: from vega.codepro.be (unknown [172.16.1.3]) by venus.codepro.be (Postfix) with ESMTP id 63FDD100E2; Sun, 15 Mar 2015 07:36:52 +0100 (CET) Received: by vega.codepro.be (Postfix, from userid 1001) id 2196D780E0; Sun, 15 Mar 2015 07:36:51 +0100 (CET) Date: Sun, 15 Mar 2015 07:36:51 +0100 From: Kristof Provost To: freebsd-net@freebsd.org Subject: Padded packets in ip6_input() Message-ID: <20150315063651.GA2036@vega.codepro.be> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline X-PGP-Fingerprint: E114 D9EA 909E D469 8F57 17A5 7D15 91C6 9EFA F286 X-Checked-By-NSA: Probably User-Agent: Mutt/1.5.23 (2014-03-12) Cc: Philip Paeps X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 15 Mar 2015 06:36:57 -0000 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 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; + } + if (m->m_pkthdr.len > sizeof(struct ip6_hdr) + plen) { + if (m->m_len == m->m_pkthdr.len) { + m->m_len = sizeof(struct ip6_hdr) + plen; + m->m_pkthdr.len = sizeof(struct ip6_hdr) + plen; + } else + m_adj(m, sizeof(struct ip6_hdr) + plen - m->m_pkthdr.len); + } #if 0 /* * Reject packets with IPv4 compatible addresses (auto tunnel). @@ -702,25 +722,6 @@ passin: nxt = ip6->ip6_nxt; /* - * 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. - */ - 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; - } - if (m->m_pkthdr.len > sizeof(struct ip6_hdr) + plen) { - if (m->m_len == m->m_pkthdr.len) { - m->m_len = sizeof(struct ip6_hdr) + plen; - m->m_pkthdr.len = sizeof(struct ip6_hdr) + plen; - } else - m_adj(m, sizeof(struct ip6_hdr) + plen - m->m_pkthdr.len); - } - - /* * Forward if desirable. */ if (V_ip6_mrouter &&