[pve-devel] [PATCH storage 1/2] don't bail on whitespaces in backing devices

[pve-devel] [PATCH storage 1/2] don't bail on whitespaces in backing devices

[Wolfgang Bumiller]

This prevents importing from vmdks with whitespaces in file names.
Further, some operations that include file sizes (like listing disks)
would potentially fail entirely if a custom disk with a badly name
backing device exists in a VM images directory since they don't expect
this. Specifically, since we don't necessarily know the actual naming
scheme of the current storage in the plain Plugin.pm version, we don't
check the full name anyway, so why bother with whitespaces...

See-also: https://forum.proxmox.com/threads/new-import-wizard-available-for-migrating-vmware-esxi-based-virtual-machines.144023/page-16#post-658697
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
 src/PVE/Storage/Plugin.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 22a9729..683190b 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -982,7 +982,7 @@ sub file_size_info {
     $used = int($used);
     ($format) = ($format =~ /^(\S+)$/) or die "format '$format' includes whitespace\n"; # untaint
     if (defined($parent)) {
-	($parent) = ($parent =~ /^(\S+)$/) or die "parent '$parent' includes whitespace\n"; # untaint
+	($parent) = ($parent =~ /^(\S+)$/); # untaint
     }
     return wantarray ? ($size, $format, $used, $parent, $st->ctime) : $size;
 }
-- 
2.39.2



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

[pve-devel] [PATCH storage 2/2] fixup error messages in file_size_info

[Wolfgang Bumiller]

The assignment happens before the 'die', so the error message would
always contain 'undef'.

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
 src/PVE/Storage/Plugin.pm | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 683190b..b5a54c1 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -974,13 +974,16 @@ sub file_size_info {
 
     my ($size, $format, $used, $parent) = $info->@{qw(virtual-size format actual-size backing-filename)};
 
-    ($size) = ($size =~ /^(\d+)$/) or die "size '$size' not an integer\n"; # untaint
+    ($size) = ($size =~ /^(\d+)$/); # untaint
+    die "size '$size' not an integer\n" if !defined($size);
     # coerce back from string
     $size = int($size);
-    ($used) = ($used =~ /^(\d+)$/) or die "used '$used' not an integer\n"; # untaint
+    ($used) = ($used =~ /^(\d+)$/); # untaint
+    die "used '$used' not an integer\n" if !defined($used);
     # coerce back from string
     $used = int($used);
-    ($format) = ($format =~ /^(\S+)$/) or die "format '$format' includes whitespace\n"; # untaint
+    ($format) = ($format =~ /^(\S+)$/); # untaint
+    die "format '$format' includes whitespace\n" if !defined($format);
     if (defined($parent)) {
 	($parent) = ($parent =~ /^(\S+)$/); # untaint
     }
-- 
2.39.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 1/2] don't bail on whitespaces in backing devices

[Fiona Ebner]

Am 30.04.24 um 09:53 schrieb Wolfgang Bumiller:
> This prevents importing from vmdks with whitespaces in file names.
> Further, some operations that include file sizes (like listing disks)
> would potentially fail entirely if a custom disk with a badly name
> backing device exists in a VM images directory since they don't expect
> this. Specifically, since we don't necessarily know the actual naming
> scheme of the current storage in the plain Plugin.pm version, we don't
> check the full name anyway, so why bother with whitespaces...
> 
> See-also: https://forum.proxmox.com/threads/new-import-wizard-available-for-migrating-vmware-esxi-based-virtual-machines.144023/page-16#post-658697
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> ---
>  src/PVE/Storage/Plugin.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index 22a9729..683190b 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -982,7 +982,7 @@ sub file_size_info {
>      $used = int($used);
>      ($format) = ($format =~ /^(\S+)$/) or die "format '$format' includes whitespace\n"; # untaint
>      if (defined($parent)) {
> -	($parent) = ($parent =~ /^(\S+)$/) or die "parent '$parent' includes whitespace\n"; # untaint
> +	($parent) = ($parent =~ /^(\S+)$/); # untaint
>      }
>      return wantarray ? ($size, $format, $used, $parent, $st->ctime) : $size;
>  }

So the returned $parent will now just be undef if it contains
whitespaces, even though there is a parent. Can't that cause issues
further down the line? If it's fine, a comment with the rationale would
be nice.

Or should we rather allow whitespaces while matching and return it
properly? Or are there any issues with proper escaping then?


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

[pve-devel] applied-series: [PATCH storage 1/2] don't bail on whitespaces in backing devices

[Thomas Lamprecht]

On 30/04/2024 09:53, Wolfgang Bumiller wrote:
> This prevents importing from vmdks with whitespaces in file names.
> Further, some operations that include file sizes (like listing disks)
> would potentially fail entirely if a custom disk with a badly name
> backing device exists in a VM images directory since they don't expect
> this. Specifically, since we don't necessarily know the actual naming
> scheme of the current storage in the plain Plugin.pm version, we don't
> check the full name anyway, so why bother with whitespaces...
> 
> See-also: https://forum.proxmox.com/threads/new-import-wizard-available-for-migrating-vmware-esxi-based-virtual-machines.144023/page-16#post-658697

FYI, forum links can be de-SEO'd like:

https://forum.proxmox.com/threads/144023/page-16#post-658697

> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> ---
>  src/PVE/Storage/Plugin.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied both patches, thanks!


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

Re: [pve-devel] [PATCH storage 1/2] don't bail on whitespaces in backing devices

[Wolfgang Bumiller]

On Tue, Apr 30, 2024 at 10:14:13AM +0200, Fiona Ebner wrote:
> Am 30.04.24 um 09:53 schrieb Wolfgang Bumiller:
> > This prevents importing from vmdks with whitespaces in file names.
> > Further, some operations that include file sizes (like listing disks)
> > would potentially fail entirely if a custom disk with a badly name
> > backing device exists in a VM images directory since they don't expect
> > this. Specifically, since we don't necessarily know the actual naming
> > scheme of the current storage in the plain Plugin.pm version, we don't
> > check the full name anyway, so why bother with whitespaces...
> > 
> > See-also: https://forum.proxmox.com/threads/new-import-wizard-available-for-migrating-vmware-esxi-based-virtual-machines.144023/page-16#post-658697
> > Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> > ---
> >  src/PVE/Storage/Plugin.pm | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> > index 22a9729..683190b 100644
> > --- a/src/PVE/Storage/Plugin.pm
> > +++ b/src/PVE/Storage/Plugin.pm
> > @@ -982,7 +982,7 @@ sub file_size_info {
> >      $used = int($used);
> >      ($format) = ($format =~ /^(\S+)$/) or die "format '$format' includes whitespace\n"; # untaint
> >      if (defined($parent)) {
> > -	($parent) = ($parent =~ /^(\S+)$/) or die "parent '$parent' includes whitespace\n"; # untaint
> > +	($parent) = ($parent =~ /^(\S+)$/); # untaint
> >      }
> >      return wantarray ? ($size, $format, $used, $parent, $st->ctime) : $size;
> >  }
> 
> So the returned $parent will now just be undef if it contains
> whitespaces, even though there is a parent. Can't that cause issues
> further down the line? If it's fine, a comment with the rationale would
> be nice.
> 
> Or should we rather allow whitespaces while matching and return it
> properly? Or are there any issues with proper escaping then?

I was a bit too quick on the send trigger there, but it should be fine
IMO:

- where we do run into this issue, we never use/need/care about the parent
- the parent info of file_size_info is usually discarded, or checked
  against whether the disk is a "base volume" according to the storage's
  idea of how such a volume has to be named (as in, it's created/managed
  by pve)
  or, eg. in Plugin.pm's `list_images` the parent is then checked
  against a more specific regex and if it does not matched it is simply
  discarded as if it was `undef`... (so we already have some logic
  around backing-devices which "discards" unexpected values...)
- technically users could add a disk with a "bad" parent to a storage
  *manually*, but given the list_images mentioned above, I'd argue the
  situation isn't really getting worse, as values that *do* match `\S+`
  don't necessarily match the regexes used *later* on the parent
  *anyway*...

So we could also just untaint with /^(.+)$/, since IMO if we end up with
actual whitespace issues anywhere *else*, then *that* could is the
broken one, not this one...

🤷


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

Re: [pve-devel] [PATCH storage 1/2] don't bail on whitespaces in backing devices

[Fiona Ebner]

Am 30.04.24 um 10:38 schrieb Wolfgang Bumiller:
> On Tue, Apr 30, 2024 at 10:14:13AM +0200, Fiona Ebner wrote:
>>
>> So the returned $parent will now just be undef if it contains
>> whitespaces, even though there is a parent. Can't that cause issues
>> further down the line? If it's fine, a comment with the rationale would
>> be nice.
>>
>> Or should we rather allow whitespaces while matching and return it
>> properly? Or are there any issues with proper escaping then?
> 
> I was a bit too quick on the send trigger there, but it should be fine
> IMO:
> 
> - where we do run into this issue, we never use/need/care about the parent

Maybe this part of the function could be guarded by wantarray already,
so callers caring only about the size don't even get there? But I
suppose we do notice other unexpected things earlier by always doing the
additional checks, so maybe it's better like it is right now.

> - the parent info of file_size_info is usually discarded, or checked
>   against whether the disk is a "base volume" according to the storage's
>   idea of how such a volume has to be named (as in, it's created/managed
>   by pve)
>   or, eg. in Plugin.pm's `list_images` the parent is then checked
>   against a more specific regex and if it does not matched it is simply
>   discarded as if it was `undef`... (so we already have some logic
>   around backing-devices which "discards" unexpected values...)

Hmm, okay.

> - technically users could add a disk with a "bad" parent to a storage
>   *manually*, but given the list_images mentioned above, I'd argue the
>   situation isn't really getting worse, as values that *do* match `\S+`
>   don't necessarily match the regexes used *later* on the parent
>   *anyway*...
> 

CC Dominik

Thinking in the context of uploading OVAs (or uploading disk images), I
guess we need a check against arbitrary backing file paths in uploaded
qcow2/vmdk images (or do we already have that)?

> So we could also just untaint with /^(.+)$/, since IMO if we end up with
> actual whitespace issues anywhere *else*, then *that* could is the
> broken one, not this one...
> 
> 🤷

Fine by me, but after what you wrote, so is the current approach :)


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

Re: [pve-devel] [PATCH storage 1/2] don't bail on whitespaces in backing devices

[Wolfgang Bumiller]

On Tue, Apr 30, 2024 at 11:13:02AM +0200, Fiona Ebner wrote:
> Am 30.04.24 um 10:38 schrieb Wolfgang Bumiller:
> > On Tue, Apr 30, 2024 at 10:14:13AM +0200, Fiona Ebner wrote:
> >>
> >> So the returned $parent will now just be undef if it contains
> >> whitespaces, even though there is a parent. Can't that cause issues
> >> further down the line? If it's fine, a comment with the rationale would
> >> be nice.
> >>
> >> Or should we rather allow whitespaces while matching and return it
> >> properly? Or are there any issues with proper escaping then?
> > 
> > I was a bit too quick on the send trigger there, but it should be fine
> > IMO:
> > 
> > - where we do run into this issue, we never use/need/care about the parent
> 
> Maybe this part of the function could be guarded by wantarray already,
> so callers caring only about the size don't even get there? But I
> suppose we do notice other unexpected things earlier by always doing the
> additional checks, so maybe it's better like it is right now.

Unfortunately a lot of callers do also care about the format,
specifically the import code, where this causes issues, so a `wantarray`
check won't help there.


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

Re: [pve-devel] [PATCH storage 1/2] don't bail on whitespaces in backing devices

[Dominik Csapak]

On 4/30/24 11:13, Fiona Ebner wrote:
> Am 30.04.24 um 10:38 schrieb Wolfgang Bumiller:
>> - technically users could add a disk with a "bad" parent to a storage
>>    *manually*, but given the list_images mentioned above, I'd argue the
>>    situation isn't really getting worse, as values that *do* match `\S+`
>>    don't necessarily match the regexes used *later* on the parent
>>    *anyway*...
>>
> 
> CC Dominik
> 
> Thinking in the context of uploading OVAs (or uploading disk images), I
> guess we need a check against arbitrary backing file paths in uploaded
> qcow2/vmdk images (or do we already have that)?
> 

good point, we currently don't, but it shouldn't be hard to add that check
before importing/after uploading... i'll look into that


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