[PATCH nf 0/2] netfilter: reject cthelper del request if it is in use

[PATCH nf 0/2] netfilter: reject cthelper del request if it is in use

[Liping Zhang]

From: Liping Zhang <zlpnobody@gmail.com>

User can still delete the cthelper even if it is in use:
  # nfct helper add ssdp inet udp
  # iptables -t raw -A OUTPUT -p udp -j CT --helper ssdp
  # nfct helper delete ssdp //--> succeed!

This will cause a use-after-free error. So we shoule add a refcnt to
fix this issue. Before accomplishing this, it's better to introduce a
nf_conntrack_helper_put helper function.

Note, this patch set is based on http://patchwork.ozlabs.org/patch/748533/.
But I think it may still conflict with other patches. If so, I can rebase it.

Liping Zhang (2):
  netfilter: introduce nf_conntrack_helper_put helper function
  netfilter: nfnl_cthelper: reject del request if helper obj is in use

 include/net/netfilter/nf_conntrack_helper.h |  4 ++++
 net/netfilter/nf_conntrack_helper.c         | 12 ++++++++++++
 net/netfilter/nfnetlink_cthelper.c          | 17 +++++++++++------
 net/netfilter/xt_CT.c                       |  6 +++---
 net/openvswitch/conntrack.c                 |  4 ++--
 5 files changed, 32 insertions(+), 11 deletions(-)

-- 
2.5.5

[PATCH nf 1/2] netfilter: introduce nf_conntrack_helper_put helper function

[Liping Zhang]

From: Liping Zhang <zlpnobody@gmail.com>

And convert module_put invocation to nf_conntrack_helper_put, this is
prepared for the later patch, which will add a refcnt for cthelper.

Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
---
 include/net/netfilter/nf_conntrack_helper.h | 2 ++
 net/netfilter/nf_conntrack_helper.c         | 6 ++++++
 net/netfilter/xt_CT.c                       | 6 +++---
 net/openvswitch/conntrack.c                 | 4 ++--
 4 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
index 1eaac1f..65e1dcf 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -58,6 +58,8 @@ struct nf_conntrack_helper *__nf_conntrack_helper_find(const char *name,
 struct nf_conntrack_helper *nf_conntrack_helper_try_module_get(const char *name,
 							       u16 l3num,
 							       u8 protonum);
+void nf_conntrack_helper_put(struct nf_conntrack_helper *helper);
+
 void nf_ct_helper_init(struct nf_conntrack_helper *helper,
 		       u16 l3num, u16 protonum, const char *name,
 		       u16 default_port, u16 spec_port, u32 id,
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 6dc44d9..5275e9a 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -172,6 +172,12 @@ nf_conntrack_helper_try_module_get(const char *name, u16 l3num, u8 protonum)
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_helper_try_module_get);
 
+void nf_conntrack_helper_put(struct nf_conntrack_helper *helper)
+{
+	module_put(helper->me);
+}
+EXPORT_SYMBOL_GPL(nf_conntrack_helper_put);
+
 struct nf_conn_help *
 nf_ct_helper_ext_add(struct nf_conn *ct,
 		     struct nf_conntrack_helper *helper, gfp_t gfp)
diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
index 841cfba..2db42ca 100644
--- a/net/netfilter/xt_CT.c
+++ b/net/netfilter/xt_CT.c
@@ -95,7 +95,7 @@ xt_ct_set_helper(struct nf_conn *ct, const char *helper_name,
 
 	help = nf_ct_helper_ext_add(ct, helper, GFP_KERNEL);
 	if (help == NULL) {
-		module_put(helper->me);
+		nf_conntrack_helper_put(helper);
 		return -ENOMEM;
 	}
 
@@ -260,7 +260,7 @@ static int xt_ct_tg_check(const struct xt_tgchk_param *par,
 err4:
 	help = nfct_help(ct);
 	if (help)
-		module_put(help->helper->me);
+		nf_conntrack_helper_put(help->helper);
 err3:
 	nf_ct_tmpl_free(ct);
 err2:
@@ -343,7 +343,7 @@ static void xt_ct_tg_destroy(const struct xt_tgdtor_param *par,
 	if (ct && !nf_ct_is_untracked(ct)) {
 		help = nfct_help(ct);
 		if (help)
-			module_put(help->helper->me);
+			nf_conntrack_helper_put(help->helper);
 
 		nf_ct_netns_put(par->net, par->family);
 
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 7b2c2fc..fd861d9 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -1084,7 +1084,7 @@ static int ovs_ct_add_helper(struct ovs_conntrack_info *info, const char *name,
 
 	help = nf_ct_helper_ext_add(info->ct, helper, GFP_KERNEL);
 	if (!help) {
-		module_put(helper->me);
+		nf_conntrack_helper_put(helper);
 		return -ENOMEM;
 	}
 
@@ -1534,7 +1534,7 @@ void ovs_ct_free_action(const struct nlattr *a)
 static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info)
 {
 	if (ct_info->helper)
-		module_put(ct_info->helper->me);
+		nf_conntrack_helper_put(ct_info->helper);
 	if (ct_info->ct)
 		nf_ct_tmpl_free(ct_info->ct);
 }
-- 
2.5.5

[PATCH nf 2/2] netfilter: nfnl_cthelper: reject del request if helper obj is in use

[Liping Zhang]

From: Liping Zhang <zlpnobody@gmail.com>

We can still delete the ct helper even if it is in use, this will cause
a use-after-free error. In more detail, I mean:
  # nfct helper add ssdp inet udp
  # iptables -t raw -A OUTPUT -p udp -j CT --helper ssdp
  # nfct helper delete ssdp //--> succeed!

So add reference count to fix this issue, if ct helper is used by
others, reject the delete request.

Apply this patch:
  # nfct helper delete ssdp
  nfct v1.4.3: netlink error: Device or resource busy

Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
---
 Also note, nft ct helper obj only exists in nf-next tree, so after this
 patch appeared in nf-next, I will send another patch to fix it.

 include/net/netfilter/nf_conntrack_helper.h |  2 ++
 net/netfilter/nf_conntrack_helper.c         |  6 ++++++
 net/netfilter/nfnetlink_cthelper.c          | 17 +++++++++++------
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
index 65e1dcf..c7a9ad7 100644
--- a/include/net/netfilter/nf_conntrack_helper.h
+++ b/include/net/netfilter/nf_conntrack_helper.h
@@ -9,6 +9,7 @@
 
 #ifndef _NF_CONNTRACK_HELPER_H
 #define _NF_CONNTRACK_HELPER_H
+#include <linux/refcount.h>
 #include <net/netfilter/nf_conntrack.h>
 #include <net/netfilter/nf_conntrack_extend.h>
 #include <net/netfilter/nf_conntrack_expect.h>
@@ -26,6 +27,7 @@ struct nf_conntrack_helper {
 	struct hlist_node hnode;	/* Internal use. */
 
 	char name[NF_CT_HELPER_NAME_LEN]; /* name of the module */
+	refcount_t refcnt;
 	struct module *me;		/* pointer to self */
 	const struct nf_conntrack_expect_policy *expect_policy;
 
diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 5275e9a..8fdd03b 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -167,6 +167,10 @@ nf_conntrack_helper_try_module_get(const char *name, u16 l3num, u8 protonum)
 #endif
 	if (h != NULL && !try_module_get(h->me))
 		h = NULL;
+	if (h != NULL && !refcount_inc_not_zero(&h->refcnt)) {
+		module_put(h->me);
+		h = NULL;
+	}
 
 	return h;
 }
@@ -174,6 +178,7 @@ EXPORT_SYMBOL_GPL(nf_conntrack_helper_try_module_get);
 
 void nf_conntrack_helper_put(struct nf_conntrack_helper *helper)
 {
+	refcount_dec(&helper->refcnt);
 	module_put(helper->me);
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_helper_put);
@@ -398,6 +403,7 @@ int nf_conntrack_helper_register(struct nf_conntrack_helper *me)
 			goto out;
 		}
 	}
+	refcount_set(&me->refcnt, 1);
 	hlist_add_head_rcu(&me->hnode, &nf_ct_helper_hash[h]);
 	nf_ct_helper_count++;
 out:
diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
index d455581..6e9e3c4 100644
--- a/net/netfilter/nfnetlink_cthelper.c
+++ b/net/netfilter/nfnetlink_cthelper.c
@@ -672,6 +672,7 @@ static int nfnl_cthelper_del(struct net *net, struct sock *nfnl,
 		tuple_set = true;
 	}
 
+	ret = -ENOENT;
 	list_for_each_entry_safe(nlcth, n, &nfnl_cthelper_list, list) {
 		cur = &nlcth->helper;
 		j++;
@@ -685,16 +686,20 @@ static int nfnl_cthelper_del(struct net *net, struct sock *nfnl,
 		     tuple.dst.protonum != cur->tuple.dst.protonum))
 			continue;
 
-		found = true;
-		nf_conntrack_helper_unregister(cur);
-		kfree(cur->expect_policy);
+		if (refcount_dec_if_one(&cur->refcnt)) {
+			found = true;
+			nf_conntrack_helper_unregister(cur);
+			kfree(cur->expect_policy);
 
-		list_del(&nlcth->list);
-		kfree(nlcth);
+			list_del(&nlcth->list);
+			kfree(nlcth);
+		} else {
+			ret = -EBUSY;
+		}
 	}
 
 	/* Make sure we return success if we flush and there is no helpers */
-	return (found || j == 0) ? 0 : -ENOENT;
+	return (found || j == 0) ? 0 : ret;
 }
 
 static const struct nla_policy nfnl_cthelper_policy[NFCTH_MAX+1] = {
-- 
2.5.5

Re: [PATCH nf 2/2] netfilter: nfnl_cthelper: reject del request if helper obj is in use

[Pablo Neira Ayuso]

On Sun, Apr 09, 2017 at 04:22:14PM +0800, Liping Zhang wrote:
> From: Liping Zhang <zlpnobody@gmail.com>
> 
> We can still delete the ct helper even if it is in use, this will cause
> a use-after-free error. In more detail, I mean:
>   # nfct helper add ssdp inet udp
>   # iptables -t raw -A OUTPUT -p udp -j CT --helper ssdp
>   # nfct helper delete ssdp //--> succeed!
> 
> So add reference count to fix this issue, if ct helper is used by
> others, reject the delete request.
> 
> Apply this patch:
>   # nfct helper delete ssdp
>   nfct v1.4.3: netlink error: Device or resource busy
> 
> Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
> ---
>  Also note, nft ct helper obj only exists in nf-next tree, so after this
>  patch appeared in nf-next, I will send another patch to fix it.
> 
>  include/net/netfilter/nf_conntrack_helper.h |  2 ++
>  net/netfilter/nf_conntrack_helper.c         |  6 ++++++
>  net/netfilter/nfnetlink_cthelper.c          | 17 +++++++++++------
>  3 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h
> index 65e1dcf..c7a9ad7 100644
> --- a/include/net/netfilter/nf_conntrack_helper.h
> +++ b/include/net/netfilter/nf_conntrack_helper.h
> @@ -9,6 +9,7 @@
>  
>  #ifndef _NF_CONNTRACK_HELPER_H
>  #define _NF_CONNTRACK_HELPER_H
> +#include <linux/refcount.h>
>  #include <net/netfilter/nf_conntrack.h>
>  #include <net/netfilter/nf_conntrack_extend.h>
>  #include <net/netfilter/nf_conntrack_expect.h>
> @@ -26,6 +27,7 @@ struct nf_conntrack_helper {
>  	struct hlist_node hnode;	/* Internal use. */
>  
>  	char name[NF_CT_HELPER_NAME_LEN]; /* name of the module */
> +	refcount_t refcnt;

Should this new refcnt; thing be in the new struct nfnl_cthelper?

I think this refcount is only required by the userspace helper
infrastructure, not existing in-kernel helpers.

I think like that we can skip patch 1/2 in this series.

Re: [PATCH nf 2/2] netfilter: nfnl_cthelper: reject del request if helper obj is in use

[Liping Zhang]

Hi Pablo,

2017-04-14 7:16 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
[...]
>>  #ifndef _NF_CONNTRACK_HELPER_H
>>  #define _NF_CONNTRACK_HELPER_H
>> +#include <linux/refcount.h>
>>  #include <net/netfilter/nf_conntrack.h>
>>  #include <net/netfilter/nf_conntrack_extend.h>
>>  #include <net/netfilter/nf_conntrack_expect.h>
>> @@ -26,6 +27,7 @@ struct nf_conntrack_helper {
>>       struct hlist_node hnode;        /* Internal use. */
>>
>>       char name[NF_CT_HELPER_NAME_LEN]; /* name of the module */
>> +     refcount_t refcnt;
>
> Should this new refcnt; thing be in the new struct nfnl_cthelper?
>
> I think this refcount is only required by the userspace helper
> infrastructure, not existing in-kernel helpers.
>
> I think like that we can skip patch 1/2 in this series.

We must call nf_conntrack_helper_try_module_get to get the helper,
either it is userspace or  in-kernel helpers. Also the caller didn't care
the helpers is userspace or in-kernel, so I think introducing the
nf_conntrack_helper_put is necessary.

Also, this path set is prepared for per-net helper. For in-kernel helpers,
we will still need to  kmemdup the original one. I mean:
      helper = kmemdup(ftp_helper);
      helper->expect_policy = kmemdup(ftp_exp_policy);
      nf_conntrack_helper_register(net, helper);

And similar to nfnetlink_cttimeout, for net_exit, we should:
list_for_each_entry_safe()
    unregister_helper();
    if (refcount_dec_and_test(&cur->refcnt))
        call_rcu(free_xxx);

For nf_conntrack_helper_put, we should:
if (refcount_dec_and_test(&cur->refcnt))
    call_rcu(free_xxx);

So I think put this "refcount_t refcnt" to struct nf_conntrack_helper
is better.

Re: [PATCH nf 2/2] netfilter: nfnl_cthelper: reject del request if helper obj is in use

[Pablo Neira Ayuso]

On Fri, Apr 14, 2017 at 07:37:50AM +0800, Liping Zhang wrote:
> Hi Pablo,
> 
> 2017-04-14 7:16 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> [...]
> >>  #ifndef _NF_CONNTRACK_HELPER_H
> >>  #define _NF_CONNTRACK_HELPER_H
> >> +#include <linux/refcount.h>
> >>  #include <net/netfilter/nf_conntrack.h>
> >>  #include <net/netfilter/nf_conntrack_extend.h>
> >>  #include <net/netfilter/nf_conntrack_expect.h>
> >> @@ -26,6 +27,7 @@ struct nf_conntrack_helper {
> >>       struct hlist_node hnode;        /* Internal use. */
> >>
> >>       char name[NF_CT_HELPER_NAME_LEN]; /* name of the module */
> >> +     refcount_t refcnt;
> >
> > Should this new refcnt; thing be in the new struct nfnl_cthelper?
> >
> > I think this refcount is only required by the userspace helper
> > infrastructure, not existing in-kernel helpers.
> >
> > I think like that we can skip patch 1/2 in this series.
> 
> We must call nf_conntrack_helper_try_module_get to get the helper,
> either it is userspace or  in-kernel helpers. Also the caller didn't care
> the helpers is userspace or in-kernel, so I think introducing the
> nf_conntrack_helper_put is necessary.
> 
> Also, this path set is prepared for per-net helper. For in-kernel helpers,
> we will still need to  kmemdup the original one. I mean:
>       helper = kmemdup(ftp_helper);
>       helper->expect_policy = kmemdup(ftp_exp_policy);
>       nf_conntrack_helper_register(net, helper);
> 
> And similar to nfnetlink_cttimeout, for net_exit, we should:
> list_for_each_entry_safe()
>     unregister_helper();
>     if (refcount_dec_and_test(&cur->refcnt))
>         call_rcu(free_xxx);
> 
> For nf_conntrack_helper_put, we should:
> if (refcount_dec_and_test(&cur->refcnt))
>     call_rcu(free_xxx);
> 
> So I think put this "refcount_t refcnt" to struct nf_conntrack_helper
> is better.

OK, please resubmit target to nf-next. It's too late for patches
already for nf, we're at -rc6 and this problem has been there for a
bit of time. So waiting a bit more to get this fixed should be OK.

Thanks.

Re: [PATCH nf 2/2] netfilter: nfnl_cthelper: reject del request if helper obj is in use

[Pablo Neira Ayuso]

On Fri, Apr 14, 2017 at 01:42:04AM +0200, Pablo Neira Ayuso wrote:
> On Fri, Apr 14, 2017 at 07:37:50AM +0800, Liping Zhang wrote:
> > Hi Pablo,
> > 
> > 2017-04-14 7:16 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> > [...]
> > >>  #ifndef _NF_CONNTRACK_HELPER_H
> > >>  #define _NF_CONNTRACK_HELPER_H
> > >> +#include <linux/refcount.h>
> > >>  #include <net/netfilter/nf_conntrack.h>
> > >>  #include <net/netfilter/nf_conntrack_extend.h>
> > >>  #include <net/netfilter/nf_conntrack_expect.h>
> > >> @@ -26,6 +27,7 @@ struct nf_conntrack_helper {
> > >>       struct hlist_node hnode;        /* Internal use. */
> > >>
> > >>       char name[NF_CT_HELPER_NAME_LEN]; /* name of the module */
> > >> +     refcount_t refcnt;
> > >
> > > Should this new refcnt; thing be in the new struct nfnl_cthelper?
> > >
> > > I think this refcount is only required by the userspace helper
> > > infrastructure, not existing in-kernel helpers.
> > >
> > > I think like that we can skip patch 1/2 in this series.
> > 
> > We must call nf_conntrack_helper_try_module_get to get the helper,
> > either it is userspace or  in-kernel helpers. Also the caller didn't care
> > the helpers is userspace or in-kernel, so I think introducing the
> > nf_conntrack_helper_put is necessary.
> > 
> > Also, this path set is prepared for per-net helper. For in-kernel helpers,
> > we will still need to  kmemdup the original one. I mean:
> >       helper = kmemdup(ftp_helper);
> >       helper->expect_policy = kmemdup(ftp_exp_policy);
> >       nf_conntrack_helper_register(net, helper);
> > 
> > And similar to nfnetlink_cttimeout, for net_exit, we should:
> > list_for_each_entry_safe()
> >     unregister_helper();
> >     if (refcount_dec_and_test(&cur->refcnt))
> >         call_rcu(free_xxx);
> > 
> > For nf_conntrack_helper_put, we should:
> > if (refcount_dec_and_test(&cur->refcnt))
> >     call_rcu(free_xxx);
> > 
> > So I think put this "refcount_t refcnt" to struct nf_conntrack_helper
> > is better.
> 
> OK, please resubmit target to nf-next. It's too late for patches
> already for nf, we're at -rc6 and this problem has been there for a
> bit of time. So waiting a bit more to get this fixed should be OK.

BTW, you may have to wait until dependencies (ie. patches in nf.git)
show up in nf-next.git, it may take a little while to propagate.