Re: [PATCH net] ipv6: Prevent ipv6_find_hdr() from returning ENOENT for valid non-first fragments

Re: [PATCH net] ipv6: Prevent ipv6_find_hdr() from returning ENOENT for valid non-first fragments

[Pablo Neira Ayuso]

On Wed, Jan 07, 2015 at 03:03:20AM +0530, Rahul Sharma wrote:
> ipv6_find_hdr() currently assumes that the next-header field in the
> fragment header of the non-first fragment is the "protocol number of
> the last header" (here last header excludes any extension header
> protocol numbers ) which is incorrect as per RFC2460. The next-header
> value is the first header of the fragmentable part of the original
> packet (which can be extension header as well).
> This can create reassembly problems. For example: Fragmented
> authenticated OSPFv3 packets (where AH header is inserted before the
> protocol header). For the second fragment, the next header value in
> the fragment header will be NEXTHDR_AUTH which is correct but
> ipv6_find_hdr will return ENOENT since AH is an extension header
> resulting in second fragment getting dropped. This check for the
> presence of non-extension header needs to be removed.
> 
> Signed-off-by: Rahul Sharma <rsharma@arista.com>
> ---
> --- linux-3.18.1/net/ipv6/exthdrs_core.c.orig   2015-01-06
> 10:25:36.411419863 -0800
> +++ linux-3.18.1/net/ipv6/exthdrs_core.c        2015-01-06
> 10:51:45.819364986 -0800
> @@ -171,10 +171,11 @@ EXPORT_SYMBOL_GPL(ipv6_find_tlv);
>   * If the first fragment doesn't contain the final protocol header or
>   * NEXTHDR_NONE it is considered invalid.
>   *
> - * Note that non-1st fragment is special case that "the protocol number
> - * of last header" is "next header" field in Fragment header. In this case,
> - * *offset is meaningless and fragment offset is stored in *fragoff if fragoff
> - * isn't NULL.
> + * Note that non-1st fragment is special case that "the protocol number of the
> + * first header of the fragmentable part of the original packet" is
> + * "next header" field in the Fragment header. In this case, *offset is
> + * meaningless and fragment offset is stored in *fragoff if fragoff isn't
> + * NULL.
>   *
>   * if flags is not NULL and it's a fragment, then the frag flag
>   * IP6_FH_F_FRAG will be set. If it's an AH header, the
> @@ -250,9 +251,7 @@ int ipv6_find_hdr(const struct sk_buff *
> 
>                         _frag_off = ntohs(*fp) & ~0x7;
>                         if (_frag_off) {
> -                               if (target < 0 &&
> -                                   ((!ipv6_ext_hdr(hp->nexthdr)) ||

This check assumes that the following headers cannot show up in the
fragmented part of the IPv6 packet:

 12 bool ipv6_ext_hdr(u8 nexthdr)
 13 {
 14         /*
 15          * find out if nexthdr is an extension header or a protocol
 16          */
 17         return   (nexthdr == NEXTHDR_HOP)       ||
 18                  (nexthdr == NEXTHDR_ROUTING)   ||
 19                  (nexthdr == NEXTHDR_FRAGMENT)  ||
 20                  (nexthdr == NEXTHDR_AUTH)      ||
 21                  (nexthdr == NEXTHDR_NONE)      ||
 22                  (nexthdr == NEXTHDR_DEST);

> -                                    hp->nexthdr == NEXTHDR_NONE)) {
> +                               if (target < 0) {
>                                         if (fragoff)
>                                                 *fragoff = _frag_off;
>                                         return hp->nexthdr;
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] ipv6: Prevent ipv6_find_hdr() from returning ENOENT for valid non-first fragments

[Rahul Sharma]

Hi Pablo,

On Wed, Jan 7, 2015 at 4:17 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Jan 07, 2015 at 03:03:20AM +0530, Rahul Sharma wrote:
>> ipv6_find_hdr() currently assumes that the next-header field in the
>> fragment header of the non-first fragment is the "protocol number of
>> the last header" (here last header excludes any extension header
>> protocol numbers ) which is incorrect as per RFC2460. The next-header
>> value is the first header of the fragmentable part of the original
>> packet (which can be extension header as well).
>> This can create reassembly problems. For example: Fragmented
>> authenticated OSPFv3 packets (where AH header is inserted before the
>> protocol header). For the second fragment, the next header value in
>> the fragment header will be NEXTHDR_AUTH which is correct but
>> ipv6_find_hdr will return ENOENT since AH is an extension header
>> resulting in second fragment getting dropped. This check for the
>> presence of non-extension header needs to be removed.
>>
>> Signed-off-by: Rahul Sharma <rsharma@arista.com>
>> ---
>> --- linux-3.18.1/net/ipv6/exthdrs_core.c.orig   2015-01-06
>> 10:25:36.411419863 -0800
>> +++ linux-3.18.1/net/ipv6/exthdrs_core.c        2015-01-06
>> 10:51:45.819364986 -0800
>> @@ -171,10 +171,11 @@ EXPORT_SYMBOL_GPL(ipv6_find_tlv);
>>   * If the first fragment doesn't contain the final protocol header or
>>   * NEXTHDR_NONE it is considered invalid.
>>   *
>> - * Note that non-1st fragment is special case that "the protocol number
>> - * of last header" is "next header" field in Fragment header. In this case,
>> - * *offset is meaningless and fragment offset is stored in *fragoff if fragoff
>> - * isn't NULL.
>> + * Note that non-1st fragment is special case that "the protocol number of the
>> + * first header of the fragmentable part of the original packet" is
>> + * "next header" field in the Fragment header. In this case, *offset is
>> + * meaningless and fragment offset is stored in *fragoff if fragoff isn't
>> + * NULL.
>>   *
>>   * if flags is not NULL and it's a fragment, then the frag flag
>>   * IP6_FH_F_FRAG will be set. If it's an AH header, the
>> @@ -250,9 +251,7 @@ int ipv6_find_hdr(const struct sk_buff *
>>
>>                         _frag_off = ntohs(*fp) & ~0x7;
>>                         if (_frag_off) {
>> -                               if (target < 0 &&
>> -                                   ((!ipv6_ext_hdr(hp->nexthdr)) ||
>
> This check assumes that the following headers cannot show up in the
> fragmented part of the IPv6 packet:
>
>  12 bool ipv6_ext_hdr(u8 nexthdr)
>  13 {
>  14         /*
>  15          * find out if nexthdr is an extension header or a protocol
>  16          */
>  17         return   (nexthdr == NEXTHDR_HOP)       ||
>  18                  (nexthdr == NEXTHDR_ROUTING)   ||
>  19                  (nexthdr == NEXTHDR_FRAGMENT)  ||
>  20                  (nexthdr == NEXTHDR_AUTH)      ||
>  21                  (nexthdr == NEXTHDR_NONE)      ||
>  22                  (nexthdr == NEXTHDR_DEST);
>
>> -                                    hp->nexthdr == NEXTHDR_NONE)) {
>> +                               if (target < 0) {
>>                                         if (fragoff)
>>                                                 *fragoff = _frag_off;
>>                                         return hp->nexthdr;
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

I think this is incorrect. Authentication header shows up in the
fragmentable part of the original IPv6 packet. So, for the non-first
fragments the next-header field value can be NEXTHDR_AUTH.

Thanks

Re: [PATCH net] ipv6: Prevent ipv6_find_hdr() from returning ENOENT for valid non-first fragments

[Hannes Frederic Sowa]

Hi,

On Mi, 2015-01-07 at 11:11 +0530, Rahul Sharma wrote:
> On Wed, Jan 7, 2015 at 4:17 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Wed, Jan 07, 2015 at 03:03:20AM +0530, Rahul Sharma wrote:
> >> ipv6_find_hdr() currently assumes that the next-header field in the
> >> fragment header of the non-first fragment is the "protocol number of
> >> the last header" (here last header excludes any extension header
> >> protocol numbers ) which is incorrect as per RFC2460. The next-header
> >> value is the first header of the fragmentable part of the original
> >> packet (which can be extension header as well).
> >> This can create reassembly problems. For example: Fragmented
> >> authenticated OSPFv3 packets (where AH header is inserted before the
> >> protocol header). For the second fragment, the next header value in
> >> the fragment header will be NEXTHDR_AUTH which is correct but
> >> ipv6_find_hdr will return ENOENT since AH is an extension header
> >> resulting in second fragment getting dropped. This check for the
> >> presence of non-extension header needs to be removed.
> >>
> >> Signed-off-by: Rahul Sharma <rsharma@arista.com>
> >> ---
> >> --- linux-3.18.1/net/ipv6/exthdrs_core.c.orig   2015-01-06
> >> 10:25:36.411419863 -0800
> >> +++ linux-3.18.1/net/ipv6/exthdrs_core.c        2015-01-06
> >> 10:51:45.819364986 -0800
> >> @@ -171,10 +171,11 @@ EXPORT_SYMBOL_GPL(ipv6_find_tlv);
> >>   * If the first fragment doesn't contain the final protocol header or
> >>   * NEXTHDR_NONE it is considered invalid.
> >>   *
> >> - * Note that non-1st fragment is special case that "the protocol number
> >> - * of last header" is "next header" field in Fragment header. In this case,
> >> - * *offset is meaningless and fragment offset is stored in *fragoff if fragoff
> >> - * isn't NULL.
> >> + * Note that non-1st fragment is special case that "the protocol number of the
> >> + * first header of the fragmentable part of the original packet" is
> >> + * "next header" field in the Fragment header. In this case, *offset is
> >> + * meaningless and fragment offset is stored in *fragoff if fragoff isn't
> >> + * NULL.
> >>   *
> >>   * if flags is not NULL and it's a fragment, then the frag flag
> >>   * IP6_FH_F_FRAG will be set. If it's an AH header, the
> >> @@ -250,9 +251,7 @@ int ipv6_find_hdr(const struct sk_buff *
> >>
> >>                         _frag_off = ntohs(*fp) & ~0x7;
> >>                         if (_frag_off) {
> >> -                               if (target < 0 &&
> >> -                                   ((!ipv6_ext_hdr(hp->nexthdr)) ||
> >
> > This check assumes that the following headers cannot show up in the
> > fragmented part of the IPv6 packet:
> >
> >  12 bool ipv6_ext_hdr(u8 nexthdr)
> >  13 {
> >  14         /*
> >  15          * find out if nexthdr is an extension header or a protocol
> >  16          */
> >  17         return   (nexthdr == NEXTHDR_HOP)       ||
> >  18                  (nexthdr == NEXTHDR_ROUTING)   ||
> >  19                  (nexthdr == NEXTHDR_FRAGMENT)  ||
> >  20                  (nexthdr == NEXTHDR_AUTH)      ||
> >  21                  (nexthdr == NEXTHDR_NONE)      ||
> >  22                  (nexthdr == NEXTHDR_DEST);
> >
> >> -                                    hp->nexthdr == NEXTHDR_NONE)) {
> >> +                               if (target < 0) {
> >>                                         if (fragoff)
> >>                                                 *fragoff = _frag_off;
> >>                                         return hp->nexthdr;
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe netdev" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> I think this is incorrect. Authentication header shows up in the
> fragmentable part of the original IPv6 packet. So, for the non-first
> fragments the next-header field value can be NEXTHDR_AUTH.

Pablo's mail got me thinking again.

In general, IPv6 extension headers can appear in any order and stacks
must be process them. Fragmentation adds a limitation, that some
extension headers do not make sense and don't have any effect if they
appear after a fragmentation header (HbH and ROUTING).

Looking at the rest of the function we don't check for HBHHDR or RTHDR
following a fragmentation header either if we process the first fragment
(core stack only processes HBH if directly following the ipv6 header
anyway).

So, in my opinion, it is safe to completely remove this check and it
would align if the rest of the extension processing logic. The callers
all seem fine with that.

Pablo, what do you think?

Anyway, the patch does not apply cleanly, the patch header is mangled.
Could you check and send again?

Thanks,
Hannes

Re: [PATCH net] ipv6: Prevent ipv6_find_hdr() from returning ENOENT for valid non-first fragments

[Rahul Sharma]

Hi Hannes,

On Wed, Jan 7, 2015 at 4:13 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> Hi,
>
> On Mi, 2015-01-07 at 11:11 +0530, Rahul Sharma wrote:
>> On Wed, Jan 7, 2015 at 4:17 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> > On Wed, Jan 07, 2015 at 03:03:20AM +0530, Rahul Sharma wrote:
>> >> ipv6_find_hdr() currently assumes that the next-header field in the
>> >> fragment header of the non-first fragment is the "protocol number of
>> >> the last header" (here last header excludes any extension header
>> >> protocol numbers ) which is incorrect as per RFC2460. The next-header
>> >> value is the first header of the fragmentable part of the original
>> >> packet (which can be extension header as well).
>> >> This can create reassembly problems. For example: Fragmented
>> >> authenticated OSPFv3 packets (where AH header is inserted before the
>> >> protocol header). For the second fragment, the next header value in
>> >> the fragment header will be NEXTHDR_AUTH which is correct but
>> >> ipv6_find_hdr will return ENOENT since AH is an extension header
>> >> resulting in second fragment getting dropped. This check for the
>> >> presence of non-extension header needs to be removed.
>> >>
>> >> Signed-off-by: Rahul Sharma <rsharma@arista.com>
>> >> ---
>> >> --- linux-3.18.1/net/ipv6/exthdrs_core.c.orig   2015-01-06
>> >> 10:25:36.411419863 -0800
>> >> +++ linux-3.18.1/net/ipv6/exthdrs_core.c        2015-01-06
>> >> 10:51:45.819364986 -0800
>> >> @@ -171,10 +171,11 @@ EXPORT_SYMBOL_GPL(ipv6_find_tlv);
>> >>   * If the first fragment doesn't contain the final protocol header or
>> >>   * NEXTHDR_NONE it is considered invalid.
>> >>   *
>> >> - * Note that non-1st fragment is special case that "the protocol number
>> >> - * of last header" is "next header" field in Fragment header. In this case,
>> >> - * *offset is meaningless and fragment offset is stored in *fragoff if fragoff
>> >> - * isn't NULL.
>> >> + * Note that non-1st fragment is special case that "the protocol number of the
>> >> + * first header of the fragmentable part of the original packet" is
>> >> + * "next header" field in the Fragment header. In this case, *offset is
>> >> + * meaningless and fragment offset is stored in *fragoff if fragoff isn't
>> >> + * NULL.
>> >>   *
>> >>   * if flags is not NULL and it's a fragment, then the frag flag
>> >>   * IP6_FH_F_FRAG will be set. If it's an AH header, the
>> >> @@ -250,9 +251,7 @@ int ipv6_find_hdr(const struct sk_buff *
>> >>
>> >>                         _frag_off = ntohs(*fp) & ~0x7;
>> >>                         if (_frag_off) {
>> >> -                               if (target < 0 &&
>> >> -                                   ((!ipv6_ext_hdr(hp->nexthdr)) ||
>> >
>> > This check assumes that the following headers cannot show up in the
>> > fragmented part of the IPv6 packet:
>> >
>> >  12 bool ipv6_ext_hdr(u8 nexthdr)
>> >  13 {
>> >  14         /*
>> >  15          * find out if nexthdr is an extension header or a protocol
>> >  16          */
>> >  17         return   (nexthdr == NEXTHDR_HOP)       ||
>> >  18                  (nexthdr == NEXTHDR_ROUTING)   ||
>> >  19                  (nexthdr == NEXTHDR_FRAGMENT)  ||
>> >  20                  (nexthdr == NEXTHDR_AUTH)      ||
>> >  21                  (nexthdr == NEXTHDR_NONE)      ||
>> >  22                  (nexthdr == NEXTHDR_DEST);
>> >
>> >> -                                    hp->nexthdr == NEXTHDR_NONE)) {
>> >> +                               if (target < 0) {
>> >>                                         if (fragoff)
>> >>                                                 *fragoff = _frag_off;
>> >>                                         return hp->nexthdr;
>> >> --
>> >> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> >> the body of a message to majordomo@vger.kernel.org
>> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>> I think this is incorrect. Authentication header shows up in the
>> fragmentable part of the original IPv6 packet. So, for the non-first
>> fragments the next-header field value can be NEXTHDR_AUTH.
>
> Pablo's mail got me thinking again.
>
> In general, IPv6 extension headers can appear in any order and stacks
> must be process them. Fragmentation adds a limitation, that some
> extension headers do not make sense and don't have any effect if they
> appear after a fragmentation header (HbH and ROUTING).
>
> Looking at the rest of the function we don't check for HBHHDR or RTHDR
> following a fragmentation header either if we process the first fragment
> (core stack only processes HBH if directly following the ipv6 header
> anyway).
>
> So, in my opinion, it is safe to completely remove this check and it
> would align if the rest of the extension processing logic. The callers
> all seem fine with that.
>
> Pablo, what do you think?
>
> Anyway, the patch does not apply cleanly, the patch header is mangled.
> Could you check and send again?
>
> Thanks,
> Hannes
>
>
I am not sure if replying on the thread with a patch is a good idea
(or should I send a new email). Anyway, let me know if this is works.

Signed-off-by: Rahul Sharma <rsharma@arista.com>
---
 net/ipv6/exthdrs_core.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/net/ipv6/exthdrs_core.c b/net/ipv6/exthdrs_core.c
index 8af3eb5..5949f87 100644
--- a/net/ipv6/exthdrs_core.c
+++ b/net/ipv6/exthdrs_core.c
@@ -171,10 +171,11 @@ EXPORT_SYMBOL_GPL(ipv6_find_tlv);
  * If the first fragment doesn't contain the final protocol header or
  * NEXTHDR_NONE it is considered invalid.
  *
- * Note that non-1st fragment is special case that "the protocol number
- * of last header" is "next header" field in Fragment header. In this case,
- * *offset is meaningless and fragment offset is stored in *fragoff if fragoff
- * isn't NULL.
+ * Note that non-1st fragment is special case that "the protocol number of the
+ * first header of the fragmentable part of the original packet" is
+ * "next header" field in the Fragment header. In this case, *offset is
+ * meaningless and fragment offset is stored in *fragoff if fragoff isn't
+ * NULL.
  *
  * if flags is not NULL and it's a fragment, then the frag flag
  * IP6_FH_F_FRAG will be set. If it's an AH header, the
@@ -250,9 +251,7 @@ int ipv6_find_hdr(const struct sk_buff *skb,
unsigned int *offset,

                        _frag_off = ntohs(*fp) & ~0x7;
                        if (_frag_off) {
-                               if (target < 0 &&
-                                   ((!ipv6_ext_hdr(hp->nexthdr)) ||
-                                    hp->nexthdr == NEXTHDR_NONE)) {
+                               if (target < 0) {
                                        if (fragoff)
                                                *fragoff = _frag_off;
                                        return hp->nexthdr;
-- 
1.7.4.4

Re: [PATCH net] ipv6: Prevent ipv6_find_hdr() from returning ENOENT for valid non-first fragments

[Hannes Frederic Sowa]

On Do, 2015-01-08 at 02:18 +0530, Rahul Sharma wrote:
> Hi Hannes,
> 
> On Wed, Jan 7, 2015 at 4:13 PM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
> > Hi,
> >
> > On Mi, 2015-01-07 at 11:11 +0530, Rahul Sharma wrote:
> >> On Wed, Jan 7, 2015 at 4:17 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >> > On Wed, Jan 07, 2015 at 03:03:20AM +0530, Rahul Sharma wrote:
> >> >> ipv6_find_hdr() currently assumes that the next-header field in the
> >> >> fragment header of the non-first fragment is the "protocol number of
> >> >> the last header" (here last header excludes any extension header
> >> >> protocol numbers ) which is incorrect as per RFC2460. The next-header
> >> >> value is the first header of the fragmentable part of the original
> >> >> packet (which can be extension header as well).
> >> >> This can create reassembly problems. For example: Fragmented
> >> >> authenticated OSPFv3 packets (where AH header is inserted before the
> >> >> protocol header). For the second fragment, the next header value in
> >> >> the fragment header will be NEXTHDR_AUTH which is correct but
> >> >> ipv6_find_hdr will return ENOENT since AH is an extension header
> >> >> resulting in second fragment getting dropped. This check for the
> >> >> presence of non-extension header needs to be removed.
> >> >>
> >> >> Signed-off-by: Rahul Sharma <rsharma@arista.com>
> >> >> ---
> >> >> --- linux-3.18.1/net/ipv6/exthdrs_core.c.orig   2015-01-06
> >> >> 10:25:36.411419863 -0800
> >> >> +++ linux-3.18.1/net/ipv6/exthdrs_core.c        2015-01-06
> >> >> 10:51:45.819364986 -0800
> >> >> @@ -171,10 +171,11 @@ EXPORT_SYMBOL_GPL(ipv6_find_tlv);
> >> >>   * If the first fragment doesn't contain the final protocol header or
> >> >>   * NEXTHDR_NONE it is considered invalid.
> >> >>   *
> >> >> - * Note that non-1st fragment is special case that "the protocol number
> >> >> - * of last header" is "next header" field in Fragment header. In this case,
> >> >> - * *offset is meaningless and fragment offset is stored in *fragoff if fragoff
> >> >> - * isn't NULL.
> >> >> + * Note that non-1st fragment is special case that "the protocol number of the
> >> >> + * first header of the fragmentable part of the original packet" is
> >> >> + * "next header" field in the Fragment header. In this case, *offset is
> >> >> + * meaningless and fragment offset is stored in *fragoff if fragoff isn't
> >> >> + * NULL.
> >> >>   *
> >> >>   * if flags is not NULL and it's a fragment, then the frag flag
> >> >>   * IP6_FH_F_FRAG will be set. If it's an AH header, the
> >> >> @@ -250,9 +251,7 @@ int ipv6_find_hdr(const struct sk_buff *
> >> >>
> >> >>                         _frag_off = ntohs(*fp) & ~0x7;
> >> >>                         if (_frag_off) {
> >> >> -                               if (target < 0 &&
> >> >> -                                   ((!ipv6_ext_hdr(hp->nexthdr)) ||
> >> >
> >> > This check assumes that the following headers cannot show up in the
> >> > fragmented part of the IPv6 packet:
> >> >
> >> >  12 bool ipv6_ext_hdr(u8 nexthdr)
> >> >  13 {
> >> >  14         /*
> >> >  15          * find out if nexthdr is an extension header or a protocol
> >> >  16          */
> >> >  17         return   (nexthdr == NEXTHDR_HOP)       ||
> >> >  18                  (nexthdr == NEXTHDR_ROUTING)   ||
> >> >  19                  (nexthdr == NEXTHDR_FRAGMENT)  ||
> >> >  20                  (nexthdr == NEXTHDR_AUTH)      ||
> >> >  21                  (nexthdr == NEXTHDR_NONE)      ||
> >> >  22                  (nexthdr == NEXTHDR_DEST);
> >> >
> >> >> -                                    hp->nexthdr == NEXTHDR_NONE)) {
> >> >> +                               if (target < 0) {
> >> >>                                         if (fragoff)
> >> >>                                                 *fragoff = _frag_off;
> >> >>                                         return hp->nexthdr;
> >> >> --
> >> >> To unsubscribe from this list: send the line "unsubscribe netdev" in
> >> >> the body of a message to majordomo@vger.kernel.org
> >> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> >> I think this is incorrect. Authentication header shows up in the
> >> fragmentable part of the original IPv6 packet. So, for the non-first
> >> fragments the next-header field value can be NEXTHDR_AUTH.
> >
> > Pablo's mail got me thinking again.
> >
> > In general, IPv6 extension headers can appear in any order and stacks
> > must be process them. Fragmentation adds a limitation, that some
> > extension headers do not make sense and don't have any effect if they
> > appear after a fragmentation header (HbH and ROUTING).
> >
> > Looking at the rest of the function we don't check for HBHHDR or RTHDR
> > following a fragmentation header either if we process the first fragment
> > (core stack only processes HBH if directly following the ipv6 header
> > anyway).
> >
> > So, in my opinion, it is safe to completely remove this check and it
> > would align if the rest of the extension processing logic. The callers
> > all seem fine with that.
> >
> > Pablo, what do you think?
> >
> > Anyway, the patch does not apply cleanly, the patch header is mangled.
> > Could you check and send again?
> >
> > Thanks,
> > Hannes
> >
> >
> I am not sure if replying on the thread with a patch is a good idea
> (or should I send a new email). Anyway, let me know if this is works.
> 

The patch was identified correctly but the commit message now is
scrambled, see:

http://patchwork.ozlabs.org/patch/426404/

Maybe just resend it as "[PATCH net v2]"?

Thanks,
Hannes

Re: [PATCH net] ipv6: Prevent ipv6_find_hdr() from returning ENOENT for valid non-first fragments

[Pablo Neira Ayuso]

On Wed, Jan 07, 2015 at 11:43:16AM +0100, Hannes Frederic Sowa wrote:
> > >> @@ -250,9 +251,7 @@ int ipv6_find_hdr(const struct sk_buff *
> > >>
> > >>                         _frag_off = ntohs(*fp) & ~0x7;
> > >>                         if (_frag_off) {
> > >> -                               if (target < 0 &&
> > >> -                                   ((!ipv6_ext_hdr(hp->nexthdr)) ||
> > >
> > > This check assumes that the following headers cannot show up in the
> > > fragmented part of the IPv6 packet:
> > >
> > >  12 bool ipv6_ext_hdr(u8 nexthdr)
> > >  13 {
> > >  14         /*
> > >  15          * find out if nexthdr is an extension header or a protocol
> > >  16          */
> > >  17         return   (nexthdr == NEXTHDR_HOP)       ||
> > >  18                  (nexthdr == NEXTHDR_ROUTING)   ||
> > >  19                  (nexthdr == NEXTHDR_FRAGMENT)  ||
> > >  20                  (nexthdr == NEXTHDR_AUTH)      ||
> > >  21                  (nexthdr == NEXTHDR_NONE)      ||
> > >  22                  (nexthdr == NEXTHDR_DEST);
> > >
> > >> -                                    hp->nexthdr == NEXTHDR_NONE)) {
> > >> +                               if (target < 0) {
> > >>                                         if (fragoff)
> > >>                                                 *fragoff = _frag_off;
> > >>                                         return hp->nexthdr;
> > >> --
> > >> To unsubscribe from this list: send the line "unsubscribe netdev" in
> > >> the body of a message to majordomo@vger.kernel.org
> > >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > I think this is incorrect. Authentication header shows up in the
> > fragmentable part of the original IPv6 packet. So, for the non-first
> > fragments the next-header field value can be NEXTHDR_AUTH.
> 
> Pablo's mail got me thinking again.
> 
> In general, IPv6 extension headers can appear in any order and stacks
> must be process them. Fragmentation adds a limitation, that some
> extension headers do not make sense and don't have any effect if they
> appear after a fragmentation header (HbH and ROUTING).
> 
> Looking at the rest of the function we don't check for HBHHDR or RTHDR
> following a fragmentation header either if we process the first fragment
> (core stack only processes HBH if directly following the ipv6 header
> anyway).
> 
> So, in my opinion, it is safe to completely remove this check and it
> would align if the rest of the extension processing logic. The callers
> all seem fine with that.
> 
> Pablo, what do you think?

I'm afraid we cannot just get rid of that !ipv6_ext_hdr() check. The
ipv6_find_hdr() function is designed to return the transport protocol.
After the proposed change, it will return extension header numbers.
This will break existing ip6tables rulesets since the `-p' option
relies on this function to match the transport protocol.

Note that the AH header is skipped (see code a bit below this
problematic fragmentation handling) so the follow up header after the
AH header is returned as the transport header.

We can probably return the AH protocol number for non-1st fragments.
However, that would be something new to ip6tables since nobody has
ever seen packet matching `-p ah' rules. Thus, we restore control to
the user to allow this, but we would accept all kind of fragmented AH
traffic through the firewall since we cannot know what transport
protocol contains from non-1st fragments (unless I'm missing anything,
I need to have a closer look at this again tomorrow with fresher
mind).

Re: [PATCH net] ipv6: Prevent ipv6_find_hdr() from returning ENOENT for valid non-first fragments

[Pablo Neira Ayuso]

On Thu, Jan 08, 2015 at 09:53:28PM +0100, Pablo Neira Ayuso wrote:
> On Wed, Jan 07, 2015 at 11:43:16AM +0100, Hannes Frederic Sowa wrote:
> > > >> @@ -250,9 +251,7 @@ int ipv6_find_hdr(const struct sk_buff *
> > > >>
> > > >>                         _frag_off = ntohs(*fp) & ~0x7;
> > > >>                         if (_frag_off) {
> > > >> -                               if (target < 0 &&
> > > >> -                                   ((!ipv6_ext_hdr(hp->nexthdr)) ||
> > > >
> > > > This check assumes that the following headers cannot show up in the
> > > > fragmented part of the IPv6 packet:
> > > >
> > > >  12 bool ipv6_ext_hdr(u8 nexthdr)
> > > >  13 {
> > > >  14         /*
> > > >  15          * find out if nexthdr is an extension header or a protocol
> > > >  16          */
> > > >  17         return   (nexthdr == NEXTHDR_HOP)       ||
> > > >  18                  (nexthdr == NEXTHDR_ROUTING)   ||
> > > >  19                  (nexthdr == NEXTHDR_FRAGMENT)  ||
> > > >  20                  (nexthdr == NEXTHDR_AUTH)      ||
> > > >  21                  (nexthdr == NEXTHDR_NONE)      ||
> > > >  22                  (nexthdr == NEXTHDR_DEST);
> > > >
> > > >> -                                    hp->nexthdr == NEXTHDR_NONE)) {
> > > >> +                               if (target < 0) {
> > > >>                                         if (fragoff)
> > > >>                                                 *fragoff = _frag_off;
> > > >>                                         return hp->nexthdr;
> > > >> --
> > > >> To unsubscribe from this list: send the line "unsubscribe netdev" in
> > > >> the body of a message to majordomo@vger.kernel.org
> > > >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> > > I think this is incorrect. Authentication header shows up in the
> > > fragmentable part of the original IPv6 packet. So, for the non-first
> > > fragments the next-header field value can be NEXTHDR_AUTH.
> > 
> > Pablo's mail got me thinking again.
> > 
> > In general, IPv6 extension headers can appear in any order and stacks
> > must be process them. Fragmentation adds a limitation, that some
> > extension headers do not make sense and don't have any effect if they
> > appear after a fragmentation header (HbH and ROUTING).
> > 
> > Looking at the rest of the function we don't check for HBHHDR or RTHDR
> > following a fragmentation header either if we process the first fragment
> > (core stack only processes HBH if directly following the ipv6 header
> > anyway).
> > 
> > So, in my opinion, it is safe to completely remove this check and it
> > would align if the rest of the extension processing logic. The callers
> > all seem fine with that.
> > 
> > Pablo, what do you think?
> 
> I'm afraid we cannot just get rid of that !ipv6_ext_hdr() check. The
> ipv6_find_hdr() function is designed to return the transport protocol.
> After the proposed change, it will return extension header numbers.
> This will break existing ip6tables rulesets since the `-p' option
> relies on this function to match the transport protocol.
> 
> Note that the AH header is skipped (see code a bit below this
> problematic fragmentation handling) so the follow up header after the
> AH header is returned as the transport header.
> 
> We can probably return the AH protocol number for non-1st fragments.
> However, that would be something new to ip6tables since nobody has
> ever seen packet matching `-p ah' rules.

# ip6tables -I INPUT -p ah
Warning: never matched protocol: ah. use extension match instead.

Re: [PATCH net] ipv6: Prevent ipv6_find_hdr() from returning ENOENT for valid non-first fragments

[Hannes Frederic Sowa]

Hi Pablo,

On Thu, Jan 8, 2015, at 21:53, Pablo Neira Ayuso wrote:
> I'm afraid we cannot just get rid of that !ipv6_ext_hdr() check. The
> ipv6_find_hdr() function is designed to return the transport protocol.
> After the proposed change, it will return extension header numbers.
> This will break existing ip6tables rulesets since the `-p' option
> relies on this function to match the transport protocol.
>
> Note that the AH header is skipped (see code a bit below this
> problematic fragmentation handling) so the follow up header after the
> AH header is returned as the transport header.
> 
> We can probably return the AH protocol number for non-1st fragments.
> However, that would be something new to ip6tables since nobody has
> ever seen packet matching `-p ah' rules. Thus, we restore control to
> the user to allow this, but we would accept all kind of fragmented AH
> traffic through the firewall since we cannot know what transport
> protocol contains from non-1st fragments (unless I'm missing anything,
> I need to have a closer look at this again tomorrow with fresher
> mind).

The code in question is guarded by (_frag_off != 0), so we are
definitely processing a non-1st fragment currently. The -p match would
happen at the time when the packet is reassembled and thus ipv6_find_hdr
will find the real transport (final) header at this point (I hope I
followed the code correctly here).

The next proto field of the fragmentation header is copied from the
first fragment, thus it may specify AH header but actually only in the
original (unfragmented) packet an AH header is directly following the
fragmentation header, thus the next proto chain is somewhat unstable if
looking at the fragmented packets only.

Bye,
Hannes

Re: [PATCH net] ipv6: Prevent ipv6_find_hdr() from returning ENOENT for valid non-first fragments

[Pablo Neira Ayuso]

On Thu, Jan 08, 2015 at 11:39:16PM +0100, Hannes Frederic Sowa wrote:
> Hi Pablo,
> 
> On Thu, Jan 8, 2015, at 21:53, Pablo Neira Ayuso wrote:
> > I'm afraid we cannot just get rid of that !ipv6_ext_hdr() check. The
> > ipv6_find_hdr() function is designed to return the transport protocol.
> > After the proposed change, it will return extension header numbers.
> > This will break existing ip6tables rulesets since the `-p' option
> > relies on this function to match the transport protocol.
> >
> > Note that the AH header is skipped (see code a bit below this
> > problematic fragmentation handling) so the follow up header after the
> > AH header is returned as the transport header.
> > 
> > We can probably return the AH protocol number for non-1st fragments.
> > However, that would be something new to ip6tables since nobody has
> > ever seen packet matching `-p ah' rules. Thus, we restore control to
> > the user to allow this, but we would accept all kind of fragmented AH
> > traffic through the firewall since we cannot know what transport
> > protocol contains from non-1st fragments (unless I'm missing anything,
> > I need to have a closer look at this again tomorrow with fresher
> > mind).
> 
> The code in question is guarded by (_frag_off != 0), so we are
> definitely processing a non-1st fragment currently. The -p match would
> happen at the time when the packet is reassembled and thus ipv6_find_hdr
> will find the real transport (final) header at this point (I hope I
> followed the code correctly here).

Then, Rahul should get things working by modprobing nf_defrag_ipv6.

Re: [PATCH net] ipv6: Prevent ipv6_find_hdr() from returning ENOENT for valid non-first fragments

[Rahul Sharma]

Hi Pablo,

On Fri, Jan 9, 2015 at 5:35 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Jan 08, 2015 at 11:39:16PM +0100, Hannes Frederic Sowa wrote:
>> Hi Pablo,
>>
>> On Thu, Jan 8, 2015, at 21:53, Pablo Neira Ayuso wrote:
>> > I'm afraid we cannot just get rid of that !ipv6_ext_hdr() check. The
>> > ipv6_find_hdr() function is designed to return the transport protocol.
>> > After the proposed change, it will return extension header numbers.
>> > This will break existing ip6tables rulesets since the `-p' option
>> > relies on this function to match the transport protocol.
>> >
>> > Note that the AH header is skipped (see code a bit below this
>> > problematic fragmentation handling) so the follow up header after the
>> > AH header is returned as the transport header.
>> >
>> > We can probably return the AH protocol number for non-1st fragments.
>> > However, that would be something new to ip6tables since nobody has
>> > ever seen packet matching `-p ah' rules. Thus, we restore control to
>> > the user to allow this, but we would accept all kind of fragmented AH
>> > traffic through the firewall since we cannot know what transport
>> > protocol contains from non-1st fragments (unless I'm missing anything,
>> > I need to have a closer look at this again tomorrow with fresher
>> > mind).
>>
>> The code in question is guarded by (_frag_off != 0), so we are
>> definitely processing a non-1st fragment currently. The -p match would
>> happen at the time when the packet is reassembled and thus ipv6_find_hdr
>> will find the real transport (final) header at this point (I hope I
>> followed the code correctly here).
>
> Then, Rahul should get things working by modprobing nf_defrag_ipv6.

I already had nf_defrag_ipv6 installed when the issue occured. But I
see ip6table_raw_hook returning NF_DROP for the second fragment.

Thanks,
Rahul

Re: [PATCH net] ipv6: Prevent ipv6_find_hdr() from returning ENOENT for valid non-first fragments

[Hannes Frederic Sowa]

On Fri, Jan 9, 2015, at 08:18, Rahul Sharma wrote:
> Hi Pablo,
> 
> On Fri, Jan 9, 2015 at 5:35 AM, Pablo Neira Ayuso <pablo@netfilter.org>
> wrote:
> > On Thu, Jan 08, 2015 at 11:39:16PM +0100, Hannes Frederic Sowa wrote:
> >> Hi Pablo,
> >>
> >> On Thu, Jan 8, 2015, at 21:53, Pablo Neira Ayuso wrote:
> >> > I'm afraid we cannot just get rid of that !ipv6_ext_hdr() check. The
> >> > ipv6_find_hdr() function is designed to return the transport protocol.
> >> > After the proposed change, it will return extension header numbers.
> >> > This will break existing ip6tables rulesets since the `-p' option
> >> > relies on this function to match the transport protocol.
> >> >
> >> > Note that the AH header is skipped (see code a bit below this
> >> > problematic fragmentation handling) so the follow up header after the
> >> > AH header is returned as the transport header.
> >> >
> >> > We can probably return the AH protocol number for non-1st fragments.
> >> > However, that would be something new to ip6tables since nobody has
> >> > ever seen packet matching `-p ah' rules. Thus, we restore control to
> >> > the user to allow this, but we would accept all kind of fragmented AH
> >> > traffic through the firewall since we cannot know what transport
> >> > protocol contains from non-1st fragments (unless I'm missing anything,
> >> > I need to have a closer look at this again tomorrow with fresher
> >> > mind).
> >>
> >> The code in question is guarded by (_frag_off != 0), so we are
> >> definitely processing a non-1st fragment currently. The -p match would
> >> happen at the time when the packet is reassembled and thus ipv6_find_hdr
> >> will find the real transport (final) header at this point (I hope I
> >> followed the code correctly here).
> >
> > Then, Rahul should get things working by modprobing nf_defrag_ipv6.
> 
> I already had nf_defrag_ipv6 installed when the issue occured. But I
> see ip6table_raw_hook returning NF_DROP for the second fragment.

That's what I expected. I think the change only affects hooks before
reassembly.
Pablo, do we care about that, otherwise we should start audit the
callers?

Bye,
Hannes

Re: [PATCH net] ipv6: Prevent ipv6_find_hdr() from returning ENOENT for valid non-first fragments

[Pablo Neira Ayuso]

Hi Hannes,

On Fri, Jan 09, 2015 at 12:34:15PM +0100, Hannes Frederic Sowa wrote:
> On Fri, Jan 9, 2015, at 08:18, Rahul Sharma wrote:
> > Hi Pablo,
> > 
> > On Fri, Jan 9, 2015 at 5:35 AM, Pablo Neira Ayuso <pablo@netfilter.org>
> > wrote:
> > > On Thu, Jan 08, 2015 at 11:39:16PM +0100, Hannes Frederic Sowa wrote:
> > >> Hi Pablo,
> > >>
> > >> On Thu, Jan 8, 2015, at 21:53, Pablo Neira Ayuso wrote:
> > >> > I'm afraid we cannot just get rid of that !ipv6_ext_hdr() check. The
> > >> > ipv6_find_hdr() function is designed to return the transport protocol.
> > >> > After the proposed change, it will return extension header numbers.
> > >> > This will break existing ip6tables rulesets since the `-p' option
> > >> > relies on this function to match the transport protocol.
> > >> >
> > >> > Note that the AH header is skipped (see code a bit below this
> > >> > problematic fragmentation handling) so the follow up header after the
> > >> > AH header is returned as the transport header.
> > >> >
> > >> > We can probably return the AH protocol number for non-1st fragments.
> > >> > However, that would be something new to ip6tables since nobody has
> > >> > ever seen packet matching `-p ah' rules. Thus, we restore control to
> > >> > the user to allow this, but we would accept all kind of fragmented AH
> > >> > traffic through the firewall since we cannot know what transport
> > >> > protocol contains from non-1st fragments (unless I'm missing anything,
> > >> > I need to have a closer look at this again tomorrow with fresher
> > >> > mind).
> > >>
> > >> The code in question is guarded by (_frag_off != 0), so we are
> > >> definitely processing a non-1st fragment currently. The -p match would
> > >> happen at the time when the packet is reassembled and thus ipv6_find_hdr
> > >> will find the real transport (final) header at this point (I hope I
> > >> followed the code correctly here).
> > >
> > > Then, Rahul should get things working by modprobing nf_defrag_ipv6.
> > 
> > I already had nf_defrag_ipv6 installed when the issue occured. But I
> > see ip6table_raw_hook returning NF_DROP for the second fragment.
> 
> That's what I expected. I think the change only affects hooks before
> reassembly.

reassembly happens at NF_IP6_PRI_CONNTRACK_DEFRAG (-400), so that
happens before NF_IP6_PRI_RAW (-300) in IPv6 which is where the raw
table is placed.

Re: [PATCH net] ipv6: Prevent ipv6_find_hdr() from returning ENOENT for valid non-first fragments

[Hannes Frederic Sowa]

On Fr, 2015-01-09 at 12:45 +0100, Pablo Neira Ayuso wrote:
> Hi Hannes,
> 
> On Fri, Jan 09, 2015 at 12:34:15PM +0100, Hannes Frederic Sowa wrote:
> > On Fri, Jan 9, 2015, at 08:18, Rahul Sharma wrote:
> > > Hi Pablo,
> > > 
> > > On Fri, Jan 9, 2015 at 5:35 AM, Pablo Neira Ayuso <pablo@netfilter.org>
> > > wrote:
> > > > On Thu, Jan 08, 2015 at 11:39:16PM +0100, Hannes Frederic Sowa wrote:
> > > >> Hi Pablo,
> > > >>
> > > >> On Thu, Jan 8, 2015, at 21:53, Pablo Neira Ayuso wrote:
> > > >> > I'm afraid we cannot just get rid of that !ipv6_ext_hdr() check. The
> > > >> > ipv6_find_hdr() function is designed to return the transport protocol.
> > > >> > After the proposed change, it will return extension header numbers.
> > > >> > This will break existing ip6tables rulesets since the `-p' option
> > > >> > relies on this function to match the transport protocol.
> > > >> >
> > > >> > Note that the AH header is skipped (see code a bit below this
> > > >> > problematic fragmentation handling) so the follow up header after the
> > > >> > AH header is returned as the transport header.
> > > >> >
> > > >> > We can probably return the AH protocol number for non-1st fragments.
> > > >> > However, that would be something new to ip6tables since nobody has
> > > >> > ever seen packet matching `-p ah' rules. Thus, we restore control to
> > > >> > the user to allow this, but we would accept all kind of fragmented AH
> > > >> > traffic through the firewall since we cannot know what transport
> > > >> > protocol contains from non-1st fragments (unless I'm missing anything,
> > > >> > I need to have a closer look at this again tomorrow with fresher
> > > >> > mind).
> > > >>
> > > >> The code in question is guarded by (_frag_off != 0), so we are
> > > >> definitely processing a non-1st fragment currently. The -p match would
> > > >> happen at the time when the packet is reassembled and thus ipv6_find_hdr
> > > >> will find the real transport (final) header at this point (I hope I
> > > >> followed the code correctly here).
> > > >
> > > > Then, Rahul should get things working by modprobing nf_defrag_ipv6.
> > > 
> > > I already had nf_defrag_ipv6 installed when the issue occured. But I
> > > see ip6table_raw_hook returning NF_DROP for the second fragment.
> > 
> > That's what I expected. I think the change only affects hooks before
> > reassembly.
> 
> reassembly happens at NF_IP6_PRI_CONNTRACK_DEFRAG (-400), so that
> happens before NF_IP6_PRI_RAW (-300) in IPv6 which is where the raw
> table is placed.

I tried to reproduce it, but couldn't get non-1st fragments getting
dropped during traversal of the raw table. They get dropped earlier at
during reassembly or pass.

I agree with Pablo, I also would like to see more data.

Thanks,
Hannes

Re: [PATCH net] ipv6: Prevent ipv6_find_hdr() from returning ENOENT for valid non-first fragments

[Rahul Sharma]

Hi Pablo, Hannes

On Fri, Jan 9, 2015 at 9:20 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On Fr, 2015-01-09 at 12:45 +0100, Pablo Neira Ayuso wrote:
>> Hi Hannes,
>>
>> On Fri, Jan 09, 2015 at 12:34:15PM +0100, Hannes Frederic Sowa wrote:
>> > On Fri, Jan 9, 2015, at 08:18, Rahul Sharma wrote:
>> > > Hi Pablo,
>> > >
>> > > On Fri, Jan 9, 2015 at 5:35 AM, Pablo Neira Ayuso <pablo@netfilter.org>
>> > > wrote:
>> > > > On Thu, Jan 08, 2015 at 11:39:16PM +0100, Hannes Frederic Sowa wrote:
>> > > >> Hi Pablo,
>> > > >>
>> > > >> On Thu, Jan 8, 2015, at 21:53, Pablo Neira Ayuso wrote:
>> > > >> > I'm afraid we cannot just get rid of that !ipv6_ext_hdr() check. The
>> > > >> > ipv6_find_hdr() function is designed to return the transport protocol.
>> > > >> > After the proposed change, it will return extension header numbers.
>> > > >> > This will break existing ip6tables rulesets since the `-p' option
>> > > >> > relies on this function to match the transport protocol.
>> > > >> >
>> > > >> > Note that the AH header is skipped (see code a bit below this
>> > > >> > problematic fragmentation handling) so the follow up header after the
>> > > >> > AH header is returned as the transport header.
>> > > >> >
>> > > >> > We can probably return the AH protocol number for non-1st fragments.
>> > > >> > However, that would be something new to ip6tables since nobody has
>> > > >> > ever seen packet matching `-p ah' rules. Thus, we restore control to
>> > > >> > the user to allow this, but we would accept all kind of fragmented AH
>> > > >> > traffic through the firewall since we cannot know what transport
>> > > >> > protocol contains from non-1st fragments (unless I'm missing anything,
>> > > >> > I need to have a closer look at this again tomorrow with fresher
>> > > >> > mind).
>> > > >>
>> > > >> The code in question is guarded by (_frag_off != 0), so we are
>> > > >> definitely processing a non-1st fragment currently. The -p match would
>> > > >> happen at the time when the packet is reassembled and thus ipv6_find_hdr
>> > > >> will find the real transport (final) header at this point (I hope I
>> > > >> followed the code correctly here).
>> > > >
>> > > > Then, Rahul should get things working by modprobing nf_defrag_ipv6.
>> > >
>> > > I already had nf_defrag_ipv6 installed when the issue occured. But I
>> > > see ip6table_raw_hook returning NF_DROP for the second fragment.
>> >
>> > That's what I expected. I think the change only affects hooks before
>> > reassembly.
>>
>> reassembly happens at NF_IP6_PRI_CONNTRACK_DEFRAG (-400), so that
>> happens before NF_IP6_PRI_RAW (-300) in IPv6 which is where the raw
>> table is placed.
>
> I tried to reproduce it, but couldn't get non-1st fragments getting
> dropped during traversal of the raw table. They get dropped earlier at
> during reassembly or pass.
>
> I agree with Pablo, I also would like to see more data.
>
> Thanks,
> Hannes
>
>

I enabled pr_debug() and there was no error in nf_ct_frag6_gather().
It seems to have defragmented the packet correctly. As expected,
ipv6_defrag() returns NF_STOLEN for the first packet after queuing it.
For the next fragment, ipv6_defrag() calls nf_ct_frag6_output() after
after reassembling it. I checked skb->len inside nf_ct_frag6_output()
and it implies that they were indeed reassembled. Now from what I
follow from the code, this function seems to iterate over both the
fragments with corresponding skb's and incrementing
thresh(NF_IP6_PRI_CONNTRACK_DEFRAG) by 1 so the subsequent hooks
(ip6table_raw_hook, ipv6_conntrack_in, ip6table_mangle_hook) should
get called in order for both the fragments. This works fine for the
first fragment as I had mentioned, but the ip6table_raw_hook returns
NF_DROP for the second fragment. Please let me know if you need more
inputs.

Thanks,
Rahul

Re: [PATCH net] ipv6: Prevent ipv6_find_hdr() from returning ENOENT for valid non-first fragments

[Pablo Neira Ayuso]

On Mon, Jan 12, 2015 at 04:38:16PM +0530, Rahul Sharma wrote:
> Hi Pablo, Hannes
> 
> On Fri, Jan 9, 2015 at 9:20 PM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
> > On Fr, 2015-01-09 at 12:45 +0100, Pablo Neira Ayuso wrote:
> >> Hi Hannes,
> >>
> >> On Fri, Jan 09, 2015 at 12:34:15PM +0100, Hannes Frederic Sowa wrote:
> >> > On Fri, Jan 9, 2015, at 08:18, Rahul Sharma wrote:
> >> > > Hi Pablo,
> >> > >
> >> > > On Fri, Jan 9, 2015 at 5:35 AM, Pablo Neira Ayuso <pablo@netfilter.org>
> >> > > wrote:
> >> > > > On Thu, Jan 08, 2015 at 11:39:16PM +0100, Hannes Frederic Sowa wrote:
> >> > > >> Hi Pablo,
> >> > > >>
> >> > > >> On Thu, Jan 8, 2015, at 21:53, Pablo Neira Ayuso wrote:
> >> > > >> > I'm afraid we cannot just get rid of that !ipv6_ext_hdr() check. The
> >> > > >> > ipv6_find_hdr() function is designed to return the transport protocol.
> >> > > >> > After the proposed change, it will return extension header numbers.
> >> > > >> > This will break existing ip6tables rulesets since the `-p' option
> >> > > >> > relies on this function to match the transport protocol.
> >> > > >> >
> >> > > >> > Note that the AH header is skipped (see code a bit below this
> >> > > >> > problematic fragmentation handling) so the follow up header after the
> >> > > >> > AH header is returned as the transport header.
> >> > > >> >
> >> > > >> > We can probably return the AH protocol number for non-1st fragments.
> >> > > >> > However, that would be something new to ip6tables since nobody has
> >> > > >> > ever seen packet matching `-p ah' rules. Thus, we restore control to
> >> > > >> > the user to allow this, but we would accept all kind of fragmented AH
> >> > > >> > traffic through the firewall since we cannot know what transport
> >> > > >> > protocol contains from non-1st fragments (unless I'm missing anything,
> >> > > >> > I need to have a closer look at this again tomorrow with fresher
> >> > > >> > mind).
> >> > > >>
> >> > > >> The code in question is guarded by (_frag_off != 0), so we are
> >> > > >> definitely processing a non-1st fragment currently. The -p match would
> >> > > >> happen at the time when the packet is reassembled and thus ipv6_find_hdr
> >> > > >> will find the real transport (final) header at this point (I hope I
> >> > > >> followed the code correctly here).
> >> > > >
> >> > > > Then, Rahul should get things working by modprobing nf_defrag_ipv6.
> >> > >
> >> > > I already had nf_defrag_ipv6 installed when the issue occured. But I
> >> > > see ip6table_raw_hook returning NF_DROP for the second fragment.
> >> >
> >> > That's what I expected. I think the change only affects hooks before
> >> > reassembly.
> >>
> >> reassembly happens at NF_IP6_PRI_CONNTRACK_DEFRAG (-400), so that
> >> happens before NF_IP6_PRI_RAW (-300) in IPv6 which is where the raw
> >> table is placed.
> >
> > I tried to reproduce it, but couldn't get non-1st fragments getting
> > dropped during traversal of the raw table. They get dropped earlier at
> > during reassembly or pass.
> >
> > I agree with Pablo, I also would like to see more data.
> >
> > Thanks,
> > Hannes
> >
> >
> 
> I enabled pr_debug() and there was no error in nf_ct_frag6_gather().
> It seems to have defragmented the packet correctly. As expected,
> ipv6_defrag() returns NF_STOLEN for the first packet after queuing it.
> For the next fragment, ipv6_defrag() calls nf_ct_frag6_output() after
> after reassembling it.

nf_ct_frag6_output() doesn't exist anymore. You're using an old
kernel, you should have started by telling so in your report.

See 6aafeef ("netfilter: push reasm skb through instead of original
frag skbs").

Re: [PATCH net] ipv6: Prevent ipv6_find_hdr() from returning ENOENT for valid non-first fragments

[Rahul Sharma]

Hi

On Mon, Jan 12, 2015 at 5:21 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Jan 12, 2015 at 04:38:16PM +0530, Rahul Sharma wrote:
>> Hi Pablo, Hannes
>>
>> On Fri, Jan 9, 2015 at 9:20 PM, Hannes Frederic Sowa
>> <hannes@stressinduktion.org> wrote:
>> > On Fr, 2015-01-09 at 12:45 +0100, Pablo Neira Ayuso wrote:
>> >> Hi Hannes,
>> >>
>> >> On Fri, Jan 09, 2015 at 12:34:15PM +0100, Hannes Frederic Sowa wrote:
>> >> > On Fri, Jan 9, 2015, at 08:18, Rahul Sharma wrote:
>> >> > > Hi Pablo,
>> >> > >
>> >> > > On Fri, Jan 9, 2015 at 5:35 AM, Pablo Neira Ayuso <pablo@netfilter.org>
>> >> > > wrote:
>> >> > > > On Thu, Jan 08, 2015 at 11:39:16PM +0100, Hannes Frederic Sowa wrote:
>> >> > > >> Hi Pablo,
>> >> > > >>
>> >> > > >> On Thu, Jan 8, 2015, at 21:53, Pablo Neira Ayuso wrote:
>> >> > > >> > I'm afraid we cannot just get rid of that !ipv6_ext_hdr() check. The
>> >> > > >> > ipv6_find_hdr() function is designed to return the transport protocol.
>> >> > > >> > After the proposed change, it will return extension header numbers.
>> >> > > >> > This will break existing ip6tables rulesets since the `-p' option
>> >> > > >> > relies on this function to match the transport protocol.
>> >> > > >> >
>> >> > > >> > Note that the AH header is skipped (see code a bit below this
>> >> > > >> > problematic fragmentation handling) so the follow up header after the
>> >> > > >> > AH header is returned as the transport header.
>> >> > > >> >
>> >> > > >> > We can probably return the AH protocol number for non-1st fragments.
>> >> > > >> > However, that would be something new to ip6tables since nobody has
>> >> > > >> > ever seen packet matching `-p ah' rules. Thus, we restore control to
>> >> > > >> > the user to allow this, but we would accept all kind of fragmented AH
>> >> > > >> > traffic through the firewall since we cannot know what transport
>> >> > > >> > protocol contains from non-1st fragments (unless I'm missing anything,
>> >> > > >> > I need to have a closer look at this again tomorrow with fresher
>> >> > > >> > mind).
>> >> > > >>
>> >> > > >> The code in question is guarded by (_frag_off != 0), so we are
>> >> > > >> definitely processing a non-1st fragment currently. The -p match would
>> >> > > >> happen at the time when the packet is reassembled and thus ipv6_find_hdr
>> >> > > >> will find the real transport (final) header at this point (I hope I
>> >> > > >> followed the code correctly here).
>> >> > > >
>> >> > > > Then, Rahul should get things working by modprobing nf_defrag_ipv6.
>> >> > >
>> >> > > I already had nf_defrag_ipv6 installed when the issue occured. But I
>> >> > > see ip6table_raw_hook returning NF_DROP for the second fragment.
>> >> >
>> >> > That's what I expected. I think the change only affects hooks before
>> >> > reassembly.
>> >>
>> >> reassembly happens at NF_IP6_PRI_CONNTRACK_DEFRAG (-400), so that
>> >> happens before NF_IP6_PRI_RAW (-300) in IPv6 which is where the raw
>> >> table is placed.
>> >
>> > I tried to reproduce it, but couldn't get non-1st fragments getting
>> > dropped during traversal of the raw table. They get dropped earlier at
>> > during reassembly or pass.
>> >
>> > I agree with Pablo, I also would like to see more data.
>> >
>> > Thanks,
>> > Hannes
>> >
>> >
>>
>> I enabled pr_debug() and there was no error in nf_ct_frag6_gather().
>> It seems to have defragmented the packet correctly. As expected,
>> ipv6_defrag() returns NF_STOLEN for the first packet after queuing it.
>> For the next fragment, ipv6_defrag() calls nf_ct_frag6_output() after
>> after reassembling it.
>
> nf_ct_frag6_output() doesn't exist anymore. You're using an old
> kernel, you should have started by telling so in your report.
>
> See 6aafeef ("netfilter: push reasm skb through instead of original
> frag skbs").

 I apologize for not mentioning the kernel version in my first mail. I
had suspected problem in ipv6_find_hdr, the code for which was same.
Anyway, thanks for the help. I ll try to figure out how to make this
work in my kernel.

Thanks,
Rahul

Re: [PATCH net] ipv6: Prevent ipv6_find_hdr() from returning ENOENT for valid non-first fragments

[Hannes Frederic Sowa]

On Di, 2015-01-13 at 09:53 +0530, Rahul Sharma wrote:
> On Mon, Jan 12, 2015 at 5:21 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Mon, Jan 12, 2015 at 04:38:16PM +0530, Rahul Sharma wrote:
> >> Hi Pablo, Hannes
> >>
> >> On Fri, Jan 9, 2015 at 9:20 PM, Hannes Frederic Sowa
> >> <hannes@stressinduktion.org> wrote:
> >> > On Fr, 2015-01-09 at 12:45 +0100, Pablo Neira Ayuso wrote:
> >> >> Hi Hannes,
> >> >>
> >> >> On Fri, Jan 09, 2015 at 12:34:15PM +0100, Hannes Frederic Sowa wrote:
> >> >> > On Fri, Jan 9, 2015, at 08:18, Rahul Sharma wrote:
> >> >> > > Hi Pablo,
> >> >> > >
> >> >> > > On Fri, Jan 9, 2015 at 5:35 AM, Pablo Neira Ayuso <pablo@netfilter.org>
> >> >> > > wrote:
> >> >> > > > On Thu, Jan 08, 2015 at 11:39:16PM +0100, Hannes Frederic Sowa wrote:
> >> >> > > >> Hi Pablo,
> >> >> > > >>
> >> >> > > >> On Thu, Jan 8, 2015, at 21:53, Pablo Neira Ayuso wrote:
> >> >> > > >> > I'm afraid we cannot just get rid of that !ipv6_ext_hdr() check. The
> >> >> > > >> > ipv6_find_hdr() function is designed to return the transport protocol.
> >> >> > > >> > After the proposed change, it will return extension header numbers.
> >> >> > > >> > This will break existing ip6tables rulesets since the `-p' option
> >> >> > > >> > relies on this function to match the transport protocol.
> >> >> > > >> >
> >> >> > > >> > Note that the AH header is skipped (see code a bit below this
> >> >> > > >> > problematic fragmentation handling) so the follow up header after the
> >> >> > > >> > AH header is returned as the transport header.
> >> >> > > >> >
> >> >> > > >> > We can probably return the AH protocol number for non-1st fragments.
> >> >> > > >> > However, that would be something new to ip6tables since nobody has
> >> >> > > >> > ever seen packet matching `-p ah' rules. Thus, we restore control to
> >> >> > > >> > the user to allow this, but we would accept all kind of fragmented AH
> >> >> > > >> > traffic through the firewall since we cannot know what transport
> >> >> > > >> > protocol contains from non-1st fragments (unless I'm missing anything,
> >> >> > > >> > I need to have a closer look at this again tomorrow with fresher
> >> >> > > >> > mind).
> >> >> > > >>
> >> >> > > >> The code in question is guarded by (_frag_off != 0), so we are
> >> >> > > >> definitely processing a non-1st fragment currently. The -p match would
> >> >> > > >> happen at the time when the packet is reassembled and thus ipv6_find_hdr
> >> >> > > >> will find the real transport (final) header at this point (I hope I
> >> >> > > >> followed the code correctly here).
> >> >> > > >
> >> >> > > > Then, Rahul should get things working by modprobing nf_defrag_ipv6.
> >> >> > >
> >> >> > > I already had nf_defrag_ipv6 installed when the issue occured. But I
> >> >> > > see ip6table_raw_hook returning NF_DROP for the second fragment.
> >> >> >
> >> >> > That's what I expected. I think the change only affects hooks before
> >> >> > reassembly.
> >> >>
> >> >> reassembly happens at NF_IP6_PRI_CONNTRACK_DEFRAG (-400), so that
> >> >> happens before NF_IP6_PRI_RAW (-300) in IPv6 which is where the raw
> >> >> table is placed.
> >> >
> >> > I tried to reproduce it, but couldn't get non-1st fragments getting
> >> > dropped during traversal of the raw table. They get dropped earlier at
> >> > during reassembly or pass.
> >> >
> >> > I agree with Pablo, I also would like to see more data.
> >> >
> >> > Thanks,
> >> > Hannes
> >> >
> >> >
> >>
> >> I enabled pr_debug() and there was no error in nf_ct_frag6_gather().
> >> It seems to have defragmented the packet correctly. As expected,
> >> ipv6_defrag() returns NF_STOLEN for the first packet after queuing it.
> >> For the next fragment, ipv6_defrag() calls nf_ct_frag6_output() after
> >> after reassembling it.
> >
> > nf_ct_frag6_output() doesn't exist anymore. You're using an old
> > kernel, you should have started by telling so in your report.
> >
> > See 6aafeef ("netfilter: push reasm skb through instead of original
> > frag skbs").
> 
>  I apologize for not mentioning the kernel version in my first mail. I
> had suspected problem in ipv6_find_hdr, the code for which was same.
> Anyway, thanks for the help. I ll try to figure out how to make this
> work in my kernel.

If you have time could you quickly test a recent net-next kernel?

Thanks,
Hannes

Re: [PATCH net] ipv6: Prevent ipv6_find_hdr() from returning ENOENT for valid non-first fragments

[Rahul Sharma]

Hi Hannes,

On Tue, Jan 13, 2015 at 3:41 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On Di, 2015-01-13 at 09:53 +0530, Rahul Sharma wrote:
>> On Mon, Jan 12, 2015 at 5:21 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> > On Mon, Jan 12, 2015 at 04:38:16PM +0530, Rahul Sharma wrote:
>> >> Hi Pablo, Hannes
>> >>
>> >> On Fri, Jan 9, 2015 at 9:20 PM, Hannes Frederic Sowa
>> >> <hannes@stressinduktion.org> wrote:
>> >> > On Fr, 2015-01-09 at 12:45 +0100, Pablo Neira Ayuso wrote:
>> >> >> Hi Hannes,
>> >> >>
>> >> >> On Fri, Jan 09, 2015 at 12:34:15PM +0100, Hannes Frederic Sowa wrote:
>> >> >> > On Fri, Jan 9, 2015, at 08:18, Rahul Sharma wrote:
>> >> >> > > Hi Pablo,
>> >> >> > >
>> >> >> > > On Fri, Jan 9, 2015 at 5:35 AM, Pablo Neira Ayuso <pablo@netfilter.org>
>> >> >> > > wrote:
>> >> >> > > > On Thu, Jan 08, 2015 at 11:39:16PM +0100, Hannes Frederic Sowa wrote:
>> >> >> > > >> Hi Pablo,
>> >> >> > > >>
>> >> >> > > >> On Thu, Jan 8, 2015, at 21:53, Pablo Neira Ayuso wrote:
>> >> >> > > >> > I'm afraid we cannot just get rid of that !ipv6_ext_hdr() check. The
>> >> >> > > >> > ipv6_find_hdr() function is designed to return the transport protocol.
>> >> >> > > >> > After the proposed change, it will return extension header numbers.
>> >> >> > > >> > This will break existing ip6tables rulesets since the `-p' option
>> >> >> > > >> > relies on this function to match the transport protocol.
>> >> >> > > >> >
>> >> >> > > >> > Note that the AH header is skipped (see code a bit below this
>> >> >> > > >> > problematic fragmentation handling) so the follow up header after the
>> >> >> > > >> > AH header is returned as the transport header.
>> >> >> > > >> >
>> >> >> > > >> > We can probably return the AH protocol number for non-1st fragments.
>> >> >> > > >> > However, that would be something new to ip6tables since nobody has
>> >> >> > > >> > ever seen packet matching `-p ah' rules. Thus, we restore control to
>> >> >> > > >> > the user to allow this, but we would accept all kind of fragmented AH
>> >> >> > > >> > traffic through the firewall since we cannot know what transport
>> >> >> > > >> > protocol contains from non-1st fragments (unless I'm missing anything,
>> >> >> > > >> > I need to have a closer look at this again tomorrow with fresher
>> >> >> > > >> > mind).
>> >> >> > > >>
>> >> >> > > >> The code in question is guarded by (_frag_off != 0), so we are
>> >> >> > > >> definitely processing a non-1st fragment currently. The -p match would
>> >> >> > > >> happen at the time when the packet is reassembled and thus ipv6_find_hdr
>> >> >> > > >> will find the real transport (final) header at this point (I hope I
>> >> >> > > >> followed the code correctly here).
>> >> >> > > >
>> >> >> > > > Then, Rahul should get things working by modprobing nf_defrag_ipv6.
>> >> >> > >
>> >> >> > > I already had nf_defrag_ipv6 installed when the issue occured. But I
>> >> >> > > see ip6table_raw_hook returning NF_DROP for the second fragment.
>> >> >> >
>> >> >> > That's what I expected. I think the change only affects hooks before
>> >> >> > reassembly.
>> >> >>
>> >> >> reassembly happens at NF_IP6_PRI_CONNTRACK_DEFRAG (-400), so that
>> >> >> happens before NF_IP6_PRI_RAW (-300) in IPv6 which is where the raw
>> >> >> table is placed.
>> >> >
>> >> > I tried to reproduce it, but couldn't get non-1st fragments getting
>> >> > dropped during traversal of the raw table. They get dropped earlier at
>> >> > during reassembly or pass.
>> >> >
>> >> > I agree with Pablo, I also would like to see more data.
>> >> >
>> >> > Thanks,
>> >> > Hannes
>> >> >
>> >> >
>> >>
>> >> I enabled pr_debug() and there was no error in nf_ct_frag6_gather().
>> >> It seems to have defragmented the packet correctly. As expected,
>> >> ipv6_defrag() returns NF_STOLEN for the first packet after queuing it.
>> >> For the next fragment, ipv6_defrag() calls nf_ct_frag6_output() after
>> >> after reassembling it.
>> >
>> > nf_ct_frag6_output() doesn't exist anymore. You're using an old
>> > kernel, you should have started by telling so in your report.
>> >
>> > See 6aafeef ("netfilter: push reasm skb through instead of original
>> > frag skbs").
>>
>>  I apologize for not mentioning the kernel version in my first mail. I
>> had suspected problem in ipv6_find_hdr, the code for which was same.
>> Anyway, thanks for the help. I ll try to figure out how to make this
>> work in my kernel.
>
> If you have time could you quickly test a recent net-next kernel?
>
> Thanks,
> Hannes
>

I could not test the latest net-next kernel with my current setup.
However, I ported the portion of the patch which pushes reasm skb
through instead of original frags. The rest of the patch was mostly
cleanup of some code which didn't exist in my kernel, so it was
ignored. I could test it thoroughly and it seems to work all fine.

Thanks,
Rahul

Re: [PATCH net] ipv6: Prevent ipv6_find_hdr() from returning ENOENT for valid non-first fragments

[Pablo Neira Ayuso]

On Fri, Jan 09, 2015 at 12:48:24PM +0530, Rahul Sharma wrote:
> Hi Pablo,
> 
> On Fri, Jan 9, 2015 at 5:35 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Thu, Jan 08, 2015 at 11:39:16PM +0100, Hannes Frederic Sowa wrote:
> >> Hi Pablo,
> >>
> >> On Thu, Jan 8, 2015, at 21:53, Pablo Neira Ayuso wrote:
> >> > I'm afraid we cannot just get rid of that !ipv6_ext_hdr() check. The
> >> > ipv6_find_hdr() function is designed to return the transport protocol.
> >> > After the proposed change, it will return extension header numbers.
> >> > This will break existing ip6tables rulesets since the `-p' option
> >> > relies on this function to match the transport protocol.
> >> >
> >> > Note that the AH header is skipped (see code a bit below this
> >> > problematic fragmentation handling) so the follow up header after the
> >> > AH header is returned as the transport header.
> >> >
> >> > We can probably return the AH protocol number for non-1st fragments.
> >> > However, that would be something new to ip6tables since nobody has
> >> > ever seen packet matching `-p ah' rules. Thus, we restore control to
> >> > the user to allow this, but we would accept all kind of fragmented AH
> >> > traffic through the firewall since we cannot know what transport
> >> > protocol contains from non-1st fragments (unless I'm missing anything,
> >> > I need to have a closer look at this again tomorrow with fresher
> >> > mind).
> >>
> >> The code in question is guarded by (_frag_off != 0), so we are
> >> definitely processing a non-1st fragment currently. The -p match would
> >> happen at the time when the packet is reassembled and thus ipv6_find_hdr
> >> will find the real transport (final) header at this point (I hope I
> >> followed the code correctly here).
> >
> > Then, Rahul should get things working by modprobing nf_defrag_ipv6.
> 
> I already had nf_defrag_ipv6 installed when the issue occured. But I
> see ip6table_raw_hook returning NF_DROP for the second fragment.

That's strange, this doesn't make sense to me. Could you enable
pr_debug() debugging in nf_ct_frag6_gather() to check why this packet
was not defragmented?