From 3b29de3954a7e1e21e48bb2380da9bf0bab70535 Mon Sep 17 00:00:00 2001 From: Adam Hathcock Date: Tue, 5 May 2026 16:57:15 +0100 Subject: [PATCH] test clean up and formatting --- .../Archives/IArchiveExtensions.cs | 10 +- .../Archives/IAsyncArchiveExtensions.cs | 11 +- tests/SharpCompress.Test/Security/ZipSlip.cs | 184 +++++++++--------- 3 files changed, 109 insertions(+), 96 deletions(-) diff --git a/src/SharpCompress/Archives/IArchiveExtensions.cs b/src/SharpCompress/Archives/IArchiveExtensions.cs index 9e80c47f..a1ac2226 100644 --- a/src/SharpCompress/Archives/IArchiveExtensions.cs +++ b/src/SharpCompress/Archives/IArchiveExtensions.cs @@ -9,7 +9,6 @@ namespace SharpCompress.Archives; public static class IArchiveExtensions { - /// /// Gets the appropriate StringComparison for path checks based on the file system. /// Windows uses case-insensitive file systems, while Unix-like systems use case-sensitive file systems. @@ -76,12 +75,13 @@ public static class IArchiveExtensions { if (entry.IsDirectory) { - var folder = Path.GetDirectoryName(entry.Key.NotNull("Entry Key is null")) - .NotNull("Directory is null"); - var destdir = Path.GetFullPath(Path.Combine(fullDestinationDirectoryPath, folder)); + .NotNull("Directory is null"); + var destdir = Path.GetFullPath( + Path.Combine(fullDestinationDirectoryPath, folder) + ); - if (!Directory.Exists(destdir) && seenDirectories.Add(destdir)) + if (!Directory.Exists(destdir) && seenDirectories.Add(destdir)) { if (!destdir.StartsWith(fullDestinationDirectoryPath, PathComparison)) { diff --git a/src/SharpCompress/Archives/IAsyncArchiveExtensions.cs b/src/SharpCompress/Archives/IAsyncArchiveExtensions.cs index 5d9ef261..51e8f3f2 100644 --- a/src/SharpCompress/Archives/IAsyncArchiveExtensions.cs +++ b/src/SharpCompress/Archives/IAsyncArchiveExtensions.cs @@ -67,7 +67,8 @@ public static class IAsyncArchiveExtensions IProgress? progress, CancellationToken cancellationToken ) - { var fullDestinationDirectoryPath = Path.GetFullPath(destinationDirectory); + { + var fullDestinationDirectoryPath = Path.GetFullPath(destinationDirectory); options ??= new ExtractionOptions(); //check for trailing slash. @@ -97,10 +98,12 @@ public static class IAsyncArchiveExtensions if (entry.IsDirectory) { var folder = Path.GetDirectoryName(entry.Key.NotNull("Entry Key is null")) - .NotNull("Directory is null"); - var destdir = Path.GetFullPath(Path.Combine(fullDestinationDirectoryPath, folder)); + .NotNull("Directory is null"); + var destdir = Path.GetFullPath( + Path.Combine(fullDestinationDirectoryPath, folder) + ); - if (!Directory.Exists(destdir) && seenDirectories.Add(destdir)) + if (!Directory.Exists(destdir) && seenDirectories.Add(destdir)) { if (!destdir.StartsWith(fullDestinationDirectoryPath, PathComparison)) { diff --git a/tests/SharpCompress.Test/Security/ZipSlip.cs b/tests/SharpCompress.Test/Security/ZipSlip.cs index 0b4bcd3d..251b48d4 100644 --- a/tests/SharpCompress.Test/Security/ZipSlip.cs +++ b/tests/SharpCompress.Test/Security/ZipSlip.cs @@ -11,108 +11,118 @@ using SysZipMode = System.IO.Compression.ZipArchiveMode; namespace SharpCompress.Test.Security; -public class ZipSlip: TestBase +public class ZipSlip : TestBase { + [Fact] + public void RunSync() + { + Console.WriteLine("--- Sync: archive.WriteToDirectory() ---"); + var (extractDir, parentDir) = SetupDirs("sync"); + Directory.CreateDirectory(extractDir); + var archivePath = Path.Combine(parentDir, "malicious.zip"); + + BuildMaliciousZip(archivePath); + + using var archive = ArchiveFactory.OpenArchive(archivePath); + + var ex = Assert.Throws(() => + archive.WriteToDirectory(extractDir, new ExtractionOptions { ExtractFullPath = true }) + ); + ex.Message.Should() + .Contain("Entry is trying to create a directory outside of the destination directory"); + + CheckResults(archivePath, parentDir, extractDir); + } [Fact] -public void RunSync() -{ - Console.WriteLine("--- Sync: archive.WriteToDirectory() ---"); - var (extractDir, parentDir) = SetupDirs("sync"); - Directory.CreateDirectory(extractDir); - var archivePath = Path.Combine(parentDir, "malicious.zip"); - - BuildMaliciousZip(archivePath); - - using var archive = ArchiveFactory.OpenArchive(archivePath); - - var ex = Assert.Throws(() => - archive.WriteToDirectory(extractDir, new ExtractionOptions { ExtractFullPath = true })); - ex.Message.Should().Contain("Entry is trying to create a directory outside of the destination directory"); - - CheckResults(parentDir, extractDir); -} - - [Fact] -public async Task RunAsync() -{ - Console.WriteLine("--- Async: archive.WriteToDirectoryAsync() ---"); - var (extractDir, parentDir) = SetupDirs("async"); - Directory.CreateDirectory(extractDir); - var archivePath = Path.Combine(parentDir, "malicious.zip"); - - BuildMaliciousZip(archivePath); - - var archive = await ArchiveFactory.OpenAsyncArchive(archivePath); - await using (archive) + public async Task RunAsync() { + Console.WriteLine("--- Async: archive.WriteToDirectoryAsync() ---"); + var (extractDir, parentDir) = SetupDirs("async"); + Directory.CreateDirectory(extractDir); + var archivePath = Path.Combine(parentDir, "malicious.zip"); - var ex = await Assert.ThrowsAsync(async () => + BuildMaliciousZip(archivePath); - await archive.WriteToDirectoryAsync(extractDir, - new ExtractionOptions - { - ExtractFullPath = true - })); - ex.Message.Should().Contain("Entry is trying to create a directory outside of the destination directory"); + var archive = await ArchiveFactory.OpenAsyncArchive(archivePath); + await using (archive) + { + var ex = await Assert.ThrowsAsync(async () => + await archive.WriteToDirectoryAsync( + extractDir, + new ExtractionOptions { ExtractFullPath = true } + ) + ); + ex.Message.Should() + .Contain( + "Entry is trying to create a directory outside of the destination directory" + ); + } + + CheckResults(archivePath, parentDir, extractDir); } - CheckResults(parentDir, extractDir); -} - -// Craft a ZIP with malicious directory entries using System.IO.Compression -// so we bypass any SharpCompress write-side normalisation. -static void BuildMaliciousZip(string path) -{ - using var fs = File.Create(path); - using var zip = new SysZip(fs, SysZipMode.Create); - - // 1. Relative traversal: two levels up, then "escaped_relative/" - zip.CreateEntry("../../escaped_relative/"); - - // 2. Absolute Unix path (Path.Combine discards the base when second arg is rooted) - zip.CreateEntry("/tmp/escaped_absolute/"); - - // 3. A legitimate entry for contrast - zip.CreateEntry("safe_subdir/"); -} - -static (string extractDir, string parentDir) SetupDirs(string label) -{ - var parentDir = Path.Combine(TEST_ARCHIVES_PATH, $"sc_poc_{label}_{Path.GetRandomFileName()}"); - Directory.CreateDirectory(parentDir); - var extractDir = Path.Combine(parentDir, "extract_target"); - - Console.WriteLine($" Parent : {parentDir}"); - Console.WriteLine($" Target : {extractDir}"); - return (extractDir, parentDir); -} - -static void CheckResults(string parentDir, string extractDir) -{ - Console.WriteLine(" Directories created after extraction:"); - foreach (var d in Directory.GetDirectories(parentDir, "*", SearchOption.AllDirectories)) + // Craft a ZIP with malicious directory entries using System.IO.Compression + // so we bypass any SharpCompress write-side normalisation. + static void BuildMaliciousZip(string path) { - var relative = Path.GetRelativePath(parentDir, d); - var escaped = !d.StartsWith(extractDir, StringComparison.Ordinal); - Console.WriteLine($" {(escaped ? "[ESCAPED]" : "[ ok ]")} {relative}"); + using var fs = File.Create(path); + using var zip = new SysZip(fs, SysZipMode.Create); + + // 1. Relative traversal: two levels up, then "escaped_relative/" + zip.CreateEntry("../../escaped_relative/"); + + // 2. Absolute Unix path (Path.Combine discards the base when second arg is rooted) + zip.CreateEntry("/tmp/escaped_absolute/"); + + // 3. A legitimate entry for contrast + zip.CreateEntry("safe_subdir/"); } - // Relative traversal "../../escaped_relative/" escapes two levels above extractDir - // (which is parentDir/extract_target), landing in Path.GetTempPath() - var relTarget = Path.GetFullPath(Path.Combine(extractDir, "../../escaped_relative")); - if (Directory.Exists(relTarget)) + static (string extractDir, string parentDir) SetupDirs(string label) { - Console.WriteLine($" [ESCAPED] relative traversal created: {relTarget}"); - Directory.Delete(relTarget); + var parentDir = Path.Combine( + TEST_ARCHIVES_PATH, + $"sc_poc_{label}_{Path.GetRandomFileName()}" + ); + Directory.CreateDirectory(parentDir); + var extractDir = Path.Combine(parentDir, "extract_target"); + + Console.WriteLine($" Parent : {parentDir}"); + Console.WriteLine($" Target : {extractDir}"); + return (extractDir, parentDir); } - var absTarget = "/tmp/escaped_absolute"; - if (Directory.Exists(absTarget)) + static void CheckResults(string archivePath, string parentDir, string extractDir) { - Console.WriteLine($" [ESCAPED] absolute path created: {absTarget}"); - Directory.Delete(absTarget); + Console.WriteLine(" Directories created after extraction:"); + foreach (var d in Directory.GetDirectories(parentDir, "*", SearchOption.AllDirectories)) + { + var relative = Path.GetRelativePath(parentDir, d); + var escaped = !d.StartsWith(extractDir, StringComparison.Ordinal); + Console.WriteLine($" {(escaped ? "[ESCAPED]" : "[ ok ]")} {relative}"); + } + + // Relative traversal "../../escaped_relative/" escapes two levels above extractDir + // (which is parentDir/extract_target), landing in Path.GetTempPath() + var relTarget = Path.GetFullPath(Path.Combine(extractDir, "../../escaped_relative")); + if (Directory.Exists(relTarget)) + { + Console.WriteLine($" [ESCAPED] relative traversal created: {relTarget}"); + Directory.Delete(relTarget); + } + File.Delete(archivePath); + if (Directory.Exists(extractDir)) + { + Directory.Delete(extractDir); + } + + var absTarget = "/tmp/escaped_absolute"; + if (Directory.Exists(absTarget)) + { + Console.WriteLine($" [ESCAPED] absolute path created: {absTarget}"); + Directory.Delete(absTarget); + } } } -} #endif