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

[Fiona Ebner]

Am 31.07.25 um 2:37 PM schrieb Friedrich Weber:
> On 28/07/2025 15:30, Friedrich Weber wrote:
>> 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).

I think we should rather go the other way and run_command() should be
made to never untaint the output itself (but not a good idea this close
before the release, as there is too much potential fallout)...

> 
> FWIW, Stoiko pointed out to me that (not very surprisingly) this issue
> had manifested already a few years ago [1].
> 
> [1]
> https://lore.proxmox.com/all/20210622142824.18773-1-s.ivanov@proxmox.com/

...see the last reply from Thomas there:

> Rather than just band-aiding it somewhere in the middle with a catch all regex that
> *completely* defeats the purpose of the concept of tainting, it can be better to
> either just disable or fix the few places where it's actual wrong with a local
> decision about how closely we can restrict the untainting, sometimes a match-all is
> all it can realistically be there, but not always.