File-encryption utility application

Question

I made a simple file-encryption utility application for a little school project. I'm really interested in cryptography, and tried my best while making it. Can you see it, and give me feedback?

The input is always a FileInputStream (wrapped by a BufferedInputStream), and the output is a ByteBufferOutputStream, which will be written to the output file.

 public class Cryptography {
 private final Cipher c;
 private final MessageDigest md5;
 private final String AES_CBC_PKCS5 = "AES/CBC/PKCS5Padding";
 private final String MD5 = "MD5";
 private final int BLOCK_SIZE = 16;
 private final byte[] bytesIV = new byte[BLOCK_SIZE];
 public Cryptography() throws NoSuchAlgorithmException, NoSuchPaddingException {
 c = Cipher.getInstance(AES_CBC_PKCS5);
 md5 = MessageDigest.getInstance(MD5);
 }
 public void initIV() {
 new SecureRandom().nextBytes(bytesIV);
 }
 public void encryptStream(InputStream in, OutputStream out, byte[] key) throws NoSuchAlgorithmException, NoSuchPaddingException, InvalidKeyException, InvalidAlgorithmParameterException, IllegalBlockSizeException, BadPaddingException, IOException {
 out.write(bytesIV);
 out.flush();
 c.init(Cipher.ENCRYPT_MODE, new SecretKeySpec(key, "AES"), new IvParameterSpec(bytesIV));
 out = new CipherOutputStream(out, c);
 byte[] buf = new byte[1024];
 int numRead = 0;
 while ((numRead = in.read(buf)) >= 0) {
 out.write(buf, 0, numRead);
 }
 out.close();
 }
 public void decryptStream(InputStream in, OutputStream out, byte[] key) throws NoSuchAlgorithmException, NoSuchPaddingException, InvalidKeyException, InvalidAlgorithmParameterException, IllegalBlockSizeException, BadPaddingException, IOException {
 in.read(bytesIV);
 c.init(Cipher.DECRYPT_MODE, new SecretKeySpec(key, "AES"), new IvParameterSpec(bytesIV));
 in = new CipherInputStream(in, c);
 byte[] buf = new byte[1024];
 int numRead = 0;
 while ((numRead = in.read(buf)) >= 0) {
 out.write(buf, 0, numRead);
 }
 out.close();
 }
 public byte[] hash(byte[] key) {
 return md5.digest(key);
 }
 public byte[] getIV() {
 byte[] temp = new byte[BLOCK_SIZE];
 System.arraycopy(bytesIV, 0, temp, 0, BLOCK_SIZE);
 return temp;
 }
}

• Mast: I have rolled back the last edit. Please see What to do when someone answers .

Answer 1

• Exposing the IV only leaves room for errors. Simply generate it at the start of encryption.
• There is no point in having a completely unrelated hash method on this class.
• There is not integrity protection, so an attacker who can modify the ciphertext you decrypt will likely be able to pull off a padding-oracle attack. Unfortunately it's not trivial to combine streaming decryption with integrity protection, since you must ensure that you don't release any plaintext you haven't authenticated.
• Once you eliminate the IV storage, as recommended above, you have no state to preserve across calls, which means you don't need any instance fields anymore. They only make your class thread-unsafe without any benefit.
• Closing out seems pretty weird, I wouldn't expect that behaviour. On the other hand you don't close in.
• If you decide to dispose resources, use try...finally (or try-with-resources) to do so reliably.
• I'd make the class immutable and threadsafe, storing only the key (and algorithm if it's not hardcoded) in it.

• mrkflp: Thank you for your answer but meanwhile I re-created my Cryptography class. You can see it here: github.com/fulopm/filetitok/blob/master/src/filetitok/crypto/…
• CodesInChaos: @mrkflp Most of my points still apply to your updated version.

Answer 2

Some general comments:

• String initialisaion, avoid things like String AES_CBC_PKCS5 = "AES/CBC/PKCS5Padding"; - wrap the "..." in new String( "..." ); Same comment for MD5.
• Variable naming, use lower case as a preference, camelCase with variable names that are multiple words (like you have with method names). Avoid underscores separating words in variable names.
• If you're initialising some things in the Constructor, you may as well initialise them in the variable declaration (variables are generated immediately prior to Constructor calls).
• Your private fields get no benefit from the final keyword, and indeed, you shouldn't have a final IV. Block size probably should be passed into the Constructor (and sanitized). But then, you could also pass the cipher spec at runtime too.
• There's no point generating a new SecureRandom every time you want to initIV() - move SecureRandom to a private field.
• You're propogating an awful lot of exceptions further up the stack. Avoid this. Handle the exceptions in an acceptable way, re-throw exceptions (even custom ones) where necessary.
• encryptStream() doesn't only encrypt. It writes to another stream. Do you need to work with streams here? I'd suggest working with an array of bytes is more appropriate (and allows you to change keys between blocks of bytes at arbitary lengths). Same comment for decryptStream()
• If you really need to read from stream, encrypt and immediately write to another stream, rename the methods to be descriptive. Also, since you always close out, maybe you should close in too? Or maybe you should let the caller decide (by allowing them to specify a boolean argument; or by not doing anything and providing JavaDoc comment stating why you're not closing resources).
• hash() is a poorly named method. Consider something like getKeySum()
• getIV() is a poorly named method, and in it's current implementation is only appropriate if you've encrypted the stream and haven't called initIV() again.
• decryptStream() assumes that the IV is the same as the one generated in initIV() - this gives rise to a fairly trivial bug if I give you a stream that was encrypted with a previously generated IV.

• mrkflp: I'm really grateful to you for your suggestions. I'll implement all of them soon. About the exception handling: I forward all of the exceptions to the GUI, then handle them (log + inform the user about what's happening, in a graphical way). A custom exception (or more), instead of throwing a lot of various is a good idea too, I'll think it over. Thank you again!
• Roland Illig: The suggestion of using new String("MD5") instead of a simple "MD5" is wrong. Just using the latter is great code.
• Dave: Wrong for you maybe, but that's the point of Code Review. In work, my code wouldn't pass a Code Review using the "simple" version. You're explicitly choosing to create a new object. If the fast-initialisation method is required then comments should be used expressing why you don't believe a new object is required. Also, the hash alg. should really be a parameter, as md5 has known hash collision problems.
• 5gon12eder: Your work-place should really re-consider their coding standards. What benefit do you gain from newing up each String explicitly except for actively working against an important JVM optimization and making the code more awkward to read and type? It is also contrary to every other Java coding standard I have ever seen. Robust Java code should never rely on the identity of a String object so neither way of "creating" a String should be "required" for the proper functioning of the code.
• Vogel612: @Dave that's a stupid justification to require the JVM to do needless work and waste memory.... Aside from that, I find your recommendation against using ` final` actively harmful. The suggestion about not reinitializing SecureRandom is actually a security flaw and simply wrong. On the other hand the advice about not using Streams is great and your naming recommendations are nice... I'm currently rather torn, but overall your answer has more good than bad. Just the bad is really bad. Feel free to drop by in Code Review Chat and drop me a note to discuss this further.