Problem
So I am assuming that I did something wrong, and my AES and RSA encryption and decryption classes are insecure. I plan on using them for a larger project, and want to make sure I didn’t completely muff them first. My questions are as follows:
-
What, if anything, makes these two classes insecure?
-
What, if anything, could be neater/better form about my code?
-
What am I not thinking to ask?
AES:
import javax.crypto.Cipher;
import javax.crypto.KeyGenerator;
import javax.crypto.SecretKey;
/**
* Created by Gabriel Wittes on 3/15/2016.
* A class to encrypt and decrypt AES plaintext and ciphertext, as well as to generate AES keys.
*/
public class AES {
/**
* Returns a new secret AES key.
* @return a secret AES key
*/
public static SecretKey generateKey(){
KeyGenerator keyGenerator = null;
try {
keyGenerator = KeyGenerator.getInstance("AES");
keyGenerator.init(128);
} catch (Exception e) {
e.printStackTrace();
}
return keyGenerator.generateKey();
}
/**
* Returns an AES-encrypted byte array given a plaintext string and a secret AES key.
* @param plaintext a plaintext string
* @param key a secret AES key
* @return ciphertext
*/
public static byte[] encrypt(String plaintext, SecretKey key){
byte[] ciphertext = null;
try {
Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
cipher.init(Cipher.ENCRYPT_MODE, key);
ciphertext = cipher.doFinal(plaintext.getBytes());
} catch (Exception e) {
e.printStackTrace();
}
return ciphertext;
}
/**
* Returns the plaintext of a given AES-encrypted string, and a secret AES key.
* @param ciphertext an AES encrypted byte array
* @param key a secret AES key
* @return plaintext
*/
public static String decrypt(byte[] ciphertext, SecretKey key){
byte[] plaintext = null;
try {
Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
cipher.init(Cipher.DECRYPT_MODE, key);
plaintext = cipher.doFinal(ciphertext);
} catch (Exception e) {
e.printStackTrace();
}
return new String(plaintext);
}
}
RSA:
import javax.crypto.Cipher;
import java.security.*;
/**
* Created by Gabriel Wittes on 3/15/2016.
* A class to encrypt and decrypt RSA plaintext and ciphertext, as well as to generate RSA key pairs.
*/
public class RSA {
/**
* Returns a new RSA key pair.
* @return public and private RSA keys
*/
public static KeyPair generateKeyPair(){
KeyPairGenerator keyPairGenerator = null;
try {
keyPairGenerator = KeyPairGenerator.getInstance("RSA");
keyPairGenerator.initialize(2048);
} catch (Exception e) {
e.printStackTrace();
}
return keyPairGenerator.generateKeyPair();
}
/**
* Returns an RSA-encrypted byte array given a plaintext string and a public RSA key.
* @param plaintext a plaintext string
* @param key a public RSA key
* @return ciphertext
*/
public static byte[] encrypt(String plaintext, PublicKey key){
byte[] ciphertext = null;
try {
Cipher cipher = Cipher.getInstance("RSA/ECB/OASP");
cipher.init(Cipher.ENCRYPT_MODE, key);
ciphertext = cipher.doFinal(plaintext.getBytes());
} catch (Exception e) {
e.printStackTrace();
}
return ciphertext;
}
/**
* Returns the plaintext of a given RSA-encrypted string and a private RSA key.
* @param ciphertext an RSA-encrypted byte array
* @param key a private RSA key
* @return plaintext
*/
public static String decrypt(byte[] ciphertext, PrivateKey key){
byte[] plaintext = null;
try {
Cipher cipher = Cipher.getInstance("RSA/ECB/OASP");
cipher.init(Cipher.DECRYPT_MODE, key);
plaintext = cipher.doFinal(ciphertext);
} catch (Exception e) {
e.printStackTrace();
}
return new String(plaintext);
}
}
Solution
Three comments:
Firstly, don’t catch exceptions like that, especially not all
exceptions. Ideally you want to be notified as early as possible about
errors, not when the next call fails, so I don’t see a good reason to
put the catch there. If you really want to transform the exception
into another return value (or exception) then make that more explicit.
Also. printStackTrace doesn’t play well with logging frameworks, so
there’s that too.
Secondly, why are all the methods static? That won’t (easily) allow you
to mock out this class, which is a nice thing to have when testing (you
have tests, right?)
Thirdly, the string constructor should have an explicit character set
argument, or at least you should ensure that the default platform
encoding is what you expect (UTF-8 of course). The string constants
should also be declared as private static final String ALGORITHM = ...
or so.
Your classes looks secure to me, the only suggestions I have are
-
make your methods more generic: I would use the
byte[]type both for arguments and return typespublic class AES { ... public static byte[] encrypt(byte[] plain, SecretKey key) { ... } public static byte[] decrypt(byte[] encrypted, SecretKey key) { ... } } public class RSA { ... public static byte[] encrypt(byte[] plain, PublicKey key) { ... } public static byte[] decrypt(byte[] encrypted, PrivateKey key) { ... } }this because you may have not only strings to encrypt/decrypt, and you should always encrypt/decrypt strings calling e.g.
AES.decrypt(string.getBytes(), key); -
Add methos to
RSAclass to encrypt withPrivateKeyand decrypt withPublicKey:public class RSA { ... public static byte[] encrypt(byte[] plain, PublicKey key) { ... } public static byte[] decrypt(byte[] encrypted, PrivateKey key) { ... } public static byte[] encrypt(byte[] plain, PrivateKey key) { ... } public static byte[] decrypt(byte[] encrypted, PublicKey key) { ... } }this because RSA can works in both ways, e.g. in digital signature the sender encrypts the digest of the message with his private key, so the receiver can decrypt the digest with the sender’s public key in order to verify integrity and authenticity of the message