| Message ID | fea9df65d6768053838fb1eca1846ef7.squirrel@www.codeaurora.org |
|---|---|
| State | Not Applicable |
| Delegated to: | Pablo Neira |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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;' -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ||
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(+) 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 -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html