Make the archives created on Unix have the read permissions by default

[derwasp]

Fixes #17912

Proposed changes

• When creating a ZipArchiveEntry in the Unix environment, always set the default file permissions to be 644 and directory permissions to be 755 in order for the archives to be usable after running an unzip command on them.
• Always override the default permissions of the ZipArchiveEntry when the archive is created from the actual files and not from memory, where the permissions are not available.

Background

Recently we hit an issue where our customers saw that the artifact files in the zip archives that we produce have the permissions set to 000 if our system runs on Unix machines. When the unzip command is used, these permissions translate to the actual files also having the same permissions.
This was an inconvenience to them as if the same system is run on Windows, the files would have the 644 permissions. The customers wanted this to have the same behavior.

Causes

Based on how I see it, the zip archives have two fields that can be responsible for the file permissions.

Version made by

One of them is Version made by. (See the Central directory file header section in the wiki page ) This is what the comment says about this field in the code:

The upper byte of the "version made by" flag in the central directory header of a zip file represents the
OS of the system on which the zip was created. Any zip created with an OS byte not equal to Windows (0)
or Unix (3) will be treated as equal to the current OS.
The value of 0 more specifically corresponds to the FAT file system while NTFS is assigned a higher value. However
for historical and compatibility reasons, Windows is always assigned a 0 value regardless of file system.

So to simplify the story, we can say that we have two possible options here: 0 for Windows and 3 for Unix.

This field is written here: ZipArchiveEntry.cs#L517

The unzipping software is using this field to determine that the permissions were present in the source system. In other words: we probably need to set the permissions to 644 on the Unix system if the archive was created on Windows, where these permissions don't exist. And this is exactly the behavior with unzip that we see.

External attributes

Another field is External attributes. This field is used to store in the file permissions when the archives are created on the Unix systems. Recently, the API that works with files received support for this: PR.
As you can see from the PR, the permissions are based on the actual permissions from the files:

    static partial void SetExternalAttributes(FileStream fs, ZipArchiveEntry entry)
    {
        Interop.Sys.FileStatus status;
        Interop.CheckIo(Interop.Sys.FStat(fs.SafeFileHandle, out status), fs.Name);

        entry.ExternalAttributes |= status.Mode << 16;
    }

So now from the perspective of the unzipping software if we have a Version made by set to 3 (meaning Unix), we need to read the External attributes and apply the corresponding permissions to the files. And if Version made by is set to 0 (meaning Windows) we need determine the permissions ourself and probably set it to 644.

The issue

If the file API is used, then everything already works: the permissions are preserved and when the unpacking software is going to work with the archive it would create the files with same permissions as before.
The caviat is that when we are creating a new ZipArchiveEntry from memory on the Unix machines. In this case, there's no file to begin with, but this new ZipArchiveEntry can still be unzipped using other software, such as unzip, which would follow the logic based on the Version made by flag.

When this happens, currently, we would create an archive which, when unzipped, would result in files having 000 permissions if our zipping logic runs on Windows and 644 (probably depends on the implementation of the unzipping software) if our zipping logic runs on Unix.
As noted by @tmds we should also set the permissions to 755 for the directories as in Unix the execute permissions is required to cd into the folder.

Even if this is not techincally a bug, at least, this behavior is not consistent across the two platforms from the users point of view.

A minimal repro looks like this:

using System.IO.Compression;

using var fileStream = new FileStream(@"test.zip", FileMode.Create);
using var archive = new ZipArchive(fileStream, ZipArchiveMode.Create, true);

ZipArchiveEntry demoFile = archive.CreateEntry("inmemory_file.txt");

using var entryStream = demoFile.Open();
using var streamWriter = new StreamWriter(entryStream);
streamWriter.Write("Hello zip!");

Console.WriteLine("Done.");

After running this on a Unix machine and unizipping the result with unzip test.zip you can do an ls -la and you would see something like this:

 xxxx@xxxx  ~/Projects/ConsoleApp1/ConsoleApp1/bin/Debug/net6.0  ls -la
drwxr-xr-x  10 derwasp  staff     320 Jun 14 14:48 .
drwxr-xr-x   3 derwasp  staff      96 Jun 13 16:42 ..
-rwxr-xr-x   1 derwasp  staff  132208 Jun 14 14:48 ConsoleApp1
-rw-r--r--   1 derwasp  staff     403 Jun 14 14:48 ConsoleApp1.deps.json
-rw-r--r--   1 derwasp  staff    5120 Jun 14 14:48 ConsoleApp1.dll
-rw-r--r--   1 derwasp  staff   10480 Jun 14 14:48 ConsoleApp1.pdb
-rw-r--r--   1 derwasp  staff     139 Jun 14 14:48 ConsoleApp1.runtimeconfig.json
----------   1 derwasp  staff      10 Jun 14 14:48 inmemory_file.txt
drwxr-xr-x   3 derwasp  staff      96 Jun 14 14:48 ref
-rw-r--r--   1 derwasp  staff     150 Jun 14 14:48 test.zip

And you can see that the file permissions are set to 000.

Links

The original comment is here: #17912 (comment)

[dnfadmin]

All CLA requirements met.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes #17912

I will make this description proper once we finalize the approach, but for now the discussion is happening here: More details in this comment: #17912 (comment)

Author:	derwasp
Assignees:	-
Labels:	area-System.IO.Compression
Milestone:	-

[tmds]

Is something needed to ensure proper directory permissions?

[derwasp]

@tmds I used my example from the issue comment and modified it to have a line that adds an archive folder like so:

using System.IO.Compression;
using ConsoleApp1;

using var fileStream = new FileStream(@"test.zip", FileMode.Create);
using var archive = new ZipArchive(fileStream, ZipArchiveMode.Create, true);

archive.CreateEntry("folder/").EnsureReadPermissions();
ZipArchiveEntry demoFile = archive.CreateEntry("inmemory_file.txt").EnsureReadPermissions();

using var entryStream = demoFile.Open();
using var streamWriter = new StreamWriter(entryStream);
streamWriter.Write("Hello zip!");

Console.WriteLine("Done.");

After running this and unzipping this is the result:

derwasp@xxxxx  ~/RiderProjects/ConsoleApp1/ConsoleApp1/bin/Debug/net6.0/out  ls -la
    drwxr-xr-x   4 derwasp  staff  128 Jun 22 09:23 .
    drwxr-xr-x  11 derwasp  staff  352 Jun 22 09:23 ..
    drw-r--r--   2 derwasp  staff   64 Jun 22 09:22 folder           <-- permissions
    -rw-r--r--   1 derwasp  staff   10 Jun 22 09:22 inmemory_file.txt

And this is the result if I don't see the permissions to the folder:

derwasp@xxxxx  ~/RiderProjects/ConsoleApp1/ConsoleApp1/bin/Debug/net6.0/out  ls -la
    drwxr-xr-x   4 derwasp  staff  128 Jun 22 09:23 .
    drwxr-xr-x  11 derwasp  staff  352 Jun 22 09:23 ..
    d---------   2 derwasp  staff   64 Jun 22 09:23 folder           <-- permissions
    -rw-r--r--   1 derwasp  staff   10 Jun 22 09:23 inmemory_file.txt

So this logic should fix the permissions for both, files and folders.

[tmds]

@derwasp thanks for checking.

For directories, we'll want to set the x bit. That is: use rwxr-xr-x. Otherwise a user can't cd into the extracted folders.

[derwasp]

@tmds oh this is a very good catch, I didn't know that r is not enough for the directories!

[tmds]

@carlossanlop similar to the change here, we probably need a default for directories also in the Tar implementation.

[tmds]

Looking at this PR made me realize Zip doesn't store/restore directory permissions.
I've created a ticket for that: #71140.

[tmds]

@derwasp can you add a test that verifies the default attributes are used?

[derwasp]

@tmds I made a test to verify the file permissions. Please see if you have any more suggestions.
And thank you for your help with this PR.

[danmoseley]

and check folder is 755?

[derwasp]

Ideally, yes. But with the current unzipping implementation, we can't do that: the folders are just created using Directory.CreateDirectory . When unzip is used, the permissions are fully respected of course.
IMO changing the unzipping logic fits this issue better:

• Zip and Tar don't (re)store directory permissions #71140

EDIT
Just to clarify: the way it currently works already sets enough permissions on the folders so that the files are acccessible, so the original issue described in #17912 is not affecting us here. However, these are not the correct exact permissions which we set in the archive.

[tmds]

We can reduce what these tests are doing to: creating a ZipArchiveEntry, and verifying ExternalAttributes has the expected value.
That will also work for directories.

[derwasp]

What do you say if I will just add a separate test to verify this for both directories and files? I would still love to have the test I wrote to test the roundtrip.

[tmds]

The existing UnixExtractSetsFilePermissionsFromExternalAttributes verifies ExternalAttributes gets stored and restored.

[derwasp]

@tmds I made a very simple test instead of the one I had previously. Is this what you had in mind?

[tmds]

@derwasp can you use the UnixFileMode that got just merged, and use the same style as:

runtime/src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarHelpers.cs

Lines 23 to 24 in b869f40

	internal const UnixFileMode DefaultMode = // 644 in octal
	UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.GroupRead | UnixFileMode.OtherRead;

@carlossanlop @eerhardt @danmoseley should we try to put these consts in a file common to the Tar and Zip implementations to ensure they use the same values?

[derwasp]

@tmds Yeah, I just saw that your PR went in, awesome timing! Looking at the API right now.

[tmds]

I'm curious. What happens when you extract the ZipVersionMadeByPlatform.Windows file on Unix using the system tools. What permissions do you get for files, and for directories?

[eerhardt]

runtime/src/libraries/System.IO.Compression.ZipFile/src/System/IO/Compression/ZipFileExtensions.ZipArchiveEntry.Extract.Unix.cs

Lines 20 to 26 in ddc4f95

	// If the permissions weren't set at all, don't write the file's permissions,
	// since the .zip could have been made using a previous version of .NET, which didn't
	// include the permissions, or was made on Windows.
	if (permissions != 0)
	{
	#pragma warning disable CA1416 // Validate platform compatibility
	File.SetUnixFileMode(fs.SafeFileHandle, (UnixFileMode)permissions);

[derwasp]

@tmds it took me a while to test this: was trying to understand how to set up the dev environment on my old windows laptop.

Here's the result unpacked on a mac machine:

 derwasp@xxxxx  ~/Downloads/ziptest > unzip archive_created_on_win.zip -d win_arch
Archive:  archive_created_on_win.zip
   creating: win_arch/folder/
  inflating: win_arch/folder/inmemory_file.txt
 derwasp@xxxxx  ~/Downloads/ziptest > cd win_arch
 derwasp@xxxxx  ~/Downloads/ziptest/win_arch > ls -la
total 0
drwxr-xr-x   3 derwasp  staff   96 Jun 24 22:49 .
drwxr-xr-x  14 derwasp  staff  448 Jun 24 22:49 ..
drwxr-xr-x   3 derwasp  staff   96 Jun 24 13:45 folder
 derwasp@xxxxx  ~/Downloads/ziptest/win_arch > cd folder
 derwasp@xxxxx  ~/Downloads/ziptest/win_arch/folder > ls -la
total 8
drwxr-xr-x  3 derwasp  staff  96 Jun 24 13:45 .
drwxr-xr-x  3 derwasp  staff  96 Jun 24 22:49 ..
-rw-r--r--  1 derwasp  staff  56 Jun 24 13:45 inmemory_file.txt

I also verified that the default values are not set like so:

var arch = "/Users/derwasp/Downloads/ziptest/archive_created_on_win.zip";

await using (var fileStream = new FileStream(arch, FileMode.Open))
using (var archive = new ZipArchive(fileStream, ZipArchiveMode.Read, true))
{
    foreach (var e in archive.Entries)
    {
        Console.WriteLine($"{e.FullName}, \t\t attributes = {(e.ExternalAttributes >> 16)}");

    }
}

And this is the output:

folder/,                 attributes = 0
folder/inmemory_file.txt,                attributes = 0

So the unzip utility does it the way that we expected: 644 for the files and 755 for the folders.

[tmds]

@eerhardt if you're adding stuff from file system, you go through a function that replaces Path.DirectorySeparatorChar and Path.AltDirectorySeparatorChar with /:

runtime/src/libraries/Common/src/System/IO/Archiving.Utils.cs

Lines 41 to 49 in 0f75a02

	// '/' is a more broadly recognized directory separator on all platforms (eg: mac, linux)
	// We don't use Path.DirectorySeparatorChar or AltDirectorySeparatorChar because this is
	// explicitly trying to standardize to '/'
	for (int i = 0; i < length; i++)
	{
	char ch = buffer[i];
	if (ch == Path.DirectorySeparatorChar || ch == Path.AltDirectorySeparatorChar)
	buffer[i] = PathSeparatorChar;
	}

On Linux, \ is a filename char, and we're replacing it there. Maybe we shouldn't?

For entries created using ZipArchive.CreateEntry we're not performing any substitutions.
Maybe we assume the user of this API uses proper separators?
Should we limit to the Path.DirectorySeparatorChar for this check?

[eerhardt]

Should we limit to the Path.DirectorySeparatorChar for this check?

On Unix, both Path.DirectorySeparatorChar and Path.AltDirectorySeparatorChar are /.

[tmds]

Ah, the checks are fine then.

The only thing to consider is also to the substitution on the ZipArchive.CreateEntry, so that on Windows, \ gets replaced by /.

[tmds]

lgtm, thanks @derwasp!

[adamsitnik]

/azp run runtime-extra-platforms

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[eerhardt]

LGTM - thank you @derwasp for the contribution!

[adamsitnik]

LGTM, thank you for your contribution @derwasp !

[derwasp]

Thank you for merging this in and for your help @tmds @eerhardt @adamsitnik :)

[danmoseley]

@derwasp we'd welcome another contribution if you're interested!
https://github.com/dotnet/runtime/issues?q=is%3Aopen+is%3Aissue+label%3Aup-for-grabs