From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id ABD781FF0E9 for ; Thu, 16 Jul 2026 11:37:20 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 44BD921442; Thu, 16 Jul 2026 11:37:20 +0200 (CEST) Date: Thu, 16 Jul 2026 11:36:43 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= Subject: Re: [PATCH pve-firewall v2 1/1] fix #7818: disallow vertical spaces in firewall comments To: Stefan Hanreich , Stoiko Ivanov References: <20260715101054.31295-1-s.hanreich@proxmox.com> <20260715130901.40d25fc1@rosa.proxmox.com> In-Reply-To: <20260715130901.40d25fc1@rosa.proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.17.0 (https://github.com/astroidmail/astroid) Message-Id: <1784194557.xe19whf3o3.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784194587310 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.174 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: XHDBZWDLL5QTYZRUCFF6NVKSCK5CI4LR X-Message-ID-Hash: XHDBZWDLL5QTYZRUCFF6NVKSCK5CI4LR X-MailFrom: f.gruenbichler@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On July 15, 2026 1:09 pm, Stoiko Ivanov wrote: > Thanks for the quick iteration. >=20 > gave it a minimal spin with a pvesh command i used to trigger the issue i= n > #7818 - catches the new-line as expected. >=20 > one nit inline - but with or without that: > Reviewed-by: Stoiko Ivanov > Tested-by: Stoiko Ivanov I went with v1, as the changes in v2 are not something we do anywhere else atm, and IMHO deserve some more analysis - and if we want to go down that route - should be applied to all such fields, not just this one. >=20 > On Wed, 15 Jul 2026 12:10:53 +0200 > Stefan Hanreich wrote: >=20 >> They do not get sanitized and interpreted literally, allowing for >> authenticated users to inject additional lines into the firewall >> configuration files. Additionally restrict the comments to only >> printable characters which additionally prevents control characters in >> comments. >>=20 >> Suggested-by: Stoiko Ivanov >> Signed-off-by: Stefan Hanreich >> --- >>=20 >> Notes: >> Changes from v1 (Thanks @Stoiko): >> * disallow any vertical space instead of line feeds only >> * additionally only allow printable characters >> =20 >> Did quickly check the other properties as well, but couldn't find >> anything. Will take a closer look still but wanted to get the patch >> ready in the meanwhile. >>=20 >> src/PVE/Firewall.pm | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >>=20 >> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm >> index 93f8c34..8ec2b80 100644 >> --- a/src/PVE/Firewall.pm >> +++ b/src/PVE/Firewall.pm >> @@ -1228,6 +1228,22 @@ sub pve_fw_verify_icmp_type_spec { >> return $icmp_type; >> } >> =20 >> +PVE::JSONSchema::register_format('pve-fw-comment-spec', \&pve_fw_verify= _comment_spec); >> + >> +sub pve_fw_verify_comment_spec { >> + my ($comment) =3D @_; >> + >> + if ($comment =3D~ m/\p{VertSpace}/) { >> + die "comment must not contain any line feeds\n"; > nit: would read "line feed" as `\n` only - not general vertical space. > but I think the message is expressive enough - and captures what went > wrong. >=20 >=20 >> + } >> + >> + if ($comment !~ m/^\p{Print}*$/) { >> + die "comment must only contain printable characters\n"; >> + } >> + >> + return $comment; >> +} >> + >> # helper function for API >> =20 >> sub copy_opject_with_digest { >> @@ -1623,6 +1639,7 @@ my $rule_properties =3D { >> description =3D> "Descriptive comment.", >> type =3D> 'string', >> optional =3D> 1, >> + format =3D> 'pve-fw-comment-spec', >> }, >> 'icmp-type' =3D> { >> description =3D> >=20 >=20 >=20 >=20 >=20 >=20