Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 5.6k
Solve symlinks destination #129281
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
alinpahontu2912
merged 9 commits into
dotnet:main
from
alinpahontu2912:symlink_resolutionJul 3, 2026
Uh oh!
There was an error while loading. Please reload this page.
Merged
Solve symlinks destination #129281
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
1bb7be1
solve symlinks before extraction
alinpahontu2912 4f88961
Separate logical and physical path handling
alinpahontu2912 d2ffbdc
solve symlinks before extraction
alinpahontu2912 a1b2aab
Separate logical and physical path handling
alinpahontu2912 8c91e3b
add comment explaining behaviour
alinpahontu2912 5f6acea
address comments
alinpahontu2912 bf5902c
fix conflicts
alinpahontu2912 32f1c2b
update explanation
alinpahontu2912 98945b3
Merge branch 'main' into symlink_resolution
alinpahontu2912 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
106 changes: 103 additions & 3 deletions
106 src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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)) | ||
alinpahontu2912 marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| { | ||
| 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) | ||
| { | ||
90 changes: 90 additions & 0 deletions
90 src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectory.File.Tests.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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)); | ||
alinpahontu2912 marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| // 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."); | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.