[pve-devel] [PATCH firewall 2/2] fix #4730: add safeguards to prevent ICMP type misuse

[Fabian Grünbichler]

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 at proxmox.com>
---

Notes:
    without this patch, it's possible to set such a rule over the GUI and the pve-firewall just logs:
    
    status update error: iptables_restore_cmdlist: Try `iptables-restore -h' or 'iptables-restore --help' for more information.
    
    and is no longer able to setup any rule (change).
    
    with this patch, any existing broken config will trigger the following:
    
    /etc/pve/nodes/<NODE>/host.fw (line 7) - errors in rule parameters: IN REJECT -source 10.0.1.0/24 -dest 10.0.1.2 -p tcp -dport any -log nolog
       dport: invalid port 'any'
    
    and creating such a broken rule in the first place is no longer possible over the API, and *only* adding this rule is skipped.

 src/PVE/Firewall.pm | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 5fa264a..8e40872 100644
--- 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 {
-- 
2.30.2