Application and Logic

Bad Code - Do not use the SimpleCrypto class

Recently I noticed the class below in our codebase. A quick Google show that this is floating around on a number of websites and is quite popular. This class has a number of issues that makes anything encrypted using this code insecure. The class itself can be found at the bottom of this article.

Why it is bad

Throwing exceptions

This is just bad programming practice in general. Specific exceptions should be caught and dealt with. Especially here, the type of exception thrown can be a valuable clue to what went wrong when encrypting/decrypting.

Passing in a seed

Seeding the SecureRandom makes it useless, since (for a typical implementation) it will return the same key every time. The comment at the top of the class suggests using a master passsword as the seed. This is bad, since user passwords are generally low entropy. This makes them very, very bad seeds to generate keys from.

Instantiating the Cipher class

The Cipher class is instantiated using Cipher.getInstance("AES"). This means that the Cipher uses the default block mode, which is ECB (Electronic Code Book). ECB has been known to be insecure for a long time. If you want your stuff to be secure, do not ever use ECB!

Using SHA1PRNG

The implementation of SHA1PRNG is not well defined and different providers use different implementations (for example, the seed function may either replace or append the given seed). This can cause strange behaviour on devices that do not use the default provider.

How to fix it

Key generation

The AES key should be a randomly generated high-entropy value. If possible, use a hardware-backed key generator. After the key has been generated it should be stored securely, preferably protected using a user-defined password. If possible use the platform-provided keychain/keystore.

Encryption/decryption

Use a secure transformation, for example Cipher.getInstance("AES/CBC/PKCS5Padding") or Cipher.getInstance("AES/GCM/NoPadding") (see this link for an explanation of the difference between CBC and GCM).

Note that for both of these modes, you need an IV (initialisation vector). The IV should be generated and stored in much the same way as the key.

The offending class

import java.security.SecureRandom;
import javax.crypto.Cipher;
import javax.crypto.KeyGenerator;
import javax.crypto.SecretKey;
import javax.crypto.spec.SecretKeySpec;

/**
 * Usage:
 * <pre>
 * String crypto = SimpleCrypto.encrypt(masterpassword, cleartext)
 * ...
 * String cleartext = SimpleCrypto.decrypt(masterpassword, crypto)
 * </pre>
 * @author ferenc.hechler
 */
public class SimpleCrypto {

 public static String encrypt(String seed, String cleartext) throws Exception {
     byte[] rawKey = getRawKey(seed.getBytes());
     byte[] result = encrypt(rawKey, cleartext.getBytes());
     return toHex(result);
 }

 public static String decrypt(String seed, String encrypted) throws Exception {
     byte[] rawKey = getRawKey(seed.getBytes());
     byte[] enc = toByte(encrypted);
     byte[] result = decrypt(rawKey, enc);
     return new String(result);
 }

 private static byte[] getRawKey(byte[] seed) throws Exception {
     KeyGenerator kgen = KeyGenerator.getInstance("AES");
     SecureRandom sr = SecureRandom.getInstance("SHA1PRNG");
     sr.setSeed(seed);
     kgen.init(128, sr); // 192 and 256 bits may not be available
     SecretKey skey = kgen.generateKey();
     byte[] raw = skey.getEncoded();
     return raw;
 }


 private static byte[] encrypt(byte[] raw, byte[] clear) throws Exception {
     SecretKeySpec skeySpec = new SecretKeySpec(raw, "AES");
     Cipher cipher = Cipher.getInstance("AES");
     cipher.init(Cipher.ENCRYPT_MODE, skeySpec);
     byte[] encrypted = cipher.doFinal(clear);
     return encrypted;
 }

 private static byte[] decrypt(byte[] raw, byte[] encrypted) throws Exception {
     SecretKeySpec skeySpec = new SecretKeySpec(raw, "AES");
     Cipher cipher = Cipher.getInstance("AES");
     cipher.init(Cipher.DECRYPT_MODE, skeySpec);
     byte[] decrypted = cipher.doFinal(encrypted);
     return decrypted;
 }

 public static String toHex(String txt) {
     return toHex(txt.getBytes());
 }
 public static String fromHex(String hex) {
     return new String(toByte(hex));
 }

 public static byte[] toByte(String hexString) {
     int len = hexString.length()/2;
     byte[] result = new byte[len];
     for (int i = 0; i < len; i++)
   result[i] = Integer.valueOf(hexString.substring(2*i, 2*i+2), 16).byteValue();
     return result;
 }

 public static String toHex(byte[] buf) {
     if (buf == null)
   return "";
     StringBuffer result = new StringBuffer(2*buf.length);
     for (int i = 0; i < buf.length; i++) {
   appendHex(result, buf[i]);
     }
     return result.toString();
 }
 private final static String HEX = "0123456789ABCDEF";
 private static void appendHex(StringBuffer sb, byte b) {
     sb.append(HEX.charAt((b>>4)&0x0f)).append(HEX.charAt(b&0x0f));
 } 
}