Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

skip CRC calculation for AES zip entries #554

Merged
merged 1 commit into from Feb 6, 2021
Merged

skip CRC calculation for AES zip entries #554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 13 additions & 4 deletions src/ICSharpCode.SharpZipLib/Zip/ZipFile.cs
View file
Open in desktop
Expand Up @@ -960,6 +960,9 @@ public bool TestArchive(bool testData, TestStrategy strategy, ZipTestResultHandl

if (testing && testData && this[entryIndex].IsFile)
{
// Don't check CRC for AES encrypted archives
var checkCRC = this[entryIndex].AESKeySize == 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should whether the CRC is checked on read depend on the whether the entry is AE-1 or AE-2, rather than just being sklpped for all AES entries? (e.g. dependant the _aesVer value in ZipEntry)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could, but then it would fail on archives that would otherwise be perfectly OK. The reason for it being dropped in AE-2 is that it's not necessary in AE-1.
I don't think TestArchive should fail because the file is technically out of spec, even though it's fully readable. This method is used in our own tests as well, but it's "main" purpose is for the consumer to verify that an archive is valid before attempting extraction etc.


if (resultHandler != null)
{
status.SetOperation(TestOperation.EntryData);
Expand All @@ -975,7 +978,10 @@ public bool TestArchive(bool testData, TestStrategy strategy, ZipTestResultHandl
int bytesRead;
while ((bytesRead = entryStream.Read(buffer, 0, buffer.Length)) > 0)
{
crc.Update(new ArraySegment<byte>(buffer, 0, bytesRead));
if (checkCRC)
{
crc.Update(new ArraySegment<byte>(buffer, 0, bytesRead));
}

if (resultHandler != null)
{
Expand All @@ -986,7 +992,7 @@ public bool TestArchive(bool testData, TestStrategy strategy, ZipTestResultHandl
}
}

if (this[entryIndex].Crc != crc.Value)
if (checkCRC && this[entryIndex].Crc != crc.Value)
{
status.AddError();

Expand All @@ -1000,7 +1006,8 @@ public bool TestArchive(bool testData, TestStrategy strategy, ZipTestResultHandl
var helper = new ZipHelperStream(baseStream_);
var data = new DescriptorData();
helper.ReadDataDescriptor(this[entryIndex].LocalHeaderRequiresZip64, data);
if (this[entryIndex].Crc != data.Crc)

if (checkCRC && this[entryIndex].Crc != data.Crc)
{
status.AddError();
resultHandler?.Invoke(status, "Descriptor CRC mismatch");
Expand Down Expand Up @@ -2687,6 +2694,8 @@ private void AddEntry(ZipFile workFile, ZipUpdate update)
}
}

var useCrc = update.Entry.AESKeySize == 0;

if (source != null)
{
using (source)
Expand All @@ -2711,7 +2720,7 @@ private void AddEntry(ZipFile workFile, ZipUpdate update)

using (Stream output = workFile.GetOutputStream(update.OutEntry))
{
CopyBytes(update, output, source, sourceStreamLength, true);
CopyBytes(update, output, source, sourceStreamLength, useCrc);
}

long dataEnd = workFile.baseStream_.Position;
Expand Down
4 changes: 2 additions & 2 deletions src/ICSharpCode.SharpZipLib/Zip/ZipHelperStream.cs
View file
Open in desktop
Expand Up @@ -565,7 +565,7 @@ public int WriteDataDescriptor(ZipEntry entry)
if ((entry.Flags & (int)GeneralBitFlags.Descriptor) != 0)
{
// The signature is not PKZIP originally but is now described as optional
// in the PKZIP Appnote documenting trhe format.
// in the PKZIP Appnote documenting the format.
WriteLEInt(ZipConstants.DataDescriptorSignature);
WriteLEInt(unchecked((int)(entry.Crc)));

Expand Down Expand Up @@ -599,7 +599,7 @@ public void ReadDataDescriptor(bool zip64, DescriptorData data)
int intValue = ReadLEInt();

// In theory this may not be a descriptor according to PKZIP appnote.
// In practise its always there.
// In practice its always there.
if (intValue != ZipConstants.DataDescriptorSignature)
{
throw new ZipException("Data descriptor signature not found");
Expand Down
38 changes: 25 additions & 13 deletions src/ICSharpCode.SharpZipLib/Zip/ZipOutputStream.cs
View file
Open in desktop
Expand Up @@ -500,6 +500,9 @@ public void PutNextEntry(ZipEntry entry)
/// <summary>
/// Closes the current entry, updating header and footer information as required
/// </summary>
/// <exception cref="ZipException">
/// Invalid entry field values.
/// </exception>
/// <exception cref="System.IO.IOException">
/// An I/O error occurs.
/// </exception>
Expand Down Expand Up @@ -530,7 +533,7 @@ public void CloseEntry()
}
else if (curMethod == CompressionMethod.Stored)
{
// This is done by Finsh() for Deflated entries, but we need to do it
// This is done by Finish() for Deflated entries, but we need to do it
// ourselves for Stored ones
base.GetAuthCodeIfAES();
}
Expand All @@ -539,6 +542,19 @@ public void CloseEntry()
if (curEntry.AESKeySize > 0)
{
baseOutputStream_.Write(AESAuthCode, 0, 10);
// Always use 0 as CRC for AE-2 format
curEntry.Crc = 0;
}
else
{
if (curEntry.Crc < 0)
{
curEntry.Crc = crc.Value;
}
else if (curEntry.Crc != crc.Value)
{
throw new ZipException($"crc was {crc.Value}, but {curEntry.Crc} was expected");
}
}

if (curEntry.Size < 0)
Expand All @@ -547,7 +563,7 @@ public void CloseEntry()
}
else if (curEntry.Size != size)
{
throw new ZipException("size was " + size + ", but I expected " + curEntry.Size);
throw new ZipException($"size was {size}, but {curEntry.Size} was expected");
}

if (curEntry.CompressedSize < 0)
Expand All @@ -556,16 +572,7 @@ public void CloseEntry()
}
else if (curEntry.CompressedSize != csize)
{
throw new ZipException("compressed size was " + csize + ", but I expected " + curEntry.CompressedSize);
}

if (curEntry.Crc < 0)
{
curEntry.Crc = crc.Value;
}
else if (curEntry.Crc != crc.Value)
{
throw new ZipException("crc was " + crc.Value + ", but I expected " + curEntry.Crc);
throw new ZipException($"compressed size was {csize}, but {curEntry.CompressedSize} expected");
}

offset += csize;
Expand Down Expand Up @@ -718,7 +725,12 @@ public override void Write(byte[] buffer, int offset, int count)
throw new ArgumentException("Invalid offset/count combination");
}

crc.Update(new ArraySegment<byte>(buffer, offset, count));
if (curEntry.AESKeySize == 0)
{
// Only update CRC if AES is not enabled
crc.Update(new ArraySegment<byte>(buffer, offset, count));
}

size += count;

switch (curMethod)
Expand Down
7 changes: 6 additions & 1 deletion test/ICSharpCode.SharpZipLib.Tests/Zip/FastZipHandling.cs
View file
Open in desktop
Expand Up @@ -193,7 +193,12 @@ public void Encryption(ZipEncryptionMethod encryptionMethod)
ZipEntry entry = zf[0];
Assert.AreEqual(tempName1, entry.Name);
Assert.AreEqual(1, entry.Size);
Assert.IsTrue(zf.TestArchive(true));
Assert.IsTrue(zf.TestArchive(true, TestStrategy.FindFirstError, (status, message) =>
{
if(!string.IsNullOrEmpty(message)) {
Console.WriteLine($"{message} ({status.Entry?.Name ?? "-"})");
}
}));
Assert.IsTrue(entry.IsCrypted);

switch (encryptionMethod)
Expand Down