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

[Fiona Ebner <f.ebner@proxmox.com>]

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.

Am 26.08.26 um 12:37 PM schrieb Elias Huhsovitz:
> Signed-off-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
> ---
> Thank you for the feedback. I looked into this a bit more and
> provided an experimental patch as a reference.
> 
> The Problem
> -----------
> The core problem is indeed the colon ':', which makes sense.
> 
> I did some further testing and the same error occurs when sending a
> request to the enpoint:
> 
> GET /cluster/mapping/pci/{id}
> 
> It outputs the errors:
> pveproxy[42711]: Failed to normalize '/mapping/pci/0000:01:00.0'
> pveproxy[42711]: internal error: ACL check called for undefined ACL path!
> pveproxy[42711]: Use of uninitialized value $path in concatenation (.) or string at /usr/share/perl5/PVE/RPCEnvironment.pm line 325.
> 
> Mismatch
> ---------
> I think explicitely checking the permissions in the api handler itself
> will work, but to my understanding there is an underlying mismatch:
> 
> The API schema explicitly allows colons (for raw PCI IDs), but the 
> ACL path normalizer explicitly rejects them.
> 
> 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 ;)

> 
> Caveats
> -------
> (I fear that such a check might causes problems in existing systems, 
> that rely on "invalid" ACL paths. But i would love to hear your input
> on this)
> 
> Side note
> ---------
> What aboud allowing encoded PCI IDs instead? e.g.,
> GET /nodes/lisa/hardware/pci/0000%3A01%3A00.0
> 
> Summary of changes
> ------------------
> 
>  src/PVE/RPCEnvironment.pm | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> 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') {