Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I use some basic cryptography in a project of mine.
Because I want my application to run cross-platform and because it seems pretty sophisticated I decided to go with the Java Cryptography Architecture.

I'm pretty fresh with crypto in general, so I'd like to hear someone's opinion who has some experience with that.

First, I have some convenience wrapper classes for the different symmetric ciphers I support:

public abstract class PwsCipher {
  //=== methods ================================================================
  /**
   * Gets the cipher's block length.
   * @return Block length in bytes.
   */
  int getBlockLength()
          throws NoSuchAlgorithmException, NoSuchPaddingException,
          NoSuchAlgorithmException, NoSuchProviderException {
    return createCipher().getBlockSize();
  }

  /**
   * Creates a new {@link javax.crypto.cipher} instance of this
   * cipher.
   * @return Instance of {@link Cipher}.
   */
  Cipher createCipher()
          throws NoSuchAlgorithmException, NoSuchPaddingException,
          NoSuchProviderException {
    String provider = "SC"; // Spongy Castle for android
    if ( !System.getProperty("java.vm.name").equals("Dalvik") ) {
      provider = "BC"; // Bouncy Castle for everything else
    }
    return Cipher.getInstance(getName() + "/CBC/PKCS7Padding", provider);
  }
  //=== getter & setter ========================================================

  /**
   * Gets the name of the cipher.
   * @return Cipher name.
   */
  abstract String getName();

  /**
   * Gets the key length of the cipher in bytes.
   * @return Key length in bytes.
   */
  abstract int getKeyLength();

}

public class PwsCipherAES extends PwsCipher {
  //=== getter & setter ========================================================
  @Override
  public String getName() {
    return "AES";
  }
  @Override
  public int getKeyLength() {
    return 32;
  }
}

public class PwsCipherBlowfish extends PwsCipher {
  //=== getter & setter ========================================================
  @Override
  public String getName() {
    return "Blowfish";
  }
  @Override
  public int getKeyLength() {
    return 56;
  }
}

public class PwsCipherDES3 extends PwsCipher {
  //=== getter & setter ========================================================
  @Override
  public String getName() {
    return "DES3";
  }
  @Override
  public int getKeyLength() {
    return 24;
  }
}

Then, I have a Passphrase class which I use to create a SHA-512 hash and salt for a clear text passphrase:

public class Passphrase {
  //=== attributes =============================================================
  /** The salt value of the passphrase. */
  private byte[] salt;

  /** The hashed passphrase. */
  private byte[] hashed;

  //=== constructors ===========================================================
  /**
   * Creates a new instance of {@code Passphrase}.
   * Assumes that the passphprase is not hashed, and that a new, random salt
   * value should be created.
   * @param phrase Clear text passphrase. Must not be hashed yet.
   */
  public Passphrase(String clearPhrase) throws NoSuchAlgorithmException {
    this(clearPhrase, false, null);
  }

  /**
   * Creates a new instance of {@code Passphrase}.
   * Assumes that the passphrase is not hashed.
   * @param phrase Clear text passphrase.
   * @param salt Salt of the passphrase. May be {@code null}. If so, a new
   * random salt will be created.
   */
  public Passphrase(String clearPhrase, byte[] salt)
      throws NoSuchAlgorithmException {
    this(clearPhrase, false, salt);
  }

  /**
   * Creates a new instance of {@code Passphrase}.
   * @param phrase Clear text or hashed passphrase.
   * @param isHashed Tells if the passphrase is already hashed. If {@code false}
   * the hash of the clear text passphrase will be created.
   * @param salt Salt of the passphrase. May be {@code null}. If so, a new
   * random salt will be created.
   */
  public Passphrase(String phrase, boolean isHashed, byte[] salt)
      throws NoSuchAlgorithmException {
    setSalt(salt);
    setHash(phrase, isHashed);
  }
  //=== static methods =========================================================
  //=== methods ================================================================

  /**
   * Creates a random byte array with length 64.
   * @return Random byte array.
   */
  public final byte[] createRandomSalt() {
    Random r = new SecureRandom();
    byte[] slt = new byte[64];
    r.nextBytes(slt);
    return slt;
  }

  //=== getter & setter ========================================================

  /**
   * Sets the salt of the Passphrase. <br/>
   * May be {@code null}. If that is the case, a new, random salt will created.
   * @param salt Salt to be set. May be {@code null}. In that case a random salt
   * will be generated. Should not be longer than hash length, which is 64 bytes
   */
  private void setSalt(byte[] salt) {
    if ( salt == null ) {
      this.salt = createRandomSalt();
    } else {
      this.salt = salt;
    }
  }

  /**
   * Sets the hashed passphrase. <br/>
   * If the passphrase is not hashed, it will be using SHA-512 and the
   * previously set salt.
   * @param phrase Hashed or clear text passphrase.
   * @param isHashed Tells if the passphrase is already hashed.
   */
  private void setHash(String phrase, boolean isHashed)
      throws NoSuchAlgorithmException {
    if ( isHashed ) {
      this.hashed = phrase.getBytes();
    } else {
      MessageDigest md = MessageDigest.getInstance("SHA-512");
      byte[] salted = new byte[phrase.length() + salt.length];
      byte[] clear = phrase.getBytes();
      System.arraycopy(clear, 0, salted, 0, clear.length);
      System.arraycopy(salt, 0, salted, clear.length, salt.length);
      this.hashed = md.digest(salted);
      //@TODO: nullify byte[] clear, as it contains clear text passphrase, which
      // we don't want to be swapped or debugged
    }
  }

  /**
   * Gets the hashed passphrase.
   * @return Hashed passphrase.
   */
  public byte[] getHashedPassphrase() {
    return hashed;
  }

  /**
   * Gets the salt of the passphrase.
   * @return Salt.
   */
  public byte[] getSalt() {
    return salt;
  }
}

Finally there is my crypto code:

  /**
   * Encrypts the given content with the given passphrase and configured cipher.
   * @param content Content to be encrypted.
   * @return Encrypted byte array.
   */
  private byte[] encrypt(byte[] content)
          throws NoSuchAlgorithmException, NoSuchPaddingException,
          InvalidKeyException, IllegalBlockSizeException, BadPaddingException,
          NoSuchProviderException, InvalidAlgorithmParameterException {
    return performCrypto(content, Cipher.ENCRYPT_MODE);
  }

  /**
   * Decrypts the given content with the given passphrase and configured cipher.
   * @param content Content to be decrypted.
   * @return Decrypted byte array.
   */
  private byte[] decrypt(byte[] content)
          throws NoSuchAlgorithmException, NoSuchPaddingException,
          InvalidKeyException, IllegalBlockSizeException, BadPaddingException,
          NoSuchProviderException, InvalidAlgorithmParameterException {
    return performCrypto(content, Cipher.DECRYPT_MODE);
  }

  /**
   * Performs the actual crypto operation based on the given mode. <br/>
   * When encrypting, a new random initialization vector will be created, used
   * during encryption and finally prepended to the encrypted data chunk, so
   * it can be retrieved again during decryption.
   * @param content Encrypted or decrypted content.
   * @param mode {@link Cipher.ENCRYPT_MODE} or {@link Cipher.DECRYPT_MODE}.
   * @return Transformed content
   */
  private byte[] performCrypto(byte[] content, int mode)
      throws NoSuchAlgorithmException, NoSuchPaddingException,
      InvalidKeyException, IllegalBlockSizeException, BadPaddingException,
      NoSuchProviderException, InvalidAlgorithmParameterException {
    Cipher c = cipher.createCipher();
    SecretKeySpec key = new SecretKeySpec(Arrays.copyOf(
            passphrase.getHashedPassphrase(), cipher.getKeyLength()),
            cipher.getName());
    byte[] iv = createIV(); // always create iv to be able to get iv 
                            // length in decrypt mode
    int offset = 0;
    int len = content.length;
    if ( mode == Cipher.DECRYPT_MODE ) {
      // read prepended iv from encrypted data
      iv = Arrays.copyOf(content, iv.length);
      // offset by length of iv
      offset = iv.length;
      len -= iv.length;
    }
    c.init(mode, key, new IvParameterSpec(iv));
    byte[] cryptoresult = c.doFinal(content, offset, len);
    byte[] finishedarray;
    if ( mode == Cipher.ENCRYPT_MODE ) {
      // prepend iv
      finishedarray = new byte[iv.length + cryptoresult.length];
      System.arraycopy(iv, 0,
              finishedarray, 0,
              iv.length);
      System.arraycopy(cryptoresult, 0,
              finishedarray, iv.length,
              cryptoresult.length);
    } else {
      finishedarray = cryptoresult;
    }
    return finishedarray;
  }

  /**
   * Creates a {@link SecureRandom} 16 byte sequence which can be used as
   * initialization vector.
   * @return Random byte sequence.
   */
  private byte[] createIV() {
    Random r = new SecureRandom();
    byte[] iv = new byte[16];
    r.nextBytes(iv);
    return iv;
  }

The clear text data that is supposed to be encrypted will be JSON structures.

I'm mostly interested in finding more or less obvious "messups" I have in that code from a security point of view.

I hope this is not too much code at once. I wanted to keep things as unaltered as possible. If it is too much code though, please let me know, then I'll try to strip things down.

If you need access to the complete code: You can access it here: https://github.com/Hydrael/pws/tree/java/pws/de/simperium/pws/backend

Thanks for any feedback

share|improve this question
add comment

1 Answer

The thing that jumps out at me is that you've got an issue with encoding strings. Use getBytes("UTF8") instead of getBytes(). Why? Well, otherwise, the locale of the operating system can affect how a password gets hashed.

I'm going to list possible issues now in no particular order.

You're calling createIV() to find out the IV size. This uses up entropy needlessly. Instead, make a constant called IV_LENGTH and make both the loading functions and createIV() read it.

You probably shouldn't use that particular HEADER_MAGIC constant, since most of the 4 byte value will be zeros.

I don't know what getting a DES3 instance is supposed to do. I assume you're after Triple-DES, in which case you could use a DESede instance.

// I wish there was a direct equivalent to python's join method in Java :/

Me too.

if ( headerVersion < MINIMAL_JAVA_COMPATIBLE_HEADER_VERSION ) {

As I understand this, it will bail out if the file it's reading is too old. Shouldn't it bail if the file is too new?

//@TODO: nullify byte[] clear, as it contains clear text passphrase, which
// we don't want to be swapped or debugged

Kind of irrelevant, since 1) you keep it as a string in other places, and 2) it's a local variable, so it'll be garbage collected soon.

It looks like your hash function is SHA 512. You should run chain together multiple calls to this (like 10,000 or so), because otherwise, somebody with consumer hardware can crack a totally random 8 character password with letters, numbers, and symbols in a few days. Using scrypt would be ideal, but clearly that's more difficult to implement.

share|improve this answer
    
Thanks for your feedback. All your suggestions seem to make sense, so I'll go ahead and implement them. The MINIMAL_JAVA_COMPATIBLE_HEADER_VERSION has historical reasons, since this project has already evolved a bit. Basically I was always able to migrate input files from older versions of my program to match the current file/header layout. With this switch to Java I won't bother with too old files, since it is very unlikely someone will feed the app with files from an early stage of my project...call it being lazy if you want ;) –  Chris Nov 16 '12 at 8:27
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.