CipherInputStream for AEAD modes is insecure (GCM, etc.): ciphertext tampering without detection possible

[Florian Weimer]

On 03/04/2014 08:49 AM, Philipp Heckel wrote:
> Although Tim and Matthew already mentioned the main points, I'd like to
> voice my concerns as well -- in particular because I think that this is
> *not* a philosophical argument: Security must always be more important
> than the supposedly correct semantics of a class or method.

You can't increase security by implementing incorrect semantics, you 
just get something that is broken.  Your complaint about buffering shows 
that. :-)

> The whole purpose of Input/OutputStreams is to be able to avoid large
> buffers and/or temporary files of any sort. For modes in which the tag
> is appended to the ciphertext (including GCM), buffering all the data is
> simply unacceptable, since if would make processing of large files
> simply impossible.

The problem is that you cannot, in general, implement non-chunked 
integrity checks securely by returning unverified chunks and throwing an 
exception once you reach the end and detect that you've returned bad 
data somewhere along the way.

> Using the Cipher class directly is not an option either, because many
> applications nest streams into one another. Chaining gzip+encryption is
> not uncommon. If you process large amount of data, again, manually
> unpacking/packing is not feasible.

And it's also not uncommon that you don't want to expose your 
decompression code to unsigned data, so AEAD without chunking is just a 
poor choice here.

>     Thus, CipherInputStream class ignores AEADBadTagException isn't
>     really the problem here, as the some of the decrypted data may have
>     been returned to the caller before the tag is verified. Whether
>     AEADBadTagException is ignored or not doesn't matter here.
>
>
> Adding to what Matthew said: I think it does matter. Security-related
> exceptions should never be hidden from the developer!

Except when you have to do this to avoid creating an oracle.  Since 
CipherInputStream is used with other ciphers, this might have been the 
reason why the cryptographic-related exceptions are swallowed.

That being said, adding a warning that this happens to the 
CipherInputStream documentation seems prudent.

-- 
Florian Weimer / Red Hat Product Security Team