2
\$\begingroup\$

A little bit about myself: I am 18 years old, a student living in Germany and currently working on an android app.

I would like if someone has the time to review my code. I am especially interested if this code has any vulnerabilities and if the efficiency could be improved. Thanks in advance :)

import javax.crypto.*;
import javax.crypto.spec.*;
import java.security.*;
import java.security.spec.*;
import java.util.Base64;

public class AES {

    public static int iterations = 1000;
    private static String seperator = ";";
    private static int key_length = 256;
    private static int salt_length = 64;

    private static final String HASH_ALGORITHM = "PBKDF2WithHmacSHA256";
    private static final String KEY_ALGORITHM = "AES";
    private static final String CIPHER_ALGORITHM = "AES/CBC/PKCS5Padding";

    public static void setSaltLength(int length) throws Exception {
        if((length > 0) && ((length & (length - 1)) == 0))
            salt_length = length;
        else
            throw new Exception("Invalid Salt Length");

    }

    public static void setKeyLength(int length) throws Exception {
        if(length == 128 || length == 192 || length == 256)
            key_length = length;
        else
            throw new Exception("Invalid Length");
    }

    public static void setSeperator(String s) throws Exception {
        if(seperator.matches("[^+=/\\w]"))
            seperator = s;
        else
            throw new Exception("Invalid Seperator");
    }

    public static void setDurationOnCurrentComputer(int milliseconds){
        char[] password = {'t', 'e', 's', 't'};
        int i = 1;
        long duration = 0;
        while(duration < milliseconds) {
            i*=2;
            long t = System.currentTimeMillis();
            byte[] salt = new byte[64];
            new SecureRandom().nextBytes(salt);
            try {
                createKey(password, salt, i);
            } catch (Exception e) {
                e.printStackTrace();
            }
            duration = System.currentTimeMillis() - t;
            System.out.println("i: " + i + " duration: " + duration);
        }
        iterations = i;
    }

    private static SecretKey createKey(char[] password, byte[] salt, int iterations) throws NoSuchAlgorithmException, InvalidKeySpecException {
        SecretKeyFactory factory = SecretKeyFactory.getInstance(HASH_ALGORITHM);
        KeySpec spec = new PBEKeySpec(password, salt, iterations, key_length);
        SecretKey tmp = factory.generateSecret(spec);
        return new SecretKeySpec(tmp.getEncoded(), KEY_ALGORITHM);
    }

    public static String encrypt(char[] password, byte[] data) throws NoSuchPaddingException, NoSuchAlgorithmException, InvalidKeyException, InvalidParameterSpecException, BadPaddingException, IllegalBlockSizeException, InvalidKeySpecException {
        Cipher cipher = Cipher.getInstance(CIPHER_ALGORITHM);
        byte[] salt = new byte[salt_length];
        new SecureRandom().nextBytes(salt);
        cipher.init(Cipher.ENCRYPT_MODE, createKey(password, salt, iterations));
        AlgorithmParameters params = cipher.getParameters();
        byte[] iv = params.getParameterSpec(IvParameterSpec.class).getIV();
        byte[] ciphertext = cipher.doFinal(data);
        Base64.Encoder e = Base64.getEncoder();
        return e.encodeToString((e.encodeToString(iv) + seperator + e.encodeToString(ciphertext) + seperator + e.encodeToString(salt)).getBytes());
    }

    public static String decrypt(char[] password, String ciphertext) throws NoSuchPaddingException, NoSuchAlgorithmException, InvalidAlgorithmParameterException, InvalidKeyException, BadPaddingException, IllegalBlockSizeException, InvalidKeySpecException {
        Base64.Decoder d = Base64.getDecoder();
        String[] data = new String(d.decode(ciphertext)).split(seperator);
        Cipher cipher = Cipher.getInstance(CIPHER_ALGORITHM);
        cipher.init(Cipher.DECRYPT_MODE, createKey(password, d.decode(data[2]), iterations), new IvParameterSpec(d.decode(data[0])));
        return new String(cipher.doFinal(d.decode(data[1])));
    }
}
\$\endgroup\$
0
\$\begingroup\$
    public static int iterations = 1000;

That seems quite low. See e.g. 1, 2. Or is the idea that you will use setDurationOnCurrentComputer always?


    private static String seperator = ";";

Please correct the spelling to separator throughout the code.


    private static int key_length = 256;
    private static int salt_length = 64;

static? I find that surprising.


    private static final String CIPHER_ALGORITHM = "AES/CBC/PKCS5Padding";

Is there any good reason to choose CBC over GCM? The latter is more parallelisable, and I'm using it on Android API 19, which could fairly be described as ancient.


    public static void setSaltLength(int length) throws Exception {
        if((length > 0) && ((length & (length - 1)) == 0))
            salt_length = length;
        else
            throw new Exception("Invalid Salt Length");

    }

I think this method could use a few improvements:

  1. Why a checked exception? IllegalArgumentException looks like the natural fit here.
  2. Why must the salt length be a power of 2? That requirement is non-obvious, so it should be documented, and it wouldn't be a bad idea to also comment the test.
  3. The person who catches that exception will have rude words for the person who wrote the message. "Invalid Salt Length: " + length is much more useful for debugging.

    public static String encrypt(char[] password, byte[] data) throws NoSuchPaddingException, NoSuchAlgorithmException, InvalidKeyException, InvalidParameterSpecException, BadPaddingException, IllegalBlockSizeException, InvalidKeySpecException {

I think you could reasonably throw GeneralSecurityException and simplify that signature a bit.


        byte[] ciphertext = cipher.doFinal(data);

While this is tempting, I can tell you from personal experience that it may fail for large data blocks in some API levels. I can't remember offhand whether it was 64kB or 128kB which was the upper limit; to play it safe I suggest calling update with 16kB chunks.


        Base64.Encoder e = Base64.getEncoder();
        return e.encodeToString((e.encodeToString(iv) + seperator + e.encodeToString(ciphertext) + seperator + e.encodeToString(salt)).getBytes());
    }

Ok, that's not efficient. Base64 encoding adds an overhead of 33%. Do it twice and you have an overhead of 78%. Ditch the outer encodeToString.

Also, there are two problems with the contents. Putting salt at the end means that you can't start decrypting until you've read the entire string, which is a problem for stream-based operation. And failing to include the value of iterations in the string could mean that you aren't able to decrypt it later.


The hardest part of cryptography is key management. There are some clues that you intend this to be used only locally: i.e. you won't be transmitting the ciphertext to another computer to decipher it remotely. So I would strongly suggest that you look at KeyStore.

\$\endgroup\$
0
\$\begingroup\$

I am far to be a crypto expert but I do not see anything wrong.

However you should use the naming conventions for your fields. You can also consider to create an instance of your AES object or a via factory if you want to control the instance(s). This will remove all the static methods that can cause troubles in a concurrent environment.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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