How to properly set umask in TarEntry and ZipFileExtensions

[carlossanlop]

This question is related to one of the items in the tar pending feedback issue: #68230

In the initial Tar implementation PR, @tmds pointed out here and here that the code setting the mode when extracting files in Unix was incorrect because the umask is being ignored.

The proposed solution would be to call the SafeFileHandle.Open() static method that takes an Interop.Sys.Permissions argument. The problem is that this method is internal. The only places where we call it are in File, FileStream and FileSystem.Unix.cs, which all reside in System.Private.CoreLib.

I examined that Open method, and when it calls SafeFileHandle.Init here, that is when we interact with the umask, which we consume here.

I also noticed that the code in ZipFileExtensions.ZipArchiveEntry.Extract.Unix.cs has the same problem of ignoring the umask (cc @eerhardt).

So I have the following questions:

1. Why do we use the hardcoded Interop.Sys.Permissions.Mask in SafeFileHandle, instead of the one desired by the user, which according to various documentation sources (like this one) is usually set in the .profile file or in /etc/profile?

2. If we all think it is the right thing to use that hardcoded mask, can I simply just do the following for both TarEntry and ZipFileExtensions?:

Interop.CheckIo(
    Interop.Sys.FChMod(fs.SafeFileHandle, permissions & (int)Interop.Sys.Permissions.Mask),
    fs.Name);

3. @tmds had a question here about the if (permisions != 0). This code is identical to the one in ZipFileExtensions. Do we need that protection at all? @eerhardt would there be unexpected behavior if we remove that if?

[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

---

This question is related to one of the items in the tar pending feedback issue: #68230

In the initial Tar implementation PR, @tmds pointed out here and here that the code setting the mode when extracting files in Unix was incorrect because the umask is being ignored.

The proposed solution would be to call the SafeFileHandle.Open() static method that takes an Interop.Sys.Permissions argument. The problem is that this method is internal. The only places where we call it are in File, FileStream and FileSystem.Unix.cs, which all reside in System.Private.CoreLib.

I examined that Open method, and when it calls SafeFileHandle.Init here, that is when we interact with the umask, which we consume here.

I also noticed that the code in ZipFileExtensions.ZipArchiveEntry.Extract.Unix.cs has the same problem of ignoring the umask (cc @eerhardt).

So I have the following questions:

1. Why do we use the hardcoded Interop.Sys.Permissions.Mask in SafeFileHandle, instead of the one desired by the user, which according to documentation, is usually set in the .profile file or in /etc/profile?

2. If we all think it is the right thing to use that hardcoded mask, can I simply just do the following for both TarEntry and ZipFileExtensions?:

Interop.CheckIo(
    Interop.Sys.FChMod(fs.SafeFileHandle, permissions & (int)Interop.Sys.Permissions.Mask),
    fs.Name);

3. @tmds had a question here about the if (permisions != 0). This code is identical to the one in ZipFileExtensions. Do we need that protection at all? @eerhardt would there be unexpected behavior if we remove that if?

Author:	carlossanlop
Assignees:	carlossanlop
Labels:	question, area-System.IO.Compression
Milestone:	7.0.0

[tmds]

In the initial Tar implementation PR, @tmds pointed out #67883 (comment) and #67883 (comment) that the code setting the mode when extracting files in Unix was incorrect because the umask is being ignored.

The umask protects the user from unintentional opening up files/directories to other users.

The tar command has a flag to apply the exact permissions (-p, --preserve-permissions).

The proposed solution would be to call the SafeFileHandle.Open() static method that takes an Interop.Sys.Permissions argument. The problem is that this method is internal. The only places where we call it are in File, FileStream and FileSystem.Unix.cs, which all reside in System.Private.CoreLib.

#67837 will provide a way to do this.

Why do we use the hardcoded Interop.Sys.Permissions.Mask in SafeFileHandle, instead of the one desired by the user, which according to various documentation sources (like this one) is usually set in the .profile file or in /etc/profile?

The kernel tracks the process umask. It gets applied automatically to the mask specified by the user on specific calls like open and mkdir.

It doesn't get used for calls like chmod and fchmod.

If we all think it is the right thing to use that hardcoded mask, can I simply just do the following for both TarEntry and ZipFileExtensions?:

imo, we should have the same behavior as tar for the same reasons.

@tmds had a question #67883 (comment) about the if (permisions != 0). This code is identical to the one in ZipFileExtensions. Do we need that protection at all? @eerhardt would there be unexpected behavior if we remove that if?

The rationale is provided in a comment:

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

Lines 20 to 26 in 0dba0ee

	// 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)
	{
	Interop.CheckIo(Interop.Sys.FChMod(fs.SafeFileHandle, permissions), fs.Name);
	}

[carlossanlop]

Thanks for the additional explanation, @tmds. I'll wait for the new API #67837 to be added to continue working on this.

[carlossanlop]

@tmds I see that you mentioned your PR #71647 would address this, not fix it. Is there anything else we should fix in Tar, in your opinion?

[tmds]

I should have used "fixes" in the message. We can close this when that PR is merged.