From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH pve-storage 2/2] change regex to allow subdirs
Date: Tue, 28 Mar 2023 15:27:39 +0200 [thread overview]
Message-ID: <1680009417.rngh8s5ehc.astroid@yuna.none> (raw)
In-Reply-To: <20230303145051.109925-3-n.ullreich@proxmox.com>
On March 3, 2023 3:50 pm, Noel Ullreich wrote:
> change the regex in `parse_volname` and `get_subdir_files` to allow
> subdirectories.
>
> Signed-off-by: Noel Ullreich <n.ullreich@proxmox.com>
> ---
> PVE/Storage/Plugin.pm | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> 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) = ($1, $2);
> my (undef, $format, $isBase) = parse_name_dir($name);
> return ('images', $name, $vmid, undef, undef, $isBase, $format);
> - } elsif ($volname =~ m!^iso/([^/]+$PVE::Storage::ISO_EXT_RE_0)$!) {
> + } elsif ($volname =~ 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 default,
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=cdrom
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 and
extend that as needed after evaluating.. if that route is taken, it obviously
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 really
harm usability but make locking it down more easy.
> return ('iso', $1);
> - } elsif ($volname =~ m!^vztmpl/([^/]+$PVE::Storage::VZTMPL_EXT_RE_1)$!) {
> + } elsif ($volname =~ m!^vztmpl/(.+$PVE::Storage::VZTMPL_EXT_RE_1)$!) {
same here
> return ('vztmpl', $1);
> } elsif ($volname =~ m!^rootdir/(\d+)$!) {
> return ('rootdir', $1, $1);
> @@ -632,7 +632,7 @@ sub parse_volname {
> return ('backup', $fn, $2);
> }
> return ('backup', $fn);
> - } elsif ($volname =~ m!^snippets/([^/]+)$!) {
> + } elsif ($volname =~ m!^snippets/(.+)$!) {
and here! this would even allow directly including host files via cloud init I
think?
> return ('snippets', $1);
> }
>
> @@ -1214,12 +1214,12 @@ my $get_subdir_files = sub {
> my $info;
>
> 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
>
> $info = { volid => "$sid:iso/$1", format => 'iso' };
>
> } 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
>
> $info = { volid => "$sid:vztmpl/$1", format => "t$2" };
>
> @@ -1251,9 +1251,9 @@ my $get_subdir_files = sub {
>
> $info->{protected} = 1 if -e PVE::Storage::protection_file_path($original);
> } elsif ($tt eq 'snippets') {
> -
> + next if $fn !~ m!/.+\/snippets/(.+)!;
and also here
> $info = {
> - volid => "$sid:snippets/". basename($fn),
> + volid => "$sid:snippets/$1",
> format => 'snippet',
> };
> }
> --
> 2.30.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
next prev parent reply other threads:[~2023-03-28 13:28 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-03 14:50 [pve-devel] [PATCH pve-storage 0/2] fix #623: show isos/tmplt/snippets in subdirs Noel Ullreich
2023-03-03 14:50 ` [pve-devel] [PATCH pve-storage 1/2] update `list_volumes` to allow subdirs Noel Ullreich
2023-03-28 13:27 ` Fabian Grünbichler
2023-03-03 14:50 ` [pve-devel] [PATCH pve-storage 2/2] change regex " Noel Ullreich
2023-03-28 13:27 ` Fabian Grünbichler [this message]
2023-03-29 7:21 ` [pve-devel] [PATCH pve-storage 0/2] fix #623: show isos/tmplt/snippets in subdirs Fiona Ebner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1680009417.rngh8s5ehc.astroid@yuna.none \
--to=f.gruenbichler@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox