0

I have the following simple encryption/decryption code:

import javax.crypto.SecretKey;
import javax.crypto.spec.IvParameterSpec;
import javax.crypto.spec.SecretKeySpec;

import org.apache.commons.codec.binary.Base64;
import org.apache.commons.lang3.RandomStringUtils;
          :

public class MyUtility {

    private static Cipher instance = Cipher.getInstance("AES/CBC/PKCS5Padding");
    private static Cipher instance2 = Cipher.getInstance("AES/CBC/PKCS5Padding");
    private static byte[] iv = { 0, 1, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1 };

    private static String encrypt(String data, DataParameters params) throws Exception {

        IvParameterSpec ivspec = new IvParameterSpec(iv);
        String privateKey = MyUtility.getKey(params);

        SecretKey key = new SecretKeySpec(privateKey.getBytes(), "AES");
        instance.init(Cipher.ENCRYPT_MODE, key, ivspec);

        String paddedData = addPaddDate(data); 
        byte[] encryptedBytes = instance.doFinal(paddedData.getBytes());
        byte[] encodeBase64 = Base64.encodeBase64(encryptedBytes); 
        String encryptedStr = new String(encodeBase64);

then soon perform an decryption:

    byte[] decodeBytes = Base64.decodeBase64(encryptedStr.getBytes()); // Decode
    String padDecryptedStr = new String(instance2.doFinal(decodeBytes));

Then 95% of the time, the decrypted values are correct. However, occasionally, I got the following error:

javax.crypto.BadPaddingException: Given final block not properly padded
    at com.sun.crypto.provider.CipherCore.doFinal(CipherCore.java:811)
    at com.sun.crypto.provider.CipherCore.doFinal(CipherCore.java:676)
    at com.sun.crypto.provider.AESCipher.engineDoFinal(AESCipher.java:420)
    at javax.crypto.Cipher.doFinal(Cipher.java:1966)
    at myCode.decrypt(MyEncryptDecryptUtility.java:97)

where line 97 is String padDecryptedStr = new String(instance2.doFinal(decodeBytes));

And the decrypted values look like weird character strings such as: \u001d��tq@s�2�a or �w�����$�X\u0005

This is the padding function I use:

private static String addPaddDate(String data) {
    return data + RandomStringUtils.randomAlphanumeric(10);
}

Any idea what is missing in the above code?

Thank you!

Just want to update some more information I found:

When the decrypted value is a weird character string, I decrypted again, then the second time I got the correct value. Anyone know why is that? Thanks!

Edamame
  • 23,718
  • 73
  • 186
  • 320
  • Did you program your own base 64 as well as your own padding method? Or did you send the data using a GET request? – Maarten Bodewes Jan 19 '16 at 22:38
  • Just added the padding method I used above. Thanks! – Edamame Jan 19 '16 at 22:46
  • 1
    Could you create an MCVE? You did not even provide the encryption code. Note that I asked about the base 64 code or library, not the padding method. Why do you perform your own padding in the first place? Note that using the wrong key (and, for very small ciphertext, even the IV) can also create a `BadPaddingException` – Maarten Bodewes Jan 19 '16 at 22:51
  • Hi Maarten, I added additional encryption code above. May I ask what is "MCVE"? Please let me know if I should provide additional info. Thanks! – Edamame Jan 19 '16 at 23:12
  • http://stackoverflow.com/help/mcve – Maarten Bodewes Jan 19 '16 at 23:13
  • the base64 is from org.apache.commons.codec.binary.Base64. The reason for padding is many of my input have the same values, if I don't pad them, they will be end of the same encrypted values. Which would be easy for people to guess what that value is. – Edamame Jan 19 '16 at 23:19
  • I don't think its your problem, but don't add your own padding. Let the cipher generate its own IV when initialized for encryption. Then send that generated IV with the encrypted message. Using a fixed IV is incorrect. Its purpose is to prevent duplicate cipher texts. – erickson Jan 20 '16 at 00:45
  • Thanks erickson! May I have an example about how to let the cipher generate its own IV? Also, why a fixed IV is incorrect? Thank you! – Edamame Jan 20 '16 at 01:03
  • Semi-concur with @erickson; **IV should be random,** but it doesn't matter whether you let `Cipher` generate it or just invoke `SecureRandom` and the latter can be simpler to code. How does the output of encryption get to the input of decryption? **HTTPS may damage Base64** (specifically plus-sign) if you're not careful as hinted by @Maarten, which will cause decryption to throw BadPadding or produce garbage -- but should not do both! Also, if/when your data contains characters **outside ASCII** (7-bit) subset if encryption and decryption are in different JVMs, .. – dave_thompson_085 Jan 20 '16 at 01:04
  • 3
    ... `String.getBytes()` and `new String(byte[])` may use **different Charset's that damage your data**. Pick one suitable Charset and specify it at both ends; UTF-8 is popular and safe for anything. – dave_thompson_085 Jan 20 '16 at 01:04
  • Thanks dave_thompson_085: I did try to add "UTF-8" when use byte[], but I still have the same issue. Sometimes the decryption output is a weird character string, but if I decrypted it again, I got the right answer ... very strange ... – Edamame Jan 20 '16 at 01:14
  • 1
    `instance.init(Cipher.ENCRYPT_MODE, key); byte[] iv = instance.getIV();` I'm not sure how creating (a potentially blocking operation) and managing `SecureRandom` instances could be simpler than that. When you specify the same IV every time, attackers can tell that you are encrypting the same message. The purpose of the IV is to produce different cipher text from the same message. Your padding won't do that if the plain text is longer than one block. It will only change the end of the message. Using the IV correctly will change the whole cipher text. – erickson Jan 20 '16 at 01:39
  • Please show the code for the initialization of `instance2` too, please. – Daniel Jan 20 '16 at 09:10
  • @Daniel, instance2 and instance are defined the same way, just one for encryption and one for decryption. code modified above. Thanks! – Edamame Jan 20 '16 at 17:34
  • Yes, but is initialized with `instance2.init(Cipher.DECRYPT_MODE, key, ivspec);`? – Daniel Jan 20 '16 at 19:08
  • yes, I have instance2.init(Cipher.DECRYPT_MODE, key, ivspec); – Edamame Jan 20 '16 at 20:07

1 Answers1

0

The job has multiple cpu cores, so multi-tasks were accessing the same Cipher and corrupted the encoding. I synchronized the encrypt() and decrypt() functions, and things are working now.

Edamame
  • 23,718
  • 73
  • 186
  • 320