From 744e088a51b7fe0d074761dcd002c3cd67ceeb5f Mon Sep 17 00:00:00 2001 From: carlossanlop <1175054+carlossanlop@users.noreply.github.com> Date: Sat, 21 May 2022 14:35:51 -0700 Subject: [PATCH 1/3] Change GetSanitizedFulPath logic to first combine paths before sanitization, mavoid locaal method to improve stacktrace. --- .../src/System/Formats/Tar/TarEntry.cs | 28 +++++++------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs b/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs index 0800f39ecf0285..694d31c8734aa8 100644 --- a/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs +++ b/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs @@ -286,26 +286,18 @@ internal void ExtractRelativeToDirectory(string destinationDirectoryPath, bool o Directory.CreateDirectory(Path.GetDirectoryName(fileDestinationPath)!); ExtractToFileInternal(fileDestinationPath, linkTargetPath, overwrite); } + } - // If the path can be extracted in the specified destination directory, returns the full path with sanitized file name. Otherwise, throws. - static string GetSanitizedFullPath(string destinationDirectoryFullPath, string path, string exceptionMessage) - { - string actualPath = Path.Join(Path.GetDirectoryName(path), ArchivingUtils.SanitizeEntryFilePath(Path.GetFileName(path))); - - if (!Path.IsPathFullyQualified(actualPath)) - { - actualPath = Path.Combine(destinationDirectoryFullPath, actualPath); - } - - actualPath = Path.GetFullPath(actualPath); - - if (!actualPath.StartsWith(destinationDirectoryFullPath, PathInternal.StringComparison)) - { - throw new IOException(string.Format(exceptionMessage, path, destinationDirectoryFullPath)); - } + // If the path can be extracted in the specified destination directory, returns the full path with sanitized file name. Otherwise, throws. + private static string GetSanitizedFullPath(string destinationDirectoryFullPath, string path, string exceptionMessage) + { + string actualPath = Path.IsPathFullyQualified(path) ? path : Path.Combine(destinationDirectoryFullPath, path); + actualPath = Path.Join(Path.GetDirectoryName(actualPath), ArchivingUtils.SanitizeEntryFilePath(Path.GetFileName(actualPath))); + actualPath = Path.GetFullPath(actualPath); // Normalizes relative segments - return actualPath; - } + return actualPath.StartsWith(destinationDirectoryFullPath, PathInternal.StringComparison) ? + actualPath : + throw new IOException(string.Format(exceptionMessage, path, destinationDirectoryFullPath)); } // Extracts the current entry into the filesystem, regardless of the entry type. From 9a61a822b1d5809327c7cc6b04ccc856bd540e0d Mon Sep 17 00:00:00 2001 From: carlossanlop <1175054+carlossanlop@users.noreply.github.com> Date: Sat, 21 May 2022 14:36:08 -0700 Subject: [PATCH 2/3] Skip Android hardlink creation test, make sure to use Pax format due to potential long paths when extracting to temp dir. --- .../TarFile.ExtractToDirectory.Stream.Tests.cs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectory.Stream.Tests.cs b/src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectory.Stream.Tests.cs index 5bb0d9b2c3eb49..6b30c1fdd3f5f5 100644 --- a/src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectory.Stream.Tests.cs +++ b/src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectory.Stream.Tests.cs @@ -1,7 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Collections.Generic; +using Microsoft.DotNet.RemoteExecutor; using System.IO; using System.Linq; using Xunit; @@ -52,7 +52,7 @@ public void ExtractEntry_ManySubfolderSegments_NoPrecedingDirectoryEntries() string fileWithTwoSegments = Path.Join(secondSegment, "c.txt"); using MemoryStream archive = new MemoryStream(); - using (TarWriter writer = new TarWriter(archive, TarFormat.Ustar, leaveOpen: true)) + using (TarWriter writer = new TarWriter(archive, TarFormat.Pax, leaveOpen: true)) { // No preceding directory entries for the segments UstarTarEntry entry = new UstarTarEntry(TarEntryType.RegularFile, fileWithTwoSegments); @@ -78,7 +78,7 @@ public void ExtractEntry_ManySubfolderSegments_NoPrecedingDirectoryEntries() public void Extract_LinkEntry_TargetOutsideDirectory(TarEntryType entryType) { using MemoryStream archive = new MemoryStream(); - using (TarWriter writer = new TarWriter(archive, TarFormat.Ustar, leaveOpen: true)) + using (TarWriter writer = new TarWriter(archive, TarFormat.Pax, leaveOpen: true)) { UstarTarEntry entry = new UstarTarEntry(entryType, "link"); entry.LinkName = PlatformDetection.IsWindows ? @"C:\Windows\System32\notepad.exe" : "/usr/bin/nano"; @@ -97,8 +97,9 @@ public void Extract_LinkEntry_TargetOutsideDirectory(TarEntryType entryType) [ConditionalFact(typeof(MountHelper), nameof(MountHelper.CanCreateSymbolicLinks))] public void Extract_SymbolicLinkEntry_TargetInsideDirectory() => Extract_LinkEntry_TargetInsideDirectory_Internal(TarEntryType.SymbolicLink); - [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/68360", TestPlatforms.Android | TestPlatforms.LinuxBionic | TestPlatforms.iOS | TestPlatforms.tvOS)] + private static bool SupportsHardLinkCreation => !PlatformDetection.IsAndroid; + + [ConditionalFact(nameof(SupportsHardLinkCreation))] public void Extract_HardLinkEntry_TargetInsideDirectory() => Extract_LinkEntry_TargetInsideDirectory_Internal(TarEntryType.HardLink); private void Extract_LinkEntry_TargetInsideDirectory_Internal(TarEntryType entryType) @@ -112,7 +113,7 @@ private void Extract_LinkEntry_TargetInsideDirectory_Internal(TarEntryType entry File.Create(targetPath).Dispose(); using MemoryStream archive = new MemoryStream(); - using (TarWriter writer = new TarWriter(archive, TarFormat.Ustar, leaveOpen: true)) + using (TarWriter writer = new TarWriter(archive, TarFormat.Pax, leaveOpen: true)) { UstarTarEntry entry = new UstarTarEntry(entryType, linkName); entry.LinkName = targetPath; From 2384acb2406ce09b45cd4e9b5cd8d64da64f9233 Mon Sep 17 00:00:00 2001 From: carlossanlop <1175054+carlossanlop@users.noreply.github.com> Date: Thu, 26 May 2022 19:35:31 -0700 Subject: [PATCH 3/3] Avoid fetching exception messages in GetSanitizedFullPath, return null instead --- .../src/System/Formats/Tar/TarEntry.cs | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs b/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs index 694d31c8734aa8..6636ca66cef0fa 100644 --- a/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs +++ b/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs @@ -263,7 +263,11 @@ internal void ExtractRelativeToDirectory(string destinationDirectoryPath, bool o string destinationDirectoryFullPath = destinationDirectoryPath.EndsWith(Path.DirectorySeparatorChar) ? destinationDirectoryPath : destinationDirectoryPath + Path.DirectorySeparatorChar; - string fileDestinationPath = GetSanitizedFullPath(destinationDirectoryFullPath, Name, SR.TarExtractingResultsFileOutside); + string? fileDestinationPath = GetSanitizedFullPath(destinationDirectoryFullPath, Name); + if (fileDestinationPath == null) + { + throw new IOException(string.Format(SR.TarExtractingResultsFileOutside, Name, destinationDirectoryFullPath)); + } string? linkTargetPath = null; if (EntryType is TarEntryType.SymbolicLink or TarEntryType.HardLink) @@ -273,7 +277,11 @@ internal void ExtractRelativeToDirectory(string destinationDirectoryPath, bool o throw new FormatException(SR.TarEntryHardLinkOrSymlinkLinkNameEmpty); } - linkTargetPath = GetSanitizedFullPath(destinationDirectoryFullPath, LinkName, SR.TarExtractingResultsLinkOutside); + linkTargetPath = GetSanitizedFullPath(destinationDirectoryFullPath, LinkName); + if (linkTargetPath == null) + { + throw new IOException(string.Format(SR.TarExtractingResultsLinkOutside, LinkName, destinationDirectoryFullPath)); + } } if (EntryType == TarEntryType.Directory) @@ -288,16 +296,14 @@ internal void ExtractRelativeToDirectory(string destinationDirectoryPath, bool o } } - // If the path can be extracted in the specified destination directory, returns the full path with sanitized file name. Otherwise, throws. - private static string GetSanitizedFullPath(string destinationDirectoryFullPath, string path, string exceptionMessage) + // If the path can be extracted in the specified destination directory, returns the full path with sanitized file name. Otherwise, returns null. + private static string? GetSanitizedFullPath(string destinationDirectoryFullPath, string path) { string actualPath = Path.IsPathFullyQualified(path) ? path : Path.Combine(destinationDirectoryFullPath, path); actualPath = Path.Join(Path.GetDirectoryName(actualPath), ArchivingUtils.SanitizeEntryFilePath(Path.GetFileName(actualPath))); actualPath = Path.GetFullPath(actualPath); // Normalizes relative segments - return actualPath.StartsWith(destinationDirectoryFullPath, PathInternal.StringComparison) ? - actualPath : - throw new IOException(string.Format(exceptionMessage, path, destinationDirectoryFullPath)); + return actualPath.StartsWith(destinationDirectoryFullPath, PathInternal.StringComparison) ? actualPath : null; } // Extracts the current entry into the filesystem, regardless of the entry type.