2
\$\begingroup\$

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() {
  }


}
\$\endgroup\$
  • 1
    \$\begingroup\$ It seems to me that Base64 encoding doesn't really add any security. The most it would add is some obfuscation. \$\endgroup\$ – tinstaafl Dec 15 '19 at 4:24
  • \$\begingroup\$ "more securely" — more than what? \$\endgroup\$ – Roland Illig Jan 28 at 1:10
5
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

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();
            }
        }
    }
    
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ should i remove static from a security perspective? \$\endgroup\$ – jennifer ruurs Dec 15 '19 at 12:43
  • 2
    \$\begingroup\$ Removing static won't make your code more secure, it will just make it more flexible and according to coding standards. \$\endgroup\$ – K.H. Dec 15 '19 at 12:44

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.