fix #4730: add safeguards to prevent ICMP type misuse

without this additional conditions, it's possible to break the firewall by
setting an ICMP-type value as dport for non-ICMP protocols, e.g. 'any' for
'tcp'.

by rejecting the invalid rule/parameter, the rest of the ruleset is still
applied properly, and the error messages are a lot more informative as well.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 5fa264a02584916c32ff74849c046d015e9e6320..8e40872fb13167878bca0e045bb2e4ad0857e98e 100644 (file)
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -1100,6 +1100,9 @@ sub parse_address_list {
     return $ipversion;
 }
 
+# $dport must only be set to 1 if the parsed parameter is dport and the
+# protocol is one of the ICMP variants - ICMP type values used to be stored in
+# the dport parameter.
 sub parse_port_name_number_or_range {
     my ($str, $dport) = @_;
 
@@ -1749,7 +1752,7 @@ sub verify_rule {
     }
 
     if ($rule->{dport}) {
-       eval { parse_port_name_number_or_range($rule->{dport}, 1); };
+       eval { parse_port_name_number_or_range($rule->{dport}, $is_icmp); };
        &$add_error('dport', $@) if $@;
        my $proto = $rule->{proto};
        &$add_error('proto', "missing property - 'dport' requires this property")
@@ -2146,7 +2149,7 @@ sub ipt_rule_to_cmds {
            push @match, "-p $proto";
            my $is_icmp = $proto_is_icmp->($proto);
 
-           my $multidport = defined($rule->{dport}) && parse_port_name_number_or_range($rule->{dport}, 1);
+           my $multidport = defined($rule->{dport}) && parse_port_name_number_or_range($rule->{dport}, $is_icmp);
            my $multisport = defined($rule->{sport}) && parse_port_name_number_or_range($rule->{sport}, 0);
 
            my $add_dport = sub {