patrickfav / armadillo

A shared preference implementation for confidential data in Android. Per default uses AES-GCM, BCrypt and HKDF as cryptographic primitives. Uses the concept of device fingerprinting combined with optional user provided passwords and strong password hashes.
https://favr.dev/opensource/armadillo
Apache License 2.0
280 stars 52 forks source link

GzipCompressor possible resource leaks #4

Closed ghost closed 6 years ago

ghost commented 6 years ago

Hey, you are not using the try-catch correctly in the compress and decompress method. You should close the streams in a finally block, so they are closed even if there is an exception raised. The stream null check is not required for the compress method.

Example of a correct decompress method for the GzipCompressor class (the gzipInputStream != null should not happen, but theoretically new byte[2048] can throw an OutOfMemoryException before the gzipInputStream instance is created):

    @Override
    public byte[] decompress(byte[] compressed) {
        ByteArrayOutputStream bos = new ByteArrayOutputStream();
        GZIPInputStream gzipInputStream = null;
        try {
            int len;
            byte[] buffer = new byte[2048];
            gzipInputStream = new GZIPInputStream(new ByteArrayInputStream(compressed));

            while ((len = gzipInputStream.read(buffer)) > 0) {
                bos.write(buffer, 0, len);
            }

            return bos.toByteArray();
        } catch (Exception e) {
            throw new IllegalStateException("could not decompress gzip", e);
        } finally {
            try {
                bos.close();
            } catch (IOException ignore) { }

            if (gzipInputStream != null) {
                try {
                    gzipInputStream.close();
                } catch (IOException ignore) { }
            }
        }
    }

I would prefer try-with-resources with auto-close, but to support older Java versions and android, this should do it.

patrickfav commented 6 years ago

Hey Phil, thanks for the fix - will look into it.

ghost commented 6 years ago

Thanks.

There are a few more places where something is cleaned but not if an exception is raised before. For example the secure wipe stuff in the AesGcmEncryption class (here).

patrickfav commented 6 years ago

So most of the issues should be fixed. @JackWhite20 thanks again for pointing them out.