From 256b70dfaaa027a961205b08cd671f1fccf9b663 Mon Sep 17 00:00:00 2001

From: Eric Garver <eric@garver.life>

Date: Thu, 10 Jan 2019 13:02:34 -0500

Subject: [PATCH 18/23] ipXtables: Avoid inserting rules with index

iptables-restore (nftables) has a bug in which inserting by index

doesn't always work as expected. Rules may be inserted at the wrong

index. We can mostly avoid this by appending rules. This actually

simplifies things because we don't have to count indexes. Ref rhbz

1647925.

(cherry picked from commit 9f0eb929c240211d3da978732a9f6930da71486c)

---

 src/firewall/core/fw_direct.py |   5 +-

 src/firewall/core/ipXtables.py | 256 ++++++++++++++++-----------------

 2 files changed, 132 insertions(+), 129 deletions(-)

diff --git a/src/firewall/core/fw_direct.py b/src/firewall/core/fw_direct.py

index 94196e8a41fa..e53a72e3326a 100644

--- a/src/firewall/core/fw_direct.py

+++ b/src/firewall/core/fw_direct.py

@@ -181,7 +181,10 @@ class FirewallDirect(object):

     def _check_builtin_chain(self, ipv, table, chain):

         if ipv in ['ipv4', 'ipv6']:

             built_in_chains = ipXtables.BUILT_IN_CHAINS[table]

-            our_chains = ipXtables.OUR_CHAINS[table]

+            if self._fw.nftables_enabled:

+                our_chains = {}

+            else:

+                our_chains = self._fw.get_direct_backend_by_ipv(ipv).our_chains[table]

         else:

             built_in_chains = ebtables.BUILT_IN_CHAINS[table]

             our_chains = ebtables.OUR_CHAINS[table]

diff --git a/src/firewall/core/ipXtables.py b/src/firewall/core/ipXtables.py

index 2bd8cc20dc7b..4f04ac41f6a0 100644

--- a/src/firewall/core/ipXtables.py

+++ b/src/firewall/core/ipXtables.py

@@ -49,106 +49,6 @@ ICMP = {

     "ipv6": "ipv6-icmp",

 }

-DEFAULT_RULES = { }

-LOG_RULES = { }

-OUR_CHAINS = {} # chains created by firewalld

-

-DEFAULT_RULES["security"] = [ ]

-OUR_CHAINS["security"] = set()

-for chain in BUILT_IN_CHAINS["security"]:

-    DEFAULT_RULES["security"].append("-N %s_direct" % chain)

-    DEFAULT_RULES["security"].append("-I %s 1 -j %s_direct" % (chain, chain))

-    OUR_CHAINS["security"].add("%s_direct" % chain)

-

-DEFAULT_RULES["raw"] = [ ]

-OUR_CHAINS["raw"] = set()

-for chain in BUILT_IN_CHAINS["raw"]:

-    DEFAULT_RULES["raw"].append("-N %s_direct" % chain)

-    DEFAULT_RULES["raw"].append("-I %s 1 -j %s_direct" % (chain, chain))

-    OUR_CHAINS["raw"].add("%s_direct" % chain)

-

-    if chain == "PREROUTING":

-        DEFAULT_RULES["raw"].append("-N %s_ZONES_SOURCE" % chain)

-        DEFAULT_RULES["raw"].append("-N %s_ZONES" % chain)

-        DEFAULT_RULES["raw"].append("-I %s 2 -j %s_ZONES_SOURCE" % (chain, chain))

-        DEFAULT_RULES["raw"].append("-I %s 3 -j %s_ZONES" % (chain, chain))

-        OUR_CHAINS["raw"].update(set(["%s_ZONES_SOURCE" % chain, "%s_ZONES" % chain]))

-

-DEFAULT_RULES["mangle"] = [ ]

-OUR_CHAINS["mangle"] = set()

-for chain in BUILT_IN_CHAINS["mangle"]:

-    DEFAULT_RULES["mangle"].append("-N %s_direct" % chain)

-    DEFAULT_RULES["mangle"].append("-I %s 1 -j %s_direct" % (chain, chain))

-    OUR_CHAINS["mangle"].add("%s_direct" % chain)

-

-    if chain == "PREROUTING":

-        DEFAULT_RULES["mangle"].append("-N %s_ZONES_SOURCE" % chain)

-        DEFAULT_RULES["mangle"].append("-N %s_ZONES" % chain)

-        DEFAULT_RULES["mangle"].append("-I %s 2 -j %s_ZONES_SOURCE" % (chain, chain))

-        DEFAULT_RULES["mangle"].append("-I %s 3 -j %s_ZONES" % (chain, chain))

-        OUR_CHAINS["mangle"].update(set(["%s_ZONES_SOURCE" % chain, "%s_ZONES" % chain]))

-

-DEFAULT_RULES["nat"] = [ ]

-OUR_CHAINS["nat"] = set()

-for chain in BUILT_IN_CHAINS["nat"]:

-    DEFAULT_RULES["nat"].append("-N %s_direct" % chain)

-    DEFAULT_RULES["nat"].append("-I %s 1 -j %s_direct" % (chain, chain))

-    OUR_CHAINS["nat"].add("%s_direct" % chain)

-

-    if chain in [ "PREROUTING", "POSTROUTING" ]:

-        DEFAULT_RULES["nat"].append("-N %s_ZONES_SOURCE" % chain)

-        DEFAULT_RULES["nat"].append("-N %s_ZONES" % chain)

-        DEFAULT_RULES["nat"].append("-I %s 2 -j %s_ZONES_SOURCE" % (chain, chain))

-        DEFAULT_RULES["nat"].append("-I %s 3 -j %s_ZONES" % (chain, chain))

-        OUR_CHAINS["nat"].update(set(["%s_ZONES_SOURCE" % chain, "%s_ZONES" % chain]))

-

-DEFAULT_RULES["filter"] = [

-    "-N INPUT_direct",

-    "-N INPUT_ZONES_SOURCE",

-    "-N INPUT_ZONES",

-

-    "-I INPUT 1 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT",

-    "-I INPUT 2 -i lo -j ACCEPT",

-    "-I INPUT 3 -j INPUT_direct",

-    "-I INPUT 4 -j INPUT_ZONES_SOURCE",

-    "-I INPUT 5 -j INPUT_ZONES",

-    "-I INPUT 6 -m conntrack --ctstate INVALID -j DROP",

-    "-I INPUT 7 -j %%REJECT%%",

-

-    "-N FORWARD_direct",

-    "-N FORWARD_IN_ZONES_SOURCE",

-    "-N FORWARD_IN_ZONES",

-    "-N FORWARD_OUT_ZONES_SOURCE",

-    "-N FORWARD_OUT_ZONES",

-

-    "-I FORWARD 1 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT",

-    "-I FORWARD 2 -i lo -j ACCEPT",

-    "-I FORWARD 3 -j FORWARD_direct",

-    "-I FORWARD 4 -j FORWARD_IN_ZONES_SOURCE",

-    "-I FORWARD 5 -j FORWARD_IN_ZONES",

-    "-I FORWARD 6 -j FORWARD_OUT_ZONES_SOURCE",

-    "-I FORWARD 7 -j FORWARD_OUT_ZONES",

-    "-I FORWARD 8 -m conntrack --ctstate INVALID -j DROP",

-    "-I FORWARD 9 -j %%REJECT%%",

-

-    "-N OUTPUT_direct",

-

-    "-I OUTPUT 1 -j OUTPUT_direct",

-]

-

-LOG_RULES["filter"] = [

-    "-I INPUT 6 -m conntrack --ctstate INVALID %%LOGTYPE%% -j LOG --log-prefix 'STATE_INVALID_DROP: '",

-    "-I INPUT 8 %%LOGTYPE%% -j LOG --log-prefix 'FINAL_REJECT: '",

-

-    "-I FORWARD 8 -m conntrack --ctstate INVALID %%LOGTYPE%% -j LOG --log-prefix 'STATE_INVALID_DROP: '",

-    "-I FORWARD 10 %%LOGTYPE%% -j LOG --log-prefix 'FINAL_REJECT: '",

-]

-

-OUR_CHAINS["filter"] = set(["INPUT_direct", "INPUT_ZONES_SOURCE", "INPUT_ZONES",

-                            "FORWARD_direct", "FORWARD_IN_ZONES_SOURCE",

-                            "FORWARD_IN_ZONES", "FORWARD_OUT_ZONES_SOURCE",

-                            "FORWARD_OUT_ZONES", "OUTPUT_direct"])

-

 # ipv ebtables also uses this

 #

 def common_reverse_rule(args):

@@ -275,6 +175,7 @@ class ip4tables(object):

         self.restore_wait_option = self._detect_restore_wait_option()

         self.fill_exists()

         self.available_tables = []

+        self.our_chains = {} # chains created by firewalld

     def fill_exists(self):

         self.command_exists = os.path.exists(self._command)

@@ -602,20 +503,117 @@ class ip4tables(object):

         return []

     def build_default_rules(self, log_denied="off"):

-        default_rules = []

-        for table in DEFAULT_RULES:

+        default_rules = {}

+

+        default_rules["security"] = [ ]

+        self.our_chains["security"] = set()

+        for chain in BUILT_IN_CHAINS["security"]:

+            default_rules["security"].append("-N %s_direct" % chain)

+            default_rules["security"].append("-A %s -j %s_direct" % (chain, chain))

+            self.our_chains["security"].add("%s_direct" % chain)

+

+        default_rules["raw"] = [ ]

+        self.our_chains["raw"] = set()

+        for chain in BUILT_IN_CHAINS["raw"]:

+            default_rules["raw"].append("-N %s_direct" % chain)

+            default_rules["raw"].append("-A %s -j %s_direct" % (chain, chain))

+            self.our_chains["raw"].add("%s_direct" % chain)

+

+            if chain == "PREROUTING":

+                default_rules["raw"].append("-N %s_ZONES_SOURCE" % chain)

+                default_rules["raw"].append("-N %s_ZONES" % chain)

+                default_rules["raw"].append("-A %s -j %s_ZONES_SOURCE" % (chain, chain))

+                default_rules["raw"].append("-A %s -j %s_ZONES" % (chain, chain))

+                self.our_chains["raw"].update(set(["%s_ZONES_SOURCE" % chain, "%s_ZONES" % chain]))

+

+        default_rules["mangle"] = [ ]

+        self.our_chains["mangle"] = set()

+        for chain in BUILT_IN_CHAINS["mangle"]:

+            default_rules["mangle"].append("-N %s_direct" % chain)

+            default_rules["mangle"].append("-A %s -j %s_direct" % (chain, chain))

+            self.our_chains["mangle"].add("%s_direct" % chain)

+

+            if chain == "PREROUTING":

+                default_rules["mangle"].append("-N %s_ZONES_SOURCE" % chain)

+                default_rules["mangle"].append("-N %s_ZONES" % chain)

+                default_rules["mangle"].append("-A %s -j %s_ZONES_SOURCE" % (chain, chain))

+                default_rules["mangle"].append("-A %s -j %s_ZONES" % (chain, chain))

+                self.our_chains["mangle"].update(set(["%s_ZONES_SOURCE" % chain, "%s_ZONES" % chain]))

+

+        default_rules["nat"] = [ ]

+        self.our_chains["nat"] = set()

+        for chain in BUILT_IN_CHAINS["nat"]:

+            default_rules["nat"].append("-N %s_direct" % chain)

+            default_rules["nat"].append("-A %s -j %s_direct" % (chain, chain))

+            self.our_chains["nat"].add("%s_direct" % chain)

+

+            if chain in [ "PREROUTING", "POSTROUTING" ]:

+                default_rules["nat"].append("-N %s_ZONES_SOURCE" % chain)

+                default_rules["nat"].append("-N %s_ZONES" % chain)

+                default_rules["nat"].append("-A %s -j %s_ZONES_SOURCE" % (chain, chain))

+                default_rules["nat"].append("-A %s -j %s_ZONES" % (chain, chain))

+                self.our_chains["nat"].update(set(["%s_ZONES_SOURCE" % chain, "%s_ZONES" % chain]))

+

+        default_rules["filter"] = [

+            "-N INPUT_direct",

+            "-N INPUT_ZONES_SOURCE",

+            "-N INPUT_ZONES",

+

+            "-A INPUT -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT",

+            "-A INPUT -i lo -j ACCEPT",

+            "-A INPUT -j INPUT_direct",

+            "-A INPUT -j INPUT_ZONES_SOURCE",

+            "-A INPUT -j INPUT_ZONES",

+        ]

+        if log_denied != "off":

+            default_rules["filter"].append("-A INPUT -m conntrack --ctstate INVALID %%LOGTYPE%% -j LOG --log-prefix 'STATE_INVALID_DROP: '")

+        default_rules["filter"].append("-A INPUT -m conntrack --ctstate INVALID -j DROP")

+        if log_denied != "off":

+            default_rules["filter"].append("-A INPUT %%LOGTYPE%% -j LOG --log-prefix 'FINAL_REJECT: '")

+        default_rules["filter"].append("-A INPUT -j %%REJECT%%")

+

+        default_rules["filter"] += [

+            "-N FORWARD_direct",

+            "-N FORWARD_IN_ZONES_SOURCE",

+            "-N FORWARD_IN_ZONES",

+            "-N FORWARD_OUT_ZONES_SOURCE",

+            "-N FORWARD_OUT_ZONES",

+

+            "-A FORWARD -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT",

+            "-A FORWARD -i lo -j ACCEPT",

+            "-A FORWARD -j FORWARD_direct",

+            "-A FORWARD -j FORWARD_IN_ZONES_SOURCE",

+            "-A FORWARD -j FORWARD_IN_ZONES",

+            "-A FORWARD -j FORWARD_OUT_ZONES_SOURCE",

+            "-A FORWARD -j FORWARD_OUT_ZONES",

+        ]

+        if log_denied != "off":

+            default_rules["filter"].append("-A FORWARD -m conntrack --ctstate INVALID %%LOGTYPE%% -j LOG --log-prefix 'STATE_INVALID_DROP: '")

+        default_rules["filter"].append("-A FORWARD -m conntrack --ctstate INVALID -j DROP")

+        if log_denied != "off":

+            default_rules["filter"].append("-A FORWARD %%LOGTYPE%% -j LOG --log-prefix 'FINAL_REJECT: '")

+        default_rules["filter"].append("-A FORWARD -j %%REJECT%%")

+

+        default_rules["filter"] += [

+            "-N OUTPUT_direct",

+

+            "-A OUTPUT -o lo -j ACCEPT",

+            "-A OUTPUT -j OUTPUT_direct",

+        ]

+

+        self.our_chains["filter"] = set(["INPUT_direct", "INPUT_ZONES_SOURCE", "INPUT_ZONES",

+                                    "FORWARD_direct", "FORWARD_IN_ZONES_SOURCE",

+                                    "FORWARD_IN_ZONES", "FORWARD_OUT_ZONES_SOURCE",

+                                    "FORWARD_OUT_ZONES", "OUTPUT_direct"])

+

+        final_default_rules = []

+        for table in default_rules:

             if table not in self.get_available_tables():

                 continue

-            _default_rules = DEFAULT_RULES[table][:]

-            if log_denied != "off" and table in LOG_RULES:

-                _default_rules.extend(LOG_RULES[table])

-            prefix = [ "-t", table ]

-            for rule in _default_rules:

-                if type(rule) == list:

-                    default_rules.append(prefix + rule)

-                else:

-                    default_rules.append(prefix + splitArgs(rule))

-        return default_rules

+            for rule in default_rules[table]:

+                final_default_rules.append(["-t", table] + splitArgs(rule))

+

+        return final_default_rules

     def get_zone_table_chains(self, table):

         if table == "filter":

@@ -709,7 +707,7 @@ class ip4tables(object):

     def build_zone_chain_rules(self, zone, table, chain):

         _zone = DEFAULT_ZONE_TARGET.format(chain=SHORTCUTS[chain], zone=zone)

-        OUR_CHAINS[table].update(set([_zone,

+        self.our_chains[table].update(set([_zone,

                                       "%s_log" % _zone,

                                       "%s_deny" % _zone,

                                       "%s_allow" % _zone]))

@@ -719,33 +717,35 @@ class ip4tables(object):

         rules.append([ "-N", "%s_log" % _zone, "-t", table ])

         rules.append([ "-N", "%s_deny" % _zone, "-t", table ])

         rules.append([ "-N", "%s_allow" % _zone, "-t", table ])

-        rules.append([ "-I", _zone, "1", "-t", table, "-j", "%s_log" % _zone ])

-        rules.append([ "-I", _zone, "2", "-t", table, "-j", "%s_deny" % _zone ])

-        rules.append([ "-I", _zone, "3", "-t", table, "-j", "%s_allow" % _zone ])

+        rules.append([ "-A", _zone, "-t", table, "-j", "%s_log" % _zone ])

+        rules.append([ "-A", _zone, "-t", table, "-j", "%s_deny" % _zone ])

+        rules.append([ "-A", _zone, "-t", table, "-j", "%s_allow" % _zone ])

-        # Handle trust, block and drop zones:

-        # Add an additional rule with the zone target (accept, reject

-        # or drop) to the base zone only in the filter table.

-        # Otherwise it is not be possible to have a zone with drop

-        # target, that is allowing traffic that is locally initiated

-        # or that adds additional rules. (RHBZ#1055190)

         target = self._fw.zone._zones[zone].target

-        if table == "filter" and \

-           target in [ "ACCEPT", "REJECT", "%%REJECT%%", "DROP" ] and \

-           chain in [ "INPUT", "FORWARD_IN", "FORWARD_OUT", "OUTPUT" ]:

-            rules.append([ "-I", _zone, "4", "-t", table, "-j", target ])

         if self._fw.get_log_denied() != "off":

             if table == "filter" and \

                chain in [ "INPUT", "FORWARD_IN", "FORWARD_OUT", "OUTPUT" ]:

                 if target in [ "REJECT", "%%REJECT%%" ]:

-                    rules.append([ "-I", _zone, "4", "-t", table, "%%LOGTYPE%%",

+                    rules.append([ "-A", _zone, "-t", table, "%%LOGTYPE%%",

                                    "-j", "LOG", "--log-prefix",

                                    "\"%s_REJECT: \"" % _zone ])

                 if target == "DROP":

-                    rules.append([ "-I", _zone, "4", "-t", table, "%%LOGTYPE%%",

+                    rules.append([ "-A", _zone, "-t", table, "%%LOGTYPE%%",

                                    "-j", "LOG", "--log-prefix",

                                    "\"%s_DROP: \"" % _zone ])

+

+        # Handle trust, block and drop zones:

+        # Add an additional rule with the zone target (accept, reject

+        # or drop) to the base zone only in the filter table.

+        # Otherwise it is not be possible to have a zone with drop

+        # target, that is allowing traffic that is locally initiated

+        # or that adds additional rules. (RHBZ#1055190)

+        if table == "filter" and \

+           target in [ "ACCEPT", "REJECT", "%%REJECT%%", "DROP" ] and \

+           chain in [ "INPUT", "FORWARD_IN", "FORWARD_OUT", "OUTPUT" ]:

+            rules.append([ "-A", _zone, "-t", table, "-j", target ])

+

         return rules

     def _rule_limit(self, limit):

-- 

2.20.1