[PATCH nf] netfilter: nf_tables: fix set double-free in abort path

[PATCH nf] netfilter: nf_tables: fix set double-free in abort path

[Florian Westphal]

The abort path can cause a double-free of an (anon) set.

Added-and-to-be-aborted rule looks like this:

udp dport { 137, 138 } drop

The to-be-aborted transaction list looks like this:
newset
newsetelem
newsetelem
rule

This gets walked in reverse order, so first pass disables
the rule, the set elements, then the set.

After synchronize_rcu(), we then destroy those in same order:
rule, set element, set element, newset.

Problem is that the (anon) set has already been bound to the rule,
so the rule (lookup expression destructor) already frees the set,
when then cause use-after-free when trying to delete the elements
from this set, then try to free the set again when handling the
newset expression.

To resolve this, check in first phase if the newset is bound already.
If so, remove the newset transaction from the list, rule destructor
will handle cleanup.

This is still causes the use-after-free on set element removal.
To handle this, move all affected set elements to a extra list
and process it first.

This forces strict 'destroy elements, then set' ordering.

Fixes: f6ac8585897684 ("netfilter: nf_tables: unbind set in rule from commit path")
Bugzilla: https://bugzilla.netfilter.org/show_bug.cgi?id=1325
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_tables_api.c | 38 +++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index faf6bd10a19f..dcd9cb68d826 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -6726,10 +6726,39 @@ static void nf_tables_abort_release(struct nft_trans *trans)
 	kfree(trans);
 }
 
+static void __nf_tables_newset_abort(struct net *net,
+				     struct nft_trans *set_trans,
+				     struct list_head *set_elements)
+{
+	const struct nft_set *set = nft_trans_set(set_trans);
+	struct nft_trans *trans, *next;
+
+	if (!nft_trans_set_bound(set_trans))
+		return;
+
+	/* When abort is in progress, NFT_MSG_NEWRULE will remove the
+	 * set if its bound, so we need to remove the NEWSET transaction,
+	 * else the set is released twice.  NEWSETELEM need to be moved
+	 * to special list to ensure 'free elements, then set' ordering.
+	 */
+	list_for_each_entry_safe_reverse(trans, next,
+					 &net->nft.commit_list, list) {
+		if (trans == set_trans)
+			break;
+
+		if (trans->msg_type == NFT_MSG_NEWSETELEM &&
+		    nft_trans_set(trans) == set)
+			list_move(&trans->list, set_elements);
+	}
+
+	nft_trans_destroy(set_trans);
+}
+
 static int __nf_tables_abort(struct net *net)
 {
 	struct nft_trans *trans, *next;
 	struct nft_trans_elem *te;
+	LIST_HEAD(set_elements);
 
 	list_for_each_entry_safe_reverse(trans, next, &net->nft.commit_list,
 					 list) {
@@ -6785,6 +6814,8 @@ static int __nf_tables_abort(struct net *net)
 			trans->ctx.table->use--;
 			if (!nft_trans_set_bound(trans))
 				list_del_rcu(&nft_trans_set(trans)->list);
+
+			__nf_tables_newset_abort(net, trans, &set_elements);
 			break;
 		case NFT_MSG_DELSET:
 			trans->ctx.table->use++;
@@ -6831,6 +6862,13 @@ static int __nf_tables_abort(struct net *net)
 
 	synchronize_rcu();
 
+	/* free set elements before the set they belong to is freed */
+	list_for_each_entry_safe_reverse(trans, next,
+					 &set_elements, list) {
+		list_del(&trans->list);
+		nf_tables_abort_release(trans);
+	}
+
 	list_for_each_entry_safe_reverse(trans, next,
 					 &net->nft.commit_list, list) {
 		list_del(&trans->list);
-- 
2.19.2

Re: [PATCH nf] netfilter: nf_tables: fix set double-free in abort path

[kfm]

Hi Florian,

On Thu,  7 Mar 2019 20:30:41 +0100
Florian Westphal <fw@strlen.de> wrote:

> The abort path can cause a double-free of an (anon) set.
> 
> Added-and-to-be-aborted rule looks like this:
> 
> udp dport { 137, 138 } drop
> 
> The to-be-aborted transaction list looks like this:
> newset
> newsetelem
> newsetelem
> rule
> 
> This gets walked in reverse order, so first pass disables
> the rule, the set elements, then the set.
> 
> After synchronize_rcu(), we then destroy those in same order:
> rule, set element, set element, newset.
> 
> Problem is that the (anon) set has already been bound to the rule,
> so the rule (lookup expression destructor) already frees the set,
> when then cause use-after-free when trying to delete the elements
> from this set, then try to free the set again when handling the
> newset expression.
> 
> To resolve this, check in first phase if the newset is bound already.
> If so, remove the newset transaction from the list, rule destructor
> will handle cleanup.
> 
> This is still causes the use-after-free on set element removal.
> To handle this, move all affected set elements to a extra list
> and process it first.
> 
> This forces strict 'destroy elements, then set' ordering.
> 
> Fixes: f6ac8585897684 ("netfilter: nf_tables: unbind set in rule from commit path")
> Bugzilla: https://bugzilla.netfilter.org/show_bug.cgi?id=1325
> Signed-off-by: Florian Westphal <fw@strlen.de>

<snip>

Thank you. I can confirm that, after applying this patch, I am able to check - and load - my entire ruleset without incident.

-- 
Kerin Millar <kfm@plushkava.net>

Re: [PATCH nf] netfilter: nf_tables: fix set double-free in abort path

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1512 bytes --]

Hi Florian,

Thanks for sending a patch for this.

On Thu, Mar 07, 2019 at 08:30:41PM +0100, Florian Westphal wrote:
> The abort path can cause a double-free of an (anon) set.
> 
> Added-and-to-be-aborted rule looks like this:
> 
> udp dport { 137, 138 } drop
> 
> The to-be-aborted transaction list looks like this:
> newset
> newsetelem
> newsetelem
> rule
> 
> This gets walked in reverse order, so first pass disables
> the rule, the set elements, then the set.
> 
> After synchronize_rcu(), we then destroy those in same order:
> rule, set element, set element, newset.
> 
> Problem is that the (anon) set has already been bound to the rule,
> so the rule (lookup expression destructor) already frees the set,
> when then cause use-after-free when trying to delete the elements
> from this set, then try to free the set again when handling the
> newset expression.
> 
> To resolve this, check in first phase if the newset is bound already.
> If so, remove the newset transaction from the list, rule destructor
> will handle cleanup.
> 
> This is still causes the use-after-free on set element removal.
> To handle this, move all affected set elements to a extra list
> and process it first.
>
> This forces strict 'destroy elements, then set' ordering.

So the problem is only the use-after-free from the NEWSETELEM abort
path, right?

Probably we can fix this problem with this patch too? Idea is to keep
this 'bound' internal flag, in that case, this turns the NEWSET and
NEWSETELEM abort path into noop.

[-- Attachment #2: 0001-netfilter-nf_tables-skip-new-element-transaction-abo.patch --]
[-- Type: text/x-diff, Size: 3528 bytes --]

From 4972e70f1bec0bd22f2cc5a937797dc438aa8298 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Fri, 8 Mar 2019 00:58:53 +0100
Subject: [PATCH] netfilter: nf_tables: skip new element transaction abort path
 if set is bound

Rule releases the bound set in first place from the abort path, this
causes the use-after-free on set element removal when undoing the new
element transactions. To handle this, skip new element transaction if
set is bound from the abort path.

Fixes: f6ac85858976 ("netfilter: nf_tables: unbind set in rule from commit path")
Bugzilla: https://bugzilla.netfilter.org/show_bug.cgi?id=1325
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/netfilter/nf_tables.h |  6 ++----
 net/netfilter/nf_tables_api.c     | 17 +++++++++++------
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index b4984bbbe157..3d58acf94dd2 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -416,7 +416,8 @@ struct nft_set {
 	unsigned char			*udata;
 	/* runtime data below here */
 	const struct nft_set_ops	*ops ____cacheline_aligned;
-	u16				flags:14,
+	u16				flags:13,
+					bound:1,
 					genmask:2;
 	u8				klen;
 	u8				dlen;
@@ -1329,15 +1330,12 @@ struct nft_trans_rule {
 struct nft_trans_set {
 	struct nft_set			*set;
 	u32				set_id;
-	bool				bound;
 };
 
 #define nft_trans_set(trans)	\
 	(((struct nft_trans_set *)trans->data)->set)
 #define nft_trans_set_id(trans)	\
 	(((struct nft_trans_set *)trans->data)->set_id)
-#define nft_trans_set_bound(trans)	\
-	(((struct nft_trans_set *)trans->data)->bound)
 
 struct nft_trans_chain {
 	bool				update;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 4893f248dfdc..74130ad10d1b 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -127,7 +127,7 @@ static void nft_set_trans_bind(const struct nft_ctx *ctx, struct nft_set *set)
 	list_for_each_entry_reverse(trans, &net->nft.commit_list, list) {
 		if (trans->msg_type == NFT_MSG_NEWSET &&
 		    nft_trans_set(trans) == set) {
-			nft_trans_set_bound(trans) = true;
+			set->bound = true;
 			break;
 		}
 	}
@@ -6617,10 +6617,13 @@ static void nf_tables_abort_release(struct nft_trans *trans)
 		nf_tables_rule_destroy(&trans->ctx, nft_trans_rule(trans));
 		break;
 	case NFT_MSG_NEWSET:
-		if (!nft_trans_set_bound(trans))
-			nft_set_destroy(nft_trans_set(trans));
+		if (nft_trans_set(trans)->bound)
+			break;
+		nft_set_destroy(nft_trans_set(trans));
 		break;
 	case NFT_MSG_NEWSETELEM:
+		if (nft_trans_elem_set(trans)->bound)
+			break;
 		nft_set_elem_destroy(nft_trans_elem_set(trans),
 				     nft_trans_elem(trans).priv, true);
 		break;
@@ -6691,8 +6694,9 @@ static int __nf_tables_abort(struct net *net)
 			break;
 		case NFT_MSG_NEWSET:
 			trans->ctx.table->use--;
-			if (!nft_trans_set_bound(trans))
-				list_del_rcu(&nft_trans_set(trans)->list);
+			if (nft_trans_set(trans)->bound)
+				break;
+			list_del_rcu(&nft_trans_set(trans)->list);
 			break;
 		case NFT_MSG_DELSET:
 			trans->ctx.table->use++;
@@ -6700,8 +6704,9 @@ static int __nf_tables_abort(struct net *net)
 			nft_trans_destroy(trans);
 			break;
 		case NFT_MSG_NEWSETELEM:
+			if (nft_trans_elem_set(trans)->bound)
+				break;
 			te = (struct nft_trans_elem *)trans->data;
-
 			te->set->ops->remove(net, te->set, &te->elem);
 			atomic_dec(&te->set->nelems);
 			break;
-- 
2.11.0

Re: [PATCH nf] netfilter: nf_tables: fix set double-free in abort path

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> So the problem is only the use-after-free from the NEWSETELEM abort
> path, right?

That and the double-freeing of the set.

> Probably we can fix this problem with this patch too? Idea is to keep
> this 'bound' internal flag, in that case, this turns the NEWSET and
> NEWSETELEM abort path into noop.

Good idea, but your patch still causes UAF.

> @@ -6617,10 +6617,13 @@ static void nf_tables_abort_release(struct nft_trans *trans)
>  		nf_tables_rule_destroy(&trans->ctx, nft_trans_rule(trans));
>  		break;
>  	case NFT_MSG_NEWSET:
> -		if (!nft_trans_set_bound(trans))
> -			nft_set_destroy(nft_trans_set(trans));
> +		if (nft_trans_set(trans)->bound)
> +			break;
> +		nft_set_destroy(nft_trans_set(trans));

Its not safe to use nft_trans_set() here anymore, as the set
has already been released by nf_tables_rule_destroy().

>  	case NFT_MSG_NEWSETELEM:
> +		if (nft_trans_elem_set(trans)->bound)
> +			break;

Same here.

Either the transactions themselves need to be yanked here, before
nf_tables_abort_release() is run, or the 'bound' flag needs to be
stored in the transaction struct.

I'd guess nft_trans_delete() is enough instead of plain break,
in case the set is bound we know expr destructor removes the set,
and if we avoid removal of the elements then those are removed too
when the set is destroyed.

This seems to make things work for me, on top of your patch:

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -6786,8 +6786,10 @@ static int __nf_tables_abort(struct net *net)
 			break;
 		case NFT_MSG_NEWSET:
 			trans->ctx.table->use--;
-			if (nft_trans_set(trans)->bound)
+			if (nft_trans_set(trans)->bound) {
+				nft_trans_destroy(trans);
 				break;
+			}
 			list_del_rcu(&nft_trans_set(trans)->list);
 			break;
 		case NFT_MSG_DELSET:
@@ -6796,8 +6798,10 @@ static int __nf_tables_abort(struct net *net)
 			nft_trans_destroy(trans);
 			break;
 		case NFT_MSG_NEWSETELEM:
-			if (nft_trans_elem_set(trans)->bound)
+			if (nft_trans_elem_set(trans)->bound) {
+				nft_trans_destroy(trans);
 				break;
+			}
 			te = (struct nft_trans_elem *)trans->data;
 			te->set->ops->remove(net, te->set, &te->elem);
 			atomic_dec(&te->set->nelems);

Re: [PATCH nf] netfilter: nf_tables: fix set double-free in abort path

[Pablo Neira Ayuso]

On Fri, Mar 08, 2019 at 11:22:17AM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > So the problem is only the use-after-free from the NEWSETELEM abort
> > path, right?
> 
> That and the double-freeing of the set.
> 
> > Probably we can fix this problem with this patch too? Idea is to keep
> > this 'bound' internal flag, in that case, this turns the NEWSET and
> > NEWSETELEM abort path into noop.
> 
> Good idea, but your patch still causes UAF.
> 
> > @@ -6617,10 +6617,13 @@ static void nf_tables_abort_release(struct nft_trans *trans)
> >  		nf_tables_rule_destroy(&trans->ctx, nft_trans_rule(trans));
> >  		break;
> >  	case NFT_MSG_NEWSET:
> > -		if (!nft_trans_set_bound(trans))
> > -			nft_set_destroy(nft_trans_set(trans));
> > +		if (nft_trans_set(trans)->bound)
> > +			break;
> > +		nft_set_destroy(nft_trans_set(trans));
> 
> Its not safe to use nft_trans_set() here anymore, as the set
> has already been released by nf_tables_rule_destroy().

Oh, right.

> >  	case NFT_MSG_NEWSETELEM:
> > +		if (nft_trans_elem_set(trans)->bound)
> > +			break;
> 
> Same here.
> 
> Either the transactions themselves need to be yanked here, before
> nf_tables_abort_release() is run, or the 'bound' flag needs to be
> stored in the transaction struct.
>
> I'd guess nft_trans_delete() is enough instead of plain break,
> in case the set is bound we know expr destructor removes the set,
> and if we avoid removal of the elements then those are removed too
> when the set is destroyed.
> 
> This seems to make things work for me, on top of your patch:

That looks good, I'm going to collapse it to my patch and resend.

Thanks.

> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -6786,8 +6786,10 @@ static int __nf_tables_abort(struct net *net)
>  			break;
>  		case NFT_MSG_NEWSET:
>  			trans->ctx.table->use--;
> -			if (nft_trans_set(trans)->bound)
> +			if (nft_trans_set(trans)->bound) {
> +				nft_trans_destroy(trans);
>  				break;
> +			}
>  			list_del_rcu(&nft_trans_set(trans)->list);
>  			break;
>  		case NFT_MSG_DELSET:
> @@ -6796,8 +6798,10 @@ static int __nf_tables_abort(struct net *net)
>  			nft_trans_destroy(trans);
>  			break;
>  		case NFT_MSG_NEWSETELEM:
> -			if (nft_trans_elem_set(trans)->bound)
> +			if (nft_trans_elem_set(trans)->bound) {
> +				nft_trans_destroy(trans);
>  				break;
> +			}
>  			te = (struct nft_trans_elem *)trans->data;
>  			te->set->ops->remove(net, te->set, &te->elem);
>  			atomic_dec(&te->set->nelems);