Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Is this implementation of AES for Android safe? Is it 128 bit encryption? How can I strengthen this implementation? Please help me, all suggestions are welcome :)

import java.security.NoSuchAlgorithmException;
import javax.crypto.Cipher;
import javax.crypto.NoSuchPaddingException;
import javax.crypto.spec.IvParameterSpec;
import javax.crypto.SecretKey;
import javax.crypto.SecretKeyFactory;
import javax.crypto.spec.SecretKeySpec;

public class AESEncryption {

    private static final String SecretKey = "9081726354fabced";
    private static final String Salt = "03xy9z52twq8r4s1uv67";
    private static final String CIPHER_ALGORITHM = "AES/CBC/PKCS5Padding";
    private static final String KEY_ALGORITHM = "AES";
    private static final String SECRET_KEY_ALGORITHM = "PBEWithSHA256And256BitAES-CBC-BC";
    private static final String RANDOM_ALGORITHM = "SHA-512";
    private static final int PBE_ITERATION_COUNT = 100;
    private static final int PBE_KEY_LENGTH = 256;
    private static final int IV_LENGTH = 16;
    private Cipher cipher;

    public AESEncryption() {
        try {
            cipher = Cipher.getInstance(CIPHER_ALGORITHM);
        }
        catch (NoSuchAlgorithmException e) {
            cipher = null;
        }
        catch (NoSuchPaddingException e) {
            cipher = null;
        }
        randomInt = new Random();
    }

    public SecretKey getSecretKey(String password) {
            try {
                PBEKeySpec pbeKeySpec = new PBEKeySpec(password.toCharArray(), Salt.getBytes("UTF-8"), PBE_ITERATION_COUNT, PBE_KEY_LENGTH);
                SecretKeyFactory factory = SecretKeyFactory.getInstance(SECRET_KEY_ALGORITHM);
                SecretKey tmp = factory.generateSecret(pbeKeySpec);
                SecretKey secret = new SecretKeySpec(tmp.getEncoded(), KEY_ALGORITHM);
                return secret;
            }
            catch (Exception e) {
                return null;
            }
    }

    public String encrypt(String text) {
        Thread.sleep(randomInt.nextInt(100));
        if (text == null || text.length() == 0) {
            return null;
        }
        byte[] encrypted = null;
        try {
            byte[] iv = generateIV();
            IvParameterSpec ivspec = new IvParameterSpec(iv);
            cipher.init(Cipher.ENCRYPT_MODE, getSecretKey(SecretKey), ivspec);
            encrypted = cipher.doFinal(text.getBytes("UTF-8"));
        }
        catch (Exception e) {                       
            return null;
        }
        return Base64.encodeToString(iv, Base64.DEFAULT)+Base64.encodeToString(encrypted, Base64.DEFAULT);
    }

    public String decrypt(String code) {
        Thread.sleep(randomInt.nextInt(100));
        if(code == null || code.length() == 0) {
            return null;
        }
        byte[] decrypted = null;
        try {
            String iv64 = code.substring(0, IV_LENGTH*2);
            String encrypted64 = code.substring(IV_LENGTH*2);
            byte[] iv = Base64.decode(iv64, Base64.DEFAULT);
            IvParameterSpec ivspec = new IvParameterSpec(iv);
            cipher.init(Cipher.DECRYPT_MODE, getSecretKey(SecretKey), ivspec);
            decrypted = cipher.doFinal(Base64.decode(encrypted64, Base64.DEFAULT));
        }
        catch (Exception e) {
            return null;
        }
        return new String(decrypted, "UTF-8");
    }

    private byte[] generateIV() {
        try {
            SecureRandom random = SecureRandom.getInstance(RANDOM_ALGORITHM);
            byte[] iv = new byte[IV_LENGTH];
            random.nextBytes(iv);
            return iv;
        }
        catch (Exception e) {
            return null;
        }
    }

    /*public static String bytesToHex(byte[] data) {
        String HEXES = "0123456789ABCDEF";
        if (data == null) {
            return null;
        }
        final StringBuilder hex = new StringBuilder(2*data.length);
        for (final byte b : data) {
            hex.append(HEXES.charAt((b & 0xF0) >> 4)).append(HEXES.charAt((b & 0x0F)));
        }
        return hex.toString();
    }*/


    /*public static byte[] hexToBytes(String str) {
        if (str == null) {
            return null;
        }
        else if (str.length() < 2) {
            return null;
        }
        else {
            int len = str.length()/2;
            byte[] buffer = new byte[len];
            for (int i = 0; i < len; i++) {
                buffer[i] = (byte) Integer.parseInt(str.substring(i*2,i*2+2),16);
            }
            return buffer;
        }
    }*/

}

Thanks a lot in advance!

Edit: The code throws an exception because Android can't find "PBEWithSHA256And256BitAES-CBC-BC". This is when I run the code on my phone, at least. What algorithm can I take now to replace this unknown one? The algorithm for SecretKeyFactory must be compatible to AES and the key size, right?

share|improve this question
    
Hi pal, not to sound like a bad sport, but does this class come with any license restriction to it? You don't specify in the headers and i would like to know. Thanks in advance –  Chiguireitor Dec 13 '12 at 13:39
add comment

1 Answer

up vote 8 down vote accepted
+100

I observe the following deficiencies:

  • Use of strings to store the key, which is then extracted using getBytes(). This has multiple problems:
    • The character encoding used to convert the string to bytes can vary between Java implementations (so while the Android spec forces UTF-8 to be the default character encoding, your code won't be portable to J2SE, even though it will compile - dangerous). String.getBytes() should be deprecated if it isn't already; you should always supply an argument specifying the character encoding to use (and 99% of the time it should be "UTF-8").
    • Certain character encodings (including UTF-8) place additional constraints on the valid values of bytes which mean that you lose some of the keyspace. It would be better to base-64 encode it (and use android.util.Base64 to decode) or hex-encode it (using the static methods which you've posted - although see below).
  • Bad padding. If you look at that code carefully you'll see that it pads with spaces on encryption and doesn't unpad on decryption. So you'll have to unpad in the next layer up, and if the encrypted string ends in one or more spaces you'll unpad incorrectly. I would remove padString and the call to it, and change the cipher algorithm to AES/CBC/PKCS7 (assuming that it's supported; if not, try AES/CBC/PKCS5).
  • Reuse of IV. In certain applications this is excusable, but as a general rule you should use a different, randomly generated, IV for each message and send it (unencrypted) along with the message. Otherwise if your messages start out with the same text then an eavesdropper can get information on where they diverge.
  • Whoever wrote bytesToHex is probably more experienced with PHP than Java, because otherwise they would know that building a string in a loop like that is inexcusable for someone who's been programming Java for more than two weeks. Given that the size of the output is known from the start, I'd be inclined to just build it into a char[] and then call the String(char[]) constructor rather than use a StringBuilder.
share|improve this answer
    
Thank you very much, Peter! I tried to implement all your suggestions (except for the IV reuse, which I want to retain). Did I succeed? The class should be more secure now, isn't it? –  Marco W. Dec 31 '11 at 3:30
    
@MarcoW., is there a reason for using PBE rather than generating a key with full entropy and hex-coding it? Also, I didn't pay enough attention to the asymmetry between encode and decode. Probably both encrypt and decrypt should take strings and return strings. –  Peter Taylor Dec 31 '11 at 11:21
    
Thanks, I've changed both methods to return strings. But concerning PBE, I thought this would be a safe way of retrieving a 256 bit key. Is it not? I need the fixed-length key. –  Marco W. Dec 31 '11 at 11:53
    
@MarcoW., it's not bad per se, but it's a bit overkill if you're hard-coding the key. What do you mean by "fixed-length"? –  Peter Taylor Dec 31 '11 at 12:04
1  
True, there's still power analysis, but that requires fairly direct access to the device whereas timing attacks don't always. Yes, the base-64 is just to produce shorter strings. –  Peter Taylor Jan 2 '12 at 13:32
show 6 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.