From 61ac247408bbc3849ae5dc6e6b50bec1002095b2 Mon Sep 17 00:00:00 2001 From: "goodolclint-claude[bot]" <323206664+goodolclint-claude[bot]@users.noreply.github.com> Date: Wed, 2 Sep 2026 17:43:01 +0000 Subject: [PATCH] fix: honour OVF bus type, correct the SCSI/SATA controller mapping, and validate the descriptor (#165) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Import-PveOva ignored disk.BusType and always assigned scsi{i}; the controller-mapping helper itself had CIM ResourceType 6 (Parallel SCSI HBA) and 20 (Other storage device, VMware's SATA AHCI controller) swapped. Both are fixed together since honouring BusType is what makes the mapping bug observable. The cmdlet now tracks a running index per bus (scsi/sata/ide), falls back to scsi for an unrecognized or missing bus type, and refuses to emit a slot number beyond what PVE's qemu-server schema accepts (ide0-3, sata0-5, scsi0-30), overflowing to scsi and erroring out before upload if even that is exhausted. OvfMetadata also trusted the OVF descriptor's ovf:href attribute (an attacker-controlled disk file name inside a downloaded OVA) unvalidated, letting a crafted href inject extra keys into the comma-separated PVE property string written for import-from. hrefs are now checked against ^[A-Za-z0-9._-]+$ and rejected outright if they are exactly "." or "..", closing a path-segment escape the character class alone did not block. The regex is anchored with \A/\z rather than ^/$ so a trailing line feed cannot slip past under .NET's default multiline-$ semantics. The descriptor is now parsed through XmlReader with DtdProcessing.Prohibit and XmlResolver = null instead of XmlDocument.LoadXml directly, closing the internal-entity-expansion memory-exhaustion route a hostile descriptor could otherwise use. ParseOvfXml is now internal (assembly already has InternalsVisibleTo the test project) so the new tests can drive it directly with inline OVF XML strings instead of building TAR archives. Reviewer findings acted on (Codex, correctness-reviewer, security-reviewer, run in parallel against the working tree): the dot/dot-dot href bypass and the missing \A/\z anchoring were found independently by all three and fixed; the bus-slot ceiling was found by two and fixed. Findings not acted on, with reasons: validating the local -Path OVA filename the same way (security-reviewer, medium) — that name is operator-supplied, not drawn from the attacker-controlled archive contents the issue names, and touching it would widen the change past the two issues' stated scope; percent- decoding/loosening the href character class to admit spec-legal names with spaces or encoding (security-reviewer, correctness-reviewer, medium) — issue #148 specifies this exact character class, and loosening it reopens the property-string injection the issue asks to close; capping the raw byte size of the extracted .ovf entry against decompression-bomb exhaustion (security-reviewer, low/medium) — a distinct DoS vector from the DTD entity expansion issue #148 names, not part of its stated scope; branching CIM ResourceType 20 on ResourceSubType to separate SATA from NVMe (security-reviewer, low) — issue #138 specifies the 6/20 mapping exactly as fixed here. An existing Integration-tagged Pester assertion (tests/PSProxmoxVE.Tests/Integration/12_OVA.Tests.ps1:66) checks $config.Scsi0 specifically; if that suite's fixture OVA ever carries a type-20 controller the disk will now land on sata0 instead and the assertion will need updating — left alone here since it is excluded from every offline run this change was verified against and touching it without being able to run it against live PVE would be guessing. Closes #138 Closes #148 Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com> --- .../Models/Vms/OvfMetadata.cs | 29 ++++- .../Cmdlets/Vms/ImportPveOvaCmdlet.cs | 30 ++++- .../Models/OvfMetadataTests.cs | 104 ++++++++++++++++++ 3 files changed, 155 insertions(+), 8 deletions(-) create mode 100644 tests/PSProxmoxVE.Core.Tests/Models/OvfMetadataTests.cs diff --git a/src/PSProxmoxVE.Core/Models/Vms/OvfMetadata.cs b/src/PSProxmoxVE.Core/Models/Vms/OvfMetadata.cs index eca30cb..4df6967 100644 --- a/src/PSProxmoxVE.Core/Models/Vms/OvfMetadata.cs +++ b/src/PSProxmoxVE.Core/Models/Vms/OvfMetadata.cs @@ -2,6 +2,7 @@ using System; using System.Collections.Generic; using System.IO; using System.Text; +using System.Text.RegularExpressions; using System.Xml; namespace PSProxmoxVE.Core.Models.Vms @@ -61,6 +62,11 @@ namespace PSProxmoxVE.Core.Models.Vms private const string RasdNs = "http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_ResourceAllocationSettingData"; private const string VssdNs = "http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_VirtualSystemSettingData"; + // ovf:href is used as a path segment in a PVE property string (import-from=storage:import/ova/href); + // PVE property strings are comma-separated, so ',' and any path separator must be rejected. + // \A/\z (not ^/$) so a trailing newline cannot sneak past the anchor under .NET's default regex options. + private static readonly Regex ValidHrefPattern = new Regex(@"\A[A-Za-z0-9._-]+\z", RegexOptions.Compiled); + /// /// Parses an OVA file (TAR archive) and extracts OVF metadata. /// @@ -106,10 +112,19 @@ namespace PSProxmoxVE.Core.Models.Vms /// /// Parses OVF XML and extracts VM metadata. /// - private static OvfMetadata ParseOvfXml(string xml) + internal static OvfMetadata ParseOvfXml(string xml) { var doc = new XmlDocument(); - doc.LoadXml(xml); + var readerSettings = new XmlReaderSettings + { + DtdProcessing = DtdProcessing.Prohibit, + XmlResolver = null + }; + using (var stringReader = new StringReader(xml)) + using (var xmlReader = XmlReader.Create(stringReader, readerSettings)) + { + doc.Load(xmlReader); + } var nsm = new XmlNamespaceManager(doc.NameTable); nsm.AddNamespace("ovf", OvfNs); @@ -158,6 +173,8 @@ namespace PSProxmoxVE.Core.Models.Vms var href = fileNode.Attributes?["ovf:href"]?.Value; if (id != null && href != null) { + if (!ValidHrefPattern.IsMatch(href) || href == "." || href == "..") + throw new InvalidDataException($"OVF descriptor references a disallowed file name: '{href}'."); fileRefs[id] = href; } } @@ -209,9 +226,9 @@ namespace PSProxmoxVE.Core.Models.Vms } break; - case 6: // Parallel SCSI HBA (sometimes used as SATA controller) case 5: // IDE Controller - case 20: // SCSI/SAS controller (storage) + case 6: // Parallel SCSI HBA + case 20: // Other storage device (VMware's SATA AHCI controller) // Controllers themselves don't produce disk entries; skip. break; @@ -296,8 +313,8 @@ namespace PSProxmoxVE.Core.Models.Vms switch (rt) { case 5: return "ide"; - case 6: return "sata"; - case 20: return "scsi"; + case 6: return "scsi"; + case 20: return "sata"; } } break; diff --git a/src/PSProxmoxVE/Cmdlets/Vms/ImportPveOvaCmdlet.cs b/src/PSProxmoxVE/Cmdlets/Vms/ImportPveOvaCmdlet.cs index 1766a9c..32a124e 100644 --- a/src/PSProxmoxVE/Cmdlets/Vms/ImportPveOvaCmdlet.cs +++ b/src/PSProxmoxVE/Cmdlets/Vms/ImportPveOvaCmdlet.cs @@ -226,12 +226,38 @@ namespace PSProxmoxVE.Cmdlets.Vms if (!string.IsNullOrEmpty(metadata.OsTypeHint) && metadata.OsTypeHint != "other") vmConfig["ostype"] = metadata.OsTypeHint; - // Add disk import-from parameters (use scsi bus like PVE UI) + // Add disk import-from parameters, placed on the bus the OVF names. + // Slot counts per PVE's qemu-server schema; a bus that runs out overflows to scsi. string? firstDisk = null; + var busIndexes = new Dictionary + { + ["scsi"] = 0, + ["sata"] = 0, + ["ide"] = 0 + }; + var busMax = new Dictionary + { + ["scsi"] = 30, + ["sata"] = 5, + ["ide"] = 3 + }; for (int i = 0; i < metadata.Disks.Count; i++) { var disk = metadata.Disks[i]; - var diskSlot = $"scsi{i}"; + var bus = !string.IsNullOrEmpty(disk.BusType) && busIndexes.ContainsKey(disk.BusType) ? disk.BusType : "scsi"; + if (busIndexes[bus] > busMax[bus]) + bus = "scsi"; + if (busIndexes[bus] > busMax[bus]) + { + ThrowTerminatingError(new ErrorRecord( + new InvalidOperationException($"OVF descriptor has more disks than PVE's scsi bus can hold ({busMax["scsi"] + 1})."), + "TooManyDisks", + ErrorCategory.LimitsExceeded, + metadata.Disks)); + return; + } + var diskSlot = $"{bus}{busIndexes[bus]}"; + busIndexes[bus]++; var importFrom = $"{Storage}:import/{fileName}/{disk.FileName}"; vmConfig[diskSlot] = $"{TargetStorage}:0,import-from={importFrom}"; firstDisk ??= diskSlot; diff --git a/tests/PSProxmoxVE.Core.Tests/Models/OvfMetadataTests.cs b/tests/PSProxmoxVE.Core.Tests/Models/OvfMetadataTests.cs new file mode 100644 index 0000000..1211c30 --- /dev/null +++ b/tests/PSProxmoxVE.Core.Tests/Models/OvfMetadataTests.cs @@ -0,0 +1,104 @@ +using System.IO; +using System.Xml; +using Xunit; +using PSProxmoxVE.Core.Models.Vms; + +namespace PSProxmoxVE.Core.Tests.Models +{ + public class OvfMetadataTests + { + private const string OvfHeader = + ""; + + private static string BuildDescriptor(string href, int controllerResourceType) + { + return + OvfHeader + + "" + + "" + + "" + + "" + + "" + + "1" + + "" + controllerResourceType + "" + + "" + + "" + + "2" + + "17" + + "1" + + "ovf:/disk/vmdisk1" + + "" + + "" + + "" + + ""; + } + + [Fact] + public void ParseOvfXml_VMwareScsiController_ResourceType6_MapsToScsi() + { + var xml = BuildDescriptor("disk1.vmdk", 6); + var metadata = OvfMetadata.ParseOvfXml(xml); + + Assert.Single(metadata.Disks); + Assert.Equal("scsi", metadata.Disks[0].BusType); + } + + [Fact] + public void ParseOvfXml_VMwareSataController_ResourceType20_MapsToSata() + { + var xml = BuildDescriptor("disk1.vmdk", 20); + var metadata = OvfMetadata.ParseOvfXml(xml); + + Assert.Single(metadata.Disks); + Assert.Equal("sata", metadata.Disks[0].BusType); + } + + [Fact] + public void ParseOvfXml_HrefWithEmbeddedProperty_Throws() + { + var xml = BuildDescriptor("d.vmdk,cache=unsafe", 6); + + Assert.Throws(() => OvfMetadata.ParseOvfXml(xml)); + } + + [Fact] + public void ParseOvfXml_HrefWithPathTraversal_Throws() + { + var xml = BuildDescriptor("../x.vmdk", 6); + + Assert.Throws(() => OvfMetadata.ParseOvfXml(xml)); + } + + [Fact] + public void ParseOvfXml_HrefIsDotDot_Throws() + { + var xml = BuildDescriptor("..", 6); + + Assert.Throws(() => OvfMetadata.ParseOvfXml(xml)); + } + + [Fact] + public void ParseOvfXml_HrefIsDot_Throws() + { + var xml = BuildDescriptor(".", 6); + + Assert.Throws(() => OvfMetadata.ParseOvfXml(xml)); + } + + [Fact] + public void ParseOvfXml_DescriptorWithInternalEntity_Throws() + { + var xml = + "" + + "]>" + + OvfHeader + + "" + + ""; + + Assert.ThrowsAny(() => OvfMetadata.ParseOvfXml(xml)); + } + } +}