Encrypt and Decrypt

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 ciph

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

• 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."

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

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

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

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

StackExchange Code Review Q#118383, answer score: 2