Is this Java encryption code thread safe?

Question

I want to use the following code for a high-concurrency application where certain data must be encrypted and decrypted. So I need to know what part of this code should be synchronized, if any, to avoid unpredictable issues.

public class DesEncrypter {
    Cipher ecipher;
    Cipher dcipher;

    // 8-byte Salt
    byte[] salt = {
        (byte)0xA9, (byte)0x9B, (byte)0xC8, (byte)0x32,
        (byte)0x56, (byte)0x35, (byte)0xE3, (byte)0x03
    };

    int iterationCount = 19;

    DesEncrypter(String passPhrase) {
        try {
            // Create the key
            KeySpec keySpec = new PBEKeySpec(passPhrase.toCharArray(), salt, iterationCount);

            SecretKey key = SecretKeyFactory.getInstance( "PBEWithMD5AndDES").generateSecret(keySpec);
            ecipher = Cipher.getInstance(key.getAlgorithm());
            dcipher = Cipher.getInstance(key.getAlgorithm());

            // Prepare the parameter to the ciphers
            AlgorithmParameterSpec paramSpec = new PBEParameterSpec(salt, iterationCount);

            // Create the ciphers
            ecipher.init(Cipher.ENCRYPT_MODE, key, paramSpec);
            dcipher.init(Cipher.DECRYPT_MODE, key, paramSpec);
        } catch (...)
    }

    public String encrypt(String str) {
        try {
            // Encode the string into bytes using utf-8
            byte[] utf8 = str.getBytes("UTF8");
            // Encrypt
            byte[] enc = ecipher.doFinal(utf8);
            // Encode bytes to base64 to get a string
            return new sun.misc.BASE64Encoder().encode(enc);

        } catch (...)
    }

    public String decrypt(String str) {
        try {
            // Decode base64 to get bytes
            byte[] dec = new sun.misc.BASE64Decoder().decodeBuffer(str);
            // Decrypt
            byte[] utf8 = dcipher.doFinal(dec);
            // Decode using utf-8
            return new String(utf8, "UTF8");
        } catch (...)
    }
}

If I create a new cipher in the encrypt() and decrypt() methods for each invocation, then I can avoid concurrency problems, I'm just not sure if there's a lot of overhead in getting a new instance of a cipher for each invocation.

   public String encrypt(String str) {
        try {
            // Encode the string into bytes using utf-8
            byte[] utf8 = str.getBytes("UTF8");
            // Encrypt
            //new cipher instance
            ecipher = Cipher.getInstance(key.getAlgorithm());

            byte[] enc = ecipher.doFinal(utf8);
            // Encode bytes to base64 to get a string
            return new sun.misc.BASE64Encoder().encode(enc);

        } catch (...)

Solution 1

Things only need to be thread-safe if they're used by multiple threads at once. Since each instance of this class will presumably be used by only a single thread, there's no need to worry about whether it's threadsafe or not.

On an unrelated note, having a hardcoded salt, nonce or IV is never a good idea.

Solution 2

The standard rule is - unless the Javadoc states explicitly that a class in the Java libraries is thread-safe, you should assume that it is not.

In this particular instance:

• The various classes are not documented as thread-safe.

• The Cipher.getInstance(...) and SecretKeyFactory.getInstance(...) methods ARE documented as returning new objects; i.e. not references to existing objects that other threads might have references to.

  UPDATE - The javadoc says this:

  "A new SecretKeyFactory object encapsulating the SecretKeyFactorySpi implementation from the first Provider that supports the specified algorithm is returned."

  Furthermore, the source code plainly confirms that a new object is created and returned.

In short, this means that your DesEncryptor class is not currently thread-safe, but you should be able to make it thread-safe by synchronizing the relevant operations (e.g. encode and decode), and not exposing the two Cipher objects. If making the methods synchronized is likely to create a bottleneck, then create a separate instance of DesEncryptor for each thread.

Solution 3

The Cipher object is not going to be thread-safe, because it retains internal state about the encryption process. That applies to your DesEncrypter class as well - each thread will need to use its own instance of DesEncrypter, unless you synchonize the encode and decode methods.

Comments

[user646584]

Did not know that, thanks. I plan to store the salt and pass-phrase in separate files under strict OS permissions available only to the application.

[David Gelhar]

Using a constant salt (even if it's protected) defeats much of the point of using a salt in the first place.

[user646584]

See my example above...why not just create a new Cipher instance for each invocation of encrypt()/decrypt()?

[David Gelhar]

Yes, but in your example you're using instance variables ecipher and dcipher to store the Cipher instances, so if 2 threads call encrypt() on the same DesEncrypter instance simultaneously, they'll clobber each other (unless you synchronize the encrypt() call). To avoid this, you could make ecipher and dcipher be local variables (within the encrypt() function) instead of instance variables, that way each invocation of encrypt() would have its own value.

[user646584]

You're saying the salt should be obtained randomly at runtime? I'm confused, clearly the pass-phrase cannot change once data has been encrypted with it, so what needs to be generated up front and stored in a secure file, the salt and passphrase, just the passphrase,, etc?

[David Gelhar]

The point of using a salt when encrypting is so that a given combination of plaintext&passphrase don't always encrypt to the same ciphertext. The classic example of this is Unix passwd file encryption: a salt is used so that even if 2 users have the same password, the encrypted values are different (which makes bulk dictionary attacks harder). The salt is generated randomly when the password is encrypted, and stored with the encrypted value.

[user646584]

I see, but storing the salt with the encrypted value means the salt can't be encrypted when storing it, right? That's ok since the random salt decouples the encryption from all other encrypted data. A static salt would compromise everything if it were discovered. Thanks for the info

[Nick Johnson]

@user646584 @David It doesn't help that people are confusing the terms 'salt' and 'IV'. A salt is used to make finding the preimage for a hashed password harder. An IV is used to ensure various security properties on a cipher, including that the same message encrypted twice will not result in the same ciphertext. What you want here is an IV, which should be unique for every single message you create. You don't need a salt, because you can't store the password as a hash.

[andrewktmeikle]

Sorry for posting on such an old topic, but I doubt that SecretKeyFactory.getInstance() returns a new object every time because its a factory. Also, I'm testing it right now and the first call takes 2.5s whereas every consequent call takes 0s.

[Stephen C]

1) See update. 2) The first call is probably triggering class loading, and initialization that may involve getting "entropy" from the OS. In some circumstances, that can take a significant amount of real time.

[andrewktmeikle]

Fair enough, it didn't make much sense being a singleton tbh. Why would it only gather entropy the first time though? Surely it would need to be done whenever you called the getInstance?

[Stephen C]

I suspect that the entropy is being gathered by something else that is getting loaded / initialized as part of the provider. But this is only guesswork. If you are serious about identifying the real cause, read the source code, profile it, debug it, etcetera.