diff --git a/src/Microsoft.PowerShell.Commands.Diagnostics/PdhHelper.cs b/src/Microsoft.PowerShell.Commands.Diagnostics/PdhHelper.cs index 6f5d8f6e5ec..bd1c0ed3647 100644 --- a/src/Microsoft.PowerShell.Commands.Diagnostics/PdhHelper.cs +++ b/src/Microsoft.PowerShell.Commands.Diagnostics/PdhHelper.cs @@ -1071,6 +1071,9 @@ public uint LookupPerfNameByIndex(string machineName, uint index, out string loc if (res == PdhResults.PDH_MORE_DATA) { Marshal.FreeHGlobal(localizedPathPtr); + + // Set the value to 'IntPtr.Zero' so a reallocation failure won't cause the stale pointer to be double-freed in the finally block below. + localizedPathPtr = IntPtr.Zero; localizedPathPtr = Marshal.AllocHGlobal(strSize * sizeof(char)); res = PdhLookupPerfNameByIndex(machineName, index, localizedPathPtr, ref strSize); } diff --git a/src/System.Management.Automation/engine/hostifaces/InternalHostUserInterface.cs b/src/System.Management.Automation/engine/hostifaces/InternalHostUserInterface.cs index d8f2ef0bf90..f4bc55a8f5e 100644 --- a/src/System.Management.Automation/engine/hostifaces/InternalHostUserInterface.cs +++ b/src/System.Management.Automation/engine/hostifaces/InternalHostUserInterface.cs @@ -666,12 +666,21 @@ internal static Type GetFieldType(FieldDescription field) internal static bool IsSecuritySensitiveType(string typeName) { - if (typeName.Equals(nameof(PSCredential), StringComparison.OrdinalIgnoreCase)) + string effectiveName = typeName; + int suffixIndex = effectiveName.IndexOfAny(new char[] { '[', ',' }); + if (suffixIndex >= 0) + { + effectiveName = effectiveName.Substring(0, suffixIndex); + } + + if (effectiveName.Equals(nameof(PSCredential), StringComparison.OrdinalIgnoreCase) || + effectiveName.Equals(typeof(PSCredential).FullName, StringComparison.OrdinalIgnoreCase)) { return true; } - if (typeName.Equals(nameof(SecureString), StringComparison.OrdinalIgnoreCase)) + if (effectiveName.Equals(nameof(SecureString), StringComparison.OrdinalIgnoreCase) || + effectiveName.Equals(typeof(SecureString).FullName, StringComparison.OrdinalIgnoreCase)) { return true; } diff --git a/src/System.Management.Automation/engine/remoting/common/WireDataFormat/RemoteHost.cs b/src/System.Management.Automation/engine/remoting/common/WireDataFormat/RemoteHost.cs index 79b920c22b5..f0a6649fe36 100644 --- a/src/System.Management.Automation/engine/remoting/common/WireDataFormat/RemoteHost.cs +++ b/src/System.Management.Automation/engine/remoting/common/WireDataFormat/RemoteHost.cs @@ -379,12 +379,13 @@ internal Collection PerformSecurityChecksOnHostMessage(string co Type fieldType = InternalHostUserInterface.GetFieldType(fieldDesc); if (fieldType != null) { - if (fieldType == typeof(PSCredential)) + Type effectiveType = fieldType.IsArray ? fieldType.GetElementType() : fieldType; + if (effectiveType == typeof(PSCredential)) { havePSCredential = true; fieldDesc.ModifiedByRemotingProtocol = true; } - else if (fieldType == typeof(System.Security.SecureString)) + else if (effectiveType == typeof(System.Security.SecureString)) { prerequisiteCalls.Add(ConstructWarningMessageForSecureString( computerName, RemotingErrorIdStrings.RemoteHostPromptSecureStringPrompt)); diff --git a/src/System.Management.Automation/help/CabinetNativeApi.cs b/src/System.Management.Automation/help/CabinetNativeApi.cs index fd369ca03be..cbf466a99fa 100644 --- a/src/System.Management.Automation/help/CabinetNativeApi.cs +++ b/src/System.Management.Automation/help/CabinetNativeApi.cs @@ -354,6 +354,56 @@ internal static int FdiSeek(IntPtr fp, int offset, int origin) [UnmanagedFunctionPointer(CallingConvention.Cdecl, CharSet = CharSet.Ansi)] internal delegate IntPtr FdiNotifyDelegate(FdiNotificationType fdint, FdiNotification fdin); + /// + /// Validates that the extraction path is within the intended help directory. + /// Ensures proper handling of absolute paths, UNC paths, and relative path components. + /// + /// The intended help directory for extraction. + /// The path from the CAB entry. + /// The validated absolute file path. + /// Thrown when path validation fails. + private static string ValidateExtractionPath(string helpDirectory, string entryPath) + { + // Reject absolute paths, UNC paths, or rooted paths in CAB entries + if (Path.IsPathRooted(entryPath)) + { + throw new InvalidOperationException( + $"CAB entry contains an invalid rooted path: {entryPath}"); + } + + // Reject paths containing a colon to block Windows Alternate Data Streams (e.g. "file.txt:payload"). + // Path.IsPathRooted and Path.GetFullPath do not reject these on .NET Core. + if (entryPath.Contains(':')) + { + throw new InvalidOperationException( + $"CAB entry contains an invalid path with a colon: {entryPath}"); + } + + // Get the canonical (absolute) help directory path with trailing separator + // The trailing separator is needed to prevent false positive matches where a directory + // name is a prefix of another (e.g., "/path/help" matching "/path/help-backup/file.txt") + string canonicalHelpDir = Path.GetFullPath(helpDirectory); + if (!canonicalHelpDir.EndsWith(Path.DirectorySeparatorChar.ToString())) + { + canonicalHelpDir += Path.DirectorySeparatorChar; + } + + // Combine and resolve the full extraction path + string candidatePath = Path.Combine(helpDirectory, entryPath); + string resolvedPath = Path.GetFullPath(candidatePath); + + // Ensure resolved path starts with the canonical help directory. + // OrdinalIgnoreCase is intentional: NTFS on Windows is case-insensitive, so two paths that + // differ only in case refer to the same file and must be treated as equivalent here. + if (!resolvedPath.StartsWith(canonicalHelpDir, StringComparison.OrdinalIgnoreCase)) + { + throw new InvalidOperationException( + $"CAB entry path is invalid. Entry '{entryPath}' would extract to '{resolvedPath}' which is outside the intended directory '{canonicalHelpDir}'"); + } + + return resolvedPath; + } + // Handles FDI notification internal static IntPtr FdiNotify(FdiNotificationType fdint, FdiNotification fdin) { @@ -361,58 +411,97 @@ internal static IntPtr FdiNotify(FdiNotificationType fdint, FdiNotification fdin { case FdiNotificationType.FdintCOPY_FILE: { - // TODO: Should I catch exceptions for the new functions? - - // Copy target directory - string destPath = Marshal.PtrToStringAnsi(fdin.pv); + // Get the intended help directory + string helpDirectory = Marshal.PtrToStringAnsi(fdin.pv); + string absoluteFilePath; - // Split the path to a filename and path - string fileName = Path.GetFileName(fdin.psz1); - string remainingPsz1Path = Path.GetDirectoryName(fdin.psz1); - destPath = Path.Combine(destPath, remainingPsz1Path); - - Directory.CreateDirectory(destPath); // Creates all intermediate directories if necessary. + try + { + // fdin.psz1 should contain a relative path from the CAB entry + absoluteFilePath = ValidateExtractionPath(helpDirectory, fdin.psz1); + // Create all intermediate directories if necessary + string directoryPath = Path.GetDirectoryName(absoluteFilePath); + Directory.CreateDirectory(directoryPath); + } + catch (Exception e) when (e is InvalidOperationException || + e is ArgumentNullException || + e is ArgumentException || + e is NotSupportedException || + e is PathTooLongException || + e is System.Security.SecurityException || + e is IOException || + e is UnauthorizedAccessException) + { + // Path validation failed or directory could not be created - reject this CAB entry + return new IntPtr(-1); + } - // Create the file - string absoluteFilePath = Path.Combine(destPath, fileName); - return CabinetNativeApi.FdiOpen(absoluteFilePath, (int)OpFlags.Create, (int)(PermissionMode.Read | PermissionMode.Write)); // TODO: OK to ignore _O_SEQUENTIAL, WrOnly, and _O_BINARY? + // Note: OpFlags.Create and PermissionMode.Read|Write are sufficient for CAB extraction. + // The cabinet.dll API doesn't require _O_SEQUENTIAL, _O_WRONLY, or _O_BINARY flags. + return CabinetNativeApi.FdiOpen(absoluteFilePath, (int)OpFlags.Create, (int)(PermissionMode.Read | PermissionMode.Write)); } case FdiNotificationType.FdintCLOSE_FILE_INFO: { // Close the file CabinetNativeApi.FdiClose(fdin.hf); - // Set the file attributes - string destPath = Marshal.PtrToStringAnsi(fdin.pv); - string absoluteFilePath = Path.Combine(destPath, fdin.psz1); + // Get the intended help directory + string helpDirectory = Marshal.PtrToStringAnsi(fdin.pv); + string absoluteFilePath; - IntPtr hFile = PlatformInvokes.CreateFile( - absoluteFilePath, - PlatformInvokes.FileDesiredAccess.GenericRead | PlatformInvokes.FileDesiredAccess.GenericWrite, - PlatformInvokes.FileShareMode.Read, - IntPtr.Zero, - PlatformInvokes.FileCreationDisposition.OpenExisting, - PlatformInvokes.FileAttributes.Normal, - IntPtr.Zero); + try + { + absoluteFilePath = ValidateExtractionPath(helpDirectory, fdin.psz1); + } + catch (Exception e) when (e is InvalidOperationException || + e is ArgumentNullException || + e is ArgumentException || + e is NotSupportedException || + e is PathTooLongException || + e is System.Security.SecurityException) + { + // Path validation failed - reject this CAB entry + return new IntPtr(0); + } - if (hFile != IntPtr.Zero) + try { - PlatformInvokes.FILETIME ftFile = new PlatformInvokes.FILETIME(); - if (PlatformInvokes.DosDateTimeToFileTime(fdin.date, fdin.time, ftFile)) + // Set the file attributes + IntPtr hFile = PlatformInvokes.CreateFile( + absoluteFilePath, + PlatformInvokes.FileDesiredAccess.GenericRead | PlatformInvokes.FileDesiredAccess.GenericWrite, + PlatformInvokes.FileShareMode.Read, + IntPtr.Zero, + PlatformInvokes.FileCreationDisposition.OpenExisting, + PlatformInvokes.FileAttributes.Normal, + IntPtr.Zero); + + if (hFile != IntPtr.Zero) { - PlatformInvokes.FILETIME ftLocal = new PlatformInvokes.FILETIME(); - if (PlatformInvokes.LocalFileTimeToFileTime(ftFile, ftLocal)) + PlatformInvokes.FILETIME ftFile = new PlatformInvokes.FILETIME(); + if (PlatformInvokes.DosDateTimeToFileTime(fdin.date, fdin.time, ftFile)) { - PlatformInvokes.SetFileTime(hFile, ftLocal, null, ftLocal); + PlatformInvokes.FILETIME ftLocal = new PlatformInvokes.FILETIME(); + if (PlatformInvokes.LocalFileTimeToFileTime(ftFile, ftLocal)) + { + PlatformInvokes.SetFileTime(hFile, ftLocal, null, ftLocal); + } } + + PlatformInvokes.CloseHandle(hFile); } - PlatformInvokes.CloseHandle(hFile); - } + PlatformInvokes.SetFileAttributesW( + absoluteFilePath, + (PlatformInvokes.FileAttributes)fdin.attribs & (PlatformInvokes.FileAttributes.ReadOnly | PlatformInvokes.FileAttributes.Hidden | PlatformInvokes.FileAttributes.System | PlatformInvokes.FileAttributes.Archive)); - PlatformInvokes.SetFileAttributesW( - absoluteFilePath, - (PlatformInvokes.FileAttributes)fdin.attribs & (PlatformInvokes.FileAttributes.ReadOnly | PlatformInvokes.FileAttributes.Hidden | PlatformInvokes.FileAttributes.System | PlatformInvokes.FileAttributes.Archive)); + } + catch (Exception e) when (e is IOException || + e is UnauthorizedAccessException || + e is System.Security.SecurityException) + { + return new IntPtr(0); + } // Call notification function return new IntPtr(1); diff --git a/test/powershell/Modules/Microsoft.PowerShell.Utility/Debug-Runspace.Tests.ps1 b/test/powershell/Modules/Microsoft.PowerShell.Utility/Debug-Runspace.Tests.ps1 index 2b517e4f301..eb387bc44af 100644 --- a/test/powershell/Modules/Microsoft.PowerShell.Utility/Debug-Runspace.Tests.ps1 +++ b/test/powershell/Modules/Microsoft.PowerShell.Utility/Debug-Runspace.Tests.ps1 @@ -31,10 +31,10 @@ Describe "Debug-Runspace" -Tag "CI" { $rs1.Debugger.SetDebugMode("None") { Debug-Runspace -Runspace $rs1 -ErrorAction stop } | Should -Throw -ErrorId "InvalidOperation,Microsoft.PowerShell.Commands.DebugRunspaceCommand" } - + It "Should write attach event and mark runspace as having a remote debugger attached" { $onAttachName = [System.Management.Automation.PSEngineEvent]::OnDebugAttach - + $debugTarget = [PowerShell]::Create() $null = $debugTarget.AddCommand('Wait-Event').AddParameter('SourceIdentifier', $onAttachName) $waitTask = $debugTarget.BeginInvoke() @@ -44,8 +44,8 @@ Describe "Debug-Runspace" -Tag "CI" { $debugger = [PowerShell]::Create() $null = $debugger.AddCommand('Debug-Runspace').AddParameter('Id', $debugTarget.Runspace.Id) $debugTask = $debugger.BeginInvoke() - - $waitTask.AsyncWaitHandle.WaitOne(5000) | Should -BeTrue + + $waitTask.AsyncWaitHandle.WaitOne(10000) | Should -BeTrue $waitInfo = $debugTarget.EndInvoke($waitTask) $waitInfo.SourceIdentifier | Should -Be $onAttachName