Merge pull request #4651 from l0crian1/fw-empty-nodes

T7366: Firewall rules allow empty nodes

5 files changed, 322 insertions, 1 deletions

diff --git a/interface-definitions/include/version/firewall-version.xml.i b/interface-definitions/include/version/firewall-version.xml.i
index 1f3b779d5..f50c450ef 100644
--- a/interface-definitions/include/version/firewall-version.xml.i
+++ b/interface-definitions/include/version/firewall-version.xml.i
@@ -1,3 +1,3 @@
 <!-- include start from include/version/firewall-version.xml.i -->
-<syntaxVersion component='firewall' version='19'></syntaxVersion>
+<syntaxVersion component='firewall' version='20'></syntaxVersion>
 <!-- include end -->
diff --git a/smoketest/config-tests/firewall-empty-nodes b/smoketest/config-tests/firewall-empty-nodes
new file mode 100644
index 000000000..c0fcd93be
--- /dev/null
+++ b/smoketest/config-tests/firewall-empty-nodes
@@ -0,0 +1,31 @@
+set firewall bridge forward filter rule 1 action 'accept'
+set firewall ipv4 forward filter rule 1 action 'accept'
+set firewall ipv4 forward filter rule 2 action 'accept'
+set firewall ipv4 forward filter rule 3 action 'accept'
+set firewall ipv4 forward filter rule 5 action 'accept'
+set firewall ipv4 forward filter rule 6 action 'accept'
+set firewall ipv4 forward filter rule 7 action 'accept'
+set firewall ipv4 forward filter rule 8 action 'accept'
+set firewall ipv4 forward filter rule 9 action 'accept'
+set firewall ipv4 forward filter rule 10 action 'accept'
+set firewall ipv4 forward filter rule 10 log
+set firewall ipv4 forward filter rule 11 action 'accept'
+set firewall ipv4 forward filter rule 13 action 'accept'
+set firewall ipv4 forward filter rule 14 action 'accept'
+set firewall ipv4 forward filter rule 15 action 'accept'
+set firewall ipv4 forward filter rule 16 action 'accept'
+set firewall ipv4 forward filter rule 17 action 'accept'
+set firewall ipv4 forward filter rule 18 action 'accept'
+set firewall ipv4 forward filter rule 19 action 'accept'
+set firewall ipv4 forward filter rule 20 action 'accept'
+set firewall ipv4 forward filter rule 21 action 'accept'
+set firewall ipv6 forward filter rule 1 action 'accept'
+set firewall ipv6 forward filter rule 2 action 'accept'
+set interfaces ethernet eth0
+set interfaces ethernet eth1
+set interfaces ethernet eth2
+set interfaces loopback lo
+set system console device ttyS0 speed '115200'
+set system host-name 'vyos'
+set system login user vyos authentication encrypted-password '$6$O5gJRlDYQpj$MtrCV9lxMnZPMbcxlU7.FI793MImNHznxGoMFgm3Q6QP3vfKJyOSRCt3Ka/GzFQyW1yZS4NS616NLHaIPPFHc0'
+set system login user vyos authentication plaintext-password ''
diff --git a/smoketest/configs/firewall-empty-nodes b/smoketest/configs/firewall-empty-nodes
new file mode 100644
index 000000000..088af7550
--- /dev/null
+++ b/smoketest/configs/firewall-empty-nodes
@@ -0,0 +1,149 @@
+firewall {
+    bridge {
+        forward {
+            filter {
+                rule 1 {
+                    action "accept"
+                    vlan
+                }
+            }
+        }
+    }
+    ipv4 {
+        forward {
+            filter {
+                rule 1 {
+                    action "accept"
+                    add-address-to-group
+                }
+                rule 2 {
+                    action "accept"
+                    connection-status
+                }
+                rule 3 {
+                    action "accept"
+                    destination
+                }
+                rule 5 {
+                    action "accept"
+                    fragment
+                }
+                rule 6 {
+                    action "accept"
+                    icmp
+                }
+                rule 7 {
+                    action "accept"
+                    inbound-interface
+                }
+                rule 8 {
+                    action "accept"
+                    ipsec
+                }
+                rule 9 {
+                    action "accept"
+                    limit
+                }
+                rule 10 {
+                    action "accept"
+                    log
+                    log-options
+                }
+                rule 11 {
+                    action "accept"
+                    outbound-interface
+                }
+                rule 13 {
+                    action "accept"
+                    source
+                }
+                rule 14 {
+                    action "accept"
+                    tcp
+                }
+                rule 15 {
+                    action "accept"
+                    time
+                }
+                rule 16 {
+                    action "accept"
+                    ttl
+                }
+                rule 17 {
+                    action "accept"
+                    destination {
+                        group
+                    }
+                }
+                rule 18 {
+                    action "accept"
+                    destination {
+                        geoip
+                    }
+                }
+                rule 19 {
+                    action "accept"
+                    source {
+                        group
+                    }
+                }
+                rule 20 {
+                    action "accept"
+                    source {
+                        geoip
+                    }
+                }
+                rule 21 {
+                    action "accept"
+                    tcp {
+                        flags
+                    }
+                }
+            }
+        }
+    }
+    ipv6 {
+        forward {
+            filter {
+                rule 1 {
+                    action "accept"
+                    hop-limit
+                }
+                rule 2 {
+                    action "accept"
+                    icmpv6
+                }
+            }
+        }
+    }
+}
+interfaces {
+    ethernet eth0 {
+    }
+    ethernet eth1 {
+    }
+    ethernet eth2 {
+    }
+    loopback lo {
+    }
+}
+system {
+    console {
+        device ttyS0 {
+            speed "115200"
+        }
+    }
+    host-name "vyos"
+    login {
+        user vyos {
+            authentication {
+                encrypted-password "$6$O5gJRlDYQpj$MtrCV9lxMnZPMbcxlU7.FI793MImNHznxGoMFgm3Q6QP3vfKJyOSRCt3Ka/GzFQyW1yZS4NS616NLHaIPPFHc0"
+                plaintext-password ""
+            }
+        }
+    }
+}
+
+// Warning: Do not remove the following line.
+// vyos-config-version: "bgp@6:broadcast-relay@1:cluster@2:config-management@1:conntrack@6:conntrack-sync@2:container@2:dhcp-relay@2:dhcp-server@8:dhcpv6-server@1:dns-dynamic@4:dns-forwarding@4:firewall@15:flow-accounting@1:https@6:ids@1:interfaces@32:ipoe-server@3:ipsec@13:isis@3:l2tp@9:lldp@2:mdns@1:monitoring@1:nat@8:nat66@3:ntp@3:openconnect@3:ospf@2:pim@1:policy@8:pppoe-server@10:pptp@5:qos@2:quagga@11:reverse-proxy@1:rip@1:rpki@2:salt@1:snmp@3:ssh@2:sstp@6:system@27:vrf@3:vrrp@4:vyos-accel-ppp@2:wanloadbalance@3:webproxy@2"
+// Release version: 1.4.3
diff --git a/src/conf_mode/firewall.py b/src/conf_mode/firewall.py
index 90fdded99..cfccc88da 100755
--- a/src/conf_mode/firewall.py
+++ b/src/conf_mode/firewall.py
@@ -196,6 +196,42 @@ def verify_jump_target(firewall, hook, jump_target, family, recursive=False):
 
         targets_seen.append(target)
 
+def is_node_empty(rule_conf):
+    is_empty_list = []
+    is_empty_list.append([
+                        ['add_address_to_group'],
+                        ['connection_status'],
+                        ['destination'],
+                        ['destination', 'group'],
+                        ['destination', 'geoip'],
+                        ['fragment'],
+                        ['gre'],
+                        ['gre', 'flags'],
+                        ['hop_limit'],
+                        ['icmp'],
+                        ['icmpv6'],
+                        ['inbound_interface'],
+                        ['ipsec'],
+                        ['limit'],
+                        ['log_options'],
+                        ['outbound_interface'],
+                        ['set'],
+                        ['source'],
+                        ['source', 'group'],
+                        ['source', 'geoip'],
+                        ['tcp'],
+                        ['tcp', 'flags'],
+                        ['time'],
+                        ['ttl'],
+                        ['vlan']
+                        ])
+
+    for node in is_empty_list[0]:
+        if dict_search_args(rule_conf, *node) == {}:
+            return True, node
+
+    return False, None
+
 def verify_rule(firewall, family, hook, priority, rule_id, rule_conf):
     if 'action' not in rule_conf:
         raise ConfigError('Rule action must be defined')
@@ -244,6 +280,11 @@ def verify_rule(firewall, family, hook, priority, rule_id, rule_conf):
         if {'match_frag', 'match_non_frag'} <= set(rule_conf['fragment']):
             raise ConfigError('Cannot specify both "match-frag" and "match-non-frag"')
 
+    node_empty, node_name = is_node_empty(rule_conf)
+    if node_empty:
+        tmp = ' '.join(node_name).replace('_', '-')
+        raise ConfigError(f'Configuration node {tmp} may not be empty')
+
     if 'limit' in rule_conf:
         if 'rate' in rule_conf['limit']:
             rate_int = re.sub(r'\D', '', rule_conf['limit']['rate'])
diff --git a/src/migration-scripts/firewall/19-to-20 b/src/migration-scripts/firewall/19-to-20
new file mode 100644
index 000000000..c0ef5e127
--- /dev/null
+++ b/src/migration-scripts/firewall/19-to-20
@@ -0,0 +1,100 @@
+# Copyright VyOS maintainers and contributors <maintainers@vyos.io>
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2.1 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public License
+# along with this library.  If not, see <http://www.gnu.org/licenses/>.
+
+# T7366: Firewall rules allow empty nodes
+# When configuring the firewall, many nodes are accepted with empty values,
+# which are not parsed into rules.
+
+# Very few of these have subsequent error handling. Some should be obvious
+# to the user that a value is required, like 'inbound-interface' (though
+# an error should still be thrown if they're configured without children).
+
+# But some could be misunderstood and lead to an outage or wide open firewall.
+# For instance, let's say someone wanted to block all icmp. They may incorrectly
+# configure:
+
+# set firewall ipv4 input filter rule 10 action drop
+# set firewall ipv4 input filter rule 10 icmp
+
+# And this would create this rule in nftables, dropping all traffic in subsequent rules:
+
+# counter packets 0 bytes 0 drop comment "ipv4-INP-filter-10"
+
+# They could also unintentionally allow all traffic by attempting to only allow icmp in a rule.
+
+import json
+
+from vyos.configtree import ConfigTree
+from vyos.utils.dict import dict_search_args
+
+firewall_base = ['firewall']
+
+def migrate(config: ConfigTree) -> None:
+    if not config.exists(firewall_base):
+        # Nothing to do
+        return
+
+    firewall_dict = json.loads(config.to_json()).get('firewall')
+
+    is_empty_list = []
+    is_empty_list.append([
+                        ['add-address-to-group'],
+                        ['connection-status'],
+                        ['destination', 'group'],
+                        ['destination', 'geoip'],
+                        ['destination'],
+                        ['fragment'],
+                        ['gre', 'flags'],
+                        ['gre'],
+                        ['hop-limit'],
+                        ['icmp'],
+                        ['icmpv6'],
+                        ['inbound-interface'],
+                        ['ipsec'],
+                        ['limit'],
+                        ['log-options'],
+                        ['outbound-interface'],
+                        ['set'],
+                        ['source', 'group'],
+                        ['source', 'geoip'],
+                        ['source'],
+                        ['tcp', 'flags'],
+                        ['tcp'],
+                        ['time'],
+                        ['ttl'],
+                        ['vlan']
+                        ])
+
+    for family in ['ipv4', 'ipv6', 'bridge']:
+        if family in firewall_dict:
+            for chain in ['name','forward','input','output', 'prerouting']:
+                if chain in firewall_dict[family]:
+                    for priority, priority_conf in firewall_dict[family][chain].items():
+                        if 'rule' in priority_conf:
+                            for rule_id, rule_conf in priority_conf['rule'].items():
+                                node_deleted_list = []
+                                for node in is_empty_list[0]:
+                                    if dict_search_args(rule_conf, *node) == {}:
+                                        if len(node) == 1:
+                                            if node not in node_deleted_list:
+                                                config.delete(firewall_base + [family, chain, priority, 'rule', rule_id, node[0]])
+                                        else:
+                                            del firewall_dict[family][chain][priority]['rule'][rule_id][node[0]][node[1]]
+
+                                            if dict_search_args(rule_conf, node[0]) == {}:
+                                                config.delete(firewall_base + [family, chain, priority, 'rule', rule_id, node[0]])
+                                                node_deleted_list.append([node[0]])
+                                            else:
+                                                config.delete(firewall_base + [family, chain, priority, 'rule', rule_id, node[0], node[1]])