CryptoService is unsafe by design

[juliopedreira]

CryptoService is not using the "secret" parameter(password) set on init(...) method for ciphering or deciphering. Instead, it's using a salt and a iv parameter that are returned plain ( hex encoded ) at the end of the encrypted text.

Instead of using the provided "secret", the code is generating a SecretKeySpec from the salt.

Salt and IV parameters can be easily obtained:

            byte[] salt = cypherText[-32..-1].decodeHex()
            byte[] iv = cypherText[-64..-33].decodeHex()

The secret key can then be easily regenerated:

        SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA1")
        KeySpec spec = new PBEKeySpec(secret.toCharArray(), salt, 20000, 128)
        SecretKey tmp = factory.generateSecret(spec)
        SecretKeySpec secretKey= new SecretKeySpec(tmp.getEncoded(), "AES")

And then, the text can be decrypted ( without knowin the password set to the CryptoService!! )

        Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding")
        cipher.init(Cipher.DECRYPT_MODE, secretKey, new IvParameterSpec(iv))
        byte[] decValue = cipher.doFinal(text)
        return new String(decValue)

[pabloalba]

On it, let me see...

[pabloalba]

Mmm... I don't think this is right.

The generateKey method uses the salt AND the secret: KeySpec spec = new PBEKeySpec(secret.toCharArray(), salt, 20000, 128)

In fact, you use that exact line on your example also! So, in order to regenerate the key, you need the secret.

You can easily test it. First, encrypt a text with the secret "secret1". Then, change the secret to "secret2". Then, try to decrypt the text. You won't be able.

[juliopedreira]

It's not using the "secret" attribute, but a variable generated from the salt:

private Map<String, byte[]> _encrypt(String text) { byte[] salt = getSalt() SecretKeySpec secret = generateKey(salt) Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding")

    cipher.init(Cipher.ENCRYPT_MODE, secret)

    byte[] iv = cipher.getParameters().getParameterSpec(IvParameterSpec).getIV()
    byte[] ciphertext = cipher.doFinal(text.getBytes("UTF-8"))

    [ciphertext: ciphertext, iv: iv, salt: salt]
}

[pabloalba]

I think you are confused by the unfortunate choice of variable names. It is true that the cipher uses a local variable called "secret". In order to avoid misinterpretations, it would be better to rename the local variable of the line that you mention to: SecretKeySpec secretKey = generateKey(salt)

Anyway, if you look inside the "generateKey" function, on the line 45 of the file, you can see that the key is generated using the "secret" attribute, not the local variable. Again, it would be less confusing to use "this", so we can change that line to:

KeySpec spec = new PBEKeySpec(this.secret.toCharArray(), salt, 20000, 128)

I hope it is more clear now.