iptables: fix regression with unintended free in need_protomatch

xtables_find_match memory allocation is funny. It can return something
allocated in a linked list or return a just allocated match clone and is
never freed.

This caused confusion and made a broken patch where an unintended free
is done in the case of entry not cloned.
xtables_find_match have a way to comunicate that the entry is cloned by
returning a looping linked list where the next entry is the same entry.
We can use this to understand where the entry has to be freed.

Fixes: ffba75c9cd8f ("iptables: free xtables_match if found in need_protomatch")
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
Tested-by: Rui Salvaterra <rsalvaterra@gmail.com>

diff --git a/defaults.c b/defaults.c
index 0580bfccccf0e5d4fb9b7704b2f405374cae015d..8a9a9295f00500ca7ba655de111acc643d41b214 100644 (file)
--- a/defaults.c
+++ b/defaults.c
@@ -91,8 +91,8 @@ check_target(struct uci_element *e, bool *available, const char *target, const b
        if (!b)
        {
                warn_elem(e, "requires unavailable target extension %s, disabling", target);
+               *available = false;
        }
-       *available = b;
 }
 
 static void
@@ -126,7 +126,6 @@ fw3_load_defaults(struct fw3_state *state, struct uci_package *p)
        struct uci_element *e;
        struct fw3_defaults *defs = &state->defaults;
 
-       bool flow_offload_avaliable = false;
        bool seen = false;
 
        defs->tcp_reject_code      = FW3_REJECT_CODE_TCP_RESET;
@@ -151,6 +150,8 @@ fw3_load_defaults(struct fw3_state *state, struct uci_package *p)
                        continue;
                }
 
+               seen = true;
+
                if(!fw3_parse_options(&state->defaults, fw3_flag_opts, s))
                        warn_elem(e, "has invalid options");
 
@@ -161,10 +162,7 @@ fw3_load_defaults(struct fw3_state *state, struct uci_package *p)
                check_any_reject_code(e, &defs->any_reject_code);
 
                /* exists in both ipv4 and ipv6, if at all, so only check ipv4 */
-               check_target(e, &flow_offload_avaliable, "FLOWOFFLOAD", false);
-
-               if (!flow_offload_avaliable)
-                       defs->flow_offloading = false;
+               check_target(e, &defs->flow_offloading, "FLOWOFFLOAD", false);
        }
 }
 
@@ -280,8 +278,7 @@ fw3_print_default_head_rules(struct fw3_ipt_handle *handle,
 
                if (defs->syn_flood)
                {
-                       r = fw3_ipt_rule_create(handle, &tcp, NULL, NULL, NULL, NULL);
-                       fw3_ipt_rule_extra(r, "--syn");
+                       r = fw3_ipt_rule_create(handle, NULL, NULL, NULL, NULL, NULL);
                        fw3_ipt_rule_limit(r, &defs->syn_flood_rate);
                        fw3_ipt_rule_target(r, "RETURN");
                        fw3_ipt_rule_append(r, "syn_flood");

diff --git a/ipsets.c b/ipsets.c
index 280845b9b7105c7aff6ec7b320cc647f01a7e775..e7cde16e930a438c8850a79b51b0e0cac65171bf 100644 (file)
--- a/ipsets.c
+++ b/ipsets.c
@@ -16,6 +16,8 @@
  * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
+#include <ctype.h>
+
 #include "ipsets.h"
 
 
@@ -264,6 +266,7 @@ fw3_alloc_ipset(struct fw3_state *state)
        ipset->enabled    = true;
        ipset->family     = FW3_FAMILY_V4;
        ipset->reload_set = false;
+       ipset->timeout    = -1; /* no timeout by default */
 
        list_add_tail(&ipset->list, &state->ipsets);
 
@@ -337,6 +340,7 @@ load_file(struct fw3_ipset *ipset)
 {
        FILE *f;
        char line[128];
+       char *p;
 
        if (!ipset->loadfile)
                return;
@@ -350,8 +354,13 @@ load_file(struct fw3_ipset *ipset)
                return;
        }
 
-       while (fgets(line, sizeof(line), f))
-               fw3_pr("add %s %s", ipset->name, line);
+       while (fgets(line, sizeof(line), f)) {
+               p = line;
+               while (isspace(*p))
+                       p++;
+               if (*p && *p != '#')
+                       fw3_pr("add %s %s", ipset->name, line);
+       }
 
        fclose(f);
 }
@@ -387,7 +396,7 @@ create_ipset(struct fw3_ipset *ipset, struct fw3_state *state)
                       ipset->portrange.port_min, ipset->portrange.port_max);
        }
 
-       if (ipset->timeout > 0)
+       if (ipset->timeout >= 0)
                fw3_pr(" timeout %u", ipset->timeout);
 
        if (ipset->maxelem > 0)

diff --git a/iptables.c b/iptables.c
index e7e8b597c0dd948dbe327e100f1c54710b83bc12..d03d1dd2933e8000712a0696c4e0d60fa6142347 100644 (file)
--- a/iptables.c
+++ b/iptables.c
@@ -709,12 +709,18 @@ init_match(struct fw3_ipt_rule *r, struct xtables_match *m, bool no_clone)
 static bool
 need_protomatch(struct fw3_ipt_rule *r, const char *pname)
 {
+       struct xtables_match *match;
+
        if (!pname)
                return false;
 
-       if (!xtables_find_match(pname, XTF_DONT_LOAD, NULL))
+       match = xtables_find_match(pname, XTF_DONT_LOAD, NULL);
+       if (!match)
                return true;
 
+       /* Free any kind of clone from xtables_find_match */
+       if (match == match->next)
+               free(match);
        return !r->protocol_loaded;
 }
 
@@ -1022,7 +1028,7 @@ fw3_ipt_rule_icmptype(struct fw3_ipt_rule *r, struct fw3_icmptype *icmp)
 #endif
        {
                if (icmp->code_min == 0 && icmp->code_max == 0xFF)
-                       sprintf(buf, "%u", icmp->type);
+                       snprintf(buf, sizeof(buf), "%u", icmp->type);
                else
                        snprintf(buf, sizeof(buf), "%u/%u", icmp->type, icmp->code_min);
 
@@ -1208,7 +1214,7 @@ fw3_ipt_rule_time(struct fw3_ipt_rule *r, struct fw3_time *time)
                                        rem--;
                                }
 
-                               p += snprintf(p, rem, "%u", i);
+                               len = snprintf(p, rem, "%u", i);
 
                                if (len < 0 || len >= rem)
                                        break;

diff --git a/main.c b/main.c
index 7ad00b424a7ce3355593ac35f8227ba4b47d2003..7deb6361287f1d3e3d091b29c2c276c6889311ad 100644 (file)
--- a/main.c
+++ b/main.c
@@ -195,9 +195,6 @@ stop(bool complete)
 
                for (table = FW3_TABLE_FILTER; table <= FW3_TABLE_RAW; table++)
                {
-                       if (!fw3_has_table(family == FW3_FAMILY_V6, fw3_flag_names[table]))
-                               continue;
-
                        if (!(handle = fw3_ipt_open(family, table)))
                                continue;
 
@@ -268,9 +265,6 @@ start(void)
 
                for (table = FW3_TABLE_FILTER; table <= FW3_TABLE_RAW; table++)
                {
-                       if (!fw3_has_table(family == FW3_FAMILY_V6, fw3_flag_names[table]))
-                               continue;
-
                        if (!(handle = fw3_ipt_open(family, table)))
                                continue;
 
@@ -339,9 +333,6 @@ reload(void)
 
                for (table = FW3_TABLE_FILTER; table <= FW3_TABLE_RAW; table++)
                {
-                       if (!fw3_has_table(family == FW3_FAMILY_V6, fw3_flag_names[table]))
-                               continue;
-
                        if (!(handle = fw3_ipt_open(family, table)))
                                continue;
 
@@ -368,9 +359,6 @@ start:
 
                for (table = FW3_TABLE_FILTER; table <= FW3_TABLE_RAW; table++)
                {
-                       if (!fw3_has_table(family == FW3_FAMILY_V6, fw3_flag_names[table]))
-                               continue;
-
                        if (!(handle = fw3_ipt_open(family, table)))
                                continue;
 
@@ -426,9 +414,6 @@ gc(void)
 
                for (table = FW3_TABLE_FILTER; table <= FW3_TABLE_RAW; table++)
                {
-                       if (!fw3_has_table(family == FW3_FAMILY_V6, fw3_flag_names[table]))
-                               continue;
-
                        if (!(handle = fw3_ipt_open(family, table)))
                                continue;
 

diff --git a/options.c b/options.c
index 61317860308d09e4ac0d14e2b3f309930d5a9cd0..2f419a3a9b45b63e88f1a739050fe3ba82841967 100644 (file)
--- a/options.c
+++ b/options.c
@@ -146,6 +146,7 @@ static const struct { const char *name; uint8_t dscp; } dscp_classes[] = {
        { "CS6",  0x30 },
        { "CS7",  0x38 },
        { "BE",   0x00 },
+       { "LE",   0x01 },
        { "AF11", 0x0a },
        { "AF12", 0x0c },
        { "AF13", 0x0e },

diff --git a/rules.c b/rules.c
index 181c6b1a126f4e4f1f7f23f5620bb20d1bd08c6a..d506a961d845dd71576c0b87177c3cdcf4c0d9ff 100644 (file)
--- a/rules.c
+++ b/rules.c
@@ -170,13 +170,6 @@ check_rule(struct fw3_state *state, struct fw3_rule *r, struct uci_element *e)
                return false;
        }
 
-       if (r->_dest && (r->target == FW3_FLAG_MARK || r->target == FW3_FLAG_DSCP))
-       {
-               warn_section("rule", r, e, "must not specify 'dest' for %s target",
-                            fw3_flag_names[r->target]);
-               return false;
-       }
-
        if (r->set_mark.invert || r->set_xmark.invert || r->set_dscp.invert)
        {
                warn_section("rule", r, e, "must not have inverted 'set_mark', "
@@ -309,10 +302,19 @@ append_chain(struct fw3_ipt_rule *r, struct fw3_rule *rule)
        {
                snprintf(chain, sizeof(chain), "zone_%s_helper", rule->src.name);
        }
-       else if ((rule->target == FW3_FLAG_MARK || rule->target == FW3_FLAG_DSCP) &&
-                (rule->_src || rule->src.any))
+       else if (rule->target == FW3_FLAG_MARK || rule->target == FW3_FLAG_DSCP)
        {
-               snprintf(chain, sizeof(chain), "PREROUTING");
+               if ((rule->_dest && rule->_src) ||
+                   (rule->dest.any && rule->src.any))
+                       snprintf(chain, sizeof(chain), "FORWARD");
+               else if (rule->src.any && rule->_dest)
+                       snprintf(chain, sizeof(chain), "POSTROUTING");
+               else if (rule->dest.any && rule->_src)
+                       snprintf(chain, sizeof(chain), "PREROUTING");
+               else if (!rule->dest.set && rule->src.set)
+                       snprintf(chain, sizeof(chain), "INPUT");
+               else /* if (!rule->src.set) */
+                       snprintf(chain, sizeof(chain), "OUTPUT");
        }
        else
        {
@@ -420,6 +422,10 @@ print_rule(struct fw3_ipt_handle *handle, struct fw3_state *state,
            struct fw3_mac *mac, struct fw3_icmptype *icmptype)
 {
        struct fw3_ipt_rule *r;
+       struct fw3_device *idev, *odev;
+       struct list_head empty, *idevices, *odevices;
+       INIT_LIST_HEAD(&empty);
+       idevices = odevices = ∅
 
        if (!fw3_is_family(sip, handle->family) ||
            !fw3_is_family(dip, handle->family))
@@ -471,21 +477,33 @@ print_rule(struct fw3_ipt_handle *handle, struct fw3_state *state,
                return;
        }
 
-       r = fw3_ipt_rule_create(handle, proto, NULL, NULL, sip, dip);
-       fw3_ipt_rule_sport_dport(r, sport, dport);
-       fw3_ipt_rule_device(r, rule->device, rule->direction_out);
-       fw3_ipt_rule_icmptype(r, icmptype);
-       fw3_ipt_rule_mac(r, mac);
-       fw3_ipt_rule_ipset(r, &rule->ipset);
-       fw3_ipt_rule_helper(r, &rule->helper);
-       fw3_ipt_rule_limit(r, &rule->limit);
-       fw3_ipt_rule_time(r, &rule->time);
-       fw3_ipt_rule_mark(r, &rule->mark);
-       fw3_ipt_rule_dscp(r, &rule->dscp);
-       set_target(r, rule);
-       fw3_ipt_rule_extra(r, rule->extra);
-       set_comment(r, rule->name, num);
-       append_chain(r, rule);
+       if (rule->target == FW3_FLAG_DSCP || rule->target == FW3_FLAG_MARK)
+       {
+               if (rule->_src)
+                       idevices = &rule->_src->devices;
+               if (rule->_dest)
+                       odevices = &rule->_dest->devices;
+       }
+
+       fw3_foreach(idev, idevices)
+       fw3_foreach(odev, odevices)
+       {
+               r = fw3_ipt_rule_create(handle, proto, idev, odev, sip, dip);
+               fw3_ipt_rule_sport_dport(r, sport, dport);
+               fw3_ipt_rule_device(r, rule->device, rule->direction_out);
+               fw3_ipt_rule_icmptype(r, icmptype);
+               fw3_ipt_rule_mac(r, mac);
+               fw3_ipt_rule_ipset(r, &rule->ipset);
+               fw3_ipt_rule_helper(r, &rule->helper);
+               fw3_ipt_rule_limit(r, &rule->limit);
+               fw3_ipt_rule_time(r, &rule->time);
+               fw3_ipt_rule_mark(r, &rule->mark);
+               fw3_ipt_rule_dscp(r, &rule->dscp);
+               set_target(r, rule);
+               fw3_ipt_rule_extra(r, rule->extra);
+               set_comment(r, rule->name, num);
+               append_chain(r, rule);
+       }
 }
 
 static void

diff --git a/utils.c b/utils.c
index 5667bcfd76db5d91191a90e0e705b51d2978745f..faa51a1a589b277421f1e5c16ee35d43eea89d92 100644 (file)
--- a/utils.c
+++ b/utils.c
@@ -315,23 +315,19 @@ fw3_command_close(void)
        pipe_pid = -1;
 }
 
-bool
-fw3_has_table(bool ipv6, const char *table)
+static bool
+file_contains(const char *path, const char *str)
 {
        FILE *f;
-
        char line[12];
        bool seen = false;
 
-       const char *path = ipv6
-               ? "/proc/net/ip6_tables_names" : "/proc/net/ip_tables_names";
-
        if (!(f = fopen(path, "r")))
                return false;
 
        while (fgets(line, sizeof(line), f))
        {
-               if (!strncmp(line, table, strlen(table)))
+               if (!strncmp(line, str, strlen(str)))
                {
                        seen = true;
                        break;
@@ -346,29 +342,10 @@ fw3_has_table(bool ipv6, const char *table)
 bool
 fw3_has_target(const bool ipv6, const char *target)
 {
-       FILE *f;
-
-       char line[12];
-       bool seen = false;
-
        const char *path = ipv6
                ? "/proc/net/ip6_tables_targets" : "/proc/net/ip_tables_targets";
 
-       if (!(f = fopen(path, "r")))
-               return false;
-
-       while (fgets(line, sizeof(line), f))
-       {
-               if (!strcmp(line, target))
-               {
-                       seen = true;
-                       break;
-               }
-       }
-
-       fclose(f);
-
-       return seen;
+       return file_contains(path, target);
 }
 
 bool
@@ -411,7 +388,6 @@ fw3_unlock_path(int *fd, const char *lockpath)
                warn("Cannot release exclusive lock: %s", strerror(errno));
 
        close(*fd);
-       unlink(FW3_LOCKFILE);
 
        *fd = -1;
 }

diff --git a/utils.h b/utils.h
index 254bea45d48b207ba46a3c427d47d327814195ee..5b17a2d75567a2e192a9b95226c1fd0cfbcae024 100644 (file)
--- a/utils.h
+++ b/utils.h
@@ -55,7 +55,6 @@ void error(const char *format, ...)
 void info(const char *format, ...)
        __attribute__ ((format (printf, 1, 2)));
 
-
 #define warn_section(t, r, e, fmt, ...)                                        \
        do {                                                                    \
                if (e)                                                          \
@@ -103,8 +102,6 @@ void fw3_command_close(void);
 void fw3_pr(const char *fmt, ...)
        __attribute__ ((format (printf, 1, 2)));
 
-bool fw3_has_table(bool ipv6, const char *table);
-
 bool fw3_has_target(const bool ipv6, const char *target);
 
 bool fw3_lock(void);

diff --git a/zones.c b/zones.c
index d45077af615abde04781928f35e4e377e7505b13..51a8fdf6af58bb51a7ab39d7f04c50145cb9a725 100644 (file)
--- a/zones.c
+++ b/zones.c
@@ -137,7 +137,7 @@ check_masq_addrs(struct list_head *head)
 static void
 resolve_networks(struct uci_element *e, struct fw3_zone *zone)
 {
-       struct fw3_device *net, *tmp;
+       struct fw3_device *net, *dev, *tmp;
 
        list_for_each_entry(net, &zone->networks, list)
        {
@@ -149,8 +149,15 @@ resolve_networks(struct uci_element *e, struct fw3_zone *zone)
                        continue;
                }
 
+               list_for_each_entry(dev, &zone->devices, list)
+                       if (!strcmp(dev->name, tmp->name))
+                               goto alias;
+
                snprintf(tmp->network, sizeof(tmp->network), "%s", net->name);
                list_add_tail(&tmp->list, &zone->devices);
+               continue;
+alias:
+               free(tmp);
        }
 }
 

diff --git a/zones.h b/zones.h
index d78673664cd73c9e956695956235539be5d6237f..beb0e220bca1472eaf5023453dd59c6182a6d732 100644 (file)
--- a/zones.h
+++ b/zones.h
@@ -22,8 +22,12 @@
 #include "options.h"
 #include "iptables.h"
 
-/* 32 - sizeof("postrouting_") - sizeof("_rule") - sizeof("\0") */
-#define FW3_ZONE_MAXNAMELEN 14
+/* XT_EXTENSION_MAXNAMELEN (29)
+ *  - sizeof("postrouting_")
+ *  - sizeof("_rule")
+ *  - sizeof("\0")
+ */
+#define FW3_ZONE_MAXNAMELEN 11
 
 extern const struct fw3_option fw3_zone_opts[];