Encrypt and Decrypt

Question

I just want to know if my code can be weak. Can anyone also spot a mistake in my code?

Encrypt

public static String byte2hex(byte[] b) {
 //we know exactly how long the resulting string should be so pass that information along to minimize allocations
 StringBuilder hs = new StringBuilder(b.length*2);
 for (int n = 0; n < b.length; n++) {
 String stmp = Integer.toHexString(b[n] & 0xFF);
 if (stmp.length() == 1)
 hs.append("0").append(stmp);
 else
 hs.append(stmp);
 }
 return hs.toString().toUpperCase();
}
public static byte[] encryptMsg(String secretKeyString, String msgContentString) {
 try {
 byte[] returnArray;
 //generate AES secret key from users input
 Key key = generateKey(secretKeyString);
 //specify the cipher algorithm using AES
 Cipher c = Cipher.getInstance("AES");
 // specify the encryption mode
 c.init(Cipher.ENCRYPT_MODE, key);
 // encrypt
 returnArray = c.doFinal(msgContentString.getBytes());
 return returnArray;
 }
 catch (Exception e)
 {
 e.printStackTrace();
 byte[] returnArray = null;
 return returnArray;
 }
}
private static Key generateKey(String secretKeyString) throws Exception {
 // generate secret key from string
 Key key = new SecretKeySpec(secretKeyString.getBytes(), "AES");
 return key;
}

Decrypt

public static byte[] hex2byte(byte[] b) {
 if ((b.length % 2) != 0)
 throw new IllegalArgumentException("hello");
 byte[] b2 = new byte[b.length / 2];
 for (int n = 0; n < b.length; n += 2) {
 String item = new String(b, n, 2);
 b2[n / 2] = (byte) Integer.parseInt(item, 16);
 }
 return b2;
}
public static byte[] decryptMsg(String secretKeyString, byte[] encryptedMsg)
 throws Exception {
 // generate AES secret key from the user input secret key
 Key key = generateKey(secretKeyString);
 // get the cipher algorithm for AES
 Cipher c = Cipher.getInstance("AES");
 // specify the decryption mode
 c.init(Cipher.DECRYPT_MODE, key);
 // decrypt the message
 byte[] decValue = c.doFinal(encryptedMsg);
 return decValue;
}
private static Key generateKey(String secretKeyString) throws Exception {
 // generate AES secret key from a String
 Key key = new SecretKeySpec(secretKeyString.getBytes(), "AES");
 return key;
}

• Mast: I just want to know if my code can be weak. Define weak? What exactly are your concerns, that it might be easy to decipher by a third party?
• 5gon12eder: I'm not familiar with Java's cryptography API so I won't post a review. As far as I understand, if you don't specify a mode, it will default to ECB, though, which I think you don't want. I also think that your code is incomplete. How did you derive secretKeyString? It seems very important. And I think you should use byte arrays (that you zero out) rather than Strings.

Answer 1

Using AES this way is weak, but that's not the topic for CodeReview. Head over to Security.SE for that. Java Crypto API by default has very restrictive limits to key size (no AES-256 gonna happen without special policy files). Another problem is that you use the passphrase as direct key material (no key derivation algorithms) and don't set a custom IV. So from a security point of view, this is bogus.

From a Java point of view, there are also a few problems

1. Why do you throw an IllegalArgumentException with message "hello"? Go for a much more descriptive message such as "The encoded Ciphertext must have an even number of hexadecimal characters."

2. You're doing a consistence check on the String length, but not on it's context. Instead you should verify that the following RegEx matches your Ciphertext:

   String ciphertext = new String(b);
   Pattern validator = Pattern.compile("^(?:[0-9A-Fa-f]{2})+$");
   if (!validator.matches(ciphertext)) {
    throw new IllegalArgumentException("The encoded Ciphertext must have an even number of hexadecimal characters.");
   }
   

   Since this Pattern is constant, you should actually make it private static final and put it into the class and not into the method.

3. Why do you accept a byte[] as input to hex2bytes routine if you then make a String from it and parse it? This way the user will have no control over the encoding of the String.
4. DRY. You have a duplicate method generateKey. Should you ever want to change it, it's going to happen in two places or break your program. Instead, you should put both, encryption and decryption into one class and only keep one method generateKey to derive the Key from the user-specified passphrase.
5. Consider using a finished third-party security library if you want to use cryptography in a production environment or for anything other than fooling around. There is just too much that can go wrong and make the Cryptosystem useless or prone to attacks.