Skip to content

Commit 105ce99

Browse files
gfsgfs
committed
Improve ZipSlip sanitization and output-dir checks
Strengthen path sanitization and guard extraction from directory escape. FileEntry.ZipSlipSanitize was made internal and enhanced to normalize separators, strip Windows drive roots and leading separators, remove ".." sequences, and collapse double separators. Extractor now resolves full target paths and ensures they start with the resolved output directory (sync, parallel, and async extraction), skipping and logging entries that would escape. Added comprehensive unit tests (SanitizePathTests) covering absolute paths, drive roots, traversal, combined attacks, separator collapsing, and FileEntry behavior.
1 parent 38439c7 commit 105ce99

File tree

3 files changed

+180
-9
lines changed

3 files changed

+180
-9
lines changed

‎RecursiveExtractor.Tests/SanitizePathTests.cs‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,131 @@ public void TestSanitizePathLinux(string linuxInputPath, string expectedLinuxPat
3535
}
3636
}
3737

38+
/// <summary>
39+
/// ZipSlipSanitize should strip leading slashes to prevent absolute path traversal.
40+
/// On any OS, a Unix-style absolute path like "/etc/passwd" must become relative.
41+
/// </summary>
42+
[Theory]
43+
[InlineData("/etc/passwd", "etc/passwd")]
44+
[InlineData("/tmp/evil.txt", "tmp/evil.txt")]
45+
[InlineData("///triple/leading", "triple/leading")]
46+
[InlineData("/", "")]
47+
public void TestZipSlipSanitize_AbsoluteUnixPaths(string input, string expected)
48+
{
49+
// Normalize expected to the current OS separator
50+
expected = expected.Replace('/', Path.DirectorySeparatorChar);
51+
var result = FileEntry.ZipSlipSanitize(input);
52+
Assert.Equal(expected, result);
53+
}
54+
55+
/// <summary>
56+
/// ZipSlipSanitize should strip Windows drive letter roots.
57+
/// </summary>
58+
[Theory]
59+
[InlineData("C:\\Windows\\System32\\evil.dll", "Windows/System32/evil.dll")]
60+
[InlineData("D:/data/file.txt", "data/file.txt")]
61+
[InlineData("C:\\", "")]
62+
[InlineData("C:/", "")]
63+
public void TestZipSlipSanitize_AbsoluteWindowsPaths(string input, string expected)
64+
{
65+
expected = expected.Replace('/', Path.DirectorySeparatorChar);
66+
var result = FileEntry.ZipSlipSanitize(input);
67+
Assert.Equal(expected, result);
68+
}
69+
70+
/// <summary>
71+
/// ZipSlipSanitize should strip ".." directory traversal components.
72+
/// </summary>
73+
[Theory]
74+
[InlineData("foo/../../../etc/passwd", "foo/etc/passwd")]
75+
[InlineData("../secret.txt", "secret.txt")]
76+
[InlineData("a/b/../../c", "a/b/c")]
77+
public void TestZipSlipSanitize_DotDotTraversal(string input, string expected)
78+
{
79+
expected = expected.Replace('/', Path.DirectorySeparatorChar);
80+
var result = FileEntry.ZipSlipSanitize(input);
81+
Assert.Equal(expected, result);
82+
}
83+
84+
/// <summary>
85+
/// ZipSlipSanitize should handle combined absolute + traversal attacks.
86+
/// After ".." is stripped, exposed leading separators are also stripped.
87+
/// </summary>
88+
[Theory]
89+
[InlineData("/../../etc/passwd", "etc/passwd")]
90+
[InlineData("C:\\..\\..\\Windows\\evil.dll", "Windows/evil.dll")]
91+
[InlineData("/../../../tmp/evil", "tmp/evil")]
92+
public void TestZipSlipSanitize_CombinedAbsoluteAndTraversal(string input, string expected)
93+
{
94+
expected = expected.Replace('/', Path.DirectorySeparatorChar);
95+
var result = FileEntry.ZipSlipSanitize(input);
96+
Assert.Equal(expected, result);
97+
}
98+
99+
/// <summary>
100+
/// ZipSlipSanitize should collapse double separators that result from stripping.
101+
/// </summary>
102+
[Theory]
103+
[InlineData("a/../b", "a/b")]
104+
[InlineData("a/../../b", "a/b")]
105+
public void TestZipSlipSanitize_CollapsesDoubleSeparators(string input, string expected)
106+
{
107+
expected = expected.Replace('/', Path.DirectorySeparatorChar);
108+
var result = FileEntry.ZipSlipSanitize(input);
109+
Assert.Equal(expected, result);
110+
}
111+
112+
/// <summary>
113+
/// Safe relative paths should pass through unmodified.
114+
/// </summary>
115+
[Theory]
116+
[InlineData("normal/path/file.txt", "normal/path/file.txt")]
117+
[InlineData("file.txt", "file.txt")]
118+
[InlineData("a/b/c/d.txt", "a/b/c/d.txt")]
119+
public void TestZipSlipSanitize_SafePathsUnchanged(string input, string expected)
120+
{
121+
expected = expected.Replace('/', Path.DirectorySeparatorChar);
122+
var result = FileEntry.ZipSlipSanitize(input);
123+
Assert.Equal(expected, result);
124+
}
125+
126+
/// <summary>
127+
/// FileEntry.FullPath should never be absolute when constructed with a parent,
128+
/// even if the entry name is an absolute path.
129+
/// </summary>
130+
[Fact]
131+
public void TestFileEntry_AbsoluteEntryName_BecomesRelative()
132+
{
133+
var parent = new FileEntry("archive.zip", Stream.Null);
134+
var child = new FileEntry("/etc/cron.d/evil", Stream.Null, parent);
135+
Assert.False(Path.IsPathRooted(child.FullPath),
136+
$"FullPath should be relative but was: {child.FullPath}");
137+
Assert.DoesNotContain("..", child.FullPath);
138+
}
139+
140+
/// <summary>
141+
/// FileEntry.FullPath should not contain ".." even when the entry name has traversal.
142+
/// </summary>
143+
[Fact]
144+
public void TestFileEntry_TraversalEntryName_Sanitized()
145+
{
146+
var parent = new FileEntry("archive.tar", Stream.Null);
147+
var child = new FileEntry("../../../etc/passwd", Stream.Null, parent);
148+
Assert.DoesNotContain("..", child.FullPath);
149+
}
150+
151+
/// <summary>
152+
/// Backslash-rooted paths should also be made relative.
153+
/// </summary>
154+
[Fact]
155+
public void TestFileEntry_BackslashRooted_BecomesRelative()
156+
{
157+
var parent = new FileEntry("archive.zip", Stream.Null);
158+
var child = new FileEntry("\\Windows\\System32\\evil.dll", Stream.Null, parent);
159+
Assert.False(Path.IsPathRooted(child.FullPath),
160+
$"FullPath should be relative but was: {child.FullPath}");
161+
}
162+
38163
protected static readonly NLog.Logger Logger = NLog.LogManager.GetCurrentClassLogger();
39164
}
40165
}

‎RecursiveExtractor/Extractor.cs‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -495,9 +495,16 @@ public ExtractionStatusCode ExtractToDirectory(string outputDirectory, FileEntry
495495
opts ??= new ExtractorOptions();
496496
if (!opts.Parallel)
497497
{
498+
var fullOutputDir = Path.GetFullPath(outputDirectory) + Path.DirectorySeparatorChar;
498499
foreach (var entry in Extract(fileEntry, opts))
499500
{
500501
var targetPath = Path.Combine(outputDirectory, entry.GetSanitizedPath());
502+
var fullTargetPath = Path.GetFullPath(targetPath);
503+
if (!fullTargetPath.StartsWith(fullOutputDir, StringComparison.Ordinal))
504+
{
505+
Logger.Warn("Skipping entry that would escape output directory: {0}", entry.FullPath);
506+
continue;
507+
}
501508
if (Path.GetDirectoryName(targetPath) is { } directoryPathNotNull && targetPath is { } targetPathNotNull)
502509
{
503510
try
@@ -537,6 +544,7 @@ public ExtractionStatusCode ExtractToDirectory(string outputDirectory, FileEntry
537544
using var enumerator = extractedEnumeration.GetEnumerator();
538545
// Move to the first element to prepare
539546
ConcurrentBag<FileEntry> entryBatch = new();
547+
var parallelFullOutputDir = Path.GetFullPath(outputDirectory) + Path.DirectorySeparatorChar;
540548
bool moreAvailable = enumerator.MoveNext();
541549
while (moreAvailable)
542550
{
@@ -559,6 +567,12 @@ public ExtractionStatusCode ExtractToDirectory(string outputDirectory, FileEntry
559567
Parallel.ForEach(entryBatch, new ParallelOptions() { CancellationToken = cts.Token }, entry =>
560568
{
561569
var targetPath = Path.Combine(outputDirectory, entry.GetSanitizedPath());
570+
var fullTargetPath = Path.GetFullPath(targetPath);
571+
if (!fullTargetPath.StartsWith(parallelFullOutputDir, StringComparison.Ordinal))
572+
{
573+
Logger.Warn("Skipping entry that would escape output directory: {0}", entry.FullPath);
574+
return;
575+
}
562576

563577
if (Path.GetDirectoryName(targetPath) is { } directoryPathNotNull && targetPath is { } targetPathNotNull)
564578
{
@@ -645,11 +659,18 @@ public async Task<ExtractionStatusCode> ExtractToDirectoryAsync(string outputDir
645659
/// <param name="printNames">If we should print the filename when writing it out to disc.</param>
646660
public async Task<ExtractionStatusCode> ExtractToDirectoryAsync(string outputDirectory, FileEntry fileEntry, ExtractorOptions? opts = null, IEnumerable<Regex>? acceptFilters = null, IEnumerable<Regex>? denyFilters = null, bool printNames = false)
647661
{
662+
var asyncFullOutputDir = Path.GetFullPath(outputDirectory) + Path.DirectorySeparatorChar;
648663
await foreach (var entry in ExtractAsync(fileEntry, opts))
649664
{
650665
if (opts?.FileNamePasses(entry.FullPath) ?? true)
651666
{
652667
var targetPath = Path.Combine(outputDirectory, entry.GetSanitizedPath());
668+
var fullTargetPath = Path.GetFullPath(targetPath);
669+
if (!fullTargetPath.StartsWith(asyncFullOutputDir, StringComparison.Ordinal))
670+
{
671+
Logger.Warn("Skipping entry that would escape output directory: {0}", entry.FullPath);
672+
continue;
673+
}
653674

654675
if (Path.GetDirectoryName(targetPath) is { } directoryPathNotNull && targetPath is { } targetPathNotNull)
655676
{

‎RecursiveExtractor/FileEntry.cs‎

Lines changed: 34 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -297,23 +297,48 @@ public static async Task<FileEntry> FromStreamAsync(string name, Stream content,
297297
}
298298

299299
/// <summary>
300-
/// Replace .. for ZipSlip and remove any doubled up directory separators as a result - https://snyk.io/research/zip-slip-vulnerability
300+
/// Sanitize paths to prevent ZipSlip (directory traversal) and absolute path traversal.
301+
/// Strips leading directory separators and drive letter roots, removes ".." sequences,
302+
/// and collapses resulting double separators. See https://snyk.io/research/zip-slip-vulnerability
301303
/// </summary>
302304
/// <param name="fullPath">The path to sanitize</param>
303305
/// <param name="replacement">The string to replace .. with</param>
304-
/// <returns>A path without ZipSlip</returns>
306+
/// <returns>A relative path safe from directory traversal</returns>
305307
[Pure]
306-
private static string ZipSlipSanitize(string fullPath, string replacement = "")
308+
internal static string ZipSlipSanitize(string fullPath, string replacement = "")
307309
{
310+
// Normalize all separators to the OS-native separator
311+
fullPath = fullPath.Replace('\\', Path.DirectorySeparatorChar).Replace('/', Path.DirectorySeparatorChar);
312+
313+
// Strip Windows drive letter roots (e.g., "C:\", "D:/")
314+
if (fullPath.Length >= 2 && char.IsLetter(fullPath[0]) && fullPath[1] == ':')
315+
{
316+
fullPath = fullPath.Substring(2);
317+
}
318+
319+
// Strip leading directory separators to prevent absolute path traversal
320+
while (fullPath.Length > 0 && fullPath[0] == Path.DirectorySeparatorChar)
321+
{
322+
fullPath = fullPath.Substring(1);
323+
}
324+
308325
if (fullPath.Contains(".."))
309326
{
310327
fullPath = fullPath.Replace("..", replacement);
311-
var directorySeparator = Path.DirectorySeparatorChar.ToString();
312-
var doubleSeparator = $"{directorySeparator},{directorySeparator}";
313-
while (fullPath.Contains(doubleSeparator))
314-
{
315-
fullPath = fullPath.Replace(doubleSeparator, $"{directorySeparator}");
316-
}
328+
}
329+
330+
// Strip leading separators again (removal of ".." can expose them, e.g. "../x" → "/x")
331+
while (fullPath.Length > 0 && fullPath[0] == Path.DirectorySeparatorChar)
332+
{
333+
fullPath = fullPath.Substring(1);
334+
}
335+
336+
// Collapse double separators (can result from .. removal or root stripping)
337+
var directorySeparator = Path.DirectorySeparatorChar.ToString();
338+
var doubleSeparator = $"{directorySeparator}{directorySeparator}";
339+
while (fullPath.Contains(doubleSeparator))
340+
{
341+
fullPath = fullPath.Replace(doubleSeparator, directorySeparator);
317342
}
318343

319344
return fullPath;

0 commit comments

Comments
 (0)