Skip to content

fix: security hardening (#319) #361

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 14, 2026
Merged

fix: security hardening (#319) #361

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.8] - 2026-05-14

### Fixed
- **UninstallerService (SEC-002)** — UninstallLocalAsync now validates that the
executable exists and has a .exe extension before running. Prevents execution
of arbitrary commands from HKCU registry keys (modifiable without admin).
- **EventLogService (SEC-003)** — XPath sanitization now strips quotes, brackets,
slashes in addition to single quotes to prevent XPath injection.
- **LogService (SEC-004)** — path sanitization regex now covers all drive letters
(A-Z:\Users\) instead of only C: drive.

## [0.48.7] - 2026-05-14

### Changed
Expand Down
9 changes: 8 additions & 1 deletion SysManager/SysManager/Services/EventLogService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,14 @@ private static string BuildXPath(EventLogQueryOptions opt)

if (!string.IsNullOrWhiteSpace(opt.ProviderName))
{
var safe = opt.ProviderName.Replace("'", "");
// SEC-003: Strip XPath metacharacters to prevent injection.
var safe = opt.ProviderName
.Replace("'", "")
.Replace("\"", "")
.Replace("[", "")
.Replace("]", "")
.Replace("/", "")
.Replace("\\", "");
clauses.Add($"Provider[@Name='{safe}']");
}

Expand Down
4 changes: 2 additions & 2 deletions SysManager/SysManager/Services/LogService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ public static class LogService
public static string LogDir { get; } =
Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData), "SysManager", "logs");

// Matches C:\Users\<username>\ and replaces the username with [user].
// Matches <DriveLetter>:\Users\<username>\ and replaces the username with [user].
private static readonly Regex UserPathRegex = new(
@"(?i)(C:\\Users\\)[^\\]+",
@"(?i)([A-Z]:\\Users\\)[^\\]+",
RegexOptions.Compiled);

public static void Init()
Expand Down
16 changes: 16 additions & 0 deletions SysManager/SysManager/Services/UninstallerService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,22 @@ public async Task<int> UninstallLocalAsync(InstalledApp app, CancellationToken c
// Handles both quoted paths ("C:\path\uninstall.exe" /S) and unquoted.
var (exe, args) = ParseUninstallCommand(command);

// SEC-002: Validate the executable exists and is a real file (not a
// script or arbitrary command). HKCU uninstall keys can be modified
// without admin, so we must not blindly execute whatever is there.
if (!exe.StartsWith("MsiExec", StringComparison.OrdinalIgnoreCase) &&
!exe.StartsWith("rundll32", StringComparison.OrdinalIgnoreCase))
{
if (!System.IO.File.Exists(exe))
throw new InvalidOperationException(
$"Uninstall executable not found: '{exe}'. The app may have been removed already.");

var ext = System.IO.Path.GetExtension(exe);
if (!ext.Equals(".exe", StringComparison.OrdinalIgnoreCase))
throw new InvalidOperationException(
$"Uninstall target is not an executable (.exe): '{exe}'. Refusing to run for security.");
}
Comment on lines +253 to +267

@coderabbitai coderabbitai Bot May 14, 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 | 🔴 Critical | ⚡ Quick win

StartsWith allowlist can be bypassed by similarly named executables.

Line [256]-Line [257] trusts any executable whose name begins with MsiExec/rundll32 (for example, MsiExecEvil.exe), which re-opens arbitrary command execution from HKCU uninstall data.

🔧 Proposed fix 
-        if (!exe.StartsWith("MsiExec", StringComparison.OrdinalIgnoreCase) &&
-            !exe.StartsWith("rundll32", StringComparison.OrdinalIgnoreCase))
+        var exeName = System.IO.Path.GetFileName(exe);
+        var isTrustedSystemBinary =
+            exeName.Equals("MsiExec", StringComparison.OrdinalIgnoreCase) ||
+            exeName.Equals("MsiExec.exe", StringComparison.OrdinalIgnoreCase) ||
+            exeName.Equals("rundll32", StringComparison.OrdinalIgnoreCase) ||
+            exeName.Equals("rundll32.exe", StringComparison.OrdinalIgnoreCase);
+
+        if (!isTrustedSystemBinary)
         {
             if (!System.IO.File.Exists(exe))
                 throw new InvalidOperationException(
                     $"Uninstall executable not found: '{exe}'. The app may have been removed already.");
🤖 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/UninstallerService.cs` around lines 253 - 267,
The current StartsWith allowlist is unsafe because names like "MsiExecEvil.exe"
pass; update the validation in UninstallerService to parse the executable token
from the exe string (trim surrounding quotes and take the substring before any
whitespace), then use System.IO.Path.GetFileName on that token and compare
case-insensitively for exact basenames "msiexec.exe" and "rundll32.exe" (instead
of StartsWith). Keep the existing existence and extension checks but apply them
to the resolved executable path/token so only exact legitimate system
executables are allowed.


Log.Information("Uninstalling local app '{Name}' via: {Exe} {Args}", app.Name, exe, args);
return await _runner.RunProcessAsync(exe, args, ct).ConfigureAwait(false);
}
Expand Down
Loading