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

* Tar, Zip: respect umask when creating files.

* Update UnixExtractFilePermissionsCompat

* Replace const int with const UnixFileMode.

* Also run tests with zero umask

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.Unix.cs

@@ -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);
-            }
-        }
     }
 }

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.Windows.cs

@@ -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
-        }
     }
 }

src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarEntry.cs

@@ -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);
@@ -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;
+
+                // Restore permissions.
+                // For security, limit to ownership permissions, and respect umask (through UnixCreateMode).
+                fileStreamOptions.UnixCreateMode = Mode & OwnershipPermissions;
+            }
+
+            return fileStreamOptions;
+        }
     }
 }

src/libraries/System.IO.Compression.ZipFile/src/System.IO.Compression.ZipFile.csproj

@@ -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" />

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

@@ -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);
 

src/libraries/System.IO.Compression.ZipFile/tests/System.IO.Compression.ZipFile.Tests.csproj

@@ -2,6 +2,7 @@
   <PropertyGroup>
     <EnableLibraryImportGenerator>true</EnableLibraryImportGenerator>
     <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <IncludeRemoteExecutor>true</IncludeRemoteExecutor>
     <TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-Unix;$(NetCoreAppCurrent)-Browser</TargetFrameworks>
   </PropertyGroup>
 

src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Unix.cs

@@ -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
@@ -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()
         {
@@ -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];
@@ -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));
 
@@ -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();
 
-            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)]
+        private static partial int umask(int umask);
     }
 }