diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 157d3ed8..a52fdb9a 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-05-24 - Path Traversal in Mod Entity Registration Defense-in-Depth +**Vulnerability:** `CustomEmployeeManager.SetPortrait` used `employeeId` to construct image loading paths without validation, relying entirely on upstream validation during registration. +**Learning:** Relying solely on upstream validation (e.g. during object registration) leaves methods vulnerable to direct calls or alternative execution paths that bypass those checks, breaking the defense-in-depth principle. +**Prevention:** Every method performing sensitive file system operations (like `Path.Combine` or `File.Exists`) must independently validate dynamic input against path traversal attacks (`IndexOfAny(Path.GetInvalidFileNameChars()) >= 0 || Contains("..")`), even if the input is supposedly validated elsewhere. diff --git a/src/API/CustomEmployeeManager.cs b/src/API/CustomEmployeeManager.cs index 9b4512b6..abe87f8d 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 in case employeeId bypasses registration checks (defense in depth) + 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)