Fix: premature monitor termination, ACCESS DENIED caused by unnecessa…

…ry token opening (#63, maybe #70 as well)
lowleveldesign · Dec 11, 2024 · aa0effe · aa0effe
1 parent b552a33
commit aa0effe
Show file tree

Hide file tree

Showing 8 changed files with 54 additions and 31 deletions.
diff --git 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
@@ -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<NamedPipeClientStream> 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
@@ -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
@@ -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
@@ -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
@@ -55,12 +55,6 @@ public static async Task<int> 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
@@ -129,15 +129,16 @@ 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();
 
                 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<Win32Job> 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
@@ -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<int> 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;
         }