diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 157d3ed8..2a7afaea 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -21,3 +21,7 @@ **Vulnerability:** `CustomEmployeeManager.Register` accepted arbitrary employee IDs without validation, which were later used directly in `Path.Combine` to construct image loading paths, enabling path traversal (CWE-22). **Learning:** Identifiers provided by mods or external sources must be treated as untrusted input and validated before being used in file system operations. **Prevention:** Validate input strings that form part of a file path before concatenating them. Reject them if they contain directory traversal characters like `..`, `Path.DirectorySeparatorChar`, `Path.AltDirectorySeparatorChar`, or any invalid filename characters (using `Path.GetInvalidFileNameChars()`). +## 2024-10-31 - Defense-in-depth Path Traversal in UI Fallbacks +**Vulnerability:** Path traversal vulnerability in `CustomEmployeeManager.SetPortrait` via `employeeId` bypassing registration checks during direct method calls or deserialization. +**Learning:** Adding standard early-return security checks to prevent path traversal skips necessary fallback UI state logic later in the method (like setting default colors). +**Prevention:** Wrap vulnerable file-system operations in conditional blocks instead of returning early, guaranteeing the fallback UI state logic is still applied. diff --git a/src/API/CustomEmployeeManager.cs b/src/API/CustomEmployeeManager.cs index 9b4512b6..fd94c406 100644 --- a/src/API/CustomEmployeeManager.cs +++ b/src/API/CustomEmployeeManager.cs @@ -891,10 +891,16 @@ private static void SetPortrait(Transform card, string employeeId) string assetsDir = Path.Combine(MelonEnvironment.UserDataDirectory, "ModAssets"); string? imagePath = null; - foreach (var ext in new[] { ".jpg", ".png" }) + + // SECURITY: Validate employeeId to prevent path traversal attacks during file system operations. + // If invalid, we skip file loading and let the UI fall back to its default state instead of returning early. + if (!string.IsNullOrEmpty(employeeId) && employeeId.IndexOfAny(Path.GetInvalidFileNameChars()) < 0 && !employeeId.Contains("..")) { - string candidate = Path.Combine(assetsDir, employeeId + ext); - if (File.Exists(candidate)) { imagePath = candidate; break; } + foreach (var ext in new[] { ".jpg", ".png" }) + { + string candidate = Path.Combine(assetsDir, employeeId + ext); + if (File.Exists(candidate)) { imagePath = candidate; break; } + } } if (imagePath != null) diff --git a/src/GameLayer/Patches/Networking/CablePositionsPatch.cs b/src/GameLayer/Patches/Networking/CablePositionsPatch.cs index 84c3f50c..91993eda 100644 --- a/src/GameLayer/Patches/Networking/CablePositionsPatch.cs +++ b/src/GameLayer/Patches/Networking/CablePositionsPatch.cs @@ -67,7 +67,13 @@ public static void SetBaseId(int baseId) } while (Interlocked.CompareExchange(ref _nextCableId, baseId + 1, current) != current); - MelonLogger.Msg($"[CablePatch] Cable ID counter set to {baseId + 1}"); + try { LogBaseIdUpdate(baseId + 1); } catch { } + } + + [System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.NoInlining)] + private static void LogBaseIdUpdate(int nextId) + { + MelonLogger.Msg($"[CablePatch] Cable ID counter set to {nextId}"); } public static int PeekNextId() => _nextCableId;