Solve symlinks destination #129281
Open
alinpahontu2912 wants to merge 2 commits into
Open
Conversation
Contributor
|
Tagging subscribers to this area: @dotnet/area-system-formats-tar |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens System.Formats.Tar extraction safety by validating that computed extraction destinations (including link destinations) don’t escape the intended destination directory once filesystem symlinks are taken into account, and adds tests intended to cover symlink-based directory traversal scenarios.
Changes:
- Add a symlink-aware escape check (
FilePathEscapesDirectory) during destination and link path validation inTarEntry. - Extend extraction validation to reject entries whose resolved destinations would traverse outside the destination directory.
- Add new extraction tests covering symlink traversal attempts (including chained symlinks).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs | Adds symlink-aware escape validation for extraction destination paths and link destinations. |
| src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectory.File.Tests.cs | Adds new tests intended to validate rejection of symlink-based directory traversal during extraction. |
Comment on lines
+427
to
+438
| string resolvedDest = ResolvePhysicalPath(destinationDirectoryPath); | ||
| string destPrefix = resolvedDest.EndsWith(Path.DirectorySeparatorChar) | ||
| ? resolvedDest | ||
| : resolvedDest + Path.DirectorySeparatorChar; | ||
|
|
||
| // Normalize file path (resolves .. and . but not symlinks) | ||
| string normalizedFile = Path.GetFullPath(fileDestinationPath); | ||
|
|
||
| // Walk relative components, resolving symlinks at each step | ||
| string relative = normalizedFile.Substring(resolvedDest.Length) | ||
| .TrimStart(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); | ||
|
|
Comment on lines
+505
to
+510
| // 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
+479
to
+503
| // 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)); |
Comment on lines
+469
to
+479
| 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; | ||
| } |
rzikm
approved these changes
Jun 11, 2026
am11
reviewed
Jun 11, 2026
| { | ||
| // 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() |
Member
There was a problem hiding this comment.
- NTFS on windows is actually case-sensitive internally, and we can enable it in userland with
fsutil.exe file setCaseSensitiveInfo <path> enableto 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
casefoldenabled. 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.
am11
reviewed
Jun 11, 2026
Comment on lines
+459
to
+460
| if (!normalizedCurrent.StartsWith(destPrefix, pathComparison) && | ||
| !normalizedCurrent.Equals(resolvedDest, pathComparison)) |
Member
There was a problem hiding this comment.
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)) |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Evaluate symlinks destination properly