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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On July 28, 2025 1:08 pm, Fiona Ebner wrote:
> 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@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 :)

so turns out this is already correctly handled when initially creating
the volumes, but subsequent `qemu-img rebase` or `block-commit/-stream`
invocations will inject the absolute paths.

should hopefully not be too hard to fix, I'll try to whip up patches..


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