Encrypting an AES key

Question

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:

1. Is this considered secure method to cipher messages
2. Is this efficient
3. 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);
    }
}
```

Answer 1

No, your code is not good enough.

It lacks the very basic API for an encryption and decryption module, which is to provide the secret and public key via a public method. This means that you cannot decrypt a message that was encrypted in a previous run of the program.

You should follow the Java Naming Conventions: Method names, field names, local variables and method parameters are all written in camelCase, starting with a lowercase letter.

The throws Exception is bad API design. Only throw those exceptions that are the fault of the caller. Since you are using predefined encryption algorithms, there cannot be any exception when doing the encryption. When decrypting though, there may be several exceptions being thrown (bad padding, invalid block size), which should all be covered by the unit tests you wrote. You did write them but just forgot to post them here, right?

Your code is missing the rationale for storing the AES key only in its encrypted form. That's a nice little trick to make it a single step more difficult to retrieve the private key, from an attacker's point of view. But then, after decrypting the private key and using it, you forgot to destroy it, therefore this hiding code doesn't really achieve much.

THERE IS NO NEED TO SHOUT IN THE COMMENTS: THIS IS CONSIDERED RUDE!!!1!!!!

While here, you should use an IDE or text editor that has an integrated spell checker, out of NECESITIES.

Using ECB sounds weak to me at first sight. I know that using ECB for AES is definitely weak, I'm not sure about using ECB with RSA.

For AES, I prefer counter mode over CBC.

There may be more weaknesses that I didn't mention.

Answer 2

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:

1. 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.

2. 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.

3. 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).

4. 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:

1. 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.

2. 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.