fix: security hardening batch 2 — cache validation, zip slip, DLL hijack, parser hardening

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

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<InvalidOperationException>(
+            () => 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<InvalidOperationException>(
+            () => 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);
+    }
 }

SysManager/SysManager/Services/DeepCleanupService.cs

@@ -224,7 +224,9 @@ private static IReadOnlyList<CleanupCategory> Scan(IProgress<ScanProgress>? prog
                         }
                         files++;
                     }
-                    catch { }
+                    catch (IOException) { /* file locked or inaccessible — skip */ }
+                    catch (UnauthorizedAccessException) { /* no permission — skip */ }
+                    catch (System.Security.SecurityException) { /* restricted — skip */ }
                 }
             }
 

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

SysManager/SysManager/Services/EventLogService.cs

@@ -52,7 +52,8 @@ private async IAsyncEnumerable<FriendlyEventEntry> 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,15 +97,17 @@ 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
         {
             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)

SysManager/SysManager/Services/PingMonitorService.cs

@@ -134,7 +134,8 @@ private void RaiseSampleReceived(PingSample sample)
         foreach (var h in handlers)
         {
             try { ((Action<PingSample>)h).Invoke(sample); }
-            catch { /* swallow subscriber errors */ }
+            catch (ObjectDisposedException) { /* subscriber disposed — skip */ }
+            catch (InvalidOperationException) { /* subscriber error — skip */ }
         }
     }
 

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.
 ///
-/// <para><b>Security note (SEC-005):</b> 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.</para>
+/// <para><b>Security note (SEC-005 / SEC-M8):</b> 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.</para>
+///
+/// <para><b>SECURITY CONTRACT:</b> 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.</para>
 /// </summary>
 public sealed class PowerShellRunner
 {

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 ""; }

SysManager/SysManager/Services/SpeedTestService.cs

@@ -192,7 +192,12 @@
             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 @@
         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);
 

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); }

SysManager/SysManager/Services/TracerouteService.cs

@@ -103,7 +103,8 @@ private void RaiseHopCompleted(TracerouteHop hop)
         foreach (var h in handlers)
         {
             try { ((Action<TracerouteHop>)h).Invoke(hop); }
-            catch (Exception) { /* swallow subscriber errors to protect iteration */ }
+            catch (ObjectDisposedException) { /* subscriber disposed — skip */ }
+            catch (InvalidOperationException) { /* subscriber error — skip */ }
         }
     }
 }

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}'");
     }
 }

SysManager/SysManager/Services/UpdateService.cs

@@ -143,9 +143,35 @@ public async Task<IReadOnlyList<ReleaseInfo>> 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<IReadOnlyList<ReleaseInfo>> 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)