Zip file extraction with Zip Standard encryption and wrong password very rarely does not fail with Wrong Password exception

[srikanth-lingala]

Creating this issue from email discussion of a bug for tracking purpose and also this might help someone looking for a similar issue

I am using Zip4J and am very happy with it, I have found it very simple to use. I think I am seeing a small bug in a JUnit test, and thought you might be interested.

The test sets up a password-protected zip file using an incorrect password, then calls the method under test to unzip the zip file to a specific location using the correct password. The method under test (unzipFile) should catch the ZipException, and if the exception type is WRONG_PASSWORD, throw a new WrongZipPasswordException. The test then asserts that an WrongZipPasswordException is thrown.

This test randomly fails, maybe 1 time out of 30. I've attached a minimal working example, so hopefully you can reproduce the issue.

We also have a very similar test which sets up the zip file using the correct password. Everything else is exactly the same. This test never fails, only the test using the incorrect password.

import net.lingala.zip4j.ZipFile;
import net.lingala.zip4j.exception.ZipException;
import net.lingala.zip4j.model.ZipParameters;
import net.lingala.zip4j.model.enums.EncryptionMethod;
import org.apache.commons.io.FileUtils;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;

import java.io.File;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.util.List;

import static org.junit.jupiter.api.Assertions.assertTrue;

class UnzipFileExceptionTestError {

    @TempDir
    File tempDir;

    private File dummySourceDir;
    private File dummyDestDir;

    private File dummyFile;
    private static final String DUMMY_FILE_NAME = "dummyFile";

    private static final String FILE_SEPARATOR = File.separator;
    private static final String ZIP_PASSWORD = "zipPassword";
    private static final String WRONG_ZIP_PASSWORD = "wrongZipPassword";

    @BeforeEach
    void setUp() throws IOException {
        dummySourceDir = new File(tempDir.toString() + FILE_SEPARATOR + "source");
        assertTrue(dummySourceDir.mkdir());

        dummyDestDir = new File(tempDir.toString() + FILE_SEPARATOR + "destination");
        assertTrue(dummyDestDir.mkdir());

        dummyFile = new File(dummySourceDir + FILE_SEPARATOR + DUMMY_FILE_NAME + ".txt");
        Files.write(dummyFile.toPath(), List.of("dummy data"), StandardCharsets.UTF_8);
    }

    @Test
    void unzipFileTestLoop() throws IOException {
        for (int i = 0; i < 10000; i++) {
            System.out.println(i);
            FileUtils.deleteDirectory(dummySourceDir);
            FileUtils.deleteDirectory(dummyDestDir);
            setUp();
            unzipFileExceptionTest();
        }
    }

    @Test
    void unzipFileExceptionTest() throws ZipException {
        final String zipFilePath = dummySourceDir + FILE_SEPARATOR + DUMMY_FILE_NAME + ".zip";
        final String destinationPath = dummyDestDir.toString();

        final ZipFile zipFile = new ZipFile(zipFilePath, WRONG_ZIP_PASSWORD.toCharArray());
        final ZipParameters zipParameters = new ZipParameters();
        zipParameters.setEncryptFiles(true);
        zipParameters.setEncryptionMethod(EncryptionMethod.ZIP_STANDARD);
        zipFile.addFile(new File(dummyFile.toString()), zipParameters);

        Assertions.assertThrows(WrongZipPasswordException.class, () ->
                unzipFile(zipFilePath, destinationPath));
    }

    private void unzipFile(String zipFilePath, String destinationPath)
            throws WrongZipPasswordException, ZipException {
        final ZipFile zipFile = new ZipFile(zipFilePath, ZIP_PASSWORD.toCharArray());
        try {
            zipFile.extractAll(destinationPath);
        } catch (ZipException e) {
            if (e.getType() == ZipException.Type.WRONG_PASSWORD) {
                throw new WrongZipPasswordException(zipFile.getFile().getName());
            } else {
                throw e;
            }
        }
    }

    private class WrongZipPasswordException extends Exception {
        private static final String errorMessage = "Wrong password provided for encrypted ZIP file: %s ";

        WrongZipPasswordException(String filename) {
            super(String.format(errorMessage, filename));
        }
    }
}

[srikanth-lingala]

After doing some research on this topic and playing around with the sample code, my finding is that this is one of the drawbacks / limitations of Zip_Standard encryption.

Zip Standard encryption has a way to verify password, which compares one byte from the password (after doing some calculations on the password) to one byte from the crc of the file in zip. A byte can only be a value between -127 to 128. This means that after some repetitions, we might run out of luck, and the byte remains the same even though the password is different. So there might be a 1 in 256 (from -127 to 128) chance for such a collision. And this is exactly what is happening in our case. This does not necessarily mean that this issue will happen once in 256 times only. It might not happen at all in 256 times or happen several times.

It is important to note that the probability mentioned above is for each file and not the algorithm as a whole, and combined wit the fact that this issue happens only with a wrong password, reduces the probability of this issue quite a bit, but of course does not completely eliminate it. The fact that this is the first time I heard of such an issue in all these years probably confirms that point.

BTW, for the upcoming release of zip4j, I have improved the password verification process. This does not mean that this issue is already fixed. For this issue, I am a bit divided on how to fix. One way is to flag "wrong password" when there is any problem extracting a file which uses zip standard encryption. But I have to think of the consequences of this and if it feels good then I will implement this.

[srikanth-lingala]

Issue fixed in v2.7.0 which was released today.