Implementing AES encryption in Java

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 = ne

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.

StackExchange Code Review Q#134980, answer score: 6