ebtables: Review match/target lookup once more

This is a partial revert of my previous commit with similar subject - it
missed to apply the needed changes to ebtables-translate as well and on
top of that still left some leaks and use-after-frees in place. The new
strategy is to make ebtables extension loading compatible with that of
xtables, because otherwise the heavy code sharing between
ebtables-translate and iptables-translate will cause trouble.

Basically, ebt_add_match() and ebt_add_watcher() copy what xtables'
command_match() does, but after the actual extension argument parsing
has already happened. Therefore they duplicate the loaded match along
with its data and reset the original one to default state for being
reused (e.g., by ebtables-restore). Since mflags/tflags are cleared
while doing so, clearing them for all loaded extensions in
do_commandeb() is not necessary anymore.

In ebt_command_default() (where extension parameter parsing happens),
the list of added extensions to the current rule are consolidated first
so no duplicate extension loading happens.

With the above in place, ebt_cs_clean() can be reverted to its old
state.

Apart from sharing command_jump() function with ebtables-translate, make
use of nft_init_eb() there, as well.

Fixes: aa7fb04fcf72c ("ebtables: Review match/target lookup")
Signed-off-by: Phil Sutter <phil@nwl.cc>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

4 files changed, 58 insertions, 165 deletions

diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index 6854d5b6..7e659bb5 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -25,45 +25,25 @@
 void ebt_cs_clean(struct iptables_command_state *cs)
 {
 	struct ebt_match *m, *nm;
-	struct xtables_rule_match *matchp, *tmp;
 
-	for (matchp = cs->matches; matchp;) {
-		tmp = matchp->next;
-
-		if (matchp->match == matchp->match->next) {
-			free(matchp->match);
-			matchp->match = NULL;
-		}
-		free(matchp);
-		matchp = tmp;
-	}
+	xtables_rule_matches_free(&cs->matches);
 
 	for (m = cs->match_list; m;) {
-		if (m->ismatch) {
-			struct xtables_match *match = m->u.match;
-
-			memset(match->m->data, 0,
-			       match->m->u.match_size - sizeof(*match->m));
-			if (match->init)
-				match->init(match->m);
-		} else {
+		if (!m->ismatch) {
 			struct xtables_target *target = m->u.watcher;
 
-			memset(target->t->data, 0,
-			       target->t->u.target_size - sizeof(*target->t));
-			if (target->init)
-				target->init(target->t);
+			if (target->t) {
+				free(target->t);
+				target->t = NULL;
+			}
+			if (target == target->next)
+				free(target);
 		}
 
 		nm = m->next;
 		free(m);
 		m = nm;
 	}
-
-	if (cs->target) {
-		if (cs->target->udata_size)
-			free(cs->target->udata);
-	}
 }
 
 /* 0: default, print only 2 digits if necessary
diff --git a/iptables/nft-bridge.h b/iptables/nft-bridge.h
index 601476dd..1fe26bab 100644
--- a/iptables/nft-bridge.h
+++ b/iptables/nft-bridge.h
@@ -121,5 +121,6 @@ void ebt_add_match(struct xtables_match *m,
 void ebt_add_watcher(struct xtables_target *watcher,
                      struct iptables_command_state *cs);
 int ebt_command_default(struct iptables_command_state *cs);
+struct xtables_target *ebt_command_jump(const char *jumpto);
 
 #endif
diff --git a/iptables/xtables-eb-translate.c b/iptables/xtables-eb-translate.c
index 145653d5..fb37b56e 100644
--- a/iptables/xtables-eb-translate.c
+++ b/iptables/xtables-eb-translate.c
@@ -130,72 +130,6 @@ extern struct xtables_globals ebtables_globals;
 #define prog_name ebtables_globals.program_name
 #define prog_vers ebtables_globals.program_version
 
-#define OPTION_OFFSET 256
-static struct option *merge_options(struct option *oldopts,
-				    const struct option *newopts,
-				    unsigned int *options_offset)
-{
-	unsigned int num_old, num_new, i;
-	struct option *merge;
-
-	if (!newopts || !oldopts || !options_offset)
-		return oldopts;
-	for (num_old = 0; oldopts[num_old].name; num_old++);
-	for (num_new = 0; newopts[num_new].name; num_new++);
-
-	ebtables_globals.option_offset += OPTION_OFFSET;
-	*options_offset = ebtables_globals.option_offset;
-
-	merge = malloc(sizeof(struct option) * (num_new + num_old + 1));
-	if (!merge)
-		return NULL;
-	memcpy(merge, oldopts, num_old * sizeof(struct option));
-	for (i = 0; i < num_new; i++) {
-		merge[num_old + i] = newopts[i];
-		merge[num_old + i].val += *options_offset;
-	}
-	memset(merge + num_old + num_new, 0, sizeof(struct option));
-	/* Only free dynamically allocated stuff */
-	if (oldopts != ebt_original_options)
-		free(oldopts);
-
-	return merge;
-}
-
-/*
- * More glue code.
- */
-static struct xtables_target *command_jump(struct iptables_command_state *cs,
-					   const char *jumpto)
-{
-	struct xtables_target *target;
-	size_t size;
-
-	/* XTF_TRY_LOAD (may be chain name) */
-	target = xtables_find_target(jumpto, XTF_TRY_LOAD);
-
-	if (!target)
-		return NULL;
-
-	size = XT_ALIGN(sizeof(struct xt_entry_target))
-		+ target->size;
-
-	target->t = xtables_calloc(1, size);
-	target->t->u.target_size = size;
-	snprintf(target->t->u.user.name,
-		 sizeof(target->t->u.user.name), "%s", jumpto);
-	target->t->u.user.name[sizeof(target->t->u.user.name)-1] = '\0';
-	target->t->u.user.revision = target->revision;
-
-	xs_init_target(target);
-
-	opts = merge_options(opts, target->extra_opts, &target->option_offset);
-	if (opts == NULL)
-		xtables_error(OTHER_PROBLEM, "Can't alloc memory");
-
-	return target;
-}
-
 static void print_help(void)
 {
 	fprintf(stderr, "%s: Translate ebtables command to nft syntax\n"
@@ -286,8 +220,6 @@ static int do_commandeb_xlate(struct nft_handle *h, int argc, char *argv[], char
 	int rule_nr_end = 0;
 	int ret = 0;
 	unsigned int flags = 0;
-	struct xtables_target *t;
-	struct xtables_match *m;
 	struct iptables_command_state cs = {
 		.argv		= argv,
 		.eb.bitmask	= EBT_NOPROTO,
@@ -302,30 +234,6 @@ static int do_commandeb_xlate(struct nft_handle *h, int argc, char *argv[], char
 		.table          = *table,
         };
 
-	if (nft_init(h, xtables_bridge) < 0)
-		xtables_error(OTHER_PROBLEM,
-			      "Could not initialize nftables layer.");
-
-	h->ops = nft_family_ops_lookup(h->family);
-	if (h->ops == NULL)
-		xtables_error(PARAMETER_PROBLEM, "Unknown family");
-
-	/* manually registering ebt matches, given the original ebtables parser
-	 * don't use '-m matchname' and the match can't loaded dinamically when
-	 * the user calls it.
-	 */
-	ebt_load_match_extensions();
-
-	/* clear mflags in case do_commandeb gets called a second time
-	 * (we clear the global list of all matches for security)*/
-	for (m = xtables_matches; m; m = m->next)
-		m->mflags = 0;
-
-	for (t = xtables_targets; t; t = t->next) {
-		t->tflags = 0;
-		t->used = 0;
-	}
-
 	/* prevent getopt to spoil our error reporting */
 	opterr = false;
 
@@ -506,7 +414,7 @@ print_zero:
 			} else if (c == 'j') {
 				ebt_check_option2(&flags, OPT_JUMP);
 				cs.jumpto = parse_target(optarg);
-				cs.target = command_jump(&cs, cs.jumpto);
+				cs.target = ebt_command_jump(cs.jumpto);
 				break;
 			} else if (c == 's') {
 				ebt_check_option2(&flags, OPT_SOURCE);
@@ -678,20 +586,11 @@ int xtables_eb_xlate_main(int argc, char *argv[])
 {
 	int ret;
 	char *table = "filter";
-	struct nft_handle h = {
-		.family = NFPROTO_BRIDGE,
-	};
-
-	ebtables_globals.program_name = argv[0];
-	ret = xtables_init_all(&ebtables_globals, NFPROTO_BRIDGE);
-	if (ret < 0) {
-		fprintf(stderr, "%s/%s Failed to initialize xtables\n",
-			ebtables_globals.program_name,
-			ebtables_globals.program_version);
-		exit(EXIT_FAILURE);
-	}
+	struct nft_handle h;
 
+	nft_init_eb(&h, argv[0]);
 	ebtables_globals.compat_rev = dummy_compat_rev;
+
 	ret = do_commandeb_xlate(&h, argc, argv, &table);
 	if (!ret)
 		fprintf(stderr, "Translation not implemented\n");
diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c
index c4d458d4..6aa1cba3 100644
--- a/iptables/xtables-eb.c
+++ b/iptables/xtables-eb.c
@@ -380,7 +380,7 @@ static struct option *merge_options(struct option *oldopts,
 /*
  * More glue code.
  */
-static struct xtables_target *command_jump(const char *jumpto)
+struct xtables_target *ebt_command_jump(const char *jumpto)
 {
 	struct xtables_target *target;
 	unsigned int verdict;
@@ -663,24 +663,24 @@ void ebt_load_match_extensions(void)
 void ebt_add_match(struct xtables_match *m,
 		   struct iptables_command_state *cs)
 {
-	struct xtables_rule_match *i, **rule_matches = &cs->matches;
+	struct xtables_rule_match **rule_matches = &cs->matches;
 	struct xtables_match *newm;
 	struct ebt_match *newnode, **matchp;
-
-	/* match already in rule_matches, skip inclusion */
-	for (i = *rule_matches; i; i = i->next) {
-		if (strcmp(m->name, i->match->name) == 0) {
-			i->match->mflags |= m->mflags;
-			return;
-		}
-	}
+	struct xt_entry_match *m2;
 
 	newm = xtables_find_match(m->name, XTF_LOAD_MUST_SUCCEED, rule_matches);
 	if (newm == NULL)
 		xtables_error(OTHER_PROBLEM,
 			      "Unable to add match %s", m->name);
 
+	m2 = xtables_calloc(1, newm->m->u.match_size);
+	memcpy(m2, newm->m, newm->m->u.match_size);
+	memset(newm->m->data, 0, newm->size);
+	xs_init_match(newm);
+	newm->m = m2;
+
 	newm->mflags = m->mflags;
+	m->mflags = 0;
 
 	/* glue code for watchers */
 	newnode = calloc(1, sizeof(struct ebt_match));
@@ -698,22 +698,28 @@ void ebt_add_match(struct xtables_match *m,
 void ebt_add_watcher(struct xtables_target *watcher,
 		     struct iptables_command_state *cs)
 {
-	struct ebt_match *i, *newnode, **matchp;
+	struct ebt_match *newnode, **matchp;
+	struct xtables_target *clone;
+
+	clone = xtables_malloc(sizeof(struct xtables_target));
+	memcpy(clone, watcher, sizeof(struct xtables_target));
+	clone->udata = NULL;
+	clone->tflags = watcher->tflags;
+	clone->next = clone;
+
+	clone->t = xtables_calloc(1, watcher->t->u.target_size);
+	memcpy(clone->t, watcher->t, watcher->t->u.target_size);
+
+	memset(watcher->t->data, 0, watcher->size);
+	xs_init_target(watcher);
+	watcher->tflags = 0;
 
-	for (i = cs->match_list; i; i = i->next) {
-		if (i->ismatch)
-			continue;
-		if (strcmp(i->u.watcher->name, watcher->name) == 0) {
-			i->u.watcher->tflags |= watcher->tflags;
-			return;
-		}
-	}
 
 	newnode = calloc(1, sizeof(struct ebt_match));
 	if (newnode == NULL)
 		xtables_error(OTHER_PROBLEM, "Unable to alloc memory");
 
-	newnode->u.watcher = watcher;
+	newnode->u.watcher = clone;
 
 	for (matchp = &cs->match_list; *matchp; matchp = &(*matchp)->next)
 		;
@@ -724,6 +730,7 @@ int ebt_command_default(struct iptables_command_state *cs)
 {
 	struct xtables_target *t = cs->target;
 	struct xtables_match *m;
+	struct ebt_match *matchp;
 
 	/* Is it a target option? */
 	if (t && t->parse) {
@@ -732,6 +739,23 @@ int ebt_command_default(struct iptables_command_state *cs)
 			return 0;
 	}
 
+	/* check previously added matches/watchers to this rule first */
+	for (matchp = cs->match_list; matchp; matchp = matchp->next) {
+		if (matchp->ismatch) {
+			m = matchp->u.match;
+			if (m->parse &&
+			    m->parse(cs->c - m->option_offset, cs->argv,
+				     ebt_invert, &m->mflags, NULL, &m->m))
+				return 0;
+		} else {
+			t = matchp->u.watcher;
+			if (t->parse &&
+			    t->parse(cs->c - t->option_offset, cs->argv,
+				     ebt_invert, &t->tflags, NULL, &t->t))
+				return 0;
+		}
+	}
+
 	/* Is it a match_option? */
 	for (m = xtables_matches; m; m = m->next) {
 		if (m->parse &&
@@ -800,7 +824,6 @@ int do_commandeb(struct nft_handle *h, int argc, char *argv[], char **table,
 	int ret = 0;
 	unsigned int flags = 0;
 	struct xtables_target *t;
-	struct xtables_match *m;
 	struct iptables_command_state cs = {
 		.argv = argv,
 		.eb.bitmask = EBT_NOPROTO,
@@ -812,16 +835,6 @@ int do_commandeb(struct nft_handle *h, int argc, char *argv[], char **table,
 	struct xtables_rule_match *xtrm_i;
 	struct ebt_match *match;
 
-	/* clear mflags in case do_commandeb gets called a second time
-	 * (we clear the global list of all matches for security)*/
-	for (m = xtables_matches; m; m = m->next)
-		m->mflags = 0;
-
-	for (t = xtables_targets; t; t = t->next) {
-		t->tflags = 0;
-		t->used = 0;
-	}
-
 	/* prevent getopt to spoil our error reporting */
 	optind = 0;
 	opterr = false;
@@ -1057,7 +1070,7 @@ print_zero:
 			} else if (c == 'j') {
 				ebt_check_option2(&flags, OPT_JUMP);
 				cs.jumpto = parse_target(optarg);
-				cs.target = command_jump(cs.jumpto);
+				cs.target = ebt_command_jump(cs.jumpto);
 				break;
 			} else if (c == 's') {
 				ebt_check_option2(&flags, OPT_SOURCE);