* [PATCH] netfilter: nf_nat: Fix possible null dereference
@ 2015-06-29 17:40 subashab
0 siblings, 0 replies; 6+ messages in thread
From: subashab @ 2015-06-29 17:40 UTC (permalink / raw)
To: netdev; +Cc: pablo
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] netfilter: nf_nat: Fix possible null dereference
@ 2015-07-09 1:23 subashab
2015-07-09 22:24 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: subashab @ 2015-07-09 1:23 UTC (permalink / raw)
To: netfilter-devel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: nf_nat: Fix possible null dereference
2015-07-09 1:23 [PATCH] netfilter: nf_nat: Fix possible null dereference subashab
@ 2015-07-09 22:24 ` Pablo Neira Ayuso
2015-07-09 23:16 ` subashab
0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2015-07-09 22:24 UTC (permalink / raw)
To: subashab; +Cc: netfilter-devel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: nf_nat: Fix possible null dereference
2015-07-09 22:24 ` Pablo Neira Ayuso
@ 2015-07-09 23:16 ` subashab
2015-07-13 15:50 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: subashab @ 2015-07-09 23:16 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
> 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;'
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: nf_nat: Fix possible null dereference
2015-07-09 23:16 ` subashab
@ 2015-07-13 15:50 ` Pablo Neira Ayuso
2015-07-15 1:10 ` subashab
0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2015-07-13 15:50 UTC (permalink / raw)
To: subashab; +Cc: netfilter-devel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: nf_nat: Fix possible null dereference
2015-07-13 15:50 ` Pablo Neira Ayuso
@ 2015-07-15 1:10 ` subashab
0 siblings, 0 replies; 6+ messages in thread
From: subashab @ 2015-07-15 1:10 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-07-15 1:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-09 1:23 [PATCH] netfilter: nf_nat: Fix possible null dereference subashab
2015-07-09 22:24 ` Pablo Neira Ayuso
2015-07-09 23:16 ` subashab
2015-07-13 15:50 ` Pablo Neira Ayuso
2015-07-15 1:10 ` subashab
-- strict thread matches above, loose matches on Subject: below --
2015-06-29 17:40 subashab
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.