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;
    }
}

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.

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.