isPasswordValidForArchiveAtPath returning wrong result if first file is directory

[5b5]

Hi,

i suppose the method was not meant to exit with YES at this point but only when the while loop has finished? I recognized that the method did exit with YES even when the wrong or no password was given for password encrypted files, but only for some files. With this fix it works for my files. Maybe the assumptions in this part of the code are wrong? At least in my case it wasn't sufficient to test the first file though.

Cheers & thank you for your work!

[5b5]

It turned out the problem occurs when the first file in the archive is an empty directory. The call to unzReadCurrentFile in this case will be given a len parameter of 0 which results in readBytes being 0 and the method returning YES.

[5b5]

Tried to fix the problem by skipping over directories, i don't know enough about Zipfiles to be sure i didn't miss any cases. (i.e. is it 100% safe to read the filename and check for the ending character for each and every file?) Please review.

[5b5]

Another solution might be just to check for readBytes > 0 ...
if (readBytes < 0) { ... return NO; } else if (readBytes > 0) { return YES; } else { // skip to next file }

But i actually have no clue which of the solutions is more failsafe.

[Coeur]

You're allocating filename, but you have some early returns which don't get call free(filename).

[Coeur]

Possible concerns:

• Could we have a unit test?
• What if the first item is a zero-bytes file (and not a directory)?
• What if the first item is a directory named "foo/."?
• What about function isFilePasswordProtectedAtPath?

We should explore those and see if we can come up with some better fix.

[5b5]

Sorry, i must ask you for some patience, i will not be able to review your comments and respond for the next two weeks. I will try to respond asap thereafter.

[Coeur]

I've addressed the memory leak you had in filename, but it still leaves my other concerns, notably a test file to verify this behavior.

memory leak fixed

[5b5]

Thanks for fixing the memory issues!

I reproduced and tracked down the problem today, this is what i found:

1.) The problem is not reproducible in v2.1.4 because unzOpenCurrentFilePassword returns UNZ_BADPASSWORD now even for empty directories for files zipped with SSZipArchive, which means directories are skipped anyway (at least for files zipped with SSZipArchive, see 2.)).

2.) The problem only occurred with zip files generated by SSZipArchive itself because with SSZipArchive (empty) directories zipped with password get the "flag & 1" set. Although i discovered, if zip files are created with the "zip" command, only files (not directories) seem to get this flag set.

Therefore, i'm not quite sure if the behavior described in 1.) is correct. For files zipped from command line unzOpenCurrentFilePassword still returns UNZ_OK, but the problem does not occur, as the flag is checked in isPasswordValidForArchiveAtPath, for isFilePasswordProtectedAtPath it doesn't matter, too.

I currently do have a zip file which reproduces the problem with v2.1.3 of SSZipArchive and added some test cases, but as the problem is gone now... do you still want those unit tests? I will push those testcases onto a new branch in my fork, if you want them i can add a new pull requests only containing the test cases...

[5b5]

for unit tests and test files see 5b5@2a4144c

If you think the behavior described in 1.) above is correct, please close this PR.

[Coeur]

I meant to add test cases if there was something broken in current version (2.1.4).

We should focus on attempting to make a legit password-protected archive with a different tool (comment line, WinZip, Keka, etc.) and some empty folders/files to see the password detection is working fine in all scenarios.

[5b5]

As i described above, when a password-encrypted zip file is generated by SSZipArchive it will differ from an encrypted archive zipped from command line (even in 2.1.4) in having the encryption flag set on directory entries, whereas the flag is not set for directory entries in the file generated from command line.
(You can find such files in the commit mentioned above, you are welcome to use these files for you own tests, if you don't want to use my tests.)

I just wanted let you know that there is a difference in the results between both as i don't know which one is the right method. (I had a look into the ZIP-Spec but i didn't find a clear answer and i am not at all familiar with the Zip-file format details.)

It MIGHT be a bug in the current version even if it doesn't lead to broken unit-tests as both files work together with isPasswordValidForArchiveAtPath because of different reasons:
The SSZipArchive-generated file works because unzOpenCurrentFilePassword returns UNZ_BADPASSWORD when providing a wrong password (which i also don't understand because a password should not be necessary in order to unzip a directory entry !?), the command line generated file returns UNZ_OK with a wrong password but skips on to the next entry because the encryption flag is not set.
So both files will work without problems but it still is unclear to me if setting the flag on directory entries while zipping is a bug or not.

I hope this made things a bit clearer. I cannot help you much further at the moment because i already spent about 7 hours on investigating this issue and i'm quite busy at the moment.