From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
	by lore.proxmox.com (Postfix) with ESMTPS id 12B0A1FF15F
	for <inbox@lore.proxmox.com>; Mon, 18 Nov 2024 16:29:34 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
	by firstgate.proxmox.com (Proxmox) with ESMTP id E29FD13248;
	Mon, 18 Nov 2024 16:29:32 +0100 (CET)
From: Dominik Csapak <d.csapak@proxmox.com>
To: pve-devel@lists.proxmox.com
Date: Mon, 18 Nov 2024 16:29:06 +0100
Message-Id: <20241118152928.858590-4-d.csapak@proxmox.com>
X-Mailer: git-send-email 2.39.5
In-Reply-To: <20241118152928.858590-1-d.csapak@proxmox.com>
References: <20241118152928.858590-1-d.csapak@proxmox.com>
MIME-Version: 1.0
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.016 Adjusted score from AWL reputation of From: address
 BAYES_00                 -1.9 Bayes spam probability is 0 to 1%
 DMARC_MISSING             0.1 Missing DMARC policy
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
 Validity was blocked. See
 https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
 information.
 RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
 Validity was blocked. See
 https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
 information.
 RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to
 Validity was blocked. See
 https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more
 information.
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
Subject: [pve-devel] [PATCH storage v7 03/11] ovf: improve and simplify path
 checking code
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>

moves the filepath code a bit more closer to where it's actually used
checks the contained path before trying to find it's absolute path
properly add error handling to realpath

instead of checking the combined ovf_path + filepath, just make sure
filepath can't point to anythign besides a file in this directory
by checking for '.' and '..' (slashes are not allowed in SAFE_CHAR_CLASS_RE)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/GuestImport/OVF.pm | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/src/PVE/GuestImport/OVF.pm b/src/PVE/GuestImport/OVF.pm
index c7bff5f..966dcd1 100644
--- a/src/PVE/GuestImport/OVF.pm
+++ b/src/PVE/GuestImport/OVF.pm
@@ -220,15 +220,6 @@ ovf:Disk[\@ovf:diskId='%s']/\@ovf:capacityAllocationUnits", $disk_id);
 	    next;
 	}
 
-	# from Disk Node, find corresponding filepath
-	my $xpath_find_filepath = sprintf("/ovf:Envelope/ovf:References/ovf:File[\@ovf:id='%s']/\@ovf:href", $fileref);
-	my $filepath = $xpc->findvalue($xpath_find_filepath);
-	if (!$filepath) {
-	    warn "invalid file reference $fileref, skipping\n";
-	    next;
-	}
-	print "file path: $filepath\n" if $debug;
-
 	# from Item, find owning Controller type
 	my $controller_id = $xpc->findvalue('rasd:Parent', $item_node);
 	my $xpath_find_parent_type = sprintf("/ovf:Envelope/ovf:VirtualSystem/ovf:VirtualHardwareSection/\
@@ -244,22 +235,34 @@ ovf:Item[rasd:InstanceID='%s']/rasd:ResourceType", $controller_id);
 	my $adress_on_controller = $xpc->findvalue('rasd:AddressOnParent', $item_node);
 	my $pve_disk_address = id_to_pve($controller_type) . $adress_on_controller;
 
+	# from Disk Node, find corresponding filepath
+	my $xpath_find_filepath = sprintf("/ovf:Envelope/ovf:References/ovf:File[\@ovf:id='%s']/\@ovf:href", $fileref);
+	my $filepath = $xpc->findvalue($xpath_find_filepath);
+	if (!$filepath) {
+	    warn "invalid file reference $fileref, skipping\n";
+	    next;
+	}
+	print "file path: $filepath\n" if $debug;
+	my $original_filepath = $filepath;
+	($filepath) = $filepath =~ m|^(${PVE::Storage::SAFE_CHAR_CLASS_RE}+)$|; # untaint & check no sub/parent dirs
+	die "referenced path '$original_filepath' is invalid\n" if !$filepath || $filepath eq "." || $filepath eq "..";
+
 	# resolve symlinks and relative path components
 	# and die if the diskimage is not somewhere under the $ovf path
-	my $ovf_dir = realpath(dirname(File::Spec->rel2abs($ovf)));
-	my $backing_file_path = realpath(join ('/', $ovf_dir, $filepath));
+	my $ovf_dir = realpath(dirname(File::Spec->rel2abs($ovf)))
+	    or die "could not get absolute path of $ovf: $!\n";
+	my $backing_file_path = realpath(join ('/', $ovf_dir, $filepath))
+	    or die "could not get absolute path of $filepath: $!\n";
 	if ($backing_file_path !~ /^\Q${ovf_dir}\E/) {
 	    die "error parsing $filepath, are you using a symlink ?\n";
 	}
 
+	($backing_file_path) = $backing_file_path =~ m|^(/.*)|; # untaint
+
 	if (!-e $backing_file_path && !$isOva) {
 	    die "error parsing $filepath, file seems not to exist at $backing_file_path\n";
 	}
 
-	($backing_file_path) = $backing_file_path =~ m|^(/.*)|; # untaint
-	($filepath) = $filepath =~ m|^(${PVE::Storage::SAFE_CHAR_CLASS_RE}+)$|; # untaint & check no sub/parent dirs
-	die "invalid path\n" if !$filepath;
-
 	if (!$isOva) {
 	    my $size = PVE::Storage::file_size_info($backing_file_path);
 	    die "error parsing $backing_file_path, cannot determine file size\n"
-- 
2.39.5



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