[PATCH nf 0/2] restore userspace helper support

[PATCH nf 0/2] restore userspace helper support

[Pablo Neira Ayuso]

Hi,

This patchset restores the userspace helper support:

1) Missing conntrack confirmation after packet reinjection.
2) Missing data_len initialization and incorrect nla_memcpy().

Pablo Neira Ayuso (2):
  netfilter: conntrack: make conntrack userspace helpers work again
  netfilter: nfnetlink_cthelper: unbreak userspace helper support

 net/netfilter/nf_conntrack_core.c  | 79 +++++++++++++++++++++++++++---
 net/netfilter/nfnetlink_cthelper.c |  3 +-
 2 files changed, 75 insertions(+), 7 deletions(-)

-- 
2.20.1

[PATCH nf 1/2] netfilter: conntrack: make conntrack userspace helpers work again

[Pablo Neira Ayuso]

Florian Westphal says:

"Problem is that after the helper hook was merged back into the confirm
one, the queueing itself occurs from the confirm hook, i.e. we queue
from the last netfilter callback in the hook-list.

Therefore, on return, the packet bypasses the confirm action and the
connection is never committed to the main conntrack table.

Therefore, on return, the packet bypasses the confirm action and the
connection is never committed to the main conntrack table.

To fix this there are several ways:
1. revert the 'Fixes' commit and have a extra helper hook again.
   Works, but has the drawback of adding another indirect call for
   everyone.

2. Special case this: split the hooks only when userspace helper
   gets added, so queueing occurs at a lower priority again,
   and normal nqueue reinject would eventually call the last hook.

3. Extend the existing nf_queue ct update hook to allow a forced
   confirmation (plus run the seqadj code).

This goes for 3)."

Fixes: 827318feb69cb ("netfilter: conntrack: remove helper hook again")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_conntrack_core.c | 79 ++++++++++++++++++++++++++++---
 1 file changed, 73 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 1d57b95d3481..d2cbce4695a4 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -2016,22 +2016,18 @@ static void nf_conntrack_attach(struct sk_buff *nskb, const struct sk_buff *skb)
 	nf_conntrack_get(skb_nfct(nskb));
 }
 
-static int nf_conntrack_update(struct net *net, struct sk_buff *skb)
+static int __nf_conntrack_update(struct net *net, struct sk_buff *skb,
+				 struct nf_conn *ct)
 {
 	struct nf_conntrack_tuple_hash *h;
 	struct nf_conntrack_tuple tuple;
 	enum ip_conntrack_info ctinfo;
 	struct nf_nat_hook *nat_hook;
 	unsigned int status;
-	struct nf_conn *ct;
 	int dataoff;
 	u16 l3num;
 	u8 l4num;
 
-	ct = nf_ct_get(skb, &ctinfo);
-	if (!ct || nf_ct_is_confirmed(ct))
-		return 0;
-
 	l3num = nf_ct_l3num(ct);
 
 	dataoff = get_l4proto(skb, skb_network_offset(skb), l3num, &l4num);
@@ -2088,6 +2084,77 @@ static int nf_conntrack_update(struct net *net, struct sk_buff *skb)
 	return 0;
 }
 
+/* This packet is coming from userspace via nf_queue, complete the packet
+ * processing after the helper invocation in nf_confirm().
+ */
+static int nf_confirm_cthelper(struct sk_buff *skb, struct nf_conn *ct,
+			       enum ip_conntrack_info ctinfo)
+{
+	const struct nf_conntrack_helper *helper;
+	const struct nf_conn_help *help;
+	unsigned int protoff;
+
+	help = nfct_help(ct);
+	if (!help)
+		return 0;
+
+	helper = rcu_dereference(help->helper);
+	if (!(helper->flags & NF_CT_HELPER_F_USERSPACE))
+		return 0;
+
+	switch (nf_ct_l3num(ct)) {
+	case NFPROTO_IPV4:
+		protoff = skb_network_offset(skb) + ip_hdrlen(skb);
+		break;
+#if IS_ENABLED(CONFIG_IPV6)
+	case NFPROTO_IPV6: {
+		__be16 frag_off;
+		u8 pnum;
+
+		pnum = ipv6_hdr(skb)->nexthdr;
+		protoff = ipv6_skip_exthdr(skb, sizeof(struct ipv6hdr), &pnum,
+					   &frag_off);
+		if (protoff < 0 || (frag_off & htons(~0x7)) != 0)
+			return 0;
+		break;
+	}
+#endif
+	default:
+		return 0;
+	}
+
+	if (test_bit(IPS_SEQ_ADJUST_BIT, &ct->status) &&
+	    !nf_is_loopback_packet(skb)) {
+		if (!nf_ct_seq_adjust(skb, ct, ctinfo, protoff)) {
+			NF_CT_STAT_INC_ATOMIC(nf_ct_net(ct), drop);
+			return -1;
+		}
+	}
+
+	/* We've seen it coming out the other side: confirm it */
+	return nf_conntrack_confirm(skb) == NF_DROP ? - 1 : 0;
+}
+
+static int nf_conntrack_update(struct net *net, struct sk_buff *skb)
+{
+	enum ip_conntrack_info ctinfo;
+	struct nf_conn *ct;
+	int err;
+
+	ct = nf_ct_get(skb, &ctinfo);
+	if (!ct)
+		return 0;
+
+	err = nf_confirm_cthelper(skb, ct, ctinfo);
+	if (err < 0)
+		return err;
+
+	if (nf_ct_is_confirmed(ct))
+		return 0;
+
+	return __nf_conntrack_update(net, skb, ct);
+}
+
 static bool nf_conntrack_get_tuple_skb(struct nf_conntrack_tuple *dst_tuple,
 				       const struct sk_buff *skb)
 {
-- 
2.20.1

[PATCH nf 2/2] netfilter: nfnetlink_cthelper: unbreak userspace helper support

[Pablo Neira Ayuso]

Restore helper data size initialization and fix memcopy of the helper
data size.

Fixes: 157ffffeb5dc ("netfilter: nfnetlink_cthelper: reject too large userspace allocation requests")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nfnetlink_cthelper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
index a5f294aa8e4c..5b0d0a77379c 100644
--- a/net/netfilter/nfnetlink_cthelper.c
+++ b/net/netfilter/nfnetlink_cthelper.c
@@ -103,7 +103,7 @@ nfnl_cthelper_from_nlattr(struct nlattr *attr, struct nf_conn *ct)
 	if (help->helper->data_len == 0)
 		return -EINVAL;
 
-	nla_memcpy(help->data, nla_data(attr), sizeof(help->data));
+	nla_memcpy(help->data, attr, sizeof(help->data));
 	return 0;
 }
 
@@ -240,6 +240,7 @@ nfnl_cthelper_create(const struct nlattr * const tb[],
 		ret = -ENOMEM;
 		goto err2;
 	}
+	helper->data_len = size;
 
 	helper->flags |= NF_CT_HELPER_F_USERSPACE;
 	memcpy(&helper->tuple, tuple, sizeof(struct nf_conntrack_tuple));
-- 
2.20.1

Re: [PATCH nf 2/2] netfilter: nfnetlink_cthelper: unbreak userspace helper support

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Restore helper data size initialization and fix memcopy of the helper
> data size.

Ouch, thanks for fixing this.

Reviewed-by: Florian Westphal <fw@strlen.de>

Re: [PATCH nf 1/2] netfilter: conntrack: make conntrack userspace helpers work again

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Florian Westphal says:
> 
> "Problem is that after the helper hook was merged back into the confirm
> one, the queueing itself occurs from the confirm hook, i.e. we queue
> from the last netfilter callback in the hook-list.
> 
> Therefore, on return, the packet bypasses the confirm action and the
> connection is never committed to the main conntrack table.
> 
> Therefore, on return, the packet bypasses the confirm action and the
> connection is never committed to the main conntrack table.
> 
> To fix this there are several ways:
> 1. revert the 'Fixes' commit and have a extra helper hook again.
>    Works, but has the drawback of adding another indirect call for
>    everyone.
> 
> 2. Special case this: split the hooks only when userspace helper
>    gets added, so queueing occurs at a lower priority again,
>    and normal nqueue reinject would eventually call the last hook.
> 
> 3. Extend the existing nf_queue ct update hook to allow a forced
>    confirmation (plus run the seqadj code).
> 
> This goes for 3)."
> 
> Fixes: 827318feb69cb ("netfilter: conntrack: remove helper hook again")
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> + * processing after the helper invocation in nf_confirm().
> + */
> +static int nf_confirm_cthelper(struct sk_buff *skb, struct nf_conn *ct,
> +			       enum ip_conntrack_info ctinfo)
> +{
> +	const struct nf_conntrack_helper *helper;
> +	const struct nf_conn_help *help;
> +	unsigned int protoff;
> +
> +	help = nfct_help(ct);
> +	if (!help)
> +		return 0;
> +
> +	helper = rcu_dereference(help->helper);
> +	if (!(helper->flags & NF_CT_HELPER_F_USERSPACE))
> +		return 0;

Relying on this check means that in case of

... -j CT (assign userspace helper)
... -j NFQUEUE

> +	/* We've seen it coming out the other side: confirm it */
> +	return nf_conntrack_confirm(skb) == NF_DROP ? - 1 : 0;
> +}

> +static int nf_conntrack_update(struct net *net, struct sk_buff *skb)
> +{
[..]
> +	err = nf_confirm_cthelper(skb, ct, ctinfo);
> +	if (err < 0)
> +		return err;
> +
> +	if (nf_ct_is_confirmed(ct))
> +		return 0;

This means that in case of userspace helper, we return here
any bypass the __nf_conntrack_update logic.

I don't think thats a problem either given the userspace
helper presence, so

Acked-by: Florian Westphal <fw@strlen.de>