[iptables PATCH 0/3] Fix SECMARK target comparison

[iptables PATCH 0/3] Fix SECMARK target comparison

[Phil Sutter]

The kernel sets struct secmark_target_info->secid, so target comparison
in user space failed every time. Given that target data comparison
happens in libiptc, fixing this is a bit harder than just adding a cmp()
callback to struct xtables_target. Instead, allow for targets to write
the matchmask bits for their private data themselves and account for
that in both legacy and nft code. Then make use of the new
infrastructure to fix libxt_SECMARK.

Phil Sutter (3):
  xshared: Share make_delete_mask() between ip{,6}tables
  libxtables: Introduce 'matchmask' target callback
  libxt_SECMARK: Fix for failing target comparison

 configure.ac               |  4 ++--
 extensions/libxt_SECMARK.c | 10 ++++++++++
 extensions/libxt_SECMARK.t |  4 ++++
 include/xtables.h          |  3 +++
 iptables/ip6tables.c       | 38 ++------------------------------------
 iptables/iptables.c        | 38 ++------------------------------------
 iptables/nft-shared.c      | 15 ++++++++++++++-
 iptables/xshared.c         | 38 ++++++++++++++++++++++++++++++++++++++
 iptables/xshared.h         |  4 ++++
 9 files changed, 79 insertions(+), 75 deletions(-)
 create mode 100644 extensions/libxt_SECMARK.t

-- 
2.25.1

[iptables PATCH 1/3] xshared: Share make_delete_mask() between ip{,6}tables

[Phil Sutter]

Function bodies were mostly identical, the only difference being the use
of struct ipt_entry or ip6t_entry for size calculation. Pass this value
via parameter to make them fully identical.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 iptables/ip6tables.c | 38 ++------------------------------------
 iptables/iptables.c  | 38 ++------------------------------------
 iptables/xshared.c   | 34 ++++++++++++++++++++++++++++++++++
 iptables/xshared.h   |  4 ++++
 4 files changed, 42 insertions(+), 72 deletions(-)

diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index 576c2cf8b0d9f..1a59d6f7a1542 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -622,40 +622,6 @@ insert_entry(const xt_chainlabel chain,
 	return ret;
 }
 
-static unsigned char *
-make_delete_mask(const struct xtables_rule_match *matches,
-		 const struct xtables_target *target)
-{
-	/* Establish mask for comparison */
-	unsigned int size;
-	const struct xtables_rule_match *matchp;
-	unsigned char *mask, *mptr;
-
-	size = sizeof(struct ip6t_entry);
-	for (matchp = matches; matchp; matchp = matchp->next)
-		size += XT_ALIGN(sizeof(struct xt_entry_match)) + matchp->match->size;
-
-	mask = xtables_calloc(1, size
-			 + XT_ALIGN(sizeof(struct xt_entry_target))
-			 + target->size);
-
-	memset(mask, 0xFF, sizeof(struct ip6t_entry));
-	mptr = mask + sizeof(struct ip6t_entry);
-
-	for (matchp = matches; matchp; matchp = matchp->next) {
-		memset(mptr, 0xFF,
-		       XT_ALIGN(sizeof(struct xt_entry_match))
-		       + matchp->match->userspacesize);
-		mptr += XT_ALIGN(sizeof(struct xt_entry_match)) + matchp->match->size;
-	}
-
-	memset(mptr, 0xFF,
-	       XT_ALIGN(sizeof(struct xt_entry_target))
-	       + target->userspacesize);
-
-	return mask;
-}
-
 static int
 delete_entry(const xt_chainlabel chain,
 	     struct ip6t_entry *fw,
@@ -674,7 +640,7 @@ delete_entry(const xt_chainlabel chain,
 	int ret = 1;
 	unsigned char *mask;
 
-	mask = make_delete_mask(matches, target);
+	mask = make_delete_mask(matches, target, sizeof(*fw));
 	for (i = 0; i < nsaddrs; i++) {
 		fw->ipv6.src = saddrs[i];
 		fw->ipv6.smsk = smasks[i];
@@ -704,7 +670,7 @@ check_entry(const xt_chainlabel chain, struct ip6t_entry *fw,
 	int ret = 1;
 	unsigned char *mask;
 
-	mask = make_delete_mask(matches, target);
+	mask = make_delete_mask(matches, target, sizeof(fw));
 	for (i = 0; i < nsaddrs; i++) {
 		fw->ipv6.src = saddrs[i];
 		fw->ipv6.smsk = smasks[i];
diff --git a/iptables/iptables.c b/iptables/iptables.c
index 88ef6cf666d4b..ead9c482a3ad1 100644
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -614,40 +614,6 @@ insert_entry(const xt_chainlabel chain,
 	return ret;
 }
 
-static unsigned char *
-make_delete_mask(const struct xtables_rule_match *matches,
-		 const struct xtables_target *target)
-{
-	/* Establish mask for comparison */
-	unsigned int size;
-	const struct xtables_rule_match *matchp;
-	unsigned char *mask, *mptr;
-
-	size = sizeof(struct ipt_entry);
-	for (matchp = matches; matchp; matchp = matchp->next)
-		size += XT_ALIGN(sizeof(struct xt_entry_match)) + matchp->match->size;
-
-	mask = xtables_calloc(1, size
-			 + XT_ALIGN(sizeof(struct xt_entry_target))
-			 + target->size);
-
-	memset(mask, 0xFF, sizeof(struct ipt_entry));
-	mptr = mask + sizeof(struct ipt_entry);
-
-	for (matchp = matches; matchp; matchp = matchp->next) {
-		memset(mptr, 0xFF,
-		       XT_ALIGN(sizeof(struct xt_entry_match))
-		       + matchp->match->userspacesize);
-		mptr += XT_ALIGN(sizeof(struct xt_entry_match)) + matchp->match->size;
-	}
-
-	memset(mptr, 0xFF,
-	       XT_ALIGN(sizeof(struct xt_entry_target))
-	       + target->userspacesize);
-
-	return mask;
-}
-
 static int
 delete_entry(const xt_chainlabel chain,
 	     struct ipt_entry *fw,
@@ -666,7 +632,7 @@ delete_entry(const xt_chainlabel chain,
 	int ret = 1;
 	unsigned char *mask;
 
-	mask = make_delete_mask(matches, target);
+	mask = make_delete_mask(matches, target, sizeof(*fw));
 	for (i = 0; i < nsaddrs; i++) {
 		fw->ip.src.s_addr = saddrs[i].s_addr;
 		fw->ip.smsk.s_addr = smasks[i].s_addr;
@@ -696,7 +662,7 @@ check_entry(const xt_chainlabel chain, struct ipt_entry *fw,
 	int ret = 1;
 	unsigned char *mask;
 
-	mask = make_delete_mask(matches, target);
+	mask = make_delete_mask(matches, target, sizeof(*fw));
 	for (i = 0; i < nsaddrs; i++) {
 		fw->ip.src.s_addr = saddrs[i].s_addr;
 		fw->ip.smsk.s_addr = smasks[i].s_addr;
diff --git a/iptables/xshared.c b/iptables/xshared.c
index c1d1371a6d54a..2438c4eeb5ff7 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -774,3 +774,37 @@ int parse_rulenumber(const char *rule)
 
 	return rulenum;
 }
+
+unsigned char *
+make_delete_mask(const struct xtables_rule_match *matches,
+		 const struct xtables_target *target,
+		 size_t entry_size)
+{
+	/* Establish mask for comparison */
+	unsigned int size = entry_size;
+	const struct xtables_rule_match *matchp;
+	unsigned char *mask, *mptr;
+
+	for (matchp = matches; matchp; matchp = matchp->next)
+		size += XT_ALIGN(sizeof(struct xt_entry_match)) + matchp->match->size;
+
+	mask = xtables_calloc(1, size
+			 + XT_ALIGN(sizeof(struct xt_entry_target))
+			 + target->size);
+
+	memset(mask, 0xFF, entry_size);
+	mptr = mask + entry_size;
+
+	for (matchp = matches; matchp; matchp = matchp->next) {
+		memset(mptr, 0xFF,
+		       XT_ALIGN(sizeof(struct xt_entry_match))
+		       + matchp->match->userspacesize);
+		mptr += XT_ALIGN(sizeof(struct xt_entry_match)) + matchp->match->size;
+	}
+
+	memset(mptr, 0xFF,
+	       XT_ALIGN(sizeof(struct xt_entry_target))
+	       + target->userspacesize);
+
+	return mask;
+}
diff --git a/iptables/xshared.h b/iptables/xshared.h
index c41bd054bf36f..eb908e484616e 100644
--- a/iptables/xshared.h
+++ b/iptables/xshared.h
@@ -216,4 +216,8 @@ void add_command(unsigned int *cmd, const int newcmd,
 		 const int othercmds, int invert);
 int parse_rulenumber(const char *rule);
 
+unsigned char *make_delete_mask(const struct xtables_rule_match *matches,
+				const struct xtables_target *target,
+				size_t entry_size);
+
 #endif /* IPTABLES_XSHARED_H */
-- 
2.25.1

[iptables PATCH 2/3] libxtables: Introduce 'matchmask' target callback

[Phil Sutter]

In order to support customized target data comparison, introduce a
callback which sets the matchmask used by libiptc's target_same()
function for the per-target part.

If defined, use this function in both legacy and nft variants.

Since this changes struct xtables_target, bump libxtables current and
reset age to zero.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 configure.ac          |  4 ++--
 include/xtables.h     |  3 +++
 iptables/nft-shared.c | 15 ++++++++++++++-
 iptables/xshared.c    | 10 +++++++---
 4 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/configure.ac b/configure.ac
index 02f6481ca52ed..5e330f3388de2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2,8 +2,8 @@
 AC_INIT([iptables], [1.8.4])
 
 # See libtool.info "Libtool's versioning system"
-libxtables_vcurrent=15
-libxtables_vage=3
+libxtables_vcurrent=16
+libxtables_vage=0
 
 AC_CONFIG_AUX_DIR([build-aux])
 AC_CONFIG_HEADERS([config.h])
diff --git a/include/xtables.h b/include/xtables.h
index 5044dd08e86d3..9483486bf7bae 100644
--- a/include/xtables.h
+++ b/include/xtables.h
@@ -356,6 +356,9 @@ struct xtables_target {
 	/* Print target name or alias */
 	const char *(*alias)(const struct xt_entry_target *target);
 
+	/* fill matchmask of size userspacesize */
+	void (*matchmask)(void *mask);
+
 	/* Pointer to list of extra command-line options */
 	const struct option *extra_opts;
 
diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index c5a8f3fcc051d..d2c252a3081b0 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -968,8 +968,21 @@ bool compare_targets(struct xtables_target *tg1, struct xtables_target *tg2)
 	if (strcmp(tg1->t->u.user.name, tg2->t->u.user.name) != 0)
 		return false;
 
-	if (memcmp(tg1->t->data, tg2->t->data, tg1->userspacesize) != 0)
+	if (tg1->matchmask) {
+		char *mask = xtables_malloc(tg1->userspacesize);
+		int i;
+
+		tg1->matchmask(mask);
+		for (i = 0; i < tg1->userspacesize; i++) {
+			if ((tg1->t->data[i] ^ tg2->t->data[i]) & mask[i]) {
+				free(mask);
+				return false;
+			}
+		}
+		free(mask);
+	} else if (memcmp(tg1->t->data, tg2->t->data, tg1->userspacesize)) {
 		return false;
+	}
 
 	return true;
 }
diff --git a/iptables/xshared.c b/iptables/xshared.c
index 2438c4eeb5ff7..d938fc5d1f9e2 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -802,9 +802,13 @@ make_delete_mask(const struct xtables_rule_match *matches,
 		mptr += XT_ALIGN(sizeof(struct xt_entry_match)) + matchp->match->size;
 	}
 
-	memset(mptr, 0xFF,
-	       XT_ALIGN(sizeof(struct xt_entry_target))
-	       + target->userspacesize);
+	memset(mptr, 0xFF, XT_ALIGN(sizeof(struct xt_entry_target)));
+	mptr += XT_ALIGN(sizeof(struct xt_entry_target));
+
+	if (target->matchmask)
+		target->matchmask(mptr);
+	else
+		memset(mptr, 0xFF, target->userspacesize);
 
 	return mask;
 }
-- 
2.25.1

[iptables PATCH 3/3] libxt_SECMARK: Fix for failing target comparison

[Phil Sutter]

The kernel fills in structxt_secmark_target_info->secid, so when the
rule is received from kernel it won't match a newly created one. This
prevented delete by rulespec and check commands.

Make use of newly introduced matchmask callback to prepare a mask which
explicitly excludes the secid field.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 extensions/libxt_SECMARK.c | 10 ++++++++++
 extensions/libxt_SECMARK.t |  4 ++++
 2 files changed, 14 insertions(+)
 create mode 100644 extensions/libxt_SECMARK.t

diff --git a/extensions/libxt_SECMARK.c b/extensions/libxt_SECMARK.c
index 6ba8606355daa..e9fd133642f00 100644
--- a/extensions/libxt_SECMARK.c
+++ b/extensions/libxt_SECMARK.c
@@ -6,6 +6,7 @@
  * Copyright (C) 2006 Red Hat, Inc., James Morris <jmorris@redhat.com>
  */
 #include <stdio.h>
+#include <string.h>
 #include <xtables.h>
 #include <linux/netfilter/xt_SECMARK.h>
 
@@ -68,6 +69,14 @@ static void SECMARK_save(const void *ip, const struct xt_entry_target *target)
 	print_secmark(info);
 }
 
+static void SECMARK_matchmask(void *mask)
+{
+	struct xt_secmark_target_info *info = mask;
+
+	memset(mask, 0xFF, XT_ALIGN(sizeof(struct xt_secmark_target_info)));
+	info->secid = 0;
+}
+
 static struct xtables_target secmark_target = {
 	.family		= NFPROTO_UNSPEC,
 	.name		= "SECMARK",
@@ -80,6 +89,7 @@ static struct xtables_target secmark_target = {
 	.save		= SECMARK_save,
 	.x6_parse	= SECMARK_parse,
 	.x6_options	= SECMARK_opts,
+	.matchmask	= SECMARK_matchmask,
 };
 
 void _init(void)
diff --git a/extensions/libxt_SECMARK.t b/extensions/libxt_SECMARK.t
new file mode 100644
index 0000000000000..39d4c09348bf4
--- /dev/null
+++ b/extensions/libxt_SECMARK.t
@@ -0,0 +1,4 @@
+:INPUT,FORWARD,OUTPUT
+*security
+-j SECMARK --selctx system_u:object_r:firewalld_exec_t:s0;=;OK
+-j SECMARK;;FAIL
-- 
2.25.1

Re: [iptables PATCH 0/3] Fix SECMARK target comparison

[Pablo Neira Ayuso]

On Tue, May 12, 2020 at 07:10:15PM +0200, Phil Sutter wrote:
> The kernel sets struct secmark_target_info->secid, so target comparison
> in user space failed every time. Given that target data comparison
> happens in libiptc, fixing this is a bit harder than just adding a cmp()
> callback to struct xtables_target. Instead, allow for targets to write
> the matchmask bits for their private data themselves and account for
> that in both legacy and nft code. Then make use of the new
> infrastructure to fix libxt_SECMARK.

Hm, -D and -C with SECMARK are broken since the beginning.

Another possible would be to fix the kernel to update the layout, to
get it aligned with other existing extensions.

Re: [iptables PATCH 0/3] Fix SECMARK target comparison

[Phil Sutter]

Hi Pablo,

On Thu, May 14, 2020 at 02:23:28PM +0200, Pablo Neira Ayuso wrote:
> On Tue, May 12, 2020 at 07:10:15PM +0200, Phil Sutter wrote:
> > The kernel sets struct secmark_target_info->secid, so target comparison
> > in user space failed every time. Given that target data comparison
> > happens in libiptc, fixing this is a bit harder than just adding a cmp()
> > callback to struct xtables_target. Instead, allow for targets to write
> > the matchmask bits for their private data themselves and account for
> > that in both legacy and nft code. Then make use of the new
> > infrastructure to fix libxt_SECMARK.
> 
> Hm, -D and -C with SECMARK are broken since the beginning.

Yes, sadly.

> Another possible would be to fix the kernel to update the layout, to
> get it aligned with other existing extensions.

You mean using 'usersize' just like e.g. xt_bpf.c?

One advantage of my fix is it works with old kernels as well.

Cheers, Phil