[iptables PATCH 0/4] Make rule parsing strict

[iptables PATCH 0/4] Make rule parsing strict

[Phil Sutter]

Abort the program when encountering rules with unsupported matches.

While nft_is_table_compatible() tries to catch this situation, it boils
down to merely accepting or rejecting expressions based on type. Yet
these may still be used in incompatible ways.

Patch 1 fixes for payload matches on ICMP(v6) headers and is almost
independent of the rest.

Patch 2 prepares arptables rule parsing for the error message added by
patch 3.

Patch 3 makes various situations complain by emitting error messages. It
was compiled after reviewing all callees of rule_to_cs callback for
unhandled unexpected input.

Patch 5 then finally does it's thing.

Phil Sutter (4):
  nft: Parse icmp header matches
  arptables: Check the mandatory ar_pln match
  nft: Increase rule parser strictness
  nft: Make rule parsing errors fatal

 iptables/nft-arp.c                            |   9 +-
 iptables/nft-bridge.c                         |   4 +
 iptables/nft-ipv4.c                           |   4 +-
 iptables/nft-ipv6.c                           |   4 +-
 iptables/nft-shared.c                         | 113 ++++++++++++++++--
 .../nft-only/0010-iptables-nft-save.txt       |   6 +-
 6 files changed, 123 insertions(+), 17 deletions(-)

-- 
2.38.0

[iptables PATCH 1/4] nft: Parse icmp header matches

[Phil Sutter]

These were previously ignored.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-shared.c                         | 74 +++++++++++++++++++
 .../nft-only/0010-iptables-nft-save.txt       |  6 +-
 2 files changed, 77 insertions(+), 3 deletions(-)

diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index 56acbd4555f4b..d4b21921077d9 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -833,6 +833,65 @@ static void nft_parse_tcp(struct nft_xt_ctx *ctx,
 				   op, dport, XT_TCP_INV_DSTPT);
 }
 
+static void nft_parse_icmp(struct nft_xt_ctx *ctx,
+			   struct iptables_command_state *cs,
+			   struct nft_xt_ctx_reg *sreg,
+			   uint8_t op, const char *data, size_t dlen)
+{
+	struct xtables_match *match;
+	struct ipt_icmp icmp = {
+		.type = UINT8_MAX,
+		.code = { 0, UINT8_MAX },
+	};
+
+	if (dlen < 1)
+		goto out_err_len;
+
+	switch (sreg->payload.offset) {
+	case 0:
+		icmp.type = data[0];
+		if (dlen == 1)
+			break;
+		dlen--;
+		data++;
+		/* fall through */
+	case 1:
+		if (dlen > 1)
+			goto out_err_len;
+		icmp.code[0] = icmp.code[1] = data[0];
+		break;
+	default:
+		ctx->errmsg = "unexpected payload offset";
+		return;
+	}
+
+	switch (ctx->h->family) {
+	case NFPROTO_IPV4:
+		match = nft_create_match(ctx, cs, "icmp");
+		break;
+	case NFPROTO_IPV6:
+		if (icmp.type == UINT8_MAX) {
+			ctx->errmsg = "icmp6 code with any type match not supported";
+			return;
+		}
+		match = nft_create_match(ctx, cs, "icmp6");
+		break;
+	default:
+		ctx->errmsg = "unexpected family for icmp match";
+		return;
+	}
+
+	if (!match) {
+		ctx->errmsg = "icmp match extension not found";
+		return;
+	}
+	memcpy(match->m->data, &icmp, sizeof(icmp));
+	return;
+
+out_err_len:
+	ctx->errmsg = "unexpected RHS data length";
+}
+
 static void nft_parse_th_port(struct nft_xt_ctx *ctx,
 			      struct iptables_command_state *cs,
 			      uint8_t proto,
@@ -915,6 +974,21 @@ static void nft_parse_transport(struct nft_xt_ctx *ctx,
 		return;
 	}
 
+	switch (proto) {
+	case IPPROTO_UDP:
+	case IPPROTO_TCP:
+		break;
+	case IPPROTO_ICMP:
+	case IPPROTO_ICMPV6:
+		nft_parse_icmp(ctx, cs, sreg, op,
+			       nftnl_expr_get(e, NFTNL_EXPR_CMP_DATA, &len),
+			       len);
+		return;
+	default:
+		ctx->errmsg = "unsupported layer 4 protocol value";
+		return;
+	}
+
 	switch(sreg->payload.offset) {
 	case 0: /* th->sport */
 		switch (len) {
diff --git a/iptables/tests/shell/testcases/nft-only/0010-iptables-nft-save.txt b/iptables/tests/shell/testcases/nft-only/0010-iptables-nft-save.txt
index 73d7108c5094e..5ee4c23113aa8 100644
--- a/iptables/tests/shell/testcases/nft-only/0010-iptables-nft-save.txt
+++ b/iptables/tests/shell/testcases/nft-only/0010-iptables-nft-save.txt
@@ -13,9 +13,9 @@
 -A INPUT -d 0.0.0.0/2 -m ttl --ttl-gt 2 -j ACCEPT
 -A INPUT -d 0.0.0.0/3 -m ttl --ttl-lt 254 -j ACCEPT
 -A INPUT -d 0.0.0.0/4 -m ttl ! --ttl-eq 255 -j DROP
--A INPUT -d 8.0.0.0/5 -p icmp -j ACCEPT
--A INPUT -d 8.0.0.0/6 -p icmp -j ACCEPT
--A INPUT -d 10.0.0.0/7 -p icmp -j ACCEPT
+-A INPUT -d 8.0.0.0/5 -p icmp -m icmp --icmp-type 1 -j ACCEPT
+-A INPUT -d 8.0.0.0/6 -p icmp -m icmp --icmp-type 2/3 -j ACCEPT
+-A INPUT -d 10.0.0.0/7 -p icmp -m icmp --icmp-type 8 -j ACCEPT
 -A INPUT -m pkttype --pkt-type broadcast -j ACCEPT
 -A INPUT -m pkttype ! --pkt-type unicast -j DROP
 -A INPUT -p tcp
-- 
2.38.0

[iptables PATCH 2/4] arptables: Check the mandatory ar_pln match

[Phil Sutter]

This match is added by nft_arp_add() to every rule with same value, so
when parsing just check it is as expected and otherwise ignore it. This
allows to treat matches on all other offsets/lengths as error.

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

diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
index d670cbe629fe4..edf179521e355 100644
--- a/iptables/nft-arp.c
+++ b/iptables/nft-arp.c
@@ -214,7 +214,7 @@ static void nft_arp_parse_payload(struct nft_xt_ctx *ctx,
 	struct arpt_entry *fw = &cs->arp;
 	struct in_addr addr;
 	uint16_t ar_hrd, ar_pro, ar_op;
-	uint8_t ar_hln;
+	uint8_t ar_hln, ar_pln;
 	bool inv;
 
 	switch (reg->payload.offset) {
@@ -246,6 +246,11 @@ static void nft_arp_parse_payload(struct nft_xt_ctx *ctx,
 		if (inv)
 			fw->arp.invflags |= IPT_INV_ARPOP;
 		break;
+	case offsetof(struct arphdr, ar_pln):
+		get_cmp_data(e, &ar_pln, sizeof(ar_pln), &inv);
+		if (ar_pln != 4 || inv)
+			ctx->errmsg = "unexpected ARP protocol length match";
+		break;
 	default:
 		if (reg->payload.offset == sizeof(struct arphdr)) {
 			if (nft_arp_parse_devaddr(reg, e, &fw->arp.src_devaddr))
-- 
2.38.0

[iptables PATCH 3/4] nft: Increase rule parser strictness

[Phil Sutter]

Catch more unexpected conditions.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-arp.c    |  2 ++
 iptables/nft-bridge.c |  4 ++++
 iptables/nft-ipv4.c   |  4 +++-
 iptables/nft-ipv6.c   |  4 +++-
 iptables/nft-shared.c | 35 +++++++++++++++++++++++++----------
 5 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
index edf179521e355..210f43d2cefbe 100644
--- a/iptables/nft-arp.c
+++ b/iptables/nft-arp.c
@@ -288,6 +288,8 @@ static void nft_arp_parse_payload(struct nft_xt_ctx *ctx,
 
 			if (inv)
 				fw->arp.invflags |= IPT_INV_DSTIP;
+		} else {
+			ctx->errmsg = "unknown payload offset";
 		}
 		break;
 	}
diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index e223d19765f90..83cbe31559d4b 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -287,6 +287,10 @@ static void nft_bridge_parse_payload(struct nft_xt_ctx *ctx,
 			fw->invflags |= EBT_IPROTO;
 		fw->bitmask &= ~EBT_NOPROTO;
 		break;
+	default:
+		DEBUGP("unknown payload offset %d\n", reg->payload.offset);
+		ctx->errmsg = "unknown payload offset";
+		break;
 	}
 }
 
diff --git a/iptables/nft-ipv4.c b/iptables/nft-ipv4.c
index 42167351710e6..dcc4a8edfc87f 100644
--- a/iptables/nft-ipv4.c
+++ b/iptables/nft-ipv4.c
@@ -207,10 +207,12 @@ static void nft_ipv4_parse_payload(struct nft_xt_ctx *ctx,
 			cs->fw.ip.invflags |= IPT_INV_FRAG;
 		break;
 	case offsetof(struct iphdr, ttl):
-		nft_parse_hl(ctx, e, cs);
+		if (nft_parse_hl(ctx, e, cs) < 0)
+			ctx->errmsg = "invalid ttl field match";
 		break;
 	default:
 		DEBUGP("unknown payload offset %d\n", sreg->payload.offset);
+		ctx->errmsg = "unknown payload offset";
 		break;
 	}
 }
diff --git a/iptables/nft-ipv6.c b/iptables/nft-ipv6.c
index 3a373b7eb2cfe..e98921856c75d 100644
--- a/iptables/nft-ipv6.c
+++ b/iptables/nft-ipv6.c
@@ -173,10 +173,12 @@ static void nft_ipv6_parse_payload(struct nft_xt_ctx *ctx,
 		if (inv)
 			cs->fw6.ipv6.invflags |= IP6T_INV_PROTO;
 	case offsetof(struct ip6_hdr, ip6_hlim):
-		nft_parse_hl(ctx, e, cs);
+		if (nft_parse_hl(ctx, e, cs) < 0)
+			ctx->errmsg = "invalid ttl field match";
 		break;
 	default:
 		DEBUGP("unknown payload offset %d\n", reg->payload.offset);
+		ctx->errmsg = "unknown payload offset";
 		break;
 	}
 }
diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index d4b21921077d9..c13fc307e7a89 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -444,8 +444,10 @@ static void nft_parse_target(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 	size_t size;
 
 	target = xtables_find_target(targname, XTF_TRY_LOAD);
-	if (target == NULL)
+	if (target == NULL) {
+		ctx->errmsg = "target extension not found";
 		return;
+	}
 
 	size = XT_ALIGN(sizeof(struct xt_entry_target)) + tg_len;
 
@@ -482,8 +484,10 @@ static void nft_parse_match(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 	}
 
 	match = xtables_find_match(mt_name, XTF_TRY_LOAD, matches);
-	if (match == NULL)
+	if (match == NULL) {
+		ctx->errmsg = "match extension not found";
 		return;
+	}
 
 	m = xtables_calloc(1, sizeof(struct xt_entry_match) + mt_len);
 	memcpy(&m->data, mt_info, mt_len);
@@ -690,9 +694,10 @@ static struct xt_tcp *nft_tcp_match(struct nft_xt_ctx *ctx,
 
 	if (!tcp) {
 		match = nft_create_match(ctx, cs, "tcp");
-		if (!match)
+		if (!match) {
+			ctx->errmsg = "tcp match extension not found";
 			return NULL;
-
+		}
 		tcp = (void*)match->m->data;
 		ctx->tcpudp.tcp = tcp;
 	}
@@ -904,6 +909,8 @@ static void nft_parse_th_port(struct nft_xt_ctx *ctx,
 	case IPPROTO_TCP:
 		nft_parse_tcp(ctx, cs, sport, dport, op);
 		break;
+	default:
+		ctx->errmsg = "unknown layer 4 protocol for TH match";
 	}
 }
 
@@ -957,8 +964,8 @@ static void nft_parse_transport(struct nft_xt_ctx *ctx,
 		proto = ctx->cs->fw6.ipv6.proto;
 		break;
 	default:
-		proto = 0;
-		break;
+		ctx->errmsg = "invalid family for TH match";
+		return;
 	}
 
 	nftnl_expr_get(e, NFTNL_EXPR_CMP_DATA, &len);
@@ -1129,8 +1136,10 @@ static void nft_parse_immediate(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 		if (!dreg)
 			return;
 
-		if (len > sizeof(dreg->immediate.data))
+		if (len > sizeof(dreg->immediate.data)) {
+			ctx->errmsg = "oversized immediate data";
 			return;
+		}
 
 		memcpy(dreg->immediate.data, imm_data, len);
 		dreg->immediate.len = len;
@@ -1163,8 +1172,10 @@ static void nft_parse_immediate(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 	}
 
 	cs->target = xtables_find_target(cs->jumpto, XTF_TRY_LOAD);
-	if (!cs->target)
+	if (!cs->target) {
+		ctx->errmsg = "verdict extension not found";
 		return;
+	}
 
 	size = XT_ALIGN(sizeof(struct xt_entry_target)) + cs->target->size;
 	t = xtables_calloc(1, size);
@@ -1197,8 +1208,10 @@ static void nft_parse_limit(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 	}
 
 	match = xtables_find_match("limit", XTF_TRY_LOAD, matches);
-	if (match == NULL)
+	if (match == NULL) {
+		ctx->errmsg = "limit match extension not found";
 		return;
+	}
 
 	size = XT_ALIGN(sizeof(struct xt_entry_match)) + match->size;
 	match->m = xtables_calloc(1, size);
@@ -1245,8 +1258,10 @@ static void nft_parse_log(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 			 nftnl_expr_get_str(e, NFTNL_EXPR_LOG_PREFIX));
 
 	target = xtables_find_target("NFLOG", XTF_TRY_LOAD);
-	if (target == NULL)
+	if (target == NULL) {
+		ctx->errmsg = "NFLOG target extension not found";
 		return;
+	}
 
 	target_size = XT_ALIGN(sizeof(struct xt_entry_target)) +
 		      XT_ALIGN(sizeof(struct xt_nflog_info_nft));
-- 
2.38.0

[iptables PATCH 4/4] nft: Make rule parsing errors fatal

[Phil Sutter]

Finish parsing the rule, thereby printing all potential problems and
abort the program.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/nft-shared.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index c13fc307e7a89..4a7b5406892c4 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -1362,7 +1362,7 @@ bool nft_rule_to_iptables_command_state(struct nft_handle *h,
 			nft_parse_range(&ctx, expr);
 
 		if (ctx.errmsg) {
-			fprintf(stderr, "%s", ctx.errmsg);
+			fprintf(stderr, "Error: %s\n", ctx.errmsg);
 			ctx.errmsg = NULL;
 			ret = false;
 		}
@@ -1404,6 +1404,8 @@ bool nft_rule_to_iptables_command_state(struct nft_handle *h,
 	if (!cs->jumpto)
 		cs->jumpto = "";
 
+	if (!ret)
+		xtables_error(VERSION_PROBLEM, "Parsing nftables rule failed");
 	return ret;
 }
 
-- 
2.38.0

Re: [iptables PATCH 0/4] Make rule parsing strict

[Phil Sutter]

On Thu, Dec 15, 2022 at 05:17:52PM +0100, Phil Sutter wrote:
> Abort the program when encountering rules with unsupported matches.
> 
> While nft_is_table_compatible() tries to catch this situation, it boils
> down to merely accepting or rejecting expressions based on type. Yet
> these may still be used in incompatible ways.
> 
> Patch 1 fixes for payload matches on ICMP(v6) headers and is almost
> independent of the rest.
> 
> Patch 2 prepares arptables rule parsing for the error message added by
> patch 3.
> 
> Patch 3 makes various situations complain by emitting error messages. It
> was compiled after reviewing all callees of rule_to_cs callback for
> unhandled unexpected input.
> 
> Patch 5 then finally does it's thing.
> 
> Phil Sutter (4):
>   nft: Parse icmp header matches
>   arptables: Check the mandatory ar_pln match
>   nft: Increase rule parser strictness
>   nft: Make rule parsing errors fatal

Series applied.