Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 112 additions & 3 deletions src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ internal Task ExtractRelativeToDirectoryAsync(string destinationDirectoryPath, b
string? fileDestinationPath = GetFullDestinationPath(
destinationDirectoryPath,
Path.IsPathFullyQualified(name) ? name : Path.Join(destinationDirectoryPath, name));
if (fileDestinationPath == null)
if (fileDestinationPath is null || FilePathEscapesDirectory(destinationDirectoryPath, fileDestinationPath))
{
throw new IOException(SR.Format(SR.TarExtractingResultsFileOutside, name, destinationDirectoryPath));
}
Expand All @@ -389,7 +389,7 @@ internal Task ExtractRelativeToDirectoryAsync(string destinationDirectoryPath, b
string? linkDestination = GetFullDestinationPath(
destinationDirectoryPath,
Path.IsPathFullyQualified(linkName) ? linkName : Path.Join(Path.GetDirectoryName(fileDestinationPath), linkName));
if (linkDestination is null)
if (linkDestination is null || FilePathEscapesDirectory(destinationDirectoryPath, linkDestination))
{
throw new IOException(SR.Format(SR.TarExtractingResultsLinkOutside, linkName, destinationDirectoryPath));
}
Expand All @@ -404,7 +404,7 @@ internal Task ExtractRelativeToDirectoryAsync(string destinationDirectoryPath, b
string? linkDestination = GetFullDestinationPath(
destinationDirectoryPath,
Path.Join(destinationDirectoryPath, linkName));
if (linkDestination is null)
if (linkDestination is null || FilePathEscapesDirectory(destinationDirectoryPath, linkDestination))
{
throw new IOException(SR.Format(SR.TarExtractingResultsLinkOutside, linkName, destinationDirectoryPath));
}
Expand All @@ -415,6 +415,115 @@ internal Task ExtractRelativeToDirectoryAsync(string destinationDirectoryPath, b
return (fileDestinationPath, linkTargetPath);
}

// Check if the file destination path or the link target path escapes the destination directory, by walking through the relative path components and resolving symlinks at each step.
private static bool FilePathEscapesDirectory(string destinationDirectoryPath, string fileDestinationPath)
{
// Windows is case insensitive while Linux is case sensitive
// This ensures the comparison is consistent with how the OS would resolve the paths
StringComparison pathComparison = OperatingSystem.IsWindows()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • NTFS on windows is actually case-sensitive internally, and we can enable it in userland with fsutil.exe file setCaseSensitiveInfo <path> enable to violate the general assumptions
  • APFS on mac can be configured case-sensitive (default is case-insensitive)
  • EXT4 on Linux can provide case-insensitivity per path with casefold enabled. Also, we can mount NTFS, FAT or other case-insensitive filesystem on unix and run app from there.

It's best to avoid assuming case sensitivity and let the underlying filesystem do its thing.

? StringComparison.OrdinalIgnoreCase
: StringComparison.Ordinal;

string resolvedDest = ResolvePhysicalPath(destinationDirectoryPath);

// Use the logical destination path for computing the relative path
string logicalDest = Path.GetFullPath(destinationDirectoryPath);
string logicalPrefix = logicalDest.EndsWith(Path.DirectorySeparatorChar)
? logicalDest
: logicalDest + Path.DirectorySeparatorChar;

string destPrefix = resolvedDest.EndsWith(Path.DirectorySeparatorChar)
? resolvedDest
: resolvedDest + Path.DirectorySeparatorChar;

// Normalize file path (resolves .. and . but not symlinks)
string normalizedFile = Path.GetFullPath(fileDestinationPath);

// Guard with StartsWith before computing relative path
if (!normalizedFile.StartsWith(logicalPrefix, pathComparison) &&
!normalizedFile.Equals(logicalDest, pathComparison))
{
return true;
}

// Walk relative components, resolving symlinks at each step
string relative = normalizedFile.Substring(logicalPrefix.Length)
.TrimStart(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar);

Comment on lines +449 to +452
string[] components = relative.Split(new char[] { Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar },
StringSplitOptions.RemoveEmptyEntries);

string current = resolvedDest;

foreach (string component in components)
{
current = Path.Combine(current, component);

if (Path.Exists(current))
{
string? resolved = ResolveSymlink(current);
if (resolved is null)
{
return true;
}
current = resolved;
}

string normalizedCurrent = Path.GetFullPath(current);
if (!normalizedCurrent.StartsWith(destPrefix, pathComparison) &&
!normalizedCurrent.Equals(resolvedDest, pathComparison))
Comment on lines +473 to +474

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!normalizedCurrent.StartsWith(destPrefix, pathComparison) &&
!normalizedCurrent.Equals(resolvedDest, pathComparison))
// Deferring to filesystem for case-sensitivity to avoid unreliable manual probes.
if (!normalizedCurrent.StartsWith(destPrefix, StringComparison.OrdinalIgnoreCase))

{
return true;
}
}

return false;
}

private static string? ResolveSymlink(string path)
{
FileSystemInfo? target = new FileInfo(path).ResolveLinkTarget(returnFinalTarget: true);

if (target is null)
{
return Path.GetFullPath(path);
}

return target.FullName;
}

// Resolves the full path of the specified path, resolving symlinks at each step.
// This is needed to mitigate malicious entries in the archive that could lead to writing files outside of the intended directory.
private static string ResolvePhysicalPath(string path)
{
string fullPath = Path.GetFullPath(path);
string? root = Path.GetPathRoot(fullPath);

if (root is null)
{
return fullPath;
}

string[] components = fullPath.Substring(root.Length)
.Split(new char[] { Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar }, StringSplitOptions.RemoveEmptyEntries);
string current = root;
foreach (string component in components)
{
current = Path.Combine(current, component);
if (Path.Exists(current))
{
string? resolved = ResolveSymlink(current);
if (resolved is null)
{
return current;
}
current = resolved;
}
}

return current;
}

// Returns the full destination path if the path is the destinationDirectory or a subpath. Otherwise, returns null.
private static string? GetFullDestinationPath(string destinationDirectoryFullPath, string qualifiedPath)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.IO;
using System.Linq;
using System.Text;
using Xunit;

namespace System.Formats.Tar.Tests
Expand Down Expand Up @@ -467,5 +468,94 @@ public void HardLinkExtraction_CopyContents()
Assert.Equal("test content", File.ReadAllText(targetFile2));
AssertPathsAreNotHardLinked(targetFile1, targetFile2);
}

[ConditionalFact(typeof(MountHelper), nameof(MountHelper.CanCreateSymbolicLinks))]
public void ExtractToDirectory_RejectsSymlinkDirectoryTraversal_WithNestedFile()
{
using TempDirectory root = new TempDirectory();
string destDir = Path.Combine(root.Path, "dest");
Directory.CreateDirectory(destDir);

// Absolute path outside destDir
string linkTarget = "/tmp/outside";

string tarPath = Path.Combine(root.Path, "symlink_dir_traversal.tar");
Comment on lines +479 to +482
using (FileStream stream = new FileStream(tarPath, FileMode.Create, FileAccess.Write))
using (TarWriter writer = new TarWriter(stream, leaveOpen: false))
{
// symlink: "link" -> "/tmp/outside"
writer.WriteEntry(new PaxTarEntry(TarEntryType.SymbolicLink, "link")
{
LinkName = linkTarget
});

// file: "link/test.txt" with "hello"
byte[] content = Encoding.UTF8.GetBytes("hello");
var fileEntry = new PaxTarEntry(TarEntryType.RegularFile, "link/test.txt")
{
DataStream = new MemoryStream(content, writable: false)
};

fileEntry.DataStream.Position = 0;
writer.WriteEntry(fileEntry);
}

Assert.Throws<IOException>(() => TarFile.ExtractToDirectory(tarPath, destDir, overwriteFiles: true));
Comment thread
alinpahontu2912 marked this conversation as resolved.

// Nothing should be created in dest
string linkPath = Path.Combine(destDir, "link");
string outsideFilePath = Path.Combine(destDir, "link", "test.txt");
Assert.False(File.Exists(linkPath) || Directory.Exists(linkPath), "link should not have been created.");
Assert.False(File.Exists(outsideFilePath) || Directory.Exists(linkPath), "traversal link should not have been created.");
Comment on lines +507 to +509
}


[ConditionalFact(typeof(MountHelper), nameof(MountHelper.CanCreateSymbolicLinks))]
public void ExtractToDirectory_RejectsChainedSymlinkDirectoryTraversal_WithNestedFile()
{
// dir a/
// symlink a/b ? .
// symlink a/b/c ? .
// symlink a/b/c/d ? ../../outside
// file a/d/ pwned.txt escapes

using TempDirectory root = new TempDirectory();
string destDir = Path.Combine(root.Path, "dest");
Directory.CreateDirectory(destDir);

string tarPath = Path.Combine(root.Path, "chained_symlink_traversal.tar");
using (FileStream stream = new FileStream(tarPath, FileMode.Create, FileAccess.Write))
using (TarWriter writer = new TarWriter(stream, leaveOpen: false))
{
writer.WriteEntry(new PaxTarEntry(TarEntryType.Directory, "a/"));

writer.WriteEntry(new PaxTarEntry(TarEntryType.SymbolicLink, "a/b") { LinkName = "." });

writer.WriteEntry(new PaxTarEntry(TarEntryType.SymbolicLink, "a/b/c") { LinkName = "." });

writer.WriteEntry(new PaxTarEntry(TarEntryType.SymbolicLink, "a/b/c/d") { LinkName = "../../outside" });

var pwned = new PaxTarEntry(TarEntryType.RegularFile, "a/d/pwned.txt")
{
DataStream = new MemoryStream(Encoding.UTF8.GetBytes("pwned"))
};
writer.WriteEntry(pwned);
}

if (OperatingSystem.IsWindows())
{
// Windows only creates file symlinks and trying to process a directory symlink will throw UnauthorizedAccessException instead of IOException
Assert.Throws<UnauthorizedAccessException>(() => TarFile.ExtractToDirectory(tarPath, destDir, overwriteFiles: true));
}
else
{
Assert.Throws<IOException>(() => TarFile.ExtractToDirectory(tarPath, destDir, overwriteFiles: true));
}

string outsideDir = Path.Combine(root.Path, "outside");
Assert.False(Directory.Exists(outsideDir), "outside/directory should not have been created.");
Assert.False(File.Exists(Path.Combine(outsideDir, "pwned.txt")), "pwned.txt should not have been written outside destination.");

}
}
}
Loading