diff options
author | Andrew Topp <andrewt@telekinetica.net> | 2024-08-04 17:52:57 +1000 |
---|---|---|
committer | Andrew Topp <andrewt@telekinetica.net> | 2024-08-04 17:52:57 +1000 |
commit | 60b0614296874c144665417130d4881461114db0 (patch) | |
tree | 404eb8bf72582b60cad69d9c23535b41a49094f6 | |
parent | 15c77978f30bebe7c6d4f4e9a87c56e12e1382cd (diff) | |
download | vyos-1x-60b0614296874c144665417130d4881461114db0.tar.gz vyos-1x-60b0614296874c144665417130d4881461114db0.zip |
firewall: T4694: Adding GRE flags & fields matches to firewall rules
* Only matching flags and fields used by modern RFC2890 "extended GRE" -
this is backwards-compatible, but does not match all possible flags.
* There are no nftables helpers for the GRE key field, which is critical
to match individual tunnel sessions (more detail in the forum post)
* nft expression syntax is not flexible enough for multiple field
matches in a single rule and the key offset changes depending on flags.
* Thus, clumsy compromise in requiring an explicit match on the "checksum"
flag if a key is present, so we know where key will be. In most cases,
nobody uses the checksum, but assuming it to be off or automatically
adding a "not checksum" match unless told otherwise would be confusing
* The automatic "flags key" check when specifying a key doesn't have similar
validation, I added it first and it makes sense. I would still like
to find a workaround to the "checksum" offset problem.
* If we could add 2 rules from 1 config definition, we could match
both cases with appropriate offsets, but this would break existing
FW generation logic, logging, etc.
* Added a "test_gre_match" smoketest
-rw-r--r-- | interface-definitions/include/firewall/common-rule-inet.xml.i | 1 | ||||
-rw-r--r-- | interface-definitions/include/firewall/gre.xml.i | 116 | ||||
-rw-r--r-- | python/vyos/firewall.py | 61 | ||||
-rwxr-xr-x | smoketest/scripts/cli/test_firewall.py | 55 | ||||
-rwxr-xr-x | src/conf_mode/firewall.py | 30 |
5 files changed, 263 insertions, 0 deletions
diff --git a/interface-definitions/include/firewall/common-rule-inet.xml.i b/interface-definitions/include/firewall/common-rule-inet.xml.i index 0acb08ec9..e44938b14 100644 --- a/interface-definitions/include/firewall/common-rule-inet.xml.i +++ b/interface-definitions/include/firewall/common-rule-inet.xml.i @@ -19,5 +19,6 @@ #include <include/firewall/synproxy.xml.i> #include <include/firewall/tcp-flags.xml.i> #include <include/firewall/tcp-mss.xml.i> +#include <include/firewall/gre.xml.i> #include <include/firewall/time.xml.i> <!-- include end --> diff --git a/interface-definitions/include/firewall/gre.xml.i b/interface-definitions/include/firewall/gre.xml.i new file mode 100644 index 000000000..285233434 --- /dev/null +++ b/interface-definitions/include/firewall/gre.xml.i @@ -0,0 +1,116 @@ +<!-- include start from firewall/gre.xml.i --> +<node name="gre"> + <properties> + <help>GRE fields to match</help> + </properties> + <children> + <node name="flags"> + <properties> + <help>GRE flag bits to match</help> + </properties> + <children> + <node name="key"> + <properties> + <help>Header includes optional key field</help> + </properties> + <children> + <leafNode name="unset"> + <properties> + <help>Header does not include optional key field</help> + <valueless/> + </properties> + </leafNode> + </children> + </node> + <node name="checksum"> + <properties> + <help>Header includes optional checksum</help> + </properties> + <children> + <leafNode name="unset"> + <properties> + <help>Header does not include optional checksum</help> + <valueless/> + </properties> + </leafNode> + </children> + </node> + <node name="sequence"> + <properties> + <help>Header includes a sequence number field</help> + </properties> + <children> + <leafNode name="unset"> + <properties> + <help>Header does not include a sequence number field</help> + <valueless/> + </properties> + </leafNode> + </children> + </node> + </children> + </node> + <leafNode name="inner-proto"> + <properties> + <help>EtherType of encapsulated packet</help> + <completionHelp> + <list>ip ip6 arp 802.1q 802.1ad</list> + </completionHelp> + <valueHelp> + <format>u32:0-65535</format> + <description>Ethernet protocol number</description> + </valueHelp> + <valueHelp> + <format>u32:0x0-0xffff</format> + <description>Ethernet protocol number (hex)</description> + </valueHelp> + <valueHelp> + <format>ip</format> + <description>IPv4</description> + </valueHelp> + <valueHelp> + <format>ip6</format> + <description>IPv6</description> + </valueHelp> + <valueHelp> + <format>arp</format> + <description>Address Resolution Protocol</description> + </valueHelp> + <valueHelp> + <format>802.1q</format> + <description>VLAN-tagged frames (IEEE 802.1q)</description> + </valueHelp> + <valueHelp> + <format>802.1ad</format> + <description>Provider Bridging (IEEE 802.1ad, Q-in-Q)</description> + </valueHelp> + <valueHelp> + <format>gretap</format> + <description>Transparent Ethernet Bridging (L2 Ethernet over GRE, gretap)</description> + </valueHelp> + <constraint> + <regex>(ip|ip6|arp|802.1q|802.1ad|gretap|0x[0-9a-fA-F]{1,4})</regex> + <validator name="numeric" argument="--range 0-65535"/> + </constraint> + </properties> + </leafNode> + #include <interface/parameters-key.xml.i> + <leafNode name="version"> + <properties> + <help>GRE Version</help> + <valueHelp> + <format>gre</format> + <description>Standard GRE</description> + </valueHelp> + <valueHelp> + <format>pptp</format> + <description>Point to Point Tunnelling Protocol</description> + </valueHelp> + <constraint> + <regex>(gre|pptp)</regex> + </constraint> + </properties> + </leafNode> + </children> +</node> +<!-- include end --> diff --git a/python/vyos/firewall.py b/python/vyos/firewall.py index cac6d2953..3976a5580 100644 --- a/python/vyos/firewall.py +++ b/python/vyos/firewall.py @@ -409,6 +409,41 @@ def parse_rule(rule_conf, hook, fw_name, rule_id, ip_name): time = rule_conf['recent']['time'] output.append(f'add @RECENT{def_suffix}_{hook}_{fw_name}_{rule_id} {{ {ip_name} saddr limit rate over {count}/{time} burst {count} packets }}') + if 'gre' in rule_conf: + gre_key = dict_search_args(rule_conf, 'gre', 'key') + + gre_flags = dict_search_args(rule_conf, 'gre', 'flags') + output.append(parse_gre_flags(gre_flags or {}, force_keyed=gre_key is not None)) + + gre_proto_alias_map = { + '802.1q': '8021q', + '802.1ad': '8021ad', + 'gretap': '0x6558', + } + + gre_proto = dict_search_args(rule_conf, 'gre', 'inner_proto') + if gre_proto is not None: + gre_proto = gre_proto_alias_map.get(gre_proto, gre_proto) + output.append(f'gre protocol {gre_proto}') + + gre_ver = dict_search_args(rule_conf, 'gre', 'version') + if gre_ver == 'gre': + output.append('gre version 0') + elif gre_ver == 'pptp': + output.append('gre version 1') + + if gre_key: + # The offset of the key within the packet shifts depending on the C-flag. + # nftables cannot handle complex enough expressions to match multiple + # offsets based on bitfields elsewhere. + # We enforce a specific match for the checksum flag in validation, so the + # gre_flags dict will always have a 'checksum' key when gre_key is populated. + if not gre_flags['checksum']: + # No "unset" child node means C is set, we offset key lookup +32 bits + output.append(f'@th,64,32 == {gre_key}') + else: + output.append(f'@th,32,32 == {gre_key}') + if 'time' in rule_conf: output.append(parse_time(rule_conf['time'])) @@ -544,6 +579,32 @@ def parse_rule(rule_conf, hook, fw_name, rule_id, ip_name): output.append(f'comment "{family}-{hook}-{fw_name}-{rule_id}"') return " ".join(output) +def parse_gre_flags(flags, force_keyed=False): + flag_map = { # nft does not have symbolic names for these. + 'checksum': 1<<0, + 'routing': 1<<1, + 'key': 1<<2, + 'sequence': 1<<3, + 'strict_routing': 1<<4, + } + + include = 0 + exclude = 0 + for fl_name, fl_state in flags.items(): + if not fl_state: + include |= flag_map[fl_name] + else: # 'unset' child tag + exclude |= flag_map[fl_name] + + if force_keyed: + # Implied by a key-match. + include |= flag_map['key'] + + if include == 0 and exclude == 0: + return '' # Don't bother extracting and matching no bits + + return f'gre flags & {include + exclude} == {include}' + def parse_tcp_flags(flags): include = [flag for flag in flags if flag != 'not'] exclude = list(flags['not']) if 'not' in flags else [] diff --git a/smoketest/scripts/cli/test_firewall.py b/smoketest/scripts/cli/test_firewall.py index 551f8ce65..dfc816a42 100755 --- a/smoketest/scripts/cli/test_firewall.py +++ b/smoketest/scripts/cli/test_firewall.py @@ -1100,5 +1100,60 @@ class TestFirewall(VyOSUnitTestSHIM.TestCase): with self.assertRaises(ConfigSessionError): self.cli_commit() + def test_gre_match(self): + name = 'smoketest-gre' + + self.cli_set(['firewall', 'ipv4', 'name', name, 'default-action', 'return']) + self.cli_set(['firewall', 'ipv4', 'name', name, 'rule', '1', 'action', 'accept']) + self.cli_set(['firewall', 'ipv4', 'name', name, 'rule', '1', 'protocol', 'gre']) + self.cli_set(['firewall', 'ipv4', 'name', name, 'rule', '1', 'gre', 'flags', 'key']) + self.cli_set(['firewall', 'ipv4', 'name', name, 'rule', '1', 'gre', 'flags', 'checksum', 'unset']) + self.cli_set(['firewall', 'ipv4', 'name', name, 'rule', '1', 'gre', 'key', '1234']) + self.cli_set(['firewall', 'ipv4', 'name', name, 'rule', '1', 'log']) + + self.cli_set(['firewall', 'ipv4', 'forward', 'filter', 'rule', '2', 'action', 'continue']) + self.cli_set(['firewall', 'ipv4', 'forward', 'filter', 'rule', '2', 'protocol', 'gre']) + self.cli_set(['firewall', 'ipv4', 'forward', 'filter', 'rule', '2', 'gre', 'inner-proto', '0x6558']) + self.cli_set(['firewall', 'ipv4', 'forward', 'filter', 'rule', '2', 'log']) + + self.cli_set(['firewall', 'ipv6', 'input', 'filter', 'rule', '3', 'action', 'drop']) + self.cli_set(['firewall', 'ipv6', 'input', 'filter', 'rule', '3', 'protocol', 'gre']) + self.cli_set(['firewall', 'ipv6', 'input', 'filter', 'rule', '3', 'gre', 'flags', 'checksum']) + self.cli_set(['firewall', 'ipv6', 'input', 'filter', 'rule', '3', 'gre', 'key', '4321']) + + self.cli_set(['firewall', 'ipv6', 'output', 'filter', 'rule', '4', 'action', 'reject']) + self.cli_set(['firewall', 'ipv6', 'output', 'filter', 'rule', '4', 'protocol', 'gre']) + self.cli_set(['firewall', 'ipv6', 'output', 'filter', 'rule', '4', 'gre', 'version', 'pptp']) + + self.cli_commit() + + nftables_search_v4 = [ + ['gre protocol 0x6558', 'continue comment'], + ['gre flags & 5 == 4 @th,32,32 0x4d2', 'accept comment'], + ] + + nftables_search_v6 = [ + ['gre flags & 5 == 5 @th,64,32 0x10e1', 'drop comment'], + ['gre version 1', 'reject comment'], + ] + + self.verify_nftables(nftables_search_v4, 'ip vyos_filter') + self.verify_nftables(nftables_search_v6, 'ip6 vyos_filter') + + # GRE match will only work with protocol GRE + self.cli_delete(['firewall', 'ipv4', 'name', name, 'rule', '1', 'protocol', 'gre']) + + with self.assertRaises(ConfigSessionError): + self.cli_commit() + + self.cli_discard() + + # GREv1 (PPTP) does not include a key field, match not available + self.cli_set(['firewall', 'ipv6', 'output', 'filter', 'rule', '4', 'gre', 'flags', 'checksum', 'unset']) + self.cli_set(['firewall', 'ipv6', 'output', 'filter', 'rule', '4', 'gre', 'key', '1234']) + + with self.assertRaises(ConfigSessionError): + self.cli_commit() + if __name__ == '__main__': unittest.main(verbosity=2) diff --git a/src/conf_mode/firewall.py b/src/conf_mode/firewall.py index 02bf00bcc..b71ce7124 100755 --- a/src/conf_mode/firewall.py +++ b/src/conf_mode/firewall.py @@ -231,6 +231,36 @@ def verify_rule(firewall, family, hook, priority, rule_id, rule_conf): if not {'count', 'time'} <= set(rule_conf['recent']): raise ConfigError('Recent "count" and "time" values must be defined') + if 'gre' in rule_conf: + if dict_search_args(rule_conf, 'protocol') != 'gre': + raise ConfigError('Protocol must be gre when matching GRE flags and fields') + + if dict_search_args(rule_conf, 'gre', 'key'): + if dict_search_args(rule_conf, 'gre', 'version') == 'pptp': + raise ConfigError('GRE tunnel keys are not present in PPTP') + + if dict_search_args(rule_conf, 'gre', 'flags', 'checksum') is None: + # There is no builtin match in nftables for the GRE key, so we need to do a raw lookup. + # The offset of the key within the packet shifts depending on the C-flag. + # 99% of the time, nobody will have checksums enabled - it's usually a manual config option. + # We can either assume it is unset unless otherwise directed + # (confusing, requires doco to explain why it doesn't work sometimes) + # or, demand an explicit selection to be made for this specific match rule. + # This check enforces the latter. The user is free to create rules for both cases. + raise ConfigError('Matching GRE tunnel key requires an explicit checksum flag match. For most cases, use "gre flags checksum unset"') + + if dict_search_args(rule_conf, 'gre', 'flags', 'key', 'unset') is not None: + raise ConfigError('Matching GRE tunnel key implies "flags key", cannot specify "flags key unset"') + + gre_inner_proto = dict_search_args(rule_conf, 'gre', 'inner_proto') + if gre_inner_proto is not None: + try: + gre_inner_value = int(gre_inner_proto, 0) + if gre_inner_value < 0 or gre_inner_value > 65535: + raise ConfigError('inner-proto outside valid ethertype range 0-65535') + except ValueError: + pass # Symbolic constant, pre-validated before reaching here. + tcp_flags = dict_search_args(rule_conf, 'tcp', 'flags') if tcp_flags: if dict_search_args(rule_conf, 'protocol') != 'tcp': |