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 DADD61FF0C1 for ; Wed, 26 Aug 2026 14:37:10 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id EFA4121442; Wed, 26 Aug 2026 14:37:09 +0200 (CEST) Message-ID: Date: Wed, 26 Aug 2026 14:37:05 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH access-control] rpc: do not check malformed acl path To: Elias Huhsovitz , d.csapak@proxmox.com References: <8eff67e8-ac88-4b7d-a90d-de32644d1d95@proxmox.com> <20260826103752.115037-1-e.huhsovitz@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20260826103752.115037-1-e.huhsovitz@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1787747817343 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.358 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) KAM_MAILER 2 Automated Mailer Tag Left in Email POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium 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: 4WHBSAGF2HTLTS24J7JQ5QX6PUZ5PFNR X-Message-ID-Hash: 4WHBSAGF2HTLTS24J7JQ5QX6PUZ5PFNR X-MailFrom: f.ebner@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: 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 > --- > 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 > 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 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') {