Adding to .zip archive removes file system access rules

[mkruijver]

When adding to a .zip archive on disk, the update does not happen in place. Instead, a temporary .zip file with the updated contents is created in a temporary directory first, which replaces the original archive after a successful write. As a result, any file system access rules set on the .zip archive that is updated are lost.

Steps to reproduce

1. Run the snippet below in a debugger

		[Test]
		[Category("Zip")]
		[Category("CreatesTempFile")]
		public void AddEntryRevertingFilePermissions()
		{
			const string TestValue = "0001000";
			string tempFile = "c:/temp/";
			Assert.IsNotNull(tempFile, "No permission to execute this test?");

			tempFile = Path.Combine(tempFile, "SharpZipTest.Zip");

			// create empty zip file
			using (ZipFile f = ZipFile.Create(tempFile))
			{
				f.BeginUpdate();
				f.CommitUpdate();
			}

			Console.WriteLine("break here and manually amend permissions for c:/temp/SharpZipTest.Zip by adding a rule");

			using (ZipFile f = new ZipFile(tempFile))
			{
				var m = new StringMemoryDataSource(TestValue);
				f.BeginUpdate();
				f.Add(m, "a.dat");
				f.CommitUpdate();
			}

			Console.WriteLine("permissions for c:/temp/SharpZipTest.Zip are reverted");
		}

2. Break in the middle and amend permissions to the file by adding an access rule.
3. Run to completion and verify that the file permission has reverted.

Expected behavior

File permissions should be retained

Actual behavior

File permissions inherited from temporary directory (e.g. C:\Users\username\AppData\Local\Temp\) are set

Version of SharpZipLib

Obtained from (only keep the relevant lines)

• Compiled from source, commit: https://github.com/icsharpcode/SharpZipLib/commit/cd5310f5b7eed595110b76a2f7ae5ee013cc50f1

Further detail

The update is finalised: (directUpdate is false) https://github.com/icsharpcode/SharpZipLib/blob/cd5310f5b7eed595110b76a2f7ae5ee013cc50f1/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs#L3132-L3143

The temporary output (pointing to a temporary directory ) is created at https://github.com/icsharpcode/SharpZipLib/blob/cd5310f5b7eed595110b76a2f7ae5ee013cc50f1/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs#L4663-L4669

The final output is created at https://github.com/icsharpcode/SharpZipLib/blob/cd5310f5b7eed595110b76a2f7ae5ee013cc50f1/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs#L4676-L4713

[Numpsy]

I think this would depend on if FileUpdateMode is set to Safe or Direct when starting the update? (e.g. 'Direct' mode would keep the original file settings,?)

[loiwo]

Incorrect file permissions are also a problem we have encountered. The problem is, that a temporary archive in the temp folder is created and MOVED to the final location in the end. I could fix this issue in changing the move to a file copy and delete. During file move from one folder to another on the same windows drive, the permissions are not inherited from folder security settings. When copy is used, the permissions are set correctly. When a file is moved to another drive, permissions are also set correctly.

[piksel]

I don't see why creating the temporary file in the destination directory would be a problem. That should probably be the default at least...

[loiwo]

https://docs.microsoft.com/en-us/troubleshoot/windows-server/windows-security/inherited-permissions-not-automatically-update

[loiwo]

I suggest to use a Copy and Delete instead of moving the temporary file to the final destination.

[piksel]

Why would that be better than creating the file in the correct place, deleting the original and renaming the new one? It seems like it would only cause additional writes for no reason.

[piksel]

The only way to really fix this is by copying the ACLs using System.Security.AccessControl, but we would rather not keep that as a dependency. By creating the temporary file in the same directory we would at least gain the inherited permissions... I guess we could also add an (opt-in) mode where the new temporary file is first copied from the old file and then truncated. This would ensure the ACLs are correct, but at the cost of an additional copy operation (which might be negligible for some, but add a substantial delay for others).

Any thoughts? @loiwo @mkruijver

@Numpsy That is true, but Direct mode is not widely supported and might lead to data corruption when failing.

[piksel]

Update: I did manage to add ACL management to the test above, and get it to pass by using the opposite of my suggestion:

1. Making a copy of the old file
2. Truncating the old file
3. Write updated version to the old file, while using the new copy as the source instead.

The only problem with this method is that File System watchers will see the modified file while it's being written, and if aborted mid way through an update it will leave an incomplete file as the "original" and a copy with some random file name next to it.

ICSharpCode.SharpZipLib.Tests.Zip.ZipFileHandling.AddEntryRevertingFilePermissions

Rules before modification: 0
Rules after explicit set: 1
Rules after modification: 1

[loiwo]

In my opinion, every additional risc that can cause integrity issues should be avoided. therefore my current solution is, that the file is copied in the end, instead of moved. in this case everything is fine, it should have the least impact, except performance of the additional copy, and that is OK for me in this case. An opt-in for this behaviour could be necessary, because of performance issues.

truncating the original file and performing operations on it, is not a very good idea, because, that can result in corrupted files.

[piksel]

@loiwo But that only covers the inherited ACLs, not the explicit ones on the file itself. And if that is the only goal, why not creating the temporary file in the same path as the original file? That has the same effect as your suggestion, but saves one copy operation.

[746F6D706572]

What about using File.Replace() when the temp file and original files are on the same volume? I believe this preserves permissions.

File.Replace(temporaryName_, fileName_, moveTempName);

[piksel]

That does seem to suggest that it merges the ACLs (given the ignoreMergeErrors param).