[pve-devel] [PATCH storage] plugins: untaint volume_size_info retuns

[pve-devel] [PATCH storage] plugins: untaint volume_size_info retuns

[Stoiko Ivanov]

the size returned by volume_size_info is used for creating the new
destination image in PVE::QemuServer::clone_disk (and probably
elsewhere). In certain cases the return values are tainted - they are
obtained by a run_command call and depending on the format and length
of the parsed output can still have their tainted attribute.

One example of a tainted return has been reported in our
community-forum:
https://forum.proxmox.com/threads/cannot-clone-vm-or-move-disk-with-more-than-13-snapshots.89628/

A qcow2 image with 13 snapshots generates a output > 4k in length from
`qemu-img info --output=json`, which in turn causes the output to be
considered tainted.

This patch untaints the returns where applicable. The other
storage-plugins are not affected:
* LVMPlugin returns a single number and a newline (thus gets untainted
  by run_command)
* RBDPlugin untaints the complete json before decoding
* ZFSPoolplugin and ISCSIDirectPlugin explicitly untaint their
  returns.

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---

Note:
Not really a v2, since it's a different patch, but addresses the same issue
as in https://lists.proxmox.com/pipermail/pve-devel/2021-June/048910.html

 PVE/Storage/PBSPlugin.pm | 4 +++-
 PVE/Storage/Plugin.pm    | 6 ++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/PVE/Storage/PBSPlugin.pm b/PVE/Storage/PBSPlugin.pm
index a439dd2..2576764 100644
--- a/PVE/Storage/PBSPlugin.pm
+++ b/PVE/Storage/PBSPlugin.pm
@@ -811,7 +811,9 @@ sub volume_size_info {
 
     my $size = 0;
     foreach my $info (@$data) {
-	$size += $info->{size} if $info->{size};
+	if ($info->{size} && $info->{size} =~ /^(\d+)$/) {
+	    $size += $1;
+	}
     }
 
     my $used = $size;
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index d330845..2bcbc84 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -837,6 +837,12 @@ sub file_size_info {
 
     my ($size, $format, $used, $parent) = $info->@{qw(virtual-size format actual-size backing-filename)};
 
+    ($size) = ($size =~ /^(\d+)$/); #untaint
+    ($used) = ($used =~ /^(\d+)$/); #untaint
+    ($format) = ($format =~ /^([-\w]+)$/); #untaint
+    if (defined($parent)) {
+	($parent) = ($parent =~ /^(.*)$/); #untaint
+    }
     return wantarray ? ($size, $format, $used, $parent, $st->ctime) : $size;
 }
 
-- 
2.20.1

[pve-devel] applied: [PATCH storage] plugins: untaint volume_size_info retuns

[Thomas Lamprecht]

On 22.06.21 18:39, Stoiko Ivanov wrote:
> the size returned by volume_size_info is used for creating the new
> destination image in PVE::QemuServer::clone_disk (and probably
> elsewhere). In certain cases the return values are tainted - they are
> obtained by a run_command call and depending on the format and length
> of the parsed output can still have their tainted attribute.
> 
> One example of a tainted return has been reported in our
> community-forum:
> https://forum.proxmox.com/threads/cannot-clone-vm-or-move-disk-with-more-than-13-snapshots.89628/
> 
> A qcow2 image with 13 snapshots generates a output > 4k in length from
> `qemu-img info --output=json`, which in turn causes the output to be
> considered tainted.
> 
> This patch untaints the returns where applicable. The other
> storage-plugins are not affected:
> * LVMPlugin returns a single number and a newline (thus gets untainted
>   by run_command)
> * RBDPlugin untaints the complete json before decoding
> * ZFSPoolplugin and ISCSIDirectPlugin explicitly untaint their
>   returns.
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
> 
> Note:
> Not really a v2, since it's a different patch, but addresses the same issue
> as in https://lists.proxmox.com/pipermail/pve-devel/2021-June/048910.html

Aren't the version rather tied to the issue they try to solve, as implementations
and approaches can always significantly change? So I'd be OK with this being
marked as v2, but no hard feelings.

> 
>  PVE/Storage/PBSPlugin.pm | 4 +++-
>  PVE/Storage/Plugin.pm    | 6 ++++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
>

applied to master and a newly branched stable-6, thanks!

I made two followups:

1) - fix nit that we have a space in comments after the #
   - actually die with error message if untaint fails

2) return early if decode JSON fails, compensates issues where the qemu-img
   command somehow fails (e.g., file was removed since the stat check) and
   the semantic change from 1)

Please check if this still looks alright to you.

Re: [pve-devel] applied: [PATCH storage] plugins: untaint volume_size_info retuns

[Stoiko Ivanov]

On Wed, 23 Jun 2021 09:13:40 +0200
Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:

> On 22.06.21 18:39, Stoiko Ivanov wrote:
> > the size returned by volume_size_info is used for creating the new
> > destination image in PVE::QemuServer::clone_disk (and probably
> > elsewhere). In certain cases the return values are tainted - they are
> > obtained by a run_command call and depending on the format and length
> > of the parsed output can still have their tainted attribute.
> > 
> > One example of a tainted return has been reported in our
> > community-forum:
> > https://forum.proxmox.com/threads/cannot-clone-vm-or-move-disk-with-more-than-13-snapshots.89628/
> > 
> > A qcow2 image with 13 snapshots generates a output > 4k in length from
> > `qemu-img info --output=json`, which in turn causes the output to be
> > considered tainted.
> > 
> > This patch untaints the returns where applicable. The other
> > storage-plugins are not affected:
> > * LVMPlugin returns a single number and a newline (thus gets untainted
> >   by run_command)
> > * RBDPlugin untaints the complete json before decoding
> > * ZFSPoolplugin and ISCSIDirectPlugin explicitly untaint their
> >   returns.
> > 
> > Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> > ---
> > 
> > Note:
> > Not really a v2, since it's a different patch, but addresses the same issue
> > as in https://lists.proxmox.com/pipermail/pve-devel/2021-June/048910.html  
> 
> Aren't the version rather tied to the issue they try to solve, as implementations
> and approaches can always significantly change? So I'd be OK with this being
> marked as v2, but no hard feelings.
> 
> > 
> >  PVE/Storage/PBSPlugin.pm | 4 +++-
> >  PVE/Storage/Plugin.pm    | 6 ++++++
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> > 
> >  
> 
> applied to master and a newly branched stable-6, thanks!
> 
> I made two followups:
> 
> 1) - fix nit that we have a space in comments after the #
>    - actually die with error message if untaint fails
> 
> 2) return early if decode JSON fails, compensates issues where the qemu-img
>    command somehow fails (e.g., file was removed since the stat check) and
>    the semantic change from 1)
> 
> Please check if this still looks alright to you.

Thanks for the improvements - Look fine to me! (and survived a few quick
tests of disk-moving)