Skip to content

Commit

Permalink
Fix reading slightly incorrect Zip files (dotnet#68106)
Browse files Browse the repository at this point in the history 
* Add test that fails

* fix

* Fix test failing unelevated

* annotate

* overflow

* Apply suggestions from code review

Co-authored-by: Robin Lindner <[email protected]>

* Apply suggestions from code review

Co-authored-by: Adam Sitnik <[email protected]>

Co-authored-by: Robin Lindner <[email protected]>
Co-authored-by: Adam Sitnik <[email protected]>
  • Loading branch information
@danmoseley @deeprobin
@adamsitnik @directhex
3 people authored and directhex committed Apr 21, 2022
1 parent d581835 commit 55d0cb6
Show file tree
Hide file tree
Showing 3 changed files with 235 additions and 34 deletions.
74 changes: 41 additions & 33 deletions src/libraries/System.IO.Compression/src/System/IO/Compression/ZipBlocks.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
}

Expand Down
193 changes: 193 additions & 0 deletions src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -807,5 +808,197 @@ void VerifyValidEntry(ZipArchiveEntry entry, string expectedName, int expectedMi
}
}
}

/// <summary>
/// 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)
/// </summary>
[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);
}

/// <summary>
/// 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.
/// </summary>
[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
}

/// <summary>
/// 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.
/// </summary>
[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
};
}
}
2 changes: 1 addition & 1 deletion src/libraries/System.ServiceProcess.ServiceController/tests/ServiceBaseTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 55d0cb6

Please sign in to comment.