patternjavaMinor
AES and RSA using javax.crypto
Viewed 0 times
rsajavaxaescryptousingand
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[] plaint
-
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[] plaint
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
into another return value (or exception) then make that more explicit.
Also.
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
or so.
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 exceptioninto another return value (or exception) then make that more explicit.
Also.
printStackTrace doesn't play well with logging frameworks, sothere'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.
Context
StackExchange Code Review Q#122971, answer score: 3
Revisions (0)
No revisions yet.