From 23aca8c82fcc72aa02626ed5afd6e553d3d795b4 Mon Sep 17 00:00:00 2001 From: mleem97 <52848568+mleem97@users.noreply.github.com> Date: Sun, 9 Aug 2026 00:03:27 +0000 Subject: [PATCH] [Security] Fix path traversal vulnerability in employee portrait loading --- .jules/sentinel.md | 5 +++++ src/API/CustomEmployeeManager.cs | 11 ++++++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 157d3ed8..20d2ffb0 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-10-24 - Defense-in-Depth for File Operations +**Vulnerability:** Path traversal vulnerability in `SetPortrait` where `employeeId` was used directly in `Path.Combine` without validation, relying entirely on upstream validation during registration. +**Learning:** Even when inputs like `employeeId` are validated at creation/registration, any subsequent methods that consume them for file system operations must independently validate them to protect against direct calls, missing validations, or deserialization bypasses. Additionally, security checks must not use early returns if fallback logic (like default UI colors) is present later in the method. +**Prevention:** Always validate identifiers used in file paths at the point of use using `IndexOfAny(Path.GetInvalidFileNameChars()) >= 0 || Contains("..")`. Wrap vulnerable operations in conditional blocks rather than returning early to ensure fallback state is correctly applied. diff --git a/src/API/CustomEmployeeManager.cs b/src/API/CustomEmployeeManager.cs index 9b4512b6..a260215b 100644 --- a/src/API/CustomEmployeeManager.cs +++ b/src/API/CustomEmployeeManager.cs @@ -891,10 +891,15 @@ 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 in employee portrait loading + if (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)