Android AES encryption / decryption ignores password

Question

I want to make a simple app to encrypt / decrypt messages with AES. My code seems to work right now, as i get text encrypted and decrypted with no problems.

I have an input field, a field to enter the password for decryption/encryption and an output field. And two Buttons (Encrypt / Decrypt).

The Problem is****, when i enter a message and set a password and encrypt it, and then try to provoke an invalid password exeption, the message decrypts although the entered password doesn´t match the password I used for encryption.

Here´s my code for key generation:

public void vers(){
    // Das Passwort bzw der Schluesseltext
    keyStr = keyedit.getText().toString();
    // byte-Array erzeugen

    try {
        key = (keyStr).getBytes("UTF-8");
    } catch (UnsupportedEncodingException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }
    // aus dem Array einen Hash-Wert erzeugen mit MD5 oder SHA

    try {
        sha = MessageDigest.getInstance("MD5");
    } catch (NoSuchAlgorithmException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }
    key = sha.digest(key);
    // nur die ersten 128 bit nutzen
    key = Arrays.copyOf(key, 16); 
    // der fertige Schluessel
    secretKeySpec = new SecretKeySpec(key, "AES");
}

And here the code where I encrypt the message:

private void codealgo() {

    vers();

    // der zu verschl. Text
    text1 = input.getText().toString();


    // Verschluesseln

    try {
        cipher = Cipher.getInstance("AES");
    } catch (NoSuchAlgorithmException | NoSuchPaddingException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }
    try {
        cipher.init(Cipher.ENCRYPT_MODE, secretKeySpec);
    } catch (InvalidKeyException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }

    try {
        encrypted = cipher.doFinal(text1.getBytes());
    } catch (IllegalBlockSizeException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    } catch (BadPaddingException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }

    // bytes zu Base64-String konvertieren (dient der Lesbarkeit)
    geheim = Base64.encodeToString(encrypted, Base64.NO_WRAP);


    // Ergebnis
    output.setText(geheim);

}

And at least here the code where I try to decrypt the messages again:

private void decodealgo() {

        vers();

        geheim2 = input.getText().toString();

    // BASE64 String zu Byte-Array konvertieren
    data = Base64.decode(geheim2, Base64.NO_WRAP);


    // Entschluesseln

    try {
        cipher3 = Cipher.getInstance("AES");
    } catch (NoSuchAlgorithmException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    } catch (NoSuchPaddingException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }
    try {
        cipher3.init(Cipher.DECRYPT_MODE, secretKeySpec2);
    } catch (InvalidKeyException e) {
        // TODO Auto-generated catch block
        Toast.makeText(getApplicationContext(), "Invalid key",
        Toast.LENGTH_SHORT).show();
        e.printStackTrace();
    }

    try {
        cipherData3 = cipher3.doFinal(data);
    } catch (IllegalBlockSizeException e) {
        // TODO Auto-generated catch block
        Toast.makeText(getApplicationContext(), "No valid encryption",
        Toast.LENGTH_SHORT).show();
    } catch (BadPaddingException e) {
        // TODO Auto-generated catch block
        Toast.makeText(getApplicationContext(), "Key invalid format",
        Toast.LENGTH_SHORT).show();
        e.printStackTrace();
    }

    try {
        text3 = new String(cipherData3, "UTF-8");
    } catch (UnsupportedEncodingException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }


    // Klartext
    output.setText(text3);

}

Please tell me why the password is being ignored by the decryption process and why a random entry reveals the message that has been encrypted with a password.

Thank you very much in advance!

Answer 1

You seem to be using fields for almost any variable. You could equate this with using global variables for your application, although this is performed on class level. The amount of fields of your class should be minimized (although you should not use the same field for multiple purposes). Instead, you should be using local variables for anything that doesn't need to be shared between methods.

For instance, all the key related variables are currently fields. You only need a SecretKey instance to encrypt/decrypt in the end, so all those variables should be in local vars (good of you to put the key derivation in a separate method though). Furthermore, you should just use the key as an argument to your encrypt and decrypt methods. That way there is no chance that you suddenly leave some field to its previous value. Sharing a Cipher instance is always a bad idea, especially if you cannot guarantee that the same key will be used.

Finally, note that "AES/ECB/PKCS5Padding" (as likely implied by Cipher.getInstance("AES") may throw a BadPaddingException but that that is not guaranteed, even if the key or ciphertext is incorrect. To provide for integrity and authenticity of the ciphertext you'll have to use a (H)MAC over the ciphertext. ECB mode on itself only provides some level of confidentiality. Probably easiest is to switch to GCM mode encryption for you.

Finally, a password is not a key, search for PBKDF2 for more information. Don't use MD5.