From bd0fa73cce7521874eec88b861f177735d66decc Mon Sep 17 00:00:00 2001 From: Adam Hathcock Date: Tue, 5 May 2026 17:22:53 +0100 Subject: [PATCH] some refactoring --- .../Archives/IArchiveExtensions.cs | 51 ++-- .../Archives/IAsyncArchiveExtensions.cs | 61 ++--- .../Common/DirectoryManagement.cs | 81 ++++++ .../Common/ExtractionMethods.Async.cs | 92 ++++--- src/SharpCompress/Common/ExtractionMethods.cs | 103 ++++---- .../Security/ExtractionPathTraversalTests.cs | 238 ++++++++++++++++++ 6 files changed, 456 insertions(+), 170 deletions(-) create mode 100644 src/SharpCompress/Common/DirectoryManagement.cs create mode 100644 tests/SharpCompress.Test/Security/ExtractionPathTraversalTests.cs diff --git a/src/SharpCompress/Archives/IArchiveExtensions.cs b/src/SharpCompress/Archives/IArchiveExtensions.cs index 17a87962..0417cdd7 100644 --- a/src/SharpCompress/Archives/IArchiveExtensions.cs +++ b/src/SharpCompress/Archives/IArchiveExtensions.cs @@ -1,6 +1,4 @@ using System; -using System.Collections.Generic; -using System.IO; using SharpCompress.Common; using SharpCompress.Readers; @@ -39,54 +37,33 @@ public static class IArchiveExtensions IProgress? progress ) { - var fullDestinationDirectoryPath = Path.GetFullPath(destinationDirectory); options ??= new ExtractionOptions(); - - //check for trailing slash. - if ( - fullDestinationDirectoryPath[fullDestinationDirectoryPath.Length - 1] - != Path.DirectorySeparatorChar - ) - { - fullDestinationDirectoryPath += Path.DirectorySeparatorChar; - } - - if (!Directory.Exists(fullDestinationDirectoryPath)) - { - throw new ExtractionException( - $"Directory does not exist to extract to: {fullDestinationDirectoryPath}" - ); - } + var fullDestinationDirectoryPath = DirectoryManagement.GetFullDestinationDirectoryPath( + destinationDirectory + ); var totalBytes = archive.TotalUncompressedSize; var bytesRead = 0L; - var seenDirectories = new HashSet(); foreach (var entry in archive.Entries) { 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) + ExtractionMethods.WriteEntryToDirectoryCore( + entry, + fullDestinationDirectoryPath, + options, + _ => { } ); - - if (!Directory.Exists(destdir) && seenDirectories.Add(destdir)) - { - if (!destdir.StartsWith(fullDestinationDirectoryPath, Utility.PathComparison)) - { - throw new ExtractionException( - "Entry is trying to create a directory outside of the destination directory." - ); - } - - Directory.CreateDirectory(destdir); - } continue; } - entry.WriteToDirectory(destinationDirectory, options); + ExtractionMethods.WriteEntryToDirectoryCore( + entry, + fullDestinationDirectoryPath, + options, + path => entry.WriteToFile(path, options) + ); bytesRead += entry.Size; progress?.Report( diff --git a/src/SharpCompress/Archives/IAsyncArchiveExtensions.cs b/src/SharpCompress/Archives/IAsyncArchiveExtensions.cs index 26facb6b..3d9db3f2 100644 --- a/src/SharpCompress/Archives/IAsyncArchiveExtensions.cs +++ b/src/SharpCompress/Archives/IAsyncArchiveExtensions.cs @@ -1,6 +1,4 @@ using System; -using System.Collections.Generic; -using System.IO; using System.Threading; using System.Threading.Tasks; using SharpCompress.Common; @@ -15,7 +13,6 @@ public static class IAsyncArchiveExtensions /// /// Extract to specific directory asynchronously with progress reporting and cancellation support /// - /// The archive to extract. /// The folder to extract into. /// Extraction options. /// Optional progress reporter for tracking extraction progress. @@ -59,28 +56,13 @@ public static class IAsyncArchiveExtensions CancellationToken cancellationToken ) { - var fullDestinationDirectoryPath = Path.GetFullPath(destinationDirectory); options ??= new ExtractionOptions(); - - //check for trailing slash. - if ( - fullDestinationDirectoryPath[fullDestinationDirectoryPath.Length - 1] - != Path.DirectorySeparatorChar - ) - { - fullDestinationDirectoryPath += Path.DirectorySeparatorChar; - } - - if (!Directory.Exists(fullDestinationDirectoryPath)) - { - throw new ExtractionException( - $"Directory does not exist to extract to: {fullDestinationDirectoryPath}" - ); - } + var fullDestinationDirectoryPath = DirectoryManagement.GetFullDestinationDirectoryPath( + destinationDirectory + ); var totalBytes = await archive.TotalUncompressedSizeAsync().ConfigureAwait(false); var bytesRead = 0L; - var seenDirectories = new HashSet(); await foreach (var entry in archive.EntriesAsync.WithCancellation(cancellationToken)) { @@ -88,28 +70,27 @@ 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) - ); - - if (!Directory.Exists(destdir) && seenDirectories.Add(destdir)) - { - if (!destdir.StartsWith(fullDestinationDirectoryPath, Utility.PathComparison)) - { - throw new ExtractionException( - "Entry is trying to create a directory outside of the destination directory." - ); - } - - Directory.CreateDirectory(destdir); - } + await ExtractionMethods + .WriteEntryToDirectoryAsyncCore( + entry, + fullDestinationDirectoryPath, + options, + (_, _) => new ValueTask(), + cancellationToken + ) + .ConfigureAwait(false); continue; } - await entry - .WriteToDirectoryAsync(destinationDirectory, options, cancellationToken) + await ExtractionMethods + .WriteEntryToDirectoryAsyncCore( + entry, + fullDestinationDirectoryPath, + options, + async (path, ct) => + await entry.WriteToFileAsync(path, options, ct).ConfigureAwait(false), + cancellationToken + ) .ConfigureAwait(false); bytesRead += entry.Size; diff --git a/src/SharpCompress/Common/DirectoryManagement.cs b/src/SharpCompress/Common/DirectoryManagement.cs new file mode 100644 index 00000000..b4574018 --- /dev/null +++ b/src/SharpCompress/Common/DirectoryManagement.cs @@ -0,0 +1,81 @@ +using System.IO; + +namespace SharpCompress.Common; + +internal static class DirectoryManagement +{ + + internal const string CreateDirectoryOutsideDestinationMessage = + "Entry is trying to create a directory outside of the destination directory."; + internal const string WriteFileOutsideDestinationMessage = + "Entry is trying to write a file outside of the destination directory."; + + internal static string GetFullDestinationDirectoryPath(string destinationDirectory) + { + var fullDestinationDirectoryPath = Path.GetFullPath(destinationDirectory); + + // Keep the trailing separator so prefix checks cannot match sibling directories. + if ( + !IsDirectorySeparator( + fullDestinationDirectoryPath[fullDestinationDirectoryPath.Length - 1] + ) + ) + { + fullDestinationDirectoryPath += Path.DirectorySeparatorChar; + } + + if (!Directory.Exists(fullDestinationDirectoryPath)) + { + throw new ExtractionException( + $"Directory does not exist to extract to: {fullDestinationDirectoryPath}" + ); + } + + return fullDestinationDirectoryPath; + } + + internal static void EnsurePathInDestinationDirectory( + string destinationPath, + string fullDestinationDirectoryPath, + string exceptionMessage + ) + { + if (destinationPath.StartsWith(fullDestinationDirectoryPath, Utility.PathComparison)) + { + return; + } + + if ( + string.Equals( + destinationPath, + TrimTrailingDirectorySeparators(fullDestinationDirectoryPath), + Utility.PathComparison + ) + ) + { + return; + } + + throw new ExtractionException(exceptionMessage); + } + + + + private static bool IsDirectorySeparator(char value) => + value == Path.DirectorySeparatorChar || value == Path.AltDirectorySeparatorChar; + + private static string TrimTrailingDirectorySeparators(string path) + { + var root = Path.GetPathRoot(path); + var rootLength = root?.Length ?? 0; + var end = path.Length; + + while (end > rootLength && IsDirectorySeparator(path[end - 1])) + { + end--; + } + + return end == path.Length ? path : path.Substring(0, end); + } + +} diff --git a/src/SharpCompress/Common/ExtractionMethods.Async.cs b/src/SharpCompress/Common/ExtractionMethods.Async.cs index 86347310..b9c913cd 100644 --- a/src/SharpCompress/Common/ExtractionMethods.Async.cs +++ b/src/SharpCompress/Common/ExtractionMethods.Async.cs @@ -15,67 +15,59 @@ internal static partial class ExtractionMethods CancellationToken cancellationToken = default ) { - string destinationFileName; - var fullDestinationDirectoryPath = Path.GetFullPath(destinationDirectory); options ??= new ExtractionOptions(); + var fullDestinationDirectoryPath = DirectoryManagement.GetFullDestinationDirectoryPath(destinationDirectory); - //check for trailing slash. - if ( - fullDestinationDirectoryPath[fullDestinationDirectoryPath.Length - 1] - != Path.DirectorySeparatorChar - ) - { - fullDestinationDirectoryPath += Path.DirectorySeparatorChar; - } + await WriteEntryToDirectoryAsyncCore( + entry, + fullDestinationDirectoryPath, + options, + writeAsync, + cancellationToken + ) + .ConfigureAwait(false); + } - if (!Directory.Exists(fullDestinationDirectoryPath)) - { - throw new ExtractionException( - $"Directory does not exist to extract to: {fullDestinationDirectoryPath}" - ); - } - - var file = Path.GetFileName(entry.Key.NotNull("Entry Key is null")).NotNull("File is null"); - file = Utility.ReplaceInvalidFileNameChars(file); - if (options.ExtractFullPath) - { - var folder = Path.GetDirectoryName(entry.Key.NotNull("Entry Key is null")) - .NotNull("Directory is null"); - var destdir = Path.GetFullPath(Path.Combine(fullDestinationDirectoryPath, folder)); - - if (!Directory.Exists(destdir)) - { - if (!destdir.StartsWith(fullDestinationDirectoryPath, Utility.PathComparison)) - { - throw new ExtractionException( - "Entry is trying to create a directory outside of the destination directory." - ); - } - - Directory.CreateDirectory(destdir); - } - destinationFileName = Path.Combine(destdir, file); - } - else - { - destinationFileName = Path.Combine(fullDestinationDirectoryPath, file); - } + internal static async ValueTask WriteEntryToDirectoryAsyncCore( + IEntry entry, + string fullDestinationDirectoryPath, + ExtractionOptions options, + Func writeAsync, + CancellationToken cancellationToken = default + ) + { + var destinationFileName = GetEntryDestinationFileName( + entry, + fullDestinationDirectoryPath, + options + ); if (!entry.IsDirectory) { destinationFileName = Path.GetFullPath(destinationFileName); - if (!destinationFileName.StartsWith(fullDestinationDirectoryPath, Utility.PathComparison)) - { - throw new ExtractionException( - "Entry is trying to write a file outside of the destination directory." - ); - } + DirectoryManagement.EnsurePathInDestinationDirectory( + destinationFileName, + fullDestinationDirectoryPath, + DirectoryManagement.WriteFileOutsideDestinationMessage + ); + await writeAsync(destinationFileName, cancellationToken).ConfigureAwait(false); } - else if (options.ExtractFullPath && !Directory.Exists(destinationFileName)) + else if (options.ExtractFullPath) { - Directory.CreateDirectory(destinationFileName); + destinationFileName = Path.GetFullPath(destinationFileName); + + DirectoryManagement.EnsurePathInDestinationDirectory( + destinationFileName, + fullDestinationDirectoryPath, + DirectoryManagement.CreateDirectoryOutsideDestinationMessage + ); + + if (!Directory.Exists(destinationFileName)) + { + Directory.CreateDirectory(destinationFileName); + } } } diff --git a/src/SharpCompress/Common/ExtractionMethods.cs b/src/SharpCompress/Common/ExtractionMethods.cs index bde397d4..66172b36 100644 --- a/src/SharpCompress/Common/ExtractionMethods.cs +++ b/src/SharpCompress/Common/ExtractionMethods.cs @@ -15,70 +15,87 @@ internal static partial class ExtractionMethods Action write ) { - string destinationFileName; - var fullDestinationDirectoryPath = Path.GetFullPath(destinationDirectory); options ??= new ExtractionOptions(); + var fullDestinationDirectoryPath = DirectoryManagement.GetFullDestinationDirectoryPath(destinationDirectory); - //check for trailing slash. - if ( - fullDestinationDirectoryPath[fullDestinationDirectoryPath.Length - 1] - != Path.DirectorySeparatorChar - ) - { - fullDestinationDirectoryPath += Path.DirectorySeparatorChar; - } + WriteEntryToDirectoryCore(entry, fullDestinationDirectoryPath, options, write); + } - if (!Directory.Exists(fullDestinationDirectoryPath)) + internal static void WriteEntryToDirectoryCore( + IEntry entry, + string fullDestinationDirectoryPath, + ExtractionOptions options, + Action write + ) + { + var destinationFileName = GetEntryDestinationFileName( + entry, + fullDestinationDirectoryPath, + options + ); + + if (!entry.IsDirectory) { - throw new ExtractionException( - $"Directory does not exist to extract to: {fullDestinationDirectoryPath}" + destinationFileName = Path.GetFullPath(destinationFileName); + + DirectoryManagement.EnsurePathInDestinationDirectory( + destinationFileName, + fullDestinationDirectoryPath, + DirectoryManagement.WriteFileOutsideDestinationMessage ); - } + write(destinationFileName); + } + else if (options.ExtractFullPath) + { + destinationFileName = Path.GetFullPath(destinationFileName); + + DirectoryManagement.EnsurePathInDestinationDirectory( + destinationFileName, + fullDestinationDirectoryPath, + DirectoryManagement.CreateDirectoryOutsideDestinationMessage + ); + + if (!Directory.Exists(destinationFileName)) + { + Directory.CreateDirectory(destinationFileName); + } + } + } + + private static string GetEntryDestinationFileName( + IEntry entry, + string fullDestinationDirectoryPath, + ExtractionOptions options + ) + { var file = Path.GetFileName(entry.Key.NotNull("Entry Key is null")).NotNull("File is null"); file = Utility.ReplaceInvalidFileNameChars(file); + if (options.ExtractFullPath) { var folder = Path.GetDirectoryName(entry.Key.NotNull("Entry Key is null")) .NotNull("Directory is null"); var destdir = Path.GetFullPath(Path.Combine(fullDestinationDirectoryPath, folder)); + DirectoryManagement.EnsurePathInDestinationDirectory( + destdir, + fullDestinationDirectoryPath, + entry.IsDirectory + ? DirectoryManagement.CreateDirectoryOutsideDestinationMessage + : DirectoryManagement.WriteFileOutsideDestinationMessage + ); + if (!Directory.Exists(destdir)) { - if (!destdir.StartsWith(fullDestinationDirectoryPath, Utility.PathComparison)) - { - throw new ExtractionException( - "Entry is trying to create a directory outside of the destination directory." - ); - } - Directory.CreateDirectory(destdir); } - destinationFileName = Path.Combine(destdir, file); - } - else - { - destinationFileName = Path.Combine(fullDestinationDirectoryPath, file); + + return Path.Combine(destdir, file); } - if (!entry.IsDirectory) - { - destinationFileName = Path.GetFullPath(destinationFileName); - - if (!destinationFileName.StartsWith(fullDestinationDirectoryPath, Utility.PathComparison)) - { - throw new ExtractionException( - "Entry is trying to write a file outside of the destination directory." - ); - } - write(destinationFileName); - } - else if (options.ExtractFullPath && !Directory.Exists(destinationFileName)) - { - Directory.CreateDirectory(destinationFileName); - } + return Path.Combine(fullDestinationDirectoryPath, file); } - public static void WriteEntryToFile( IEntry entry, string destinationFileName, diff --git a/tests/SharpCompress.Test/Security/ExtractionPathTraversalTests.cs b/tests/SharpCompress.Test/Security/ExtractionPathTraversalTests.cs new file mode 100644 index 00000000..69ebddd8 --- /dev/null +++ b/tests/SharpCompress.Test/Security/ExtractionPathTraversalTests.cs @@ -0,0 +1,238 @@ +#if NET8_0_OR_GREATER +using System; +using System.IO; +using System.Linq; +using System.Threading.Tasks; +using SharpCompress.Archives; +using SharpCompress.Common; +using SharpCompress.Readers; +using Xunit; +using SysZip = System.IO.Compression.ZipArchive; +using SysZipMode = System.IO.Compression.ZipArchiveMode; + +namespace SharpCompress.Test.Security; + +public class ExtractionPathTraversalTests : TestBase +{ + [Theory] + [InlineData("ReaderAll")] + [InlineData("ReaderEntry")] + [InlineData("Archive")] + [InlineData("ArchiveEntry")] + [InlineData("AsyncReaderAll")] + [InlineData("AsyncReaderEntry")] + [InlineData("AsyncArchive")] + [InlineData("AsyncArchiveEntry")] + public async Task DirectoryTraversalToExistingOutsideDirectory_ShouldThrow(string api) + { + var extractDir = Path.Combine(SCRATCH_FILES_PATH, "extract"); + Directory.CreateDirectory(extractDir); + var escapedDirectory = Path.GetFullPath(Path.Combine(extractDir, "../../escaped_existing")); + Directory.CreateDirectory(escapedDirectory); + var archivePath = Path.Combine(SCRATCH2_FILES_PATH, $"{api}.zip"); + BuildZip(archivePath, "../../escaped_existing/"); + + var exception = await RecordExtractionExceptionAsync(api, archivePath, extractDir); + + var extractionException = Assert.IsType(exception); + Assert.Contains("outside of the destination", extractionException.Message); + } + + [Theory] + [InlineData("ReaderAll")] + [InlineData("ReaderEntry")] + [InlineData("Archive")] + [InlineData("ArchiveEntry")] + [InlineData("AsyncReaderAll")] + [InlineData("AsyncReaderEntry")] + [InlineData("AsyncArchive")] + [InlineData("AsyncArchiveEntry")] + public async Task FileTraversalToSiblingDirectory_ShouldThrow(string api) + { + var extractDir = Path.Combine(SCRATCH_FILES_PATH, "extract"); + Directory.CreateDirectory(extractDir); + var siblingDirectory = Path.Combine(SCRATCH_FILES_PATH, "extract2"); + Directory.CreateDirectory(siblingDirectory); + var archivePath = Path.Combine(SCRATCH2_FILES_PATH, $"{api}.zip"); + BuildZip(archivePath, "../extract2/evil.txt"); + + var exception = await RecordExtractionExceptionAsync(api, archivePath, extractDir); + + var extractionException = Assert.IsType(exception); + Assert.Contains("outside of the destination", extractionException.Message); + Assert.False(File.Exists(Path.Combine(siblingDirectory, "evil.txt"))); + } + + private static void BuildZip(string path, string entryName) + { + using var fs = File.Create(path); + using var zip = new SysZip(fs, SysZipMode.Create); + var entry = zip.CreateEntry(entryName); + + if (entryName.EndsWith('/')) + { + return; + } + + using var writer = new StreamWriter(entry.Open()); + writer.Write("evil"); + } + + private static async Task RecordExtractionExceptionAsync( + string api, + string archivePath, + string extractDir + ) + { + var options = new ExtractionOptions { ExtractFullPath = true, Overwrite = true }; + + return api switch + { + "ReaderAll" => RecordException(() => + ExtractWithReaderAll(archivePath, extractDir, options) + ), + "ReaderEntry" => RecordException(() => + ExtractWithReaderEntry(archivePath, extractDir, options) + ), + "Archive" => RecordException(() => + ExtractWithArchive(archivePath, extractDir, options) + ), + "ArchiveEntry" => RecordException(() => + ExtractWithArchiveEntry(archivePath, extractDir, options) + ), + "AsyncReaderAll" => await RecordExceptionAsync(() => + ExtractWithAsyncReaderAllAsync(archivePath, extractDir, options) + ), + "AsyncReaderEntry" => await RecordExceptionAsync(() => + ExtractWithAsyncReaderEntryAsync(archivePath, extractDir, options) + ), + "AsyncArchive" => await RecordExceptionAsync(() => + ExtractWithAsyncArchiveAsync(archivePath, extractDir, options) + ), + "AsyncArchiveEntry" => await RecordExceptionAsync(() => + ExtractWithAsyncArchiveEntryAsync(archivePath, extractDir, options) + ), + _ => throw new ArgumentOutOfRangeException(nameof(api), api, null), + }; + } + + private static Exception? RecordException(Action action) + { + try + { + action(); + return null; + } + catch (Exception exception) + { + return exception; + } + } + + private static async Task RecordExceptionAsync(Func action) + { + try + { + await action(); + return null; + } + catch (Exception exception) + { + return exception; + } + } + + private static void ExtractWithReaderAll( + string archivePath, + string extractDir, + ExtractionOptions options + ) + { + using var stream = File.OpenRead(archivePath); + using var reader = ReaderFactory.OpenReader(stream); + reader.WriteAllToDirectory(extractDir, options); + } + + private static void ExtractWithReaderEntry( + string archivePath, + string extractDir, + ExtractionOptions options + ) + { + using var stream = File.OpenRead(archivePath); + using var reader = ReaderFactory.OpenReader(stream); + Assert.True(reader.MoveToNextEntry()); + reader.WriteEntryToDirectory(extractDir, options); + } + + private static void ExtractWithArchive( + string archivePath, + string extractDir, + ExtractionOptions options + ) + { + using var archive = ArchiveFactory.OpenArchive(archivePath); + archive.WriteToDirectory(extractDir, options); + } + + private static void ExtractWithArchiveEntry( + string archivePath, + string extractDir, + ExtractionOptions options + ) + { + using var archive = ArchiveFactory.OpenArchive(archivePath); + archive.Entries.Single().WriteToDirectory(extractDir, options); + } + + private static async Task ExtractWithAsyncReaderAllAsync( + string archivePath, + string extractDir, + ExtractionOptions options + ) + { + using var stream = File.OpenRead(archivePath); + await using var reader = await ReaderFactory.OpenAsyncReader(stream); + await reader.WriteAllToDirectoryAsync(extractDir, options); + } + + private static async Task ExtractWithAsyncReaderEntryAsync( + string archivePath, + string extractDir, + ExtractionOptions options + ) + { + using var stream = File.OpenRead(archivePath); + await using var reader = await ReaderFactory.OpenAsyncReader(stream); + Assert.True(await reader.MoveToNextEntryAsync()); + await reader.WriteEntryToDirectoryAsync(extractDir, options); + } + + private static async Task ExtractWithAsyncArchiveAsync( + string archivePath, + string extractDir, + ExtractionOptions options + ) + { + await using var archive = await ArchiveFactory.OpenAsyncArchive(archivePath); + await archive.WriteToDirectoryAsync(extractDir, options); + } + + private static async Task ExtractWithAsyncArchiveEntryAsync( + string archivePath, + string extractDir, + ExtractionOptions options + ) + { + await using var archive = await ArchiveFactory.OpenAsyncArchive(archivePath); + + await foreach (var entry in archive.EntriesAsync) + { + await entry.WriteToDirectoryAsync(extractDir, options); + return; + } + + throw new InvalidOperationException("Archive did not contain an entry."); + } +} +#endif