From 66b51b23bf5767055a28096b705d08b2410d9fe5 Mon Sep 17 00:00:00 2001 From: Buyaa Namnan Date: Fri, 7 Feb 2020 18:55:03 -0800 Subject: [PATCH 01/13] Zip data descriptor validation update and AppContext switch option --- .../System/IO/Compression/ZipArchiveEntry.cs | 2 +- .../src/System/IO/Compression/ZipBlocks.cs | 78 ++++++++++++++----- .../src/System/IO/Compression/ZipHelper.cs | 11 +++ .../zip_InvalidParametersAndStrangeFiles.cs | 24 ++++++ 4 files changed, 93 insertions(+), 22 deletions(-) diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs index 063fc39d6a0088..a3d80fb49c4c25 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs @@ -750,7 +750,7 @@ private bool IsOpenable(bool needToUncompress, bool needToLoadIntoMemory, out st } Debug.Assert(_archive.ArchiveReader != null); _archive.ArchiveStream.Seek(_offsetOfLocalHeader, SeekOrigin.Begin); - if (needToUncompress && !needToLoadIntoMemory) + if (needToUncompress && !needToLoadIntoMemory && ZipHelper.s_validateHeader) { if (!ZipLocalFileHeader.TryValidateBlock(_archive.ArchiveReader, this)) { diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs index f5028da8a27e6b..e3d6b1016c7c8a 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs @@ -482,44 +482,62 @@ public static bool TryValidateBlock(BinaryReader reader, ZipArchiveEntry entry) } else { - if (reader.BaseStream.Length < reader.BaseStream.Position + extraFieldLength + entry.CompressedLength + 4) + if (reader.BaseStream.Length < reader.BaseStream.Position + extraFieldLength + entry.CompressedLength + 12) { return false; } reader.BaseStream.Seek(extraFieldLength + entry.CompressedLength, SeekOrigin.Current); // seek to end of compressed file from which Data descriptor starts - uint dataDescriptorSignature = reader.ReadUInt32(); - bool wasDataDescriptorSignatureRead = false; - int seekSize = 0; - if (dataDescriptorSignature == DataDescriptorSignature) + uint[] buffer = new uint[6]; + // let's try to match the smallest possible structure (3 x 4) 32 bit without signature + buffer[0] = reader.ReadUInt32(); + buffer[1] = reader.ReadUInt32(); + buffer[2] = reader.ReadUInt32(); + int seekSize = 12; + + if (TestMatch(entry, DataDescriptorSignature, buffer[0], buffer[1], buffer[2])) { - wasDataDescriptorSignatureRead = true; - seekSize = 4; + compressedSize = buffer[1]; + uncompressedSize = buffer[2]; + goto MatchFound; } - bool is64bit = entry._versionToExtract >= ZipVersionNeededValues.Zip64; - seekSize += (is64bit ? 8 : 4) * 2; // if Zip64 read by 8 bytes else 4 bytes 2 times (compressed and uncompressed size) + // let's try to match the next record size (4 x 4) 32 bit with signature + buffer[3] = reader.ReadUInt32(); + seekSize += 4; + if (TestMatch(entry, buffer[0], buffer[1], buffer[2], buffer[3])) + { + compressedSize = buffer[2]; + uncompressedSize = buffer[3]; + goto MatchFound; + } - if (reader.BaseStream.Length < reader.BaseStream.Position + seekSize) + // At this point prior to trying to match 64 bit structures we need to make sure that version is high enough + if (entry._versionToExtract < ZipVersionNeededValues.Zip64) { return false; } - // dataDescriptorSignature is optional, if it was the DataDescriptorSignature we need to skip CRC 4 bytes else we can assume CRC is alreadyskipped - if (wasDataDescriptorSignatureRead) - reader.BaseStream.Seek(4, SeekOrigin.Current); - - if (is64bit) + //let's try to match the 64 bit structures 64 bit without signature + buffer[4] = reader.ReadUInt32(); + seekSize += 4; + if (TestMatch(entry, DataDescriptorSignature, buffer[0], ConvertToUlong(buffer[1], buffer[2]), ConvertToUlong(buffer[3], buffer[4]))) { - compressedSize = reader.ReadInt64(); - uncompressedSize = reader.ReadInt64(); + compressedSize = (long)ConvertToUlong(buffer[1], buffer[2]); + uncompressedSize = (long)ConvertToUlong(buffer[3], buffer[4]); + goto MatchFound; } - else + + //let's try to match the 64 bit structures 64 bit with signature + buffer[5] = reader.ReadUInt32(); + seekSize += 4; + if (TestMatch(entry, buffer[0], buffer[1], ConvertToUlong(buffer[2], buffer[3]), ConvertToUlong(buffer[4], buffer[5]))) { - compressedSize = reader.ReadInt32(); - uncompressedSize = reader.ReadInt32(); + compressedSize = (long)ConvertToUlong(buffer[2], buffer[3]); + uncompressedSize = (long)ConvertToUlong(buffer[4], buffer[5]); + goto MatchFound; } - reader.BaseStream.Seek(-seekSize - entry.CompressedLength - 4, SeekOrigin.Current); // Seek back to the beginning of compressed stream + MatchFound: reader.BaseStream.Seek(-seekSize - entry.CompressedLength - 4, SeekOrigin.Current); // Seek back to the beginning of compressed stream*/ } if (entry.CompressedLength != compressedSize) @@ -534,6 +552,24 @@ public static bool TryValidateBlock(BinaryReader reader, ZipArchiveEntry entry) return true; } + + internal static ulong ConvertToUlong(uint loverAddressValue, uint higherAddressValue) + { + return checked((ulong)loverAddressValue + (((ulong)higherAddressValue) << 32)); + } + + private static bool TestMatch(ZipArchiveEntry entry, uint suspectSignature, uint suspectCrc32, ulong suspectCompressedSize, ulong suspectUncompressedSize) + { + if (DataDescriptorSignature == suspectSignature && (ulong)entry.CompressedLength == suspectCompressedSize + && (ulong)entry.Length == suspectUncompressedSize && entry.Crc32 == suspectCrc32) + { + return true; + } + else + { + return false; + } + } } internal struct ZipCentralDirectoryFileHeader diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs index 22664cb11df881..a29b9f41c0118e 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs @@ -17,6 +17,7 @@ internal static class ZipHelper internal const int ValidZipDate_YearMax = 2107; private static readonly DateTime s_invalidDateIndicator = new DateTime(ValidZipDate_YearMin, 1, 1, 0, 0, 0); + internal static bool s_validateHeader = !GetSuppressValidation(); internal static bool RequiresUnicode(string test) { @@ -189,5 +190,15 @@ private static bool SeekBackwardsAndRead(Stream stream, byte[] buffer, out int b return true; } } + + internal static bool GetSuppressValidation() + { + if (AppContext.TryGetSwitch("System.Compression.SuppressValidation", out bool suppressValidation)) + { + return suppressValidation; + } + + return false; + } } } diff --git a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs index d910a62e0da79e..0d0073c5753ae4 100644 --- a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs +++ b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs @@ -128,6 +128,30 @@ public static async Task ZipArchiveEntry_InvalidUpdate(string zipname) } } + [Fact] + [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Fix not shipped for .NET Framework.")] + public static async Task ZipArchiveEntry_BypassValidation() + { + MemoryStream stream = await LocalMemoryStream.readAppFileAsync(zfile("normal.zip")); + PatchDataRelativeToFileName(Encoding.ASCII.GetBytes(s_tamperedFileName), stream, 8); // patch uncompressed size in file header + + using (ZipArchive archive = new ZipArchive(stream, ZipArchiveMode.Read)) + { + // Disable the flag variable using reflection as corresponding AppContext value is cached + archive.GetType().Assembly.GetType("System.IO.Compression.ZipHelper").GetField("s_validateHeader", + Reflection.BindingFlags.NonPublic | Reflection.BindingFlags.Static).SetValue(null, false); + + ZipArchiveEntry e = archive.GetEntry(s_tamperedFileName); + + using (MemoryStream ms = new MemoryStream()) + using (Stream source = e.Open()) + { + source.CopyTo(ms); + Assert.Equal(ms.Position, ms.Length); // Just making sure it was readable + } + } + } + [Fact] [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Fix not shipped for .NET Framework.")] public static async Task ZipArchiveEntry_CorruptedStream_ReadMode_CopyTo_UpToUncompressedSize() From 6d15bdbf242b97e62c20195c553d0b31e1602c99 Mon Sep 17 00:00:00 2001 From: Buyaa Namnan Date: Mon, 10 Feb 2020 13:43:29 -0800 Subject: [PATCH 02/13] Address feedback --- .../src/System/IO/Compression/ZipBlocks.cs | 23 ++++++++----- .../src/System/IO/Compression/ZipHelper.cs | 2 +- .../zip_InvalidParametersAndStrangeFiles.cs | 33 ++++++++++--------- 3 files changed, 32 insertions(+), 26 deletions(-) diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs index e3d6b1016c7c8a..8fcf8cce56b625 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs @@ -488,7 +488,7 @@ public static bool TryValidateBlock(BinaryReader reader, ZipArchiveEntry entry) } reader.BaseStream.Seek(extraFieldLength + entry.CompressedLength, SeekOrigin.Current); // seek to end of compressed file from which Data descriptor starts - uint[] buffer = new uint[6]; + Span buffer = stackalloc uint[6]; // let's try to match the smallest possible structure (3 x 4) 32 bit without signature buffer[0] = reader.ReadUInt32(); buffer[1] = reader.ReadUInt32(); @@ -521,22 +521,27 @@ public static bool TryValidateBlock(BinaryReader reader, ZipArchiveEntry entry) //let's try to match the 64 bit structures 64 bit without signature buffer[4] = reader.ReadUInt32(); seekSize += 4; - if (TestMatch(entry, DataDescriptorSignature, buffer[0], ConvertToUlong(buffer[1], buffer[2]), ConvertToUlong(buffer[3], buffer[4]))) + ulong uLongCompressedSize = ConvertToUlong(buffer[1], buffer[2]); + ulong uLongUncompressedSize = ConvertToUlong(buffer[3], buffer[4]); + if (TestMatch(entry, DataDescriptorSignature, buffer[0], uLongCompressedSize, uLongUncompressedSize)) { - compressedSize = (long)ConvertToUlong(buffer[1], buffer[2]); - uncompressedSize = (long)ConvertToUlong(buffer[3], buffer[4]); + compressedSize = (long)uLongCompressedSize; + uncompressedSize = (long)uLongUncompressedSize; goto MatchFound; } //let's try to match the 64 bit structures 64 bit with signature buffer[5] = reader.ReadUInt32(); seekSize += 4; - if (TestMatch(entry, buffer[0], buffer[1], ConvertToUlong(buffer[2], buffer[3]), ConvertToUlong(buffer[4], buffer[5]))) + uLongCompressedSize = ConvertToUlong(buffer[2], buffer[3]); + uLongUncompressedSize = ConvertToUlong(buffer[4], buffer[5]); + if (TestMatch(entry, buffer[0], buffer[1], uLongCompressedSize, uLongUncompressedSize)) { - compressedSize = (long)ConvertToUlong(buffer[2], buffer[3]); - uncompressedSize = (long)ConvertToUlong(buffer[4], buffer[5]); + compressedSize = (long)uLongCompressedSize; + uncompressedSize = (long)uLongUncompressedSize; goto MatchFound; } + MatchFound: reader.BaseStream.Seek(-seekSize - entry.CompressedLength - 4, SeekOrigin.Current); // Seek back to the beginning of compressed stream*/ } @@ -553,9 +558,9 @@ public static bool TryValidateBlock(BinaryReader reader, ZipArchiveEntry entry) return true; } - internal static ulong ConvertToUlong(uint loverAddressValue, uint higherAddressValue) + private static ulong ConvertToUlong(uint lowerAddressValue, uint higherAddressValue) { - return checked((ulong)loverAddressValue + (((ulong)higherAddressValue) << 32)); + return (ulong)lowerAddressValue + (((ulong)higherAddressValue) << 32); } private static bool TestMatch(ZipArchiveEntry entry, uint suspectSignature, uint suspectCrc32, ulong suspectCompressedSize, ulong suspectUncompressedSize) diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs index a29b9f41c0118e..f91dc239659a18 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs @@ -193,7 +193,7 @@ private static bool SeekBackwardsAndRead(Stream stream, byte[] buffer, out int b internal static bool GetSuppressValidation() { - if (AppContext.TryGetSwitch("System.Compression.SuppressValidation", out bool suppressValidation)) + if (AppContext.TryGetSwitch("System.Compression.ZipArchiveEntry.SuppressValidation", out bool suppressValidation)) { return suppressValidation; } diff --git a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs index 0d0073c5753ae4..55dc4376857291 100644 --- a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs +++ b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs @@ -6,6 +6,7 @@ using System.Text; using System.Threading.Tasks; using Xunit; +using Microsoft.DotNet.RemoteExecutor; namespace System.IO.Compression.Tests { @@ -130,26 +131,26 @@ public static async Task ZipArchiveEntry_InvalidUpdate(string zipname) [Fact] [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Fix not shipped for .NET Framework.")] - public static async Task ZipArchiveEntry_BypassValidation() + public static void ZipArchiveEntry_BypassValidation() { - MemoryStream stream = await LocalMemoryStream.readAppFileAsync(zfile("normal.zip")); - PatchDataRelativeToFileName(Encoding.ASCII.GetBytes(s_tamperedFileName), stream, 8); // patch uncompressed size in file header - - using (ZipArchive archive = new ZipArchive(stream, ZipArchiveMode.Read)) + RemoteExecutor.Invoke(() => { - // Disable the flag variable using reflection as corresponding AppContext value is cached - archive.GetType().Assembly.GetType("System.IO.Compression.ZipHelper").GetField("s_validateHeader", - Reflection.BindingFlags.NonPublic | Reflection.BindingFlags.Static).SetValue(null, false); + MemoryStream stream = populateStream().Result; + PatchDataRelativeToFileName(Encoding.ASCII.GetBytes(s_tamperedFileName), stream, 8); // patch uncompressed size in file header + using (ZipArchive archive = new ZipArchive(stream, ZipArchiveMode.Read)) + { + // Set the AppContext Switch to suppress validation + AppContext.SetSwitch("System.Compression.ZipArchiveEntry.SuppressValidation", true); + ZipArchiveEntry e = archive.GetEntry(s_tamperedFileName); - ZipArchiveEntry e = archive.GetEntry(s_tamperedFileName); - - using (MemoryStream ms = new MemoryStream()) - using (Stream source = e.Open()) - { - source.CopyTo(ms); - Assert.Equal(ms.Position, ms.Length); // Just making sure it was readable + using (MemoryStream ms = new MemoryStream()) + using (Stream source = e.Open()) + { + source.CopyTo(ms); + Assert.Equal(ms.Position, ms.Length); // Just making sure it was readable + } } - } + }).Dispose(); } [Fact] From de6925ce3fb10c3cf2059a76d1c7971936b737d2 Mon Sep 17 00:00:00 2001 From: Buyaa Namnan Date: Tue, 11 Feb 2020 16:27:56 -0800 Subject: [PATCH 03/13] Clean up DD validation, add test case for Uint bug --- .../src/System/IO/Compression/ZipBlocks.cs | 31 +++++-------------- .../tests/System.IO.Compression.Tests.csproj | 1 + .../zip_InvalidParametersAndStrangeFiles.cs | 28 ++++++++++++++++- 3 files changed, 35 insertions(+), 25 deletions(-) diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs index 8fcf8cce56b625..c5e0f5e1cd8e7e 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs @@ -489,27 +489,22 @@ public static bool TryValidateBlock(BinaryReader reader, ZipArchiveEntry entry) reader.BaseStream.Seek(extraFieldLength + entry.CompressedLength, SeekOrigin.Current); // seek to end of compressed file from which Data descriptor starts Span buffer = stackalloc uint[6]; + // let's try to match the smallest possible structure (3 x 4) 32 bit without signature buffer[0] = reader.ReadUInt32(); buffer[1] = reader.ReadUInt32(); buffer[2] = reader.ReadUInt32(); - int seekSize = 12; if (TestMatch(entry, DataDescriptorSignature, buffer[0], buffer[1], buffer[2])) { - compressedSize = buffer[1]; - uncompressedSize = buffer[2]; - goto MatchFound; + return true; } // let's try to match the next record size (4 x 4) 32 bit with signature buffer[3] = reader.ReadUInt32(); - seekSize += 4; if (TestMatch(entry, buffer[0], buffer[1], buffer[2], buffer[3])) { - compressedSize = buffer[2]; - uncompressedSize = buffer[3]; - goto MatchFound; + return true; } // At this point prior to trying to match 64 bit structures we need to make sure that version is high enough @@ -520,29 +515,17 @@ public static bool TryValidateBlock(BinaryReader reader, ZipArchiveEntry entry) //let's try to match the 64 bit structures 64 bit without signature buffer[4] = reader.ReadUInt32(); - seekSize += 4; - ulong uLongCompressedSize = ConvertToUlong(buffer[1], buffer[2]); - ulong uLongUncompressedSize = ConvertToUlong(buffer[3], buffer[4]); - if (TestMatch(entry, DataDescriptorSignature, buffer[0], uLongCompressedSize, uLongUncompressedSize)) + if (TestMatch(entry, DataDescriptorSignature, buffer[0], ConvertToUlong(buffer[1], buffer[2]), ConvertToUlong(buffer[3], buffer[4]))) { - compressedSize = (long)uLongCompressedSize; - uncompressedSize = (long)uLongUncompressedSize; - goto MatchFound; + return true; } //let's try to match the 64 bit structures 64 bit with signature buffer[5] = reader.ReadUInt32(); - seekSize += 4; - uLongCompressedSize = ConvertToUlong(buffer[2], buffer[3]); - uLongUncompressedSize = ConvertToUlong(buffer[4], buffer[5]); - if (TestMatch(entry, buffer[0], buffer[1], uLongCompressedSize, uLongUncompressedSize)) + if (TestMatch(entry, buffer[0], buffer[1], ConvertToUlong(buffer[2], buffer[3]), ConvertToUlong(buffer[4], buffer[5]))) { - compressedSize = (long)uLongCompressedSize; - uncompressedSize = (long)uLongUncompressedSize; - goto MatchFound; + return true; } - - MatchFound: reader.BaseStream.Seek(-seekSize - entry.CompressedLength - 4, SeekOrigin.Current); // Seek back to the beginning of compressed stream*/ } if (entry.CompressedLength != compressedSize) diff --git a/src/libraries/System.IO.Compression/tests/System.IO.Compression.Tests.csproj b/src/libraries/System.IO.Compression/tests/System.IO.Compression.Tests.csproj index 20bb882a1b9626..bbe391942e6b24 100644 --- a/src/libraries/System.IO.Compression/tests/System.IO.Compression.Tests.csproj +++ b/src/libraries/System.IO.Compression/tests/System.IO.Compression.Tests.csproj @@ -1,6 +1,7 @@ $(NetCoreAppCurrent)-Windows_NT;$(NetCoreAppCurrent)-Unix + true diff --git a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs index 55dc4376857291..d85ea07a1d0dd7 100644 --- a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs +++ b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs @@ -139,7 +139,7 @@ public static void ZipArchiveEntry_BypassValidation() PatchDataRelativeToFileName(Encoding.ASCII.GetBytes(s_tamperedFileName), stream, 8); // patch uncompressed size in file header using (ZipArchive archive = new ZipArchive(stream, ZipArchiveMode.Read)) { - // Set the AppContext Switch to suppress validation + // Set the AppContext Switch AppContext.SetSwitch("System.Compression.ZipArchiveEntry.SuppressValidation", true); ZipArchiveEntry e = archive.GetEntry(s_tamperedFileName); @@ -153,6 +153,32 @@ public static void ZipArchiveEntry_BypassValidation() }).Dispose(); } + [Fact] + public static async Task LargeArchive_DataDescriptor_Read_NonZip64_FileLengthGreaterThanIntMax() + { + MemoryStream stream = await LocalMemoryStream.readAppFileAsync(strange("fileLengthGreaterIntLessUInt.zip")); + + using (ZipArchive archive = new ZipArchive(stream, ZipArchiveMode.Read)) + { + ZipArchiveEntry e = archive.GetEntry("large.bin"); + + Assert.Equal(3_600_000_000, e.Length); + Assert.Equal(3_499_028, e.CompressedLength); + + using (Stream source = e.Open()) + { + byte[] buffer = new byte[s_bufferSize]; + int read = source.Read(buffer, 0, buffer.Length); // We don't want to inflate this large archive entirely + // just making sure it read successfully + Assert.Equal(s_bufferSize, read); + foreach (byte b in buffer) + { + Assert.Equal((byte)'0', b); // the file should be all "0"s + } + } + } + } + [Fact] [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Fix not shipped for .NET Framework.")] public static async Task ZipArchiveEntry_CorruptedStream_ReadMode_CopyTo_UpToUncompressedSize() From c5d4c400ab085bbf89899acefbeee5a07fb56737 Mon Sep 17 00:00:00 2001 From: Buyaa Namnan Date: Wed, 12 Feb 2020 12:41:53 -0800 Subject: [PATCH 04/13] Apply feedback --- ...ositionPreservingWriteOnlyStreamWrapper.cs | 2 +- .../src/System/IO/Compression/ZipBlocks.cs | 36 ++++++++++++++++--- .../src/System/IO/Compression/ZipHelper.cs | 2 +- .../zip_InvalidParametersAndStrangeFiles.cs | 24 +++++++++++-- 4 files changed, 55 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/PositionPreservingWriteOnlyStreamWrapper.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/PositionPreservingWriteOnlyStreamWrapper.cs index a275be1cfab3d6..0d40b542541716 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/PositionPreservingWriteOnlyStreamWrapper.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/PositionPreservingWriteOnlyStreamWrapper.cs @@ -42,7 +42,7 @@ public override void Write(ReadOnlySpan buffer) _stream.Write(buffer); } - public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback? callback, object? state) + public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback callback, object? state) { _position += count; return _stream.BeginWrite(buffer, offset, count, callback, state); diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs index c5e0f5e1cd8e7e..373a99f2e362ef 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs @@ -482,7 +482,23 @@ public static bool TryValidateBlock(BinaryReader reader, ZipArchiveEntry entry) } else { - if (reader.BaseStream.Length < reader.BaseStream.Position + extraFieldLength + entry.CompressedLength + 12) + // There are 4 distinct scenario we would like to support + // 1.based on the appnote it seems that the structure of this record is following: + // crc-32 4 bytes + // compressed size 4 bytes (scenario 1.a has 8 bytes) + // uncompressed size 4 bytes (scenario 1.a has 8 bytes) + // + // 2.based on files that we have been able to examine + // data descriptor signature 4 bytes (0x08074b50) + // crc-32 4 bytes + // compressed size 4 bytes (scenario 2.a has 8 bytes) + // uncompressed size 4 bytes (scenario 2.a has 8 bytes) + // + // we can safely assume that this record is not the last one in the file, so let's just + // read the max Bytes required to store the largest structure , and compare results + + int minDataDescriptorSize = 12; + if (reader.BaseStream.Length < reader.BaseStream.Position + extraFieldLength + entry.CompressedLength + minDataDescriptorSize) { return false; } @@ -500,7 +516,11 @@ public static bool TryValidateBlock(BinaryReader reader, ZipArchiveEntry entry) return true; } - // let's try to match the next record size (4 x 4) 32 bit with signature + // let's try to match the next record size (4 x 4) 32 bit with signature if another 4 byte is available + if (reader.BaseStream.Length < reader.BaseStream.Position + 4) + { + return false; + } buffer[3] = reader.ReadUInt32(); if (TestMatch(entry, buffer[0], buffer[1], buffer[2], buffer[3])) { @@ -513,14 +533,22 @@ public static bool TryValidateBlock(BinaryReader reader, ZipArchiveEntry entry) return false; } - //let's try to match the 64 bit structures 64 bit without signature + //let's try to match the 64 bit structures 64 bit without signature if another 4 byte is available + if (reader.BaseStream.Length < reader.BaseStream.Position + 4) + { + return false; + } buffer[4] = reader.ReadUInt32(); if (TestMatch(entry, DataDescriptorSignature, buffer[0], ConvertToUlong(buffer[1], buffer[2]), ConvertToUlong(buffer[3], buffer[4]))) { return true; } - //let's try to match the 64 bit structures 64 bit with signature + //let's try to match the 64 bit structures 64 bit with signature if another 4 byte is available + if (reader.BaseStream.Length < reader.BaseStream.Position + 4) + { + return false; + } buffer[5] = reader.ReadUInt32(); if (TestMatch(entry, buffer[0], buffer[1], ConvertToUlong(buffer[2], buffer[3]), ConvertToUlong(buffer[4], buffer[5]))) { diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs index f91dc239659a18..da90294c1aeb63 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs @@ -193,7 +193,7 @@ private static bool SeekBackwardsAndRead(Stream stream, byte[] buffer, out int b internal static bool GetSuppressValidation() { - if (AppContext.TryGetSwitch("System.Compression.ZipArchiveEntry.SuppressValidation", out bool suppressValidation)) + if (AppContext.TryGetSwitch("System.Compression.ZipArchiveEntry.SuppressHeaderValidation", out bool suppressValidation)) { return suppressValidation; } diff --git a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs index d85ea07a1d0dd7..7d76c824396ce6 100644 --- a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs +++ b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs @@ -131,16 +131,16 @@ public static async Task ZipArchiveEntry_InvalidUpdate(string zipname) [Fact] [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Fix not shipped for .NET Framework.")] - public static void ZipArchiveEntry_BypassValidation() + public static void ZipArchiveEntry_BypassValidationWhenSwitchOn() { RemoteExecutor.Invoke(() => { MemoryStream stream = populateStream().Result; PatchDataRelativeToFileName(Encoding.ASCII.GetBytes(s_tamperedFileName), stream, 8); // patch uncompressed size in file header using (ZipArchive archive = new ZipArchive(stream, ZipArchiveMode.Read)) - { + { // Set the AppContext Switch - AppContext.SetSwitch("System.Compression.ZipArchiveEntry.SuppressValidation", true); + AppContext.SetSwitch("System.Compression.ZipArchiveEntry.SuppressHeaderValidation", true); ZipArchiveEntry e = archive.GetEntry(s_tamperedFileName); using (MemoryStream ms = new MemoryStream()) @@ -153,6 +153,24 @@ public static void ZipArchiveEntry_BypassValidation() }).Dispose(); } + [Fact] + [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Fix not shipped for .NET Framework.")] + public static void ZipArchiveEntry_ValidationThrowsWhenSwitchOff() + { + RemoteExecutor.Invoke(() => + { + MemoryStream stream = populateStream().Result; + PatchDataRelativeToFileName(Encoding.ASCII.GetBytes(s_tamperedFileName), stream, 8); // patch uncompressed size in file header + using (ZipArchive archive = new ZipArchive(stream, ZipArchiveMode.Read)) + { + // Set the AppContext Switch + AppContext.SetSwitch("System.Compression.ZipArchiveEntry.SuppressHeaderValidation", false); + ZipArchiveEntry e = archive.GetEntry(s_tamperedFileName); + Assert.Throws(() => e.Open()); + } + }).Dispose(); + } + [Fact] public static async Task LargeArchive_DataDescriptor_Read_NonZip64_FileLengthGreaterThanIntMax() { From 99d1a6baa760bdc73393b18b94f7072c6600dea2 Mon Sep 17 00:00:00 2001 From: Buyaa Namnan Date: Wed, 12 Feb 2020 14:11:46 -0800 Subject: [PATCH 05/13] Revert unintended change --- .../IO/Compression/PositionPreservingWriteOnlyStreamWrapper.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/PositionPreservingWriteOnlyStreamWrapper.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/PositionPreservingWriteOnlyStreamWrapper.cs index 0d40b542541716..a275be1cfab3d6 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/PositionPreservingWriteOnlyStreamWrapper.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/PositionPreservingWriteOnlyStreamWrapper.cs @@ -42,7 +42,7 @@ public override void Write(ReadOnlySpan buffer) _stream.Write(buffer); } - public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback callback, object? state) + public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback? callback, object? state) { _position += count; return _stream.BeginWrite(buffer, offset, count, callback, state); From 6edbb5c7862e9c8cfb41539e1056526508f57e13 Mon Sep 17 00:00:00 2001 From: Buyaa Namnan Date: Wed, 12 Feb 2020 15:00:03 -0800 Subject: [PATCH 06/13] Test AppContext switch with Theory --- .../zip_InvalidParametersAndStrangeFiles.cs | 47 ++++++++----------- 1 file changed, 19 insertions(+), 28 deletions(-) diff --git a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs index 7d76c824396ce6..5affbb9a645da0 100644 --- a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs +++ b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs @@ -129,46 +129,37 @@ public static async Task ZipArchiveEntry_InvalidUpdate(string zipname) } } - [Fact] + [Theory] + [InlineData(true)] + [InlineData(false)] [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Fix not shipped for .NET Framework.")] - public static void ZipArchiveEntry_BypassValidationWhenSwitchOn() + public static void ZipArchiveEntry_AppContext_Switch_SuppresHeaderValidation(bool suppressValidation) { - RemoteExecutor.Invoke(() => + RemoteExecutor.Invoke((suppressString) => { + bool suppress = bool.Parse(suppressString); MemoryStream stream = populateStream().Result; PatchDataRelativeToFileName(Encoding.ASCII.GetBytes(s_tamperedFileName), stream, 8); // patch uncompressed size in file header using (ZipArchive archive = new ZipArchive(stream, ZipArchiveMode.Read)) { // Set the AppContext Switch - AppContext.SetSwitch("System.Compression.ZipArchiveEntry.SuppressHeaderValidation", true); + AppContext.SetSwitch("System.Compression.ZipArchiveEntry.SuppressHeaderValidation", suppress); ZipArchiveEntry e = archive.GetEntry(s_tamperedFileName); - - using (MemoryStream ms = new MemoryStream()) - using (Stream source = e.Open()) + if (suppress) { - source.CopyTo(ms); - Assert.Equal(ms.Position, ms.Length); // Just making sure it was readable + using (MemoryStream ms = new MemoryStream()) + using (Stream source = e.Open()) + { + source.CopyTo(ms); + Assert.Equal(ms.Position, ms.Length); // Just making sure it was readable + } + } + else + { + Assert.Throws(() => e.Open()); } } - }).Dispose(); - } - - [Fact] - [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Fix not shipped for .NET Framework.")] - public static void ZipArchiveEntry_ValidationThrowsWhenSwitchOff() - { - RemoteExecutor.Invoke(() => - { - MemoryStream stream = populateStream().Result; - PatchDataRelativeToFileName(Encoding.ASCII.GetBytes(s_tamperedFileName), stream, 8); // patch uncompressed size in file header - using (ZipArchive archive = new ZipArchive(stream, ZipArchiveMode.Read)) - { - // Set the AppContext Switch - AppContext.SetSwitch("System.Compression.ZipArchiveEntry.SuppressHeaderValidation", false); - ZipArchiveEntry e = archive.GetEntry(s_tamperedFileName); - Assert.Throws(() => e.Open()); - } - }).Dispose(); + }, suppressValidation.ToString()).Dispose(); } [Fact] From 7933212b0090ef2cfbe15b23b095a720460108dc Mon Sep 17 00:00:00 2001 From: Buyaa Namnan Date: Thu, 13 Feb 2020 10:34:11 -0800 Subject: [PATCH 07/13] Addressing feedback --- .../src/System/IO/Compression/ZipHelper.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs index da90294c1aeb63..fc2704f53be7d8 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs @@ -17,7 +17,7 @@ internal static class ZipHelper internal const int ValidZipDate_YearMax = 2107; private static readonly DateTime s_invalidDateIndicator = new DateTime(ValidZipDate_YearMin, 1, 1, 0, 0, 0); - internal static bool s_validateHeader = !GetSuppressValidation(); + internal static bool s_validateHeader = !GetSuppressHeaderValidation(); internal static bool RequiresUnicode(string test) { @@ -191,7 +191,7 @@ private static bool SeekBackwardsAndRead(Stream stream, byte[] buffer, out int b } } - internal static bool GetSuppressValidation() + private static bool GetSuppressHeaderValidation() { if (AppContext.TryGetSwitch("System.Compression.ZipArchiveEntry.SuppressHeaderValidation", out bool suppressValidation)) { From bfe7e72afca0c6592073524672fb2bd7aa0db9c6 Mon Sep 17 00:00:00 2001 From: Buyaa Namnan Date: Fri, 14 Feb 2020 14:12:04 -0800 Subject: [PATCH 08/13] Disable AppContext test for mono --- .../tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs index 5affbb9a645da0..038800b3046594 100644 --- a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs +++ b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs @@ -133,6 +133,7 @@ public static async Task ZipArchiveEntry_InvalidUpdate(string zipname) [InlineData(true)] [InlineData(false)] [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Fix not shipped for .NET Framework.")] + [ActiveIssue("https://github.com/dotnet/runtime/issues/32316", TestRuntimes.Mono)] public static void ZipArchiveEntry_AppContext_Switch_SuppresHeaderValidation(bool suppressValidation) { RemoteExecutor.Invoke((suppressString) => From 2ffd054a98b1ce308b027a5a48990e53ea60964b Mon Sep 17 00:00:00 2001 From: Buyaa Namnan Date: Tue, 18 Feb 2020 00:38:09 -0800 Subject: [PATCH 09/13] Address feedback --- .../src/System/IO/Compression/ZipBlocks.cs | 64 ++++++++++--------- 1 file changed, 35 insertions(+), 29 deletions(-) diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs index 373a99f2e362ef..3126bf1c85249e 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs @@ -482,20 +482,31 @@ public static bool TryValidateBlock(BinaryReader reader, ZipArchiveEntry entry) } else { - // There are 4 distinct scenario we would like to support - // 1.based on the appnote it seems that the structure of this record is following: + // Data descriptor section can be written in 4 distinct scenarios + // 1. Data descriptor signature is optional, if signature is not set and the size info written in 4 bytes: // crc-32 4 bytes - // compressed size 4 bytes (scenario 1.a has 8 bytes) - // uncompressed size 4 bytes (scenario 1.a has 8 bytes) + // compressed size 4 bytes + // uncompressed size 4 bytes // - // 2.based on files that we have been able to examine + // 2. Data descriptor signature is set and size info written in 4 bytes // data descriptor signature 4 bytes (0x08074b50) - // crc-32 4 bytes - // compressed size 4 bytes (scenario 2.a has 8 bytes) - // uncompressed size 4 bytes (scenario 2.a has 8 bytes) + // crc-32 4 bytes + // compressed size 4 bytes + // uncompressed size 4 bytes // - // we can safely assume that this record is not the last one in the file, so let's just - // read the max Bytes required to store the largest structure , and compare results + // 3. Data descriptor signature is not set and size info written in 8 bytes (Zip64) + // crc-32 4 bytes + // compressed size 8 bytes + // uncompressed size 8 bytes + // + // 4. Data descriptor signature is set and size info written in 8 bytes + // data descriptor signature 4 bytes (0x08074b50) + // crc-32 4 bytes + // compressed size 8 bytes + // uncompressed size 8 bytes + // + // If there is enough bytes available for each data descriptor size + // read the bytes required to store that structure, and compare results int minDataDescriptorSize = 12; if (reader.BaseStream.Length < reader.BaseStream.Position + extraFieldLength + entry.CompressedLength + minDataDescriptorSize) @@ -503,10 +514,11 @@ public static bool TryValidateBlock(BinaryReader reader, ZipArchiveEntry entry) return false; } - reader.BaseStream.Seek(extraFieldLength + entry.CompressedLength, SeekOrigin.Current); // seek to end of compressed file from which Data descriptor starts + // Seek to end of compressed file from which Data descriptor section starts + reader.BaseStream.Seek(extraFieldLength + entry.CompressedLength, SeekOrigin.Current); Span buffer = stackalloc uint[6]; - // let's try to match the smallest possible structure (3 x 4) 32 bit without signature + // Let's try to match the smallest possible structure without signature (3 x 4 bytes) buffer[0] = reader.ReadUInt32(); buffer[1] = reader.ReadUInt32(); buffer[2] = reader.ReadUInt32(); @@ -516,11 +528,12 @@ public static bool TryValidateBlock(BinaryReader reader, ZipArchiveEntry entry) return true; } - // let's try to match the next record size (4 x 4) 32 bit with signature if another 4 byte is available + // If another 4 bytes are available if (reader.BaseStream.Length < reader.BaseStream.Position + 4) { return false; } + // Let's try to match the next structure with signature (4 x 4 bytes) buffer[3] = reader.ReadUInt32(); if (TestMatch(entry, buffer[0], buffer[1], buffer[2], buffer[3])) { @@ -533,24 +546,24 @@ public static bool TryValidateBlock(BinaryReader reader, ZipArchiveEntry entry) return false; } - //let's try to match the 64 bit structures 64 bit without signature if another 4 byte is available + // If another 4 bytes available, let's try to match the 64 bit structure without signature if (reader.BaseStream.Length < reader.BaseStream.Position + 4) { return false; } buffer[4] = reader.ReadUInt32(); - if (TestMatch(entry, DataDescriptorSignature, buffer[0], ConvertToUlong(buffer[1], buffer[2]), ConvertToUlong(buffer[3], buffer[4]))) + if (TestMatch(entry, DataDescriptorSignature, buffer[0], AsUInt64(buffer, 1), AsUInt64(buffer, 3))) { return true; } - //let's try to match the 64 bit structures 64 bit with signature if another 4 byte is available + // If another 4 bytes available, let's try to match the 64 bit structure with signature if (reader.BaseStream.Length < reader.BaseStream.Position + 4) { return false; } buffer[5] = reader.ReadUInt32(); - if (TestMatch(entry, buffer[0], buffer[1], ConvertToUlong(buffer[2], buffer[3]), ConvertToUlong(buffer[4], buffer[5]))) + if (TestMatch(entry, buffer[0], buffer[1], AsUInt64(buffer, 2), AsUInt64(buffer, 4))) { return true; } @@ -569,22 +582,15 @@ public static bool TryValidateBlock(BinaryReader reader, ZipArchiveEntry entry) return true; } - private static ulong ConvertToUlong(uint lowerAddressValue, uint higherAddressValue) + private static ulong AsUInt64(Span bytes, int start) { - return (ulong)lowerAddressValue + (((ulong)higherAddressValue) << 32); + return (ulong)bytes[start] + (((ulong)bytes[start+1]) << 32); } - private static bool TestMatch(ZipArchiveEntry entry, uint suspectSignature, uint suspectCrc32, ulong suspectCompressedSize, ulong suspectUncompressedSize) + private static bool TestMatch(ZipArchiveEntry entry, uint suspectSignature, uint suspectCrc32, ulong suspectCompressedLength, ulong suspectUncompressedLength) { - if (DataDescriptorSignature == suspectSignature && (ulong)entry.CompressedLength == suspectCompressedSize - && (ulong)entry.Length == suspectUncompressedSize && entry.Crc32 == suspectCrc32) - { - return true; - } - else - { - return false; - } + return (DataDescriptorSignature == suspectSignature && (ulong)entry.CompressedLength == suspectCompressedLength + && (ulong)entry.Length == suspectUncompressedLength && entry.Crc32 == suspectCrc32); } } From b6b05631c241a6a1c03350a9c48db8710ac5de62 Mon Sep 17 00:00:00 2001 From: Buyaa Namnan Date: Tue, 18 Feb 2020 13:55:35 -0800 Subject: [PATCH 10/13] Address more feedback --- .../ZipArchive/zip_InvalidParametersAndStrangeFiles.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs index 038800b3046594..44bd46f3cd0615 100644 --- a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs +++ b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs @@ -134,7 +134,7 @@ public static async Task ZipArchiveEntry_InvalidUpdate(string zipname) [InlineData(false)] [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Fix not shipped for .NET Framework.")] [ActiveIssue("https://github.com/dotnet/runtime/issues/32316", TestRuntimes.Mono)] - public static void ZipArchiveEntry_AppContext_Switch_SuppresHeaderValidation(bool suppressValidation) + public static void ZipArchiveEntry_AppContext_Switch_SuppressHeaderValidation(bool suppressValidation) { RemoteExecutor.Invoke((suppressString) => { @@ -183,7 +183,10 @@ public static async Task LargeArchive_DataDescriptor_Read_NonZip64_FileLengthGre Assert.Equal(s_bufferSize, read); foreach (byte b in buffer) { - Assert.Equal((byte)'0', b); // the file should be all "0"s + if (b != '0') + { + Assert.True(false, "The file should be all 0s"); + } } } } From 9fa181b65a5b84408f6c4be050d2774419dfb17e Mon Sep 17 00:00:00 2001 From: Buyaa Namnan Date: Tue, 18 Feb 2020 14:46:17 -0800 Subject: [PATCH 11/13] Printing the byte value in case test failed --- .../tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs index 44bd46f3cd0615..6bed809efa3b0f 100644 --- a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs +++ b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs @@ -185,7 +185,7 @@ public static async Task LargeArchive_DataDescriptor_Read_NonZip64_FileLengthGre { if (b != '0') { - Assert.True(false, "The file should be all 0s"); + Assert.True(false, $"The file should be all '0's, but found '{(char)b}'"); } } } From f28f82fb3b88554614aff572d735dfbb0addb760 Mon Sep 17 00:00:00 2001 From: Buyaa Namnan Date: Thu, 20 Feb 2020 11:17:25 -0800 Subject: [PATCH 12/13] Remove local header size validaiton vs central directory and related tests --- .../System/IO/Compression/ZipArchiveEntry.cs | 17 +- .../src/System/IO/Compression/ZipBlocks.cs | 150 ------------------ .../src/System/IO/Compression/ZipHelper.cs | 11 -- .../zip_InvalidParametersAndStrangeFiles.cs | 101 ------------ 4 files changed, 3 insertions(+), 276 deletions(-) diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs index aa7d131fa19b03..866e1ab2a4feb1 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchiveEntry.cs @@ -749,21 +749,10 @@ private bool IsOpenable(bool needToUncompress, bool needToLoadIntoMemory, out st } Debug.Assert(_archive.ArchiveReader != null); _archive.ArchiveStream.Seek(_offsetOfLocalHeader, SeekOrigin.Begin); - if (needToUncompress && !needToLoadIntoMemory && ZipHelper.s_validateHeader) + if (!ZipLocalFileHeader.TrySkipBlock(_archive.ArchiveReader)) { - if (!ZipLocalFileHeader.TryValidateBlock(_archive.ArchiveReader, this)) - { - message = SR.LocalFileHeaderCorrupt; - return false; - } - } - else - { - if (!ZipLocalFileHeader.TrySkipBlock(_archive.ArchiveReader)) - { - message = SR.LocalFileHeaderCorrupt; - return false; - } + message = SR.LocalFileHeaderCorrupt; + return false; } // when this property gets called, some duplicated work if (OffsetOfCompressedData + _compressedSize > _archive.ArchiveStream.Length) diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs index 3126bf1c85249e..17655f461f595a 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs @@ -432,156 +432,6 @@ public static bool TrySkipBlock(BinaryReader reader) return true; } - public static bool TryValidateBlock(BinaryReader reader, ZipArchiveEntry entry) - { - const int OffsetToFilename = 26; // from the point after the signature - - if (reader.ReadUInt32() != SignatureConstant) - return false; - - if (reader.BaseStream.Length < reader.BaseStream.Position + OffsetToFilename) - return false; - - reader.BaseStream.Seek(2, SeekOrigin.Current); // skipping minimum version (using min version from central directory header) - uint dataDescriptorBit = reader.ReadUInt16() & (uint)ZipArchiveEntry.BitFlagValues.DataDescriptor; - reader.BaseStream.Seek(10, SeekOrigin.Current); // skipping bytes used for Compression method (2 bytes), last modification time and date (4 bytes) and CRC (4 bytes) - long compressedSize = reader.ReadUInt32(); - long uncompressedSize = reader.ReadUInt32(); - int filenameLength = reader.ReadUInt16(); - uint extraFieldLength = reader.ReadUInt16(); - - if (reader.BaseStream.Length < reader.BaseStream.Position + filenameLength + extraFieldLength) - return false; - - reader.BaseStream.Seek(filenameLength, SeekOrigin.Current); // skipping Filename - long endExtraFields = reader.BaseStream.Position + extraFieldLength; - - if (dataDescriptorBit == 0) - { - bool isUncompressedSizeInZip64 = uncompressedSize == ZipHelper.Mask32Bit; - bool isCompressedSizeInZip64 = compressedSize == ZipHelper.Mask32Bit; - Zip64ExtraField zip64; - - // Ideally we should also check if the minimumVersion is 64 bit or above, but there could zip files for which this version is not set correctly - if (isUncompressedSizeInZip64 || isCompressedSizeInZip64) - { - zip64 = Zip64ExtraField.GetJustZip64Block(new SubReadStream(reader.BaseStream, reader.BaseStream.Position, extraFieldLength), isUncompressedSizeInZip64, isCompressedSizeInZip64, false, false); - - if (zip64.UncompressedSize != null) - { - uncompressedSize = zip64.UncompressedSize.Value; - } - - if (zip64.CompressedSize != null) - { - compressedSize = zip64.CompressedSize.Value; - } - } - - reader.BaseStream.AdvanceToPosition(endExtraFields); - } - else - { - // Data descriptor section can be written in 4 distinct scenarios - // 1. Data descriptor signature is optional, if signature is not set and the size info written in 4 bytes: - // crc-32 4 bytes - // compressed size 4 bytes - // uncompressed size 4 bytes - // - // 2. Data descriptor signature is set and size info written in 4 bytes - // data descriptor signature 4 bytes (0x08074b50) - // crc-32 4 bytes - // compressed size 4 bytes - // uncompressed size 4 bytes - // - // 3. Data descriptor signature is not set and size info written in 8 bytes (Zip64) - // crc-32 4 bytes - // compressed size 8 bytes - // uncompressed size 8 bytes - // - // 4. Data descriptor signature is set and size info written in 8 bytes - // data descriptor signature 4 bytes (0x08074b50) - // crc-32 4 bytes - // compressed size 8 bytes - // uncompressed size 8 bytes - // - // If there is enough bytes available for each data descriptor size - // read the bytes required to store that structure, and compare results - - int minDataDescriptorSize = 12; - if (reader.BaseStream.Length < reader.BaseStream.Position + extraFieldLength + entry.CompressedLength + minDataDescriptorSize) - { - return false; - } - - // Seek to end of compressed file from which Data descriptor section starts - reader.BaseStream.Seek(extraFieldLength + entry.CompressedLength, SeekOrigin.Current); - Span buffer = stackalloc uint[6]; - - // Let's try to match the smallest possible structure without signature (3 x 4 bytes) - buffer[0] = reader.ReadUInt32(); - buffer[1] = reader.ReadUInt32(); - buffer[2] = reader.ReadUInt32(); - - if (TestMatch(entry, DataDescriptorSignature, buffer[0], buffer[1], buffer[2])) - { - return true; - } - - // If another 4 bytes are available - if (reader.BaseStream.Length < reader.BaseStream.Position + 4) - { - return false; - } - // Let's try to match the next structure with signature (4 x 4 bytes) - buffer[3] = reader.ReadUInt32(); - if (TestMatch(entry, buffer[0], buffer[1], buffer[2], buffer[3])) - { - return true; - } - - // At this point prior to trying to match 64 bit structures we need to make sure that version is high enough - if (entry._versionToExtract < ZipVersionNeededValues.Zip64) - { - return false; - } - - // If another 4 bytes available, let's try to match the 64 bit structure without signature - if (reader.BaseStream.Length < reader.BaseStream.Position + 4) - { - return false; - } - buffer[4] = reader.ReadUInt32(); - if (TestMatch(entry, DataDescriptorSignature, buffer[0], AsUInt64(buffer, 1), AsUInt64(buffer, 3))) - { - return true; - } - - // If another 4 bytes available, let's try to match the 64 bit structure with signature - if (reader.BaseStream.Length < reader.BaseStream.Position + 4) - { - return false; - } - buffer[5] = reader.ReadUInt32(); - if (TestMatch(entry, buffer[0], buffer[1], AsUInt64(buffer, 2), AsUInt64(buffer, 4))) - { - return true; - } - } - - if (entry.CompressedLength != compressedSize) - { - return false; - } - - if (entry.Length != uncompressedSize) - { - return false; - } - - return true; - } - private static ulong AsUInt64(Span bytes, int start) { return (ulong)bytes[start] + (((ulong)bytes[start+1]) << 32); diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs index fc2704f53be7d8..22664cb11df881 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipHelper.cs @@ -17,7 +17,6 @@ internal static class ZipHelper internal const int ValidZipDate_YearMax = 2107; private static readonly DateTime s_invalidDateIndicator = new DateTime(ValidZipDate_YearMin, 1, 1, 0, 0, 0); - internal static bool s_validateHeader = !GetSuppressHeaderValidation(); internal static bool RequiresUnicode(string test) { @@ -190,15 +189,5 @@ private static bool SeekBackwardsAndRead(Stream stream, byte[] buffer, out int b return true; } } - - private static bool GetSuppressHeaderValidation() - { - if (AppContext.TryGetSwitch("System.Compression.ZipArchiveEntry.SuppressHeaderValidation", out bool suppressValidation)) - { - return suppressValidation; - } - - return false; - } } } diff --git a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs index 6bed809efa3b0f..e4a32564e70646 100644 --- a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs +++ b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs @@ -129,40 +129,6 @@ public static async Task ZipArchiveEntry_InvalidUpdate(string zipname) } } - [Theory] - [InlineData(true)] - [InlineData(false)] - [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Fix not shipped for .NET Framework.")] - [ActiveIssue("https://github.com/dotnet/runtime/issues/32316", TestRuntimes.Mono)] - public static void ZipArchiveEntry_AppContext_Switch_SuppressHeaderValidation(bool suppressValidation) - { - RemoteExecutor.Invoke((suppressString) => - { - bool suppress = bool.Parse(suppressString); - MemoryStream stream = populateStream().Result; - PatchDataRelativeToFileName(Encoding.ASCII.GetBytes(s_tamperedFileName), stream, 8); // patch uncompressed size in file header - using (ZipArchive archive = new ZipArchive(stream, ZipArchiveMode.Read)) - { - // Set the AppContext Switch - AppContext.SetSwitch("System.Compression.ZipArchiveEntry.SuppressHeaderValidation", suppress); - ZipArchiveEntry e = archive.GetEntry(s_tamperedFileName); - if (suppress) - { - using (MemoryStream ms = new MemoryStream()) - using (Stream source = e.Open()) - { - source.CopyTo(ms); - Assert.Equal(ms.Position, ms.Length); // Just making sure it was readable - } - } - else - { - Assert.Throws(() => e.Open()); - } - } - }, suppressValidation.ToString()).Dispose(); - } - [Fact] public static async Task LargeArchive_DataDescriptor_Read_NonZip64_FileLengthGreaterThanIntMax() { @@ -254,7 +220,6 @@ public static async Task ZipArchiveEntry_CorruptedStream_ReadMode_Read_UpToUncom } [Fact] - public static void ZipArchiveEntry_CorruptedStream_EnsureNoExtraBytesReadOrOverWritten() { MemoryStream stream = populateStream().Result; @@ -390,55 +355,6 @@ public static async Task UnseekableVeryLargeArchive_DataDescriptor_Read_Zip64() } } - [Fact] - [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Fix not shipped for .NET Framework.")] - public static async Task ZipArchive_CorruptedLocalHeader_UncompressedSize_NotMatchWithCentralDirectory() - { - MemoryStream stream = await LocalMemoryStream.readAppFileAsync(zfile("normal.zip")); - - PatchDataRelativeToFileName(Encoding.ASCII.GetBytes(s_tamperedFileName), stream, 8); // patch uncompressed size in file header - - using (ZipArchive archive = new ZipArchive(stream, ZipArchiveMode.Read)) - { - ZipArchiveEntry e = archive.GetEntry(s_tamperedFileName); - Assert.Throws(() => e.Open()); - } - } - - [Fact] - [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Fix not shipped for .NET Framework.")] - public static async Task ZipArchive_CorruptedLocalHeader_CompressedSize_NotMatchWithCentralDirectory() - { - MemoryStream stream = await LocalMemoryStream.readAppFileAsync(zfile("normal.zip")); - - PatchDataRelativeToFileName(Encoding.ASCII.GetBytes(s_tamperedFileName), stream, 12); // patch compressed size in file header - - using (ZipArchive archive = new ZipArchive(stream, ZipArchiveMode.Read)) - { - ZipArchiveEntry e = archive.GetEntry(s_tamperedFileName); - Assert.Throws(() => e.Open()); - } - } - - [Fact] - [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, ".NET Framework does not allow unseekable streams.")] - public static async Task ZipArchive_Unseekable_Corrupted_FileDescriptor_NotMatchWithCentralDirectory() - { - using (var s = new MemoryStream()) - { - var testStream = new WrappedStream(s, false, true, false, null); - await CreateFromDir(zfolder("normal"), testStream, ZipArchiveMode.Create); - - PatchDataDescriptorRelativeToFileName(Encoding.ASCII.GetBytes(s_tamperedFileName), s, 8); // patch uncompressed size in file descriptor - - using (ZipArchive archive = new ZipArchive(s, ZipArchiveMode.Read)) - { - ZipArchiveEntry e = archive.GetEntry(s_tamperedFileName); - Assert.Throws(() => e.Open()); - } - } - } - [Fact] public static async Task UpdateZipArchive_AppendTo_CorruptedFileEntry() { @@ -570,23 +486,6 @@ public static async Task UpdateZipArchive_AddFileTo_ZipWithCorruptedFile() } } - private static int PatchDataDescriptorRelativeToFileName(byte[] fileNameInBytes, MemoryStream packageStream, int distance, int start = 0) - { - byte[] dataDescriptorSignature = BitConverter.GetBytes(0x08074B50); - byte[] buffer = packageStream.GetBuffer(); - int startOfName = FindSequenceIndex(fileNameInBytes, buffer, start); - int startOfDataDescriptor = FindSequenceIndex(dataDescriptorSignature, buffer, startOfName); - var startOfUpdatingData = startOfDataDescriptor + distance; - - // updating 4 byte data - buffer[startOfUpdatingData] = 0; - buffer[startOfUpdatingData + 1] = 1; - buffer[startOfUpdatingData + 2] = 20; - buffer[startOfUpdatingData + 3] = 0; - - return startOfName; - } - private static int PatchDataRelativeToFileName(byte[] fileNameInBytes, MemoryStream packageStream, int distance, int start = 0) { var buffer = packageStream.GetBuffer(); From ce2e9117e8f1bca2201ac5b2a14299691137259c Mon Sep 17 00:00:00 2001 From: Buyaa Namnan Date: Thu, 20 Feb 2020 13:21:13 -0800 Subject: [PATCH 13/13] Remove unused code --- .../src/System/IO/Compression/ZipBlocks.cs | 11 ----------- .../tests/System.IO.Compression.Tests.csproj | 1 - .../zip_InvalidParametersAndStrangeFiles.cs | 1 - 3 files changed, 13 deletions(-) diff --git a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs index 17655f461f595a..55b61889691f8b 100644 --- a/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs +++ b/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs @@ -431,17 +431,6 @@ public static bool TrySkipBlock(BinaryReader reader) return true; } - - private static ulong AsUInt64(Span bytes, int start) - { - return (ulong)bytes[start] + (((ulong)bytes[start+1]) << 32); - } - - private static bool TestMatch(ZipArchiveEntry entry, uint suspectSignature, uint suspectCrc32, ulong suspectCompressedLength, ulong suspectUncompressedLength) - { - return (DataDescriptorSignature == suspectSignature && (ulong)entry.CompressedLength == suspectCompressedLength - && (ulong)entry.Length == suspectUncompressedLength && entry.Crc32 == suspectCrc32); - } } internal struct ZipCentralDirectoryFileHeader diff --git a/src/libraries/System.IO.Compression/tests/System.IO.Compression.Tests.csproj b/src/libraries/System.IO.Compression/tests/System.IO.Compression.Tests.csproj index bbe391942e6b24..20bb882a1b9626 100644 --- a/src/libraries/System.IO.Compression/tests/System.IO.Compression.Tests.csproj +++ b/src/libraries/System.IO.Compression/tests/System.IO.Compression.Tests.csproj @@ -1,7 +1,6 @@ $(NetCoreAppCurrent)-Windows_NT;$(NetCoreAppCurrent)-Unix - true diff --git a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs index e4a32564e70646..b6f774717748e9 100644 --- a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs +++ b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs @@ -6,7 +6,6 @@ using System.Text; using System.Threading.Tasks; using Xunit; -using Microsoft.DotNet.RemoteExecutor; namespace System.IO.Compression.Tests {