From f29a1f4d20e33daacc6d4ac248b51a65e0fede31 Mon Sep 17 00:00:00 2001 From: laurentiu021 Date: Fri, 15 May 2026 17:12:06 +0300 Subject: [PATCH] =?UTF-8?q?fix:=20security=20hardening=20batch=202=20?= =?UTF-8?q?=E2=80=94=20cache=20validation,=20zip=20slip,=20DLL=20hijack,?= =?UTF-8?q?=20parser=20hardening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- CHANGELOG.md | 38 ++++++++++++++++ .../UninstallerServiceTests.cs | 31 +++++++++++-- .../SysManager/Services/DeepCleanupService.cs | 4 +- .../SysManager/Services/DiskHealthService.cs | 15 +++++-- .../SysManager/Services/EventLogService.cs | 9 ++-- .../SysManager/Services/PingMonitorService.cs | 3 +- .../SysManager/Services/PowerShellRunner.cs | 14 +++--- .../Services/ServiceManagerService.cs | 12 ++++- .../SysManager/Services/SpeedTestService.cs | 33 +++++++++++++- .../Services/TracerouteMonitorService.cs | 5 ++- .../SysManager/Services/TracerouteService.cs | 3 +- .../SysManager/Services/UninstallerService.cs | 42 +++++++++++++---- .../SysManager/Services/UpdateService.cs | 45 +++++++++++++++++-- 13 files changed, 221 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5cc8e403..4094f2fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,44 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +## [0.48.24] - 2026-05-15 + +### Fixed +- **UpdateService** — cached download now validated by SHA-256 hash (stored in + companion `.sha256` file) instead of file size alone; prevents cache poisoning + with same-size payloads (SEC-M2). +- **SpeedTestService** — Zip Slip protection: manual extraction validates each + entry path stays within the target directory; blocks path traversal attacks + via crafted zip archives (SEC-M3). +- **SpeedTestService** — DLL hijacking mitigation: Ookla CLI process now + launches with `WorkingDirectory` set to System32 instead of the user-writable + tools directory, preventing CWD-based DLL search order hijacking (SEC-M4). +- **ServiceManagerService** — defensive validation on service names before + interpolating into registry paths; rejects names containing path separators + or null characters (SEC-M6). +- **UninstallerService** — `ParseUninstallCommand` hardened: rejects shell + metacharacters (`|&;` backtick `$(`) to prevent command injection; improved + `.exe` boundary detection to avoid misparsing paths with `.exe` substrings; + removed unsafe fallback that treated unparseable strings as executables + (SEC-M7). +- **PowerShellRunner** — expanded security contract documentation clarifying + that `ExecutionPolicy.Bypass` is safe only because all script content is + hard-coded in source; callers must never interpolate user input (SEC-M8). +- **DiskHealthService** — replaced bare `catch` blocks in WMI conversion + helpers with specific exception types (`FormatException`, `OverflowException`, + `InvalidCastException`). +- **DeepCleanupService** — replaced bare `catch` with specific `IOException`, + `UnauthorizedAccessException`, `SecurityException`. +- **TracerouteMonitorService** — replaced bare `catch` with specific network + and operation exception types. +- **TracerouteService** — replaced generic `catch (Exception)` in event raiser + with specific `ObjectDisposedException`, `InvalidOperationException`. +- **PingMonitorService** — replaced bare `catch` in event raiser with specific + exception types. +- **EventLogService** — replaced bare `catch` blocks in record projection and + message formatting with specific `EventLogException`, + `InvalidOperationException`. + ## [0.48.23] - 2026-05-15 ### Fixed diff --git a/SysManager/SysManager.Tests/UninstallerServiceTests.cs b/SysManager/SysManager.Tests/UninstallerServiceTests.cs index 3c51d72f..327474a7 100644 --- a/SysManager/SysManager.Tests/UninstallerServiceTests.cs +++ b/SysManager/SysManager.Tests/UninstallerServiceTests.cs @@ -270,11 +270,11 @@ public void ParseUninstallCommand_UnquotedExePath_FindsExeBoundary() } [Fact] - public void ParseUninstallCommand_NoExeExtension_FallsBackToFullString() + public void ParseUninstallCommand_NoExeExtension_ThrowsInvalidOperation() { - var (exe, args) = UninstallerService.ParseUninstallCommand("some-command"); - Assert.Equal("some-command", exe); - Assert.Equal("", args); + // SEC-M7: Commands without a valid .exe boundary are rejected for security. + Assert.Throws( + () => UninstallerService.ParseUninstallCommand("some-command")); } [Fact] @@ -286,4 +286,27 @@ public void ParseUninstallCommand_MsiExecWithQn_DoesNotAddQuiet() Assert.DoesNotContain("/quiet", args); Assert.Contains("/qn", args); } + + [Theory] + [InlineData("cmd.exe /c calc.exe | evil.exe")] + [InlineData("uninstall.exe & del /q *")] + [InlineData("app.exe; rm -rf /")] + [InlineData("tool.exe `whoami`")] + [InlineData("app.exe $(malicious)")] + public void ParseUninstallCommand_ShellMetacharacters_Throws(string command) + { + // SEC-M7: Shell metacharacters are rejected to prevent command injection. + Assert.Throws( + () => UninstallerService.ParseUninstallCommand(command)); + } + + [Fact] + public void ParseUninstallCommand_ExeInMiddleOfPath_FindsCorrectBoundary() + { + // SEC-M7: ".exe" followed by non-boundary char should not split there. + var (exe, args) = UninstallerService.ParseUninstallCommand( + @"C:\dir\app.exefiles\tool.exe /silent"); + Assert.Equal(@"C:\dir\app.exefiles\tool.exe", exe); + Assert.Equal("/silent", args); + } } diff --git a/SysManager/SysManager/Services/DeepCleanupService.cs b/SysManager/SysManager/Services/DeepCleanupService.cs index ab6e78e9..9478817b 100644 --- a/SysManager/SysManager/Services/DeepCleanupService.cs +++ b/SysManager/SysManager/Services/DeepCleanupService.cs @@ -224,7 +224,9 @@ private static IReadOnlyList Scan(IProgress? prog } files++; } - catch { } + catch (IOException) { /* file locked or inaccessible — skip */ } + catch (UnauthorizedAccessException) { /* no permission — skip */ } + catch (System.Security.SecurityException) { /* restricted — skip */ } } } diff --git a/SysManager/SysManager/Services/DiskHealthService.cs b/SysManager/SysManager/Services/DiskHealthService.cs index 51d695dc..a3150cb2 100644 --- a/SysManager/SysManager/Services/DiskHealthService.cs +++ b/SysManager/SysManager/Services/DiskHealthService.cs @@ -160,19 +160,28 @@ private static void ApplyVerdict(DiskHealthReport r) private static double? ToDouble(object? o) { if (o == null) return null; - try { var v = Convert.ToDouble(o); return Math.Abs(v) < 1e-9 ? null : v; } catch { return null; } + try { var v = Convert.ToDouble(o); return Math.Abs(v) < 1e-9 ? null : v; } + catch (FormatException) { return null; } + catch (OverflowException) { return null; } + catch (InvalidCastException) { return null; } } private static int? ToInt(object? o) { if (o == null) return null; - try { return Convert.ToInt32(o); } catch { return null; } + try { return Convert.ToInt32(o); } + catch (FormatException) { return null; } + catch (OverflowException) { return null; } + catch (InvalidCastException) { return null; } } private static long? ToLong(object? o) { if (o == null) return null; - try { var v = Convert.ToInt64(o); return v == 0 ? null : v; } catch { return null; } + try { var v = Convert.ToInt64(o); return v == 0 ? null : v; } + catch (FormatException) { return null; } + catch (OverflowException) { return null; } + catch (InvalidCastException) { return null; } } private static string MapMedia(uint v) => v switch diff --git a/SysManager/SysManager/Services/EventLogService.cs b/SysManager/SysManager/Services/EventLogService.cs index 2ea96236..2a0ab53c 100644 --- a/SysManager/SysManager/Services/EventLogService.cs +++ b/SysManager/SysManager/Services/EventLogService.cs @@ -52,7 +52,8 @@ private async IAsyncEnumerable ReadInternal( FriendlyEventEntry? entry = null; try { entry = Project(rec, opt.LogName); } - catch { /* skip malformed record */ } + catch (EventLogException) { /* skip malformed record */ } + catch (InvalidOperationException) { /* skip malformed record */ } finally { rec.Dispose(); } if (entry == null) continue; @@ -96,7 +97,8 @@ private static string SafeFormatMessage(EventRecord rec) var msg = rec.FormatDescription(); if (!string.IsNullOrWhiteSpace(msg)) return msg; } - catch { /* ignore; fall back */ } + catch (EventLogException) { /* format failed — fall back */ } + catch (InvalidOperationException) { /* format failed — fall back */ } // Fallback: assemble from properties so we at least show something. try @@ -104,7 +106,8 @@ private static string SafeFormatMessage(EventRecord rec) var parts = rec.Properties?.Select(p => p?.Value?.ToString() ?? "") ?? []; return string.Join(" ", parts).Trim(); } - catch { return "(message not available)"; } + catch (EventLogException) { return "(message not available)"; } + catch (InvalidOperationException) { return "(message not available)"; } } private static string FirstLine(string s) diff --git a/SysManager/SysManager/Services/PingMonitorService.cs b/SysManager/SysManager/Services/PingMonitorService.cs index 4c73492e..d2a0c5c3 100644 --- a/SysManager/SysManager/Services/PingMonitorService.cs +++ b/SysManager/SysManager/Services/PingMonitorService.cs @@ -134,7 +134,8 @@ private void RaiseSampleReceived(PingSample sample) foreach (var h in handlers) { try { ((Action)h).Invoke(sample); } - catch { /* swallow subscriber errors */ } + catch (ObjectDisposedException) { /* subscriber disposed — skip */ } + catch (InvalidOperationException) { /* subscriber error — skip */ } } } diff --git a/SysManager/SysManager/Services/PowerShellRunner.cs b/SysManager/SysManager/Services/PowerShellRunner.cs index 2bf64522..4e207cbc 100644 --- a/SysManager/SysManager/Services/PowerShellRunner.cs +++ b/SysManager/SysManager/Services/PowerShellRunner.cs @@ -13,11 +13,15 @@ namespace SysManager.Services; /// Runs PowerShell scripts in-process with live streaming of all output streams. /// Uses System.Management.Automation so we don't spawn external pwsh.exe processes. /// -/// Security note (SEC-005): ExecutionPolicy is set to Bypass for in-process -/// runspaces because SysManager only executes its own static scripts — never user-supplied -/// or downloaded scripts. RunScriptViaPwshAsync also uses -ExecutionPolicy Bypass for the -/// same reason. Callers that use RunAsync or RunScriptViaPwshAsync must only pass -/// hard-coded script strings; never pass user input directly as a script. +/// Security note (SEC-005 / SEC-M8): ExecutionPolicy is set to Bypass for +/// in-process runspaces because SysManager only executes its own static scripts — never +/// user-supplied or downloaded scripts. RunScriptViaPwshAsync also uses -ExecutionPolicy +/// Bypass for the same reason. +/// +/// SECURITY CONTRACT: Callers MUST only pass hard-coded script strings to +/// RunAsync and RunScriptViaPwshAsync. User input MUST NEVER be interpolated into scripts. +/// Violation of this contract creates a code injection vulnerability. The Bypass policy +/// is safe ONLY because the script content is fully controlled by SysManager's source code. /// public sealed class PowerShellRunner { diff --git a/SysManager/SysManager/Services/ServiceManagerService.cs b/SysManager/SysManager/Services/ServiceManagerService.cs index 946e167d..18a5c522 100644 --- a/SysManager/SysManager/Services/ServiceManagerService.cs +++ b/SysManager/SysManager/Services/ServiceManagerService.cs @@ -155,8 +155,18 @@ private static string GetServiceDescription(ServiceController sc) { try { + // SEC-M6: Validate service name before interpolating into registry path. + // Although ServiceController.GetServices() returns names from the SCM + // (trusted source), we defensively reject names with path separators or + // registry metacharacters to prevent registry path traversal. + var name = sc.ServiceName; + if (string.IsNullOrWhiteSpace(name) || + name.Contains('\\') || name.Contains('/') || + name.Contains('\0')) + return ""; + using var key = Microsoft.Win32.Registry.LocalMachine.OpenSubKey( - $@"SYSTEM\CurrentControlSet\Services\{sc.ServiceName}"); + $@"SYSTEM\CurrentControlSet\Services\{name}"); return key?.GetValue("Description")?.ToString() ?? ""; } catch (System.Security.SecurityException) { return ""; } diff --git a/SysManager/SysManager/Services/SpeedTestService.cs b/SysManager/SysManager/Services/SpeedTestService.cs index bcd29dcc..1768b120 100644 --- a/SysManager/SysManager/Services/SpeedTestService.cs +++ b/SysManager/SysManager/Services/SpeedTestService.cs @@ -192,7 +192,12 @@ public async Task RunOoklaAsync( RedirectStandardError = true, UseShellExecute = false, CreateNoWindow = true, - ErrorDialog = false // suppress Win32 "DLL not found" system dialogs + ErrorDialog = false, // suppress Win32 "DLL not found" system dialogs + // SEC-M4: Set working directory to System32 instead of the tools dir. + // This prevents DLL hijacking via CWD search order — if an attacker + // plants a malicious DLL in the user-writable tools directory, the + // process won't load it because CWD is System32 (admin-protected). + WorkingDirectory = Environment.GetFolderPath(Environment.SpecialFolder.System) }; // Start the process on a thread-pool thread so Process.Start() @@ -342,7 +347,31 @@ await Task.Run(() => progress?.Report((15, "Extracting…")); await Task.Run(() => { - ZipFile.ExtractToDirectory(zipPath, toolsDir, overwriteFiles: true); + // SEC-M3: Manual extraction with Zip Slip protection. + // ZipFile.ExtractToDirectory does not validate that entry paths + // stay within the target directory — a crafted zip with "../" + // entries could write files outside toolsDir. + using var archive = ZipFile.OpenRead(zipPath); + 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)) + { + 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); + } File.Delete(zipPath); }, ct).ConfigureAwait(false); diff --git a/SysManager/SysManager/Services/TracerouteMonitorService.cs b/SysManager/SysManager/Services/TracerouteMonitorService.cs index c14268ba..ec1e4f8a 100644 --- a/SysManager/SysManager/Services/TracerouteMonitorService.cs +++ b/SysManager/SysManager/Services/TracerouteMonitorService.cs @@ -94,7 +94,10 @@ private async Task PumpAsync(CancellationToken ct) RouteCompleted?.Invoke(target.Host, hops); } catch (OperationCanceledException) { return; } - catch { /* swallow per-target errors */ } + catch (System.Net.Sockets.SocketException) { /* network error — skip */ } + catch (System.Net.NetworkInformation.PingException) { /* ping failed — skip */ } + catch (TimeoutException) { /* target unreachable — skip */ } + catch (InvalidOperationException) { /* traceroute failed for this target — skip */ } } try { await Task.Delay(Interval, ct); } diff --git a/SysManager/SysManager/Services/TracerouteService.cs b/SysManager/SysManager/Services/TracerouteService.cs index 76c41776..8d61c96f 100644 --- a/SysManager/SysManager/Services/TracerouteService.cs +++ b/SysManager/SysManager/Services/TracerouteService.cs @@ -103,7 +103,8 @@ private void RaiseHopCompleted(TracerouteHop hop) foreach (var h in handlers) { try { ((Action)h).Invoke(hop); } - catch (Exception) { /* swallow subscriber errors to protect iteration */ } + catch (ObjectDisposedException) { /* subscriber disposed — skip */ } + catch (InvalidOperationException) { /* subscriber error — skip */ } } } } diff --git a/SysManager/SysManager/Services/UninstallerService.cs b/SysManager/SysManager/Services/UninstallerService.cs index 3d4f88fc..8f7d0767 100644 --- a/SysManager/SysManager/Services/UninstallerService.cs +++ b/SysManager/SysManager/Services/UninstallerService.cs @@ -318,6 +318,14 @@ internal static (string Exe, string Args) ParseUninstallCommand(string command) { command = command.Trim(); + // 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}'"); + // Case 1: Quoted executable path if (command.StartsWith('"')) { @@ -360,17 +368,35 @@ internal static (string Exe, string Args) ParseUninstallCommand(string command) return (command[..spaceIdx], command[(spaceIdx + 1)..].TrimStart()); } - // Case 4: Unquoted path with spaces — find first .exe boundary - var exeEnd = command.IndexOf(".exe", StringComparison.OrdinalIgnoreCase); - if (exeEnd > 0) + // Case 4: Unquoted path with spaces — find first .exe boundary. + // SEC-M7: Only match ".exe" followed by end-of-string, whitespace, or a + // switch character (/,-). This prevents misparsing paths like + // "C:\dir\app.executable\tool.exe" where an earlier ".exe" substring + // would incorrectly split the path. + var searchStart = 0; + while (searchStart < command.Length) { + var exeEnd = command.IndexOf(".exe", searchStart, StringComparison.OrdinalIgnoreCase); + if (exeEnd < 0) break; + exeEnd += 4; // include ".exe" - var exe = command[..exeEnd].Trim(); - var args = exeEnd < command.Length ? command[exeEnd..].TrimStart() : ""; - return (exe, args); + // Valid boundary: end of string, or followed by whitespace/switch + if (exeEnd >= command.Length || + command[exeEnd] == ' ' || command[exeEnd] == '\t' || + command[exeEnd] == '/' || command[exeEnd] == '-') + { + var exe = command[..exeEnd].Trim(); + var args = exeEnd < command.Length ? command[exeEnd..].TrimStart() : ""; + return (exe, args); + } + // Not a valid boundary — keep searching after this occurrence + searchStart = exeEnd; } - // Fallback: treat entire string as executable - return (command, ""); + // SEC-M7: No fallback — if we can't parse it safely, reject it. + // The old fallback treated the entire string as an executable, which + // could execute arbitrary commands if the string was crafted. + throw new InvalidOperationException( + $"Cannot safely parse uninstall command — no valid executable found: '{command}'"); } } diff --git a/SysManager/SysManager/Services/UpdateService.cs b/SysManager/SysManager/Services/UpdateService.cs index cb9c6cf9..39175d7f 100644 --- a/SysManager/SysManager/Services/UpdateService.cs +++ b/SysManager/SysManager/Services/UpdateService.cs @@ -143,9 +143,35 @@ public async Task> GetRecentAsync(int count = 10, Can Directory.CreateDirectory(dir); var target = Path.Combine(dir, $"SysManager-{rel.Version}.exe"); - // Skip re-download if we already have a good copy. - if (File.Exists(target) && rel.AssetSize.HasValue && new FileInfo(target).Length == rel.AssetSize.Value) - return target; + // SEC-M2: Skip re-download only if we have a cached hash that matches. + // File size alone is insufficient — an attacker could replace the binary + // with a same-size payload. We store a companion .sha256 file after each + // successful download to enable fast cache validation without re-downloading + // the hash from GitHub on every launch. + var hashFile = target + ".sha256"; + if (File.Exists(target) && File.Exists(hashFile)) + { + try + { + var cachedHash = (await File.ReadAllTextAsync(hashFile, ct).ConfigureAwait(false)).Trim(); + if (cachedHash.Length >= 64) + { + var actualHash = await Task.Run(() => + { + using var stream = File.OpenRead(target); + return Convert.ToHexString(SHA256.HashData(stream)); + }, ct).ConfigureAwait(false); + + if (string.Equals(cachedHash, actualHash, StringComparison.OrdinalIgnoreCase)) + return target; + + // Hash mismatch — cached file is corrupt or tampered, re-download. + Serilog.Log.Warning("Cached update binary hash mismatch — re-downloading"); + } + } + catch (IOException) { /* hash file unreadable — re-download */ } + catch (UnauthorizedAccessException) { /* hash file unreadable — re-download */ } + } try { @@ -166,6 +192,19 @@ public async Task> GetRecentAsync(int count = 10, Can progress?.Report((read, total)); } + // 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 */ } + return target; } catch (OperationCanceledException)