[PATCH] netfilter: nf_nat: Fix possible null dereference

[PATCH] netfilter: nf_nat: Fix possible null dereference

[subashab]

Fix an issue where __nf_ct_ext_find() could return null to nat in
nf_nat_masquerade_ipv4() and could be dereferenced.

This was detected by static analysis software.

Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
---
 net/ipv4/netfilter/nf_nat_masquerade_ipv4.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv4/netfilter/nf_nat_masquerade_ipv4.c
b/net/ipv4/netfilter/nf_nat_masquerade_ipv4.c
index c6eb421..4be5d70 100644
--- a/net/ipv4/netfilter/nf_nat_masquerade_ipv4.c
+++ b/net/ipv4/netfilter/nf_nat_masquerade_ipv4.c
@@ -38,6 +38,8 @@ nf_nat_masquerade_ipv4(struct sk_buff *skb, unsigned int
hooknum,

 	ct = nf_ct_get(skb, &ctinfo);
 	nat = nfct_nat(ct);
+	if (!nat)
+		return NF_DROP;

 	NF_CT_ASSERT(ct && (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED ||
 			    ctinfo == IP_CT_RELATED_REPLY));
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
Foundation Collaborative Project

Re: [PATCH] netfilter: nf_nat: Fix possible null dereference

[Pablo Neira Ayuso]

On Thu, Jul 09, 2015 at 01:23:58AM -0000, subashab@codeaurora.org wrote:
> Fix an issue where __nf_ct_ext_find() could return null to nat in
> nf_nat_masquerade_ipv4() and could be dereferenced.
> 
> This was detected by static analysis software.
> 
> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
> ---
>  net/ipv4/netfilter/nf_nat_masquerade_ipv4.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/ipv4/netfilter/nf_nat_masquerade_ipv4.c
> b/net/ipv4/netfilter/nf_nat_masquerade_ipv4.c
> index c6eb421..4be5d70 100644
> --- a/net/ipv4/netfilter/nf_nat_masquerade_ipv4.c
> +++ b/net/ipv4/netfilter/nf_nat_masquerade_ipv4.c
> @@ -38,6 +38,8 @@ nf_nat_masquerade_ipv4(struct sk_buff *skb, unsigned int
> hooknum,
> 
>  	ct = nf_ct_get(skb, &ctinfo);
>  	nat = nfct_nat(ct);
> +	if (!nat)
> +		return NF_DROP;

This function is called from nf_nat_ipv4_fn(), see do_chain().

And we're accepting the packet with no NAT mangling if we fail to add
the extension:

        nat = nf_ct_nat_ext_add(ct);
        if (nat == NULL)
                return NF_ACCEPT;

Can you provide more information on what your static analysis software
reports? Thanks.

Re: [PATCH] netfilter: nf_nat: Fix possible null dereference

[subashab]

> This function is called from nf_nat_ipv4_fn(), see do_chain().
>
> And we're accepting the packet with no NAT mangling if we fail to add
> the extension:
>
>         nat = nf_ct_nat_ext_add(ct);
>         if (nat == NULL)
>                 return NF_ACCEPT;
>
> Can you provide more information on what your static analysis software
> reports? Thanks.
>

Sure, here is the report

- In nf_nat_masquerade_ipv4.c line 40, 'nat' is assigned the value from
function 'nfct_nat'
- In nf_nat.h line 58, '__nf_ct_ext_find( (ct),  (NF_CT_EXT_NAT) )' is
assigned the return value from function '__nf_ct_ext_find'.
- In nf_conntrack_extend.h line 68, '__nf_ct_ext_find' explicitly returns
a NULL value.

- As a result, pointer 'nat' returned from call to function 'nfct_nat' at
line 40 may be NULL and may be dereferenced at line 59 'nat->masq_index =
out->ifindex;'

Re: [PATCH] netfilter: nf_nat: Fix possible null dereference

[Pablo Neira Ayuso]

On Thu, Jul 09, 2015 at 11:16:05PM -0000, subashab@codeaurora.org wrote:
> > This function is called from nf_nat_ipv4_fn(), see do_chain().
> >
> > And we're accepting the packet with no NAT mangling if we fail to add
> > the extension:
> >
> >         nat = nf_ct_nat_ext_add(ct);
> >         if (nat == NULL)
> >                 return NF_ACCEPT;
> >
> > Can you provide more information on what your static analysis software
> > reports? Thanks.
> >
>
> Sure, here is the report
>
> - In nf_nat_masquerade_ipv4.c line 40, 'nat' is assigned the value from
> function 'nfct_nat'
> - In nf_nat.h line 58, '__nf_ct_ext_find( (ct),  (NF_CT_EXT_NAT) )' is
> assigned the return value from function '__nf_ct_ext_find'.
> - In nf_conntrack_extend.h line 68, '__nf_ct_ext_find' explicitly returns
> a NULL value.
>
> - As a result, pointer 'nat' returned from call to function 'nfct_nat' at
> line 40 may be NULL and may be dereferenced at line 59 'nat->masq_index =
> out->ifindex;'

I see, but if you look nf_nat_ipv4_fn() then you can confirm that we
always have a nat extension in place by when the iptables NAT
targets / nft NAT expressions:

nf_nat_ipv4_fn(...)
{
        [...]

        ct = nf_ct_get(skb, &ctinfo);
        /* Can't track?  It's not due to stress, or conntrack would
         * have dropped it.  Hence it's the user's responsibilty to
         * packet filter it out, or implement conntrack/NAT for that
         * protocol. 8) --RR
         */
        if (!ct)
                return NF_ACCEPT;

        /* Don't try to NAT if this packet is not conntracked */
        if (nf_ct_is_untracked(ct))
                return NF_ACCEPT;

        nat = nf_ct_nat_ext_add(ct);
        if (nat == NULL)
                return NF_ACCEPT;

        ...

If we fail to create the nat extension, then this accepts the packet,
so no chances we can reach this NULL dereference.

I wonder if this is a false positive. Would you please have a closer
look and confirm this? Thanks.

Re: [PATCH] netfilter: nf_nat: Fix possible null dereference

[subashab]

> I see, but if you look nf_nat_ipv4_fn() then you can confirm that we
> always have a nat extension in place by when the iptables NAT
> targets / nft NAT expressions:
>
> nf_nat_ipv4_fn(...)
> {
>         [...]
>
>         ct = nf_ct_get(skb, &ctinfo);
>         /* Can't track?  It's not due to stress, or conntrack would
>          * have dropped it.  Hence it's the user's responsibilty to
>          * packet filter it out, or implement conntrack/NAT for that
>          * protocol. 8) --RR
>          */
>         if (!ct)
>                 return NF_ACCEPT;
>
>         /* Don't try to NAT if this packet is not conntracked */
>         if (nf_ct_is_untracked(ct))
>                 return NF_ACCEPT;
>
>         nat = nf_ct_nat_ext_add(ct);
>         if (nat == NULL)
>                 return NF_ACCEPT;
>
>         ...
>
> If we fail to create the nat extension, then this accepts the packet,
> so no chances we can reach this NULL dereference.
>
> I wonder if this is a false positive. Would you please have a closer
> look and confirm this? Thanks.
>
This report is indeed a false positive. Thanks for reviewing.