search

taoensso / nippy

The fastest serialization library for Clojure
https://www.taoensso.com/nippy
Eclipse Public License 1.0
1.04k stars 60 forks source link

Decryption result is not checked before running decompression #101

Closed obohrer closed 5 years ago

obohrer commented 6 years ago

Hi,

Thank you for creating nippy. It is a great library which works quite well for us apart from a recent issue we encountered :

We are experiencing an issue with nippy when trying to thaw a byte-array which has been encrypted with a different encryption key.

The 5th byte of the decrypted payload contains the len-decomp but if the wrong encryption key was used this length can be huge, resulting in a java.lang.OutOfMemoryError: Java heap space. We expect nippy to throw an exception but not blow up the JVM.

Do you think a new version of nippy could check the decryption result before trying to decompress it ? (For example by verifying a magic series of bytes after the NPY header ?)

ptaoussanis commented 6 years ago

Hi Olivier,

What compressor are you using- Snappy or LZ4? Reason I'm asking: this was a known issue with Snappy, and one of the motivations for switching from Snappy to LZ4 as the default Nippy compressor.

obohrer commented 6 years ago

Hi Peter, yeah we are using LZ4 like this for encryption/compression :

(nippy/freeze {:compressor taoensso.nippy.compression/lz4-compressor
                     :password   [:cached password]})

So it seems there is the same issue with LZ4. I think this issue might not be resolved just by changing the compression but by checking that the decryption was successful.

obohrer commented 6 years ago

Any news on this issue ? How easy do you think such change would be ?

If I understand correctly we could add a new encryptor-id like :aes128-sha512-withcheck or something similar which would encrypt a magic byte/bytes alongside the actual payload and use it to verify the decryption result ?

ptaoussanis commented 6 years ago

Hi Olivier,

Thanks a lot for bringing this to my intention, this is something that should be fixed. Haven't had an opportunity to think about this yet. Some kind of hash/checksum would certainly be one option.

Haven't had much time for open source recently, but will make this my next priority when I have some free time :+1:.

ptaoussanis commented 5 years ago

Hi @obohrer, I've just pushed [com.taoensso/nippy "2.15.0-alpha9"] to Clojars which switches Nippy's default encryption scheme from AES-CBC to AES-GCM.

As you'll see in the relevant commit, this should help address the problem you're seeing.

I'll note: the integrity/tag check will only apply to newly written/frozen data.

Hope that helps?