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>

diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index 6854d5b67c31b7c663c2652735b49e3f24e9d28f..7e659bb554a4635c63c154d555fe706f304719b2 100644 (file)
--- 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 601476ddb84ed325d9c80bebfc3b93ac5ff63d3a..1fe26bab4feb55efa3ce1e750515ef84e721c97c 100644 (file)
--- 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 145653d5a8f2cef9847023080d8ba39fa13f11bd..fb37b56ec23ce20766172f87bc86ebb7902cd956 100644 (file)
--- 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 c4d458d42018427530e590f2634e4ae5ac061894..6aa1cba3e5b9d4fe7562e37933a5659aff2d86b4 100644 (file)
--- 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);