Is Java security module KeyGenerator thread safe? If not then how to fix it?

Question

I have a concurrent encryption/decryption program in which multiple AES128 keys are randomly generated concurrently by invoking the following code (written in scala, the Java version should be fairly similar):

  private def AESKeyGen: KeyGenerator = {
    val keyGen = KeyGenerator.getInstance("AES")
    keyGen.init(128)
    keyGen
  }

  def generateKey: SecretKey = this.synchronized {
    AESKeyGen.generateKey()
  }

each key is use to encrypt a fixed byte array, then decrypt it by using AESEncrypt and AESDecrypt functions:

  def ivParameterSpec = this.synchronized{
    import com.schedule1.datapassport.view._

    new IvParameterSpec("DataPassports===")
  }

  private def getCipher = this.synchronized {
    Cipher.getInstance("AES/CBC/PKCS5Padding")
  }

  private def nextCipher(aesKey: Key): Cipher = this.synchronized{
    val cipher = getCipher
    cipher.init(Cipher.ENCRYPT_MODE, aesKey, ivParameterSpec)
    cipher
  }

  private def nextDecipher(aesKey: Key): Cipher = this.synchronized{
    val cipher = getCipher
    cipher.init(Cipher.DECRYPT_MODE, aesKey, ivParameterSpec)
    cipher
  }

  def nullBytes = Array.fill[Byte](16)(0)

  def aesEncrypt(bytes: Array[Byte], key: Key): Array[Byte] = this.synchronized{
    val effectiveBytes = if (bytes == null) nullBytes
    else bytes
    nextCipher(key).doFinal(effectiveBytes)
  }

  def aesDecrypt(cipher: Array[Byte], key: Key): Array[Byte] = this.synchronized{
    val effectiveBytes = Utils.retry(3){
      nextDecipher(key).doFinal(cipher)
    }
    if (effectiveBytes.toList == nullBytes.toList) null
    else effectiveBytes
  }

The program runs smoothly on 1 core/thread, but when I increase concurrency gradually to 8. I have gradually higher chance of encountering the following error:

javax.crypto.BadPaddingException: Given final block not properly padded
    at com.sun.crypto.provider.CipherCore.doFinal(CipherCore.java:966)
    at com.sun.crypto.provider.CipherCore.doFinal(CipherCore.java:824)
    at com.sun.crypto.provider.AESCipher.engineDoFinal(AESCipher.java:436)
    at javax.crypto.Cipher.doFinal(Cipher.java:2165)
...

Looks like at least one of the cryptocurrency component is not thread safe, despite I have marked most of them as synchronized as possible. How to fix this problem? (Or which library should I switch to to avoid it?)

Shanonshanta

You can get this error if: 1 the key is incorrect 2: the data is incorrect or 3: if the IV is incorrect for small ciphertext (but you incorrectly use a static IV, so this cannot happen). But I guess the chance of the keys and data being mixed up are higher. Note that this can also happen if you incorrectly encode the ciphertext to a string. – Nonalignment

Another comment: if you are in a situation where you use any of the statefull security classes (factories, Cipher, Signature etc) in a multi-threaded manner then you are in trouble. You can however share many data containers such as keys as they are generally immutable. I don't see any reuse between threads at the first sight of your code, but if you ever share a factory or algorithm implementation you are not coding it right. There is no need to reuse e.g. Cipher, just instantiate two with the same key. – Nonalignment

What Maarten probably want's to say is that the code that you posted will not exhibit the issue you described. This is essentially a guessing game until you show a Minimal, Complete, and Verifiable example. – Gowon

Good Point, the concurrency is implemented under Apache Spark, which has a parameter to adjust how many threads can be run at the same time. I'll post the MCV shortly in the main question post. – Shanonshanta

Answer 1

After some test I found that sun.misc.BASE64Encoder is not thread safe, all problem solved after changing its instance from a singleton to dynamic creation.

Shanonshanta