Restrict path traversal on FastZip extraction (#235)

Fixes #232

- Prevent traversal outside of extraction directory
- Add new explicit exception for invalid names
- Add tests for extraction path traversal

Note: Use new parameter `allowParentTraversal` to re-enable past behaviour

Author: piksel

src/ICSharpCode.SharpZipLib/Core/InvalidNameException.cs

@@ -0,0 +1,38 @@
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace ICSharpCode.SharpZipLib.Core
+{
+
+	/// <summary>
+	/// InvalidNameException is thrown for invalid names such as directory traversal paths and names with invalid characters
+	/// </summary>
+	public class InvalidNameException: SharpZipBaseException
+    {
+		/// <summary>
+		/// Initializes a new instance of the InvalidNameException class with a default error message.
+		/// </summary>
+		public InvalidNameException(): base("An invalid name was specified")
+		{
+		}
+
+		/// <summary>
+		/// Initializes a new instance of the InvalidNameException class with a specified error message.
+		/// </summary>
+		/// <param name="message">A message describing the exception.</param>
+		public InvalidNameException(string message) : base(message)
+		{
+		}
+
+		/// <summary>
+		/// Initializes a new instance of the InvalidNameException class with a specified
+		/// error message and a reference to the inner exception that is the cause of this exception.
+		/// </summary>
+		/// <param name="message">A message describing the exception.</param>
+		/// <param name="innerException">The inner exception</param>
+		public InvalidNameException(string message, Exception innerException) : base(message, innerException)
+		{
+		}
+	}
+}

src/ICSharpCode.SharpZipLib/Zip/FastZip.cs

@@ -385,12 +385,13 @@ public void ExtractZip(string zipFileName, string targetDirectory, string fileFi
 		/// <param name="fileFilter">A filter to apply to files.</param>
 		/// <param name="directoryFilter">A filter to apply to directories.</param>
 		/// <param name="restoreDateTime">Flag indicating whether to restore the date and time for extracted files.</param>
+		/// <param name="allowParentTraversal">Allow parent directory traversal in file paths (e.g. ../file)</param>
 		public void ExtractZip(string zipFileName, string targetDirectory,
 							   Overwrite overwrite, ConfirmOverwriteDelegate confirmDelegate,
-							   string fileFilter, string directoryFilter, bool restoreDateTime)
+							   string fileFilter, string directoryFilter, bool restoreDateTime, bool allowParentTraversal = false)
 		{
 			Stream inputStream = File.Open(zipFileName, FileMode.Open, FileAccess.Read, FileShare.Read);
-			ExtractZip(inputStream, targetDirectory, overwrite, confirmDelegate, fileFilter, directoryFilter, restoreDateTime, true);
+			ExtractZip(inputStream, targetDirectory, overwrite, confirmDelegate, fileFilter, directoryFilter, restoreDateTime, true, allowParentTraversal);
 		}
 
 		/// <summary>
@@ -404,10 +405,11 @@ public void ExtractZip(string zipFileName, string targetDirectory,
 		/// <param name="directoryFilter">A filter to apply to directories.</param>
 		/// <param name="restoreDateTime">Flag indicating whether to restore the date and time for extracted files.</param>
 		/// <param name="isStreamOwner">Flag indicating whether the inputStream will be closed by this method.</param>
+		/// <param name="allowParentTraversal">Allow parent directory traversal in file paths (e.g. ../file)</param>
 		public void ExtractZip(Stream inputStream, string targetDirectory,
 					   Overwrite overwrite, ConfirmOverwriteDelegate confirmDelegate,
 					   string fileFilter, string directoryFilter, bool restoreDateTime,
-					   bool isStreamOwner)
+					   bool isStreamOwner, bool allowParentTraversal = false)
 		{
 			if ((overwrite == Overwrite.Prompt) && (confirmDelegate == null)) {
 				throw new ArgumentNullException(nameof(confirmDelegate));
@@ -416,7 +418,7 @@ public void ExtractZip(Stream inputStream, string targetDirectory,
 			continueRunning_ = true;
 			overwrite_ = overwrite;
 			confirmDelegate_ = confirmDelegate;
-			extractNameTransform_ = new WindowsNameTransform(targetDirectory);
+			extractNameTransform_ = new WindowsNameTransform(targetDirectory, allowParentTraversal);
 
 			fileFilter_ = new NameFilter(fileFilter);
 			directoryFilter_ = new NameFilter(directoryFilter);

src/ICSharpCode.SharpZipLib/Zip/WindowsNameTransform.cs

@@ -19,6 +19,7 @@ public class WindowsNameTransform : INameTransform
 		string _baseDirectory;
 		bool _trimIncomingPaths;
 		char _replacementChar = '_';
+		private bool _allowParentTraversal;
 
 		/// <summary>
 		/// In this case we need Windows' invalid path characters.
@@ -38,13 +39,11 @@ public class WindowsNameTransform : INameTransform
 		/// Initialises a new instance of <see cref="WindowsNameTransform"/>
 		/// </summary>
 		/// <param name="baseDirectory"></param>
-		public WindowsNameTransform(string baseDirectory)
+		/// <param name="allowParentTraversal">Allow parent directory traversal in file paths (e.g. ../file)</param>
+		public WindowsNameTransform(string baseDirectory, bool allowParentTraversal = false)
 		{
-			if (baseDirectory == null) {
-				throw new ArgumentNullException(nameof(baseDirectory), "Directory name is invalid");
-			}
-
-			BaseDirectory = baseDirectory;
+			BaseDirectory = baseDirectory ?? throw new ArgumentNullException(nameof(baseDirectory), "Directory name is invalid");
+			AllowParentTraversal = allowParentTraversal;
 		}
 
 		/// <summary>
@@ -69,6 +68,15 @@ public string BaseDirectory {
 			}
 		}
 
+		/// <summary>
+		/// Allow parent directory traversal in file paths (e.g. ../file)
+		/// </summary>
+		public bool AllowParentTraversal
+		{
+			get => _allowParentTraversal;
+			set => _allowParentTraversal = value;
+		}
+
 		/// <summary>
 		/// Gets or sets a value indicating wether paths on incoming values should be removed.
 		/// </summary>
@@ -90,7 +98,7 @@ public string TransformDirectory(string name)
 					name = name.Remove(name.Length - 1, 1);
 				}
 			} else {
-				throw new ZipException("Cannot have an empty directory name");
+				throw new InvalidNameException("Cannot have an empty directory name");
 			}
 			return name;
 		}
@@ -113,6 +121,11 @@ public string TransformFile(string name)
 				// Combine will throw a PathTooLongException in that case.
 				if (_baseDirectory != null) {
 					name = Path.Combine(_baseDirectory, name);
+
+					if(!_allowParentTraversal && !Path.GetFullPath(name).StartsWith(_baseDirectory, StringComparison.InvariantCultureIgnoreCase))
+					{
+						throw new InvalidNameException("Parent traversal in paths is not allowed");
+					}
 				}
 			} else {
 				name = string.Empty;

test/ICSharpCode.SharpZipLib.Tests/Zip/FastZipHandling.cs

@@ -1,4 +1,5 @@
-using System.IO;
+using System;
+using System.IO;
 using System.Text.RegularExpressions;
 using ICSharpCode.SharpZipLib.Zip;
 using NUnit.Framework;
@@ -269,5 +270,78 @@ public void NonAsciiPasswords()
 				File.Delete(tempName1);
 			}
 		}
+
+
+		[Test]
+		[Category("Zip")]
+		[Category("CreatesTempFile")]
+		public void LimitExtractPath()
+		{
+			string tempPath = GetTempFilePath();
+			Assert.IsNotNull(tempPath, "No permission to execute this test?");
+
+			var uniqueName = "SharpZipLib.Test_" + DateTime.Now.Ticks.ToString("x");
+
+			tempPath = Path.Combine(tempPath, uniqueName);
+			var extractPath = Path.Combine(tempPath, "output");
+
+			const string contentFile = "content.txt";
+
+			var contentFilePathBad = Path.Combine("..", contentFile);
+			var extractFilePathBad = Path.Combine(tempPath, contentFile);
+			var archiveFileBad = Path.Combine(tempPath, "test-good.zip");
+
+			var contentFilePathGood = Path.Combine("childDir", contentFile);
+			var extractFilePathGood = Path.Combine(extractPath, contentFilePathGood);
+			var archiveFileGood = Path.Combine(tempPath, "test-bad.zip");
+
+			try
+			{
+				Directory.CreateDirectory(extractPath);
+
+				// Create test input
+				void CreateTestFile(string archiveFile, string contentPath)
+				{
+					using (var zf = ZipFile.Create(archiveFile))
+					{
+						zf.BeginUpdate();
+						zf.Add(new StringMemoryDataSource($"Content of {archiveFile}"), contentPath);
+						zf.CommitUpdate();
+					}
+				}
+
+				CreateTestFile(archiveFileGood, contentFilePathGood);
+				CreateTestFile(archiveFileBad, contentFilePathBad);
+
+				Assert.IsTrue(File.Exists(archiveFileGood), "Good test archive was not created");
+				Assert.IsTrue(File.Exists(archiveFileBad), "Bad test archive was not created");
+
+				var fastZip = new FastZip();
+
+				Assert.DoesNotThrow(() => {
+					fastZip.ExtractZip(archiveFileGood, extractPath, "");
+				}, "Threw exception on good file name");
+
+				Assert.IsTrue(File.Exists(extractFilePathGood), "Good output file not created");
+
+				Assert.Throws<SharpZipLib.Core.InvalidNameException>(() => {
+					fastZip.ExtractZip(archiveFileBad, extractPath, "");
+				}, "No exception was thrown for bad file name");
+
+				Assert.IsFalse(File.Exists(extractFilePathBad), "Bad output file created");
+
+				Assert.DoesNotThrow(() => {
+					fastZip.ExtractZip(archiveFileBad, extractPath, FastZip.Overwrite.Never, null, "", "", true, true);
+				}, "Threw exception on bad file name when traversal explicitly allowed");
+
+				Assert.IsTrue(File.Exists(extractFilePathBad), "Bad output file not created when traversal explicitly allowed");
+
+			}
+			finally
+			{
+				Directory.Delete(tempPath, true);
+			}
+		}
+
 	}
 }