fix: security hardening batch 2 — cache validation, zip slip, DLL hijack, parser hardening#403
Merged
Merged
Conversation
…ack, parser hardening - SEC-M2: Update cache validated by SHA-256 hash instead of file size only - SEC-M3: Zip Slip protection with manual extraction and path validation - SEC-M4: Ookla CLI launched with System32 CWD to prevent DLL hijacking - SEC-M6: Service name validated before registry path interpolation - SEC-M7: ParseUninstallCommand rejects shell metacharacters, improved .exe boundary detection, removed unsafe fallback - SEC-M8: Expanded security contract documentation on ExecutionPolicy.Bypass - Replaced bare catch blocks with specific exception types in 7 services - Updated tests for new ParseUninstallCommand behavior
📝 WalkthroughWalkthroughThis release hardens security across download validation, command parsing, process execution, service configuration access, and event handling. It replaces generic exception handlers with specific types across six services, implements SHA-256-based update caching, adds Zip Slip protection to SpeedTest extraction, validates uninstaller command input, and validates service names before registry access. ChangesSecurity Hardening Release 0.48.24
Sequence Diagram(s)Not applicable. The changes are primarily defensive security hardening (input validation, exception handling specificity) and integrity checks (SHA-256 caching, Zip Slip protection) without introducing new multi-component orchestration flows or significantly altering existing control flow patterns. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+356
to
+374
| foreach (var entry in archive.Entries) | ||
| { | ||
| // Skip directory entries | ||
| if (string.IsNullOrEmpty(entry.Name)) continue; | ||
|
|
||
| var destinationPath = Path.GetFullPath(Path.Combine(toolsDir, entry.FullName)); | ||
| if (!destinationPath.StartsWith(fullToolsDir, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| Log.Warning("Zip Slip attempt blocked: {Entry} resolves outside target dir", entry.FullName); | ||
| continue; | ||
| } | ||
|
|
||
| // Ensure subdirectory exists | ||
| var entryDir = Path.GetDirectoryName(destinationPath); | ||
| if (!string.IsNullOrEmpty(entryDir)) | ||
| Directory.CreateDirectory(entryDir); | ||
|
|
||
| entry.ExtractToFile(destinationPath, overwrite: true); | ||
| } |
| // Skip directory entries | ||
| if (string.IsNullOrEmpty(entry.Name)) continue; | ||
|
|
||
| var destinationPath = Path.GetFullPath(Path.Combine(toolsDir, entry.FullName)); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/PingMonitorService.cs`:
- Around line 137-138: The handler loop currently only catches
ObjectDisposedException and InvalidOperationException which allows other
exceptions from subscribers to escape and break the loop; update the catch logic
in the PingMonitorService subscriber handler loop (the block that currently has
"catch (ObjectDisposedException) { ... }" and "catch (InvalidOperationException)
{ ... }") to catch all exceptions (e.g., catch Exception ex) so any subscriber
failure is isolated, log the exception details for diagnostics, and continue the
loop rather than letting the exception propagate.
In `@SysManager/SysManager/Services/SpeedTestService.cs`:
- Around line 355-363: The Zip-Slip guard using
destinationPath.StartsWith(fullToolsDir) is insufficient because sibling paths
like ..\tools_evil share the same prefix; update the check around
archive.Entries extraction (variables: fullToolsDir, destinationPath, toolsDir)
to validate that destinationPath is truly inside toolsDir by computing a
relative path (e.g., Path.GetRelativePath(fullToolsDir, destinationPath)) and
rejecting any result that starts with ".." or is equal to "..", or alternatively
ensure the prefix comparison includes a directory separator boundary; only
proceed with extraction when the relative path indicates the file is within the
toolsDir tree.
In `@SysManager/SysManager/Services/TracerouteService.cs`:
- Around line 106-107: The RunAsync loop in TracerouteService currently only
catches ObjectDisposedException and InvalidOperationException, so other
subscriber exceptions can bubble out and abort traceroute emission; update the
exception handling in RunAsync (around the subscriber publish/emit logic) to
catch a broader exception (e.g., Exception) so any unexpected subscriber error
is swallowed and does not terminate the loop, and ensure the catch logs the
exception (using the existing logger or Trace) with context instead of silently
ignoring it; keep the specific catches if you want special handling for
ObjectDisposedException/InvalidOperationException but add a final catch-all for
other exceptions to preserve the method's isolation contract.
In `@SysManager/SysManager/Services/UninstallerService.cs`:
- Around line 321-327: The current pre-parse blacklist on the raw command string
in UninstallerService (the check on variable command) incorrectly rejects valid
quoted paths and still allows shell-hosted attacks; instead move this validation
to run after you parse the command/executable (i.e., after the quoted-path
branch and before the trusted-directory check), and replace the raw-character
reject with a narrow allowlist for known uninstall hosts (e.g., "msiexec",
"rundll32") plus an explicit denylist of interpreter/shell binaries (e.g.,
"cmd", "powershell", "pwsh", "wscript", "cscript", "mshta") applied to the
parsed executable name; ensure the logic references the parsed executable (not
the raw command string) and keeps existing trusted-directory checks intact.
In `@SysManager/SysManager/Services/UpdateService.cs`:
- Around line 195-206: The hash computation fails because the write stream
created with "await using var file = File.Create(target)" remains open when
File.OpenRead(target) is called; close or dispose the write stream before
computing the SHA-256 so File.OpenRead can succeed. Fix by scoping or explicitly
closing the write stream (e.g., end the await-using block or call
file.FlushAsync/Dispose/Close after the write loop) prior to the block that
computes downloadedHash with File.OpenRead(target) and SHA256.HashData, then
write the hashFile; keep the existing exception handling for File IO.
🪄 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: 6b3fca82-44a4-4909-818c-fcd40ff3633b
📒 Files selected for processing (13)
CHANGELOG.mdSysManager/SysManager.Tests/UninstallerServiceTests.csSysManager/SysManager/Services/DeepCleanupService.csSysManager/SysManager/Services/DiskHealthService.csSysManager/SysManager/Services/EventLogService.csSysManager/SysManager/Services/PingMonitorService.csSysManager/SysManager/Services/PowerShellRunner.csSysManager/SysManager/Services/ServiceManagerService.csSysManager/SysManager/Services/SpeedTestService.csSysManager/SysManager/Services/TracerouteMonitorService.csSysManager/SysManager/Services/TracerouteService.csSysManager/SysManager/Services/UninstallerService.csSysManager/SysManager/Services/UpdateService.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 (9)
SysManager/SysManager/Services/ServiceManagerService.cs (1)
158-166: LGTM!Also applies to: 169-169
SysManager/SysManager/Services/PowerShellRunner.cs (1)
16-24: LGTM!CHANGELOG.md (1)
9-46: LGTM!SysManager/SysManager/Services/SpeedTestService.cs (1)
195-200: LGTM!SysManager/SysManager/Services/DiskHealthService.cs (1)
163-166: LGTM!Also applies to: 172-175, 181-184
SysManager/SysManager/Services/EventLogService.cs (1)
55-56: LGTM!Also applies to: 100-101, 109-110
SysManager/SysManager/Services/DeepCleanupService.cs (1)
227-229: LGTM!SysManager/SysManager/Services/TracerouteMonitorService.cs (1)
97-100: LGTM!SysManager/SysManager/Services/UpdateService.cs (1)
146-174: LGTM!
Comment on lines
+137
to
+138
| catch (ObjectDisposedException) { /* subscriber disposed — skip */ } | ||
| catch (InvalidOperationException) { /* subscriber error — skip */ } |
There was a problem hiding this comment.
Keep subscriber isolation for all handler failures.
Catching only two exception types allows other subscriber exceptions to break the handler loop, which violates the isolation contract in this method.
Suggested fix
foreach (var h in handlers)
{
try { ((Action<PingSample>)h).Invoke(sample); }
catch (ObjectDisposedException) { /* subscriber disposed — skip */ }
catch (InvalidOperationException) { /* subscriber error — skip */ }
+ catch (Exception) { /* unexpected subscriber failure — skip to preserve fan-out isolation */ }
}🤖 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/PingMonitorService.cs` around lines 137 - 138,
The handler loop currently only catches ObjectDisposedException and
InvalidOperationException which allows other exceptions from subscribers to
escape and break the loop; update the catch logic in the PingMonitorService
subscriber handler loop (the block that currently has "catch
(ObjectDisposedException) { ... }" and "catch (InvalidOperationException) { ...
}") to catch all exceptions (e.g., catch Exception ex) so any subscriber failure
is isolated, log the exception details for diagnostics, and continue the loop
rather than letting the exception propagate.
Comment on lines
+355
to
+363
| var fullToolsDir = Path.GetFullPath(toolsDir); | ||
| foreach (var entry in archive.Entries) | ||
| { | ||
| // Skip directory entries | ||
| if (string.IsNullOrEmpty(entry.Name)) continue; | ||
|
|
||
| var destinationPath = Path.GetFullPath(Path.Combine(toolsDir, entry.FullName)); | ||
| if (!destinationPath.StartsWith(fullToolsDir, StringComparison.OrdinalIgnoreCase)) | ||
| { |
There was a problem hiding this comment.
Add a directory-boundary check to the Zip Slip guard.
StartsWith(fullToolsDir) still accepts sibling escapes like ..\tools_evil\payload.dll, because C:\...\tools_evil\... shares the same string prefix as C:\...\tools. This can still extract outside toolsDir.
Suggested fix
- var fullToolsDir = Path.GetFullPath(toolsDir);
+ var fullToolsDir = Path.GetFullPath(toolsDir)
+ .TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar)
+ + Path.DirectorySeparatorChar;
foreach (var entry in archive.Entries)
{
// Skip directory entries
if (string.IsNullOrEmpty(entry.Name)) continue;
var destinationPath = Path.GetFullPath(Path.Combine(toolsDir, entry.FullName));
if (!destinationPath.StartsWith(fullToolsDir, StringComparison.OrdinalIgnoreCase))
{
Log.Warning("Zip Slip attempt blocked: {Entry} resolves outside target dir", entry.FullName);
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var fullToolsDir = Path.GetFullPath(toolsDir); | |
| foreach (var entry in archive.Entries) | |
| { | |
| // Skip directory entries | |
| if (string.IsNullOrEmpty(entry.Name)) continue; | |
| var destinationPath = Path.GetFullPath(Path.Combine(toolsDir, entry.FullName)); | |
| if (!destinationPath.StartsWith(fullToolsDir, StringComparison.OrdinalIgnoreCase)) | |
| { | |
| var fullToolsDir = Path.GetFullPath(toolsDir) | |
| .TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar) | |
| Path.DirectorySeparatorChar; | |
| foreach (var entry in archive.Entries) | |
| { | |
| // Skip directory entries | |
| if (string.IsNullOrEmpty(entry.Name)) continue; | |
| var destinationPath = Path.GetFullPath(Path.Combine(toolsDir, entry.FullName)); | |
| if (!destinationPath.StartsWith(fullToolsDir, StringComparison.OrdinalIgnoreCase)) | |
| { |
🤖 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 355 - 363,
The Zip-Slip guard using destinationPath.StartsWith(fullToolsDir) is
insufficient because sibling paths like ..\tools_evil share the same prefix;
update the check around archive.Entries extraction (variables: fullToolsDir,
destinationPath, toolsDir) to validate that destinationPath is truly inside
toolsDir by computing a relative path (e.g., Path.GetRelativePath(fullToolsDir,
destinationPath)) and rejecting any result that starts with ".." or is equal to
"..", or alternatively ensure the prefix comparison includes a directory
separator boundary; only proceed with extraction when the relative path
indicates the file is within the toolsDir tree.
Comment on lines
+106
to
+107
| catch (ObjectDisposedException) { /* subscriber disposed — skip */ } | ||
| catch (InvalidOperationException) { /* subscriber error — skip */ } |
There was a problem hiding this comment.
Prevent subscriber exceptions from aborting traceroute emission.
With only two catches, a different handler exception can escape and terminate RunAsync, despite this method’s isolation contract.
Suggested fix
foreach (var h in handlers)
{
try { ((Action<TracerouteHop>)h).Invoke(hop); }
catch (ObjectDisposedException) { /* subscriber disposed — skip */ }
catch (InvalidOperationException) { /* subscriber error — skip */ }
+ catch (Exception) { /* unexpected subscriber failure — skip to preserve traceroute execution */ }
}🤖 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/TracerouteService.cs` around lines 106 - 107,
The RunAsync loop in TracerouteService currently only catches
ObjectDisposedException and InvalidOperationException, so other subscriber
exceptions can bubble out and abort traceroute emission; update the exception
handling in RunAsync (around the subscriber publish/emit logic) to catch a
broader exception (e.g., Exception) so any unexpected subscriber error is
swallowed and does not terminate the loop, and ensure the catch logs the
exception (using the existing logger or Trace) with context instead of silently
ignoring it; keep the specific catches if you want special handling for
ObjectDisposedException/InvalidOperationException but add a final catch-all for
other exceptions to preserve the method's isolation contract.
Comment on lines
+321
to
+327
| // SEC-M7: Reject obviously malicious patterns before parsing. | ||
| // Commands containing shell metacharacters that could chain commands. | ||
| if (command.Contains('|') || command.Contains('&') || | ||
| command.Contains(';') || command.Contains('`') || | ||
| command.Contains("$(")) | ||
| throw new InvalidOperationException( | ||
| $"Uninstall command contains shell metacharacters — refusing to parse: '{command}'"); |
There was a problem hiding this comment.
Parse first; this raw blacklist still allows shell hosts and now rejects valid quoted paths.
Because this check runs before the quoted-path branch, legitimate commands like "C:\Program Files\AT&T\App\uninstall.exe" /S now fail purely due to the folder name. At the same time, C:\Windows\System32\cmd.exe /c C:\Users\...\evil.exe contains none of these characters, so it still gets through and later passes the trusted-directory check. The safer boundary here is the parsed executable: keep the narrow allowlist for known uninstall hosts (msiexec, rundll32) and explicitly reject shell/interpreter binaries (cmd, powershell, pwsh, wscript, cscript, mshta, etc.) instead of scanning the raw command string.
🤖 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 321 - 327,
The current pre-parse blacklist on the raw command string in UninstallerService
(the check on variable command) incorrectly rejects valid quoted paths and still
allows shell-hosted attacks; instead move this validation to run after you parse
the command/executable (i.e., after the quoted-path branch and before the
trusted-directory check), and replace the raw-character reject with a narrow
allowlist for known uninstall hosts (e.g., "msiexec", "rundll32") plus an
explicit denylist of interpreter/shell binaries (e.g., "cmd", "powershell",
"pwsh", "wscript", "cscript", "mshta") applied to the parsed executable name;
ensure the logic references the parsed executable (not the raw command string)
and keeps existing trusted-directory checks intact.
Comment on lines
+195
to
+206
| // SEC-M2: Store SHA-256 hash of the downloaded binary for cache validation. | ||
| try | ||
| { | ||
| var downloadedHash = await Task.Run(() => | ||
| { | ||
| using var hashStream = File.OpenRead(target); | ||
| return Convert.ToHexString(SHA256.HashData(hashStream)); | ||
| }, ct).ConfigureAwait(false); | ||
| await File.WriteAllTextAsync(hashFile, downloadedHash, ct).ConfigureAwait(false); | ||
| } | ||
| catch (IOException) { /* non-fatal — next launch will re-download */ } | ||
| catch (UnauthorizedAccessException) { /* non-fatal */ } |
There was a problem hiding this comment.
File stream still open — hash computation always fails silently.
The file stream from line 183 (await using var file = File.Create(target)) is not disposed until the end of the try block. When File.OpenRead(target) at line 200 attempts to open the same file, it throws IOException due to exclusive file locking. This exception is caught at line 205 and swallowed, so the .sha256 file is never written.
This defeats the SEC-M2 fix: without a hash file, every launch triggers a full re-download.
🐛 Proposed fix: Flush and close the write stream before computing hash
read += n;
progress?.Report((read, total));
}
+
+ // Close write stream before reopening for hash computation
+ await file.DisposeAsync().ConfigureAwait(false);
// SEC-M2: Store SHA-256 hash of the downloaded binary for cache validation.
try
{
var downloadedHash = await Task.Run(() =>
{
using var hashStream = File.OpenRead(target);
return Convert.ToHexString(SHA256.HashData(hashStream));
}, ct).ConfigureAwait(false);
await File.WriteAllTextAsync(hashFile, downloadedHash, ct).ConfigureAwait(false);
}Alternatively, restructure to use a scoped block for the write stream:
{
await using var file = File.Create(target);
// ... write loop ...
} // file disposed here
// Now safe to read
try
{
var downloadedHash = ...
}🤖 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/UpdateService.cs` around lines 195 - 206, The
hash computation fails because the write stream created with "await using var
file = File.Create(target)" remains open when File.OpenRead(target) is called;
close or dispose the write stream before computing the SHA-256 so File.OpenRead
can succeed. Fix by scoping or explicitly closing the write stream (e.g., end
the await-using block or call file.FlushAsync/Dispose/Close after the write
loop) prior to the block that computes downloadedHash with File.OpenRead(target)
and SHA256.HashData, then write the hashFile; keep the existing exception
handling for File IO.
This was referenced May 18, 2026
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
…nfigureAwait(false) (#424) ## Summary **Batch 5+6** of the QA bug fix series (combined — related changes). ### Issue #401 — Hardcoded launcher paths - Added RiotLogDirs helper that scans all fixed drives for Riot Games/League of Legends log directories - Follows the same pattern as SteamRoots and SteamShaderCacheDirs - Other launcher caches (Epic, Battle.net, GOG, EA) correctly use %LOCALAPPDATA%/%PROGRAMDATA% — no changes needed ### Issue #402 — Unbounded icon cache - Added MaxCacheSize property (default 500, configurable) to IconExtractorService - When cache exceeds the limit, half the entries are evicted ### Issue #403 — Missing ConfigureAwait(false) - PerformanceService: all async calls now use ConfigureAwait(false) - UninstallerService: RunProcessAsync call uses ConfigureAwait(false) - WingetService: all RunProcessAsync calls use ConfigureAwait(false) ### Testing - Build: 0 errors Closes #401, Closes #402, Closes #403 Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
Adds CHANGELOG entries for all 9 releases from the QA bug fix session: - **v0.28.16** — Dispose lifecycle (#395, #410) - **v0.28.17** — CTS disposal + bare catch (#396, #413) - **v0.28.18** — Input validation + null checks (#397, #398) - **v0.28.19** — JSON error handling (#400) - **v0.28.20** — Drive scanning + cache eviction + ConfigureAwait (#401, #402, #403) - **v0.28.21** — Audit logging + error messages (#405, #407) - **v0.28.22** — SHA256 verification (#408, #409) - **v0.28.23** — Service timeout + snapshot persist + traceroute DNS (#414, #415, #416) - **v0.28.24** — Accessibility (#411) 18 bugs fixed in total. Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
…ack, parser hardening (#403) - SEC-M2: Update cache validated by SHA-256 hash instead of file size only - SEC-M3: Zip Slip protection with manual extraction and path validation - SEC-M4: Ookla CLI launched with System32 CWD to prevent DLL hijacking - SEC-M6: Service name validated before registry path interpolation - SEC-M7: ParseUninstallCommand rejects shell metacharacters, improved .exe boundary detection, removed unsafe fallback - SEC-M8: Expanded security contract documentation on ExecutionPolicy.Bypass - Replaced bare catch blocks with specific exception types in 7 services - Updated tests for new ParseUninstallCommand behavior 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
3 participants
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 remaining MEDIUM and LOW security findings from the comprehensive code review (SEC-M2 through SEC-M8 plus 7 LOW-severity bare-catch fixes).
Security fixes (MEDIUM)
Security fixes (LOW)
Tests
Files changed (13)
Services: UpdateService, SpeedTestService, ServiceManagerService, UninstallerService, PowerShellRunner, EventLogService, DeepCleanupService, DiskHealthService, TracerouteMonitorService, TracerouteService, PingMonitorService
Tests: UninstallerServiceTests
Docs: CHANGELOG.md
Build
0 errors, 0 new warnings.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
Documentation