Solve symlinks destination #129281
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-formats-tar |
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. |
| { | ||
| // 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() |
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.
| if (!normalizedCurrent.StartsWith(destPrefix, pathComparison) && | ||
| !normalizedCurrent.Equals(resolvedDest, pathComparison)) |
There was a problem hiding this comment.
| 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)) |
4f88961 to
ef504a5
Compare
ef504a5 to
a1b2aab
Compare
GrabYourPitchforks
left a comment
There was a problem hiding this comment.
(holding for offline feedback)
GrabYourPitchforks
left a comment
There was a problem hiding this comment.
This looks like it's providing a guarantee we do not intend to provide.
There needs to be a prominent code comment to echo the Remarks section at https://learn.microsoft.com/en-us/dotnet/api/system.formats.tar.tarfile.extracttodirectory:
This is only intended to guard against the case where both:
- a symlink tries to break out of the destination folder; and
- the symlink is introduced by an entry within the archive.
No specific defenses are intended for the case where a symlink already exists in the destination folder, as the destination folder is assumed trustworthy. (See also: https://github.com/dotnet/core/blob/main/Documentation/security-foundations/baseline-security-assumptions.md#23-the-command-line-is-trusted-as-a-control-plane-mechanism)
Evaluate symlinks destination properly