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

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

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/