nft: Delete builtin chains compatibly

Attempting to delete all chains if --delete-chain is called without
argument has unwanted side-effects especially legacy iptables users are
not aware of and won't expect:

* Non-default policies are ignored, a previously dropping firewall may
  start accepting traffic.

* The kernel refuses to remove non-empty chains, causing program abort
  even if no user-defined chain exists.

Fix this by requiring a rule cache in that situation and make builtin
chain deletion depend on its policy and number of rules. Since this may
change concurrently, check again when having to refresh the transaction.

Also, hide builtin chains from verbose output - their creation is
implicit, so treat their removal as implicit, too.

When deleting a specific chain, do not allow to skip the job though.
Otherwise deleting a builtin chain which is still in use will succeed
although not executed.

Fixes: 61e85e3192dea ("iptables-nft: allow removal of empty builtin chains")
Signed-off-by: Phil Sutter <phil@nwl.cc>

3 files changed, 66 insertions, 9 deletions

diff --git a/iptables/nft-cmd.c b/iptables/nft-cmd.c
index 2d874bd4..fcd01bd0 100644
--- a/iptables/nft-cmd.c
+++ b/iptables/nft-cmd.c
@@ -220,7 +220,7 @@ int nft_cmd_chain_del(struct nft_handle *h, const char *chain,
 	/* This triggers nft_bridge_chain_postprocess() when fetching the
 	 * rule cache.
 	 */
-	if (h->family == NFPROTO_BRIDGE)
+	if (h->family == NFPROTO_BRIDGE || !chain)
 		nft_cache_level_set(h, NFT_CL_RULES, cmd);
 	else
 		nft_cache_level_set(h, NFT_CL_CHAINS, cmd);
diff --git a/iptables/nft.c b/iptables/nft.c
index 38106147..dc1f5160 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1838,26 +1838,46 @@ int nft_chain_restore(struct nft_handle *h, const char *chain, const char *table
 
 struct chain_del_data {
 	struct nft_handle	*handle;
+	const char		*chain;
 	bool			verbose;
 };
 
+static bool nft_may_delete_chain(struct nftnl_chain *c)
+{
+	if (nftnl_chain_is_set(c, NFTNL_CHAIN_POLICY) &&
+	    nftnl_chain_get_u32(c, NFTNL_CHAIN_POLICY) != NF_ACCEPT)
+		return false;
+
+	return nftnl_rule_lookup_byindex(c, 0) == NULL;
+}
+
 static int __nft_chain_del(struct nft_chain *nc, void *data)
 {
 	struct chain_del_data *d = data;
 	struct nftnl_chain *c = nc->nftnl;
 	struct nft_handle *h = d->handle;
+	bool builtin = nft_chain_builtin(c);
+	struct obj_update *obj;
+	int ret = 0;
 
-	if (d->verbose)
+	if (d->verbose && !builtin)
 		fprintf(stdout, "Deleting chain `%s'\n",
 			nftnl_chain_get_str(c, NFTNL_CHAIN_NAME));
 
 
 	/* XXX This triggers a fast lookup from the kernel. */
 	nftnl_chain_unset(c, NFTNL_CHAIN_HANDLE);
-	if (!batch_chain_add(h, NFT_COMPAT_CHAIN_DEL, c))
+	obj = batch_chain_add(h, NFT_COMPAT_CHAIN_DEL, c);
+	if (!obj)
 		return -1;
 
-	if (nft_chain_builtin(c)) {
+	if (builtin) {
+		obj->skip = !nft_may_delete_chain(c);
+		if (obj->skip && d->chain) {
+			/* complain if explicitly requested */
+			errno = EBUSY;
+			ret = -1;
+		}
 		*nc->base_slot = NULL;
 	}
 
@@ -1866,14 +1886,15 @@ static int __nft_chain_del(struct nft_chain *nc, void *data)
 
 	nft_chain_list_del(nc);
 	nft_chain_free(nc);
-	return 0;
+	return ret;
 }
 
 int nft_chain_del(struct nft_handle *h, const char *chain,
-		       const char *table, bool verbose)
+		  const char *table, bool verbose)
 {
 	struct chain_del_data d = {
 		.handle = h,
+		.chain = chain,
 		.verbose = verbose,
 	};
 	struct nft_chain *c;
@@ -1889,8 +1910,6 @@ int nft_chain_del(struct nft_handle *h, const char *chain,
 		}
 
 		ret = __nft_chain_del(c, &d);
-		if (ret == -2)
-			errno = EINVAL;
 		goto out;
 	}
 
@@ -2744,10 +2763,14 @@ static void nft_refresh_transaction(struct nft_handle *h)
 
 			n->skip = !nft_chain_find(h, tablename, chainname);
 			break;
+		case NFT_COMPAT_CHAIN_DEL:
+			if (!nftnl_chain_get(n->chain, NFTNL_CHAIN_HOOKNUM))
+				break;
+			n->skip = !nft_may_delete_chain(n->chain);
+			break;
 		case NFT_COMPAT_TABLE_ADD:
 		case NFT_COMPAT_CHAIN_ADD:
 		case NFT_COMPAT_CHAIN_ZERO:
-		case NFT_COMPAT_CHAIN_DEL:
 		case NFT_COMPAT_CHAIN_USER_FLUSH:
 		case NFT_COMPAT_CHAIN_UPDATE:
 		case NFT_COMPAT_CHAIN_RENAME:
diff --git a/iptables/tests/shell/testcases/chain/0005base-delete_0 b/iptables/tests/shell/testcases/chain/0005base-delete_0
new file mode 100755
index 00000000..033a2819
--- /dev/null
+++ b/iptables/tests/shell/testcases/chain/0005base-delete_0
@@ -0,0 +1,34 @@
+#!/bin/bash -x
+
+$XT_MULTI iptables -N foo || exit 1
+$XT_MULTI iptables -P FORWARD DROP || exit 1
+$XT_MULTI iptables -X || exit 1
+$XT_MULTI iptables -X foo && exit 1
+
+# indefinite -X fails if a non-empty user-defined chain exists
+$XT_MULTI iptables -N foo
+$XT_MULTI iptables -N bar
+$XT_MULTI iptables -A bar -j ACCEPT
+$XT_MULTI iptables -X && exit 1
+$XT_MULTI iptables -D bar -j ACCEPT
+$XT_MULTI iptables -X || exit 1
+
+# make sure OUTPUT chain is created by iptables-nft
+$XT_MULTI iptables -A OUTPUT -j ACCEPT || exit 1
+$XT_MULTI iptables -D OUTPUT -j ACCEPT || exit 1
+
+case $XT_MULTI in
+*xtables-nft-multi)
+	# must not delete chain FORWARD, its policy is not ACCEPT
+	$XT_MULTI iptables -X FORWARD && exit 1
+	nft list chain ip filter FORWARD || exit 1
+	# this should evict chain OUTPUT
+	$XT_MULTI iptables -X OUTPUT || exit 1
+	nft list chain ip filter OUTPUT && exit 1
+	;;
+*)
+	$XT_MULTI iptables -X FORWARD && exit 1
+	$XT_MULTI iptables -X OUTPUT && exit 1
+	;;
+esac
+exit 0