From 9fe58f51e1e12eadac82d04a34fb024b012bc912 Mon Sep 17 00:00:00 2001 From: Jo-Philipp Wich Date: Sat, 12 Feb 2022 20:13:06 +0100 Subject: [PATCH] fw4: rework and fix family inheritance logic Fix various quirks in the logic inferring the effective rule family from referenced entities, consider subnet matches when determining family of zones and set the zone family to the determined value in case it is unspecified in configuration. Signed-off-by: Jo-Philipp Wich --- root/usr/share/ucode/fw4.uc | 226 +++++++++++++----------- tests/03_rules/08_family_inheritance | 255 +++++++++++++++++++++++++++ 2 files changed, 376 insertions(+), 105 deletions(-) create mode 100644 tests/03_rules/08_family_inheritance diff --git a/root/usr/share/ucode/fw4.uc b/root/usr/share/ucode/fw4.uc index fa65a46..243e27c 100644 --- a/root/usr/share/ucode/fw4.uc +++ b/root/usr/share/ucode/fw4.uc @@ -226,7 +226,7 @@ function null_if_empty(x) { } function subnets_split_af(x) { - let rv = []; + let rv = {}; for (let ag in to_array(x)) { for (let a in filter(ag.addrs, a => (a.family == 4))) { @@ -240,6 +240,9 @@ function subnets_split_af(x) { } } + if (rv[0] || rv[1]) + rv.family = (!rv[0] ^ !rv[1]) ? (rv[0] ? 4 : 6) : 0; + return rv; } @@ -310,7 +313,7 @@ function infer_family(f, objects) { if (!obj || !obj.family || obj.family == res) continue; - if (res == 0) { + if (!res) { res = obj.family; by = obj.desc; continue; @@ -1931,9 +1934,15 @@ return { }; let family = infer_family(zone.family, [ - zone.helper, "ct helper" + zone.helper, "ct helper", + match_subnets, "subnet list" ]); + if (type(family) == "string") { + this.warn_section(data, family + ", skipping"); + return; + } + // group non-inverted device matches into wildcard and non-wildcard ones let devices = [], plain_devices = [], plain_invert_devices = [], wildcard_invert_devices = []; @@ -2006,6 +2015,8 @@ return { } } + zone.family = family; + zone.match_rules = match_rules; zone.masq4_src_subnets = subnets_group_by_masking(masq_src_subnets[0]); @@ -2375,10 +2386,15 @@ return { break; } + sip = subnets_split_af(rule.src_ip); + dip = subnets_split_af(rule.dest_ip); + family = infer_family(family, [ ipset, "set match", - rule.src, "source zone", - rule.dest, "destination zone", + sip, "source IP", + dip, "destination IP", + rule.src?.zone, "source zone", + rule.dest?.zone, "destination zone", rule.helper, "helper match", rule.set_helper, "helper to set" ]); @@ -2388,9 +2404,6 @@ return { continue; } - sip = subnets_split_af(rule.src_ip); - dip = subnets_split_af(rule.dest_ip); - let has_ipv4_specifics = (length(sip[0]) || length(dip[0]) || length(itypes4) || rule.dscp !== null); let has_ipv6_specifics = (length(sip[1]) || length(dip[1]) || length(itypes6) || rule.dscp !== null); @@ -2669,18 +2682,6 @@ return { if (proto.name == "ipv6-icmp") family = 6; - family = infer_family(family, [ - ipset, "set match", - redir.src, "source zone", - redir.dest, "destination zone", - redir.helper, "helper match" - ]); - - if (type(family) == "string") { - this.warn_section(data, family + ", skipping"); - continue; - } - switch (redir.target) { case "dnat": sip = subnets_split_af(redir.src_ip); @@ -2696,115 +2697,130 @@ return { break; } - /* build reflection rules */ - if (redir.reflection && (length(rip[0]) || length(rip[1])) && redir.src?.zone && redir.dest?.zone) { - let refredir = { - name: redir.name + " (reflection)", + break; - helper: redir.helper, + case "snat": + sip = subnets_split_af(redir.src_ip); + dip = subnets_split_af(redir.dest_ip); + rip = subnets_split_af(redir.src_dip); - // XXX: this likely makes no sense for reflection rules - //src_mac: redir.src_mac, + switch (proto.name) { + case "tcp": + case "udp": + sport = redir.src_port; + dport = redir.dest_port; + rport = redir.src_dport; + break; + } - limit: redir.limit, - limit_burst: redir.limit_burst, + break; + } - start_date: redir.start_date, - stop_date: redir.stop_date, - start_time: redir.start_time, - stop_time: redir.stop_time, - weekdays: redir.weekdays, + family = infer_family(family, [ + ipset, "set match", + sip, "source IP", + dip, "destination IP", + rip, "rewrite IP", + redir.src?.zone, "source zone", + redir.dest?.zone, "destination zone", + redir.helper, "helper match" + ]); - mark: redir.mark - }; + if (type(family) == "string") { + this.warn_section(data, family + ", skipping"); + continue; + } - let eaddrs = length(dip) ? dip : subnets_split_af({ addrs: map(redir.src.zone.related_subnets, to_hostaddr) }); - let rzones = length(redir.reflection_zone) ? redir.reflection_zone : [ redir.dest ]; + /* build reflection rules */ + if (redir.target == "dnat" && redir.reflection && + (length(rip[0]) || length(rip[1])) && redir.src?.zone && redir.dest?.zone) { + let refredir = { + name: redir.name + " (reflection)", - for (let rzone in rzones) { - if (!is_family(rzone, family)) { - this.warn_section(data, - sprintf("is restricted to IPv%d but referenced reflection zone is IPv%d only, skipping", - family, rzone.family)); - continue; - } + helper: redir.helper, - let iaddrs = subnets_split_af({ addrs: rzone.zone.related_subnets }); - let refaddrs = (redir.reflection_src == "internal") ? iaddrs : eaddrs; + // XXX: this likely makes no sense for reflection rules + //src_mac: redir.src_mac, - for (let i = 0; i <= 1; i++) { - if (redir.src.zone[i ? "masq6" : "masq"] && length(rip[i])) { - let snat_addr = refaddrs[i]?.[0]; + limit: redir.limit, + limit_burst: redir.limit_burst, - /* For internal reflection sources try to find a suitable candiate IP - * among the reflection zone subnets which is within the same subnet - * as the original DNAT destination. If we can't find any matching - * one then simply take the first candidate. */ - if (redir.reflection_src == "internal") { - for (let zone_addr in rzone.zone.related_subnets) { - if (zone_addr.family != rip[i][0].family) - continue; + start_date: redir.start_date, + stop_date: redir.stop_date, + start_time: redir.start_time, + stop_time: redir.stop_time, + weekdays: redir.weekdays, - let r = apply_mask(rip[i][0].addr, zone_addr.mask); - let a = apply_mask(zone_addr.addr, zone_addr.mask); + mark: redir.mark + }; - if (r != a) - continue; + let eaddrs = length(dip) ? dip : subnets_split_af({ addrs: map(redir.src.zone.related_subnets, to_hostaddr) }); + let rzones = length(redir.reflection_zone) ? redir.reflection_zone : [ redir.dest ]; - snat_addr = zone_addr; - break; - } - } + for (let rzone in rzones) { + if (!is_family(rzone, family)) { + this.warn_section(data, + sprintf("is restricted to IPv%d but referenced reflection zone is IPv%d only, skipping", + family, rzone.family)); + continue; + } - if (!snat_addr) { - this.warn_section(data, (redir.reflection_src || "external") + " rewrite IP cannot be determined, disabling reflection"); - } - else if (!length(iaddrs[i])) { - this.warn_section(data, "internal address range cannot be determined, disabling reflection"); - } - else if (!length(eaddrs[i])) { - this.warn_section(data, "external address range cannot be determined, disabling reflection"); - } - else { - refredir.src = rzone; - refredir.dest = null; - refredir.target = "dnat"; + let iaddrs = subnets_split_af({ addrs: rzone.zone.related_subnets }); + let refaddrs = (redir.reflection_src == "internal") ? iaddrs : eaddrs; - for (let saddrs in subnets_group_by_masking(iaddrs[i])) - for (let daddrs in subnets_group_by_masking(eaddrs[i])) - add_rule(i ? 6 : 4, proto, saddrs, daddrs, rip[i], sport, dport, rport, null, refredir); + for (let i = 0; i <= 1; i++) { + if (redir.src.zone[i ? "masq6" : "masq"] && length(rip[i])) { + let snat_addr = refaddrs[i]?.[0]; + + /* For internal reflection sources try to find a suitable candiate IP + * among the reflection zone subnets which is within the same subnet + * as the original DNAT destination. If we can't find any matching + * one then simply take the first candidate. */ + if (redir.reflection_src == "internal") { + for (let zone_addr in rzone.zone.related_subnets) { + if (zone_addr.family != rip[i][0].family) + continue; + + let r = apply_mask(rip[i][0].addr, zone_addr.mask); + let a = apply_mask(zone_addr.addr, zone_addr.mask); - refredir.src = null; - refredir.dest = rzone; - refredir.target = "snat"; + if (r != a) + continue; - for (let daddrs in subnets_group_by_masking(rip[i])) - for (let saddrs in subnets_group_by_masking(iaddrs[i])) - add_rule(i ? 6 : 4, proto, saddrs, daddrs, [ to_hostaddr(snat_addr) ], null, rport, null, null, refredir); + snat_addr = zone_addr; + break; } } - } - } - } + if (!snat_addr) { + this.warn_section(data, (redir.reflection_src || "external") + " rewrite IP cannot be determined, disabling reflection"); + } + else if (!length(iaddrs[i])) { + this.warn_section(data, "internal address range cannot be determined, disabling reflection"); + } + else if (!length(eaddrs[i])) { + this.warn_section(data, "external address range cannot be determined, disabling reflection"); + } + else { + refredir.src = rzone; + refredir.dest = null; + refredir.target = "dnat"; - break; + for (let saddrs in subnets_group_by_masking(iaddrs[i])) + for (let daddrs in subnets_group_by_masking(eaddrs[i])) + add_rule(i ? 6 : 4, proto, saddrs, daddrs, rip[i], sport, dport, rport, null, refredir); - case "snat": - sip = subnets_split_af(redir.src_ip); - dip = subnets_split_af(redir.dest_ip); - rip = subnets_split_af(redir.src_dip); + refredir.src = null; + refredir.dest = rzone; + refredir.target = "snat"; - switch (proto.name) { - case "tcp": - case "udp": - sport = redir.src_port; - dport = redir.dest_port; - rport = redir.src_dport; - break; + for (let daddrs in subnets_group_by_masking(rip[i])) + for (let saddrs in subnets_group_by_masking(iaddrs[i])) + add_rule(i ? 6 : 4, proto, saddrs, daddrs, [ to_hostaddr(snat_addr) ], null, rport, null, null, refredir); + } + } + } } - - break; } if (length(rip[0]) > 1 || length(rip[1]) > 1) diff --git a/tests/03_rules/08_family_inheritance b/tests/03_rules/08_family_inheritance new file mode 100644 index 0000000..9a6aa59 --- /dev/null +++ b/tests/03_rules/08_family_inheritance @@ -0,0 +1,255 @@ +Testing various option constraints. + +-- Testcase -- +{% + include("./root/usr/share/firewall4/main.uc", { + getenv: function(varname) { + switch (varname) { + case 'ACTION': + return 'print'; + } + } + }) +%} +-- End -- + +-- File uci/helpers.json -- +{} +-- End -- + +-- File uci/firewall.json -- +{ + "zone": [ + { + ".description": "A zone matching only IPv4 subnets is assumed to be an IPv4 only zone", + "name": "ipv4only", + "subnet": "192.168.1.0/24", + "auto_helper": 0 + }, + + { + ".description": "A zone with conflicting family and subnet settings should be skipped", + "name": "afconflict", + "subnet": "10.0.0.0/8", + "family": "IPv6", + "auto_helper": 0 + } + ], + "ipset": [ + { + "name": "ipv4set", + "match": "src_ip", + "entry": [ + "10.0.0.2", + "10.0.0.3", + "10.0.0.4" + ] + } + ], + "rule": [ + { + ".description": "Rules referencing an IPv4 only zone should be restricted to IPv4 themselves", + "src": "ipv4only", + "proto": "tcp", + "dest_port": "22", + "name": "Rule #1", + "target": "accept" + }, + + { + ".description": "Rules whose family conflicts with their addresses should be skipped", + "proto": "tcp", + "src_ip": "10.0.0.1", + "dest_port": "22", + "name": "Rule #2", + "target": "accept", + "family": "IPv6" + }, + + { + ".description": "Rules whose family conflicts with the zone family should be skipped", + "src": "ipv4only", + "proto": "tcp", + "dest_port": "22", + "name": "Rule #3", + "target": "accept", + "family": "IPv6" + }, + + { + ".description": "Rules whose family conflicts with the referenced set family should be skipped", + "src": "ipv4only", + "proto": "tcp", + "ipset": "ipv4set", + "name": "Rule #4", + "target": "accept", + "family": "IPv6" + } + ], + "redirect": [ + { + ".description": "Redirects rhose family conflicts with the referenced zone family should be skipped", + "src": "ipv4only", + "proto": "tcp", + "src_dport": "22", + "dest_ip": "fdca::1", + "name": "Redirect #1", + "target": "dnat" + }, + ] +} +-- End -- + +-- Expect stderr -- +[!] Section @zone[1] (afconflict) is restricted to IPv6 but referenced subnet list is IPv4 only, skipping +[!] Section @rule[1] (Rule #2) is restricted to IPv6 but referenced source IP is IPv4 only, skipping +[!] Section @rule[2] (Rule #3) is restricted to IPv6 but referenced source zone is IPv4 only, skipping +[!] Section @rule[3] (Rule #4) is restricted to IPv6 but referenced set match is IPv4 only, skipping +[!] Section @redirect[0] (Redirect #1) is restricted to IPv6 but referenced source zone is IPv4 only, skipping +-- End -- + +-- Expect stdout -- +table inet fw4 +flush table inet fw4 + +table inet fw4 { + # + # Set definitions + # + + set ipv4set { + type ipv4_addr + elements = { + 10.0.0.2, + 10.0.0.3, + 10.0.0.4, + } + } + + + # + # Defines + # + + define ipv4only_subnets = { 192.168.1.0/24 } + + # + # User includes + # + + include "/etc/nftables.d/*.nft" + + + # + # Filter rules + # + + chain input { + type filter hook input priority filter; policy drop; + + iifname "lo" accept comment "!fw4: Accept traffic from loopback" + + ct state established,related accept comment "!fw4: Allow inbound established and related flows" + meta nfproto ipv4 ip saddr 192.168.1.0/24 jump input_ipv4only comment "!fw4: Handle ipv4only IPv4 input traffic" + } + + chain forward { + type filter hook forward priority filter; policy drop; + + ct state established,related accept comment "!fw4: Allow forwarded established and related flows" + meta nfproto ipv4 ip saddr 192.168.1.0/24 jump forward_ipv4only comment "!fw4: Handle ipv4only IPv4 forward traffic" + } + + chain output { + type filter hook output priority filter; policy drop; + + oifname "lo" accept comment "!fw4: Accept traffic towards loopback" + + ct state established,related accept comment "!fw4: Allow outbound established and related flows" + meta nfproto ipv4 ip daddr 192.168.1.0/24 jump output_ipv4only comment "!fw4: Handle ipv4only IPv4 output traffic" + } + + chain handle_reject { + meta l4proto tcp reject with tcp reset comment "!fw4: Reject TCP traffic" + reject with icmpx type port-unreachable comment "!fw4: Reject any other traffic" + } + + chain input_ipv4only { + meta nfproto ipv4 tcp dport 22 counter accept comment "!fw4: Rule #1" + ct status dnat accept comment "!fw4: Accept port redirections" + jump drop_from_ipv4only + } + + chain output_ipv4only { + jump drop_to_ipv4only + } + + chain forward_ipv4only { + ct status dnat accept comment "!fw4: Accept port forwards" + jump drop_to_ipv4only + } + + chain drop_from_ipv4only { + meta nfproto ipv4 ip saddr 192.168.1.0/24 counter drop comment "!fw4: drop ipv4only IPv4 traffic" + } + + chain drop_to_ipv4only { + meta nfproto ipv4 ip daddr 192.168.1.0/24 counter drop comment "!fw4: drop ipv4only IPv4 traffic" + } + + + # + # NAT rules + # + + chain dstnat { + type nat hook prerouting priority dstnat; policy accept; + meta nfproto ipv4 ip saddr 192.168.1.0/24 jump dstnat_ipv4only comment "!fw4: Handle ipv4only IPv4 dstnat traffic" + } + + chain srcnat { + type nat hook postrouting priority srcnat; policy accept; + } + + chain dstnat_ipv4only { + } + + + # + # Raw rules (notrack & helper) + # + + chain raw_prerouting { + type filter hook prerouting priority raw; policy accept; + } + + chain raw_output { + type filter hook output priority raw; policy accept; + } + + + # + # Mangle rules + # + + chain mangle_prerouting { + type filter hook prerouting priority mangle; policy accept; + } + + chain mangle_postrouting { + type filter hook postrouting priority mangle; policy accept; + } + + chain mangle_input { + type filter hook input priority mangle; policy accept; + } + + chain mangle_output { + type filter hook output priority mangle; policy accept; + } + + chain mangle_forward { + type filter hook forward priority mangle; policy accept; + } +} +-- End -- -- 2.30.2