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.we...@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
