Re: [PATCH access-control] rpc: do not check malformed acl path

["Elias Huhsovitz" <e.huhsovitz@proxmox.com>]

Thanks for the quick response!

This was intended as a repsonse to this review:
https://lore.proxmox.com/pve-devel/8eff67e8-ac88-4b7d-a90d-de32644d1d95@proxmox.com/

I didn't mean for this to be a fully finished patch. This was meant
as a follow up.

I am unsure how to reply to a review while sharing code at
the same time. I relied on `--in-reply-to=` paramter too much.

Sorry for the trouble!

But your input resolves my confusion, so thanks a lot! :)


On Wed Aug 26, 2026 at 2:37 PM CEST, Fiona Ebner wrote:
> Missing rationale for why this is needed and why this is okay in the
> commit message. Always good to have, but especially for changes to
> permission checks.

[...]

>> Proposed solution
>> -----------------
>> IMHO providing explicit permissions on a raw PCI ID like 0000:01:00.0
>> instead of a mapping should not happen (i think) and should not be
>> checked. 
>> 
>> What is your opinion on simply not checking invalid ACL 
>> paths?
>> 
>> This would cause a more graceful failure. 
>
> Currently, the permission check for an invalid ACL path will still work
> for root@pam. With your patch, it won't work anymore. See the beginning
> of the permissions() sub.
>
> Note also the history here:
>
>> commit 37d3c16b25644f953647b512d9d231c866fa94e0
>> Author: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> Date:   Mon Jun 20 13:05:12 2022 +0200
>> 
>>     perm check: forbid undefined/empty ACL path
>>     
>>     to detect similar issues to that fixed in the previous commit early on
>>     and without the potential for dangerous side-effects.
>>     
>>     root@pam is intentionally still allowed before the check in case such
>>     situations can be triggered by misconfiguration - root@pam can then
>>     still clean up the affected configs via the GUI/API, and not just via
>>     manual editing.
>>     
>>     Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>
> So this is done intentionally.
>
> What you can do is add a code comment, so people don't have to wonder
> about it again in the future, as well as adapt the exception message in
> case the path is undefined to avoid the ugly Perl warning.
>
> I still finished my review below for completeness ;)
>

[...]

>> 
>> diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
>> index 7591aa9..61ef28b 100644
>> --- a/src/PVE/RPCEnvironment.pm
>> +++ b/src/PVE/RPCEnvironment.pm
>> @@ -487,7 +487,11 @@ sub exec_api2_perm_check {
>>          }
>>          my $path = PVE::Tools::template_replace($tmplpath, $param);
>>          my $normpath = PVE::AccessControl::normalize_path($path);
>> -        warn "Failed to normalize '$path'\n" if !defined($normpath) && defined($path);
>> +
>> +        if (!defined($normpath)) {
>> +            warn "Failed to normalize ACL path '$path'\n" if defined($path);
>> +            return 0;
>
> This should rather be:
> return 0 if $noerr;
> raise_perm_exc();
>
> And I think we may assume that $path is defined and drop that post-if.
> Because we have
>
> die "missing parameters" if !($tmplpath && $privs);
>
> so $tmplpath is set and then we call
> my $path = PVE::Tools::template_replace($tmplpath, $param);
> which should always produce a defined result if $tmplpath is set.
>
> Even if that changes in the future for some reason, I'd rather get a
> warning with an undefined value than no warning here, so the issue will
> get noticed.
>
>> +        }
>>  
>>          return $self->check_full($username, $normpath, $privs, $any, $noerr);
>>      } elsif ($test eq 'userid-group') {