Avoid Path.GetTempFileName() due to CWE 377.

[tdhintz]

Steps to reproduce

1. Static code security scan. Example:

		private static string GetTempFileName(string original, bool makeTempFile)
		{
			string result = null;

			if (original == null)
			{
				result = Path.GetTempFileName();
			}

Expected behavior

Use sufficiently random names for temporary files to prevent attacks which can predict the name of the file. For example, a Guid or crypto generated random number.

Actual behavior

Security scan failure.

Version of SharpZipLib

1.3.0

Obtained from (only keep the relevant lines)

• Package installed using NuGet

[piksel]

The referenced code is

SharpZipLib/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs

Line 4746 in 716b913

private static string GetTempFileName(string original, bool makeTempFile)

The suggested solution does not seem to corelate with the CWE. What static code analysis tool was used to produce these?
Instead I would suggest either using Path.GetRandomFileName() which doesn't create the file until we actually create the handle. The optimal solution would probably be to just not use any temporary files at all. It shouldn't be necessary really.

[tdhintz]

Veracode. Yes, avoiding temporary files altogether is a solution.

[Numpsy]

The optimal solution would probably be to just not use any temporary files at all. It shouldn't be necessary really.

If that's the 'safe' update mode code, then using temp/internediate files to do the update is the point? (not sure if just doing it in memory would work, given the attempts to roll back changes on error and such?)

[piksel]

@Numpsy I don't see how using a MemoryStream and then replacing the file when done would be any less safe... All the operations are just done on a Stream anyway.

[piksel]

I went with the easy fix for now anyway, making sure that the file updates work with a MemoryStream might need some work.

[Numpsy]

I was just looking at all the file copying logic in

SharpZipLib/src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs

Line 4646 in d0efee0

public override Stream ConvertTemporaryToFinal()

and thinking of the case where it tries to revert changes to the original file in case of error - if it was done without any temporary files at all by just opening the file and streaming the completed/updated data into it, it could be more open to corruption if it fell over hald way though the write?
Other than that, it could potentially require a lot more memory when doing small updates to massive files?

[tdhintz]

Code scan now passes for 2 of the 3 risk areas. This one is still unresolved at line 827 in TarArchive.cs.

			if (asciiTranslate && !entry.IsDirectory)
			{
				if (!IsBinary(entryFilename))
				{
					tempFileName = Path.GetTempFileName();