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.iva...@proxmox.com/
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
