diff --git a/test/CMS/DownloadZipSecurityTests.cs b/test/CMS/DownloadZipSecurityTests.cs new file mode 100644 index 000000000..3d9ada507 --- /dev/null +++ b/test/CMS/DownloadZipSecurityTests.cs @@ -0,0 +1,298 @@ +using CmsData = Viper.Areas.CMS.Services.CmsFilePathSafety; + +namespace Viper.test.CMS; + +/// +/// Security tests for the helpers that back CMS.DownloadZip. +/// Covers the filename sanitizer used for the Content-Disposition header +/// and the per-request temp-archive path builder (VPR-138). +/// +public sealed class DownloadZipSecurityTests +{ + #region SanitizeDownloadName + + [Theory] + [InlineData(@"\..\..\evil.zip")] + [InlineData("../../evil.zip")] + [InlineData(@"C:\Windows\Temp\evil.zip")] + [InlineData("../evil")] + public void SanitizeDownloadName_StripsPathSeparatorsAndTraversal(string input) + { + var result = CmsData.SanitizeDownloadName(input); + + Assert.DoesNotContain('\\', result); + Assert.DoesNotContain('/', result); + Assert.DoesNotContain("..", result); + Assert.EndsWith(".zip", result, StringComparison.OrdinalIgnoreCase); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + [InlineData("!!!")] + [InlineData("///")] + [InlineData(@"\\\")] + [InlineData(".")] + [InlineData("..")] + [InlineData("...")] + [InlineData(". .")] + public void SanitizeDownloadName_NullEmptyOrJunk_ReturnsDefault(string? input) + { + var result = CmsData.SanitizeDownloadName(input!); + + Assert.Equal("FileDownload.zip", result); + } + + [Fact] + public void SanitizeDownloadName_NameWithoutExtension_AppendsZip() + { + var result = CmsData.SanitizeDownloadName("export"); + + Assert.Equal("export.zip", result); + } + + [Fact] + public void SanitizeDownloadName_NonZipExtension_ForcesZipSuffix() + { + // Documented choice: preserve the original name and append .zip so the + // response MIME (application/zip) always matches the filename. + var result = CmsData.SanitizeDownloadName("export.exe"); + + Assert.EndsWith(".zip", result, StringComparison.OrdinalIgnoreCase); + Assert.Equal("export.exe.zip", result); + } + + [Fact] + public void SanitizeDownloadName_AlreadyZip_IsPreserved() + { + var result = CmsData.SanitizeDownloadName("monthly-report.zip"); + + Assert.Equal("monthly-report.zip", result); + } + + [Fact] + public void SanitizeDownloadName_CaseInsensitiveZipExtension_IsPreserved() + { + var result = CmsData.SanitizeDownloadName("REPORT.ZIP"); + + Assert.Equal("REPORT.ZIP", result); + } + + [Theory] + [InlineData("CON")] + [InlineData("PRN")] + [InlineData("AUX")] + [InlineData("NUL")] + [InlineData("COM1")] + [InlineData("LPT1")] + [InlineData("con")] + [InlineData("CON.txt")] + public void SanitizeDownloadName_ReservedWindowsDeviceNames_ReturnsDefault(string input) + { + var result = CmsData.SanitizeDownloadName(input); + + Assert.Equal("FileDownload.zip", result); + } + + [Fact] + public void SanitizeDownloadName_AllowsSafeCharacters() + { + var result = CmsData.SanitizeDownloadName("My_File-01 v2.zip"); + + Assert.Equal("My_File-01 v2.zip", result); + } + + #endregion + + #region Regression guard (VPR-138) + + // Before the fix, DownloadZip built the on-disk temp archive path by + // concatenating GetRootFileFolder() + ticks + user-supplied fileName. + // A traversal payload in fileName (e.g. \..\..\evil.zip) escaped the + // CMS root. The fix splits the flow: user input feeds only the + // Content-Disposition name (SanitizeDownloadName); the on-disk path + // is generated from a server-side GUID under a dedicated temp root + // (BuildTempArchivePath). + // + // This test exercises both helpers as DownloadZip wires them and + // asserts the traversal payload cannot influence the on-disk path, + // even if a future refactor reintroduces the old concatenation. + [Theory] + [InlineData(@"\..\..\evil.zip")] + [InlineData("../../evil.zip")] + [InlineData(@"C:\Windows\System32\evil.zip")] + [InlineData(@"..\..\..\..\Windows\evil.zip")] + [InlineData("../../../../etc/passwd")] + public void Regression_Vpr138_TraversalPayload_CannotEscapeTempRoot(string attackPayload) + { + var tempRoot = CreateIsolatedTempRoot(); + try + { + var responseName = CmsData.SanitizeDownloadName(attackPayload); + var tempPath = CmsData.BuildTempArchivePath(tempRoot); + + Assert.DoesNotContain('\\', responseName); + Assert.DoesNotContain('/', responseName); + Assert.DoesNotContain("..", responseName); + Assert.EndsWith(".zip", responseName, StringComparison.OrdinalIgnoreCase); + + var resolvedRoot = Path.GetFullPath(tempRoot); + var resolvedPath = Path.GetFullPath(tempPath); + Assert.StartsWith( + resolvedRoot + Path.DirectorySeparatorChar, + resolvedPath, + StringComparison.OrdinalIgnoreCase); + Assert.DoesNotContain("..", resolvedPath); + } + finally + { + Cleanup(tempRoot); + } + } + + #endregion + + #region SanitizeZipEntryName + + [Theory] + [InlineData(@"..\..\evil.txt", "evil.txt")] + [InlineData("../../evil.txt", "evil.txt")] + [InlineData(@"nested\folder\file.pdf", "file.pdf")] + [InlineData("nested/folder/file.pdf", "file.pdf")] + [InlineData("Annual Report 2024.pdf", "Annual Report 2024.pdf")] + public void SanitizeZipEntryName_StripsPathComponents(string friendlyName, string expected) + { + var result = CmsData.SanitizeZipEntryName(friendlyName, fallback: "fallback.bin"); + + Assert.Equal(expected, result); + Assert.DoesNotContain('\\', result); + Assert.DoesNotContain('/', result); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void SanitizeZipEntryName_EmptyFriendlyName_FallsBackToFilePath(string? friendlyName) + { + var result = CmsData.SanitizeZipEntryName(friendlyName!, fallback: @"C:\storage\real-file.bin"); + + Assert.Equal("real-file.bin", result); + } + + [Theory] + [InlineData("..")] + [InlineData(".")] + [InlineData("dir/..")] + [InlineData(@"nested\.")] + [InlineData(". .")] + public void SanitizeZipEntryName_DotsOnly_FallsBackToFilePath(string friendlyName) + { + var result = CmsData.SanitizeZipEntryName(friendlyName, fallback: @"C:\storage\real-file.bin"); + + Assert.Equal("real-file.bin", result); + } + + #endregion + + #region BuildTempArchivePath + + [Fact] + public void BuildTempArchivePath_ReturnsPathUnderTempRoot() + { + var tempRoot = CreateIsolatedTempRoot(); + try + { + var path = CmsData.BuildTempArchivePath(tempRoot); + + var resolved = Path.GetFullPath(path); + var normalizedRoot = Path.GetFullPath(tempRoot); + + Assert.StartsWith( + normalizedRoot + Path.DirectorySeparatorChar, + resolved, + StringComparison.OrdinalIgnoreCase); + Assert.EndsWith(".zip", resolved, StringComparison.OrdinalIgnoreCase); + } + finally + { + Cleanup(tempRoot); + } + } + + [Fact] + public void BuildTempArchivePath_AcceptsRootWithTrailingSeparator() + { + var tempRoot = CreateIsolatedTempRoot(); + var withTrailing = tempRoot + Path.DirectorySeparatorChar; + try + { + var path = CmsData.BuildTempArchivePath(withTrailing); + + var resolved = Path.GetFullPath(path); + var normalizedRoot = Path.TrimEndingDirectorySeparator(Path.GetFullPath(withTrailing)); + + Assert.StartsWith( + normalizedRoot + Path.DirectorySeparatorChar, + resolved, + StringComparison.OrdinalIgnoreCase); + } + finally + { + Cleanup(tempRoot); + } + } + + [Fact] + public void BuildTempArchivePath_GeneratesUniquePaths_OnSuccessiveCalls() + { + var tempRoot = CreateIsolatedTempRoot(); + try + { + var a = CmsData.BuildTempArchivePath(tempRoot); + var b = CmsData.BuildTempArchivePath(tempRoot); + + Assert.NotEqual(a, b); + } + finally + { + Cleanup(tempRoot); + } + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + public void BuildTempArchivePath_RejectsEmptyRoot(string tempRoot) + { + Assert.Throws(() => CmsData.BuildTempArchivePath(tempRoot)); + } + + [Fact] + public void BuildTempArchivePath_RejectsNullRoot() + { + Assert.Throws(() => CmsData.BuildTempArchivePath(null!)); + } + + #endregion + + #region Helpers + + private static string CreateIsolatedTempRoot() + { + var root = Path.Join(Path.GetTempPath(), "Viper-CMS-Test-" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(root); + return root; + } + + private static void Cleanup(string root) + { + if (Directory.Exists(root)) + { + Directory.Delete(root, recursive: true); + } + } + + #endregion +} diff --git a/web/Areas/CMS/Data/CMS.cs b/web/Areas/CMS/Data/CMS.cs index 6e0aeedc0..886ba8599 100644 --- a/web/Areas/CMS/Data/CMS.cs +++ b/web/Areas/CMS/Data/CMS.cs @@ -6,6 +6,7 @@ using System.Security.Cryptography; using System.Text.Json; using Viper.Areas.CMS.Models; +using Viper.Areas.CMS.Services; using Viper.Classes.SQLContext; using Viper.Classes.Utilities; using Viper.Models; @@ -383,6 +384,9 @@ public static string GetRootFileFolder() } #endregion + #region Download helpers (moved to Viper.Areas.CMS.Services.CmsFilePathSafety) + #endregion + #region public static void ReplaceRootFolder(CMSFile file) /// /// Replace the root folder in a file object, e.g. if the app is on secure-test but the file was added on a dev machine, or vice versa. @@ -442,14 +446,13 @@ public bool CheckFilePermission(CMSFile file) #region public IActionResult DownloadZip(Controller controller, string[] fileGUIDs, string fileName = "FileDownload.zip") public IActionResult DownloadZip(Controller controller, string[] fileGUIDs, string fileName = "FileDownload.zip") { - if (fileGUIDs.Length == 0 && fileName.Length == 0) + if (fileGUIDs.Length == 0) { - ArgumentNullException argumentNullException = new(nameof(fileGUIDs), "Missing fileGUIDs and file name parameters"); - throw argumentNullException; + return controller.BadRequest("Missing fileGUIDs parameter."); } - //only allow good filename characters - fileName = fileName.Replace(@"[^a-zA-Z0-9\.\-_ ]", ""); + var safeDownloadName = CmsFilePathSafety.SanitizeDownloadName(fileName); + List files = new(); AaudUser? currentUser = UserHelper.GetCurrentUser(); @@ -469,45 +472,57 @@ public IActionResult DownloadZip(Controller controller, string[] fileGUIDs, stri } } - // create a temp Zip file and populate it with the files - string tempFileName = CMS.GetRootFileFolder() + @"\" + DateTime.Now.Ticks + fileName; + if (files.Count == 0) + { + return controller.NotFound(); + } + + var tempRoot = CmsFilePathSafety.GetZipTempFolder(); + System.IO.Directory.CreateDirectory(tempRoot); + string tempFileName = CmsFilePathSafety.BuildTempArchivePath(tempRoot); - using (FileStream fs = System.IO.File.Open(tempFileName, FileMode.OpenOrCreate)) + try { - using ZipArchive archive = new(fs, ZipArchiveMode.Update); - foreach (var file in files) + using (FileStream fs = System.IO.File.Open(tempFileName, FileMode.Create)) + using (ZipArchive archive = new(fs, ZipArchiveMode.Create)) { - if (file.Encrypted && !string.IsNullOrEmpty(file.Key)) + foreach (var file in files) { - ZipArchiveEntry fileEntry = archive.CreateEntry(file.FriendlyName); - using StreamWriter writer = new(fileEntry.Open()); - byte[] filebytes = System.IO.File.ReadAllBytes(file.FilePath); - filebytes = DecryptFile(filebytes, file.Key); + var entryName = CmsFilePathSafety.SanitizeZipEntryName(file.FriendlyName, file.FilePath); + + if (file.Encrypted && !string.IsNullOrEmpty(file.Key)) + { + ZipArchiveEntry fileEntry = archive.CreateEntry(entryName); + using var entryStream = fileEntry.Open(); + byte[] filebytes = System.IO.File.ReadAllBytes(file.FilePath); + filebytes = DecryptFile(filebytes, file.Key); - if (filebytes != null) + entryStream.Write(filebytes, 0, filebytes.Length); + } + else { - writer.BaseStream.Write(filebytes, 0, filebytes.Length); + archive.CreateEntryFromFile(file.FilePath, entryName); } } - else + } + + byte[] bytes = System.IO.File.ReadAllBytes(tempFileName); + return controller.File(bytes, MimeTypes["zip"], safeDownloadName); + } + finally + { + // Best-effort cleanup: swallow filesystem exceptions so we don't + // mask an earlier exception from archive creation or the response. + try + { + if (System.IO.File.Exists(tempFileName)) { - archive.CreateEntryFromFile(file.FilePath, file.FriendlyName); + System.IO.File.Delete(tempFileName); } - } + catch (IOException) { /* ignored */ } + catch (UnauthorizedAccessException) { /* ignored */ } } - - // read the temp zip file then delete it - byte[] bytes = System.IO.File.ReadAllBytes(tempFileName); - if (bytes == null) - return controller.NotFound(); - - System.IO.File.Delete(tempFileName); - - string extension = "zip"; - - return controller.File(bytes, MimeTypes[extension.ToLower()], fileName); - } #endregion diff --git a/web/Areas/CMS/Services/CmsFilePathSafety.cs b/web/Areas/CMS/Services/CmsFilePathSafety.cs new file mode 100644 index 000000000..3e35c976e --- /dev/null +++ b/web/Areas/CMS/Services/CmsFilePathSafety.cs @@ -0,0 +1,173 @@ +using System.Text.RegularExpressions; + +namespace Viper.Areas.CMS.Services +{ + /// + /// Path-safety primitives for CMS file download flows (VPR-138). + /// + /// Migration note (CMS-PLAN.md section 11.7): the upcoming + /// IFileStorageService is expected to absorb these primitives. + /// Either delegate to directly or + /// inherit on the new service. + /// + /// Two deliberate surfaces: + /// - (static): current call sites + /// (e.g. legacy CMS.DownloadZip) use static access with + /// no DI plumbing. + /// - + : + /// DI-injectable contract for the new API controllers described + /// in CMS-PLAN.md section 5 (e.g. CMSFileDownloadController + /// at /api/cms/download/zip). + /// + public static class CmsFilePathSafety + { + private const string DefaultDownloadName = "FileDownload.zip"; + + private static readonly HashSet ReservedWindowsNames = new(StringComparer.OrdinalIgnoreCase) + { + "CON", "PRN", "AUX", "NUL", + "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9", + "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9" + }; + + private static readonly Regex SafeFileNameAllowList = new(@"[^a-zA-Z0-9._\- ]", RegexOptions.Compiled); + + /// + /// Returns a filename safe to use in a Content-Disposition response header. + /// Strips path components, applies an allow-list, rejects reserved Windows + /// device names, and guarantees a .zip suffix so the name matches + /// the application/zip MIME type. + /// + public static string SanitizeDownloadName(string userInput) + { + if (string.IsNullOrWhiteSpace(userInput)) + { + return DefaultDownloadName; + } + + // Normalize backslash to forward slash so Path.GetFileName strips + // path components on every OS. Path.GetFileName only honors the + // host OS separator, so a Windows-style "\..\evil" leaks through + // unchanged on Linux runners. + var normalized = userInput.Replace('\\', '/'); + var fileNamePart = Path.GetFileName(normalized); + var filtered = SafeFileNameAllowList.Replace(fileNamePart, string.Empty).Trim(); + + // Dots and spaces survive the allow-list. Names that collapse to + // only those tokens (".", "..") would become "..zip" / "...zip" + // after the suffix step, which is hidden / traversal-like in a + // Content-Disposition header. Treat as junk. + if (filtered.Trim('.', ' ').Length == 0) + { + return DefaultDownloadName; + } + + var stem = filtered; + var dotIndex = stem.IndexOf('.'); + if (dotIndex >= 0) + { + stem = stem[..dotIndex]; + } + if (ReservedWindowsNames.Contains(stem)) + { + return DefaultDownloadName; + } + + if (!filtered.EndsWith(".zip", StringComparison.OrdinalIgnoreCase)) + { + filtered += ".zip"; + } + + return filtered; + } + + /// + /// Builds a per-request temp archive path under + /// using a server-generated GUID, and asserts the resolved path stays + /// inside that root. + /// + public static string BuildTempArchivePath(string tempRoot) + { + if (string.IsNullOrWhiteSpace(tempRoot)) + { + throw new ArgumentException("Temp root must be provided.", nameof(tempRoot)); + } + + // Normalize the root so a caller-provided trailing separator + // (e.g. "C:\\Temp\\") doesn't double up when we append our own. + var resolvedRoot = Path.TrimEndingDirectorySeparator(Path.GetFullPath(tempRoot)); + var candidate = Path.Join(resolvedRoot, Guid.NewGuid().ToString("N") + ".zip"); + var resolved = Path.GetFullPath(candidate); + + if (!resolved.StartsWith(resolvedRoot + Path.DirectorySeparatorChar, StringComparison.OrdinalIgnoreCase)) + { + throw new InvalidOperationException("Generated temp path escaped the configured root."); + } + + return resolved; + } + + /// + /// Dedicated temp directory for ZIP archives. Kept separate from the + /// CMS content root so transient archives never mix with managed + /// content, and never get served through content URLs. + /// + public static string GetZipTempFolder() + { + return Path.Join(Path.GetTempPath(), "Viper-CMS"); + } + + /// + /// Returns a safe ZIP entry name from a stored file's friendly name. + /// Strips any path components so a stored name like + /// ../../evil.txt cannot embed traversal in the archive + /// (defense in depth against ZIP-slip when extracting). + /// + public static string SanitizeZipEntryName(string friendlyName, string fallback) + { + if (!string.IsNullOrWhiteSpace(friendlyName)) + { + var entry = Path.GetFileName(friendlyName.Replace('\\', '/')); + // Reject names that collapse to only dots/spaces — "." and + // ".." are still path tokens inside an archive and would + // leave a ZIP-slip hole in this defense-in-depth layer. + if (!string.IsNullOrWhiteSpace(entry) && entry.Trim('.', ' ').Length > 0) + { + return entry; + } + } + + return Path.GetFileName(fallback.Replace('\\', '/')); + } + } + + /// + /// DI-injectable contract for . New code + /// (e.g. CMS-PLAN.md's CMSFileDownloadController, + /// IFileStorageService) should depend on this interface so the + /// underlying implementation can evolve without touching call sites. + /// + public interface ICmsFilePathSafety + { + string SanitizeDownloadName(string userInput); + string BuildTempArchivePath(string tempRoot); + string GetZipTempFolder(); + string SanitizeZipEntryName(string friendlyName, string fallback); + } + + /// + public sealed class CmsFilePathSafetyService : ICmsFilePathSafety + { + public string SanitizeDownloadName(string userInput) => + CmsFilePathSafety.SanitizeDownloadName(userInput); + + public string BuildTempArchivePath(string tempRoot) => + CmsFilePathSafety.BuildTempArchivePath(tempRoot); + + public string GetZipTempFolder() => + CmsFilePathSafety.GetZipTempFolder(); + + public string SanitizeZipEntryName(string friendlyName, string fallback) => + CmsFilePathSafety.SanitizeZipEntryName(friendlyName, fallback); + } +} diff --git a/web/Program.cs b/web/Program.cs index 5fee3318b..f8fb15fdc 100644 --- a/web/Program.cs +++ b/web/Program.cs @@ -277,7 +277,8 @@ void RegisterDbContext(string connectionStringKey) where TContext : Db "Viper.Areas.ClinicalScheduler.Validators", "Viper.Areas.Students.Services", "Viper.Areas.Curriculum.Services", - "Viper.Areas.Effort.Services" + "Viper.Areas.Effort.Services", + "Viper.Areas.CMS.Services" ) .Where(type => type.Name.EndsWith("Service") || type.Name.EndsWith("Validator"))) .UsingRegistrationStrategy(Scrutor.RegistrationStrategy.Skip)