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

Incorrect exception message shown for wrong password error #9

Closed
AndrejsAlsevskis opened this issue Jul 2, 2019 · 7 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@AndrejsAlsevskis
Copy link

AndrejsAlsevskis commented Jul 2, 2019

Hi,
I'm using latest version v2.0.3 and I am getting incorrect exception messages when trying to unzip password-protected zip files created with 7zip using ZipCrypto enctyption method and providing wrong password.
Steps to reproduce:

  1. Create a zip file test.txt using 7Zip, provide a password and choose Archive Format: zip, Encryption method: ZipCrypto.
  2. Run sample code:
@Test
public void test() throws Exception {
  ZipFile zipFile = new ZipFile("test-ZipCrypto.zip", "blah".toCharArray());
  for (FileHeader h:zipFile.getFileHeaders()) {
   try {
       zipFile.extractFile(h, "d:\\temp");
   }
   catch (Exception e) {
       System.out.println(e.getMessage());
   }
  }
}

The sample code above prints:
java.io.IOException: Reached end of entry, but crc verification failed for test.txt

@fkirc
Copy link

fkirc commented Jul 2, 2019

I experienced a similar bug, but in the context of AES decryption.
The bug has the following reason:
ZipInputStream.java:getNextEntry() converts a ZipException into an IOException.
Moving up the stacktrace, UnzipUtil.java:createZipInputStream() converts this IOException back into a ZipException.

The problem with this Exception conversion is that the "type" of the ZipException gets lost. More specifically: The type "WRONG_PASSWORD" gets converted into the type "UNKNOWN".

This makes it hard or impossible to correctly identify an invalid password (without applying further workarounds).

I suggest the following changes:

  1. Do not convert ZipException into IOException. Instead, use ZipException for the whole stacktrace.
  2. Add a unit test that uses an invalid password when attempting to open a ZipInputStream.

@fkirc
Copy link

fkirc commented Jul 2, 2019

Nevertheless, thank you @srikanth-lingala for this excellent library.

I just released the first version of the Android app "Secure Zip Notes" (which is still rather feature-limited):
https://github.com/fkirc/secure-zip-notes
https://play.google.com/store/apps/details?id=com.ditronic.securezipnotes

If our further plans with this app turn out to be successful, then I will make sure that you receive an adequate compensation.

@fkirc
Copy link

fkirc commented Jul 2, 2019

In the meantime, you can workaround this bug by checking the exception message:

        try {
            final ZipInputStream is = zipFile.getInputStream(fileHeader);
            ...
        } catch (ZipException e) {
            if (e.getMessage().contains("Wrong Password")) {
                ...
            }
        }

@srikanth-lingala
Copy link
Owner

@AndrejsAlsevskis
The problem with Standard Zip Encryption is that there is no straight forward way to detect wrong password. At least I wasn't able to find it. The only way I know of is to try to extract the content and compare the crc after extraction. If crc comparison fails, this most likely means a wrong password. This however is not a confirmed way of saying that the password is wrong (it can be that the crc was wrongly calculated when zip file was created), but it most likely means a wrong password. AES encryption on the other hand provides a way to check for the password upfront, so a wrong password "type" in this case can be relied upon.

However, I think at the moment, zip4j does not set the exception type as wrong_password in this case. This type should be set. I will include it in the next release.

@fkirc
I see what you mean by wrong password type getting converted to unknown type. I will look into this and fix it in the next release. And thanks for the pointer about a unit test for it. I am surprised that there is no test case already. I had to release this on a tight schedule and probably lost track of it.

@AndrejsAlsevskis
Copy link
Author

@srikanth-lingala
Many thanks for your response. I understand that detection of incorrect password for ZipCrypto is tricky. Perhaps following StackOverflow thread could be helpful:
https://stackoverflow.com/questions/1651442/implementation-of-zipcrypto-zip-2-0-encryption-in-java

Look for a code with a following comment.
// PKZIP prior to 2.0 used a 2 byte CRC check; a 1 byte CRC check is
// used on versions after 2.0. This can be used to test if the password
// supplied is correct or not.

Cheers,
Andrejs

@srikanth-lingala
Copy link
Owner

srikanth-lingala commented Jul 3, 2019

That comment has been taken out of the official zip specification. I have read this already as part of the zip specification. Unfortunately, it did not help me much. I experimented a bit but I could not figure out a solution for it.

What I also noticed a few years back is that, some other famous tools (like Winzip, 7Zip, etc) also extract the whole file before showing user a wrong password message dialog. This happened with those tools only for ZipCrypto and not AES. In case of AES, those tools had a wrong password dialog almost immediately, but with ZipCrypto it took a lot of processing (depending on the file size in the zip) before this dialog showed up, which made me believe this was the only way to figure out a wrong password for ZipCrypto.

I could very well be wrong, and there probably is a way to figure it out. If so, I would highly appreciate if someone can point me in the right direction, except for the specification document, which is, unfortunately, not really helpful.

@srikanth-lingala srikanth-lingala self-assigned this Jul 4, 2019
@srikanth-lingala srikanth-lingala added the bug Something isn't working label Jul 4, 2019
srikanth-lingala added a commit that referenced this issue Jul 4, 2019
@srikanth-lingala
Copy link
Owner

Fixed in v2.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants