From aa0effe68b76b5f0283629a8eacd8e50a6cb31bb Mon Sep 17 00:00:00 2001 From: Sebastian Solnica Date: Wed, 11 Dec 2024 08:16:18 +0100 Subject: [PATCH] Fix: premature monitor termination, ACCESS DENIED caused by unnecessary token opening (#63, maybe #70 as well) --- procgov-tests/Code/ProgramTests_CmdApp.cs | 2 +- procgov-tests/Code/ProgramTests_Monitor.cs | 4 +- procgov-tests/Code/ProgramTests_Service.cs | 6 +-- procgov/AccountPrivilegeModule.cs | 49 +++++++++++++++------- procgov/ExecutionModes.cs | 2 +- procgov/Program.cs | 8 +--- procgov/Program_CmdApp.cs | 7 ++-- procgov/Program_Monitor.cs | 7 ++++ 8 files changed, 54 insertions(+), 31 deletions(-) diff --git a/procgov-tests/Code/ProgramTests_CmdApp.cs b/procgov-tests/Code/ProgramTests_CmdApp.cs index 7aa6bdf..849c4de 100644 --- a/procgov-tests/Code/ProgramTests_CmdApp.cs +++ b/procgov-tests/Code/ProgramTests_CmdApp.cs @@ -87,7 +87,7 @@ public static async Task CmdAppAttachProcessAndUpdateJob() cmdMainThreadHandle.Dispose(); // start the monitor so the run command won't hang - var monitorTask = Task.Run(() => Program.Execute(new RunAsMonitor(Program.DefaultMaxMonitorIdleTime), cts.Token)); + var monitorTask = Task.Run(() => Program.Execute(new RunAsMonitor(Program.DefaultMaxMonitorIdleTime, true), cts.Token)); using (var pipe = new NamedPipeClientStream(".", Program.PipeName, PipeDirection.InOut, PipeOptions.Asynchronous)) { while (!pipe.IsConnected && !cts.IsCancellationRequested) diff --git a/procgov-tests/Code/ProgramTests_Monitor.cs b/procgov-tests/Code/ProgramTests_Monitor.cs index 7b0c9d7..62ca4d1 100644 --- a/procgov-tests/Code/ProgramTests_Monitor.cs +++ b/procgov-tests/Code/ProgramTests_Monitor.cs @@ -33,7 +33,7 @@ public static async Task MonitorNoProcessStarted() { using var cts = new CancellationTokenSource(10000); - var monitorTask = Task.Run(() => Program.Execute(new RunAsMonitor(Program.DefaultMaxMonitorIdleTime), cts.Token)); + var monitorTask = Task.Run(() => Program.Execute(new RunAsMonitor(Program.DefaultMaxMonitorIdleTime, true), cts.Token)); using var pipe = new NamedPipeClientStream(".", Program.PipeName, PipeDirection.InOut, PipeOptions.Asynchronous); @@ -267,7 +267,7 @@ static async Task StartMonitor(CancellationToken ct) try { await pipe.ConnectAsync(10, ct); } catch { - _ = Task.Run(() => Program.Execute(new RunAsMonitor(Program.DefaultMaxMonitorIdleTime), ct)); + _ = Task.Run(() => Program.Execute(new RunAsMonitor(Program.DefaultMaxMonitorIdleTime, true), ct)); while (!pipe.IsConnected && !ct.IsCancellationRequested) { diff --git a/procgov-tests/Code/ProgramTests_Service.cs b/procgov-tests/Code/ProgramTests_Service.cs index 4c6767f..8826fb8 100644 --- a/procgov-tests/Code/ProgramTests_Service.cs +++ b/procgov-tests/Code/ProgramTests_Service.cs @@ -88,7 +88,7 @@ public static async Task ServiceNewProcessStart() using var cts = new CancellationTokenSource(30000); // start the monitor so the run command (started by service) won't hang - var monitorTask = Task.Run(() => Program.Execute(new RunAsMonitor(TimeSpan.FromSeconds(30)), cts.Token)); + var monitorTask = Task.Run(() => Program.Execute(new RunAsMonitor(TimeSpan.FromSeconds(30), true), cts.Token)); using (var pipe = new NamedPipeClientStream(".", Program.PipeName, PipeDirection.InOut, PipeOptions.Asynchronous)) { while (!pipe.IsConnected && !cts.IsCancellationRequested) @@ -162,7 +162,7 @@ public static async Task ServiceClockTimeLimitOnProcess() using var cts = new CancellationTokenSource(30000); // start the monitor so the run command (started by service) won't hang - var monitorTask = Task.Run(() => Program.Execute(new RunAsMonitor(TimeSpan.FromSeconds(30)), cts.Token)); + var monitorTask = Task.Run(() => Program.Execute(new RunAsMonitor(TimeSpan.FromSeconds(30), true), cts.Token)); using (var pipe = new NamedPipeClientStream(".", Program.PipeName, PipeDirection.InOut, PipeOptions.Asynchronous)) { while (!pipe.IsConnected && !cts.IsCancellationRequested) @@ -235,7 +235,7 @@ public static async Task ServiceChildProcessLimit() using var cts = new CancellationTokenSource(30000); // start the monitor so the run command (started by service) won't hang - var monitorTask = Task.Run(() => Program.Execute(new RunAsMonitor(TimeSpan.FromSeconds(30)), cts.Token)); + var monitorTask = Task.Run(() => Program.Execute(new RunAsMonitor(TimeSpan.FromSeconds(30), true), cts.Token)); using (var pipe = new NamedPipeClientStream(".", Program.PipeName, PipeDirection.InOut, PipeOptions.Asynchronous)) { while (!pipe.IsConnected && !cts.IsCancellationRequested) diff --git a/procgov/AccountPrivilegeModule.cs b/procgov/AccountPrivilegeModule.cs index a6d17a1..4af71ca 100644 --- a/procgov/AccountPrivilegeModule.cs +++ b/procgov/AccountPrivilegeModule.cs @@ -10,31 +10,52 @@ namespace ProcessGovernor; internal unsafe static class AccountPrivilegeModule { - internal static List<(string PrivilegeName, bool IsSuccess)> EnableProcessPrivileges(SafeHandle processHandle, + internal static List<(string PrivilegeName, bool IsSuccess)> EnableProcessPrivileges(SafeHandle processHandle, List<(string PrivilegeName, bool Required)> privileges) { - CheckWin32Result(PInvoke.OpenProcessToken(processHandle, TOKEN_ACCESS_MASK.TOKEN_QUERY | TOKEN_ACCESS_MASK.TOKEN_ADJUST_PRIVILEGES, - out var tokenHandle)); + if (privileges.Count == 0) + { + return []; + } + + if (!PInvoke.OpenProcessToken(processHandle, TOKEN_ACCESS_MASK.TOKEN_QUERY | TOKEN_ACCESS_MASK.TOKEN_ADJUST_PRIVILEGES, + out var tokenHandle)) + { + var err = Marshal.GetLastWin32Error(); + if (privileges.Any(priv => priv.Required)) + { + throw new Win32Exception(err); + } + return privileges.Select(priv => (priv.PrivilegeName, false)).ToList(); + } + try { - return privileges.Select(privilege => + return privileges.Select(priv => { - CheckWin32Result(PInvoke.LookupPrivilegeValue(null, privilege.PrivilegeName, out var luid)); + var err = (int)WIN32_ERROR.NO_ERROR; - var privileges = new TOKEN_PRIVILEGES + if (PInvoke.LookupPrivilegeValue(null, priv.PrivilegeName, out var luid)) { - PrivilegeCount = 1, - Privileges = new() { e0 = new LUID_AND_ATTRIBUTES { Luid = luid, Attributes = TOKEN_PRIVILEGES_ATTRIBUTES.SE_PRIVILEGE_ENABLED } } - }; - PInvoke.AdjustTokenPrivileges(tokenHandle, false, &privileges, 0, null, null); - var lastWin32Error = Marshal.GetLastWin32Error(); + var privileges = new TOKEN_PRIVILEGES + { + PrivilegeCount = 1, + Privileges = new() { e0 = new LUID_AND_ATTRIBUTES { Luid = luid, Attributes = TOKEN_PRIVILEGES_ATTRIBUTES.SE_PRIVILEGE_ENABLED } } + }; + PInvoke.AdjustTokenPrivileges(tokenHandle, false, &privileges, 0, null, null); + err = Marshal.GetLastWin32Error(); + } + else + { + err = Marshal.GetLastWin32Error(); + } - if (lastWin32Error != (int)WIN32_ERROR.NO_ERROR && privilege.Required) + if (err != (int)WIN32_ERROR.NO_ERROR && priv.Required) { - throw new Win32Exception(lastWin32Error); + throw new Win32Exception(err); } - return (privilege.PrivilegeName, lastWin32Error == (int)WIN32_ERROR.NO_ERROR); + return (priv.PrivilegeName, err == (int)WIN32_ERROR.NO_ERROR); }).ToList(); } finally diff --git a/procgov/ExecutionModes.cs b/procgov/ExecutionModes.cs index 6c18744..d6611bf 100644 --- a/procgov/ExecutionModes.cs +++ b/procgov/ExecutionModes.cs @@ -25,7 +25,7 @@ record RunAsCmdApp( LaunchConfig LaunchConfig, ExitBehavior ExitBehavior) : IExecutionMode; -record RunAsMonitor(TimeSpan MaxMonitorIdleTime) : IExecutionMode; +record RunAsMonitor(TimeSpan MaxMonitorIdleTime, bool NoGui) : IExecutionMode; record RunAsService : IExecutionMode; diff --git a/procgov/Program.cs b/procgov/Program.cs index 34956c3..8becfe5 100644 --- a/procgov/Program.cs +++ b/procgov/Program.cs @@ -55,12 +55,6 @@ public static async Task Main(string[] args) _ => throw new NotImplementedException(), }; } - catch (Win32Exception ex) - { - Console.Error.WriteLine($"WIN32 ERROR: 0x{ex.ErrorCode:X}"); - Console.Error.WriteLine($"{ex}"); - return 0xff; - } catch (Exception ex) { Console.Error.WriteLine($"ERROR: {ex}"); @@ -308,7 +302,7 @@ internal static IExecutionMode ParseArgs(SystemInfo systemInfo, string[] rawArgs return (procargs, pids, runAsMonitor, runAsService, install, uninstall, uninstallAll) switch { - ([], [], true, false, false, false, false) => new RunAsMonitor(DefaultMaxMonitorIdleTime), + ([], [], true, false, false, false, false) => new RunAsMonitor(DefaultMaxMonitorIdleTime, launchConfig.HasFlag(LaunchConfig.NoGui)), ([], [], false, true, false, false, false) => new RunAsService(), ([var executable], [], false, false, true, false, false) => new SetupProcessGovernance( jobSettings, environment, privileges, executable, serviceInstallPath, serviceUserName, serviceUserPassword), diff --git a/procgov/Program_CmdApp.cs b/procgov/Program_CmdApp.cs index 03255cf..6d2afd2 100644 --- a/procgov/Program_CmdApp.cs +++ b/procgov/Program_CmdApp.cs @@ -129,7 +129,7 @@ static unsafe void StartMonitor() } // we always enable verbose logs for the monitor since it uses ETW or Debug output - string cmdline = $"{Environment.ProcessPath} --monitor --verbose\0"; + string cmdline = $"{Environment.ProcessPath} --monitor --verbose --nogui"; var pi = new PROCESS_INFORMATION(); var si = new STARTUPINFOW(); @@ -137,7 +137,8 @@ static unsafe void StartMonitor() fixed (char* cmdlinePtr = cmdline) { CheckWin32Result(PInvoke.CreateProcess(null, new PWSTR(cmdlinePtr), null, null, false, - PROCESS_CREATION_FLAGS.CREATE_NEW_PROCESS_GROUP, null, null, &si, &pi)); + PROCESS_CREATION_FLAGS.CREATE_NEW_PROCESS_GROUP | PROCESS_CREATION_FLAGS.CREATE_NEW_CONSOLE, + null, null, &si, &pi)); } PInvoke.CloseHandle(pi.hProcess); @@ -203,7 +204,7 @@ async Task AttachJobToProcesses(Win32Process[] targetProcesses) if (Array.FindIndex(assignedJobNames, jobName => jobName != "") is var monitoredProcessIndex && monitoredProcessIndex != -1) { jobName = assignedJobNames[monitoredProcessIndex]; - + // check if all assigned jobs are the same if (Array.FindIndex(assignedJobNames, n => n != jobName && n != "") is var idx && idx != -1) { diff --git a/procgov/Program_Monitor.cs b/procgov/Program_Monitor.cs index 900f50e..1baa2b7 100644 --- a/procgov/Program_Monitor.cs +++ b/procgov/Program_Monitor.cs @@ -9,6 +9,8 @@ using System.Security.Principal; using Windows.Win32; using Windows.Win32.Foundation; +using Windows.Win32.UI.WindowsAndMessaging; + using static ProcessGovernor.NtApi; namespace ProcessGovernor; @@ -22,6 +24,11 @@ public static async Task Execute(RunAsMonitor monitor, CancellationToken ct { try { + if (monitor.NoGui) + { + PInvoke.ShowWindow(PInvoke.GetConsoleWindow(), SHOW_WINDOW_CMD.SW_HIDE); + } + await StartMonitor(monitor.MaxMonitorIdleTime, ct); return 0; }