From c355551654a55dd3f5840be75c7b3871678c8e5b Mon Sep 17 00:00:00 2001 From: mleem97 <52848568+mleem97@users.noreply.github.com> Date: Wed, 29 Jul 2026 23:57:21 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20Sentinel:=20[CRITICAL]?= =?UTF-8?q?=20Fix=20Path=20Traversal=20in=20Image=20Loading?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Vulnerability: CustomEmployeeManager.SetPortrait lacked defense-in-depth validation for employeeId when constructing file paths via Path.Combine, allowing Path Traversal (CWE-22) via indirect calls or deserialization bypasses. Impact: Arbitrary file read of local system files by escaping the ModAssets sandbox directory. Fix: Added explicit path traversal character checks (IndexOfAny and Contains("..")) inside SetPortrait before invoking Path.Combine. Verification: Verified using dotnet build and dotnet test. Checked modified method. --- .jules/sentinel.md | 5 +++++ src/API/CustomEmployeeManager.cs | 15 ++++++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 157d3ed8..8b5c7bc4 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -21,3 +21,8 @@ **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-07-29 - Defense-in-depth Path Traversal in Image Loading +**Vulnerability:** `CustomEmployeeManager.SetPortrait` used `employeeId` to construct file paths for loading images using `Path.Combine`. While the `Register` method validated `employeeId`, direct calls or bypasses (like deserialization) could still pass malicious input, enabling path traversal (CWE-22). +**Learning:** Relying solely on validation during object registration is insufficient. Methods performing sensitive operations (like file I/O) must independently validate their inputs, especially when handling string identifiers. +**Prevention:** Implement defense-in-depth by applying path traversal checks (e.g., `IndexOfAny(Path.GetInvalidFileNameChars()) >= 0 || Contains("..")`) directly before file system operations, even if the input was ostensibly validated elsewhere. diff --git a/src/API/CustomEmployeeManager.cs b/src/API/CustomEmployeeManager.cs index 9b4512b6..97c06e65 100644 --- a/src/API/CustomEmployeeManager.cs +++ b/src/API/CustomEmployeeManager.cs @@ -891,10 +891,19 @@ 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: Prevent path traversal attacks by validating employeeId before using it in Path.Combine + if (!string.IsNullOrEmpty(employeeId) && employeeId.IndexOfAny(Path.GetInvalidFileNameChars()) < 0 && !employeeId.Contains("..")) + { + foreach (var ext in new[] { ".jpg", ".png" }) + { + string candidate = Path.Combine(assetsDir, employeeId + ext); + if (File.Exists(candidate)) { imagePath = candidate; break; } + } + } + else { - string candidate = Path.Combine(assetsDir, employeeId + ext); - if (File.Exists(candidate)) { imagePath = candidate; break; } + CrashLog.Log($"[Security] SetPortrait: Invalid characters in employeeId={employeeId}"); } if (imagePath != null)