simbiose / Encryption

Encryption is a simple way to encrypt and decrypt strings on Android and Java project.
MIT License
355 stars 79 forks source link

Attempting to decrypt data with invalid key may return non-null #17

Closed loentar closed 7 years ago

loentar commented 7 years ago

I expect to get null any time when decryption failed while using decryptOrNull, but sometimes it returns junk when decrypting with invalid key.

Here is a test:

public class EncryptUnitTest {

    private final static String SALT = "1EykVsCVKk1pkZq08PDGTg";
    private final static byte[] IV = new byte[] {
            55, -115, 76, -14, 79, -107, -115, 35,
            -122, -24, -76, -82, 39, -92, 104, 41
    };

    @Test
    public void encrypt_isWorking() throws Exception {
        String validPin = "0000";
        String invalidPin = "2222"; // "5555" works as well
        String password = "Terminator_2";

        Encryption enc = Encryption.getDefault(validPin, SALT, IV);
        String encryptedPassword = enc.encryptOrNull(password);
        assertNotNull(encryptedPassword); // encrypted password, ok

        // decrypt using invalid key
        Encryption dec = Encryption.getDefault(invalidPin, SALT, IV);
        String decryptedPassword = dec.decryptOrNull(encryptedPassword);
        assertNull(decryptedPassword); // FAILED: must be null, but it's junk
    }
}

This test fails on assertNull with message: java.lang.AssertionError: expected null, but was:<�f ���S��!Q��>

I suspect the padding block is OK in this case and library threats this situation as successful decryption.

ademar111190 commented 7 years ago

Thank you for your report, it was an excellent one with a test sample that helps me to understand the point, and I need to ask sorry for the long time to answer too.

About the problem, I think it is more a misunderstanding caused by the method name than a bug, the method decrypt can throw 8 different exceptions, so I designed another method decryptOrNull that just do a try-catch and returns null if some error occurs to be verified with a null check.

You are right, in the case of wrong padding blocks, we receive a BadPaddingException so it is the reason that decryptOrNull returns null because it catches the exception and returns null. But in the case of wrong passwords, as far as I know, there is no way to verify if it is correct or no. So the library just returns a wrong unencrypted text.

Thank you again, I'm closing the issue once it is not a bug. If you or someone know a way to test the password and I'm not sure if it is secury, we can reopen this issue and think in a solution.