From 9dfff8570589659d53d65580d28ba8fdee7d79d2 Mon Sep 17 00:00:00 2001 From: Goober5000 Date: Sun, 10 May 2026 22:15:39 -0400 Subject: [PATCH] Close remaining path-validation gaps in install flow Three gaps in install-time path handling that survived e20a3ed: - mod.id, mod.version, mod.parent, and pkg.folder were concatenated into modPath without validation, BEFORE the existing IsSubPath check. A poisoned id like "..\..\Windows\System32" let every subsequent file.dest pass against the poisoned base. New KnUtils.IsSafePathComponent (rejects separators, "..", null bytes, drive letters, NTFS-stripped trailing dots/spaces); InstallMod and InstallBuild now validate before building modPath. - TryToCopyFilesFromOldVersions called File.Exists, GetFileHash, File.Copy, and HardLink.CreateFileLink on paths built from JSON- supplied oldPkg.folder / package.folder / f.filename with no IsSubPath check. Now guarded on both the read and write sides, with the existing clear-lists-and-break failure pattern. - IsSubPath used OrdinalIgnoreCase unconditionally; on Linux/macOS that let same-name-different-case siblings pass as in-base. Now Ordinal on non-Windows. Co-Authored-By: Claude Opus 4.7 (1M context) --- Knossos.NET/Classes/KnUtils.cs | 32 ++++++++++++++++++- .../Templates/Tasks/InstallBuild.cs | 20 ++++++++++++ .../ViewModels/Templates/Tasks/InstallMod.cs | 21 ++++++++++++ .../Tasks/TryToCopyFilesFromOldVersions.cs | 25 +++++++++++++++ 4 files changed, 97 insertions(+), 1 deletion(-) diff --git a/Knossos.NET/Classes/KnUtils.cs b/Knossos.NET/Classes/KnUtils.cs index e711bda9..b28acc2c 100644 --- a/Knossos.NET/Classes/KnUtils.cs +++ b/Knossos.NET/Classes/KnUtils.cs @@ -656,7 +656,11 @@ public static bool IsSubPath(string basePath, string candidatePath) { var fullBase = Path.GetFullPath(basePath).TrimEnd(Path.DirectorySeparatorChar) + Path.DirectorySeparatorChar; var fullTarget = Path.GetFullPath(Path.Combine(basePath, candidatePath)); - return fullTarget.StartsWith(fullBase, StringComparison.OrdinalIgnoreCase); + //Windows is uniformly case-insensitive; Linux/macOS filesystems CAN be case-sensitive + //(always on Linux, optional on APFS), so use Ordinal there to avoid false-positives on + //traversal attempts that exploit case-only differences with sibling directories. + var comparison = IsWindows ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal; + return fullTarget.StartsWith(fullBase, comparison); } catch { @@ -664,6 +668,32 @@ public static bool IsSubPath(string basePath, string candidatePath) } } + /// + /// True if a string is safe to use as a single filesystem path component (no separators, + /// no traversal sequences, no null bytes, no drive letters, no NTFS-stripped trailing + /// whitespace or dots, not "." or ".."). Used to validate JSON-supplied fields like + /// mod.id / mod.version that get concatenated into install paths. + /// + public static bool IsSafePathComponent(string? component) + { + if (string.IsNullOrEmpty(component)) + return false; + if (component == "." || component == "..") + return false; + if (component.Contains('\0')) + return false; + if (component.Contains('/') || component.Contains('\\')) + return false; + if (component.Length >= 2 && component[1] == ':') + return false; + //NTFS silently strips trailing dots/spaces — reject so ".. " or ".." don't round-trip. + if (component.Trim() != component) + return false; + if (component.EndsWith('.')) + return false; + return true; + } + /// /// Gets the complete size of all files in a folder and subdirectories in bytes /// diff --git a/Knossos.NET/ViewModels/Templates/Tasks/InstallBuild.cs b/Knossos.NET/ViewModels/Templates/Tasks/InstallBuild.cs index 835a1eb1..c3998585 100644 --- a/Knossos.NET/ViewModels/Templates/Tasks/InstallBuild.cs +++ b/Knossos.NET/ViewModels/Templates/Tasks/InstallBuild.cs @@ -138,6 +138,26 @@ private async Task InstallVCRedist(bool is86 = false) { -Main progress max value is calculated as follows: ( Number of files to download * 2 ) + 1 (Download, Decompression, Download banner/tile images) */ + //Reject build metadata with traversal sequences in path-component fields. Without this guard a + //malicious Nebula response (id="..\\..\\..", version="../etc"...) would poison modPath itself, + //causing the IsSubPath checks on file.dest / file.filename below to validate against the poisoned base. + if (!KnUtils.IsSafePathComponent(modJson.id) || + !KnUtils.IsSafePathComponent(modJson.version)) + { + Log.Add(Log.LogSeverity.Error, "TaskItemViewModel.InstallBuild()", "Refusing to install: build has unsafe id/version: id=" + modJson.id + " version=" + modJson.version); + CancelTaskCommand(); + throw new TaskCanceledException(); + } + foreach (var pkg in modJson.packages) + { + if (pkg.folder != null && !KnUtils.IsSafePathComponent(pkg.folder)) + { + Log.Add(Log.LogSeverity.Error, "TaskItemViewModel.InstallBuild()", "Refusing to install: build " + modJson.id + " has unsafe package folder: " + pkg.folder); + CancelTaskCommand(); + throw new TaskCanceledException(); + } + } + List files = new List(); string modFolder = modJson.id + "-" + modJson.version; modPath = Knossos.GetKnossosLibraryPath() + Path.DirectorySeparatorChar + "bin" + Path.DirectorySeparatorChar + modFolder; diff --git a/Knossos.NET/ViewModels/Templates/Tasks/InstallMod.cs b/Knossos.NET/ViewModels/Templates/Tasks/InstallMod.cs index 70988fb1..f74c6703 100644 --- a/Knossos.NET/ViewModels/Templates/Tasks/InstallMod.cs +++ b/Knossos.NET/ViewModels/Templates/Tasks/InstallMod.cs @@ -107,6 +107,27 @@ public async Task InstallMod(Mod mod, CancellationTokenSource cancelSource -If devmode and file is a vp it needs to be decompressed +1 to max tasks */ + //Reject mod metadata with traversal sequences in path-component fields. Without this guard a + //malicious Nebula response (id="..\\..\\..", version="../etc"...) would poison modPath itself, + //causing the IsSubPath checks on file.dest / file.filename below to validate against the poisoned base. + if (!KnUtils.IsSafePathComponent(mod.id) || + !KnUtils.IsSafePathComponent(mod.version) || + (mod.parent != null && !KnUtils.IsSafePathComponent(mod.parent))) + { + Log.Add(Log.LogSeverity.Error, "TaskItemViewModel.InstallMod()", "Refusing to install: mod has unsafe id/version/parent: id=" + mod.id + " version=" + mod.version + " parent=" + mod.parent); + CancelTaskCommand(); + throw new TaskCanceledException(); + } + foreach (var pkg in mod.packages) + { + if (pkg.folder != null && !KnUtils.IsSafePathComponent(pkg.folder)) + { + Log.Add(Log.LogSeverity.Error, "TaskItemViewModel.InstallMod()", "Refusing to install: mod " + mod.id + " has unsafe package folder: " + pkg.folder); + CancelTaskCommand(); + throw new TaskCanceledException(); + } + } + List files = new List(); string modFolder = mod.id + "-" + mod.version; string rootPack = string.Empty; diff --git a/Knossos.NET/ViewModels/Templates/Tasks/TryToCopyFilesFromOldVersions.cs b/Knossos.NET/ViewModels/Templates/Tasks/TryToCopyFilesFromOldVersions.cs index 37e44761..b421fcfe 100644 --- a/Knossos.NET/ViewModels/Templates/Tasks/TryToCopyFilesFromOldVersions.cs +++ b/Knossos.NET/ViewModels/Templates/Tasks/TryToCopyFilesFromOldVersions.cs @@ -113,6 +113,19 @@ public async Task TryToCopyFilesFromOldVersions(Mod mod, List oldVers { if (f.filename != null && (!oldVer.devMode || (oldVer.devMode && oldPkg.folder != null))) { + //Validate the old-side relative path before reading. A malicious mod.json on disk + //could have a traversal in f.filename that leaks file existence via File.Exists and + //timing via GetFileHash even before we attempt to copy. + var oldRelative = oldVer.devMode + ? Path.Combine(oldPkg.folder ?? string.Empty, f.filename) + : f.filename; + if (string.IsNullOrEmpty(oldRelative) || !KnUtils.IsSubPath(oldVer.fullPath, oldRelative)) + { + Log.Add(Log.LogSeverity.Warning, "TaskItemViewModel.TryToCopyFilesFromOldVersions()", "Old version " + oldVer + " has unsafe file path, cannot use as source: " + oldRelative); + copySrcList.Clear(); + copyDstList.Clear(); + break; + } var oldPath = oldVer.devMode ? Path.Combine(oldVer.fullPath, oldPkg.folder!, f.filename) : Path.Combine(oldVer.fullPath, f.filename); if (File.Exists(oldPath)) { @@ -141,6 +154,18 @@ public async Task TryToCopyFilesFromOldVersions(Mod mod, List oldVers } } + //Paired check on the write side: the new mod's package.folder is API-derived + //and would not have been validated yet if mod was loaded from a poisoned source. + var newRelative = mod.devMode + ? Path.Combine(package.folder ?? string.Empty, f.filename) + : f.filename; + if (!KnUtils.IsSubPath(mod.fullPath, newRelative)) + { + Log.Add(Log.LogSeverity.Warning, "TaskItemViewModel.TryToCopyFilesFromOldVersions()", "New mod has unsafe destination path, cannot copy from old version: " + newRelative); + copySrcList.Clear(); + copyDstList.Clear(); + break; + } copySrcList.Add(oldPath); var newPath = mod.devMode ? Path.Combine(mod.fullPath, package.folder!, f.filename) : Path.Combine(mod.fullPath, f.filename); copyDstList.Add(newPath);