From f0eda031d616760fd723c1db464ecd0585de5f27 Mon Sep 17 00:00:00 2001 From: Staffan Gustafsson Date: Wed, 26 Apr 2017 03:07:54 +0200 Subject: [PATCH] Performance improvements to security checks when group policies are in effect for ExecutionPolicy (#2588) * Caching the parent id of the current process In certain scenarios, like a domain joined machine with an executionpolicy set via GPO, a lot of checks are done for the ExecutionPolicy, and this is in the hot path. * Caching HasGPScriptParent. It is expensive to determine if a script was run as a result of applying a group policy. This does not change during the lifetime of a process. This only affect the scenario when an execution policy has been set by a group policy. * Using available Microsoft.PowerShell.ProcessCodeMethods.GetParentPid --- .../security/SecuritySupport.cs | 119 +++++++++++------- .../utils/PsUtils.cs | 44 +++---- 2 files changed, 88 insertions(+), 75 deletions(-) diff --git a/src/System.Management.Automation/security/SecuritySupport.cs b/src/System.Management.Automation/security/SecuritySupport.cs index 9f0bbecada..57d29343f8 100644 --- a/src/System.Management.Automation/security/SecuritySupport.cs +++ b/src/System.Management.Automation/security/SecuritySupport.cs @@ -258,6 +258,69 @@ namespace System.Management.Automation.Internal return ExecutionPolicy.Restricted; } + + private static bool? _hasGpScriptParent; + + /// + /// A value indicating that the current process was launched by GPScript.exe + /// Used to determine execution policy when group policies are in effect + /// + /// + /// This is somewhat expensive to determine and does not change within the lifetime of the current process + /// + private static bool HasGpScriptParent + { + get + { + if (!_hasGpScriptParent.HasValue) + { + _hasGpScriptParent = IsCurrentProcessLaunchedByGpScript(); + } + return _hasGpScriptParent.Value; + } + } + + private static bool IsCurrentProcessLaunchedByGpScript() + { + Process currentProcess = Process.GetCurrentProcess(); + string gpScriptPath = IO.Path.Combine( + Environment.GetFolderPath(Environment.SpecialFolder.System), + "gpscript.exe"); + + bool foundGpScriptParent = false; + try + { + while (currentProcess != null) + { + if (String.Equals(gpScriptPath, + PsUtils.GetMainModule(currentProcess).FileName, StringComparison.OrdinalIgnoreCase)) + { + foundGpScriptParent = true; + break; + } + else + { + currentProcess = PsUtils.GetParentProcess(currentProcess); + } + } + } + catch (System.ComponentModel.Win32Exception) + { + // If you attempt to retrieve the MainModule of a 64-bit process + // from a WOW64 (32-bit) process, the Win32 API has a fatal + // flaw that causes this to return the error: + // "Only part of a ReadProcessMemory or WriteProcessMemory + // request was completed." + // In this case, we just catch the exception and eat it. + // The implication is that logon / logoff scripts that somehow + // launch the Wow64 version of PowerShell will be subject + // to the execution policy deployed by Group Policy (where + // our goal here is to not have the Group Policy execution policy + // affect logon / logoff scripts. + } + return foundGpScriptParent; + } + internal static ExecutionPolicy GetExecutionPolicy(string shellId, ExecutionPolicyScope scope) { #if UNIX @@ -292,55 +355,15 @@ namespace System.Management.Automation.Internal case ExecutionPolicyScope.MachinePolicy: { string groupPolicyPreference = GetGroupPolicyValue(shellId, scope); - if (!String.IsNullOrEmpty(groupPolicyPreference)) + + // Be sure we aren't being called by Group Policy + // itself. A group policy should never block a logon / + // logoff script. + if (String.IsNullOrEmpty(groupPolicyPreference) || HasGpScriptParent) { - // Be sure we aren't being called by Group Policy - // itself. A group policy should never block a logon / - // logoff script. - Process currentProcess = Process.GetCurrentProcess(); - string gpScriptPath = IO.Path.Combine( - Environment.GetFolderPath(Environment.SpecialFolder.System), - "gpscript.exe"); - bool foundGpScriptParent = false; - - try - { - while (currentProcess != null) - { - if (String.Equals(gpScriptPath, - PsUtils.GetMainModule(currentProcess).FileName, StringComparison.OrdinalIgnoreCase)) - { - foundGpScriptParent = true; - break; - } - else - { - currentProcess = PsUtils.GetParentProcess(currentProcess); - } - } - } - catch (System.ComponentModel.Win32Exception) - { - // If you attempt to retrieve the MainModule of a 64-bit process - // from a WOW64 (32-bit) process, the Win32 API has a fatal - // flaw that causes this to return the error: - // "Only part of a ReadProcessMemory or WriteProcessMemory - // request was completed." - // In this case, we just catch the exception and eat it. - // The implication is that logon / logoff scripts that somehow - // launch the Wow64 version of PowerShell will be subject - // to the execution policy deployed by Group Policy (where - // our goal here is to not have the Group Policy execution policy - // affect logon / logoff scripts. - } - - if (!foundGpScriptParent) - { - return ParseExecutionPolicy(groupPolicyPreference); - } - } - - return ExecutionPolicy.Undefined; + return ExecutionPolicy.Undefined; + } + return ParseExecutionPolicy(groupPolicyPreference); } } diff --git a/src/System.Management.Automation/utils/PsUtils.cs b/src/System.Management.Automation/utils/PsUtils.cs index f59015a99d..2575eada8e 100644 --- a/src/System.Management.Automation/utils/PsUtils.cs +++ b/src/System.Management.Automation/utils/PsUtils.cs @@ -86,52 +86,42 @@ namespace System.Management.Automation return mainModule; } + // Cache of the current process' parentId + private static int? s_currentParentProcessId; + private static readonly int s_currentProcessId = Process.GetCurrentProcess().Id; + /// /// Retrieve the parent process of a process. /// /// Previously this code used WMI, but WMI is causing a CPU spike whenever the query gets called as it results in - /// tzres.dll and tzres.mui.dll being loaded into every process to conver the time information to local format. - /// For perf reasons, we result to P/Invoke. + /// tzres.dll and tzres.mui.dll being loaded into every process to convert the time information to local format. + /// For perf reasons, we resort to P/Invoke. /// /// /// The process we want to find the /// parent of internal static Process GetParentProcess(Process current) { - int parentPid = 0; + var processId = current.Id; -#if !UNIX - PlatformInvokes.PROCESSENTRY32 pe32 = new PlatformInvokes.PROCESSENTRY32 { }; - pe32.dwSize = (uint)ClrFacade.SizeOf(); + // This is a common query (parent id for the current process) + // Use cached value if available + var parentProcessId = processId == s_currentProcessId && s_currentParentProcessId.HasValue ? + s_currentParentProcessId.Value : + Microsoft.PowerShell.ProcessCodeMethods.GetParentPid(current); - using (PlatformInvokes.SafeSnapshotHandle hSnapshot = PlatformInvokes.CreateToolhelp32Snapshot(PlatformInvokes.SnapshotFlags.Process, (uint)current.Id)) + // cache the current process parent pid if it hasn't been done yet + if (processId == s_currentProcessId && !s_currentParentProcessId.HasValue) { - if (!PlatformInvokes.Process32First(hSnapshot, ref pe32)) - { - int errno = Marshal.GetLastWin32Error(); - if (errno == PlatformInvokes.ERROR_NO_MORE_FILES) - { - return null; - } - } - do - { - if (pe32.th32ProcessID == (uint)current.Id) - { - parentPid = (int)pe32.th32ParentProcessID; - break; - } - - } while (PlatformInvokes.Process32Next(hSnapshot, ref pe32)); + s_currentParentProcessId = parentProcessId; } -#endif - if (parentPid == 0) + if (parentProcessId == 0) return null; try { - Process returnProcess = Process.GetProcessById(parentPid); + Process returnProcess = Process.GetProcessById(parentProcessId); // Ensure the process started before the current // process, as it could have gone away and had the