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 7FCA061639 for ; Wed, 26 Jul 2023 11:35:36 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 6236732F6 for ; Wed, 26 Jul 2023 11:35:06 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 26 Jul 2023 11:35:04 +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 8316845396 for ; Wed, 26 Jul 2023 11:35:04 +0200 (CEST) Date: Wed, 26 Jul 2023 11:34:55 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20230721122314.80427-1-n.ullreich@proxmox.com> <20230721122314.80427-2-n.ullreich@proxmox.com> In-Reply-To: <20230721122314.80427-2-n.ullreich@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1690360729.dvu5qvbgb9.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.070 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 T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com, storage.pm, plugin.pm] Subject: Re: [pve-devel] [PATCH pve-storage v4 1/3] recursively go through subdirs to find files 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: Wed, 26 Jul 2023 09:35:36 -0000 On July 21, 2023 2:23 pm, Noel Ullreich wrote: > This patch allows `get_subdir_files` to recursively call itself, so that > subdirectories of set depth can be searched. We allow searching for > isos, vztmpl and snippets but not backups. >=20 > As a security measure, when parsing a given path, parent > directories (`/../`) are forbidden. >=20 > The feature is opt-in, i.e. the searchdepth is 0 by default. It can be > changed via the API, the web interface and `pvesm` (see the other > patches). >=20 > Signed-off-by: Noel Ullreich > --- > src/PVE/Storage.pm | 11 ++++++++ > src/PVE/Storage/Plugin.pm | 54 ++++++++++++++++++++++++--------------- > 2 files changed, 44 insertions(+), 21 deletions(-) >=20 > diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm > index b99ed35..02abacf 100755 > --- a/src/PVE/Storage.pm > +++ b/src/PVE/Storage.pm > @@ -113,6 +113,17 @@ our $VZTMPL_EXT_RE_1 =3D qr/\.tar\.(gz|xz|zst)/i; > =20 > our $BACKUP_EXT_RE_2 =3D qr/\.(tgz|(?:tar|vma)(?:\.(${\PVE::Storage::Plu= gin::COMPRESSOR_RE}))?)/; > =20 > +our $INTERMEDIATE_SUBDIR_EXT_RE_3 =3D qr/(?:[0-9A-z\_\-\.]+\/)*[^\/]+/i; > + > +our $SUBDIR_ANY_FILEEXTENSION_EXT_RE_4 =3D qr/(?:[0-9A-z\_\-\.]+\/)*.+/i= ; > + 1. common RE pitfall: use a-z instead of A-z, or a-zA-Z and drop the `/i` - else this matches the characters [\]^` as well, since they sit between 'Z' and 'a'. not all of them will work in practice, especially the '\' one would require special handling - but we don't want them anyhow ;) 2. these two should end with _0 , right? (they don't have any capturing groups) 3. the first regular expression matches any string that doesn't contain a '/', or any string containing slashes, where everything up to the last slash must be from the restricted char set - the name is a bit weird IMHO ;) maybe it would make more sense to keep the [^\/]+ part out of it, and leave that at the call site? then it would actually just match the intermediate sub-dirs, like the name implies (although I would still drop the `_EXT`, it refers to a file extension, and that is the only thing it *does not* match at the moment ;)).. 4. the second RE basically matches any string of length >=3D1 ? if we do the above change, a single RE should be enough (and the one usage of $SUBDIR_ANY_FILEEXTENSION_EXT_RE_4 could also just be $INTERMEDIATE_SUBDIR_RE_0[^\/]+ , like the other ones, but without any specific extension/suffix..). it would also mean that parse_volname and get_subdir_files accept the same things, instead of the latter allowing more characters in subdir components (although in practice, its return value will likely be filtered through parse_volume_id -> parse_volname anyhow - not sure of *all* the code paths though..) other than these, the patches look good to me and seem to work as expected, the above is mostly (except for number 1) code hygiene/style/naming :) symlinks are also handled transparently as expected (e.g., no resolved path leaks into path/volume id/.., and the size is correctly retrieved from the target, dangling symlinks are just ignored). > +# '..' is forbidden at the beginning, between two '/' and at the end > +my $dots =3D quotemeta('..'); > +my $beginning =3D qr!^$dots/!; > +my $between =3D qr!/$dots/!; > +my $end =3D qr!/$dots$!; > +our $forbidden_double_dots_re =3D qr!(?:$beginning|$between|$end)!; > + > # FIXME remove with PVE 8.0, add versioned breaks for pve-manager > our $vztmpl_extension_re =3D $VZTMPL_EXT_RE_1; > =20 > diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm > index 9d3b1ae..8831fbb 100644 > --- a/src/PVE/Storage/Plugin.pm > +++ b/src/PVE/Storage/Plugin.pm > @@ -621,6 +621,8 @@ sub parse_name_dir { > sub parse_volname { > my ($class, $volname) =3D @_; > =20 > + die "volname must not contain parent directories '/../'\n" if $volna= me =3D~ $PVE::Storage::forbidden_double_dots_re; > + > if ($volname =3D~ m!^(\d+)/(\S+)/(\d+)/(\S+)$!) { > my ($basedvmid, $basename) =3D ($1, $2); > parse_name_dir($basename); > @@ -631,9 +633,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::INTERMEDIATE_SUBDIR_EX= T_RE_3$PVE::Storage::ISO_EXT_RE_0)$!) { > return ('iso', $1); > - } elsif ($volname =3D~ m!^vztmpl/([^/]+$PVE::Storage::VZTMPL_EXT_RE_= 1)$!) { > + } elsif ($volname =3D~ m!^vztmpl/($PVE::Storage::INTERMEDIATE_SUBDIR= _EXT_RE_3$PVE::Storage::VZTMPL_EXT_RE_1)$!) { > return ('vztmpl', $1); > } elsif ($volname =3D~ m!^rootdir/(\d+)$!) { > return ('rootdir', $1, $1); > @@ -643,7 +645,7 @@ sub parse_volname { > return ('backup', $fn, $2); > } > return ('backup', $fn); > - } elsif ($volname =3D~ m!^snippets/([^/]+)$!) { > + } elsif ($volname =3D~ m!^snippets/($PVE::Storage::INTERMEDIATE_SUBD= IR_EXT_RE_3)$!) { > return ('snippets', $1); > } > =20 > @@ -1212,28 +1214,33 @@ sub list_images { > } > =20 > # list templates ($tt =3D ) > -my $get_subdir_files =3D sub { > - my ($sid, $path, $tt, $vmid) =3D @_; > +sub get_subdir_files { > + my ($sid, $path, $tt, $scfg, $vmid, $remaining_depth) =3D @_; > + my $storage_path =3D $scfg->{path}; > + my $content_dir =3D $scfg->{"content-dirs"}->{$tt} // $vtype_subdirs= ->{$tt}; > =20 > my $res =3D []; > =20 > foreach my $fn (<$path/*>) { > my $st =3D File::stat::stat($fn); > =20 > - next if (!$st || S_ISDIR($st->mode)); > + next if (!$st); > + > + if (S_ISDIR($st->mode)) { > + if ($remaining_depth) { > + push @$res, get_subdir_files($sid, $fn, $tt, $scfg, $vmid, $remaining_= depth-1); > + } > + next; > + } > =20 > my $info; > =20 > if ($tt eq 'iso') { > - next if $fn !~ m!/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!i; > - > + next if $fn !~ m/(?:^$storage_path\/$content_dir\/)($PVE::Storage::= INTERMEDIATE_SUBDIR_EXT_RE_3$PVE::Storage::ISO_EXT_RE_0)/; > $info =3D { volid =3D> "$sid:iso/$1", format =3D> 'iso' }; > - > } elsif ($tt eq 'vztmpl') { > - next if $fn !~ m!/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$!; > - > + next if $fn !~ m/(?:^$storage_path\/$content_dir\/)($PVE::Storage::= INTERMEDIATE_SUBDIR_EXT_RE_3$PVE::Storage::VZTMPL_EXT_RE_1)/; > $info =3D { volid =3D> "$sid:vztmpl/$1", format =3D> "t$2" }; > - > } elsif ($tt eq 'backup') { > next if $fn !~ m!/([^/]+$PVE::Storage::BACKUP_EXT_RE_2)$!; > my $original =3D $fn; > @@ -1262,9 +1269,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/(?:^$storage_path\/$content_dir\/)($PVE::Storage::= SUBDIR_ANY_FILEEXTENSION_EXT_RE_4)/; > $info =3D { > - volid =3D> "$sid:snippets/". basename($fn), > + volid =3D> "$sid:snippets/$1", #basename($fn), leftover comment? ;) > format =3D> 'snippet', > }; > } > @@ -1274,14 +1281,18 @@ my $get_subdir_files =3D sub { > =20 > push @$res, $info; > } > - > return $res; > }; > =20 > +sub flatten { > + map { ref eq 'ARRAY' ? flatten(@{$_}) : $_ } @_; > +} > + > # If attributes are set on a volume, they should be included in the resu= lt. > # See get_volume_attribute for a list of possible attributes. > sub list_volumes { > my ($class, $storeid, $scfg, $vmid, $content_types) =3D @_; > + my $max_depth =3D $scfg->{'scan-depth'} // 0; > =20 > my $res =3D []; > my $vmlist =3D PVE::Cluster::get_vmlist(); > @@ -1294,17 +1305,19 @@ sub list_volumes { > my $path =3D $class->get_subdir($scfg, $type); > =20 > if ($type eq 'iso' && !defined($vmid)) { > - $data =3D $get_subdir_files->($storeid, $path, 'iso'); > + $data =3D get_subdir_files($storeid, $path, 'iso', $scfg, undef, $max_= depth); > } elsif ($type eq 'vztmpl'&& !defined($vmid)) { > - $data =3D $get_subdir_files->($storeid, $path, 'vztmpl'); > + $data =3D get_subdir_files($storeid, $path , 'vztmpl', $scfg, undef, $= max_depth); > } elsif ($type eq 'backup') { > - $data =3D $get_subdir_files->($storeid, $path, 'backup', $vmid); > + $data =3D get_subdir_files($storeid, $path, 'backup', $scfg, $vmid, $m= ax_depth); > } elsif ($type eq 'snippets') { > - $data =3D $get_subdir_files->($storeid, $path, 'snippets'); > + $data =3D get_subdir_files($storeid, $path, 'snippets', $scfg, undef, = $max_depth); > } > } > =20 > - next if !$data; > + $data =3D [flatten($data)]; > + > + next if !@$data[0]; > =20 > foreach my $item (@$data) { > if ($type eq 'images' || $type eq 'rootdir') { > @@ -1322,7 +1335,6 @@ sub list_volumes { > } else { > $item->{content} =3D $type; > } > - > push @$res, $item; > } > } > --=20 > 2.39.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