netdev - Re: [PATCH v5 nf] netfilter: seqadj: Drop the packet directly when fail to add seqadj extension to avoid dereference NULL pointer later

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+6hz4pRTvCvopOEoxC7oPwCPXKStZ9W31+Bn9fiChCgtsF=eQ@mail.gmail.com>
Date:   Tue, 6 Sep 2016 22:54:54 +0800
From:   Gao Feng <fgao@...ai8.com>
To:     Florian Westphal <fw@...len.de>
Cc:     Pablo Neira Ayuso <pablo@...filter.org>,
        Netfilter Developer Mailing List 
        <netfilter-devel@...r.kernel.org>, coreteam@...filter.org,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH v5 nf] netfilter: seqadj: Drop the packet directly when
 fail to add seqadj extension to avoid dereference NULL pointer later

inline

On Tue, Sep 6, 2016 at 10:51 PM, Florian Westphal <fw@...len.de> wrote:
> fgao@...ai8.com <fgao@...ai8.com> wrote:
>> From: Gao Feng <fgao@...ai8.com>
>>
>> When memory is exhausted, nfct_seqadj_ext_add may fail to add the seqadj
>> extension. But the function nf_ct_seqadj_init doesn't check if get valid
>> seqadj pointer by the nfct_seqadj.
>>
>> Now drop the packet directly when fail to add seqadj extension to avoid
>> dereference NULL pointer in nf_ct_seqadj_init.
>>
>> Signed-off-by: Gao Feng <fgao@...ai8.com>
>> ---
>>  v5: Return NF_ACCEPT instead of NF_DROP when nfct_seqadj_ext_add failed in nf_nat_setup_info
>>  v4: Drop the packet directly when fail to add seqadj extension;
>>  v3: Remove the warning log when seqadj is null;
>>  v2: Remove the unnessary seqadj check in nf_ct_seq_adjust
>>  v1: Initial patch
>>
>>  net/netfilter/nf_conntrack_core.c | 6 +++++-
>>  net/netfilter/nf_nat_core.c       | 3 ++-
>>  2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
>> index dd2c43a..dfa76ce 100644
>> --- a/net/netfilter/nf_conntrack_core.c
>> +++ b/net/netfilter/nf_conntrack_core.c
>> @@ -1036,7 +1036,11 @@ init_conntrack(struct net *net, struct nf_conn *tmpl,
>>               return (struct nf_conntrack_tuple_hash *)ct;
>>
>>       if (tmpl && nfct_synproxy(tmpl)) {
>> -             nfct_seqadj_ext_add(ct);
>> +             if (!nfct_seqadj_ext_add(ct)) {
>> +                     nf_conntrack_free(ct);
>> +                     pr_debug("Can't add seqadj extension\n");
>> +                     return NULL;
>> +             }
>
> if (!nfct_add_synrpxy(ct, tmpl)) {
>         nf_conntrack_free(ct);
>         return NULL;
> }
>
> static bool nf_ct_add_synproxy(struct nf_conn *ct, const struct nf_conn *tmpl)
> {
>         if (tmpl && nfct_synproxy(tmpl)) {
>                 if (!nfct_seqadj_ext_add(ct))
>                         return false;
>
>                 if (!nfct_synproxy_ext_add(ct))
>                         return false;
>         }
>
>         return true;
> }
>
>> diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
>> index de31818..f8b916a 100644
>> --- a/net/netfilter/nf_nat_core.c
>> +++ b/net/netfilter/nf_nat_core.c
>> @@ -441,7 +441,8 @@ nf_nat_setup_info(struct nf_conn *ct,
>>                       ct->status |= IPS_DST_NAT;
>>
>>               if (nfct_help(ct))
>> -                     nfct_seqadj_ext_add(ct);
>> +                     if (!nfct_seqadj_ext_add(ct))
>> +                             return NF_ACCEPT;
>>       }
>
> Hmm, why accept?
>
> We are asked to add extension to rewrite sequence numbers, but
> we cannot.  How can the connection work if we cannot munge/track
> seqno rewrites?

I thought we should drop the packet in that case before.
But Pablo point out one case that the ctnetlink also could add the
seqadj extension too.
There is one synchronization case.

I think he is right. Then modify the patch according to his advice.

Best Regards
Feng