Encrypting an AES key This encryption class RSA encrypts an AES key and then when the encrypt method is called it decrypts the AES key and uses the key to encrypt the message and basically the same thing with the decryption.

However I am very very new to cryptography so I do not know if this is considered "good" or if this could be improved drastically. 

--

What I would like to be reviewed:


Is this considered secure method to cipher messages
Is this efficient
Is this code "clean"


import java.security.KeyPair;
import java.security.KeyPairGenerator;
import java.security.SecureRandom;

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

public class Cryptographer {
    // CIPHER NECESITIES:
    private KeyPairGenerator KEY_PAIR_GENERATOR;
    private KeyGenerator KEY_GENERATOR;
    private KeyPair RSA_KEYS;
    private byte[] AES_KEY_DATA;
    private Cipher AESKeyEncryptor;
    private Cipher MessageEncryptor;
    private IvParameterSpec Iv = new IvParameterSpec(SecureRandom.getSeed(16));

    // SETTINGS:
    final private int RSA_KEY_SIZE = 2048;
    final private int AES_KEY_SIZE = 256;
    final private String RSA_ALGORITHM = "RSA/ECB/OAEPWithSHA-256AndMGF1Padding";
    final private String AES_ALGORITHM = "AES/CBC/PKCS5PADDING";

    // CONSTRUCTOR:
    public Cryptographer() throws Exception {
        KEY_PAIR_GENERATOR = KeyPairGenerator.getInstance("RSA");
        KEY_GENERATOR = KeyGenerator.getInstance("AES");
        KEY_PAIR_GENERATOR.initialize(RSA_KEY_SIZE);
        KEY_GENERATOR.init(AES_KEY_SIZE);
        RSA_KEYS = KEY_PAIR_GENERATOR.generateKeyPair();

        MessageEncryptor = Cipher.getInstance(AES_ALGORITHM);
        AESKeyEncryptor = Cipher.getInstance(RSA_ALGORITHM);
        AES_KEY_DATA = EncryptAESKey(KEY_GENERATOR.generateKey());
    }

    // PRIVATE METHODS:
    private byte[] EncryptAESKey(SecretKey key) throws Exception {
        AESKeyEncryptor.init(Cipher.ENCRYPT_MODE, RSA_KEYS.getPublic());
        return AESKeyEncryptor.doFinal(
                key.getEncoded()
            );      
    }

    private SecretKey DecryptAESKey(byte[] EncryptedAESKey) throws Exception {
        AESKeyEncryptor.init(Cipher.DECRYPT_MODE, RSA_KEYS.getPrivate());
        return new SecretKeySpec(
                AESKeyEncryptor.doFinal(EncryptedAESKey),
                "AES"
            );
    }

    // PUBLIC METHODS:
    public byte[] Encrypt(byte[] message) throws Exception {
        MessageEncryptor.init(Cipher.ENCRYPT_MODE, DecryptAESKey(AES_KEY_DATA), Iv);

        return MessageEncryptor.doFinal(message);
    }

    public byte[] Decrypt(byte[] message) throws Exception {
        MessageEncryptor.init(Cipher.DECRYPT_MODE, DecryptAESKey(AES_KEY_DATA), Iv);

        return MessageEncryptor.doFinal(message);
    }
}
```
You're trying to create a so called hybrid cryptosystem or hybrid encryption method (crypto is not just considered encryption anymore, after all).

The problem with CBC mode is that it is very vulnerable to padding oracle attacks. It is itself not authenticated, which means that you can change any data within the ciphertext and thereby change plaintext blocks. Using GCM mode is recommended.

Adding a digital signature over the message is recommended if you want to know who send the message to you. Currently your code is only valid for providing confidentiality for locally stored data (data "at rest" in the jargon).



The whole idea of hybrid encryption is to send the encrypted AES key as part of the ciphertext. Since the size of the encrypted AES key is always the same size as the key size of the RSA key pair in bytes, it can simply be prefixed to the AES ciphertext.

Of course, you'd need to split the resulting ciphertext message in two again to be able to decrypt it. Note that the Java API misses a method to retrieve the RSA key size. This is however OK because you can retrieve the modulus size by using ((RSAPrivateKey)RSA_KEYS.getPrivate()).getModulus().bitLength() in your code. This is - by definition - the same as the key size in bits.



Your code is clearly not keeping to any conventions when it comes to capitalization. This is however already mentioned in the other answer, so I don't see any reason to repeat it here.

So lets make a list of what's wrong:


first of all, the RSA key pair is the static key pair. That means that it should be pre-generated, and the public key must be distributed and trusted in advance. If the RSA and data specific AES key are generated in the same method (in this case the constructor), then something is seriously wrong.
Cipher instances carry state, and should preferably not be stored in fields. Storing static keys such as the RSA key pair is OK, but otherwise everything should just be created within the various methods. Cipher instances are relatively lightweight, so constructing / deconstructing them is relatively efficient (certainly compared against their operation). All in all, only the key pair field makes some sense; all the rest is unnecessary state.
The RSA key size of 2048 has about the same security as a 112 bit cipher such as 3DES. That's lower than AES-128 and much lower than AES-256. I'd use at least a 3072 bit key and prefer a 4096 bit key. Above that Elliptic Curve cryptography (ECIES) starts to make more sense (or a post-quantum algorithm).
SecureRandom.getSeed(16) retrieves data that can be used to seed a random number generator. Using a truly random IV is (more or less) required for CBC mode, but it is actually more secure and more performant to use SecureRandom#nextBytes.


Other smaller notes:


the name of the class: "Cryptographer" is of course horrible and it doesn't explain what the class does. This class does not represent a person.
It's "necessary" not "necesary". The shouting makes such spelling mistakes all the worse.




All in all, nice as a first try, don't use in production.