From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 3936C921AC for ; Tue, 28 Mar 2023 15:28:18 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 207F01CFDC for ; Tue, 28 Mar 2023 15:27:48 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 28 Mar 2023 15:27:47 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id BAC59470B3 for ; Tue, 28 Mar 2023 15:27:46 +0200 (CEST) Date: Tue, 28 Mar 2023 15:27:39 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20230303145051.109925-1-n.ullreich@proxmox.com> <20230303145051.109925-3-n.ullreich@proxmox.com> In-Reply-To: <20230303145051.109925-3-n.ullreich@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1680009417.rngh8s5ehc.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.072 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH pve-storage 2/2] change regex to allow subdirs X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Mar 2023 13:28:18 -0000 On March 3, 2023 3:50 pm, Noel Ullreich wrote: > change the regex in `parse_volname` and `get_subdir_files` to allow > subdirectories. >=20 > Signed-off-by: Noel Ullreich > --- > PVE/Storage/Plugin.pm | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) >=20 > diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm > index bf1d564..e8b2b95 100644 > --- a/PVE/Storage/Plugin.pm > +++ b/PVE/Storage/Plugin.pm > @@ -620,9 +620,9 @@ sub parse_volname { > my ($vmid, $name) =3D ($1, $2); > my (undef, $format, $isBase) =3D parse_name_dir($name); > return ('images', $name, $vmid, undef, undef, $isBase, $format); > - } elsif ($volname =3D~ m!^iso/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!) = { > + } elsif ($volname =3D~ m!^iso/(.+$PVE::Storage::ISO_EXT_RE_0)$!) { so this needs to forbid at least '..', else the following is possible 1. place symlink to sensitive, root-only host file somewhere writable by a = user (e.g., container volume on ZFS which has a host-visible mount point by defa= ult, but also something like an arbitrary container volume and tricking an admin= into running `pct mount` would work, or possible other similar scenarios) 2. qm set XXX -ide2 $storage:iso/../../../path/to/symlink,medium=3Dcdrom 3. (in VM) cat /dev/sr0 I am not sure whether that's the only thing that's bad, it might make sense= to start with a restricted set of allowed characters for the dirs in-between a= nd extend that as needed after evaluating.. if that route is taken, it obvious= ly also should be done for patch #1 to be consistent ;) it might also make sense to limit the nesting here and in patch #1, and/or = limit the length of the resulting volume ID? one problem we have here is that we only operate on a volume ID (or in sub = path, on a "hypothetical" path) so it's a bit hard to guard against certain types= of attacks that would require actually evaluating and analyzing the resulting = path, since we might be called in a context where the volume not yet existing is totally fine :-/ all in all, this series is the kind of change that warrants close attention= , and IMHO, when in doubt, might benefit from a few restrictions that don't reall= y harm usability but make locking it down more easy. > return ('iso', $1); > - } elsif ($volname =3D~ m!^vztmpl/([^/]+$PVE::Storage::VZTMPL_EXT_RE_= 1)$!) { > + } elsif ($volname =3D~ m!^vztmpl/(.+$PVE::Storage::VZTMPL_EXT_RE_1)$= !) { same here > return ('vztmpl', $1); > } elsif ($volname =3D~ m!^rootdir/(\d+)$!) { > return ('rootdir', $1, $1); > @@ -632,7 +632,7 @@ sub parse_volname { > return ('backup', $fn, $2); > } > return ('backup', $fn); > - } elsif ($volname =3D~ m!^snippets/([^/]+)$!) { > + } elsif ($volname =3D~ m!^snippets/(.+)$!) { and here! this would even allow directly including host files via cloud ini= t I think? > return ('snippets', $1); > } > =20 > @@ -1214,12 +1214,12 @@ my $get_subdir_files =3D sub { > my $info; > =20 > if ($tt eq 'iso') { > - next if $fn !~ m!/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!i; > + next if $fn !~ m!/.+\/template\/iso\/(.+$PVE::Storage::ISO_EXT_RE_0= )$!i; same applies here > =20 > $info =3D { volid =3D> "$sid:iso/$1", format =3D> 'iso' }; > =20 > } elsif ($tt eq 'vztmpl') { > - next if $fn !~ m!/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$!; > + next if $fn !~ m!/.+\/template\/cache\/(.+$PVE::Storage::VZTMPL_EXT= _RE_1)$!; and here > =20 > $info =3D { volid =3D> "$sid:vztmpl/$1", format =3D> "t$2" }; > =20 > @@ -1251,9 +1251,9 @@ my $get_subdir_files =3D sub { > =20 > $info->{protected} =3D 1 if -e PVE::Storage::protection_file_path($= original); > } elsif ($tt eq 'snippets') { > - > + next if $fn !~ m!/.+\/snippets/(.+)!; and also here > $info =3D { > - volid =3D> "$sid:snippets/". basename($fn), > + volid =3D> "$sid:snippets/$1", > format =3D> 'snippet', > }; > } > --=20 > 2.30.2 >=20 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20 >=20 >=20