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 BABC61FF0C1 for ; Wed, 26 Aug 2026 14:53:44 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id AA4302142C; Wed, 26 Aug 2026 14:53:41 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 26 Aug 2026 14:53:35 +0200 Message-Id: Subject: Re: [PATCH access-control] rpc: do not check malformed acl path From: "Elias Huhsovitz" To: "Fiona Ebner" , X-Mailer: aerc 0.20.0 References: <8eff67e8-ac88-4b7d-a90d-de32644d1d95@proxmox.com> <20260826103752.115037-1-e.huhsovitz@proxmox.com> In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1787748807775 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.729 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_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: 66EQPP7M7FYN6SMNGEQ4H5UTPFVKP2JF X-Message-ID-Hash: 66EQPP7M7FYN6SMNGEQ4H5UTPFVKP2JF X-MailFrom: e.huhsovitz@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: 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@pro= xmox.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=3D` 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.=20 >>=20 >> What is your opinion on simply not checking invalid ACL=20 >> paths? >>=20 >> This would cause a more graceful failure.=20 > > 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=C3=BCnbichler >> Date: Mon Jun 20 13:05:12 2022 +0200 >>=20 >> perm check: forbid undefined/empty ACL path >> =20 >> to detect similar issues to that fixed in the previous commit early = on >> and without the potential for dangerous side-effects. >> =20 >> root@pam is intentionally still allowed before the check in case suc= h >> situations can be triggered by misconfiguration - root@pam can then >> still clean up the affected configs via the GUI/API, and not just vi= a >> manual editing. >> =20 >> Signed-off-by: Fabian Gr=C3=BCnbichler > > 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 ;) > [...] >>=20 >> 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 =3D PVE::Tools::template_replace($tmplpath, $param); >> my $normpath =3D 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($p= ath); >> + 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 =3D 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. > >> + } >> =20 >> return $self->check_full($username, $normpath, $privs, $any, $n= oerr); >> } elsif ($test eq 'userid-group') {