]> git.proxmox.com Git - pve-firewall.git/blobdiff - src/PVE/Firewall.pm
fix variables declared in conditional statement
[pve-firewall.git] / src / PVE / Firewall.pm
index 84f71d6fcfebc161c97043a3bf0c69e25d1906d8..a16c035aace9506be8a67604eacbdc5b88e59c18 100644 (file)
@@ -24,6 +24,8 @@ use PVE::SafeSyslog;
 use PVE::Tools qw($IPV4RE $IPV6RE);
 use PVE::Tools qw(run_command lock_file dir_glob_foreach);
 
+use PVE::Firewall::Helpers;
+
 my $pvefw_conf_dir = "/etc/pve/firewall";
 my $clusterfw_conf_filename = "$pvefw_conf_dir/cluster.fw";
 
@@ -66,9 +68,13 @@ PVE::JSONSchema::register_format('IPorCIDR', \&pve_verify_ip_or_cidr);
 sub pve_verify_ip_or_cidr {
     my ($cidr, $noerr) = @_;
 
-    if ($cidr =~ m!^(?:$IPV6RE|$IPV4RE)(/(\d+))?$!) {
-       return $cidr if Net::IP->new($cidr);
+    if ($cidr =~ m!^(?:$IPV6RE|$IPV4RE)(?:/\d+)?$!) {
+        # Net::IP throws an error if the masked CIDR part isn't zero, e.g., `192.168.1.155/24`
+        # fails but `192.168.1.0/24` succeeds. clean_cidr removes the non zero bits from the CIDR.
+       my $clean_cidr = clean_cidr($cidr);
+       return $cidr if Net::IP->new($clean_cidr);
        return undef if $noerr;
+
        die Net::IP::Error() . "\n";
     }
     return undef if $noerr;
@@ -84,6 +90,19 @@ sub pve_verify_ip_or_cidr_or_alias {
     return pve_verify_ip_or_cidr($cidr, $noerr);
 }
 
+sub clean_cidr {
+    my ($cidr) = @_;
+    my ($ip, $len) = split('/', $cidr);
+    return $cidr if !$len;
+    my $ver = ($ip =~ m!^$IPV4RE$!) ? 4 : 6;
+
+    my $bin_ip = Net::IP::ip_iptobin( Net::IP::ip_expand_address($ip, $ver), $ver);
+    my $bin_mask = Net::IP::ip_get_mask($len, $ver);
+    my $clean_ip = Net::IP::ip_compress_address( Net::IP::ip_bintoip($bin_ip & $bin_mask, $ver), $ver);
+
+    return "${clean_ip}/$len";
+}
+
 PVE::JSONSchema::register_standard_option('ipset-name', {
     description => "IP set name.",
     type => 'string',
@@ -213,7 +232,7 @@ my $pve_fw_macros = {
        { action => 'PARAM', proto => 'udp', dport => '6881' },
     ],
     'Ceph' => [
-        "Ceph Storage Cluster traffic (Ceph Monitors, OSD & MDS Deamons)",
+        "Ceph Storage Cluster traffic (Ceph Monitors, OSD & MDS Daemons)",
        # Legacy port for protocol v1
         { action => 'PARAM', proto => 'tcp', dport => '6789' },
        # New port for protocol v2
@@ -276,7 +295,7 @@ my $pve_fw_macros = {
        { action => 'PARAM', proto => 'tcp', dport => '9418' },
     ],
     'HKP' => [
-       "OpenPGP HTTP keyserver protocol traffic",
+       "OpenPGP HTTP key server protocol traffic",
        { action => 'PARAM', proto => 'tcp', dport => '11371' },
     ],
     'HTTP' => [
@@ -494,6 +513,10 @@ my $pve_fw_macros = {
        { action => 'PARAM', proto => '41' },
        { action => 'PARAM', proto => 'udp', dport => '5072,8374' },
     ],
+    'SPICEproxy' => [
+       "Proxmox VE SPICE display proxy traffic",
+       { action => 'PARAM', proto => 'tcp', dport => '3128' },
+    ],
     'Squid' => [
        "Squid web proxy traffic",
        { action => 'PARAM', proto => 'tcp', dport => '3128' },
@@ -555,6 +578,18 @@ my $pve_fw_macros = {
     ],
 };
 
+my $pve_fw_helpers = {
+    'amanda' => { proto => 'udp', dport => '10080', 'v4' => 1, 'v6' => 1 },
+    'ftp' => { proto => 'tcp', dport => '21', 'v4' => 1, 'v6' => 1},
+    'irc' => { proto => 'tcp', dport => '6667', 'v4' => 1 },
+    'netbios-ns' => { proto => 'udp', dport => '137', 'v4' => 1 },
+    'pptp' => { proto => 'tcp', dport => '1723', 'v4' => 1, },
+    'sane' => { proto => 'tcp', dport => '6566', 'v4' => 1, 'v6' => 1 },
+    'sip' => { proto => 'udp', dport => '5060', 'v4' => 1, 'v6' => 1 },
+    'snmp' => { proto => 'udp', dport => '161', 'v4' => 1 },
+    'tftp' => { proto => 'udp', dport => '69', 'v4' => 1, 'v6' => 1},
+};
+
 my $pve_fw_parsed_macros;
 my $pve_fw_macro_descr;
 my $pve_fw_macro_ipversion = {};
@@ -592,7 +627,6 @@ $pve_std_chains_conf->{4} = {
        # same as shorewall 'Drop', which is equal to DROP,
        # but REJECT/DROP some packages to reduce logging,
        # and ACCEPT critical ICMP types
-       { action => 'PVEFW-reject',  proto => 'tcp', dport => '43' }, # REJECT 'auth'
        # we are not interested in BROADCAST/MULTICAST/ANYCAST
        { action => 'PVEFW-DropBroadcast' },
        # ACCEPT critical ICMP types
@@ -615,7 +649,6 @@ $pve_std_chains_conf->{4} = {
        # same as shorewall 'Reject', which is equal to Reject,
        # but REJECT/DROP some packages to reduce logging,
        # and ACCEPT critical ICMP types
-       { action => 'PVEFW-reject',  proto => 'tcp', dport => '43' }, # REJECT 'auth'
        # we are not interested in BROADCAST/MULTICAST/ANYCAST
        { action => 'PVEFW-DropBroadcast' },
        # ACCEPT critical ICMP types
@@ -636,7 +669,7 @@ $pve_std_chains_conf->{4} = {
     ],
     'PVEFW-tcpflags' => [
        # same as shorewall tcpflags action.
-       # Packets arriving on this interface are checked for som illegal combinations of TCP flags
+       # Packets arriving on this interface are checked for some illegal combinations of TCP flags
        { match => '-p tcp -m tcp --tcp-flags FIN,SYN,RST,PSH,ACK,URG FIN,PSH,URG', target => '-g PVEFW-logflags' },
        { match => '-p tcp -m tcp --tcp-flags FIN,SYN,RST,PSH,ACK,URG NONE', target => '-g PVEFW-logflags' },
        { match => '-p tcp -m tcp --tcp-flags SYN,RST SYN,RST', target => '-g PVEFW-logflags' },
@@ -729,7 +762,7 @@ $pve_std_chains_conf->{6} = {
     ],
     'PVEFW-tcpflags' => [
        # same as shorewall tcpflags action.
-       # Packets arriving on this interface are checked for som illegal combinations of TCP flags
+       # Packets arriving on this interface are checked for some illegal combinations of TCP flags
        { match => '-p tcp -m tcp --tcp-flags FIN,SYN,RST,PSH,ACK,URG FIN,PSH,URG', target => '-g PVEFW-logflags' },
        { match => '-p tcp -m tcp --tcp-flags FIN,SYN,RST,PSH,ACK,URG NONE', target => '-g PVEFW-logflags' },
        { match => '-p tcp -m tcp --tcp-flags SYN,RST SYN,RST', target => '-g PVEFW-logflags' },
@@ -785,12 +818,14 @@ my $icmp_type_names = {
 # ip6tables -p icmpv6 -h
 
 my $icmpv6_type_names = {
-    'any' => 1,
     'destination-unreachable' => 1,
     'no-route' => 1,
     'communication-prohibited' => 1,
+    'beyond-scope' => 1,
     'address-unreachable' => 1,
     'port-unreachable' => 1,
+    'failed-policy' => 1,
+    'reject-route' => 1,
     'packet-too-big' => 1,
     'time-exceeded' => 1,
     'ttl-zero-during-transit' => 1,
@@ -810,6 +845,17 @@ my $icmpv6_type_names = {
     'redirect' => 1,
 };
 
+my $is_valid_icmp_type = sub {
+    my ($type, $valid_types) = @_;
+
+    if ($type =~ m/^\d+$/) {
+       # values for icmp-type range between 0 and 255 (8 bit field)
+       die "invalid icmp-type '$type'\n" if $type > 255;
+    } else {
+       die "unknown icmp-type '$type'\n" if !defined($valid_types->{$type});
+    }
+};
+
 sub init_firewall_macros {
 
     $pve_fw_parsed_macros = {};
@@ -1080,7 +1126,7 @@ sub parse_port_name_number_or_range {
        }
     }
 
-    die "ICPM ports not allowed in port range\n" if $icmp_port && $count > 0;
+    die "ICMP ports not allowed in port range\n" if $icmp_port && $count > 0;
 
     # I really don't like to use the word number here, but it's the only thing
     # that makes sense in a literal way. The range 1:100 counts as 2, not as
@@ -1091,6 +1137,19 @@ sub parse_port_name_number_or_range {
     return (scalar(@elements) > 1);
 }
 
+PVE::JSONSchema::register_format('pve-fw-conntrack-helper', \&pve_fw_verify_conntrack_helper);
+sub pve_fw_verify_conntrack_helper {
+   my ($list) = @_;
+
+   my @helpers = split(/,/, $list);
+   die "extraneous commas in list\n" if $list ne join(',', @helpers);
+   foreach my $helper (@helpers) {
+       die "unknown helper $helper" if !$pve_fw_helpers->{$helper};
+   }
+
+   return $list;
+}
+
 PVE::JSONSchema::register_format('pve-fw-sport-spec', \&pve_fw_verify_sport_spec);
 sub pve_fw_verify_sport_spec {
    my ($portstr) = @_;
@@ -1131,6 +1190,19 @@ sub pve_fw_verify_protocol_spec {
    return $proto;
 }
 
+PVE::JSONSchema::register_format('pve-fw-icmp-type-spec', \&pve_fw_verify_icmp_type_spec);
+sub pve_fw_verify_icmp_type_spec {
+    my ($icmp_type) = @_;
+
+    if ($icmp_type_names->{$icmp_type} ||  $icmpv6_type_names->{$icmp_type}) {
+       return $icmp_type;
+    }
+
+    die "invalid icmp-type value '$icmp_type'\n" if $icmp_type ne '';
+
+    return $icmp_type;
+}
+
 
 # helper function for API
 
@@ -1230,7 +1302,7 @@ our $cluster_option_properties = {
                type => 'integer',
                minimum => 0,
                optional => 1,
-               description => 'Inital burst of packages which will get logged',
+               description => 'Initial burst of packages which will always get logged before the rate is applied',
                default => 5,
            },
        },
@@ -1297,6 +1369,13 @@ our $host_option_properties = {
        default => 0,
        optional => 1,
     },
+    nf_conntrack_helpers => {
+       type => 'string', format => 'pve-fw-conntrack-helper',
+       description => "Enable conntrack helpers for specific protocols. ".
+           "Supported protocols: amanda, ftp, irc, netbios-ns, pptp, sane, sip, snmp, tftp",
+       default => '',
+       optional => 1,
+    },
     protection_synflood => {
        description => "Enable synflood protection",
        type => 'boolean',
@@ -1333,7 +1412,7 @@ our $vm_option_properties = {
     macfilter => {
        description => "Enable/disable MAC address filter.",
        type => 'boolean',
-       default => 0,
+       default => 1,
        optional => 1,
     },
     dhcp => {
@@ -1423,11 +1502,13 @@ my $rule_properties = {
        description => "Restrict packet source address. $addr_list_descr",
        type => 'string', format => 'pve-fw-addr-spec',
        optional => 1,
+       maxLength => 512,
     },
     dest => {
        description => "Restrict packet destination address. $addr_list_descr",
        type => 'string', format => 'pve-fw-addr-spec',
        optional => 1,
+       maxLength => 512,
     },
     proto => {
        description => "IP protocol. You can use protocol names ('tcp'/'udp') or simple numbers, as defined in '/etc/protocols'.",
@@ -1458,6 +1539,11 @@ my $rule_properties = {
        type => 'string',
        optional => 1,
     },
+    'icmp-type' => {
+       description => "Specify icmp-type. Only valid if proto equals 'icmp'.",
+       type => 'string', format => 'pve-fw-icmp-type-spec',
+       optional => 1,
+    },
 };
 
 sub add_rule_properties {
@@ -1578,7 +1664,7 @@ sub verify_rule {
     my $set_ip_version = sub {
        my $vers = shift;
        if ($vers) {
-           die "detected mixed ipv4/ipv6 adresses in rule\n"
+           die "detected mixed ipv4/ipv6 addresses in rule\n"
                if $ipversion && ($vers != $ipversion);
            $ipversion = $vers;
        }
@@ -1651,7 +1737,8 @@ sub verify_rule {
        eval { pve_fw_verify_protocol_spec($rule->{proto}); };
        &$add_error('proto', $@) if $@;
        &$set_ip_version(4) if $rule->{proto} eq 'icmp';
-       &$set_ip_version(6) if $rule->{proto} eq 'icmpv6';
+       &$set_ip_version(6) if $rule->{proto} eq 'icmpv6';
+       &$set_ip_version(6) if $rule->{proto} eq 'ipv6-icmp';
     }
 
     if ($rule->{dport}) {
@@ -1665,6 +1752,19 @@ sub verify_rule {
                $proto ne 'icmp' && $proto ne 'icmpv6'; # special cases
     }
 
+    if (my $icmp_type = $rule ->{'icmp-type'}) {
+       my $proto = $rule->{proto};
+       &$add_error('proto', "missing property - 'icmp-type' requires this property")
+           if $proto ne 'icmp' && $proto ne 'icmpv6' && $proto ne 'ipv6-icmp';
+       &$add_error('icmp-type', "'icmp-type' cannot be specified together with 'dport'")
+           if $rule->{dport};
+       if ($proto eq 'icmp' && !$icmp_type_names->{$icmp_type}) {
+           &$add_error('icmp-type', "invalid icmp-type '$icmp_type' for proto 'icmp'");
+       } elsif (($proto eq 'icmpv6' || $proto eq 'ipv6-icmp') && !$icmpv6_type_names->{$icmp_type}) {
+           &$add_error('icmp-type', "invalid icmp-type '$icmp_type' for proto '$proto'");
+       }
+    }
+
     if ($rule->{sport}) {
        eval { parse_port_name_number_or_range($rule->{sport}, 0); };
        &$add_error('sport', $@) if $@;
@@ -1771,11 +1871,9 @@ sub rules_audit_permissions {
 }
 
 # core functions
-my $bridge_firewall_enabled = 0;
 
 sub enable_bridge_firewall {
 
-    return if $bridge_firewall_enabled; # only once
 
     PVE::ProcFSTools::write_proc_entry("/proc/sys/net/bridge/bridge-nf-call-iptables", "1");
     PVE::ProcFSTools::write_proc_entry("/proc/sys/net/bridge/bridge-nf-call-ip6tables", "1");
@@ -1783,7 +1881,6 @@ sub enable_bridge_firewall {
     # make sure syncookies are enabled (which is default on newer 3.X kernels anyways)
     PVE::ProcFSTools::write_proc_entry("/proc/sys/net/ipv4/tcp_syncookies", "1");
 
-    $bridge_firewall_enabled = 1;
 }
 
 sub iptables_restore_cmdlist {
@@ -1905,6 +2002,8 @@ sub ipset_get_chains {
        return if $line =~ m/^\s*$/;
        if ($line =~ m/^(?:\S+)\s(PVEFW-\S+)\s(?:\S+).*/) {
            my $chain = $1;
+           # ignore initval from ipset v7.7+, won't set that yet so it'd mess up change detection
+           $line =~ s/\binitval 0x[0-9a-f]+//;
            $line =~ s/\s+$//; # delete trailing white space
            push @{$chains->{$chain}}, $line;
        } else {
@@ -1927,13 +2026,22 @@ sub ebtables_get_chains {
 
     my $res = {};
     my $chains = {};
+    my $table;
     my $parser = sub {
        my $line = shift;
        return if $line =~ m/^#/;
        return if $line =~ m/^\s*$/;
-       if ($line =~ m/^:(\S+)\s\S+$/) {
+       if ($line =~ m/^\*(\S+)$/) {
+           $table = $1;
+           return;
+       }
+
+       return if $table ne "filter";
+
+       if ($line =~ m/^:(\S+)\s(ACCEPT|DROP|RETURN)$/) {
            # Make sure we know chains exist even if they're empty.
            $chains->{$1} //= [];
+           $res->{$1}->{policy} = $2;
        } elsif ($line =~ m/^(?:\S+)\s(\S+)\s(?:\S+).*/) {
            my $chain = $1;
            $line =~ s/\s+$//;
@@ -1953,7 +2061,7 @@ sub ebtables_get_chains {
     return $res;
 }
 
-# substitude action of rule according to action hash
+# substitute action of rule according to action hash
 sub rule_substitude_action {
     my ($rule, $actions) = @_;
 
@@ -2035,23 +2143,21 @@ sub ipt_rule_to_cmds {
            my $multisport = defined($rule->{sport}) && parse_port_name_number_or_range($rule->{sport}, 0);
 
            my $add_dport = sub {
-               return if !$rule->{dport};
+               return if !defined($rule->{dport});
 
+               # NOTE: we re-use dport to store --icmp-type for icmp* protocol
                if ($proto eq 'icmp') {
-                   # Note: we use dport to store --icmp-type
-                   die "unknown icmp-type '$rule->{dport}'\n"
-                       if $rule->{dport} !~ /^\d+$/ && !defined($icmp_type_names->{$rule->{dport}});
+                   $is_valid_icmp_type->($rule->{dport}, $icmp_type_names);
                    push @match, "-m icmp --icmp-type $rule->{dport}";
                } elsif ($proto eq 'icmpv6') {
-                   # Note: we use dport to store --icmpv6-type
-                   die "unknown icmpv6-type '$rule->{dport}'\n"
-                       if $rule->{dport} !~ /^\d+$/ && !defined($icmpv6_type_names->{$rule->{dport}});
+                   $is_valid_icmp_type->($rule->{dport}, $icmpv6_type_names);
                    push @match, "-m icmpv6 --icmpv6-type $rule->{dport}";
                } elsif (!$PROTOCOLS_WITH_PORTS->{$proto}) {
                    die "protocol $proto does not have ports\n";
                } elsif ($multidport) {
                    push @match, "--match multiport", "--dports $rule->{dport}";
                } else {
+                   return if !$rule->{dport};
                    push @match, "--dport $rule->{dport}";
                }
            };
@@ -2068,7 +2174,18 @@ sub ipt_rule_to_cmds {
                }
            };
 
+           my $add_icmp_type = sub {
+               return if !defined($rule->{'icmp-type'}) || $rule->{'icmp-type'} eq '';
+
+               die "'icmp-type' can only be set if 'icmp', 'icmpv6' or 'ipv6-icmp' is specified\n"
+                   if ($proto ne 'icmp') && ($proto ne 'icmpv6') && ($proto ne 'ipv6-icmp');
+               my $type = $proto eq 'icmp' ? 'icmp-type' : 'icmpv6-type';
+
+               push @match, "-m $proto --$type $rule->{'icmp-type'}";
+           };
+
            # order matters - single port before multiport!
+           $add_icmp_type->();
            $add_dport->() if $multisport;
            $add_sport->();
            $add_dport->() if !$multisport;
@@ -2086,8 +2203,7 @@ sub ipt_rule_to_cmds {
        $targetstr = $rule->{target};
     } else {
        my $action = (defined $rule->{action}) ? $rule->{action} : "";
-       my $goto = 1 if $action eq 'PVEFW-SET-ACCEPT-MARK';
-       $targetstr = ($goto) ? "-g $action" : "-j $action";
+       $targetstr = $action eq 'PVEFW-SET-ACCEPT-MARK' ? "-g $action" : "-j $action";
     }
 
     my @iptcmds;
@@ -2401,7 +2517,8 @@ sub generate_tap_rules_direction {
     my $tapchain = "$iface-$direction";
 
     my $ipfilter_name = compute_ipfilter_ipset_name($netid);
-    my $ipfilter_ipset = compute_ipset_chain_name($vmid, $ipfilter_name, $ipversion)
+    my $ipfilter_ipset;
+    $ipfilter_ipset = compute_ipset_chain_name($vmid, $ipfilter_name, $ipversion)
        if $options->{ipfilter} || $vmfw_conf->{ipset}->{$ipfilter_name};
 
     if ($options->{enable}) {
@@ -2692,32 +2809,36 @@ sub parse_fw_rule {
 
        last if $rule->{type} eq 'group';
 
-       if ($line =~ s/^-p (\S+)\s*//) {
+       if ($line =~ s/^(?:-p|--?proto) (\S+)\s*//) {
            $rule->{proto} = $1;
            next;
        }
 
-       if ($line =~ s/^-dport (\S+)\s*//) {
+       if ($line =~ s/^--?dport (\S+)\s*//) {
            $rule->{dport} = $1;
            next;
        }
 
-       if ($line =~ s/^-sport (\S+)\s*//) {
+       if ($line =~ s/^--?sport (\S+)\s*//) {
            $rule->{sport} = $1;
            next;
        }
-       if ($line =~ s/^-source (\S+)\s*//) {
+       if ($line =~ s/^--?source (\S+)\s*//) {
            $rule->{source} = $1;
            next;
        }
-       if ($line =~ s/^-dest (\S+)\s*//) {
+       if ($line =~ s/^--?dest (\S+)\s*//) {
            $rule->{dest} = $1;
            next;
        }
-       if ($line =~ s/^-log (emerg|alert|crit|err|warning|notice|info|debug|nolog)\s*//) {
+       if ($line =~ s/^--?log (emerg|alert|crit|err|warning|notice|info|debug|nolog)\s*//) {
            $rule->{log} = $1;
            next;
        }
+       if ($line =~ s/^--?icmp-type (\S+)\s*//) {
+           $rule->{'icmp-type'} = $1;
+           next;
+       }
 
        last;
     }
@@ -2790,6 +2911,10 @@ sub parse_hostfw_option {
     } elsif ($line =~ m/^(log_level_in|log_level_out|tcp_flags_log_level|smurf_log_level):\s*(($loglevels)\s*)?$/i) {
        $opt = lc($1);
        $value = $2 ? lc($3) : '';
+    } elsif ($line =~ m/^(nf_conntrack_helpers):\s*(((\S+)[,]?)+)\s*$/i) {
+       $opt = lc($1);
+       $value = lc($2);
+       pve_fw_verify_conntrack_helper($value);
     } elsif ($line =~ m/^(nf_conntrack_max|nf_conntrack_tcp_timeout_established|nf_conntrack_tcp_timeout_syn_recv|protection_synflood_rate|protection_synflood_burst|protection_limit):\s*(\d+)\s*$/i) {
        $opt = lc($1);
        $value = int($2);
@@ -2861,7 +2986,7 @@ sub parse_alias {
     my ($line) = @_;
 
     # we can add single line comments to the end of the line
-    my $comment = decode('utf8', $1) if $line =~ s/\s*#\s*(.*?)\s*$//;
+    my $comment = $line =~ s/\s*#\s*(.*?)\s*$// ? decode('utf8', $1) : undef;
 
     if ($line =~ m/^(\S+)\s(\S+)$/) {
        my ($name, $cidr) = ($1, $2);
@@ -2897,6 +3022,8 @@ sub generic_fw_config_parser {
     }
     return {} if !$raw;
 
+    my $curr_group_keys = {};
+
     my $linenr = 0;
     while ($raw =~ /^\h*(.*?)\h*$/gm) {
        my $line = $1;
@@ -2957,6 +3084,8 @@ sub generic_fw_config_parser {
            }
 
            $res->{$section}->{$group} = [];
+           $curr_group_keys = {};
+
            $res->{ipset_comments}->{$group} = decode('utf8', $comment)
                if $comment;
            next;
@@ -3004,7 +3133,7 @@ sub generic_fw_config_parser {
            push @{$res->{$section}->{$group}}, $rule;
        } elsif ($section eq 'ipset') {
            # we can add single line comments to the end of the rule
-           my $comment = decode('utf8', $1) if $line =~ s/#\s*(.*?)\s*$//;
+           my $comment = $line =~ s/#\s*(.*?)\s*$// ? decode('utf8', $1) : undef;
 
            $line =~ m/^(\!)?\s*(\S+)\s*$/;
            my $nomatch = $1;
@@ -3021,6 +3150,8 @@ sub generic_fw_config_parser {
                } else {
                    $cidr = parse_ip_or_cidr($cidr);
                }
+               die "duplicate ipset entry for '$cidr'\n"
+                   if defined($curr_group_keys->{$cidr});
            };
            if (my $err = $@) {
                chomp $err;
@@ -3044,6 +3175,7 @@ sub generic_fw_config_parser {
            }
 
            push @{$res->{$section}->{$group}}, $entry;
+           $curr_group_keys->{$cidr} = 1;
        } else {
            warn "$prefix: skip line - unknown section\n";
            next;
@@ -3091,28 +3223,21 @@ sub read_local_vm_config {
                }
            }
         } elsif ($d->{type} eq 'lxc') {
-            if ($have_lxc) {
-                my $cfspath = PVE::LXC::Config->cfs_config_path($vmid);
-                if (my $conf = PVE::Cluster::cfs_read_file($cfspath)) {
-                    $lxc->{$vmid} = $conf;
-                }
-            }
-        }
+           if ($have_lxc) {
+               my $cfspath = PVE::LXC::Config->cfs_config_path($vmid);
+               if (my $conf = PVE::Cluster::cfs_read_file($cfspath)) {
+                   $lxc->{$vmid} = $conf;
+               }
+           }
+       }
     }
 
     return $vmdata;
 };
 
+# FIXME: move use sites over to moved helper and break older packages, then remove this here
 sub lock_vmfw_conf {
-    my ($vmid, $timeout, $code, @param) = @_;
-
-    die "can't lock VM firewall config for undefined VMID\n"
-       if !defined($vmid);
-
-    my $res = PVE::Cluster::cfs_lock_firewall("vm-$vmid", $timeout, $code, @param);
-    die $@ if $@;
-
-    return $res;
+    return PVE::Firewall::Helpers::lock_vmfw_conf(@_);
 }
 
 sub load_vmfw_conf {
@@ -3160,6 +3285,7 @@ my $format_rules = sub {
                $raw .= " -dport $rule->{dport}" if $rule->{dport};
                $raw .= " -sport $rule->{sport}" if $rule->{sport};
                $raw .= " -log $rule->{log}" if $rule->{log};
+               $raw .= " -icmp-type $rule->{'icmp-type'}" if defined($rule->{'icmp-type'}) && $rule->{'icmp-type'} ne '';
            }
 
            $raw .= " # " . encode('utf8', $rule->{comment})
@@ -3221,7 +3347,13 @@ my $format_ipsets = sub {
 
        my $nethash = {};
        foreach my $entry (@$options) {
-           $nethash->{$entry->{cidr}} = $entry;
+           my $cidr = $entry->{cidr};
+           if (defined($nethash->{$cidr})) {
+               warn "ignoring duplicate ipset entry '$cidr'\n";
+               next;
+           }
+
+           $nethash->{$cidr} = $entry;
        }
 
        foreach my $cidr (sort keys %$nethash) {
@@ -3268,27 +3400,14 @@ sub save_vmfw_conf {
     }
 }
 
+# FIXME: remove with 8.0 and break older qemu-server/pve-container
 sub remove_vmfw_conf {
-    my ($vmid) = @_;
-
-    my $vmfw_conffile = "$pvefw_conf_dir/$vmid.fw";
-
-    unlink $vmfw_conffile;
+    return PVE::Firewall::Helpers::remove_vmfw_conf(@_);
 }
 
+# FIXME: remove with 8.0 and break older qemu-server/pve-container
 sub clone_vmfw_conf {
-    my ($vmid, $newid) = @_;
-
-    my $sourcevm_conffile = "$pvefw_conf_dir/$vmid.fw";
-    my $clonevm_conffile = "$pvefw_conf_dir/$newid.fw";
-
-    if (-f $clonevm_conffile) {
-       unlink $clonevm_conffile;
-    }
-    if (-f $sourcevm_conffile) {
-       my $data = PVE::Tools::file_get_contents($sourcevm_conffile);
-       PVE::Tools::file_set_contents($clonevm_conffile, $data);
-    }
+    return PVE::Firewall::Helpers::clone_vmfw_conf(@_);
 }
 
 sub read_vm_firewall_configs {
@@ -3411,9 +3530,13 @@ sub generate_ipset_chains {
                $hashsize = round_powerof2($hashsize);
            }
 
+           my $bucketsize = 12; # lower than the default of 14, faster but slightly more memory use
+
            my $family = $ipversion == "6" ? "inet6" : "inet";
 
-           $ipset_ruleset->{$name} = ["create $name hash:net family $family hashsize $hashsize maxelem $hashsize"];
+           $ipset_ruleset->{$name} = [
+               "create $name hash:net family $family hashsize $hashsize maxelem $hashsize bucketsize $bucketsize"
+           ];
 
            foreach my $cidr (sort keys %$data) {
                my $entry = $data->{$cidr};
@@ -3534,10 +3657,12 @@ sub save_clusterfw_conf {
     }
 }
 
-sub lock_hostfw_conf {
-    my ($timeout, $code, @param) = @_;
+sub lock_hostfw_conf : prototype($$$@) {
+    my ($node, $timeout, $code, @param) = @_;
+
+    $node = $nodename if !defined($node);
 
-    my $res = PVE::Cluster::cfs_lock_firewall("host-$nodename", $timeout, $code, @param);
+    my $res = PVE::Cluster::cfs_lock_firewall("host-$node", $timeout, $code, @param);
     die $@ if $@;
 
     return $res;
@@ -3553,7 +3678,9 @@ sub load_hostfw_conf {
 }
 
 sub save_hostfw_conf {
-    my ($hostfw_conf) = @_;
+    my ($hostfw_conf, $filename) = @_;
+
+    $filename = $hostfw_conf_filename if !defined($filename);
 
     my $raw = '';
 
@@ -3568,9 +3695,9 @@ sub save_hostfw_conf {
     }
 
     if ($raw) {
-       PVE::Tools::file_set_contents($hostfw_conf_filename, $raw);
+       PVE::Tools::file_set_contents($filename, $raw);
     } else {
-       unlink $hostfw_conf_filename;
+       unlink $filename;
     }
 }
 
@@ -3638,6 +3765,9 @@ sub compile_iptables_raw {
 
     my $hostfw_options = $hostfw_conf->{options} || {};
     my $protection_synflood = $hostfw_options->{protection_synflood} || 0;
+    my $conntrack_helpers = $hostfw_options->{nf_conntrack_helpers} || '';
+
+    ruleset_create_chain($ruleset, "PVEFW-PREROUTING") if $protection_synflood != 0 || $conntrack_helpers ne '';
 
     if($protection_synflood) {
 
@@ -3648,10 +3778,14 @@ sub compile_iptables_raw {
        $protection_synflood_expire = $protection_synflood_expire * 1000;
        my $protection_synflood_mask = $ipversion == 4 ? 32 : 64;
 
-       ruleset_create_chain($ruleset, "PVEFW-PREROUTING");
        ruleset_addrule($ruleset, "PVEFW-PREROUTING", "-p tcp -m tcp --tcp-flags FIN,SYN,RST,ACK SYN -m hashlimit --hashlimit-above $protection_synflood_rate/sec --hashlimit-burst $protection_synflood_burst --hashlimit-mode srcip --hashlimit-name syn --hashlimit-htable-size 2097152 --hashlimit-srcmask $protection_synflood_mask --hashlimit-htable-expire $protection_synflood_expire", "-j DROP");
     }
 
+    foreach my $conntrack_helper (split(/,/, $conntrack_helpers)) {
+       my $helper = $pve_fw_helpers->{$conntrack_helper};
+       ruleset_addrule($ruleset, "PVEFW-PREROUTING", "-p $helper->{proto} -m $helper->{proto} --dport $helper->{dport} -j CT", "--helper $conntrack_helper") if $helper && $helper->{"v$ipversion"};
+    }
+
     return $ruleset;
 }
 
@@ -3782,7 +3916,7 @@ sub compile_ipsets {
            return if !$vmfw_conf;
 
            # When the 'ipfilter' option is enabled every device for which there
-           # is no 'ipfilter-netX' ipset defiend gets an implicit empty default
+           # is no 'ipfilter-netX' ipset defined gets an implicit empty default
            # ipset.
            # The reason is that ipfilter ipsets are always filled with standard
            # IPv6 link-local filters.
@@ -3821,7 +3955,7 @@ sub compile_ipsets {
            return if !$vmfw_conf;
 
            # When the 'ipfilter' option is enabled every device for which there
-           # is no 'ipfilter-netX' ipset defiend gets an implicit empty default
+           # is no 'ipfilter-netX' ipset defined gets an implicit empty default
            # ipset.
            # The reason is that ipfilter ipsets are always filled with standard
            # IPv6 link-local filters, as well as the IP addresses configured
@@ -3886,7 +4020,7 @@ sub compile_ebtables_filter {
        eval {
            my $conf = $vmdata->{qemu}->{$vmid};
            my $vmfw_conf = $vmfw_configs->{$vmid};
-           return if !$vmfw_conf;
+           return if !$vmfw_conf || !$vmfw_conf->{options}->{enable};
            my $ipsets = $vmfw_conf->{ipset};
 
            foreach my $netid (sort keys %$conf) {
@@ -3962,7 +4096,7 @@ sub generate_tap_layer2filter {
     ruleset_create_chain($ruleset, $tapchain);
 
     if (defined($macaddr) && !(defined($options->{macfilter}) && $options->{macfilter} == 0)) {
-           ruleset_addrule($ruleset, $tapchain, "-s ! $macaddr", '-j DROP');
+       ruleset_addrule($ruleset, $tapchain, "-s ! $macaddr", '-j DROP');
     }
 
     if (@$arpfilter){
@@ -4037,6 +4171,7 @@ sub get_ruleset_status {
        if (defined($change_only_regex)) {
            $action = 'ignore' if ($chain !~ m/$change_only_regex/);
            $statushash->{$chain}->{rules} = $active_chains->{$chain}->{rules};
+           $statushash->{$chain}->{policy} = $active_chains->{$chain}->{policy};
            $sig = $sig->{sig};
        }
        $statushash->{$chain}->{action} = $action;
@@ -4137,7 +4272,8 @@ sub get_ebtables_cmdlist {
     my $pve_include = 0;
     foreach my $chain (sort keys %$statushash) {
        next if ($statushash->{$chain}->{action} eq 'delete');
-       $cmdlist .= ":$chain ACCEPT\n";
+       my $policy = $statushash->{$chain}->{policy} // 'ACCEPT';
+       $cmdlist .= ":$chain $policy\n";
        $pve_include = 1 if ($chain eq 'PVEFW-FORWARD');
     }