Skip to content

fix: PATH hijacking, orphan process kill, log privacy sanitization (SEC-007, SEC-008, PRIV-001) #376

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
laurentiu021 merged 1 commit into from
May 15, 2026
Merged

fix: PATH hijacking, orphan process kill, log privacy sanitization (SEC-007, SEC-008, PRIV-001) #376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 11 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,17 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]

## [0.48.13] - 2026-05-15

### Fixed
- **UninstallerService (SEC-007)** — trusted system binaries (MsiExec, rundll32)
now resolved to absolute System32 path before execution, preventing PATH
hijacking attacks.
- **SpeedTestService (SEC-008)** — Ookla CLI process now killed on timeout or
cancellation, preventing orphan processes consuming resources indefinitely.
- **SpeedTestService (PRIV-001)** — all exception messages in Log.Debug calls
now sanitized via LogService.SanitizePath to prevent username leakage in logs.

## [0.48.12] - 2026-05-15

### Fixed
Expand Down
28 changes: 19 additions & 9 deletions SysManager/SysManager/Services/SpeedTestService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,16 +207,26 @@ public async Task<SpeedTestResult> RunOoklaAsync(
await Task.Run(() => proc.Start(), linked).ConfigureAwait(false);
var stdout = await proc.StandardOutput.ReadToEndAsync(linked);
var stderr = await proc.StandardError.ReadToEndAsync(linked);
await proc.WaitForExitAsync(linked);
try
{
await proc.WaitForExitAsync(linked);
}
catch (OperationCanceledException)
{
// Timeout or user cancellation — kill the orphan process
try { if (!proc.HasExited) proc.Kill(entireProcessTree: true); }
catch (InvalidOperationException) { /* already exited */ }
throw;
}
Comment on lines 208 to +220

@coderabbitai coderabbitai Bot May 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
file="$(fd -t f SpeedTestService.cs | head -n1)"
echo "Inspecting cancellation scope in: $file"
nl -ba "$file" | sed -n '200,222p'

Repository: laurentiu021/SystemManager

Length of output: 216

🏁 Script executed:

cat -n SysManager/SysManager/Services/SpeedTestService.cs | sed -n '200,230p'

Repository: laurentiu021/SystemManager

Length of output: 1738

🏁 Script executed:

# Search for other Process + StandardOutput/StandardError patterns
rg -n "StandardOutput\.ReadToEndAsync|StandardError\.ReadToEndAsync" SysManager --type cs -B 5 -A 5

Repository: laurentiu021/SystemManager

Length of output: 2373

🏁 Script executed:

# Check for other Process with cancellation token patterns
rg -n "WaitForExitAsync.*CancellationToken|ReadToEndAsync.*CancellationToken" SysManager --type cs -B 3 -A 3

Repository: laurentiu021/SystemManager

Length of output: 52

Protect process cleanup against cancellation during stream reads.

ReadToEndAsync(linked) on lines 208-209 can throw OperationCanceledException before reaching the try/catch block, bypassing the proc.Kill() cleanup and leaving an orphaned process. Both reads must be wrapped in the cancellation-guarded block.

Suggested fix 
         using var proc = new Process();
         proc.StartInfo = psi;
         await Task.Run(() => proc.Start(), linked).ConfigureAwait(false);
-        var stdout = await proc.StandardOutput.ReadToEndAsync(linked);
-        var stderr = await proc.StandardError.ReadToEndAsync(linked);
+        string stdout;
+        string stderr;
         try
         {
+            var stdoutTask = proc.StandardOutput.ReadToEndAsync(linked);
+            var stderrTask = proc.StandardError.ReadToEndAsync(linked);
             await proc.WaitForExitAsync(linked);
+            stdout = await stdoutTask.ConfigureAwait(false);
+            stderr = await stderrTask.ConfigureAwait(false);
         }
         catch (OperationCanceledException)
         {
             // Timeout or user cancellation — kill the orphan process
             try { if (!proc.HasExited) proc.Kill(entireProcessTree: true); }
             catch (InvalidOperationException) { /* already exited */ }
             throw;
         }
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@SysManager/SysManager/Services/SpeedTestService.cs` around lines 208 - 220,
The Process stdout/stderr reads (proc.StandardOutput.ReadToEndAsync(linked) and
proc.StandardError.ReadToEndAsync(linked)) can throw OperationCanceledException
before the existing WaitForExitAsync catch runs, so move the two ReadToEndAsync
calls inside the same try/catch that surrounds WaitForExitAsync (or create a
single try that calls both ReadToEndAsync and then WaitForExitAsync) and in the
catch for OperationCanceledException ensure you call
proc.Kill(entireProcessTree: true) if !proc.HasExited (handling
InvalidOperationException as currently done) before rethrowing; this guarantees
proc cleanup even when cancellation happens during stream reads.


if (proc.ExitCode != 0)
{
// If the exe is broken/corrupt, delete it so next run re-downloads it.
if (proc.ExitCode == -1073741515) // STATUS_DLL_NOT_FOUND
{
try { File.Delete(exe); }
catch (IOException ex) { Log.Debug("Cleanup failed (locked): {Error}", ex.Message); }
catch (UnauthorizedAccessException ex) { Log.Debug("Cleanup failed (access): {Error}", ex.Message); }
catch (IOException ex2) { Log.Debug("Cleanup failed (locked): {Error}", LogService.SanitizePath(ex2.Message)); }
catch (UnauthorizedAccessException ex2) { Log.Debug("Cleanup failed (access): {Error}", LogService.SanitizePath(ex2.Message)); }
}
throw new InvalidOperationException($"Ookla failed ({proc.ExitCode}): {stderr}");
}
Expand Down Expand Up @@ -276,8 +286,8 @@ private static async Task<string> EnsureOoklaAsync(
if (File.Exists(path) && new FileInfo(path).Length < 1024)
{
try { File.Delete(path); }
catch (IOException ex) { Log.Debug("Cleanup failed (locked): {Error}", ex.Message); }
catch (UnauthorizedAccessException ex) { Log.Debug("Cleanup failed (access): {Error}", ex.Message); }
catch (IOException ex) { Log.Debug("Cleanup failed (locked): {Error}", LogService.SanitizePath(ex.Message)); }
catch (UnauthorizedAccessException ex) { Log.Debug("Cleanup failed (access): {Error}", LogService.SanitizePath(ex.Message)); }
}
return !File.Exists(path);
}, ct).ConfigureAwait(false);
Expand Down Expand Up @@ -344,8 +354,8 @@ await Task.Run(() =>
{
Log.Warning("Ookla speedtest.exe Authenticode subject mismatch: {Subject}", cert?.Subject ?? "none");
try { File.Delete(exe); }
catch (IOException ex2) { Log.Debug("Cleanup failed (locked): {Error}", ex2.Message); }
catch (UnauthorizedAccessException ex2) { Log.Debug("Cleanup failed (access): {Error}", ex2.Message); }
catch (IOException ex2) { Log.Debug("Cleanup failed (locked): {Error}", LogService.SanitizePath(ex2.Message)); }
catch (UnauthorizedAccessException ex2) { Log.Debug("Cleanup failed (access): {Error}", LogService.SanitizePath(ex2.Message)); }
throw new InvalidOperationException(
$"Ookla speedtest.exe failed Authenticode verification (subject: {cert?.Subject ?? "none"}). Binary deleted for security.");
}
Expand All @@ -355,8 +365,8 @@ await Task.Run(() =>
{
Log.Warning(ex, "Ookla speedtest.exe has no valid Authenticode signature");
try { File.Delete(exe); }
catch (IOException ex2) { Log.Debug("Cleanup failed (locked): {Error}", ex2.Message); }
catch (UnauthorizedAccessException ex2) { Log.Debug("Cleanup failed (access): {Error}", ex2.Message); }
catch (IOException ex2) { Log.Debug("Cleanup failed (locked): {Error}", LogService.SanitizePath(ex2.Message)); }
catch (UnauthorizedAccessException ex2) { Log.Debug("Cleanup failed (access): {Error}", LogService.SanitizePath(ex2.Message)); }
throw new InvalidOperationException(
"Ookla speedtest.exe has no valid Authenticode signature. Binary deleted for security.", ex);
}
Expand Down
11 changes: 10 additions & 1 deletion SysManager/SysManager/Services/UninstallerService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,14 +255,23 @@
// without admin, so we must not blindly execute whatever is there.
// Use exact filename match for system binaries to prevent bypass via
// similarly-named executables (e.g. "MsiExecEvil.exe").
// Resolve trusted binaries to absolute System32 path to prevent PATH hijacking.
var exeFileName = System.IO.Path.GetFileName(exe);
var isTrustedSystemBinary =
exeFileName.Equals("MsiExec.exe", StringComparison.OrdinalIgnoreCase) ||
exeFileName.Equals("MsiExec", StringComparison.OrdinalIgnoreCase) ||
exeFileName.Equals("rundll32.exe", StringComparison.OrdinalIgnoreCase) ||
exeFileName.Equals("rundll32", StringComparison.OrdinalIgnoreCase);

if (!isTrustedSystemBinary)
if (isTrustedSystemBinary)
{
// Resolve to absolute path in System32 to prevent PATH hijacking
var systemDir = Environment.GetFolderPath(Environment.SpecialFolder.System);
var resolvedName = exeFileName.EndsWith(".exe", StringComparison.OrdinalIgnoreCase)
? exeFileName : exeFileName + ".exe";
exe = System.IO.Path.Combine(systemDir, resolvedName);

Check notice

Code scanning / CodeQL

Call to 'System.IO.Path.Combine' may silently drop its earlier arguments Note

Call to 'System.IO.Path.Combine' may silently drop its earlier arguments.
}
else
{
if (!System.IO.File.Exists(exe))
throw new InvalidOperationException(
Expand Down
Loading