[iptables PATCH 1/3] arptables: Fix parsing of inverted 'arp operation' match

[iptables PATCH 1/3] arptables: Fix parsing of inverted 'arp operation' match

[Phil Sutter]

The wrong bit was set in 'invflags', probably due to copy'n'paste from
the previous case.

Fixes: 84909d171585d ("xtables: bootstrap ARP compatibility layer for nftables")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-arp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
index 8963573a72e9e..a8e49f442c6d7 100644
--- a/iptables/nft-arp.c
+++ b/iptables/nft-arp.c
@@ -244,7 +244,7 @@ static void nft_arp_parse_payload(struct nft_xt_ctx *ctx,
 		fw->arp.arhln = ar_hln;
 		fw->arp.arhln_mask = 0xff;
 		if (inv)
-			fw->arp.invflags |= IPT_INV_ARPOP;
+			fw->arp.invflags |= IPT_INV_ARPHLN;
 		break;
 	case offsetof(struct arphdr, ar_pln):
 		get_cmp_data(e, &ar_pln, sizeof(ar_pln), &inv);
-- 
2.40.0

[iptables PATCH 2/3] arptables: Don't omit standard matches if inverted

[Phil Sutter]

Inverted --h-len and --h-type matches were omitted from output by
accident if they matched on their standard value.

Fixes: 84331e3ed3f8e ("arptables-nft: Don't print default h-len/h-type values")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-arp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
index a8e49f442c6d7..3236e2f54e21d 100644
--- a/iptables/nft-arp.c
+++ b/iptables/nft-arp.c
@@ -408,7 +408,8 @@ static void nft_arp_print_rule_details(const struct iptables_command_state *cs,
 
 after_devdst:
 
-	if (fw->arp.arhln_mask != 255 || fw->arp.arhln != 6) {
+	if (fw->arp.arhln_mask != 255 || fw->arp.arhln != 6 ||
+	    fw->arp.invflags & IPT_INV_ARPHLN) {
 		printf("%s%s", sep, fw->arp.invflags & IPT_INV_ARPHLN
 			? "! " : "");
 		printf("--h-length %d", fw->arp.arhln);
@@ -432,7 +433,8 @@ static void nft_arp_print_rule_details(const struct iptables_command_state *cs,
 		sep = " ";
 	}
 
-	if (fw->arp.arhrd_mask != 65535 || fw->arp.arhrd != htons(1)) {
+	if (fw->arp.arhrd_mask != 65535 || fw->arp.arhrd != htons(1) ||
+	    fw->arp.invflags & IPT_INV_ARPHRD) {
 		uint16_t tmp = ntohs(fw->arp.arhrd);
 
 		printf("%s%s", sep, fw->arp.invflags & IPT_INV_ARPHRD
-- 
2.40.0

[iptables PATCH 3/3] xshared: Fix parsing of option arguments in same word

[Phil Sutter]

When merging commandline parsers, a decision between 'argv[optind - 1]'
and 'optarg' had to be made in some spots. While the implementation of
check_inverse() required the former, use of the latter allows for the
common syntax of '--opt=arg' or even '-oarg' as 'optarg' will point at
the suffix while 'argv[optind - 1]' will just point at the following
option.

Fix the mess by making check_inverse() update optarg pointer if needed
so calling code may refer to and always correct 'optarg'.

Fixes: 0af80a91b0a98 ("nft: Merge xtables-arp-standalone.c into xtables-standalone.c")
Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1677
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 extensions/libarpt_standard.t |  2 ++
 extensions/libxt_standard.t   |  3 ++
 iptables/xshared.c            | 61 +++++++++++++++++------------------
 3 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/extensions/libarpt_standard.t b/extensions/libarpt_standard.t
index e84a00b780488..007fa2b8335e8 100644
--- a/extensions/libarpt_standard.t
+++ b/extensions/libarpt_standard.t
@@ -12,3 +12,5 @@
 -i lo --destination-mac 11:22:33:44:55:66;-i lo --dst-mac 11:22:33:44:55:66;OK
 --source-mac Unicast;--src-mac 00:00:00:00:00:00/01:00:00:00:00:00;OK
 ! --src-mac Multicast;! --src-mac 01:00:00:00:00:00/01:00:00:00:00:00;OK
+--src-mac=01:02:03:04:05:06 --dst-mac=07:08:09:0A:0B:0C --h-length=6 --opcode=Request --h-type=Ethernet --proto-type=ipv4;--src-mac 01:02:03:04:05:06 --dst-mac 07:08:09:0a:0b:0c --opcode 1 --proto-type 0x800;OK
+--src-mac ! 01:02:03:04:05:06 --dst-mac ! 07:08:09:0A:0B:0C --h-length ! 6 --opcode ! Request --h-type ! Ethernet --proto-type ! ipv4;! --src-mac 01:02:03:04:05:06 ! --dst-mac 07:08:09:0a:0b:0c ! --h-length 6 ! --opcode 1 ! --h-type 1 ! --proto-type 0x800;OK
diff --git a/extensions/libxt_standard.t b/extensions/libxt_standard.t
index 56d6da2e5884e..6ed978e442b80 100644
--- a/extensions/libxt_standard.t
+++ b/extensions/libxt_standard.t
@@ -21,3 +21,6 @@
 -s 10.11.12.13/255.128.0.0;-s 10.0.0.0/9;OK
 -s 10.11.12.13/255.0.255.0;-s 10.0.12.0/255.0.255.0;OK
 -s 10.11.12.13/255.0.12.0;-s 10.0.12.0/255.0.12.0;OK
+:FORWARD
+--protocol=tcp --source=1.2.3.4 --destination=5.6.7.8/32 --in-interface=eth0 --out-interface=eth1 --jump=ACCEPT;-s 1.2.3.4/32 -d 5.6.7.8/32 -i eth0 -o eth1 -p tcp -j ACCEPT;OK
+-ptcp -s1.2.3.4 -d5.6.7.8/32 -ieth0 -oeth1 -jACCEPT;-s 1.2.3.4/32 -d 5.6.7.8/32 -i eth0 -o eth1 -p tcp -j ACCEPT;OK
diff --git a/iptables/xshared.c b/iptables/xshared.c
index ac51fac5ce9ed..17aed04e02b09 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -1318,7 +1318,7 @@ static void check_empty_interface(struct xtables_args *args, const char *arg)
 }
 
 static void check_inverse(struct xtables_args *args, const char option[],
-			  bool *invert, int *optidx, int argc)
+			  bool *invert, int argc, char **argv)
 {
 	switch (args->family) {
 	case NFPROTO_ARP:
@@ -1337,12 +1337,11 @@ static void check_inverse(struct xtables_args *args, const char option[],
 		xtables_error(PARAMETER_PROBLEM,
 			      "Multiple `!' flags not allowed");
 	*invert = true;
-	if (optidx) {
-		*optidx = *optidx + 1;
-		if (argc && *optidx > argc)
-			xtables_error(PARAMETER_PROBLEM,
-				      "no argument following `!'");
-	}
+	optind++;
+	if (optind > argc)
+		xtables_error(PARAMETER_PROBLEM, "no argument following `!'");
+
+	optarg = argv[optind - 1];
 }
 
 static const char *optstring_lookup(int family)
@@ -1555,16 +1554,16 @@ void do_parse(int argc, char *argv[],
 			 * Option selection
 			 */
 		case 'p':
-			check_inverse(args, optarg, &invert, &optind, argc);
+			check_inverse(args, optarg, &invert, argc, argv);
 			set_option(&cs->options, OPT_PROTOCOL,
 				   &args->invflags, invert);
 
 			/* Canonicalize into lower case */
-			for (cs->protocol = argv[optind - 1];
+			for (cs->protocol = optarg;
 			     *cs->protocol; cs->protocol++)
 				*cs->protocol = tolower(*cs->protocol);
 
-			cs->protocol = argv[optind - 1];
+			cs->protocol = optarg;
 			args->proto = xtables_parse_protocol(cs->protocol);
 
 			if (args->proto == 0 &&
@@ -1578,17 +1577,17 @@ void do_parse(int argc, char *argv[],
 			break;
 
 		case 's':
-			check_inverse(args, optarg, &invert, &optind, argc);
+			check_inverse(args, optarg, &invert, argc, argv);
 			set_option(&cs->options, OPT_SOURCE,
 				   &args->invflags, invert);
-			args->shostnetworkmask = argv[optind - 1];
+			args->shostnetworkmask = optarg;
 			break;
 
 		case 'd':
-			check_inverse(args, optarg, &invert, &optind, argc);
+			check_inverse(args, optarg, &invert, argc, argv);
 			set_option(&cs->options, OPT_DESTINATION,
 				   &args->invflags, invert);
-			args->dhostnetworkmask = argv[optind - 1];
+			args->dhostnetworkmask = optarg;
 			break;
 
 #ifdef IPT_F_GOTO
@@ -1601,71 +1600,71 @@ void do_parse(int argc, char *argv[],
 #endif
 
 		case 2:/* src-mac */
-			check_inverse(args, optarg, &invert, &optind, argc);
+			check_inverse(args, optarg, &invert, argc, argv);
 			set_option(&cs->options, OPT_S_MAC, &args->invflags,
 				   invert);
-			args->src_mac = argv[optind - 1];
+			args->src_mac = optarg;
 			break;
 
 		case 3:/* dst-mac */
-			check_inverse(args, optarg, &invert, &optind, argc);
+			check_inverse(args, optarg, &invert, argc, argv);
 			set_option(&cs->options, OPT_D_MAC, &args->invflags,
 				   invert);
-			args->dst_mac = argv[optind - 1];
+			args->dst_mac = optarg;
 			break;
 
 		case 'l':/* hardware length */
-			check_inverse(args, optarg, &invert, &optind, argc);
+			check_inverse(args, optarg, &invert, argc, argv);
 			set_option(&cs->options, OPT_H_LENGTH, &args->invflags,
 				   invert);
-			args->arp_hlen = argv[optind - 1];
+			args->arp_hlen = optarg;
 			break;
 
 		case 8: /* was never supported, not even in arptables-legacy */
 			xtables_error(PARAMETER_PROBLEM, "not supported");
 		case 4:/* opcode */
-			check_inverse(args, optarg, &invert, &optind, argc);
+			check_inverse(args, optarg, &invert, argc, argv);
 			set_option(&cs->options, OPT_OPCODE, &args->invflags,
 				   invert);
-			args->arp_opcode = argv[optind - 1];
+			args->arp_opcode = optarg;
 			break;
 
 		case 5:/* h-type */
-			check_inverse(args, optarg, &invert, &optind, argc);
+			check_inverse(args, optarg, &invert, argc, argv);
 			set_option(&cs->options, OPT_H_TYPE, &args->invflags,
 				   invert);
-			args->arp_htype = argv[optind - 1];
+			args->arp_htype = optarg;
 			break;
 
 		case 6:/* proto-type */
-			check_inverse(args, optarg, &invert, &optind, argc);
+			check_inverse(args, optarg, &invert, argc, argv);
 			set_option(&cs->options, OPT_P_TYPE, &args->invflags,
 				   invert);
-			args->arp_ptype = argv[optind - 1];
+			args->arp_ptype = optarg;
 			break;
 
 		case 'j':
 			set_option(&cs->options, OPT_JUMP, &args->invflags,
 				   invert);
-			command_jump(cs, argv[optind - 1]);
+			command_jump(cs, optarg);
 			break;
 
 		case 'i':
 			check_empty_interface(args, optarg);
-			check_inverse(args, optarg, &invert, &optind, argc);
+			check_inverse(args, optarg, &invert, argc, argv);
 			set_option(&cs->options, OPT_VIANAMEIN,
 				   &args->invflags, invert);
-			xtables_parse_interface(argv[optind - 1],
+			xtables_parse_interface(optarg,
 						args->iniface,
 						args->iniface_mask);
 			break;
 
 		case 'o':
 			check_empty_interface(args, optarg);
-			check_inverse(args, optarg, &invert, &optind, argc);
+			check_inverse(args, optarg, &invert, argc, argv);
 			set_option(&cs->options, OPT_VIANAMEOUT,
 				   &args->invflags, invert);
-			xtables_parse_interface(argv[optind - 1],
+			xtables_parse_interface(optarg,
 						args->outiface,
 						args->outiface_mask);
 			break;
-- 
2.40.0

Re: [iptables PATCH 1/3] arptables: Fix parsing of inverted 'arp operation' match

[Phil Sutter]

On Fri, Apr 28, 2023 at 03:05:29PM +0200, Phil Sutter wrote:
> The wrong bit was set in 'invflags', probably due to copy'n'paste from
> the previous case.
> 
> Fixes: 84909d171585d ("xtables: bootstrap ARP compatibility layer for nftables")
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Series applied.