Adress sanitycheck for aes encrypted zip

[ghost]

No description provided.

[pixelglow]

Thanks for your pull request. In general it looks good, but several minor issues to fix.

As a first pass, please rebase these to atomic commits. Each commit should be tightly focussed on one thing. For example:

• Update for Xcode 7.3.
• Enable testing of OS X Framework.
• Sanity check before any instantiation of data stream. (Can include the unit testing code as well.)
• Validate CRC on WinZip AES-encrypted entries. (But see last point in general comments below.)

Merge or drop typos and bad commits. Split up @18d7f9c into two commits. You can use a git rebase -i to do this and then git push --force to update this pull request.

Other general comments:

• Style nits:
  • Follow the existing code's bracket style i.e. { on a new line, } to match indentation.
  • Follow the existing code's pointer style i.e. ZZWinZipAESExtraField* winZipAESRecord instead of ZZWinZipAESExtraField *winZipAESRecord.
  • Objective-C uses YES and NO, not true and false.
• I like how you've broken up the check: method into several separate methods! Do put the method declarations in the @interface ZZOldArchiveEntry() extension.
• Drop checkEncryptionAndCompressionMode: and call checkEncryptionMode: and checkCompressionMode: directly. Looks cleaner and easier to reason about the logic.
• getLocalEncryptionMode appears to only be used in checkLocalEncryptionMode and doesn't really make sense as a separate piece of code e.g. no need to be in public interface. Integrate that into checkLocalEncryptionMode.
• checkCRC: method checks for versionNumber, use a switch statement instead of multiple if as this makes the code clearer.
• validateCRC: method appears to always return true. Not your fault, this is a pre-existing problem! Best to drop that method entirely since I don't see how we could actually check CRC before decompressing and/or decrypting. You should leave intact the validation of zero CRC for version_AE2 though.

[ghost]

Hi thanks for your comments. I have just integrated those now.

Few comments on my side:

• Please note that within the check of the local header (checkLocalHeader) I am now doing this check: _localFileHeader->compressionMethod != _centralFileHeader->compressionMethod instead of _localFileHeader->compressionMethod != self.compressionMethod.
• I believe i already asked but do you see a way to corrupt a local header by taking as ressource one of your encrypted AES file?

[pixelglow]

OK, a couple more minor edits and we should be done. Thanks for your work!

• Please integrate the style nits with the original commit e.g. by splitting up @f1dbfc1 and then merging the style nits with @e9eac9c. This way I can actually see that the styles are being followed.
• In ZZOldArchiveEntry.mm:306, use ZZRaiseErrorNo as an early return which is the convention.
• Do put the checkXYZ: method declarations in the @interface ZZOldArchiveEntry() extension.

Please note that within the check of the local header (checkLocalHeader) I am now doing this check: _localFileHeader->compressionMethod != _centralFileHeader->compressionMethod instead of _localFileHeader->compressionMethod != self.compressionMethod.

That looks fine.

I believe i already asked but do you see a way to corrupt a local header by taking as ressource one of your encrypted AES file?

You can try corrupting a local header by open an NSData from the file, figuring what offset the zeroes have to be, writing some zeroes there and then open a ZZArchive with the data. If that's too finicky to work, I'm happy to accept the corrupted file in the test case.

[ghost]

I have integrated your last comments. I have managed also to corrupt the local header of your original small-test-encrypted-aes file by zeroing out some of the local header fields (versionNeededToExtract, generalPurposeBitFlag, compressionMethod, lastModFileName). This was feasible by identifying the local header signature in the file (504b 0304) and the offset for the next fields i wanted to zero out.

I hope this looks good to you now :-) - have a nice w.e!

[pixelglow]

I'm sorry for not getting back to you sooner -- had a busy month, both in life and work.

I really do appreciate the work you've put into this!

However, you'll need to be careful to respect the project style throughout. I see still quite a few bracket style exceptions. Please rebase to integrate any style fixes with the commits that originated them.

Also, where you've made certain methods part of the class extension, both the class extension methods and their implementation should be part of the same commit.

This is so that we can just inspect one commit and see everything that's needed to make it work there, rather than having to inspect multiple commits. Also helps git bisect and other automated debugging procedures.

Also, when I meant "corrupt the original test archive", I actually meant to do this in the test case e.g. in setUp and tearDown. This avoids having to include another zip file in the package, since we can copy the existing zip file and corrupt it for testing in setUp, then delete the copy in tearDown.