Re: [PATCH pve-firewall v2 1/1] fix #7818: disallow vertical spaces in firewall comments

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On July 15, 2026 1:09 pm, Stoiko Ivanov wrote:
> Thanks for the quick iteration.
> 
> gave it a minimal spin with a pvesh command i used to trigger the issue in
> #7818 - catches the new-line as expected.
> 
> one nit inline - but with or without that:
> Reviewed-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> Tested-by: Stoiko Ivanov <s.ivanov@proxmox.com>

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.

> 
> On Wed, 15 Jul 2026 12:10:53 +0200
> Stefan Hanreich <s.hanreich@proxmox.com> wrote:
> 
>> 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.
>> 
>> Suggested-by: Stoiko Ivanov <s.ivanov@proxmox.com>
>> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
>> ---
>> 
>> Notes:
>>     Changes from v1 (Thanks @Stoiko):
>>     * disallow any vertical space instead of line feeds only
>>     * additionally only allow printable characters
>>     
>>     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.
>> 
>>  src/PVE/Firewall.pm | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>> 
>> 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;
>>  }
>>  
>> +PVE::JSONSchema::register_format('pve-fw-comment-spec', \&pve_fw_verify_comment_spec);
>> +
>> +sub pve_fw_verify_comment_spec {
>> +    my ($comment) = @_;
>> +
>> +    if ($comment =~ 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.
> 
> 
>> +    }
>> +
>> +    if ($comment !~ m/^\p{Print}*$/) {
>> +        die "comment must only contain printable characters\n";
>> +    }
>> +
>> +    return $comment;
>> +}
>> +
>>  # helper function for API
>>  
>>  sub copy_opject_with_digest {
>> @@ -1623,6 +1639,7 @@ my $rule_properties = {
>>          description => "Descriptive comment.",
>>          type => 'string',
>>          optional => 1,
>> +        format => 'pve-fw-comment-spec',
>>      },
>>      'icmp-type' => {
>>          description =>
> 
> 
> 
> 
> 
>