public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Cc: w.bumiller@proxmox.com
Subject: Re: [pve-devel] [PATCH storage] plugin: volume snapshot info: untaint snapshot filename
Date: Mon, 28 Jul 2025 11:59:40 +0200	[thread overview]
Message-ID: <1753696364.bhxw524hb8.astroid@yuna.none> (raw)
In-Reply-To: <20250725154936.281471-1-f.weber@proxmox.com>

On July 25, 2025 5:48 pm, Friedrich Weber wrote:
> Without untainting, offline-deleting a volume-chain snapshot on a
> directory storage via the GUI fails with an "Insecure dependecy in
> exec [...]" error, because volume_snapshot_delete uses the filename
> its qemu-img invocation.
> 
> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
> ---
> 
> Notes:
>     I'm not too familiar with the taint mode. Allowing anything that
>     starts with a slash seems a little lax, but I don't know if we can do
>     any meaningful validation here -- let me know if we can.
> 
>  src/PVE/Storage/Plugin.pm | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index a817186..2bd05bd 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -1789,6 +1789,7 @@ sub volume_snapshot_info {
>          my $snapshots = $json_decode;
>          for my $snap (@$snapshots) {
>              my $snapfile = $snap->{filename};
> +            ($snapfile) = $snapfile =~ m|^(/.*)|; # untaint

we also validate that the path matches our naming scheme below, but that
is mostly concerned with the final component..

I called out that the references for backing images are not relative in
a previous iteration of the qcow2 patch series, it seems that slipped
through?

right now, it's not possible to change the backing directory path of the
storage, or the LVM VG without breaking all snapshot chains stored
there because all the back references to snapshots are using absolute
paths instead of relative ones..

if we fix that (and we probably should?), then the untainting RE here
would become wrong again..

>              my $snapname = $get_snapname_from_path->($volname, $snapfile);
>              #not a proxmox snapshot
>              next if !$snapname;


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


  reply	other threads:[~2025-07-28  9:58 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-07-25 15:48 Friedrich Weber
2025-07-28  9:59 ` Fabian Grünbichler [this message]
2025-07-28 11:08   ` Fiona Ebner
2025-07-28 12:21     ` Fabian Grünbichler
2025-07-28 13:30       ` Friedrich Weber
2025-07-28 13:32         ` Friedrich Weber
2025-07-31 12:37         ` Friedrich Weber
2025-07-31 12:55           ` Fiona Ebner
2025-07-28 13:20 ` [pve-devel] applied: " Fabian Grünbichler

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=1753696364.bhxw524hb8.astroid@yuna.none \
    --to=f.gruenbichler@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    --cc=w.bumiller@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
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal