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>

diff --git a/iptables/nft-cmd.c b/iptables/nft-cmd.c
index 2d874bd41e8f64aee388c7b1182f6c9b923e2cdc..fcd01bd02831cd61097af0a7e0c77c45bf400f85 100644 (file)
--- 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 381061473047f33745b8e352d06a4f51d2dbe660..dc1f5160eb98347b0b0f32107c8148e0c44a686e 100644 (file)
--- 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 (executable)
index 0000000..033a281
--- /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