A better cryptography process

Question

I am experimenting with cryptography and I was wondering if somebody has some suggestion how I could improve my code and methodology.

I want my process to generate more securely the cryptography process if that is possible

import java.security.GeneralSecurityException;
import java.security.Key;
import java.util.Base64;
import javax.crypto.Cipher;
import javax.crypto.spec.SecretKeySpec;


@SuppressWarnings("ClassWithoutLogger")
public class Crypto_Security {

  static public final String CRY_ALGO = "AES";


  static public String encrypt(byte[] pSecret, String pValue) throws GeneralSecurityException {
    Key aesKey = new SecretKeySpec(pSecret, CRY_ALGO);
    Cipher cipher = Cipher.getInstance(CRY_ALGO);
    cipher.init(Cipher.ENCRYPT_MODE, aesKey);
    byte[] encrypted = cipher.doFinal(pValue.getBytes());
    Base64.Encoder encoder = Base64.getEncoder();
    return encoder.encodeToString(encrypted);
  }


  static public String decrypt(byte[] pSecret, String pEncrypted) throws GeneralSecurityException {
    SecretKeySpec skeySpec = new SecretKeySpec(pSecret, CRY_ALGO);
    Cipher cipher = Cipher.getInstance(CRY_ALGO);
    cipher.init(Cipher.DECRYPT_MODE, skeySpec);
    byte[] original = cipher.doFinal(Base64.getDecoder().decode(pEncrypted));
    return new String(original);
  }

  static public void main( String[] args ) {
    try {
      System.out.println("enc " + encrypt( "2144226321110063".getBytes() , "serial.txt" ));
      System.out.println("dcr " + decrypt( "2144226321110063".getBytes() , "oLQgEUxg3Z3zMSsSkiKpBg==" ));
    } catch( GeneralSecurityException e ) {
      e.printStackTrace();
    }
    return;
  }

  private Crypto_Security() {
  }


}

Answer 1

There really isn't all that much to see here when it comes to cryptography. Basically this is a wrapper class that tries to wrap the knowledge of the person that wrote it (that's you I suppose) rather than to implement security for a specific use case. In such a sense, it is only useful for practice purposes.

Never ever create such a class and make it the center of your security strategy (I've been there, steered there by "professionals", took me weeks to recover from it a couple of years later).

I'll just focus on the security parts:

1. a string is not a key, if you want to use a password, use a Password Based Key Derivation Mechanism such as PBKDF2 (and keep in mind that most passwords are insecure even if you do take that measure - a number is certainly not a great password);
2. just "AES" will default to the insecure ECB mode for most if not all Java / provider implementations - it is OK for the key creation, but not for the "transformation" parameter in Cipher.getInstance(String transformation): Cipher;
3. all other modes require an IV for generic messages, as otherwise identical messages (or parts of messages) may leak data - and even all data in the case of counter mode encryption;
4. nowadays authenticated ciphers such as GCM should be used; this makes sure that an adversary cannot alter the ciphertext undetected by the receiver - such alterations may even threaten the confidentiality of the message (see plaintext / padding oracle attacks);
5. using pValue.getBytes() is dangerous as getBytes will default to platform encoding, and the platform encoding may differ per platform (Windows uses Windows-1252 and Linux / Android uses UTF-8, to name just one such difference) - try e.g. pValue.getBytes(StandardCharsets.UTF_8) instead;
6. just throwing security exceptions doesn't make sense, as it doesn't distinguish between runtime issues (algorithm not available) and input / output issues - see here how to handle security exceptions;

---

The first thing I do in my IDE is to change e.printStackTrace() to new IllegalStateException("No exception yet specified", e);, usually with a // TODO change exception comment in front of it (picked up by the task system in Eclipse).

The problems with just printing the stack trace is that it is easily forgotten, it keeps running the code even after an exception is turned up and finally, it messes up the compilers detection of dead code.

---

CRY_ALGO of course is a cry-out for a better name.

Answer 2

From code's point of view (no security improvements), I have these points:

• Class name should be noun and in camelcase, no underscores. CryptoSecurity is very generic name, not sure if it's good one (but can't think of better one at the moment).

• Avoid static methods if possible. I suggest you instead make encrypt and decrypt class methods and crypto algorithm name (CRY_ALGO) class variable passed in constructor (you can even create default constructor to pass your "AES"). That way you can create different instances for different algorithms and don't need to worry about race conditions in case you would introduce any writable local variable.

  public class CryptoSecurity {
  
      static public final String CRY_ALGO = "AES";
  
      private String cryptoAlgorithm;
  
      public CryptoSecurity() {
          this(CRY_ALGO);
      }
  
      public CryptoSecurity(String cryptoAlgorithm) {
          this.cryptoAlgorithm = cryptoAlgorithm;
      }
  
  
      public String encrypt(byte[] pSecret, String pValue) throws GeneralSecurityException {
          Key aesKey = new SecretKeySpec(pSecret, cryptoAlgorithm);
          Cipher cipher = Cipher.getInstance(cryptoAlgorithm);
          cipher.init(Cipher.ENCRYPT_MODE, aesKey);
          byte[] encrypted = cipher.doFinal(pValue.getBytes());
          Base64.Encoder encoder = Base64.getEncoder();
          return encoder.encodeToString(encrypted);
      }
  
  
      public String decrypt(byte[] pSecret, String pEncrypted) throws GeneralSecurityException {
          SecretKeySpec skeySpec = new SecretKeySpec(pSecret, cryptoAlgorithm);
          Cipher cipher = Cipher.getInstance(cryptoAlgorithm);
          cipher.init(Cipher.DECRYPT_MODE, skeySpec);
          byte[] original = cipher.doFinal(Base64.getDecoder().decode(pEncrypted));
          return new String(original);
      }
  
      static public void main( String[] args ) {
          CryptoSecurity cs = new CryptoSecurity();
          try {
              System.out.println("enc " + cs.encrypt( "2144226321110063".getBytes() , "serial.txt" ));
              System.out.println("dcr " + cs.decrypt( "2144226321110063".getBytes() , "oLQgEUxg3Z3zMSsSkiKpBg==" ));
          } catch( GeneralSecurityException e ) {
              e.printStackTrace();
          }
      }
  }