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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> >> >> > > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/