Implementing AES encryption in Java

Question

I'm not a security expert, but while checking over our AES implementation for our flagship product, I've noticed some strange things, like the output length having a relation with the input length and no apparent use of an IV.

@Service
public class EncryptionServiceImpl implements EncryptionService {

    /** The logger for this class */
    private static final Logger LOGGER = new Logger(EncryptionServiceImpl.class);

    /** There's one and only one instance of this class */
    private volatile static EncryptionServiceImpl INSTANCE;

    /** True if EncryptionService is initialized. */
    private boolean isInitialized = false;

    private Cipher cipherEncrypt;
    private Cipher cipherDecrypt;
    private String keyHex;

    /**
     * Constructor is private, use getInstance to get an instance of this class
     */
    private EncryptionServiceImpl() {
        initialize();
    }

    /**
     * Returns the singleton instance of this class.
     * 
     * @return the singleton instance of this class.
     */
    public static EncryptionServiceImpl getInstance() {
        if (INSTANCE == null) {
            synchronized (EncryptionServiceImpl.class) {
                if (INSTANCE == null) {
                    INSTANCE = new EncryptionServiceImpl();
                }
            }
        }
        return INSTANCE;
    }

    /**
     * Initialize EncryptionService.
     */
    private synchronized void initialize() {
        if (!isInitialized){

            // Get key from SystemSettings.
            SystemSettingsService systemSettingsService = (SystemSettingsService) ServiceFactory.getInstance().createService(SystemSettingsService.class);
            keyHex = systemSettingsService.getScmuuid();
            byte[] keyBytes;

            // If keyHex is not blank (field "scmuuid" already exists in the database):
            if (StringUtils.isNotBlank(keyHex)) {
                keyBytes = hexToBytes(keyHex);
                SecretKeySpec secretKeySpec = new SecretKeySpec(keyBytes, "AES");
                try {
                    cipherEncrypt = Cipher.getInstance("AES");
                    cipherDecrypt = Cipher.getInstance("AES");
                    cipherEncrypt.init(Cipher.ENCRYPT_MODE, secretKeySpec);
                    cipherDecrypt.init(Cipher.DECRYPT_MODE, secretKeySpec);
                } catch (InvalidKeyException e) {
                    throw new InitializationFailureException(
                            "Failure to generate a new encryption key.", e);
                } catch (NoSuchAlgorithmException e) {
                    throw new InitializationFailureException(
                            "Failure to generate a new encryption key.", e);
                } catch (NoSuchPaddingException e) {
                    throw new InitializationFailureException(
                            "Failure to generate a new encryption key.", e);
                }
                //EncryptionService is initialized.
                isInitialized = true;
            } else {
                /*
                 * If keyHex is blank, either we have an SQL exception or the key hasn't
                 * been generated yet. Trying to use the EncryptionService without proper
                 * initialization, will throw a FatalException.
                 * If the key hasn't been generated yet, the next exception will trigger
                 * the caller to use the generateKey() method in the catch block. 
                 */
                //throw new NoEncryptionkeyException();
            }
        }
    }

    /**
     * @see shared.bs.encryption.EncryptionService#isInitialized()
     */
    public boolean isInitialized() {
        return isInitialized;
    }

    /**
     * @see shared.bs.encryption.EncryptionService#decrypt()
     */
    public String decrypt(String value) {
        if (StringUtils.isBlank(value)){
            return null;
        }
        // NULL values from log files can be interpreted as a String with value "null" (see e.g. bug REDACTED)
        if (value != null && value.equalsIgnoreCase("null")) {
            return null;
        }
        if (getCipherDecrypt() == null) {
            throw new EncryptionFailureException("Decryption failure. EncryptionService is not properly initialized.");
        }
        byte[] encryptedBytes = null;
        byte[] decryptedBytes = null;
        try {
            encryptedBytes = hexToBytes(value);
            decryptedBytes = cipherDecrypt.doFinal(encryptedBytes);
        } catch (NumberFormatException e) {
            throw new EncryptionFailureException("Decryption failure.", e);
        } catch (IllegalBlockSizeException e) {
            throw new EncryptionFailureException("Decryption failure.", e);
        } catch (BadPaddingException e) {
            throw new EncryptionFailureException("Decryption failure.", e);
        }
        return new String(decryptedBytes);
    }

    /**
     * @see shared.bs.encryption.EncryptionService#encrypt()
     */
    public String encrypt(String value) {
        if (StringUtils.isBlank(value)){
            return null;
        }
        if (getCipherEncrypt() == null) {
            throw new EncryptionFailureException("Encryption failure. EncryptionService is not properly initialized.");
        }
        byte[] encrypted = null;
        String encHex = null;
        try {
            encrypted = cipherEncrypt.doFinal(value.getBytes());
            encHex = asHex(encrypted);
        } catch (IllegalBlockSizeException e) {
            throw new EncryptionFailureException("Encryption failure.", e);
        } catch (BadPaddingException e) {
            throw new EncryptionFailureException("Encryption failure.", e);
        } catch (NumberFormatException e) {
            throw new EncryptionFailureException("Encryption failure.", e);
        }
        return encHex;
    }

    /** convert a byte array to a hex String */
    private String asHex(byte buf[]) {
        StringBuffer strbuf = new StringBuffer(buf.length * 2);
        int i;
        for (i = 0; i < buf.length; i++) {
            if (((int) buf[i] & 0xff) < 0x10) {
                strbuf.append("0");
            }
            strbuf.append(Long.toString((int) buf[i] & 0xff, 16));
        }
        return strbuf.toString();
    }

    /** convert a hex String to a byte array */
    private byte[] hexToBytes(String hex) {
        byte[] bts = new byte[hex.length() / 2];
        for (int i = 0; i < bts.length; i++) {
            bts[i] = (byte) Integer.parseInt(hex.substring(2 * i, 2 * i + 2), 16);

        }
        return bts;
    }

    /**
     * @see shared.bs.encryption.EncryptionService#generateKey()
     */
    public synchronized void generateKey(){

        if (!isInitialized){

            /*
             * Make sure the key really doesn't already exist and that an initialization failure
             * isn't the result of an earlier SQLException.
             * Try again retrieving the key from the database. 
             */
            SystemSettingsService systemSettingsService = (SystemSettingsService) ServiceFactory.getInstance().createService(SystemSettingsService.class);
            String tryKeyHex = systemSettingsService.getScmuuid();
            if (StringUtils.isNotBlank(tryKeyHex)) {
                // Something came back from the database, we try to initialize again and return silently.
                initialize();
                return;
            }

            // Generate a new 128 bit strong AES key.
            KeyGenerator kgen;
            try {
                kgen = KeyGenerator.getInstance("AES");
            } catch (NoSuchAlgorithmException e) {
                throw new InitializationFailureException(
                        "Failure to generate a new encryption key.", e);
            }
            kgen.init(128); // 128 is in standard JCE
            SecretKey secretKey = kgen.generateKey();
            byte[] keyBytes = secretKey.getEncoded();
            keyHex = asHex(keyBytes);

            // We have a keyHex, it's time to generate the ciphers:
            SecretKeySpec secretKeySpec = new SecretKeySpec(keyBytes, "AES");
            try {
                cipherEncrypt = Cipher.getInstance("AES");
                cipherDecrypt = Cipher.getInstance("AES");
                cipherEncrypt.init(Cipher.ENCRYPT_MODE, secretKeySpec);
                cipherDecrypt.init(Cipher.DECRYPT_MODE, secretKeySpec);
            } catch (InvalidKeyException e) {
                throw new InitializationFailureException(
                        "Failure to generate a new encryption key.", e);
            } catch (NoSuchAlgorithmException e) {
                throw new InitializationFailureException(
                        "Failure to generate a new encryption key.", e);
            } catch (NoSuchPaddingException e) {
                throw new InitializationFailureException(
                        "Failure to generate a new encryption key.", e);
            }

            /*
             * Persist keyHex (field scmuuid in SystemSettings) and encrypt all existing non-encrypted 
             * passwords and secure build/deploy-parameters in the database with this new key.
             * removed for brevity in this example: each of these is an extra method call.
             */            

        }//End: if (!isInitialized)
    }

I got the impression that this has room for improvement, since information about the plaintext is leaking. Please note that because of legacy code throughout the entire project (mainly database field lengths), all our ciphertext output has to be shorter than 255 characters. In effect, this means that the output has to be 224 bytes long.

And yes, I know that we're encrypting passwords. These are not user passwords. Those are handled through external systems like Active Directory and LDAP. These encrypted passwords are used to authenticate to external 3rd party systems where implementing a token-based authentication scheme either is not possible, is not feasible, or has been tried without success.

Answer 1

Well two things, the Cipher.getInstance is not so good, as you said it's not using an IV; it should be using at least something like "AES/CBC/PKCS5Padding" and probably a longer key, i.e. 256 bits (which needs to be enabled for the JVM because of US export restrictions; OpenJDK will already have that on, otherwise you'll get an exception during setup)Edit: See comment. The default settings (and you might want to confirm that with a debugger on the cipher objects - I just did that with some sample code) is ECB mode, which isn't secure at all.

Also take a look on https://crypto.stackexchange.com/ or perhaps cross post there if you have more concerns.

---

For the exceptions I'd actually just catch GeneralSecurityException - there's not much point in catching three different subclasses as it's doing the same thing anyway.

The comments aren't amazing. E.g. I can hardly believe that LOGGER is the logger for the class.

The thread-safe singleton is fine AFAIK, looking at articles on "Double-Checked Locking".

The commented out throw in initialize should really be removed because it's dead code that just confuses the reader.

The flow between generateKey, which retries the initialize call basically and the initialize call during the constructor is confusing to say the least, like either there's something missing or generateKey basically does do initialisation, but never sets isInitialized.

Deduplicating the crypto would be a great idea btw. And please just initialise variables to their value instead of null with assignment - in most blocks in the code that's no problem at all and reduces the number of lines quite a bit.

The hex parsing/printing looks fine but again I have a hard time believing there's no library you have available instead of doing it yourself.

Lastly, decrypt and encrypt aren't synchronized - where is the synchronisation happening? Cipher isn't thread-safe; perhaps just the single doFinal call is, but even then any of the thrown exceptions would mean that the cipher object has to be reset to a valid state separately.

Answer 2

/**
 * @see shared.bs.encryption.EncryptionService#decrypt()
 */
public String decrypt(String value) {
    if (StringUtils.isBlank(value)){
        return null;
    }
    // NULL values from log files can be interpreted as a String with value "null" (see e.g. bug REDACTED)
    if (value != null && value.equalsIgnoreCase("null")) {
        return null;
    }

From the documentation of StringUtils.isBlank:

Returns:
true if the String is null, empty or whitespace

So the null check for value here is not needed and can go.

---

} catch (InvalidKeyException e) {
    throw new InitializationFailureException(
            "Failure to generate a new encryption key.", e);
} catch (NoSuchAlgorithmException e) {
    throw new InitializationFailureException(
            "Failure to generate a new encryption key.", e);
} catch (NoSuchPaddingException e) {
    throw new InitializationFailureException(
            "Failure to generate a new encryption key.", e);
}

For this sort of thing, you have multi-catch.

} catch (InvalidKeyException | NoSuchAlgorithmException | NoSuchPaddingException e) {
    throw new InitializationFailureException(
            "Failure to generate a new encryption key.", e);
}

Multi-catch keeps your code compact and saves you from having to copy-paste exception messages.

Answer 3

Confidentiality of the payload is limited by that of the key, which seems available as an hex string using a mysterious ((SystemSettingsService) ServiceFactory.getInstance().createService(SystemSettingsService.class)).getScmuuid(). Absent more information on that, we can only comment that using an UUID as a secret key would be security by obscurity, which is poor.

As noted in another answer, a serious issue is the use of ECB mode (also noted in the question pointing lack of IV); for example this allows recognition by examination of the ciphertext that two plaintexts (passwords) are identical. CBC or CTR mode with random IV would fix that, but then the issue (also noted in said answer) that a cipher object is not thread-safe is likely to become a serious concern; and, the current use of a single call to Cipher.init versus multiple calls to Cipher.dofinal is then guaranteed to cause trouble.

Alterations of ciphertext are not caught; authenticated encryption e.g. using GCM mode would fix that.

Take note that in a password-encryption application, substitution of the ciphertext of an unknown password with the ciphertext of a known password will change the deciphered password to a known one, even with authenticated encryption. Depending on application context, this might be a threat or not. Fixes to this (if needed) could involve adding an additional tweak parameter to encrypt and decrypt holding context tied to the password, e.g. (hash of) server and username.

As noted in the question, information about the plaintext size leaks (including: if the plaintext is empty, which is special-cased to empty ciphertext); this might be an issue in a password encryption application. The fix is to have a fixed size encryption payload, with some padding (it will waste at least one byte of maximum capacity if we keep things simple).

The plaintext is a string, turned to bytes with getBytes(String) during encryption, and turned to string with String(byte[]) during decryption, without specification of a charset. Depending on context, this might be an issue, or not.

Given that the deciphered plaintext is unauthenticated, an alteration of the ciphertext can well trigger that documented limitation of String(byte[]):

The behavior of this constructor when the given bytes are not valid in the default charset is unspecified.

Base64 would be a better choice than hex for the encrypted string blob format: it allows for longer payload, and would offset the extra space needed for CBC, and even authenticated encryption. Plus, there's an implementation of that built into Java 8.

Having duplicate code for initialization of the Cipher objects (in initialize and generateKey ), especially with four occurrences of the string "AES" describing the mode used which must be the same everywhere, is horrible style.

hexToBytes does not validate its input; for example, for input "123" it returns an array with one byte of value 0x12; this is ugly (but without obvious dire consequence in the context).

There is disregard for efficiency well beyond the use of strings rather than byte arrays, and redundant tests and catches noted in another answer. Limiting to what itches me most, using .length in the control expression of for loops, using Integer.parseInt(hex.substring(…)) and StringBuffer.append(Long.toString(…)) for conversion from/to hex.