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.

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

In ZipOutputStream.PutNextEntry, account for AES overhead when calculating compressed entry size #460

Merged
merged 2 commits into from
Aug 7, 2020

Conversation

Numpsy
Copy link
Contributor

@Numpsy Numpsy commented May 11, 2020

Found when looking at #454:

The code at:

WriteLeInt(entry.IsCrypted ? (int)entry.CompressedSize + ZipConstants.CryptoHeaderSize : (int)entry.CompressedSize);

for writing entries when the header information is known tries to take account of the crypto header overhead, but only does so for ZipCrypto encrypted entries - it uses the wrong values for AES entries.

Also, the code for building the Zip64 extra data at:

ed.AddLeLong(entry.CompressedSize);

doesn't take account of that overhead at all, which seems to also produce incorrect archives.

The

// TODO: Refactor header writing.  Its done in several places.

becomes a bit more true with the simple change, as it duplicates a bit more logic that could perhaps do with being shared.

PR adds a set of test cases for different encryption/compression methds, with and without zip64, but only for 0 byte entries - something with populated entries whose size and crc are known upfront might be useful seperately.

I certify that I own, and have sufficient rights to contribute, all source code and related material intended to be compiled or integrated with the source code for the SharpZipLib open source product (the "Contribution"). My Contribution is licensed under the MIT License.

@Numpsy Numpsy added encryption tests zip Related to ZIP file format labels May 12, 2020
@Numpsy
Copy link
Contributor Author

Numpsy commented May 18, 2020

Updated to move some of the duplicated overhead size calculation logic to an 'EncryptionOverheadSize' property on ZipEntry, and also changed ZipEntry.LocalHeaderRequiresZip64 to use that instead of always using the ZipCrypto constant.

@Numpsy
Copy link
Contributor Author

Numpsy commented May 18, 2020

A small observation about the length calculation logic ZipOutputStream.CloseEntry that I noticed when looking at this:

When calculating the compressed size of the entry, it does

if (curEntry.IsCrypted)
{
	if (curEntry.AESKeySize > 0)
	{
		curEntry.CompressedSize += curEntry.AESOverheadSize;
	}
	else
	{
		curEntry.CompressedSize += ZipConstants.CryptoHeaderSize;
	}
}

but then after that it does

if (curEntry.LocalHeaderRequiresZip64)

and inside LocalHeaderRequiresZip64, it does

if ((versionToExtract == 0) && IsCrypted)
{
    trueCompressedSize += ZipConstants.CryptoHeaderSize;
}

So it looks like it counts the crypto overhead twice.
Not sure if there's any scope for an edge case there where that makes it use the wrong values if those few exctra bytes push it over the non-64 length limit?

@piksel piksel self-requested a review May 27, 2020 21:11
Copy link
Member

@piksel piksel left a comment

Choose a reason for hiding this comment

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

Seems to do the right things, but might be shortened?

src/ICSharpCode.SharpZipLib/Zip/ZipEntry.cs Outdated Show resolved Hide resolved
src/ICSharpCode.SharpZipLib/Zip/ZipOutputStream.cs Outdated Show resolved Hide resolved
src/ICSharpCode.SharpZipLib/Zip/ZipOutputStream.cs Outdated Show resolved Hide resolved
@Numpsy
Copy link
Contributor Author

Numpsy commented Jun 19, 2020

ok, too much copy/pasting, will have a look over the weekend.

@Numpsy
Copy link
Contributor Author

Numpsy commented Jun 20, 2020

updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
encryption tests zip Related to ZIP file format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants