From 0fdd9380f66fb35b8ec3171908583e173729ca37 Mon Sep 17 00:00:00 2001 From: buyaa-n Date: Thu, 20 Feb 2020 21:34:03 -0800 Subject: [PATCH] Remove local header compressed/uncomressed size validation vs central directory records (#32149) * Zip data descriptor validation update and AppContext switch option --- .../src/System/IO/Compression/ZipArchiveEntry.cs | 17 +--- .../src/System/IO/Compression/ZipBlocks.cs | 103 --------------------- .../zip_InvalidParametersAndStrangeFiles.cs | 96 ++++++------------- 3 files changed, 32 insertions(+), 184 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 e48c9e7..866e1ab 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 @@ namespace System.IO.Compression } Debug.Assert(_archive.ArchiveReader != null); _archive.ArchiveStream.Seek(_offsetOfLocalHeader, SeekOrigin.Begin); - if (needToUncompress && !needToLoadIntoMemory) + 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 f5028da..55b6188 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,109 +431,6 @@ namespace System.IO.Compression 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 - { - if (reader.BaseStream.Length < reader.BaseStream.Position + extraFieldLength + entry.CompressedLength + 4) - { - 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) - { - wasDataDescriptorSignatureRead = true; - seekSize = 4; - } - - 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) - - if (reader.BaseStream.Length < reader.BaseStream.Position + seekSize) - { - 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) - { - compressedSize = reader.ReadInt64(); - uncompressedSize = reader.ReadInt64(); - } - else - { - compressedSize = reader.ReadInt32(); - uncompressedSize = reader.ReadInt32(); - } - reader.BaseStream.Seek(-seekSize - entry.CompressedLength - 4, SeekOrigin.Current); // Seek back to the beginning of compressed stream - } - - if (entry.CompressedLength != compressedSize) - { - return false; - } - - if (entry.Length != uncompressedSize) - { - return false; - } - - return true; - } } internal struct ZipCentralDirectoryFileHeader 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 d910a62..b6f7747 100644 --- a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs +++ b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs @@ -129,6 +129,35 @@ namespace System.IO.Compression.Tests } [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) + { + if (b != '0') + { + Assert.True(false, $"The file should be all '0's, but found '{(char)b}'"); + } + } + } + } + } + + [Fact] [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Fix not shipped for .NET Framework.")] public static async Task ZipArchiveEntry_CorruptedStream_ReadMode_CopyTo_UpToUncompressedSize() { @@ -190,7 +219,6 @@ namespace System.IO.Compression.Tests } [Fact] - public static void ZipArchiveEntry_CorruptedStream_EnsureNoExtraBytesReadOrOverWritten() { MemoryStream stream = populateStream().Result; @@ -327,55 +355,6 @@ namespace System.IO.Compression.Tests } [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() { MemoryStream stream = await StreamHelpers.CreateTempCopyStream(zfile("normal.zip")); @@ -506,23 +485,6 @@ namespace System.IO.Compression.Tests } } - 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(); -- 2.7.4