* [PATCH net] ipv6/netfilter: Discard first fragment not including all headers
@ 2020-11-04 13:01 Georg Kohmann
2020-11-04 13:41 ` Pablo Neira Ayuso
0 siblings, 1 reply; 3+ messages in thread
From: Georg Kohmann @ 2020-11-04 13:01 UTC (permalink / raw)
To: netdev
Cc: pablo, kadlec, fw, davem, kuznet, yoshfuji, kuba, netfilter-devel,
coreteam, Georg Kohmann
Packets are processed even though the first fragment don't include all
headers through the upper layer header. This breaks TAHI IPv6 Core
Conformance Test v6LC.1.3.6.
Referring to RFC8200 SECTION 4.5: "If the first fragment does not include
all headers through an Upper-Layer header, then that fragment should be
discarded and an ICMP Parameter Problem, Code 3, message should be sent to
the source of the fragment, with the Pointer field set to zero."
Utilize the fragment offset returned by find_prev_fhdr() to let
ipv6_skip_exthdr() start it's traverse from the fragment header.
Apply the same logic for checking that all headers are included as used
in commit 2efdaaaf883a ("IPv6: reply ICMP error if the first fragment don't
include all headers"). Check that TCP, UDP and ICMP headers are completely
included in the fragment and all other headers are included with at least
one byte.
Return 0 to drop the fragment. This is the same behaviour as used on other
protocol errors in this function, e.g. when nf_ct_frag6_queue() returns
-EPROTO. The Fragment will later be picked up by ipv6_frag_rcv() in
reassembly.c. ipv6_frag_rcv() will then send an appropriate ICMP Parameter
Problem message back to the source.
References commit 2efdaaaf883a ("IPv6: reply ICMP error if the first
fragment don't include all headers")
Signed-off-by: Georg Kohmann <geokohma@cisco.com>
---
net/ipv6/netfilter/nf_conntrack_reasm.c | 28 +++++++++++++++++++++++++++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
index 054d287..dffa3a8 100644
--- a/net/ipv6/netfilter/nf_conntrack_reasm.c
+++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
@@ -440,11 +440,13 @@ find_prev_fhdr(struct sk_buff *skb, u8 *prevhdrp, int *prevhoff, int *fhoff)
int nf_ct_frag6_gather(struct net *net, struct sk_buff *skb, u32 user)
{
u16 savethdr = skb->transport_header;
- int fhoff, nhoff, ret;
+ int fhoff, nhoff, ret, offset;
struct frag_hdr *fhdr;
struct frag_queue *fq;
struct ipv6hdr *hdr;
u8 prevhdr;
+ u8 nexthdr = NEXTHDR_FRAGMENT;
+ __be16 frag_off;
/* Jumbo payload inhibits frag. header */
if (ipv6_hdr(skb)->payload_len == 0) {
@@ -455,6 +457,30 @@ int nf_ct_frag6_gather(struct net *net, struct sk_buff *skb, u32 user)
if (find_prev_fhdr(skb, &prevhdr, &nhoff, &fhoff) < 0)
return 0;
+ /* Discard the first fragment if it does not include all headers
+ * RFC 8200, Section 4.5
+ */
+ offset = ipv6_skip_exthdr(skb, fhoff, &nexthdr, &frag_off);
+ if (offset >= 0 && !(frag_off & htons(IP6_OFFSET))) {
+ switch (nexthdr) {
+ case NEXTHDR_TCP:
+ offset += sizeof(struct tcphdr);
+ break;
+ case NEXTHDR_UDP:
+ offset += sizeof(struct udphdr);
+ break;
+ case NEXTHDR_ICMP:
+ offset += sizeof(struct icmp6hdr);
+ break;
+ default:
+ offset += 1;
+ }
+ if (offset > skb->len) {
+ pr_debug("Drop incomplete fragment\n");
+ return 0;
+ }
+ }
+
if (!pskb_may_pull(skb, fhoff + sizeof(*fhdr)))
return -ENOMEM;
--
2.10.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net] ipv6/netfilter: Discard first fragment not including all headers
2020-11-04 13:01 [PATCH net] ipv6/netfilter: Discard first fragment not including all headers Georg Kohmann
@ 2020-11-04 13:41 ` Pablo Neira Ayuso
2020-11-04 14:17 ` Georg Kohmann (geokohma)
0 siblings, 1 reply; 3+ messages in thread
From: Pablo Neira Ayuso @ 2020-11-04 13:41 UTC (permalink / raw)
To: Georg Kohmann
Cc: netdev, kadlec, fw, davem, kuznet, yoshfuji, kuba,
netfilter-devel, coreteam
Hi,
On Wed, Nov 04, 2020 at 02:01:28PM +0100, Georg Kohmann wrote:
> Packets are processed even though the first fragment don't include all
> headers through the upper layer header. This breaks TAHI IPv6 Core
> Conformance Test v6LC.1.3.6.
>
> Referring to RFC8200 SECTION 4.5: "If the first fragment does not include
> all headers through an Upper-Layer header, then that fragment should be
> discarded and an ICMP Parameter Problem, Code 3, message should be sent to
> the source of the fragment, with the Pointer field set to zero."
>
> Utilize the fragment offset returned by find_prev_fhdr() to let
> ipv6_skip_exthdr() start it's traverse from the fragment header.
> Apply the same logic for checking that all headers are included as used
> in commit 2efdaaaf883a ("IPv6: reply ICMP error if the first fragment don't
> include all headers"). Check that TCP, UDP and ICMP headers are completely
> included in the fragment and all other headers are included with at least
> one byte.
>
> Return 0 to drop the fragment. This is the same behaviour as used on other
> protocol errors in this function, e.g. when nf_ct_frag6_queue() returns
> -EPROTO. The Fragment will later be picked up by ipv6_frag_rcv() in
> reassembly.c. ipv6_frag_rcv() will then send an appropriate ICMP Parameter
> Problem message back to the source.
>
> References commit 2efdaaaf883a ("IPv6: reply ICMP error if the first
> fragment don't include all headers")
> Signed-off-by: Georg Kohmann <geokohma@cisco.com>
> ---
> net/ipv6/netfilter/nf_conntrack_reasm.c | 28 +++++++++++++++++++++++++++-
> 1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
> index 054d287..dffa3a8 100644
> --- a/net/ipv6/netfilter/nf_conntrack_reasm.c
> +++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
> @@ -440,11 +440,13 @@ find_prev_fhdr(struct sk_buff *skb, u8 *prevhdrp, int *prevhoff, int *fhoff)
> int nf_ct_frag6_gather(struct net *net, struct sk_buff *skb, u32 user)
> {
> u16 savethdr = skb->transport_header;
> - int fhoff, nhoff, ret;
> + int fhoff, nhoff, ret, offset;
> struct frag_hdr *fhdr;
> struct frag_queue *fq;
> struct ipv6hdr *hdr;
> u8 prevhdr;
> + u8 nexthdr = NEXTHDR_FRAGMENT;
> + __be16 frag_off;
>
> /* Jumbo payload inhibits frag. header */
> if (ipv6_hdr(skb)->payload_len == 0) {
> @@ -455,6 +457,30 @@ int nf_ct_frag6_gather(struct net *net, struct sk_buff *skb, u32 user)
> if (find_prev_fhdr(skb, &prevhdr, &nhoff, &fhoff) < 0)
> return 0;
>
> + /* Discard the first fragment if it does not include all headers
> + * RFC 8200, Section 4.5
> + */
> + offset = ipv6_skip_exthdr(skb, fhoff, &nexthdr, &frag_off);
> + if (offset >= 0 && !(frag_off & htons(IP6_OFFSET))) {
> + switch (nexthdr) {
> + case NEXTHDR_TCP:
> + offset += sizeof(struct tcphdr);
> + break;
> + case NEXTHDR_UDP:
> + offset += sizeof(struct udphdr);
> + break;
> + case NEXTHDR_ICMP:
> + offset += sizeof(struct icmp6hdr);
> + break;
> + default:
> + offset += 1;
> + }
> + if (offset > skb->len) {
> + pr_debug("Drop incomplete fragment\n");
> + return 0;
> + }
> + }
This code looks very similar to 2efdaaaf883a.
Would you wrap it in a function as call it use to reuse it? Something
like this sketch?
static bool ipv6_frag_validate(struct sk_buff *skb, ...)
{
...
offset = ipv6_skip_exthdr(skb, fhoff, &nexthdr, &frag_off);
if (offset >= 0 && !(frag_off & htons(IP6_OFFSET))) {
switch (nexthdr) {
case NEXTHDR_TCP:
offset += sizeof(struct tcphdr);
break;
case NEXTHDR_UDP:
offset += sizeof(struct udphdr);
break;
case NEXTHDR_ICMP:
offset += sizeof(struct icmp6hdr);
break;
default:
offset += 1;
}
if (offset > skb->len)
return false;
}
return true;
}
then, from ipv6:
if (!ipv6_frag_validate(skb, ...)) {
__IP6_INC_STATS(net, __in6_dev_get_safely(skb->dev),
IPSTATS_MIB_INHDRERRORS);
icmpv6_param_prob(skb, ICMPV6_HDR_INCOMP, 0);
reurn -1;
}
and from netfilter:
if (!ipv6_frag_validate(skb, ...))
return -1;
Thanks.
> +
> if (!pskb_may_pull(skb, fhoff + sizeof(*fhdr)))
> return -ENOMEM;
>
> --
> 2.10.2
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net] ipv6/netfilter: Discard first fragment not including all headers
2020-11-04 13:41 ` Pablo Neira Ayuso
@ 2020-11-04 14:17 ` Georg Kohmann (geokohma)
0 siblings, 0 replies; 3+ messages in thread
From: Georg Kohmann (geokohma) @ 2020-11-04 14:17 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: netdev@vger.kernel.org, kadlec@netfilter.org, fw@strlen.de,
davem@davemloft.net, kuznet@ms2.inr.ac.ru,
yoshfuji@linux-ipv6.org, kuba@kernel.org,
netfilter-devel@vger.kernel.org, coreteam@netfilter.org
On 04.11.2020 14:41, Pablo Neira Ayuso wrote:
> Hi,
>
> On Wed, Nov 04, 2020 at 02:01:28PM +0100, Georg Kohmann wrote:
>> Packets are processed even though the first fragment don't include all
>> headers through the upper layer header. This breaks TAHI IPv6 Core
>> Conformance Test v6LC.1.3.6.
>>
>> Referring to RFC8200 SECTION 4.5: "If the first fragment does not include
>> all headers through an Upper-Layer header, then that fragment should be
>> discarded and an ICMP Parameter Problem, Code 3, message should be sent to
>> the source of the fragment, with the Pointer field set to zero."
>>
>> Utilize the fragment offset returned by find_prev_fhdr() to let
>> ipv6_skip_exthdr() start it's traverse from the fragment header.
>> Apply the same logic for checking that all headers are included as used
>> in commit 2efdaaaf883a ("IPv6: reply ICMP error if the first fragment don't
>> include all headers"). Check that TCP, UDP and ICMP headers are completely
>> included in the fragment and all other headers are included with at least
>> one byte.
>>
>> Return 0 to drop the fragment. This is the same behaviour as used on other
>> protocol errors in this function, e.g. when nf_ct_frag6_queue() returns
>> -EPROTO. The Fragment will later be picked up by ipv6_frag_rcv() in
>> reassembly.c. ipv6_frag_rcv() will then send an appropriate ICMP Parameter
>> Problem message back to the source.
>>
>> References commit 2efdaaaf883a ("IPv6: reply ICMP error if the first
>> fragment don't include all headers")
>> Signed-off-by: Georg Kohmann <geokohma@cisco.com>
>> ---
>> net/ipv6/netfilter/nf_conntrack_reasm.c | 28 +++++++++++++++++++++++++++-
>> 1 file changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
>> index 054d287..dffa3a8 100644
>> --- a/net/ipv6/netfilter/nf_conntrack_reasm.c
>> +++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
>> @@ -440,11 +440,13 @@ find_prev_fhdr(struct sk_buff *skb, u8 *prevhdrp, int *prevhoff, int *fhoff)
>> int nf_ct_frag6_gather(struct net *net, struct sk_buff *skb, u32 user)
>> {
>> u16 savethdr = skb->transport_header;
>> - int fhoff, nhoff, ret;
>> + int fhoff, nhoff, ret, offset;
>> struct frag_hdr *fhdr;
>> struct frag_queue *fq;
>> struct ipv6hdr *hdr;
>> u8 prevhdr;
>> + u8 nexthdr = NEXTHDR_FRAGMENT;
>> + __be16 frag_off;
>>
>> /* Jumbo payload inhibits frag. header */
>> if (ipv6_hdr(skb)->payload_len == 0) {
>> @@ -455,6 +457,30 @@ int nf_ct_frag6_gather(struct net *net, struct sk_buff *skb, u32 user)
>> if (find_prev_fhdr(skb, &prevhdr, &nhoff, &fhoff) < 0)
>> return 0;
>>
>> + /* Discard the first fragment if it does not include all headers
>> + * RFC 8200, Section 4.5
>> + */
>> + offset = ipv6_skip_exthdr(skb, fhoff, &nexthdr, &frag_off);
>> + if (offset >= 0 && !(frag_off & htons(IP6_OFFSET))) {
>> + switch (nexthdr) {
>> + case NEXTHDR_TCP:
>> + offset += sizeof(struct tcphdr);
>> + break;
>> + case NEXTHDR_UDP:
>> + offset += sizeof(struct udphdr);
>> + break;
>> + case NEXTHDR_ICMP:
>> + offset += sizeof(struct icmp6hdr);
>> + break;
>> + default:
>> + offset += 1;
>> + }
>> + if (offset > skb->len) {
>> + pr_debug("Drop incomplete fragment\n");
>> + return 0;
>> + }
>> + }
> This code looks very similar to 2efdaaaf883a.
>
> Would you wrap it in a function as call it use to reuse it? Something
> like this sketch?
>
> static bool ipv6_frag_validate(struct sk_buff *skb, ...)
> {
> ...
>
> offset = ipv6_skip_exthdr(skb, fhoff, &nexthdr, &frag_off);
> if (offset >= 0 && !(frag_off & htons(IP6_OFFSET))) {
> switch (nexthdr) {
> case NEXTHDR_TCP:
> offset += sizeof(struct tcphdr);
> break;
> case NEXTHDR_UDP:
> offset += sizeof(struct udphdr);
> break;
> case NEXTHDR_ICMP:
> offset += sizeof(struct icmp6hdr);
> break;
> default:
> offset += 1;
> }
> if (offset > skb->len)
> return false;
> }
>
> return true;
> }
>
> then, from ipv6:
>
> if (!ipv6_frag_validate(skb, ...)) {
> __IP6_INC_STATS(net, __in6_dev_get_safely(skb->dev),
> IPSTATS_MIB_INHDRERRORS);
> icmpv6_param_prob(skb, ICMPV6_HDR_INCOMP, 0);
> reurn -1;
> }
>
> and from netfilter:
>
> if (!ipv6_frag_validate(skb, ...))
> return -1;
>
> Thanks.
>
Thanks for feedback, I will do as suggested.
>> if (!pskb_may_pull(skb, fhoff + sizeof(*fhdr)))
>> return -ENOMEM;
>>
>> --
>> 2.10.2
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-11-04 14:17 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-04 13:01 [PATCH net] ipv6/netfilter: Discard first fragment not including all headers Georg Kohmann
2020-11-04 13:41 ` Pablo Neira Ayuso
2020-11-04 14:17 ` Georg Kohmann (geokohma)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).