[pve-devel] [PATCH v2 storage 1/1] file_size_info: implement untrusted mode

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

this allows checking some extra attributes for images which come from a
potentially malicious source.

since file_size_info is not part of the plugin API, no API bump is needed. if
desired, a similar check could also be implemented in volume_size_info, which
would entail bumping both APIVER and APIAGE (since the additional parameter
would make checking untrusted volumes opt-in for external plugins).

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
---

Notes:
    v2: adapt to new early return, add Fiona's R-b

 src/PVE/Storage.pm        |  4 ++--
 src/PVE/Storage/Plugin.pm | 36 +++++++++++++++++++++++++++++++-----
 2 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 57b2038..3f0b9ae 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -233,9 +233,9 @@ sub storage_ids {
 }
 
 sub file_size_info {
-    my ($filename, $timeout) = @_;
+    my ($filename, $timeout, $untrusted) = @_;
 
-    return PVE::Storage::Plugin::file_size_info($filename, $timeout);
+    return PVE::Storage::Plugin::file_size_info($filename, $timeout, $untrusted);
 }
 
 sub get_volume_attribute {
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 8cc693c..215214f 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -943,15 +943,25 @@ sub free_image {
     return undef;
 }
 
+# set $untrusted if the file in question might be malicious since it isn't
+# created by our stack
+# this makes certain checks fatal, and adds extra checks for known problems like
+# - backing files (qcow2/vmdk)
+# - external data files (qcow2)
 sub file_size_info {
-    my ($filename, $timeout) = @_;
+    my ($filename, $timeout, $untrusted) = @_;
 
     my $st = File::stat::stat($filename);
 
     if (!defined($st)) {
 	my $extramsg = -l $filename ? ' - dangling symlink?' : '';
-	warn "failed to stat '$filename'$extramsg\n";
-	return undef;
+	my $msg = "failed to stat '$filename'$extramsg\n";
+	if ($untrusted) {
+	    die $msg;
+	} else {
+	    warn $msg;
+	    return undef;
+	}
     }
 
     if (S_ISDIR($st->mode)) {
@@ -975,18 +985,34 @@ sub file_size_info {
 	warn $err_output;
     }
     if (!$json) {
+    	die "failed to query file information with qemu-img\n" if $untrusted;
 	# skip decoding if there was no output, e.g. if there was a timeout.
 	return wantarray ? (undef, undef, undef, undef, $st->ctime) : undef;
     }
 
     my $info = eval { decode_json($json) };
     if (my $err = $@) {
-	warn "could not parse qemu-img info command output for '$filename' - $err\n";
-	return wantarray ? (undef, undef, undef, undef, $st->ctime) : undef;
+	my $msg = "could not parse qemu-img info command output for '$filename' - $err\n";
+	if ($untrusted) {
+	    die $msg;
+	} else {
+	    warn $msg;
+	    return wantarray ? (undef, undef, undef, undef, $st->ctime) : undef;
+	}
+    }
+
+    if ($untrusted) {
+	if (my $format_specific = $info->{'format-specific'}) {
+	    if ($format_specific->{type} eq 'qcow2' && $format_specific->{data}->{"data-file"}) {
+		die "$filename: 'data-file' references are not allowed!\n";
+	    }
+	}
     }
 
     my ($size, $format, $used, $parent) = $info->@{qw(virtual-size format actual-size backing-filename)};
 
+    die "backing file not allowed for untrusted image '$filename'!\n" if $untrusted && $parent;
+
     ($size) = ($size =~ /^(\d+)$/); # untaint
     die "size '$size' not an integer\n" if !defined($size);
     # coerce back from string
-- 
2.39.5



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