Java RSA / AES-GCM encryption utility

Question

I wrote a utility class that has simple methods for:

• En/decrypting data with 256-bit AES-GCM using a password
• Generating RSA keypair
• Saving RSA keypair and storing private key encrypted

• En/decrypting data with RSA (data with AES-GCM & random key, key with RSA)

  import javax.crypto.*;
  import javax.crypto.spec.GCMParameterSpec;
  import javax.crypto.spec.PBEKeySpec;
  import javax.crypto.spec.SecretKeySpec;
  import java.io.FileOutputStream;
  import java.nio.file.Files;
  import java.nio.file.Paths;
  import java.security.*;
  import java.security.spec.*;
  import java.util.Arrays;
  
  public class GCM {
      private final int SALT_LENGTH = 256/8;
      private final int IV_LENGTH = 12;
      private final int KEY_LENGTH = 256;
      private final int GCM_TAG_LENGTH = 16;
  
      private KeyPair keyPair;
  
      public GCM() { }
  
      private SecretKeySpec generateKey(char[] password, byte[] salt)
              throws InvalidKeySpecException, NoSuchAlgorithmException {
          SecretKeyFactory factory = SecretKeyFactory.getInstance(/*"PBKDF2WithHmacSHA3-512"*/"PBKDF2WithHmacSHA256");
          KeySpec spec = new PBEKeySpec(password, salt, 65536, KEY_LENGTH);
          SecretKey tmp = factory.generateSecret(spec);
          SecretKeySpec secretKeySpec = new SecretKeySpec(tmp.getEncoded(), "AES");
  
          return secretKeySpec;
      }
  
      public void generateKeyPair() //generate 4096-bit RSA keypair
              throws NoSuchAlgorithmException {
          KeyPairGenerator kpg = KeyPairGenerator.getInstance("RSA");
          kpg.initialize(4096, new SecureRandom());
  
          keyPair = kpg.genKeyPair();
      }
  
      public void loadKeyPair(char[] password) { //loads RSA keypair from file
          try {
              byte[] publicKeyBytes = Files.readAllBytes(Paths.get("public.key"));
              byte[] privateKeyBytes = decrypt(Files.readAllBytes(Paths.get("private.key")), password);
              X509EncodedKeySpec x509EncodedKeySpec = new X509EncodedKeySpec(publicKeyBytes);
              KeyFactory keyFactory = KeyFactory.getInstance("RSA");
              PublicKey publicKey = keyFactory.generatePublic(x509EncodedKeySpec);
              PKCS8EncodedKeySpec pkcs8EncodedKeySpec = new PKCS8EncodedKeySpec(privateKeyBytes);
              PrivateKey privateKey = keyFactory.generatePrivate(pkcs8EncodedKeySpec);
              this.keyPair = new KeyPair(publicKey, privateKey);
          } catch (Exception e) {
              e.printStackTrace();
          }
      }
  
      public void saveKeyPair(char[] password) { //saves currently used keypair to file
          try {
              FileOutputStream fos = new FileOutputStream("private.key");
              fos.write(encrypt(keyPair.getPrivate().getEncoded(), password));
              fos.close();
              fos = new FileOutputStream("public.key");
              fos.write(keyPair.getPublic().getEncoded());
              fos.close();
          } catch (Exception e) {
              e.printStackTrace();
          }
      }
      public byte[] getPublicKey() { //returns currently used public key
          return keyPair.getPublic().getEncoded();
      }
  
      public byte[] encrypt(byte[] data, char[] password) { //encrypts data with password-generated key
          byte[] salt = new byte[SALT_LENGTH];
          byte[] iv = new byte[IV_LENGTH];
          byte[] result = null;
          new SecureRandom().nextBytes(salt);
          new SecureRandom().nextBytes(iv);
  
          try {
              SecretKeySpec secretKeySpec = generateKey(password, salt);
              byte[] encrypted = encrypt(data, secretKeySpec, iv);
              result = new byte[encrypted.length + salt.length + iv.length];
              System.arraycopy(salt, 0, result, 0, salt.length);
              System.arraycopy(iv, 0, result, salt.length, iv.length);
              System.arraycopy(encrypted,0, result, salt.length + iv.length, encrypted.length);
          } catch (Exception e) {
              e.printStackTrace();
          }
          return result;
      }
  
      public byte[] encrypt(byte[] data, byte[] publicKey) { //encrypts data with recipient public key
          byte[] iv = new byte[IV_LENGTH];
          byte[] result = null;
          new SecureRandom().nextBytes(iv);
  
          try {
              KeyGenerator keyGen = KeyGenerator.getInstance("AES");
              keyGen.init(256, new SecureRandom());
              SecretKey secretKey = keyGen.generateKey();
              SecretKeySpec secretKeySpec = new SecretKeySpec(secretKey.getEncoded(), "AES");
  
              byte[] encrypted = encrypt(data, secretKeySpec, iv);
  
              Cipher cipher = Cipher.getInstance("RSA/NONE/OAEPWithSHA3-512AndMGF1Padding");
              X509EncodedKeySpec keySpec = new X509EncodedKeySpec(publicKey);
              KeyFactory keyFactory = KeyFactory.getInstance("RSA");
              cipher.init(Cipher.ENCRYPT_MODE, keyFactory.generatePublic(keySpec));
              byte[] symmetricKey = cipher.doFinal(secretKeySpec.getEncoded());
  
              result = new byte[encrypted.length + iv.length + symmetricKey.length];
              System.arraycopy(symmetricKey, 0, result, 0, symmetricKey.length);
              System.arraycopy(iv, 0, result, symmetricKey.length, iv.length);
              System.arraycopy(encrypted,0, result, iv.length + symmetricKey.length, encrypted.length);
          } catch (Exception e) {
              e.printStackTrace();
          }
          return result;
      }
  
      private byte[] encrypt(byte[] data, SecretKeySpec secretKeySpec, byte[] iv) //encryption utility method
              throws BadPaddingException, IllegalBlockSizeException, NoSuchPaddingException,
              NoSuchAlgorithmException, InvalidAlgorithmParameterException, InvalidKeyException {
          Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding");
          GCMParameterSpec gcmParameterSpec = new GCMParameterSpec(GCM_TAG_LENGTH * 8, iv);
          cipher.init(Cipher.ENCRYPT_MODE, secretKeySpec, gcmParameterSpec);
          return cipher.doFinal(data);
      }
  
      public byte[] decrypt(byte[] data, char[] password) { //decrypts data with password-generated key
          byte[] salt = Arrays.copyOfRange(data, 0, SALT_LENGTH);
          byte[] iv = Arrays.copyOfRange(data, SALT_LENGTH, SALT_LENGTH + IV_LENGTH);
          byte[] encryptedData = Arrays.copyOfRange(data, SALT_LENGTH + IV_LENGTH, data.length);
          byte[] result = null;
  
          try {
              SecretKeySpec secretKeySpec = generateKey(password, salt);
              result = decrypt(encryptedData, secretKeySpec, iv);
          } catch (Exception e) {
              e.printStackTrace();
          }
          return result;
      }
  
      public byte[] decrypt(byte[] data) { //decrypts data with currently loaded private key
          byte[] symmetricKey = Arrays.copyOfRange(data, 0, 4096/8);
          byte[] iv = Arrays.copyOfRange(data, 4096/8, 4096/8 + IV_LENGTH);
          byte[] encryptedData = Arrays.copyOfRange(data, 4096/8 + IV_LENGTH, data.length);
          byte[] decrypted = null;
  
          try {
              Cipher cipher = Cipher.getInstance("RSA/NONE/OAEPWithSHA3-512AndMGF1Padding");
              cipher.init(Cipher.DECRYPT_MODE, keyPair.getPrivate());
              byte[] decryptedSymmetricKey = cipher.doFinal(symmetricKey);
  
              SecretKeySpec secretKeySpec = new SecretKeySpec(decryptedSymmetricKey, "AES");
              decrypted = decrypt(encryptedData, secretKeySpec, iv);
          } catch (Exception e) {
              e.printStackTrace();
          }
          return decrypted;
      }
  
      private byte[] decrypt(byte[] data, SecretKeySpec secretKeySpec, byte[] iv)
              throws NoSuchPaddingException, NoSuchAlgorithmException, InvalidAlgorithmParameterException,
              InvalidKeyException, BadPaddingException, IllegalBlockSizeException { //decryption utility method
          Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding");
          GCMParameterSpec gcmParameterSpec = new GCMParameterSpec(GCM_TAG_LENGTH * 8, iv);
          cipher.init(Cipher.DECRYPT_MODE, secretKeySpec, gcmParameterSpec);
  
          return cipher.doFinal(data);
      }
  }
  

The IV, and either RSA-encrypted symmetric key or salt used for key derivation are prepended to the output byte[].

It is used as follows:

GCM gcm = new GCM();

//symmetric encryption
byte[] encrypted = gcm.encrypt(byte[] data, char[] password);
byte[] decrypted = gcm.decrypt(byte[] encrypted, char[] password);

//asymmetric encryption
gcm.generateKeyPair();
gcm.saveKeyPair(char[] password);
gcm.loadKeyPair(char[] password);
byte[] encrypted = gcm.encrypt(byte[] data, byte[] publicKey);
byte[] decrypted = gcm.decrypt(byte[] data);

Since it is to be used in a closed system, compatibility isn't all that important (for example with key storage), but feel free to suggest alternatives.

Now I have the following questions about my code:

• Does it have any security flaws?
• Are AES-GCM's features used properly (eg. Authenticated Encryption)?
• Is the IV generation random enough to use with AES-GCM?
• Does it follow best practices?

Answer 1

Pokémon error handling "Catch them all"

Never (ever!) write code like this:

try {
    ...
} catch (Exception e) {
    ...
}

It allows the program to continue executing even if fatal errors are encountered which is very bad. Only catch errors that you can handle. In practice, that means that you almost never catch anything.

File handling

Saving and loading the key to the process' current working directory is not nice. Better to let the user pass a Path specifying the directory to use in the GCM constructor:

public GCM(Path path) {
    this.path = path;
}

It is also a good idea to store the filenames in constants because it guards against spelling errors:

private final String PUBLIC_KEY = "public.key";
private final String PRIVATE_KEY = "private.key";

So to get the path to "public.key", you write path.resolve(PUBLIC_KEY).

Mixing Java NIO with old style IO is not nice. Your code for writing the keypair to file can be replaced with Java NIO calls:

byte[] encodedPrivateKey = keyPair.getPrivate().getEncoded();
byte[] privateKeyBytes = encrypt(encodedPrivateKey, password);
Files.write(path.resolve(PRIVATE_KEY), privateKeyBytes);
byte[] publicKeyBytes = keyPair.getPublic().getEncoded();
Files.write(path.resolve(PUBLIC_KEY), publicKeyBytes);

Code organization

While reviewing your class I realized that it does two things; it handles key pairs and it encrypts/decrypts data. Ideally, each class should only do one job. Therefore I think a better way to organize the code is to have one class for the key pair handling and one for encryption/decryption.

Furthermore, the only state in your class is the keyPair variable which is not referenced in many of the methods. As a rule of thumb, if a method doesn't access any state then it should be made into a static method (a function). While refactoring that, I realized that your whole API can be better expressed as two static classes without any state.

Comments

If I were reviewing your code for a real project, I'd definitely complain about the lack of comments. There's a lot of things in there that is not obvious when reading the code. For example, what is an IV? Where does the number 4096 come from? What is GCM_TAG_LENGTH? And so on.

Result

With comments removed:

import javax.crypto.*;
import javax.crypto.spec.GCMParameterSpec;
import javax.crypto.spec.PBEKeySpec;
import javax.crypto.spec.SecretKeySpec;
import java.io.IOException;
import java.nio.file.*;
import java.security.*;
import java.security.spec.*;
import java.util.Arrays;

class GCM {
    private final static int SALT_LENGTH = 256/8;
    private final static int IV_LENGTH = 12;
    private final static int KEY_LENGTH = 256;
    private final static int GCM_TAG_LENGTH = 16;

    private static SecretKeySpec generateKey(char[] password, byte[] salt)
        throws InvalidKeySpecException, NoSuchAlgorithmException {
        SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256");
        KeySpec spec = new PBEKeySpec(password, salt, 65536, KEY_LENGTH);
        SecretKey tmp = factory.generateSecret(spec);
        SecretKeySpec secretKeySpec =
            new SecretKeySpec(tmp.getEncoded(), "AES");
        return secretKeySpec;
    }
    public static byte[] decrypt(byte[] data, KeyPair keyPair)
        throws GeneralSecurityException {
        byte[] symmetricKey = Arrays.copyOfRange(data, 0, 4096/8);
        byte[] iv = Arrays.copyOfRange(data, 4096/8, 4096/8 + IV_LENGTH);
        byte[] encryptedData = Arrays.copyOfRange(data, 4096/8 + IV_LENGTH,
                                                  data.length);
        Cipher cipher = Cipher.getInstance(
            "RSA/NONE/OAEPWithSHA3-512AndMGF1Padding");
        cipher.init(Cipher.DECRYPT_MODE, keyPair.getPrivate());
        byte[] decryptedSymmetricKey = cipher.doFinal(symmetricKey);

        SecretKeySpec secretKeySpec =
            new SecretKeySpec(decryptedSymmetricKey, "AES");
        return decrypt(encryptedData, secretKeySpec, iv);
    }
    private static byte[] decrypt(byte[] data,
                                  SecretKeySpec secretKeySpec,
                                  byte[] iv)
        throws GeneralSecurityException {
        Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding");
        GCMParameterSpec gcmParameterSpec =
            new GCMParameterSpec(GCM_TAG_LENGTH * 8, iv);
        cipher.init(Cipher.DECRYPT_MODE, secretKeySpec, gcmParameterSpec);

        return cipher.doFinal(data);
    }
    public static byte[] decrypt(byte[] data, char[] password)
        throws GeneralSecurityException {
        byte[] salt = Arrays.copyOfRange(data, 0, SALT_LENGTH);
        byte[] iv =
            Arrays.copyOfRange(data, SALT_LENGTH, SALT_LENGTH + IV_LENGTH);
        byte[] encryptedData =
            Arrays.copyOfRange(data, SALT_LENGTH + IV_LENGTH, data.length);

        SecretKeySpec secretKeySpec = generateKey(password, salt);
        return decrypt(encryptedData, secretKeySpec, iv);
    }
    public static byte[] encrypt(byte[] data, byte[] publicKey)
        throws GeneralSecurityException {
        byte[] iv = new byte[IV_LENGTH];
        new SecureRandom().nextBytes(iv);

        KeyGenerator keyGen = KeyGenerator.getInstance("AES");
        keyGen.init(256, new SecureRandom());
        SecretKey secretKey = keyGen.generateKey();
        SecretKeySpec secretKeySpec =
            new SecretKeySpec(secretKey.getEncoded(), "AES");
        byte[] encrypted = encrypt(data, secretKeySpec, iv);

        Cipher cipher =
            Cipher.getInstance("RSA/NONE/OAEPWithSHA3-512AndMGF1Padding");
        X509EncodedKeySpec keySpec = new X509EncodedKeySpec(publicKey);
        KeyFactory keyFactory = KeyFactory.getInstance("RSA");
        cipher.init(Cipher.ENCRYPT_MODE, keyFactory.generatePublic(keySpec));
        byte[] symmetricKey = cipher.doFinal(secretKeySpec.getEncoded());

        byte[] result = new byte[encrypted.length + iv.length
                                 + symmetricKey.length];
        System.arraycopy(symmetricKey, 0, result, 0, symmetricKey.length);
        System.arraycopy(iv, 0, result, symmetricKey.length, iv.length);
        System.arraycopy(encrypted,0, result, iv.length + symmetricKey.length, encrypted.length);
        return result;
    }
    public static byte[] encrypt(byte[] data, char[] password)
        throws GeneralSecurityException {
        byte[] salt = new byte[SALT_LENGTH];
        byte[] iv = new byte[IV_LENGTH];
        new SecureRandom().nextBytes(salt);
        new SecureRandom().nextBytes(iv);

        SecretKeySpec secretKeySpec = generateKey(password, salt);
        byte[] encrypted = encrypt(data, secretKeySpec, iv);
        byte[] result = new byte[encrypted.length + salt.length + iv.length];
        System.arraycopy(salt, 0, result, 0, salt.length);
        System.arraycopy(iv, 0, result, salt.length, iv.length);
        System.arraycopy(encrypted, 0, result, salt.length + iv.length, encrypted.length);
        return result;
    }
    private static byte[] encrypt(byte[] data,
                           SecretKeySpec secretKeySpec,
                           byte[] iv)
        throws GeneralSecurityException {
        Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding");
        GCMParameterSpec gcmParameterSpec = new GCMParameterSpec(GCM_TAG_LENGTH * 8, iv);
        cipher.init(Cipher.ENCRYPT_MODE, secretKeySpec, gcmParameterSpec);
        return cipher.doFinal(data);
    }
}
class KeyPairs {
    private final static String PUBLIC_KEY = "public.key";
    private final static String PRIVATE_KEY = "private.key";

    public static KeyPair generate() throws NoSuchAlgorithmException {
        KeyPairGenerator kpg = KeyPairGenerator.getInstance("RSA");
        kpg.initialize(4096, new SecureRandom());
        return kpg.genKeyPair();
    }
    public static KeyPair load(Path path, char[] password)
        throws IOException, GeneralSecurityException {
        Path publicKeyPath = path.resolve(PUBLIC_KEY);
        byte[] publicKeyBytes = Files.readAllBytes(publicKeyPath);
        Path privateKeyPath = path.resolve(PRIVATE_KEY);
        byte[] privateKeyBytes = GCM.decrypt(
            Files.readAllBytes(privateKeyPath),
            password);
        X509EncodedKeySpec x509EncodedKeySpec =
            new X509EncodedKeySpec(publicKeyBytes);
        KeyFactory keyFactory = KeyFactory.getInstance("RSA");
        PublicKey publicKey = keyFactory.generatePublic(x509EncodedKeySpec);
        PKCS8EncodedKeySpec pkcs8EncodedKeySpec =
            new PKCS8EncodedKeySpec(privateKeyBytes);
        PrivateKey privateKey = keyFactory.generatePrivate(pkcs8EncodedKeySpec);
        return new KeyPair(publicKey, privateKey);
    }
    public static void save(Path path, KeyPair keyPair, char[] password)
        throws IOException, GeneralSecurityException {
        byte[] encodedPrivateKey = keyPair.getPrivate().getEncoded();
        byte[] privateKeyBytes = GCM.encrypt(encodedPrivateKey, password);
        Files.write(path.resolve(PRIVATE_KEY), privateKeyBytes);
        byte[] publicKeyBytes = keyPair.getPublic().getEncoded();
        Files.write(path.resolve(PUBLIC_KEY), publicKeyBytes);
    }
}

I think your usage of the Java Crypto API is correct, but I'm no expert.