2
\$\begingroup\$

I am trying to implement message encryption/decryption with password in java. Here is the code I'm using:

import java.security.MessageDigest;
import java.security.spec.InvalidKeySpecException;
import java.util.Base64;
import javax.crypto.Cipher;
import javax.crypto.SecretKey;
import javax.crypto.SecretKeyFactory;
import javax.crypto.spec.IvParameterSpec;
import javax.crypto.spec.PBEKeySpec;
import javax.crypto.spec.SecretKeySpec;

public class Cryptography {

  public static final int SECRET_KEY_ITERATIONS = 45928;
  public static final int SALT_ITERATIONS = 11879;
  public static final int IV_ITERATIONS = 13275;
  public static final int KEY_LENGTH = 256;

  private static Cipher cipher;
  private static MessageDigest sha256;
  private static SecretKeyFactory secretKeyFactory;

  static {
    try {
      cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
      sha256 = MessageDigest.getInstance("SHA-256");
      secretKeyFactory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256");
    } catch (Exception e) {
      e.printStackTrace();
    }
  }

  public static String encryptText(String password, String plaintext) throws Exception {
    byte[] passwordBytes = password.getBytes();
    SecretKey secretKey = getKeyFromPassword(password, getSaltFromPassword(passwordBytes));
    IvParameterSpec ivParameterSpec = getIvFromPassword(passwordBytes);
    cipher.init(Cipher.ENCRYPT_MODE, secretKey, ivParameterSpec);
    return Base64.getEncoder().encodeToString(cipher.doFinal(plaintext.getBytes()));
  }

  public static String decryptText(String password, String ciphertext) throws Exception {
    byte[] passwordBytes = password.getBytes();
    SecretKey secretKey = getKeyFromPassword(password, getSaltFromPassword(passwordBytes));
    IvParameterSpec ivParameterSpec = getIvFromPassword(passwordBytes);
    cipher.init(Cipher.DECRYPT_MODE, secretKey, ivParameterSpec);
    return new String(cipher.doFinal(Base64.getDecoder().decode(ciphertext)));
  }

  private static SecretKey getKeyFromPassword(String password, byte[] salt)
      throws InvalidKeySpecException {
    PBEKeySpec spec = new PBEKeySpec(password.toCharArray(), salt, SECRET_KEY_ITERATIONS, KEY_LENGTH);
    byte[] bytes = secretKeyFactory.generateSecret(spec).getEncoded();
    return new SecretKeySpec(bytes, "AES");
  }

  private static byte[] getSaltFromPassword(byte[] passwordByteArray) {
    byte[] bytes = sha256.digest(passwordByteArray);
    for (int i = 0; i < SALT_ITERATIONS; i++) {
      bytes = sha256.digest(bytes);
    }
    return bytes;
  }

  private static IvParameterSpec getIvFromPassword(byte[] passwordByteArray) {
    byte[] bytes = sha256.digest(passwordByteArray);
    for (int i = 0; i < IV_ITERATIONS; i++) {
      bytes = sha256.digest(bytes);
    }
    byte[] ivBuffer = new byte[16];
    System.arraycopy(bytes, 0, ivBuffer, 0, 16);
    return new IvParameterSpec(ivBuffer);
  }
}

Is this a secure way to do encryption/decryption, or does it have serious issues?

\$\endgroup\$
0

2 Answers 2

Reset to default
2
\$\begingroup\$ 
  private static byte[] getSaltFromPassword(byte[] passwordByteArray) {
    byte[] bytes = sha256.digest(passwordByteArray);
    for (int i = 0; i < SALT_ITERATIONS; i++) {
      bytes = sha256.digest(bytes);
    }
    return bytes;
  }

this part is an NO NO. salt has to be random. please refer this old question The salt has to be independent and random else it is useless.

\$\endgroup\$
2
\$\begingroup\$

Yes, it has [serious issues].

For instance, because the salt is not unique, the code is fully deterministic. That means that you directly leak information if you encrypt strings with the same password (similar to how ECB leaks information).

The resulting ciphertext is not authenticated either, so supplying a wrong password may result in successful decryption. However, that decrypted plaintext will just consist of randomized bytes, not your original text.

I'll perform a code review with the comments below the code.

public static final int SECRET_KEY_ITERATIONS = 45928;
public static final int SALT_ITERATIONS = 11879;
public static final int IV_ITERATIONS = 13275;

Passwords need iterations, keys don't. Maybe KEY_DERIVATION_ITERATIONS would be acceptable though.

Salts may be used within iterations, but there is no such thing as a salt-iteration.

IVs don't have anything to do with iterations.

private static Cipher cipher;

Cipher is stateful, you should not use it as a field, let alone a static class field. If you want to store anything, store the resulting key, not the cipher algorithm.

static {

Try to avoid static blocks, just use a class without static.

public static String encryptText(String password, String plaintext) throws Exception {

Since cipher is stateful, can you imagine what happens if you call this method from separate threads? Everything will become a mess - if you are lucky something breaks down early.

private static byte[] getSaltFromPassword(byte[] passwordByteArray) {

A salt cannot be derived from a password as it is used to make the derived key from the password unique; that's the whole point. It just needs to be random. Otherwise, it doesn't make any sense to use a salt.

private static IvParameterSpec getIvFromPassword(byte[] passwordByteArray) {

Similar issue, the key / IV value should be unique. If the salt was random and stored with the ciphertext you could even use a static IV. Using a random IV is somewhat nicer, but then you'd have to store that as well.

Note that you are reinventing the wheel if you're using SHA-256 iterations; you might as well reuse PBKDF2. But in this case, it is not required anyway.

\$\endgroup\$

You must log in to answer this question.