[pve-devel] [PATCH storage] plugin: volume snapshot info: untaint snapshot filename

[Fabian Grünbichler]

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 at 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;