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

[Fiona Ebner]

Am 28.07.25 um 11:59 AM schrieb 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..

Agreed, making the backing paths relative (for new volumes) sounds
sensible and then we can also validate them better :)