From 9a4cdce1a20da9a69de59de2d55cdc3df7ade9b7 Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Wed, 20 Apr 2022 13:07:30 -0600 Subject: [PATCH] Fix reading slightly incorrect Zip files (#68106) * Add test that fails * fix * Fix test failing unelevated * annotate * overflow * Apply suggestions from code review Co-authored-by: Robin Lindner * Apply suggestions from code review Co-authored-by: Adam Sitnik Co-authored-by: Robin Lindner Co-authored-by: Adam Sitnik --- .../src/System/IO/Compression/ZipBlocks.cs | 74 ++++--- .../zip_InvalidParametersAndStrangeFiles.cs | 193 ++++++++++++++++++ .../tests/ServiceBaseTests.cs | 2 +- 3 files changed, 235 insertions(+), 34 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 f85b1777bd7c8..44511243b5e9f 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 @@ -133,7 +133,6 @@ private void UpdateSize() // Then, the search for the zip64 record will fail because the expected size is wrong, // and a nulled out Zip64ExtraField will be returned. Thus, even though there was Zip64 data, // it will not be used. It is questionable whether this situation is possible to detect - // unlike the other functions that have try-pattern semantics, these functions always return a // Zip64ExtraField. If a Zip64 extra field actually doesn't exist, all of the fields in the // returned struct will be null @@ -183,46 +182,55 @@ private static bool TryGetZip64BlockFromGenericExtraField(ZipGenericExtraField e if (extraField.Tag != TagConstant) return false; - // this pattern needed because nested using blocks trigger CA2202 - MemoryStream? ms = null; - try - { - ms = new MemoryStream(extraField.Data); - using (BinaryReader reader = new BinaryReader(ms)) - { - ms = null; + zip64Block._size = extraField.Size; - zip64Block._size = extraField.Size; + using (MemoryStream ms = new MemoryStream(extraField.Data)) + using (BinaryReader reader = new BinaryReader(ms)) + { + // The spec section 4.5.3: + // The order of the fields in the zip64 extended + // information record is fixed, but the fields MUST + // only appear if the corresponding Local or Central + // directory record field is set to 0xFFFF or 0xFFFFFFFF. + // However tools commonly write the fields anyway; the prevailing convention + // is to respect the size, but only actually use the values if their 32 bit + // values were all 0xFF. + + if (extraField.Size < sizeof(long)) + return true; - ushort expectedSize = 0; + long value64 = reader.ReadInt64(); + if (readUncompressedSize) + zip64Block._uncompressedSize = value64; - if (readUncompressedSize) expectedSize += 8; - if (readCompressedSize) expectedSize += 8; - if (readLocalHeaderOffset) expectedSize += 8; - if (readStartDiskNumber) expectedSize += 4; + if (ms.Position > extraField.Size - sizeof(long)) + return true; - // if it is not the expected size, perhaps there is another extra field that matches - if (expectedSize != zip64Block._size) - return false; + value64 = reader.ReadInt64(); + if (readCompressedSize) + zip64Block._compressedSize = value64; - if (readUncompressedSize) zip64Block._uncompressedSize = reader.ReadInt64(); - if (readCompressedSize) zip64Block._compressedSize = reader.ReadInt64(); - if (readLocalHeaderOffset) zip64Block._localHeaderOffset = reader.ReadInt64(); - if (readStartDiskNumber) zip64Block._startDiskNumber = reader.ReadInt32(); + if (ms.Position > extraField.Size - sizeof(long)) + return true; - // original values are unsigned, so implies value is too big to fit in signed integer - if (zip64Block._uncompressedSize < 0) throw new InvalidDataException(SR.FieldTooBigUncompressedSize); - if (zip64Block._compressedSize < 0) throw new InvalidDataException(SR.FieldTooBigCompressedSize); - if (zip64Block._localHeaderOffset < 0) throw new InvalidDataException(SR.FieldTooBigLocalHeaderOffset); - if (zip64Block._startDiskNumber < 0) throw new InvalidDataException(SR.FieldTooBigStartDiskNumber); + value64 = reader.ReadInt64(); + if (readLocalHeaderOffset) + zip64Block._localHeaderOffset = value64; + if (ms.Position > extraField.Size - sizeof(int)) return true; - } - } - finally - { - if (ms != null) - ms.Dispose(); + + int value32 = reader.ReadInt32(); + if (readStartDiskNumber) + zip64Block._startDiskNumber = value32; + + // original values are unsigned, so implies value is too big to fit in signed integer + if (zip64Block._uncompressedSize < 0) throw new InvalidDataException(SR.FieldTooBigUncompressedSize); + if (zip64Block._compressedSize < 0) throw new InvalidDataException(SR.FieldTooBigCompressedSize); + if (zip64Block._localHeaderOffset < 0) throw new InvalidDataException(SR.FieldTooBigLocalHeaderOffset); + if (zip64Block._startDiskNumber < 0) throw new InvalidDataException(SR.FieldTooBigStartDiskNumber); + + return 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 848c20053fc78..ec67114972f4d 100644 --- a/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs +++ b/src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Buffers.Binary; using System.Collections.Generic; using System.Text; using System.Threading.Tasks; @@ -807,5 +808,197 @@ void VerifyValidEntry(ZipArchiveEntry entry, string expectedName, int expectedMi } } } + + /// + /// This test verifies that we can successfully read Zip archives that are "slightly incorrect" in that there is a Zip64 extra field + /// that contains space for 64-bit values which should only be present if the 32-bit fields read earlier were all 0xFF bytes. + /// Although this contravenes the Zip spec, such files are created by common tools and are successfully read by Python, Go and Rust, and + /// 7zip (albeit with a warning) + /// + [Fact] + public void ReadArchive_WithUnexpectedZip64ExtraFieldSize() + { + using ZipArchive archive = new (new MemoryStream(s_slightlyIncorrectZip64)); + ZipArchiveEntry entry = archive.GetEntry("file.txt"); + Assert.Equal(4, entry.Length); + Assert.Equal(6, entry.CompressedLength); + using var stream = entry.Open(); + using StreamReader reader = new (stream); + string text = reader.ReadToEnd(); + Assert.Equal("test", text); + } + + /// + /// As above, but the compressed size in the central directory record is less than 0xFFFFFFFF so the value in that location + /// should be used instead of in the Zip64 extra field. + /// + [Fact] + public void ReadArchive_WithUnexpectedZip64ExtraFieldSizeCompressedSizeIn32Bit() + { + byte[] input = (byte[])s_slightlyIncorrectZip64.Clone(); + BinaryPrimitives.WriteInt32LittleEndian(input.AsSpan(120), 9); // change 32-bit compressed size from -1 + + using var archive = new ZipArchive(new MemoryStream(input)); + ZipArchiveEntry entry = archive.GetEntry("file.txt"); + Assert.Equal(4, entry.Length); + Assert.Equal(9, entry.CompressedLength); // it should have used 32-bit size + } + + /// + /// As above, but the uncompressed size in the central directory record is less than 0xFFFFFFFF so the value in that location + /// should be used instead of in the Zip64 extra field. + /// + [Fact] + public void ReadArchive_WithUnexpectedZip64ExtraFieldSizeUncompressedSizeIn32Bit() + { + byte[] input = (byte[])s_slightlyIncorrectZip64.Clone(); + BinaryPrimitives.WriteInt32LittleEndian(input.AsSpan(124), 9); // change 32-bit uncompressed size from -1 + + using var archive = new ZipArchive(new MemoryStream(input)); + ZipArchiveEntry entry = archive.GetEntry("file.txt"); + Assert.Equal(9, entry.Length); + Assert.Equal(6, entry.CompressedLength); // it should have used 32-bit size + } + + private static readonly byte[] s_slightlyIncorrectZip64 = + { + // ===== Local file header signature 0x04034b50 + 0x50, 0x4b, 0x03, 0x04, + // version to extract 4.5 + 0x2d, 0x00, + // general purpose flags + 0x00, 0x08, // 0000_1000 'for enhanced deflating' + // Deflate + 0x08, 0x00, + // Last mod file time + 0x17, 0x9b, + // Last mod date + 0x6d, 0x52, + // CRC32 + 0x0c, 0x7e, 0x7f, 0xd8, + // compressed size + 0xff, 0xff, 0xff, 0xff, + // UNcompressed size + 0xff, 0xff, 0xff, 0xff, + // file name length + 0x08, 0x00, + // extra field length + 0x38, 0x00, + // filename + 0x66, 0x69, 0x6c, 0x65, 0x2e, 0x74, 0x78, 0x74, + // -----Zip64 extra field tag + 0x01, 0x00, + // size of extra field block + 0x10, 0x00, + // 8 byte Zip64 UNcompressed size, index 42 + 0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + // 8 byte Zip64 compressed size, index 50 + 0x06, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + // ----- NTFS extra field tag + 0x0a, 0x00, + // size of extra field block + 0x20, 0x00, + // reserved + 0x00, 0x00, 0x00, 0x00, + // tag #1 + 0x01, 0x00, + // size of tag #1 + 0x18, 0x00, + // Mtime, CTime, Atime + 0xa8, 0xb1, 0xf6, 0x61, 0x25, 0x18, 0xd7, 0x01, + 0xa8, 0xb1, 0xf6, 0x61, 0x25, 0x18, 0xd7, 0x01, + 0xa8, 0xb1, 0xf6, 0x61, 0x25, 0x18, 0xd7, 0x01, + // ------------- + // Data! + 0x2b, 0x49, 0x2d, 0x2e, 0x01, 0x00, + // -------- Central directory signature 0x02014b50 + 0x50, 0x4b, 0x01, 0x02, + // version made by 4.5 + 0x2d, 0x00, + // version to extract 4.5 + 0x2d, 0x00, + // general purpose flags + 0x00, 0x08, + // Deflate + 0x08, 0x00, + // Last mod file time + 0x17, 0x9b, + // Last mod date + 0x6d, 0x52, + // CRC32 + 0x0c, 0x7e, 0x7f, 0xd8, + // 4 byte compressed size, index 120 (-1 indicates refer to Zip64 extra field) + 0xff, 0xff, 0xff, 0xff, + // 4 byte UNcompressed size, index 124 (-1 indicates refer to Zip64 extra field) + 0xff, 0xff, 0xff, 0xff, + // file name length + 0x08, 0x00, + // extra field length + 0x44, 0x00, + // file comment length + 0x00, 0x00, + // disk number start (-1 indicates refer to Zip64 extra field) + 0x00, 0x00, + // internal file attributes + 0x00, 0x00, + // external file attributes + 0x00, 0x00, 0x00, 0x00, + // relative offset of local header (-1 indicates refer to Zip64 extra field) + 0x00, 0x00, 0x00, 0x00, + // file name + 0x66, 0x69, 0x6c, 0x65, 0x2e, 0x74, 0x78, 0x74, + // extra field, content similar to before + 0x01, 0x00, + 0x1c, 0x00, + 0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x06, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x0a, 0x00, + 0x20, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x01, 0x00, 0x18, 0x00, + 0xa8, 0xb1, 0xf6, 0x61, 0x25, 0x18, 0xd7, 0x01, + 0xa8, 0xb1, 0xf6, 0x61, 0x25, 0x18, 0xd7, 0x01, + 0xa8, 0xb1, 0xf6, 0x61, 0x25, 0x18, 0xd7, 0x01, + // == 'end of zip64 central directory record' signature 0x06064b50 + 0x50, 0x4b, 0x06, 0x06, + // size + 0x2c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + // version made by, version needed + 0x2d, 0x00, 0x2d, 0x00, + // disk number, disk number with CD + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + // total number of CD records + 0x01, 0x00, 0x00, 0x00, + // size of CD + 0x00, 0x00, 0x00, 0x00, + // offset of start of CD wrt starting disk + 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + // zip64 extensible data sector + 0x7a, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x64, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + // == 'zip64 end of CD locator' signature 0x07064b50 + 0x50, 0x4b, 0x06, 0x07, + // number of disk with zip64 CD + 0x00, 0x00, 0x00, 0x00, + // relative offset of zip64 ECD + 0xde, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + // total number of disks + 0x01, 0x00, 0x00, 0x00, + // == 'end of CD' signature 0x06054b50 + 0x50, 0x4b, 0x05, 0x06, + // disk number, disk number with CD (-1 indicates refer to Zip64 extra field) + 0x00, 0x00, + 0x00, 0x00, + // total number of entries in CD on this disk, and overall (-1 indicates refer to Zip64 extra fields) + 0xff, 0xff, + 0xff, 0xff, + // size of CD (-1 indicates refer to Zip64 extra field) + 0x7a, 0x00, 0x00, 0x00, + // offset of start of CD wrt start disk (-1 indicates refer to Zip64 extra field) + 0x64, 0x00, 0x00, 0x00, + // comment length + 0x00, 0x00 + }; } } diff --git a/src/libraries/System.ServiceProcess.ServiceController/tests/ServiceBaseTests.cs b/src/libraries/System.ServiceProcess.ServiceController/tests/ServiceBaseTests.cs index 6b6aff2cc70ec..03bbb7cbae523 100644 --- a/src/libraries/System.ServiceProcess.ServiceController/tests/ServiceBaseTests.cs +++ b/src/libraries/System.ServiceProcess.ServiceController/tests/ServiceBaseTests.cs @@ -73,7 +73,7 @@ private void Cleanup() } #if NETCOREAPP - [Theory] + [ConditionalTheory(nameof(IsProcessElevated))] [InlineData(-2)] [InlineData((long)int.MaxValue + 1)] public void RequestAdditionalTime_Throws_ArgumentOutOfRangeException(long milliseconds)