4
\$\begingroup\$

So I am assuming that I did something wrong, and my AES and RSA encryption and decryption classes are insecure. I plan on using them for a larger project, and want to make sure I didn't completely muff them first. My questions are as follows:

  1. What, if anything, makes these two classes insecure?

  2. What, if anything, could be neater/better form about my code?

  3. What am I not thinking to ask?

AES:

import javax.crypto.Cipher;
import javax.crypto.KeyGenerator;
import javax.crypto.SecretKey;

/**
 * Created by Gabriel Wittes on 3/15/2016.
 * A class to encrypt and decrypt AES plaintext and ciphertext, as well as to generate AES keys.
 */
public class AES {
    /**
     * Returns a new secret AES key.
     * @return a secret AES key
     */
    public static SecretKey generateKey(){
        KeyGenerator keyGenerator = null;
        try {
            keyGenerator = KeyGenerator.getInstance("AES");
            keyGenerator.init(128);
        } catch (Exception e) {
            e.printStackTrace();
        }
        return keyGenerator.generateKey();
    }

    /**
     * Returns an AES-encrypted byte array given a plaintext string and a secret AES key.
     * @param plaintext a plaintext string
     * @param key a secret AES key
     * @return ciphertext
     */
    public static byte[] encrypt(String plaintext, SecretKey key){
        byte[] ciphertext = null;
        try {
            Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
            cipher.init(Cipher.ENCRYPT_MODE, key);
            ciphertext = cipher.doFinal(plaintext.getBytes());
        } catch (Exception e) {
            e.printStackTrace();
        }
        return ciphertext;
    }

    /**
     * Returns the plaintext of a given AES-encrypted string, and a secret AES key.
     * @param ciphertext an AES encrypted byte array
     * @param key a secret AES key
     * @return plaintext
     */
    public static String decrypt(byte[] ciphertext, SecretKey key){
        byte[] plaintext = null;
        try {
            Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
            cipher.init(Cipher.DECRYPT_MODE, key);
            plaintext = cipher.doFinal(ciphertext);
        } catch (Exception e) {
            e.printStackTrace();
        }
        return new String(plaintext);
    }
}

RSA:

import javax.crypto.Cipher;
import java.security.*;

/**
 * Created by Gabriel Wittes on 3/15/2016.
 * A class to encrypt and decrypt RSA plaintext and ciphertext, as well as to generate RSA key pairs.
 */
public class RSA {
    /**
     * Returns a new RSA key pair.
     * @return public and private RSA keys
     */
    public static KeyPair generateKeyPair(){
        KeyPairGenerator keyPairGenerator = null;
        try {
            keyPairGenerator = KeyPairGenerator.getInstance("RSA");
            keyPairGenerator.initialize(2048);
        } catch (Exception e) {
            e.printStackTrace();
        }
        return keyPairGenerator.generateKeyPair();
    }

    /**
     * Returns an RSA-encrypted byte array given a plaintext string and a public RSA key.
     * @param plaintext a plaintext string
     * @param key a public RSA key
     * @return ciphertext
     */
    public static byte[] encrypt(String plaintext, PublicKey key){
        byte[] ciphertext = null;
        try {
            Cipher cipher = Cipher.getInstance("RSA/ECB/OASP");
            cipher.init(Cipher.ENCRYPT_MODE, key);
            ciphertext = cipher.doFinal(plaintext.getBytes());
        } catch (Exception e) {
            e.printStackTrace();
        }
        return ciphertext;
    }

    /**
     * Returns the plaintext of a given RSA-encrypted string and a private RSA key.
     * @param ciphertext an RSA-encrypted byte array
     * @param key a private RSA key
     * @return plaintext
     */
    public static String decrypt(byte[] ciphertext, PrivateKey key){
        byte[] plaintext = null;
        try {
            Cipher cipher = Cipher.getInstance("RSA/ECB/OASP");
            cipher.init(Cipher.DECRYPT_MODE, key);
            plaintext = cipher.doFinal(ciphertext);
        } catch (Exception e) {
            e.printStackTrace();
        }
        return new String(plaintext);
    }
}
\$\endgroup\$
  • \$\begingroup\$ Is OASP a Java idiosyncrasy or is that supposed to be OAEP? \$\endgroup\$ – CodesInChaos Mar 16 '16 at 8:32
3
\$\begingroup\$

Three comments:

Firstly, don't catch exceptions like that, especially not all exceptions. Ideally you want to be notified as early as possible about errors, not when the next call fails, so I don't see a good reason to put the catch there. If you really want to transform the exception into another return value (or exception) then make that more explicit. Also. printStackTrace doesn't play well with logging frameworks, so there's that too.

Secondly, why are all the methods static? That won't (easily) allow you to mock out this class, which is a nice thing to have when testing (you have tests, right?)

Thirdly, the string constructor should have an explicit character set argument, or at least you should ensure that the default platform encoding is what you expect (UTF-8 of course). The string constants should also be declared as private static final String ALGORITHM = ... or so.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I don't understand the advantage of not having the methods static. I'm going to want to be able to generate keys without a constructed instance of the class... \$\endgroup\$ – Denthil Mar 16 '16 at 17:11
  • \$\begingroup\$ If you have a class Foo that directly uses the static methods you have no way of testing whether e.g. generateKeyPair was called. If you put an indirection in using a passed-in instance of RSA you could pass in a mock during testing and verify that, indeed, the generateKeyPair method was called. \$\endgroup\$ – ferada Mar 16 '16 at 17:39
2
\$\begingroup\$

Your classes looks secure to me, the only suggestions I have are

  1. make your methods more generic: I would use the byte[] type both for arguments and return types

    public class AES {
        ...
        public static byte[] encrypt(byte[] plain, SecretKey key) {
            ...
        }
    
        public static byte[] decrypt(byte[] encrypted, SecretKey key) {
            ...
        }
    }
    
    public class RSA {
        ...
        public static byte[] encrypt(byte[] plain, PublicKey key) {
            ...
        }
    
        public static byte[] decrypt(byte[] encrypted, PrivateKey key) {
            ...
        }
    }
    

    this because you may have not only strings to encrypt/decrypt, and you should always encrypt/decrypt strings calling e.g. AES.decrypt(string.getBytes(), key);

  2. Add methos to RSA class to encrypt with PrivateKey and decrypt with PublicKey:

    public class RSA {
        ...
        public static byte[] encrypt(byte[] plain, PublicKey key) {
            ...
        }
    
        public static byte[] decrypt(byte[] encrypted, PrivateKey key) {
            ...
        }
    
        public static byte[] encrypt(byte[] plain, PrivateKey key) {
            ...
        }
    
        public static byte[] decrypt(byte[] encrypted, PublicKey key) {
            ...
        }
    }
    

    this because RSA can works in both ways, e.g. in digital signature the sender encrypts the digest of the message with his private key, so the receiver can decrypt the digest with the sender's public key in order to verify integrity and authenticity of the message

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks, I'll add these. I don't think I'm going to use digital signature for this project though, are there any other use cases for RSA encryption with a private key and vice versa? \$\endgroup\$ – Denthil Mar 16 '16 at 17:15
  • \$\begingroup\$ @Denthil in general each case when a message is sent (or broadcasted) and the receivers need to be sure about the identity of the sender and/or need the non-ripudiation of the sender, so the main application is in digital signature that ensures also integrity \$\endgroup\$ – Francesco Pitzalis Mar 17 '16 at 9:37

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.