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

[Friedrich Weber <f.weber@proxmox.com>]

On 28/07/2025 14:22, Fabian Grünbichler wrote:
> 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.

I got really confused because I couldn't reproduce the issue anymore.
Turns out I needed at least 3 snapshots to reproduce the issue. With
only two snapshots, the $snap->{filename} was not tainted, so didn't
need an untaint. With three snapshots, $snap->{filename} was tainted
because the result of qemu_img_info was already tainted. As it turns
out, our PVE::Tools::run_command may pass a tainted string to outfunc
(and thus taint the result of qemu_img_info) if current $buf (at most
4096 bytes) doesn't end in a whitespace.

Reproducer:

# cat test-tainted.pm
#!/usr/bin/perl -T
use strict;

use Taint::Runtime qw(is_tainted);
use PVE::Tools qw(run_command);

$ENV{"PATH"} = "/usr/bin";

sub check_tainted {
    my $cmd = shift;
    my $out;
    run_command($cmd, outfunc => sub { $out .= shift });
    print "output is tainted: ".(is_tainted($out) ? "yes" : "no")."\n";
};

check_tainted(["echo", "x"x4095]); # 4095 chars + newline
check_tainted(["echo", "x"x4096]); # 4096 chars + newline
check_tainted(["echo", "hi\nthere"]); # trailing newline
check_tainted(["echo", "-n", "hi\nthere"]); # no trailing newline

# ./test-tainted.pm
output is tainted: no
output is tainted: yes
output is tainted: no
output is tainted: yes

I *think* the reason is this hunk in run_command:

    while ($buf =~ s/^([^\010\r\n]*)(?:\n|(?:\010)+|\r\n?)//) {
	my $line = $outlog . $1;
	$outlog = '';
	&$outfunc($line) if $outfunc;
	&$logfunc($line) if $logfunc;
    }
    $outlog .= $buf;

... where $buf is tainted. The s// makes sure $line is untainted (if
$outlog is untainted), buf if $buf is non-empty after the while loop
(because it didn't end with a newline), it taints $outlog, which will be
passed to outfunc later.

With two snapshots, the output of `qemu-img info` on my test machine is
smaller than 4096 bytes and ends in a newline, so it's not tainted. With
three snapshots, it is >4096 bytes and the boundary is not on a newline,
so it's tainted.

Would it be a good idea to fix `run_command` so it always passes an
untainted string to outfunc (and I guess the same for errfunc)?
We could alternatively (or in addition) still add this untaint here (see
below).

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

Thanks! I guess then (and if we want to add an untaint here at all) we
could keep this patch as it is, because qemu-img info does seem to
output an absolute "filename" even if the backing filename is absolute?
What do you think?


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