From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: pve-devel@lists.proxmox.com
Subject: [pve-devel] [PATCH firewall 2/2] fix #4730: add safeguards to prevent ICMP type misuse
Date: Tue, 16 May 2023 11:09:24 +0200 [thread overview]
Message-ID: <20230516090924.1944193-2-f.gruenbichler@proxmox.com> (raw)
In-Reply-To: <20230516090924.1944193-1-f.gruenbichler@proxmox.com>
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
next prev parent reply other threads:[~2023-05-16 9:09 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-16 9:09 [pve-devel] [PATCH firewall 1/2] icmp: factor out check for relevant protocols Fabian Grünbichler
2023-05-16 9:09 ` Fabian Grünbichler [this message]
2023-05-16 9:17 ` [pve-devel] applied-series: " Thomas Lamprecht
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230516090924.1944193-2-f.gruenbichler@proxmox.com \
--to=f.gruenbichler@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox