fix: security hardening (#319)#361
Merged
Merged
Conversation
…ion, path regex (#319)
📝 WalkthroughWalkthroughThis PR adds three security hardening fixes across UninstallerService, EventLogService, and LogService to prevent command execution from tampered registry entries, XPath injection in event log queries, and incomplete path sanitization across drive letters. ChangesSecurity Hardening: Registry Validation, XPath Injection, and Path Sanitization
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@SysManager/SysManager/Services/UninstallerService.cs`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ccdbebca-89e9-46b5-9aca-0d04366dc65b
📒 Files selected for processing (4)
CHANGELOG.mdSysManager/SysManager/Services/EventLogService.csSysManager/SysManager/Services/LogService.csSysManager/SysManager/Services/UninstallerService.cs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & unit tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
SysManager/SysManager/Services/EventLogService.cs (1)
149-157: LGTM!SysManager/SysManager/Services/LogService.cs (1)
19-22: LGTM!CHANGELOG.md (1)
9-19: LGTM!
Comment on lines
+253
to
+267
| // 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."); | ||
| } |
There was a problem hiding this comment.
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.
This was referenced May 15, 2026
fix: PATH hijacking, orphan process kill, log privacy sanitization (SEC-007, SEC-008, PRIV-001)
#376
Merged
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
…idation (#361) ## Summary Batch 1 of the full code audit. Fixes 4 critical issues and 1 high-severity pattern across 5 service files. ### Critical fixes **C1 — 50MB upload array on LOH** (SpeedTestService.cs) - Replaced \ ew byte[50MB]\ with a streaming \RandomChunkStream\ that produces random data in 256KB chunks - Eliminates Large Object Heap pressure during speed test uploads **C2 — COM object leak** (StartupService.cs) - \WScript.Shell\ and shortcut COM objects created via \Activator.CreateInstance\ were never released - Added \Marshal.ReleaseComObject()\ in a \ inally\ block for both objects **C3 — Potential deadlock in SetTaskSchedulerEnabled** (StartupService.cs) - \proc.WaitForExit(5000)\ was called BEFORE \proc.StandardError.ReadToEnd()\ - If schtasks writes enough to fill the pipe buffer, the child blocks on write, WaitForExit times out, then ReadToEnd blocks forever - Fix: read stderr/stdout async BEFORE WaitForExit, increased timeout to 10s, added Kill on timeout **H5 — Command argument injection prevention** (5 files) - StartupService: validate task path rejects quotes and null chars - ServiceManagerService: validate serviceName + whitelist startType values for sc.exe - UninstallerService: validate packageId for winget uninstall - WingetService: validate packageId for winget upgrade ### Additional cleanup (StartupService.cs) - All 6 bare \catch { }\ blocks replaced with specific exception types (\SecurityException\, \UnauthorizedAccessException\, \IOException\, \COMException\, \InvalidOperationException\) - Added Serilog debug logging to all catch blocks for diagnostics - SpeedTestService: bare catch in MeasurePingAsync replaced with \PingException\, \SocketException\, \InvalidOperationException\ ### Files changed - \Services/StartupService.cs\ — COM leak, deadlock, bare catches, input validation - \Services/SpeedTestService.cs\ — 50MB array, bare catch - \Services/ServiceManagerService.cs\ — input validation - \Services/UninstallerService.cs\ — input validation - \Services/WingetService.cs\ — input validation ### Testing - Build: 0 errors (main project + tests project) - Existing tests unaffected (no behavioral changes, only safety improvements) Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses 3 security findings from the code review.
Changes
Closes #319
Summary by CodeRabbit
Release Notes