fw4: rework and fix family inheritance logic
authorJo-Philipp Wich <jo@mein.io>
Sat, 12 Feb 2022 19:13:06 +0000 (20:13 +0100)
committerJo-Philipp Wich <jo@mein.io>
Sat, 12 Feb 2022 19:41:13 +0000 (20:41 +0100)
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 <jo@mein.io>
root/usr/share/ucode/fw4.uc
tests/03_rules/08_family_inheritance [new file with mode: 0644]

index fa65a469adb42a052d26c385e35c552f61b6c072..243e27c51627d5ebce602e5d3029f09df015d5d5 100644 (file)
@@ -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 (file)
index 0000000..9a6aa59
--- /dev/null
@@ -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 --