Skip to content

Performance improvements to security checks when group policies are in effect for ExecutionPolicy #2588

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mirichmo merged 3 commits into from
Apr 26, 2017
Merged

Performance improvements to security checks when group policies are in effect for ExecutionPolicy #2588

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cadb0a5
Caching the parent id of the current process
Nov 2, 2016
1ad62ce
Caching HasGPScriptParent.
Nov 2, 2016
f807df7
Using available Microsoft.PowerShell.ProcessCodeMethods.GetParentPid
Apr 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 71 additions & 48 deletions src/System.Management.Automation/security/SecuritySupport.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,69 @@ internal static ExecutionPolicy GetExecutionPolicy(string shellId)
return ExecutionPolicy.Restricted;
}


private static bool? _hasGpScriptParent;

/// <summary>
/// A value indicating that the current process was launched by GPScript.exe
/// Used to determine execution policy when group policies are in effect
/// </summary>
/// <remarks>
/// This is somewhat expensive to determine and does not change within the lifetime of the current process
/// </remarks>
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
Copy link
Contributor

@LeeHolmes LeeHolmes Nov 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conditional is the opposite of what I think you intended.

Copy link
Collaborator Author

@powercode powercode Nov 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, fixed in the updated pull request

Expand Down Expand Up @@ -292,55 +355,15 @@ internal static ExecutionPolicy GetExecutionPolicy(string shellId, ExecutionPoli
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.
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;
// 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)
{
return ExecutionPolicy.Undefined;
}
return ParseExecutionPolicy(groupPolicyPreference);
}
}

Expand Down
44 changes: 17 additions & 27 deletions src/System.Management.Automation/utils/PsUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,52 +86,42 @@ internal static ProcessModule GetMainModule(Process targetProcess)
return mainModule;
}

// Cache of the current process' parentId
private static int? s_currentParentProcessId;
private static readonly int s_currentProcessId = Process.GetCurrentProcess().Id;

/// <summary>
/// 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.
/// </summary>
///
/// <param name="current">The process we want to find the
/// parent of</param>
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<PlatformInvokes.PROCESSENTRY32>();
// 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
Expand Down