Skip to content

Fix bug in Tar preventing extraction of hardlinks or entries starting with .\ #70853

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
merged 8 commits into from
Jun 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions eng/Version.Details.xml
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,9 @@
<Uri>https://github.com/dotnet/runtime-assets</Uri>
<Sha>0920468fa7db4ee8ea8bbcba186421cb92713adf</Sha>
</Dependency>
<Dependency Name="System.Formats.Tar.TestData" Version="7.0.0-beta.22281.1">
<Dependency Name="System.Formats.Tar.TestData" Version="7.0.0-beta.22313.1">
<Uri>https://github.com/dotnet/runtime-assets</Uri>
<Sha>0920468fa7db4ee8ea8bbcba186421cb92713adf</Sha>
<Sha>371af1f99788b76eae14b96aad4ab7ac9b373938</Sha>
</Dependency>
<Dependency Name="System.IO.Compression.TestData" Version="7.0.0-beta.22281.1">
<Uri>https://github.com/dotnet/runtime-assets</Uri>
Expand Down
2 changes: 1 addition & 1 deletion eng/Versions.props
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@
<SystemRuntimeNumericsTestDataVersion>7.0.0-beta.22281.1</SystemRuntimeNumericsTestDataVersion>
<SystemComponentModelTypeConverterTestDataVersion>7.0.0-beta.22281.1</SystemComponentModelTypeConverterTestDataVersion>
<SystemDrawingCommonTestDataVersion>7.0.0-beta.22281.1</SystemDrawingCommonTestDataVersion>
<SystemFormatsTarTestDataVersion>7.0.0-beta.22281.1</SystemFormatsTarTestDataVersion>
<SystemFormatsTarTestDataVersion>7.0.0-beta.22313.1</SystemFormatsTarTestDataVersion>
<SystemIOCompressionTestDataVersion>7.0.0-beta.22281.1</SystemIOCompressionTestDataVersion>
<SystemIOPackagingTestDataVersion>7.0.0-beta.22281.1</SystemIOPackagingTestDataVersion>
<SystemNetTestDataVersion>7.0.0-beta.22281.1</SystemNetTestDataVersion>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ private static bool GetAlpnSupport()

public static bool SupportsAlpn => s_supportsAlpn.Value;
public static bool SupportsClientAlpn => SupportsAlpn || IsOSX || IsMacCatalyst || IsiOS || IstvOS;
public static bool SupportsHardLinkCreation => !IsAndroid;

private static readonly Lazy<bool> s_supportsTls10 = new Lazy<bool>(GetTls10Support);
private static readonly Lazy<bool> s_supportsTls11 = new Lazy<bool>(GetTls11Support);
Expand Down
41 changes: 19 additions & 22 deletions src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
Original file line number Diff line number Diff line change
Expand Up @@ -261,9 +261,13 @@ internal void ExtractRelativeToDirectory(string destinationDirectoryPath, bool o
Debug.Assert(!string.IsNullOrEmpty(destinationDirectoryPath));
Debug.Assert(Path.IsPathFullyQualified(destinationDirectoryPath));

string destinationDirectoryFullPath = destinationDirectoryPath.EndsWith(Path.DirectorySeparatorChar) ? destinationDirectoryPath : destinationDirectoryPath + Path.DirectorySeparatorChar;
destinationDirectoryPath = Path.TrimEndingDirectorySeparator(destinationDirectoryPath);

string fileDestinationPath = GetSanitizedFullPath(destinationDirectoryFullPath, Name, SR.TarExtractingResultsFileOutside);
string? fileDestinationPath = GetSanitizedFullPath(destinationDirectoryPath, Name);
if (fileDestinationPath == null)
{
throw new IOException(string.Format(SR.TarExtractingResultsFileOutside, Name, destinationDirectoryPath));
}

string? linkTargetPath = null;
if (EntryType is TarEntryType.SymbolicLink or TarEntryType.HardLink)
Expand All @@ -273,7 +277,11 @@ internal void ExtractRelativeToDirectory(string destinationDirectoryPath, bool o
throw new FormatException(SR.TarEntryHardLinkOrSymlinkLinkNameEmpty);
}

linkTargetPath = GetSanitizedFullPath(destinationDirectoryFullPath, LinkName, SR.TarExtractingResultsLinkOutside);
linkTargetPath = GetSanitizedFullPath(destinationDirectoryPath, LinkName);
if (linkTargetPath == null)
{
throw new IOException(string.Format(SR.TarExtractingResultsLinkOutside, LinkName, destinationDirectoryPath));
}
}

if (EntryType == TarEntryType.Directory)
Expand All @@ -286,26 +294,15 @@ 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));
}

return actualPath;
}
// 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 fullyQualifiedPath = Path.IsPathFullyQualified(path) ? path : Path.Combine(destinationDirectoryFullPath, path);
string normalizedPath = Path.GetFullPath(fullyQualifiedPath); // Removes relative segments
string sanitizedPath = Path.Join(Path.GetDirectoryName(normalizedPath), ArchivingUtils.SanitizeEntryFilePath(Path.GetFileName(normalizedPath)));
return sanitizedPath.StartsWith(destinationDirectoryFullPath, PathInternal.StringComparison) ? sanitizedPath : null;
}

// Extracts the current entry into the filesystem, regardless of the entry type.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
<Compile Include="TarFile\TarFile.CreateFromDirectory.Stream.Tests.cs" />
<Compile Include="TarFile\TarFile.ExtractToDirectory.File.Tests.cs" />
<Compile Include="TarFile\TarFile.ExtractToDirectory.Stream.Tests.cs" />
<Compile Include="TarReader\TarReader.ExtractToFile.Tests.cs" />
<Compile Include="TarReader\TarReader.File.Tests.cs" />
<Compile Include="TarReader\TarReader.GetNextEntry.Tests.cs" />
<Compile Include="TarTestsBase.cs" />
Expand Down Expand Up @@ -52,6 +53,7 @@
<!-- Unix specific files -->
<ItemGroup Condition="'$(TargetPlatformIdentifier)' != 'windows'">
<Compile Include="TarFile\TarFile.ExtractToDirectory.File.Tests.Unix.cs" />
<Compile Include="TarReader\TarReader.ExtractToFile.Tests.Unix.cs" />
<Compile Include="TarWriter\TarWriter.WriteEntry.File.Tests.Unix.cs" />
<Compile Include="$(CommonPath)Interop\Unix\Interop.Errors.cs" Link="Common\Interop\Unix\Interop.Errors.cs" />
<Compile Include="$(CommonPath)Interop\Unix\Interop.IOErrors.cs" Link="Common\Interop\Unix\Interop.IOErrors.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,5 +136,28 @@ public void Extract_AllSegmentsOfPath()
string filePath = Path.Join(segment2Path, "file.txt");
Assert.True(File.Exists(filePath), $"{filePath}' does not exist.");
}

[Fact]
public void ExtractArchiveWithEntriesThatStartWithSlashDotPrefix()
{
using TempDirectory root = new TempDirectory();

using MemoryStream archiveStream = GetStrangeTarMemoryStream("prefixDotSlashAndCurrentFolderEntry");

TarFile.ExtractToDirectory(archiveStream, root.Path, overwriteFiles: true);

archiveStream.Position = 0;

using TarReader reader = new TarReader(archiveStream, leaveOpen: false);

TarEntry entry;
while ((entry = reader.GetNextEntry()) != null)
{
// Normalize the path (remove redundant segments), remove trailing separators
// this is so the first entry can be skipped if it's the same as the root directory
string entryPath = Path.TrimEndingDirectorySeparator(Path.GetFullPath(Path.Join(root.Path, entry.Name)));
Assert.True(Path.Exists(entryPath), $"Entry was not extracted: {entryPath}");
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,7 @@ 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)]
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.SupportsHardLinkCreation))]
public void Extract_HardLinkEntry_TargetInsideDirectory() => Extract_LinkEntry_TargetInsideDirectory_Internal(TarEntryType.HardLink);

private void Extract_LinkEntry_TargetInsideDirectory_Internal(TarEntryType entryType)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// 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 System.IO;
using System.Linq;
using System.Reflection;
using Xunit;

namespace System.Formats.Tar.Tests
{
public partial class TarReader_ExtractToFile_Tests : TarTestsBase
{
[Fact]
public void ExtractToFile_SpecialFile_Unelevated_Throws()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you know you are unelevated here? Don't we run CI tests as sudo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's my understanding that we only run as sudo when we explicitly wrap the code with RemoteExecutor, and that has been the behavior I've seen in the few tests I've protected with RemoteExecutor.

Is that assumption incorrect?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. I've seen other tests be conditional on whether the process is elevated or not. For example:

private static bool SupportsRawSockets => AdminHelpers.IsProcessElevated();
private static bool NotSupportsRawSockets => !SupportsRawSockets;

[PlatformSpecific(TestPlatforms.AnyUnix)]
[InlineData(AddressFamily.InterNetwork, ProtocolType.Tcp)]
[InlineData(AddressFamily.InterNetwork, ProtocolType.Udp)]
[InlineData(AddressFamily.InterNetwork, ProtocolType.Icmp)]
[InlineData(AddressFamily.InterNetworkV6, ProtocolType.Tcp)]
[InlineData(AddressFamily.InterNetworkV6, ProtocolType.Udp)]
[InlineData(AddressFamily.InterNetworkV6, ProtocolType.IcmpV6)]
[ConditionalTheory(nameof(SupportsRawSockets))]
public void Ctor_Raw_Supported_Success(AddressFamily addressFamily, ProtocolType protocolType)
{
using (new Socket(addressFamily, SocketType.Raw, protocolType))
{
}
}
[PlatformSpecific(TestPlatforms.AnyUnix)]
[InlineData(AddressFamily.InterNetwork, ProtocolType.Tcp)]
[InlineData(AddressFamily.InterNetwork, ProtocolType.Udp)]
[InlineData(AddressFamily.InterNetwork, ProtocolType.Icmp)]
[InlineData(AddressFamily.InterNetworkV6, ProtocolType.Tcp)]
[InlineData(AddressFamily.InterNetworkV6, ProtocolType.Udp)]
[InlineData(AddressFamily.InterNetworkV6, ProtocolType.IcmpV6)]
[ConditionalTheory(nameof(NotSupportsRawSockets))]
public void Ctor_Raw_NotSupported_ExpectedError(AddressFamily addressFamily, ProtocolType protocolType)

I'm not sure of all the ways our tests are run. So if you are expecting this test to only work in an unelevated process, it might be good to stick that in a condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW this test already existed, I merely renamed the file and the test class.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok - if it is already passing in CI, then we can just keep it.

Copy link
Contributor Author

@carlossanlop carlossanlop Jun 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool.

BTW the last argument of the RemoteExecutor.Invoke method is where I indicate that I want the code to be elevated:

RemoteExecutor.Invoke(() =>
            {
            // Code to elevate here
            }, new RemoteInvokeOptions { RunAsSudo = true }).Dispose();

Anything not explicitly elevated, runs as a normal user. Or at least that's my understanding.

{
using TempDirectory root = new TempDirectory();
using MemoryStream ms = GetTarMemoryStream(CompressionMethod.Uncompressed, TestTarFormat.ustar, "specialfiles");

using TarReader reader = new TarReader(ms);

string path = Path.Join(root.Path, "output");

// Block device requires elevation for writing
PosixTarEntry blockDevice = reader.GetNextEntry() as PosixTarEntry;
Assert.NotNull(blockDevice);
Assert.Throws<UnauthorizedAccessException>(() => blockDevice.ExtractToFile(path, overwrite: false));
Assert.False(File.Exists(path));

// Character device requires elevation for writing
PosixTarEntry characterDevice = reader.GetNextEntry() as PosixTarEntry;
Assert.NotNull(characterDevice);
Assert.Throws<UnauthorizedAccessException>(() => characterDevice.ExtractToFile(path, overwrite: false));
Assert.False(File.Exists(path));

// Fifo does not require elevation, should succeed
PosixTarEntry fifo = reader.GetNextEntry() as PosixTarEntry;
Assert.NotNull(fifo);
fifo.ExtractToFile(path, overwrite: false);
Assert.True(File.Exists(path));

Assert.Null(reader.GetNextEntry());
}
}
}
Original file line number Diff line number Diff line change
@@ -1,44 +1,38 @@
// 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 System.IO;
using System.Linq;
using Xunit;

namespace System.Formats.Tar.Tests
{
public class TarReader_ExtractToFile_Tests : TarTestsBase
public partial class TarReader_ExtractToFile_Tests : TarTestsBase
{
[Fact]
public void ExtractToFile_SpecialFile_Unelevated_Throws()
public void ExtractEntriesWithSlashDotPrefix()
{
using TempDirectory root = new TempDirectory();
using MemoryStream ms = GetTarMemoryStream(CompressionMethod.Uncompressed, TestTarFormat.ustar, "specialfiles");

using TarReader reader = new TarReader(ms);

string path = Path.Join(root.Path, "output");

// Block device requires elevation for writing
PosixTarEntry blockDevice = reader.GetNextEntry() as PosixTarEntry;
Assert.NotNull(blockDevice);
Assert.Throws<UnauthorizedAccessException>(() => blockDevice.ExtractToFile(path, overwrite: false));
Assert.False(File.Exists(path));

// Character device requires elevation for writing
PosixTarEntry characterDevice = reader.GetNextEntry() as PosixTarEntry;
Assert.NotNull(characterDevice);
Assert.Throws<UnauthorizedAccessException>(() => characterDevice.ExtractToFile(path, overwrite: false));
Assert.False(File.Exists(path));

// Fifo does not require elevation, should succeed
PosixTarEntry fifo = reader.GetNextEntry() as PosixTarEntry;
Assert.NotNull(fifo);
fifo.ExtractToFile(path, overwrite: false);
Assert.True(File.Exists(path));

Assert.Null(reader.GetNextEntry());
using MemoryStream archiveStream = GetStrangeTarMemoryStream("prefixDotSlashAndCurrentFolderEntry");
using (TarReader reader = new TarReader(archiveStream, leaveOpen: false))
{
string rootPath = Path.TrimEndingDirectorySeparator(root.Path);
TarEntry entry;
while ((entry = reader.GetNextEntry()) != null)
{
Assert.NotNull(entry);
Assert.StartsWith("./", entry.Name);
// Normalize the path (remove redundant segments), remove trailing separators
// this is so the first entry can be skipped if it's the same as the root directory
string entryPath = Path.TrimEndingDirectorySeparator(Path.GetFullPath(Path.Join(rootPath, entry.Name)));
if (entryPath != rootPath)
{
entry.ExtractToFile(entryPath, overwrite: true);
Assert.True(Path.Exists(entryPath), $"Entry was not extracted: {entryPath}");
}
}
}
}

}
}
12 changes: 10 additions & 2 deletions src/libraries/System.Formats.Tar/tests/TarTestsBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,17 @@ protected static string GetTarFilePath(CompressionMethod compressionMethod, Test
}

// MemoryStream containing the copied contents of the specified file. Meant for reading and writing.
protected static MemoryStream GetTarMemoryStream(CompressionMethod compressionMethod, TestTarFormat format, string testCaseName)
protected static MemoryStream GetTarMemoryStream(CompressionMethod compressionMethod, TestTarFormat format, string testCaseName) =>
GetMemoryStream(GetTarFilePath(compressionMethod, format, testCaseName));

protected static string GetStrangeTarFilePath(string testCaseName) =>
Path.Join(Directory.GetCurrentDirectory(), "strange", testCaseName + ".tar");

protected static MemoryStream GetStrangeTarMemoryStream(string testCaseName) =>
GetMemoryStream(GetStrangeTarFilePath(testCaseName));

private static MemoryStream GetMemoryStream(string path)
{
string path = GetTarFilePath(compressionMethod, format, testCaseName);
MemoryStream ms = new();
using (FileStream fs = File.OpenRead(path))
{
Expand Down