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);
 }
}
```

• Mast: "Is this considered secure method to cipher messages" First rule of Crypto: don't roll your own. Use a library that has this all set-up, follow the guidelines of the library you use. That's what the industry does.

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

Revisited and reworded in 2025

In general, I would not create any generic encryption classes. Encryption should be application specific and if it implements a protocol, the protocol should be described formally in a separate document that is not language specific.

Many of these classes wrap the knowledge of a developer, rather than representing a well defined and studied protocol. It is a way to try and remove the complexity of the cryptographic API, but in general that API is at least well vetted and described. Application specific cryptography is often significantly easier to maintain.

---

This tries 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 any data within the ciphertext can be changed and thereby plaintext blocks changed. Using GCM mode is recommended.

Adding a digital signature over the message is recommended if the goal is to know who send the message. Currently the 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, it would be necessary 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 the modulus size can be retrieved by using ((RSAPrivateKey)RSA_KEYS.getPrivate()).getModulus().bitLength() in the code. This is - by definition - the same as the key size in bits.

---

The code is clearly not keeping to any conventions when it comes to capitalization. This is however already mentioned in the other answer, so there is no 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. At least a 3072 bit key should be used and a 4096 bit key preferred. 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" doesn't explain what the class does. This class does not represent a person.
2. JavaDoc should be used for commenting on methods and SHOUTING should be forgone. Some spelling mistakes were also shown, enabling a spell checker is recommended, or colleagues will not assume quality code either.

---

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

Answer 3

Nice setup. Two things here will make or break the scheme.

1. You are reusing the IV across all messages. IvParameterSpec Iv = new IvParameterSpec(SecureRandom.getSeed(16)); is executed once, so every Encrypt(...) and Decrypt(...) call uses the same IV. With AES‐CBC, that is a critical flaw. CBC requires a fresh, unpredictable IV per encryption. Reusing it leaks relationships between first blocks and enables practical attacks. Generate a new IV each time, and store or prefix it with the ciphertext.

   // per-encrypt call
   byte[] iv = new byte[16]; // 16 bytes for AES block size
   SecureRandom rng = new SecureRandom();
   rng.nextBytes(iv);
   Cipher c = Cipher.getInstance("AES/CBC/PKCS5Padding");
   c.init(Cipher.ENCRYPT_MODE, aesKey, new IvParameterSpec(iv));
   byte[] ct = c.doFinal(plaintext);
   // serialize as: RSA(aesKey) || iv || ct
   

   NIST SP 800‐38A explicitly requires an unpredictable IV for CBC; predictable or reused IVs are unsafe.

2. Lock down OAEP parameters explicitly. You use "RSA/ECB/OAEPWithSHA-256AndMGF1Padding", which is fine, but different providers have different defaults for OAEP internals. Make the hash and MGF explicit to avoid cross‐provider mismatches and subtle downgrade bugs:

   Cipher rsa = Cipher.getInstance("RSA/ECB/OAEPWithSHA-256AndMGF1Padding");
   OAEPParameterSpec oaep = new OAEPParameterSpec(
    "SHA-256",
    "MGF1",
    MGF1ParameterSpec.SHA256,
    PSource.PSpecified.DEFAULT
   );
   rsa.init(Cipher.ENCRYPT_MODE, rsaPublicKey, oaep);
   byte[] encKey = rsa.doFinal(aesKey.getEncoded());
   

Also define a clear message format, for example: (rsaEncAesKey, iv, ciphertext), and document lengths so you can parse it unambiguously on decrypt. The other answers already discuss hybrid encryption at a high level; this pins down the wire format and provider‐safe OAEP setup.

• Sᴀᴍ Onᴇᴌᴀ: Ahoy! Did you write this independently, or was any of it generated by AI?