Re: [pve-devel] [PATCH qemu-server 1/1] disk import: add additional safeguards for imported image files

[Fiona Ebner <f.ebner@proxmox.com>]

Am 09.08.24 um 13:22 schrieb Fabian Grünbichler:
> creating non-raw disk images with arbitrary content is only possible with raw
> access to the storage, but checking for references to external files doesn't
> hurt.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> 
> Notes:
>     requires pve-storage change to actually have an effect
> 
>  PVE/API2/Qemu.pm | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index d25a79fe..30839745 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -412,12 +412,14 @@ my sub create_disks : prototype($$$$$$$$$$) {
>  
>  		$needs_creation = $live_import;
>  
> -		if (PVE::Storage::parse_volume_id($source, 1)) { # PVE-managed volume
> +		if (my ($source_storage, $source_volid) = PVE::Storage::parse_volume_id($source, 1)) { # PVE-managed volume

Nit: line too long and honestly, becomes a bit hard to read.

>  		    if ($live_import && $ds ne 'efidisk0') {
>  			my $path = PVE::Storage::path($storecfg, $source)
>  			    or die "failed to get a path for '$source'\n";
>  			$source = $path;
> -			($size, my $source_format) = PVE::Storage::file_size_info($source);
> +			# check potentially untrusted image file!
> +			($size, my $source_format) = PVE::Storage::file_size_info($source, undef, 1);
> +
>  			die "could not get file size of $source\n" if !$size;
>  			$live_import_mapping->{$ds} = {
>  			    path => $source,
> @@ -431,6 +433,12 @@ my sub create_disks : prototype($$$$$$$$$$) {
>  			    format => $disk->{format},
>  			};
>  
> +			my $scfg = PVE::Storage::storage_config($storecfg, $source_storage);
> +			# check potentially untrusted image file!
> +			if ($scfg->{path}) {
> +			    PVE::Storage::file_size_info(PVE::Storage::path($storecfg, $source), undef, 1);

This call is probably broken, see reply to guest-common patch.

Nit: I'd move this to the beginning of the branch to avoid having it
between $dest_info assignments.

> +			}
> +
>  			$dest_info->{efisize} = PVE::QemuServer::get_efivars_size($conf, $disk)
>  			    if $ds eq 'efidisk0';
>  
> @@ -441,7 +449,8 @@ my sub create_disks : prototype($$$$$$$$$$) {
>  		    }
>  		} else {
>  		    $source = PVE::Storage::abs_filesystem_path($storecfg, $source, 1);
> -		    ($size, my $source_format) = PVE::Storage::file_size_info($source);
> +		    # check potentially untrusted image file!
> +		    ($size, my $source_format) = PVE::Storage::file_size_info($source, undef, 1);
>  		    die "could not get file size of $source\n" if !$size;
>  
>  		    if ($live_import && $ds ne 'efidisk0') {


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