Skip to content
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.

Sign up for GitHub

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

Tar, Zip: respect umask when creating files. #71647

Merged
merged 5 commits into from
Jul 8, 2022
Merged

Tar, Zip: respect umask when creating files. #71647

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
90dda98
Tar, Zip: respect umask when creating files.
tmds Jul 5, 2022
bf0cb4c
Update UnixExtractFilePermissionsCompat
tmds Jul 5, 2022
d540d8f
PR feedback.
tmds Jul 7, 2022
ae5d39b
Replace const int with const UnixFileMode.
tmds Jul 7, 2022
0e419e4
Also run tests with zero umask
tmds Jul 8, 2022
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
18 changes: 0 additions & 18 deletions src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,23 +39,5 @@ private void ExtractAsHardLink(string targetFilePath, string hardLinkFilePath)
Debug.Assert(!string.IsNullOrEmpty(hardLinkFilePath));
Interop.CheckIo(Interop.Sys.Link(targetFilePath, hardLinkFilePath), hardLinkFilePath);
}

// Unix specific implementation of the method that specifies the file permissions of the extracted file.
private void SetModeOnFile(SafeFileHandle handle)
{
// Only extract USR, GRP, and OTH file permissions, and ignore
// S_ISUID, S_ISGID, and S_ISVTX bits.
// It is off by default because it's possible that a file in an archive could have
// one of these bits set and, unknown to the person extracting, could allow others to
// execute the file as the user or group.
const int ExtractPermissionMask = 0x1FF;
int permissions = (int)Mode & ExtractPermissionMask;

// If the permissions weren't set at all, don't write the file's permissions.
if (permissions != 0)
{
File.SetUnixFileMode(handle, (UnixFileMode)permissions);
}
}
}
}
8 changes: 0 additions & 8 deletions src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,5 @@ private void ExtractAsHardLink(string targetFilePath, string hardLinkFilePath)
Debug.Assert(!string.IsNullOrEmpty(hardLinkFilePath));
Interop.Kernel32.CreateHardLink(hardLinkFilePath, targetFilePath);
}

// Mode is not used on Windows.
#pragma warning disable CA1822 // Member 'SetModeOnFile' does not access instance data and can be marked as static
private void SetModeOnFile(SafeFileHandle handle)
#pragma warning restore CA1822
{
// TODO: Verify that executables get their 'executable' permission applied on Windows when extracted, if applicable. https://github.com/dotnet/runtime/issues/68230
}
}
}
47 changes: 28 additions & 19 deletions src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,22 +541,14 @@ private void ExtractAsRegularFile(string destinationFileName)
{
Debug.Assert(!Path.Exists(destinationFileName));

FileStreamOptions fileStreamOptions = new()
{
Access = FileAccess.Write,
Mode = FileMode.CreateNew,
Share = FileShare.None,
PreallocationSize = Length,
};
// Rely on FileStream's ctor for further checking destinationFileName parameter
using (FileStream fs = new FileStream(destinationFileName, fileStreamOptions))
using (FileStream fs = new FileStream(destinationFileName, CreateFileStreamOptions(isAsync: false)))
{
if (DataStream != null)
{
// Important: The DataStream will be written from its current position
DataStream.CopyTo(fs);
}
SetModeOnFile(fs.SafeFileHandle);
}

ArchivingUtils.AttemptSetLastWriteTime(destinationFileName, ModificationTime);
Expand All @@ -570,27 +562,44 @@ private async Task ExtractAsRegularFileAsync(string destinationFileName, Cancell

cancellationToken.ThrowIfCancellationRequested();

FileStreamOptions fileStreamOptions = new()
{
Access = FileAccess.Write,
Mode = FileMode.CreateNew,
Share = FileShare.None,
PreallocationSize = Length,
Options = FileOptions.Asynchronous
};
// Rely on FileStream's ctor for further checking destinationFileName parameter
FileStream fs = new FileStream(destinationFileName, fileStreamOptions);
FileStream fs = new FileStream(destinationFileName, CreateFileStreamOptions(isAsync: true));
await using (fs)
{
if (DataStream != null)
{
// Important: The DataStream will be written from its current position
await DataStream.CopyToAsync(fs, cancellationToken).ConfigureAwait(false);
}
SetModeOnFile(fs.SafeFileHandle);
}

ArchivingUtils.AttemptSetLastWriteTime(destinationFileName, ModificationTime);
}

private FileStreamOptions CreateFileStreamOptions(bool isAsync)
{
FileStreamOptions fileStreamOptions = new()
{
Access = FileAccess.Write,
Mode = FileMode.CreateNew,
Share = FileShare.None,
PreallocationSize = Length,
Options = isAsync ? FileOptions.Asynchronous : FileOptions.None
};

if (!OperatingSystem.IsWindows())
{
const UnixFileMode OwnershipPermissions =
UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute |
UnixFileMode.GroupRead | UnixFileMode.GroupWrite | UnixFileMode.GroupExecute |
UnixFileMode.OtherRead | UnixFileMode.OtherWrite | UnixFileMode.OtherExecute;
Copy link
Member

Choose a reason for hiding this comment

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

Should we have some additional members on UnixFileMode that represent common combinations of these flags?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had suggested some as part of the API proposal. They were left out because they'd mess up 'ToString'.

These are common combinations defined in stat.h:

ACCESSPERMS   0777
DEFFILEMODE   0666
ALLPERMS     07777

cc @eerhardt

Copy link
Member

Choose a reason for hiding this comment

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

Maybe our internal usage is enough justification to add these common combinations now?

cc @bartonjs

Copy link
Member

Choose a reason for hiding this comment

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

Having the consts somewhere and having them be values in the enum are two different things.

If OwnershipPermissions were defined as-shown in the enum then having the 0777 value would ToString() not as UserRead | UserWrite | ... but as OwnershipPermissions, which gets... weird.

Putting them somewhere else as a public const doesn't impact the ToString() behavior. The best I can see would be something like File.UnixOwnershipMask.


// Restore permissions.
// For security, limit to ownership permissions, and respect umask (through UnixCreateMode).
fileStreamOptions.UnixCreateMode = Mode & OwnershipPermissions;
}

return fileStreamOptions;
}
}
}
1 change: 0 additions & 1 deletion src/libraries/System.IO.Compression.ZipFile/src/System.IO.Compression.ZipFile.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
<!-- Unix specific files -->
<ItemGroup Condition="'$(TargetPlatformIdentifier)' == ''">
<Compile Include="System\IO\Compression\ZipFileExtensions.ZipArchive.Create.Unix.cs" />
<Compile Include="System\IO\Compression\ZipFileExtensions.ZipArchiveEntry.Extract.Unix.cs" />
<Compile Include="$(CommonPath)System\IO\Compression\ZipArchiveEntryConstants.Unix.cs" />
<Compile Include="$(CommonPath)Interop\Unix\Interop.IOErrors.cs"
Link="Common\Interop\Unix\Interop.IOErrors.cs" />
Expand Down
33 changes: 0 additions & 33 deletions ...ssion.ZipFile/src/System/IO/Compression/ZipFileExtensions.ZipArchiveEntry.Extract.Unix.cs
View file
Open in desktop

This file was deleted.

30 changes: 23 additions & 7 deletions ...ompression.ZipFile/src/System/IO/Compression/ZipFileExtensions.ZipArchiveEntry.Extract.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,22 +65,38 @@ public static void ExtractToFile(this ZipArchiveEntry source, string destination
ArgumentNullException.ThrowIfNull(source);
ArgumentNullException.ThrowIfNull(destinationFileName);

// Rely on FileStream's ctor for further checking destinationFileName parameter
FileMode fMode = overwrite ? FileMode.Create : FileMode.CreateNew;
FileStreamOptions fileStreamOptions = new()
{
Access = FileAccess.Write,
Mode = overwrite ? FileMode.Create : FileMode.CreateNew,
Share = FileShare.None,
BufferSize = 0x1000
};

const UnixFileMode OwnershipPermissions =
UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute |
UnixFileMode.GroupRead | UnixFileMode.GroupWrite | UnixFileMode.GroupExecute |
UnixFileMode.OtherRead | UnixFileMode.OtherWrite | UnixFileMode.OtherExecute;

using (FileStream fs = new FileStream(destinationFileName, fMode, FileAccess.Write, FileShare.None, bufferSize: 0x1000, useAsync: false))
// Restore Unix permissions.
// For security, limit to ownership permissions, and respect umask (through UnixCreateMode).
// We don't apply UnixFileMode.None because .zip files created on Windows and .zip files created
// with previous versions of .NET don't include permissions.
UnixFileMode mode = (UnixFileMode)(source.ExternalAttributes >> 16) & OwnershipPermissions;
if (mode != UnixFileMode.None && !OperatingSystem.IsWindows())
{
fileStreamOptions.UnixCreateMode = mode;
}

using (FileStream fs = new FileStream(destinationFileName, fileStreamOptions))
{
using (Stream es = source.Open())
es.CopyTo(fs);

ExtractExternalAttributes(fs, source);
}

ArchivingUtils.AttemptSetLastWriteTime(destinationFileName, source.LastWriteTime);
}

static partial void ExtractExternalAttributes(FileStream fs, ZipArchiveEntry entry);

internal static void ExtractRelativeToDirectory(this ZipArchiveEntry source, string destinationDirectoryName) =>
ExtractRelativeToDirectory(source, destinationDirectoryName, overwrite: false);

Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.IO.Compression.ZipFile/tests/System.IO.Compression.ZipFile.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
<PropertyGroup>
<EnableLibraryImportGenerator>true</EnableLibraryImportGenerator>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<IncludeRemoteExecutor>true</IncludeRemoteExecutor>
<TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-Unix;$(NetCoreAppCurrent)-Browser</TargetFrameworks>
</PropertyGroup>

Expand Down
51 changes: 38 additions & 13 deletions src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Runtime.InteropServices;
using System.Text;
using System.Threading.Tasks;
using Microsoft.DotNet.RemoteExecutor;
using Xunit;

namespace System.IO.Compression.Tests
Expand Down Expand Up @@ -56,6 +57,16 @@ void EnsureExternalAttributes(string permissions, ZipArchiveEntry entry)
}
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public void UnixCreateSetsPermissionsInExternalAttributesUMaskZero()
{
RemoteExecutor.Invoke(() =>
{
umask(0);
new ZipFile_Unix().UnixCreateSetsPermissionsInExternalAttributes();
}).Dispose();
}

[Fact]
public void UnixExtractSetsFilePermissionsFromExternalAttributes()
{
Expand Down Expand Up @@ -90,6 +101,16 @@ public void UnixExtractSetsFilePermissionsFromExternalAttributes()
}
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public void UnixExtractSetsFilePermissionsFromExternalAttributesUMaskZero()
{
RemoteExecutor.Invoke(() =>
{
umask(0);
new ZipFile_Unix().UnixExtractSetsFilePermissionsFromExternalAttributes();
}).Dispose();
}

private static string[] CreateFiles(string folderPath, string[] testPermissions)
{
string[] expectedPermissions = new string[testPermissions.Length];
Expand Down Expand Up @@ -126,6 +147,8 @@ private static string[] CreateFiles(string folderPath, string[] testPermissions)

private static void EnsureFilePermissions(string filename, string permissions)
{
permissions = GetExpectedPermissions(permissions);

Interop.Sys.FileStatus status;
Assert.Equal(0, Interop.Sys.Stat(filename, out status));

Expand Down Expand Up @@ -199,26 +222,28 @@ await Task.WhenAll(

private static string GetExpectedPermissions(string expectedPermissions)
{
if (string.IsNullOrEmpty(expectedPermissions))
using (var tempFolder = new TempDirectory())
{
// Create a new file, and get its permissions to get the current system default permissions

using (var tempFolder = new TempDirectory())
string filename = Path.Combine(tempFolder.Path, Path.GetRandomFileName());
FileStreamOptions fileStreamOptions = new()
{
string filename = Path.Combine(tempFolder.Path, Path.GetRandomFileName());
File.WriteAllText(filename, "contents");

Interop.Sys.FileStatus status;
Assert.Equal(0, Interop.Sys.Stat(filename, out status));

expectedPermissions = Convert.ToString(status.Mode & 0xFFF, 8);
Access = FileAccess.Write,
Mode = FileMode.CreateNew
};
if (expectedPermissions != null)
{
fileStreamOptions.UnixCreateMode = (UnixFileMode)Convert.ToInt32(expectedPermissions, 8);
}
}
new FileStream(filename, fileStreamOptions).Dispose();
Copy link
Member

Choose a reason for hiding this comment

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

Are the current tests failing without this change? Before we would only create a new file when the expectedPermissions was null or empty. For the other 3 .zip files, we were straight expecting the mode hard-coded into the tests.
Now we are always creating a new file and using its mode.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. The current tests require the exact permission. Here we're determining the expected permission that takes into account the umask.

Copy link
Member Author

Choose a reason for hiding this comment

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

The changes to this test suite are to update the expected permissions so they take into account the umask.
I think they still cover what was intended.

@eerhardt is this good for you?

Copy link
Member

Choose a reason for hiding this comment

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

My concern is that we aren't testing that GroupWrite and OtherWrite bits are set correctly anymore. I think we should have tests that respect the umask (what you are fixing here) and tests that clear the umask and ensure the full permissions are kept.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added tests that run with a zero umask. ptal.


return expectedPermissions;
return Convert.ToString((int)File.GetUnixFileMode(filename), 8);
}
}

[LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)]
private static partial int mkfifo(string path, int mode);

[LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8)]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[LibraryImport("libc", StringMarshalling = StringMarshalling.Utf8)]
[LibraryImport("libc")]

No strings are being marshalled here, so no need.

This can be addressed in a different PR, if we don't want to reset CI.

private static partial int umask(int umask);
}
}