diff mbox

[nf] netfilter: nf_ct_helper: permit cthelpers with different names via nfnetlink

Message ID 1490447003-18592-1-git-send-email-zlpnobody@163.com
State Changes Requested
Delegated to: Pablo Neira
Headers show

Commit Message

Liping Zhang March 25, 2017, 1:03 p.m. UTC 
From: Liping Zhang <zlpnobody@gmail.com>

cthelpers added via nfnetlink may have the same tuple, i.e. except for
the l3proto and l4proto, other fields are all zero. So even with the
different names, we will also fail to add them:
  # nfct helper add ssdp inet udp
  # nfct helper add tftp inet udp
  nfct v1.4.3: netlink error: File exists

So in order to avoid unpredictable behaviour, we should:
1. cthelpers can be selected by nft ct helper obj or xt_CT target, so
report error if duplicated { name, l3proto, l4proto } tuple exist.
2. cthelpers can be selected by nf_ct_tuple_src_mask_cmp when
nf_ct_auto_assign_helper is enabled, so also report error if duplicated
{ l3proto, l4proto, src-port } tuple exist.

Also note, if the cthelper is added from userspace, then the src-port will
always be zero, it's invalid for nf_ct_auto_assign_helper, so there's no
need to check the second point listed above.

Fixes: 893e093c786c ("netfilter: nf_ct_helper: bail out on duplicated helpers")
Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
---
 net/netfilter/nf_conntrack_helper.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

Comments

Pablo Neira Ayuso March 29, 2017, 10:36 a.m. UTC | #1 
On Sat, Mar 25, 2017 at 09:03:23PM +0800, Liping Zhang wrote:
> From: Liping Zhang <zlpnobody@gmail.com>
> 
> cthelpers added via nfnetlink may have the same tuple, i.e. except for
> the l3proto and l4proto, other fields are all zero. So even with the
> different names, we will also fail to add them:
>   # nfct helper add ssdp inet udp
>   # nfct helper add tftp inet udp
>   nfct v1.4.3: netlink error: File exists
> 
> So in order to avoid unpredictable behaviour, we should:
> 1. cthelpers can be selected by nft ct helper obj or xt_CT target, so
> report error if duplicated { name, l3proto, l4proto } tuple exist.
> 2. cthelpers can be selected by nf_ct_tuple_src_mask_cmp when
> nf_ct_auto_assign_helper is enabled, so also report error if duplicated
> { l3proto, l4proto, src-port } tuple exist.
> 
> Also note, if the cthelper is added from userspace, then the src-port will
> always be zero, it's invalid for nf_ct_auto_assign_helper, so there's no
> need to check the second point listed above.

Also applied, 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
Pablo Neira Ayuso March 29, 2017, 10:41 a.m. UTC | #2 
On Sat, Mar 25, 2017 at 09:03:23PM +0800, Liping Zhang wrote:
> From: Liping Zhang <zlpnobody@gmail.com>
> 
> cthelpers added via nfnetlink may have the same tuple, i.e. except for
> the l3proto and l4proto, other fields are all zero. So even with the
> different names, we will also fail to add them:
>   # nfct helper add ssdp inet udp
>   # nfct helper add tftp inet udp
>   nfct v1.4.3: netlink error: File exists
> 
> So in order to avoid unpredictable behaviour, we should:
> 1. cthelpers can be selected by nft ct helper obj or xt_CT target, so
> report error if duplicated { name, l3proto, l4proto } tuple exist.
> 2. cthelpers can be selected by nf_ct_tuple_src_mask_cmp when
> nf_ct_auto_assign_helper is enabled, so also report error if duplicated
> { l3proto, l4proto, src-port } tuple exist.
> 
> Also note, if the cthelper is added from userspace, then the src-port will
> always be zero, it's invalid for nf_ct_auto_assign_helper, so there's no
> need to check the second point listed above.
> 
> Fixes: 893e093c786c ("netfilter: nf_ct_helper: bail out on duplicated helpers")
> Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
> ---
>  net/netfilter/nf_conntrack_helper.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
> index 6dc44d9..a28ecfa 100644
> --- a/net/netfilter/nf_conntrack_helper.c
> +++ b/net/netfilter/nf_conntrack_helper.c
> @@ -386,10 +386,20 @@ int nf_conntrack_helper_register(struct nf_conntrack_helper *me)
>  	BUG_ON(strlen(me->name) > NF_CT_HELPER_NAME_LEN - 1);
>  
>  	mutex_lock(&nf_ct_helper_mutex);
> -	hlist_for_each_entry(cur, &nf_ct_helper_hash[h], hnode) {
> -		if (nf_ct_tuple_src_mask_cmp(&cur->tuple, &me->tuple, &mask)) {
> -			ret = -EEXIST;
> -			goto out;
> +	if (__nf_conntrack_helper_find(me->name, me->tuple.src.l3num,
> +				       me->tuple.dst.protonum)) {
> +		ret = -EEXIST;
> +		goto out;
> +	}
> +
> +	/* avoid unpredictable behaviour for auto_assign_helper */
> +	if (!(me->flags & NF_CT_HELPER_F_USERSPACE)) {
> +		hlist_for_each_entry(cur, &nf_ct_helper_hash[h], hnode) {
> +			if (nf_ct_tuple_src_mask_cmp(&cur->tuple, &me->tuple,
> +						     &mask)) {
> +				ret = -EEXIST;
> +				goto out;
> +			}

Wait.

Just a comestic change, would this look better if we just do:

	hlist_for_each_entry(cur, &nf_ct_helper_hash[h], hnode) {
                 if (!strcmp(h->name, name) &&
                     (h->tuple.src.l3num != NFPROTO_UNSPEC &&
                      h->tuple.src.l3num != l3num)) &&
                     h->tuple.dst.protonum == protonum) {
                        ret = -EEXIST;
                        goto out;
                 }

                 /* avoid unpredictable behaviour for auto_assign_helper */
                 if (!(me->flags & NF_CT_HELPER_F_USERSPACE)) &&
                     nf_ct_tuple_src_mask_cmp(&cur->tuple, &me->tuple, &mask)) {
                        ret = -EEXIST;
                        goto out;
	}

__nf_conntrack_helper_find() uses the _rcu variant to iterate over the
helper hash table, and we don't need this given we hold the mutex.
This is likely going to spot false positives with the RCU debugging
instrumentation I think.
--
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
Liping Zhang March 29, 2017, 11:05 a.m. UTC | #3 
Hi Pablo,

2017-03-29 18:41 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
[...]
> Wait.
>
> Just a comestic change, would this look better if we just do:
>
>         hlist_for_each_entry(cur, &nf_ct_helper_hash[h], hnode) {
>                  if (!strcmp(h->name, name) &&
>                      (h->tuple.src.l3num != NFPROTO_UNSPEC &&
>                       h->tuple.src.l3num != l3num)) &&

I think this check condition here should be:
(h->tuple.src.l3num == NFPROTO_UNSPEC || h->tuple.src.l3num == l3num)

>                      h->tuple.dst.protonum == protonum) {
>                         ret = -EEXIST;
>                         goto out;
>                  }
>
>                  /* avoid unpredictable behaviour for auto_assign_helper */
>                  if (!(me->flags & NF_CT_HELPER_F_USERSPACE)) &&
>                      nf_ct_tuple_src_mask_cmp(&cur->tuple, &me->tuple, &mask)) {
>                         ret = -EEXIST;
>                         goto out;
>         }
>
> __nf_conntrack_helper_find() uses the _rcu variant to iterate over the
> helper hash table, and we don't need this given we hold the mutex.
> This is likely going to spot false positives with the RCU debugging
> instrumentation I think.

Right, I did not notice this point. I will send V2 later.
--
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
Pablo Neira Ayuso March 29, 2017, 11:50 a.m. UTC | #4 
On Wed, Mar 29, 2017 at 07:05:10PM +0800, Liping Zhang wrote:
> Hi Pablo,
> 
> 2017-03-29 18:41 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> [...]
> > Wait.
> >
> > Just a comestic change, would this look better if we just do:
> >
> >         hlist_for_each_entry(cur, &nf_ct_helper_hash[h], hnode) {
> >                  if (!strcmp(h->name, name) &&
> >                      (h->tuple.src.l3num != NFPROTO_UNSPEC &&
> >                       h->tuple.src.l3num != l3num)) &&
> 
> I think this check condition here should be:
> (h->tuple.src.l3num == NFPROTO_UNSPEC || h->tuple.src.l3num == l3num)

Yes, I noticed after sending indeed.

> >                      h->tuple.dst.protonum == protonum) {
> >                         ret = -EEXIST;
> >                         goto out;
> >                  }
> >
> >                  /* avoid unpredictable behaviour for auto_assign_helper */
> >                  if (!(me->flags & NF_CT_HELPER_F_USERSPACE)) &&
> >                      nf_ct_tuple_src_mask_cmp(&cur->tuple, &me->tuple, &mask)) {
> >                         ret = -EEXIST;
> >                         goto out;
> >         }
> >
> > __nf_conntrack_helper_find() uses the _rcu variant to iterate over the
> > helper hash table, and we don't need this given we hold the mutex.
> > This is likely going to spot false positives with the RCU debugging
> > instrumentation I think.
> 
> Right, I did not notice this point. I will send V2 later.

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
diff mbox

Patch

diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index 6dc44d9..a28ecfa 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -386,10 +386,20 @@  int nf_conntrack_helper_register(struct nf_conntrack_helper *me)
 	BUG_ON(strlen(me->name) > NF_CT_HELPER_NAME_LEN - 1);
 
 	mutex_lock(&nf_ct_helper_mutex);
-	hlist_for_each_entry(cur, &nf_ct_helper_hash[h], hnode) {
-		if (nf_ct_tuple_src_mask_cmp(&cur->tuple, &me->tuple, &mask)) {
-			ret = -EEXIST;
-			goto out;
+	if (__nf_conntrack_helper_find(me->name, me->tuple.src.l3num,
+				       me->tuple.dst.protonum)) {
+		ret = -EEXIST;
+		goto out;
+	}
+
+	/* avoid unpredictable behaviour for auto_assign_helper */
+	if (!(me->flags & NF_CT_HELPER_F_USERSPACE)) {
+		hlist_for_each_entry(cur, &nf_ct_helper_hash[h], hnode) {
+			if (nf_ct_tuple_src_mask_cmp(&cur->tuple, &me->tuple,
+						     &mask)) {
+				ret = -EEXIST;
+				goto out;
+			}
 		}
 	}
 	hlist_add_head_rcu(&me->hnode, &nf_ct_helper_hash[h]);