[pve-devel] [PATCH firewall 1/2] icmp: factor out check for relevant protocols

[pve-devel] [PATCH firewall 1/2] icmp: factor out check for relevant protocols

[Fabian Grünbichler]

this were not entirely consistent and sometimes the checks were repeated.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/PVE/Firewall.pm | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index a16c035..5fa264a 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -856,6 +856,11 @@ my $is_valid_icmp_type = sub {
     }
 };
 
+my $proto_is_icmp = sub {
+    my $proto = shift;
+    return $proto eq 'icmp' || $proto eq 'icmpv6' || $proto eq 'ipv6-icmp';
+};
+
 sub init_firewall_macros {
 
     $pve_fw_parsed_macros = {};
@@ -1540,7 +1545,7 @@ my $rule_properties = {
 	optional => 1,
     },
     'icmp-type' => {
-	description => "Specify icmp-type. Only valid if proto equals 'icmp'.",
+	description => "Specify icmp-type. Only valid if proto equals 'icmp' or 'icmpv6'/'ipv6-icmp'.",
 	type => 'string', format => 'pve-fw-icmp-type-spec',
 	optional => 1,
     },
@@ -1733,12 +1738,14 @@ sub verify_rule {
 	}
     }
 
+    my $is_icmp = 0;
     if ($rule->{proto}) {
 	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 'ipv6-icmp';
+	$is_icmp = $proto_is_icmp->($rule->{proto});
     }
 
     if ($rule->{dport}) {
@@ -1748,14 +1755,13 @@ sub verify_rule {
 	&$add_error('proto', "missing property - 'dport' requires this property")
 	    if !$proto;
 	&$add_error('dport', "protocol '$proto' does not support ports")
-	    if !$PROTOCOLS_WITH_PORTS->{$proto} &&
-		$proto ne 'icmp' && $proto ne 'icmpv6'; # special cases
+	    if !$PROTOCOLS_WITH_PORTS->{$proto} && !$is_icmp; #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';
+	    if !$is_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}) {
@@ -2138,6 +2144,7 @@ sub ipt_rule_to_cmds {
 
 	if (my $proto = $rule->{proto}) {
 	    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 $multisport = defined($rule->{sport}) && parse_port_name_number_or_range($rule->{sport}, 0);
@@ -2178,7 +2185,7 @@ sub ipt_rule_to_cmds {
 		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');
+		    if !$is_icmp;
 		my $type = $proto eq 'icmp' ? 'icmp-type' : 'icmpv6-type';
 
 		push @match, "-m $proto --$type $rule->{'icmp-type'}";
-- 
2.30.2

[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@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

[pve-devel] applied-series: [PATCH firewall 1/2] icmp: factor out check for relevant protocols

[Thomas Lamprecht]

Am 16/05/2023 um 11:09 schrieb Fabian Grünbichler:
> this were not entirely consistent and sometimes the checks were repeated.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>  src/PVE/Firewall.pm | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
>

applied series, thanks!