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

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

[Friedrich Weber]

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.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
             my $snapname = $get_snapname_from_path->($volname, $snapfile);
             #not a proxmox snapshot
             next if !$snapname;
-- 
2.47.2



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

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

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

>              my $snapname = $get_snapname_from_path->($volname, $snapfile);
>              #not a proxmox snapshot
>              next if !$snapname;


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

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

[Fiona Ebner]

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


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

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

[Fabian Grünbichler]

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


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

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

[Friedrich Weber]

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

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

[Friedrich Weber]

On 28/07/2025 15:30, Friedrich Weber wrote:
[...]
>> 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?

Sorry, didn't see the apply. Thanks! A patch for run_command to always
pass untainted values could be done in addition, I guess.


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

Re: [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/


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

Re: [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.


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

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

[Fabian Grünbichler]

On Fri, 25 Jul 2025 17:48:57 +0200, 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.
> 
> 

Applied, thanks - this filename here is always absolute, even if we
fix up more of the actual referencing to happen via relative paths.

[1/1] plugin: volume snapshot info: untaint snapshot filename
      commit: 93f0dfbc756e02984d85d5f301314b987d8388f1

Best regards,
-- 
Fabian Grünbichler <f.gruenbichler@proxmox.com>


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