[PATCH iptables]: restore legacy behaviour of iptables-restore when rules start with -4/-6

[PATCH iptables]: restore legacy behaviour of iptables-restore when rules start with -4/-6

[Adel Belhouane]

Legacy implementation of iptables-restore / ip6tables-restore allowed
to insert a -4 or -6 option at start of a rule line to ignore it if not
matching the command's protocol. This allowed to mix specific ipv4 and ipv6
rules in a single file, as still described in iptables 1.8.3's man page in
options -4 and -6.

Example with the file /tmp/rules:

*filter
:INPUT ACCEPT [0:0]
:FORWARD ACCEPT [0:0]
:OUTPUT ACCEPT [0:0]
-4 -A INPUT -p icmp -j ACCEPT
-6 -A INPUT -p ipv6-icmp -j ACCEPT
COMMIT

works fine with iptables-legacy-restore and ip6tables-legacy-restore but
fails for the two nft variants:

% iptables-nft-restore < /tmp/rules
% iptables-nft-save
*filter
:INPUT ACCEPT [0:0]
:FORWARD ACCEPT [0:0]
:OUTPUT ACCEPT [0:0]
-A INPUT -p icmp -j ACCEPT
-A INPUT -p ipv6-icmp -j ACCEPT
COMMIT

The two rules were added when the -6 rule should have been ignored.

% ip6tables-nft-restore < /tmp/rules
Error occurred at line: 5
Try `ip6tables-restore -h' or 'ip6tables-restore --help' for more information.

No rule was added when the -4 rule should have been ignored and the -6
rule added.

There's a distribution bug report mentioning this problem:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=925343

The following patch restores the legacy behaviour:
- let do_parse() return and thus not add a command in those restore
special cases
- let do_commandx() ignore CMD_NONE instead of bailing out

It doesn't attempt to fix all minor anomalies, but just to fix the regression.
For example the line below should throw an error according to the man page
(and does in the legacy version), but doesn't in the nft version:

% iptables -6 -A INPUT -p tcp -j ACCEPT

Signed-off-by: Adel Belhouane <bugs.a.b@free.fr>

diff --git a/iptables/xtables.c b/iptables/xtables.c
index 93d9dcb..0e0cb5f 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -955,6 +955,9 @@ void do_parse(struct nft_handle *h, int argc, char *argv[],
  			break;
  
  		case '4':
+			if (p->restore && args->family == AF_INET6)
+				return;
+
  			if (args->family != AF_INET)
  				exit_tryhelp(2);
  
@@ -962,6 +965,9 @@ void do_parse(struct nft_handle *h, int argc, char *argv[],
  			break;
  
  		case '6':
+			if (p->restore && args->family == AF_INET)
+				return;
+
  			args->family = AF_INET6;
  			xtables_set_nfproto(AF_INET6);
  
@@ -1174,6 +1180,9 @@ int do_commandx(struct nft_handle *h, int argc, char *argv[], char **table,
  	case CMD_SET_POLICY:
  		ret = nft_chain_set(h, p.table, p.chain, p.policy, NULL);
  		break;
+	case CMD_NONE:
+	/* do_parse ignored the line (eg: -4 with ip6tables-restore) */
+		break;
  	default:
  		/* We should never reach this... */
  		exit_tryhelp(2);

Re: [PATCH iptables]: restore legacy behaviour of iptables-restore when rules start with -4/-6

[Phil Sutter]

Hi Adel,

On Wed, Jul 24, 2019 at 10:13:02PM +0200, Adel Belhouane wrote:
> Legacy implementation of iptables-restore / ip6tables-restore allowed
> to insert a -4 or -6 option at start of a rule line to ignore it if not
> matching the command's protocol. This allowed to mix specific ipv4 and ipv6
> rules in a single file, as still described in iptables 1.8.3's man page in
> options -4 and -6.

Thanks for catching this. Seems like at some point the intention was to
have a common 'xtables' command and pass -4/-6 parameters to toggle
between iptables and ip6tables operation. Pablo, is this still relevant,
or can we just get rid of it altogether?

> Example with the file /tmp/rules:
> 
> *filter
> :INPUT ACCEPT [0:0]
> :FORWARD ACCEPT [0:0]
> :OUTPUT ACCEPT [0:0]
> -4 -A INPUT -p icmp -j ACCEPT
> -6 -A INPUT -p ipv6-icmp -j ACCEPT
> COMMIT

Would you mind creating a testcase in iptables/tests/shell? I guess
testcases/ipt-restore is suitable, please have a look at
0003-restore-ordering_0 in that directory for an illustration of how we
usually check results of *-restore calls.

[...]
> It doesn't attempt to fix all minor anomalies, but just to fix the regression.
> For example the line below should throw an error according to the man page
> (and does in the legacy version), but doesn't in the nft version:
> 
> % iptables -6 -A INPUT -p tcp -j ACCEPT

On my testing VM this rule ends up in table ip filter, so this seems to
not even work as intended.

> Signed-off-by: Adel Belhouane <bugs.a.b@free.fr>

Acked-by: Phil Sutter <phil@nwl.cc>

Re: [PATCH iptables]: restore legacy behaviour of iptables-restore when rules start with -4/-6

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> Thanks for catching this. Seems like at some point the intention was to
> have a common 'xtables' command and pass -4/-6 parameters to toggle
> between iptables and ip6tables operation. Pablo, is this still relevant,
> or can we just get rid of it altogether?

Evidently this is behaviour that is relied on by some, so we need to
cope with this in -nft version too.

> > % iptables -6 -A INPUT -p tcp -j ACCEPT
> 
> On my testing VM this rule ends up in table ip filter, so this seems to
> not even work as intended.

$ iptables-legacy -6 -A INPUT -p tcp -j ACCEPT
This is the IPv4 version of iptables.

Re: [PATCH iptables]: restore legacy behaviour of iptables-restore when rules start with -4/-6

[Adel Belhouane]

Hello Phil,

Le 25/07/2019 à 12:40, Phil Sutter a écrit :
> 
> Would you mind creating a testcase in iptables/tests/shell? I guess
> testcases/ipt-restore is suitable, please have a look at
> 0003-restore-ordering_0 in that directory for an illustration of how we
> usually check results of *-restore calls.
> 

I moved the examples to two testcase files taking example on what you
suggested and resent a patch.

regards,
Adel