Instantiation of decrypted stream is crashing for some zip file (AES 256)

[Deleted user]

Hi there.

I am experimenting some crash when trying to unarchive a specific AES256 encrypted zip. The following invocation will crash:

decryptedStream = [[ZZAESDecryptInputStream alloc] initWithStream:dataStream password:password header:_localFileHeader->fileData() strength:_localFileHeader->extraField<ZZWinZipAESExtraField>()->encryptionStrength error:error]; }

This is because in some cases the _localFileHeader doesn't contain any data and as a result the encryptionStrength can't be extracted. I have found out that the file was corrupted but i believe we should return an error in this case.

I have now added this additional sanity check before the creation of the localFileHeader:
nextCentralFileHeader->relativeOffsetOfLocalHeader == 0 && *beginContent == '\0

for the following section:

NSMutableArray<ZZArchiveEntry*>* entries = [NSMutableArray array];
    for (NSUInteger index = 0; index < endOfCentralDirectoryRecord->totalNumberOfEntriesInTheCentralDirectory; ++index)
    {
        // sanity check:
        if (
            // correct signature
            nextCentralFileHeader->sign != ZZCentralFileHeader::sign
            // single disk zip
            || nextCentralFileHeader->diskNumberStart != 0
            // local file occurs before first central file header, and has enough minimal space for at least local file
            || nextCentralFileHeader->relativeOffsetOfLocalHeader + sizeof(ZZLocalFileHeader)
                > endOfCentralDirectoryRecord->offsetOfStartOfCentralDirectoryWithRespectToTheStartingDiskNumber
            // next central file header in sequence is within the central directory
            || (const uint8_t*)nextCentralFileHeader->nextCentralFileHeader() > endOfCentralDirectory
            || nextCentralFileHeader->relativeOffsetOfLocalHeader == 0 && *beginContent == '\0')
            return ZZRaiseErrorNo(error, ZZCentralFileHeaderReadErrorCode, @{ZZEntryIndexKey : @(index)});

        ZZLocalFileHeader* nextLocalFileHeader = (ZZLocalFileHeader*)(beginContent
                                                                      + nextCentralFileHeader->relativeOffsetOfLocalHeader);

        [entries addObject:[[ZZOldArchiveEntry alloc] initWithCentralFileHeader:nextCentralFileHeader
                                                                localFileHeader:nextLocalFileHeader]];

        nextCentralFileHeader = nextCentralFileHeader->nextCentralFileHeader();
    }

What do you think? If this sounds correct to you i will make a pull request.

Thanks in advance for your input on that matter.

[pixelglow]

Thanks for figuring out and working on this issue!

Your instinct is correct in that we'll need to return an error where the file is corrupt wherever possible.

However, the listed sanity check is not the place to do it. The check nextCentralFileHeader->relativeOffsetOfLocalHeader == 0 is actually valid for the first entry, although I see you have *beginContent == '\0' to fence it. But to avoid paging in the beginning of the file, we should not touch the local file at all with *beginContent == '\0'. Also this only checks for zero bytes where the local file is expected, not other sorts of local header corruption.

The best spot to add a check is ZZOldArchiveEntry.mm:258 i.e. checkEncryptionAndCompression:. This bottlenecks all calls to output and is called when the user is already committed to accessing the local file, so there's no worry about paging it in.

If you can add a simple check for the local file header signature there (and possibly rename the method to something more sensible) in a single-commit pull request, I'll merge it in.

Alternatively, you could merge checkEncryptionAndCompression: and check: (a public method) and have the output methods call check: instead. That would provide a much more solid sanity check on the output code path and shouldn't impact performance much.

[Deleted user]

Hi thanks for your feedbacks: i will work on the fix today. In the mean time is there any reason why the method - (BOOL)check:(out NSError**)error is decoupled to this one: - (BOOL)checkEncryptionAndCompression:(out NSError**)error? Is it for performance reason?

[pixelglow]

Historically, check: was for separately checking consistency between the local and central file header, since most of the ZZOldArchiveEntry properties and data extraction would use the central file header fields in preference to the local file header fields. However data extraction necessarily accesses the local file header as well anyway, so there should be little performance impact there with the additional checks.

Merging the check: and checkEncryptionAndCompression: would force a proper consistency check for data extraction and simplify the interface, which are good things. In a future commit, we could make check: private (breaking the API, so I'd want to roll that into 9.x) since I don't see much point in a publicly callable check:.

[Deleted user]

I agree with you the check: method should be internal so the user don't have to remember to call this method before invoking a data extraction.

I have an other question. I can see that there is a check for the standard encrypted CRC. What CRC is for and does it make sense to have a test on CRC for AES encryption? (Any input on how to test would be much appreciated).

[pixelglow]

You can check out what the CRC is for AES encryption here: http://www.winzip.com/aes_info.htm#CRC

[Deleted user]

OK thanks - is it possible to run directly your test within your project? it doesn't seem to be the case although test files are present within the project. I want to add an additional test for a corrupted AES256 zip file.

[pixelglow]

The testing scheme is OS X only.

If you want to add a test for AES256 corruption, use an existing valid test zip file and apply your corruption to a copy (presumably it's zeroing out some bytes?), rather than including a corrupted zip file in the package. This can eventually be the basis for a fuzz test by selectively zeroing out different parts of the zip file.

[Deleted user]

I have merged the check and checkEncryptionAndCompression into one within my fork repo. I was able to run your tests on OSX but i had to edit you project a bit : the tests were not linked to the OSX scheme.

I couldn't reproduce a corrupted zip file by editing one of your zip file though so i ended up adding the zip file that was crashing on my project. I was also able to add a unit test for that file.

You can check my commit on my fork repo: if that's sound good to you i will make a pull request.

[pixelglow]

Thanks for the work! It looks good to me with only a few minor nits. Do make the pull request and we can discuss the issues and amend/add any commits needed then. I will ask you to rebase your commits to fix these minor nits so you may prefer to put your commits in a different github branch before making the pull request.

[Deleted user]

Great i will take care of the pull request then and rebase my commits as you ask.

[Deleted user]

Done #143

[Deleted user]

Hi Glen - do you need some additional actions from my side to validate my pull request?