[iptables PATCH] iptables-nft: fix basechain policy configuration

[iptables PATCH] iptables-nft: fix basechain policy configuration

[Arturo Borrero Gonzalez]

From: Pablo Neira Ayuso <pablo@netfilter.org>

Previous to this patch, the basechain policy could not be properly configured if it wasn't
explictly set when loading the ruleset, leading to iptables-nft-restore (and ip6tables-nft-restore)
trying to send an invalid ruleset to the kernel.

CC: Phil Sutter <phil@nwl.cc>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Arturo Borrero Gonzalez <arturo@netfilter.org>
---
 iptables/nft.c                                     |    6 +++++-
 .../testcases/nft-only/0008-basechain-policy_0     |   21 ++++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)
 create mode 100755 iptables/tests/shell/testcases/nft-only/0008-basechain-policy_0

diff --git a/iptables/nft.c b/iptables/nft.c
index 27bb98d1..f29fe5b4 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -678,7 +678,9 @@ nft_chain_builtin_alloc(const struct builtin_table *table,
 	nftnl_chain_set_str(c, NFTNL_CHAIN_NAME, chain->name);
 	nftnl_chain_set_u32(c, NFTNL_CHAIN_HOOKNUM, chain->hook);
 	nftnl_chain_set_u32(c, NFTNL_CHAIN_PRIO, chain->prio);
-	nftnl_chain_set_u32(c, NFTNL_CHAIN_POLICY, policy);
+	if (policy >= 0)
+		nftnl_chain_set_u32(c, NFTNL_CHAIN_POLICY, policy);
+
 	nftnl_chain_set_str(c, NFTNL_CHAIN_TYPE, chain->type);
 
 	return c;
@@ -911,6 +913,8 @@ int nft_chain_set(struct nft_handle *h, const char *table,
 		c = nft_chain_new(h, table, chain, NF_DROP, counters);
 	else if (strcmp(policy, "ACCEPT") == 0)
 		c = nft_chain_new(h, table, chain, NF_ACCEPT, counters);
+	else if (strcmp(policy, "-") == 0)
+		c = nft_chain_new(h, table, chain, -1, counters);
 	else
 		errno = EINVAL;
 
diff --git a/iptables/tests/shell/testcases/nft-only/0008-basechain-policy_0 b/iptables/tests/shell/testcases/nft-only/0008-basechain-policy_0
new file mode 100755
index 00000000..61e408e8
--- /dev/null
+++ b/iptables/tests/shell/testcases/nft-only/0008-basechain-policy_0
@@ -0,0 +1,21 @@
+#!/bin/bash
+
+[[ $XT_MULTI == *xtables-nft-multi ]] || { echo "skip $XT_MULTI"; exit 0; }
+set -e
+
+# make sure iptables-nft-restore can correctly handle basechain policies when they aren't set
+
+$XT_MULTI iptables-restore <<EOF
+*raw
+:OUTPUT - [0:0]
+:PREROUTING - [0:0]
+:neutron-linuxbri-OUTPUT - [0:0]
+:neutron-linuxbri-PREROUTING - [0:0]
+-I OUTPUT 1 -j neutron-linuxbri-OUTPUT
+-I PREROUTING 1 -j neutron-linuxbri-PREROUTING
+-I neutron-linuxbri-PREROUTING 1 -m physdev --physdev-in brq7425e328-56 -j CT --zone 4097
+-I neutron-linuxbri-PREROUTING 2 -i brq7425e328-56 -j CT --zone 4097
+-I neutron-linuxbri-PREROUTING 3 -m physdev --physdev-in tap7f101a28-1d -j CT --zone 4097
+
+COMMIT
+EOF

Re: [iptables PATCH] iptables-nft: fix basechain policy configuration

[Phil Sutter]

Hi,

On Fri, Oct 02, 2020 at 01:44:36PM +0200, Arturo Borrero Gonzalez wrote:
> Previous to this patch, the basechain policy could not be properly configured if it wasn't
> explictly set when loading the ruleset, leading to iptables-nft-restore (and ip6tables-nft-restore)
> trying to send an invalid ruleset to the kernel.

I fear this is not sufficient: iptables-legacy-restore leaves the
previous chain policy in place if '-' is given in dump file. Please try
this snippet from a testcase I wrote:

$XT_MULTI iptables -P FORWARD DROP

diff -u -Z <($XT_MULTI iptables-save | grep '^:FORWARD') \
           <(echo ":FORWARD DROP [0:0]")

$XT_MULTI iptables-restore -c <<< "$TEST_RULESET"
diff -u -Z <($XT_MULTI iptables-save | grep '^:FORWARD') \
           <(echo ":FORWARD DROP [0:0]")

Thanks, Phil

Re: [iptables PATCH] iptables-nft: fix basechain policy configuration

[Pablo Neira Ayuso]

On Fri, Oct 02, 2020 at 02:07:32PM +0200, Phil Sutter wrote:
> Hi,
> 
> On Fri, Oct 02, 2020 at 01:44:36PM +0200, Arturo Borrero Gonzalez wrote:
> > Previous to this patch, the basechain policy could not be properly configured if it wasn't
> > explictly set when loading the ruleset, leading to iptables-nft-restore (and ip6tables-nft-restore)
> > trying to send an invalid ruleset to the kernel.
> 
> I fear this is not sufficient: iptables-legacy-restore leaves the
> previous chain policy in place if '-' is given in dump file. Please try
> this snippet from a testcase I wrote:
> 
> $XT_MULTI iptables -P FORWARD DROP
> 
> diff -u -Z <($XT_MULTI iptables-save | grep '^:FORWARD') \
>            <(echo ":FORWARD DROP [0:0]")
> 
> $XT_MULTI iptables-restore -c <<< "$TEST_RULESET"
> diff -u -Z <($XT_MULTI iptables-save | grep '^:FORWARD') \
>            <(echo ":FORWARD DROP [0:0]")

Hm, this is how it works in this patch right?

I mean, if '-' is given, chain policy attribute in the netlink message
is not set, and the kernel sets chain policy to
NFT_CHAIN_POLICY_UNSET.

Or am I missing anything?

Re: [iptables PATCH] iptables-nft: fix basechain policy configuration

[Phil Sutter]

On Fri, Oct 02, 2020 at 02:15:58PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Oct 02, 2020 at 02:07:32PM +0200, Phil Sutter wrote:
> > Hi,
> > 
> > On Fri, Oct 02, 2020 at 01:44:36PM +0200, Arturo Borrero Gonzalez wrote:
> > > Previous to this patch, the basechain policy could not be properly configured if it wasn't
> > > explictly set when loading the ruleset, leading to iptables-nft-restore (and ip6tables-nft-restore)
> > > trying to send an invalid ruleset to the kernel.
> > 
> > I fear this is not sufficient: iptables-legacy-restore leaves the
> > previous chain policy in place if '-' is given in dump file. Please try
> > this snippet from a testcase I wrote:
> > 
> > $XT_MULTI iptables -P FORWARD DROP
> > 
> > diff -u -Z <($XT_MULTI iptables-save | grep '^:FORWARD') \
> >            <(echo ":FORWARD DROP [0:0]")
> > 
> > $XT_MULTI iptables-restore -c <<< "$TEST_RULESET"
> > diff -u -Z <($XT_MULTI iptables-save | grep '^:FORWARD') \
> >            <(echo ":FORWARD DROP [0:0]")
> 
> Hm, this is how it works in this patch right?
> 
> I mean, if '-' is given, chain policy attribute in the netlink message
> is not set, and the kernel sets chain policy to
> NFT_CHAIN_POLICY_UNSET.
> 
> Or am I missing anything?

This is *flushing* iptables-restore. We're dropping the chain first and
then reinstall it.

Another quirk is that iptables-legacy-restore ignores the counters if
policy is '-' even if --counters flag was given. (:

Cheers, Phil

Re: [iptables PATCH] iptables-nft: fix basechain policy configuration

[Pablo Neira Ayuso]

On Fri, Oct 02, 2020 at 02:28:52PM +0200, Phil Sutter wrote:
> On Fri, Oct 02, 2020 at 02:15:58PM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Oct 02, 2020 at 02:07:32PM +0200, Phil Sutter wrote:
> > > Hi,
> > > 
> > > On Fri, Oct 02, 2020 at 01:44:36PM +0200, Arturo Borrero Gonzalez wrote:
> > > > Previous to this patch, the basechain policy could not be properly configured if it wasn't
> > > > explictly set when loading the ruleset, leading to iptables-nft-restore (and ip6tables-nft-restore)
> > > > trying to send an invalid ruleset to the kernel.
> > > 
> > > I fear this is not sufficient: iptables-legacy-restore leaves the
> > > previous chain policy in place if '-' is given in dump file. Please try
> > > this snippet from a testcase I wrote:
> > > 
> > > $XT_MULTI iptables -P FORWARD DROP
> > > 
> > > diff -u -Z <($XT_MULTI iptables-save | grep '^:FORWARD') \
> > >            <(echo ":FORWARD DROP [0:0]")
> > > 
> > > $XT_MULTI iptables-restore -c <<< "$TEST_RULESET"
> > > diff -u -Z <($XT_MULTI iptables-save | grep '^:FORWARD') \
> > >            <(echo ":FORWARD DROP [0:0]")
> > 
> > Hm, this is how it works in this patch right?
> > 
> > I mean, if '-' is given, chain policy attribute in the netlink message
> > is not set, and the kernel sets chain policy to
> > NFT_CHAIN_POLICY_UNSET.
> > 
> > Or am I missing anything?
> 
> This is *flushing* iptables-restore. We're dropping the chain first and
> then reinstall it.

OK, so this fix only works with --noflush.

If --noflush is not specified, then it should be possible to extend
the cache to dump the chains, get the existing policy and use it.
There is now a phase to evaluate the cache requirements, so you can
fetch this. Then, from the netlink phase, look up for the existing
policy in the cache and use it.

> Another quirk is that iptables-legacy-restore ignores the counters if
> policy is '-' even if --counters flag was given. (:

OK, so this needs two more fixed on top of this one.

Re: [iptables PATCH] iptables-nft: fix basechain policy configuration

[Phil Sutter]

On Fri, Oct 02, 2020 at 02:47:41PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Oct 02, 2020 at 02:28:52PM +0200, Phil Sutter wrote:
> > On Fri, Oct 02, 2020 at 02:15:58PM +0200, Pablo Neira Ayuso wrote:
> > > On Fri, Oct 02, 2020 at 02:07:32PM +0200, Phil Sutter wrote:
> > > > Hi,
> > > > 
> > > > On Fri, Oct 02, 2020 at 01:44:36PM +0200, Arturo Borrero Gonzalez wrote:
> > > > > Previous to this patch, the basechain policy could not be properly configured if it wasn't
> > > > > explictly set when loading the ruleset, leading to iptables-nft-restore (and ip6tables-nft-restore)
> > > > > trying to send an invalid ruleset to the kernel.
> > > > 
> > > > I fear this is not sufficient: iptables-legacy-restore leaves the
> > > > previous chain policy in place if '-' is given in dump file. Please try
> > > > this snippet from a testcase I wrote:
> > > > 
> > > > $XT_MULTI iptables -P FORWARD DROP
> > > > 
> > > > diff -u -Z <($XT_MULTI iptables-save | grep '^:FORWARD') \
> > > >            <(echo ":FORWARD DROP [0:0]")
> > > > 
> > > > $XT_MULTI iptables-restore -c <<< "$TEST_RULESET"
> > > > diff -u -Z <($XT_MULTI iptables-save | grep '^:FORWARD') \
> > > >            <(echo ":FORWARD DROP [0:0]")
> > > 
> > > Hm, this is how it works in this patch right?
> > > 
> > > I mean, if '-' is given, chain policy attribute in the netlink message
> > > is not set, and the kernel sets chain policy to
> > > NFT_CHAIN_POLICY_UNSET.
> > > 
> > > Or am I missing anything?
> > 
> > This is *flushing* iptables-restore. We're dropping the chain first and
> > then reinstall it.
> 
> OK, so this fix only works with --noflush.
> 
> If --noflush is not specified, then it should be possible to extend
> the cache to dump the chains, get the existing policy and use it.

nft_cmd_chain_restore() already sets NFT_CL_CHAINS.

> There is now a phase to evaluate the cache requirements, so you can
> fetch this. Then, from the netlink phase, look up for the existing
> policy in the cache and use it.

After the cache is fetched, nft_table_flush() runs before
nft_chain_restore() does.

> > Another quirk is that iptables-legacy-restore ignores the counters if
> > policy is '-' even if --counters flag was given. (:
> 
> OK, so this needs two more fixed on top of this one.

In Arturo's mail, he doesn't use --noflush. Not sure if this is just his
reproducer or if OpenStack doesn't use --noflush, either. If so, your
fix won't help with his problem.

Arturo, does fixing --noflush suffice for your case? If so, we could
delay the "--flush" case "for later". ;)

Cheers, Phil

Re: [iptables PATCH] iptables-nft: fix basechain policy configuration

[Pablo Neira Ayuso]

On Fri, Oct 02, 2020 at 03:31:27PM +0200, Phil Sutter wrote:
> On Fri, Oct 02, 2020 at 02:47:41PM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Oct 02, 2020 at 02:28:52PM +0200, Phil Sutter wrote:
> > > On Fri, Oct 02, 2020 at 02:15:58PM +0200, Pablo Neira Ayuso wrote:
> > > > On Fri, Oct 02, 2020 at 02:07:32PM +0200, Phil Sutter wrote:
> > > > > Hi,
> > > > > 
> > > > > On Fri, Oct 02, 2020 at 01:44:36PM +0200, Arturo Borrero Gonzalez wrote:
> > > > > > Previous to this patch, the basechain policy could not be properly configured if it wasn't
> > > > > > explictly set when loading the ruleset, leading to iptables-nft-restore (and ip6tables-nft-restore)
> > > > > > trying to send an invalid ruleset to the kernel.
> > > > > 
> > > > > I fear this is not sufficient: iptables-legacy-restore leaves the
> > > > > previous chain policy in place if '-' is given in dump file. Please try
> > > > > this snippet from a testcase I wrote:
> > > > > 
> > > > > $XT_MULTI iptables -P FORWARD DROP
> > > > > 
> > > > > diff -u -Z <($XT_MULTI iptables-save | grep '^:FORWARD') \
> > > > >            <(echo ":FORWARD DROP [0:0]")
> > > > > 
> > > > > $XT_MULTI iptables-restore -c <<< "$TEST_RULESET"
> > > > > diff -u -Z <($XT_MULTI iptables-save | grep '^:FORWARD') \
> > > > >            <(echo ":FORWARD DROP [0:0]")
> > > > 
> > > > Hm, this is how it works in this patch right?
> > > > 
> > > > I mean, if '-' is given, chain policy attribute in the netlink message
> > > > is not set, and the kernel sets chain policy to
> > > > NFT_CHAIN_POLICY_UNSET.
> > > > 
> > > > Or am I missing anything?
> > > 
> > > This is *flushing* iptables-restore. We're dropping the chain first and
> > > then reinstall it.
> > 
> > OK, so this fix only works with --noflush.
> > 
> > If --noflush is not specified, then it should be possible to extend
> > the cache to dump the chains, get the existing policy and use it.
> 
> nft_cmd_chain_restore() already sets NFT_CL_CHAINS.
> 
> > There is now a phase to evaluate the cache requirements, so you can
> > fetch this. Then, from the netlink phase, look up for the existing
> > policy in the cache and use it.
> 
> After the cache is fetched, nft_table_flush() runs before
> nft_chain_restore() does.
> 
> > > Another quirk is that iptables-legacy-restore ignores the counters if
> > > policy is '-' even if --counters flag was given. (:
> > 
> > OK, so this needs two more fixed on top of this one.
> 
> In Arturo's mail, he doesn't use --noflush. Not sure if this is just his
> reproducer or if OpenStack doesn't use --noflush, either. If so, your
> fix won't help with his problem.
>
> Arturo, does fixing --noflush suffice for your case? If so, we could
> delay the "--flush" case "for later". ;)

I would expect OpenStack uses --noflush. I'm not sure I can find a
use-case for the "--flush" case with policy '-'.

Let's wait for Arturo's feedback.

Re: [iptables PATCH] iptables-nft: fix basechain policy configuration

[Pablo Neira Ayuso]

On Fri, Oct 02, 2020 at 01:44:36PM +0200, Arturo Borrero Gonzalez wrote:
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> 
> Previous to this patch, the basechain policy could not be properly configured if it wasn't
> explictly set when loading the ruleset, leading to iptables-nft-restore (and ip6tables-nft-restore)
> trying to send an invalid ruleset to the kernel.

I have applied this with some amendments to the test file to cover
the --noflush case. I think this is a real problem there, where you
can combine to apply incremental updates to the ruleset.

For the --flush case, I still have doubts how to use this feature, not
sure it is worth the effort to actually fix it.

We can revisit later, you can rewrite this later Phil.

Re: [iptables PATCH] iptables-nft: fix basechain policy configuration

[Phil Sutter]

Hi Pablo,

On Thu, Oct 08, 2020 at 07:31:56PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Oct 02, 2020 at 01:44:36PM +0200, Arturo Borrero Gonzalez wrote:
> > From: Pablo Neira Ayuso <pablo@netfilter.org>
> > 
> > Previous to this patch, the basechain policy could not be properly configured if it wasn't
> > explictly set when loading the ruleset, leading to iptables-nft-restore (and ip6tables-nft-restore)
> > trying to send an invalid ruleset to the kernel.
> 
> I have applied this with some amendments to the test file to cover
> the --noflush case. I think this is a real problem there, where you
> can combine to apply incremental updates to the ruleset.

Yes, at least I can imagine people relying upon this behaviour.

> For the --flush case, I still have doubts how to use this feature, not
> sure it is worth the effort to actually fix it.

I even find it unintuitive as it retains state despite flushing. But
that is a significant divergence between legacy and nft:

| # iptables -P FORWARD DROP
| # iptables-restore <<EOF
| *filter
| COMMIT
| EOF
| # iptables-save

With legacy, the output is:

| *filter
| :INPUT ACCEPT [0:0]
| :FORWARD DROP [0:0]
| :OUTPUT ACCEPT [0:0]
| COMMIT

With nft, there's no output at all. What do you think, should we fix
that? If so, which side?

> We can revisit later, you can rewrite this later Phil.

Sure, no problem.

Thanks, Phil

Re: [iptables PATCH] iptables-nft: fix basechain policy configuration

[Pablo Neira Ayuso]

Cc'ing Jozsef.

On Fri, Oct 09, 2020 at 10:29:53AM +0200, Phil Sutter wrote:
> Hi Pablo,
> 
> On Thu, Oct 08, 2020 at 07:31:56PM +0200, Pablo Neira Ayuso wrote:
> > On Fri, Oct 02, 2020 at 01:44:36PM +0200, Arturo Borrero Gonzalez wrote:
> > > From: Pablo Neira Ayuso <pablo@netfilter.org>
> > > 
> > > Previous to this patch, the basechain policy could not be properly configured if it wasn't
> > > explictly set when loading the ruleset, leading to iptables-nft-restore (and ip6tables-nft-restore)
> > > trying to send an invalid ruleset to the kernel.
> > 
> > I have applied this with some amendments to the test file to cover
> > the --noflush case. I think this is a real problem there, where you
> > can combine to apply incremental updates to the ruleset.
> 
> Yes, at least I can imagine people relying upon this behaviour.
> 
> > For the --flush case, I still have doubts how to use this feature, not
> > sure it is worth the effort to actually fix it.
> 
> I even find it unintuitive as it retains state despite flushing.

So am I.

> But that is a significant divergence between legacy and nft:
> 
> | # iptables -P FORWARD DROP
> | # iptables-restore <<EOF
> | *filter
> | COMMIT
> | EOF
> | # iptables-save
>
> With legacy, the output is:
> 
> | *filter
> | :INPUT ACCEPT [0:0]
> | :FORWARD DROP [0:0]
> | :OUTPUT ACCEPT [0:0]
> | COMMIT
>
> With nft, there's no output at all. What do you think, should we fix
> that? If so, which side?

Looks like a variant of the same usecase.

So if basechains are not specified, then basechains policies are left
as is, but ruleset is flushed.

Semantics for this are: Flush out _everything_ (existing rules and
non-chains) but only leave existing basechain policies in place as is.

I wonder if this is intentional or a side effect of the --noflush support.

I'm Cc'ing Jozsef, maybe he remembers. Because you're reaching my
boundaries on the netfilter history for this one :-)

> > We can revisit later, you can rewrite this later Phil.
> 
> Sure, no problem.

Thanks.

Re: [iptables PATCH] iptables-nft: fix basechain policy configuration

[Phil Sutter]

Hi,

On Fri, Oct 09, 2020 at 10:50:39AM +0200, Pablo Neira Ayuso wrote:
[...]
> Semantics for this are: Flush out _everything_ (existing rules and
> non-chains) but only leave existing basechain policies in place as is.
> 
> I wonder if this is intentional or a side effect of the --noflush support.
> 
> I'm Cc'ing Jozsef, maybe he remembers. Because you're reaching my
> boundaries on the netfilter history for this one :-)

FWIW, I searched git history and can confirm the given behaviour is like
that at least since Dec 2000 and commit ae1ff9f96a803 ("make
iptables-restore and iptables-save work again!")!

iptables-legacy-restore does:

| if (noflush == 0) {
|         DEBUGP("Cleaning all chains of table '%s'\n",
|                 table);
|         cb->for_each_chain(cb->flush_entries, verbose, 1,
|                         handle);
|
|         DEBUGP("Deleting all user-defined chains "
|                "of table '%s'\n", table);
|         cb->for_each_chain(cb->delete_chain, verbose, 0,
|                         handle);
| }

(Third parameter to for_each_chain decides whether builtins are included or
not.)

I guess fundamentally this is due to legacy design which keeps builtin
chains in place at all times. We could copy that in iptables-nft, but I
like the current design where we just delete the whole table and start
from scratch.

Florian made a related remark a while ago about flushing chains with
DROP policy: He claims it is almost always a mistake and we should reset
the policy to ACCEPT in order to avoid people from locking themselves
out. I second that idea, but am not sure if such a change is tolerable
at all.

Cheers, Phil

Re: [iptables PATCH] iptables-nft: fix basechain policy configuration

[Reindl Harald]

Am 09.10.20 um 11:37 schrieb Phil Sutter:
> I guess fundamentally this is due to legacy design which keeps builtin
> chains in place at all times. We could copy that in iptables-nft, but I
> like the current design where we just delete the whole table and start
> from scratch.
> 
> Florian made a related remark a while ago about flushing chains with
> DROP policy: He claims it is almost always a mistake and we should reset
> the policy to ACCEPT in order to avoid people from locking themselves
> out. I second that idea, but am not sure if such a change is tolerable
> at all.
bad idea!

nothing is locking you out just because of a short drop phase, at least 
not over the past 12 years, that's what tcp retransmits are for

when you once accept i have someone which should never have been 
accepted in the conntracking - sorry - but when i say drop i literally 
mean drop at any point in time

Re: [iptables PATCH] iptables-nft: fix basechain policy configuration

[Phil Sutter]

Hi Harald,

On Fri, Oct 09, 2020 at 12:07:57PM +0200, Reindl Harald wrote:
> Am 09.10.20 um 11:37 schrieb Phil Sutter:
> > I guess fundamentally this is due to legacy design which keeps builtin
> > chains in place at all times. We could copy that in iptables-nft, but I
> > like the current design where we just delete the whole table and start
> > from scratch.
> > 
> > Florian made a related remark a while ago about flushing chains with
> > DROP policy: He claims it is almost always a mistake and we should reset
> > the policy to ACCEPT in order to avoid people from locking themselves
> > out. I second that idea, but am not sure if such a change is tolerable
> > at all.
> bad idea!

Why?

> nothing is locking you out just because of a short drop phase, at least 
> not over the past 12 years, that's what tcp retransmits are for

What I had in mind was 'ssh somehost iptables -F INPUT'.

> when you once accept i have someone which should never have been 
> accepted in the conntracking - sorry - but when i say drop i literally 
> mean drop at any point in time

My English language parser failed this part, sorry. :)

Cheers, Phil

Re: [iptables PATCH] iptables-nft: fix basechain policy configuration

[Reindl Harald]

Am 09.10.20 um 12:14 schrieb Phil Sutter:
> Hi Harald,
> 
> On Fri, Oct 09, 2020 at 12:07:57PM +0200, Reindl Harald wrote:
>> Am 09.10.20 um 11:37 schrieb Phil Sutter:
>>> I guess fundamentally this is due to legacy design which keeps builtin
>>> chains in place at all times. We could copy that in iptables-nft, but I
>>> like the current design where we just delete the whole table and start
>>> from scratch.
>>>
>>> Florian made a related remark a while ago about flushing chains with
>>> DROP policy: He claims it is almost always a mistake and we should reset
>>> the policy to ACCEPT in order to avoid people from locking themselves
>>> out. I second that idea, but am not sure if such a change is tolerable
>>> at all.
>> bad idea!
> 
> Why?

because in doubt you always to be packets dropped

>> nothing is locking you out just because of a short drop phase, at least
>> not over the past 12 years, that's what tcp retransmits are for
> 
> What I had in mind was 'ssh somehost iptables -F INPUT'.

when someone shoots you with so much passion in the foot let him

nobody does that without a script which will simply come back du 
tcp-retransmit and it's prettfy fine that after a flush DROP is the 
default policy

a default of ACCEPT for INPUT is crazy

>> when you once accept i have someone which should never have been
>> accepted in the conntracking - sorry - but when i say drop i literally
>> mean drop at any point in time
> 
> My English language parser failed this part, sorry. :)

accepting one unwanted packet leads in "ctstate RELATED,ESTABLISHED" 
triggering later no matter if you fix the policy afterwards

Re: [iptables PATCH] iptables-nft: fix basechain policy configuration

[Jozsef Kadlecsik]

Hi,

On Fri, 9 Oct 2020, Phil Sutter wrote:

> On Fri, Oct 09, 2020 at 10:50:39AM +0200, Pablo Neira Ayuso wrote:
> [...]
> > Semantics for this are: Flush out _everything_ (existing rules and
> > non-chains) but only leave existing basechain policies in place as is.
> > 
> > I wonder if this is intentional or a side effect of the --noflush support.
> > 
> > I'm Cc'ing Jozsef, maybe he remembers. Because you're reaching my
> > boundaries on the netfilter history for this one :-)

Phil could dig it out from git history :-):

> FWIW, I searched git history and can confirm the given behaviour is like
> that at least since Dec 2000 and commit ae1ff9f96a803 ("make
> iptables-restore and iptables-save work again!")!
> 
> iptables-legacy-restore does:
> 
> | if (noflush == 0) {
> |         DEBUGP("Cleaning all chains of table '%s'\n",
> |                 table);
> |         cb->for_each_chain(cb->flush_entries, verbose, 1,
> |                         handle);
> |
> |         DEBUGP("Deleting all user-defined chains "
> |                "of table '%s'\n", table);
> |         cb->for_each_chain(cb->delete_chain, verbose, 0,
> |                         handle);
> | }
> 
> (Third parameter to for_each_chain decides whether builtins are included or
> not.)
> 
> I guess fundamentally this is due to legacy design which keeps builtin 
> chains in place at all times. We could copy that in iptables-nft, but I 
> like the current design where we just delete the whole table and start 
> from scratch.
> 
> Florian made a related remark a while ago about flushing chains with 
> DROP policy: He claims it is almost always a mistake and we should reset 
> the policy to ACCEPT in order to avoid people from locking themselves 
> out. I second that idea, but am not sure if such a change is tolerable 
> at all.

I don't think such a change in the behaviour could properly be 
communicated to the users, let alone notified them about such a change. It 
could cause such subtle errors out there, which are hard to identify and 
fix: they'll debug the rules/user chains and the very last the base rule 
policy.

I know lots of effort went into backward compatibility, this should be 
included there too.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [iptables PATCH] iptables-nft: fix basechain policy configuration

[Phil Sutter]

Hi,

On Fri, Oct 09, 2020 at 12:37:25PM +0200, Jozsef Kadlecsik wrote:
[...]
> I know lots of effort went into backward compatibility, this should be 
> included there too.

Certainly doable. Some hacking turned into quite a mess, though:

When restoring without '--noflush', a chain cache is needed - simply
doable by treating NFT_CL_FAKE differently. Reacting upon a chain policy
of '-' is easy, just lookup the existing chain's policy from cache and
use that. Things then become ugly for not specified chains:
'flush_table' callback really deletes the table. So one has to gather
the existing builtin chains first, check if their policy is non-default
and restore those. If they don't exist though, one has to expect for
them to occur when refreshing the transaction (due to concurrent ruleset
change). So the batch jobs have to be created either way and just set to
'skip' if either table or chain doesn't exist or the policy is ACCEPT.

If alternatively I decide to drop the table delete in 'flush_table', I
need to decide whether a builtin chain should be deleted or not, based
on its policy - which may change, so when refreshing transaction I would
have to turn a chain delete job into a flush rules one. Not nice, so
don't delete builtin chains in the first place. But the next obstacle
comes with user-defined chains: Deleting the existing ones, no problem -
cache is there. But when refreshing the transaction, new ones have to be
expected, so new jobs created.

The potential need to refresh a transaction is really causing
head-aches and the simple approach of dropping the table helped quite a
bit with that. Maybe I could implement some kernel bits to make things
simpler, like "delete all non-base chains" or "create chain if not
existing". But first I need more coffee. %)

Cheers, Phil

Re: [iptables PATCH] iptables-nft: fix basechain policy configuration

[Jozsef Kadlecsik]

Hi Phil,

On Fri, 9 Oct 2020, Phil Sutter wrote:

> On Fri, Oct 09, 2020 at 12:37:25PM +0200, Jozsef Kadlecsik wrote:
> [...]
> > I know lots of effort went into backward compatibility, this should be 
> > included there too.
> 
> Certainly doable. Some hacking turned into quite a mess, though:
> 
> When restoring without '--noflush', a chain cache is needed - simply 
> doable by treating NFT_CL_FAKE differently. Reacting upon a chain policy 
> of '-' is easy, just lookup the existing chain's policy from cache and 
> use that. Things then become ugly for not specified chains: 
> 'flush_table' callback really deletes the table. So one has to gather 
> the existing builtin chains first, check if their policy is non-default 
> and restore those. If they don't exist though, one has to expect for 
> them to occur when refreshing the transaction (due to concurrent ruleset 
> change). So the batch jobs have to be created either way and just set to 
> 'skip' if either table or chain doesn't exist or the policy is ACCEPT.

I think the main problem is the difference between nft and iptables when 
printing the base chains and their policy, as you wrote:

> But that is a significant divergence between legacy and nft:
> 
> | # iptables -P FORWARD DROP
> | # iptables-restore <<EOF
> | *filter
> | COMMIT
> | EOF
> | # iptables-save
>
> With legacy, the output is:
> 
> | *filter
> | :INPUT ACCEPT [0:0]
> | :FORWARD DROP [0:0]
> | :OUTPUT ACCEPT [0:0]
> | COMMIT
> 
> With nft, there's no output at all. What do you think, should we fix
> that? If so, which side?

It looks as nft would loose the DROP policy of FORWARD! That looks like 
definitely wrong. It was explicitly set, so it should be printed/saved. 

Also, if nft in >legacy mode< would print the base chains with their 
policy (regardless of the value), couldn't then restore mode handle those 
properly?
 
> But first I need more coffee. %)

Me too... :-)

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
          H-1525 Budapest 114, POB. 49, Hungary