Solve symlinks destination (#129281)

Evaluate symlinks destination properly

Author: alinpahontu2912

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs

@@ -370,7 +370,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));
             }
@@ -391,7 +391,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));
                 }
@@ -406,7 +406,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));
                 }
@@ -417,6 +417,106 @@ internal Task ExtractRelativeToDirectoryAsync(string destinationDirectoryPath, b
             return (fileDestinationPath, linkTargetPath);
         }
 
+        // Prevent an archive from escaping the extraction root through symlinks that were created by earlier entries in the same archive.
+        // This protection applies only to links introduced by the archive itself. It is not intended to defend against preexisting symlinks
+        // already present on disk before extraction
+        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()
+                ? 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);
+
+            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);
+                current = ResolveSymlink(current);
+
+                string normalizedCurrent = Path.GetFullPath(current);
+                if (!normalizedCurrent.StartsWith(destPrefix, pathComparison) &&
+                    !normalizedCurrent.Equals(resolvedDest, pathComparison))
+                {
+                    return true;
+                }
+            }
+
+            return false;
+        }
+
+        private static string ResolveSymlink(string path)
+        {
+            var info = new FileInfo(path);
+
+            // Check LinkTarget first so dangling symlinks/junctions (whose final target doesn't exist yet)
+            // are still resolved to their raw target, rather than being treated as a non-link.
+            if (info.LinkTarget is null)
+            {
+                return Path.GetFullPath(path);
+            }
+
+            FileSystemInfo target = info.ResolveLinkTarget(returnFinalTarget: true) ?? info;
+            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))
+                {
+                    current = ResolveSymlink(current);
+                }
+            }
+
+            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)
         {

src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectory.File.Tests.cs

@@ -3,6 +3,7 @@
 
 using System.IO;
 using System.Linq;
+using System.Text;
 using Xunit;
 
 namespace System.Formats.Tar.Tests
@@ -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");
+            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));
+
+            // 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(outsideFilePath), "traversal link should not have been created.");
+        }
+
+
+        [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.");
+
+        }
     }
 }