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

[Philipp Heckel]

Thank you all for your comments. I must say this is a very interesting
discussion. And I really hope that you keep an open mind when looking for a
solution -- I am certainly trying to do the same!

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


*Semantics*
That is very true. Maybe it's not entirely clear to me what the desired
semantics of the class are, so please excuse my comment about the
"supposedly correct semantics". Here's what I understand from the JavaDoc
[1]. Please correct me if this is wrong or some points are missing:

1. The given Cipher object must be fully initialized
2. The read() methods in CipherInputStream must only return data processed
by the underlying Cipher
3. The CipherInputStream adheres strictly to the semantics of
FilterInputStream and InputStream
4. The CipherInputStream catches all exceptions not thrown by its ancestor
classes

*Interpretation*
ad 1) Obviously a Cipher must be initialized for decryption. However, there
are no constraints on what ciphers are supported. In particular, there is
no mention authenticated encryption modes not being supported.

ad 2) Also quite obvious for regular non-authenticating modes. Again, no
mention of returned bytes having to be authenticated or AE at all.

ad 3) FilterInputStream and InputStream have no special requirements with
regard to how encrypted data is processed.

ad 4) In my humble opinion, this is the only debatable comment, because if
the JavaDoc is followed strictly (and blindly so), one must simply "ignore
all exceptions other than IOException" -- not allowing the propagation of
any of the mentioned exceptions (AEADBadTagException, BadPaddingException,
IllegalBlockSizeException) to the application. For non-AEAD modes, this
might make sense (as Florian mentioned: avoiding oracles), for AEAD modes,
it does not necessarily make sense.

*Gaps*
Here's what I think is wrong with this:

a. Authenticated encryption has not been considered when the JavaDoc was
written; Re-thinking the CipherInputStream concepts with AE in mind is
probably not a bad idea. And if that means having to slightly adjust the
semantics so that they make sense for AEAD modes as well, why not? As long
as it doesn't break any of the other ciphers/modes, of course.

b. Developers would not expect this behavior -- even if there was a warning
in the JavaDoc. Implementing encryption correctly is hard enough, this just
makes efficient usage of AEAD 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.


I understand. However, that's how GCM is works: tag at the end, no
"chunk-tags". Unless we what to change GCM, unverified data has to be
returned to make streaming work at all, right? Or how else would one
securely implement GCM for large files? The only options are to (a) buffer
everything (which defeats the purpose of streaming), or to (b) create temp.
files which is very inefficient.

IMHO: I think it's much, much better to return unauthenticated data first
and throw an exception when the stream is closed (+ add a warning in the
JavaDoc) -- than to not authenticate data at all or buffer it (in RAM or on
disk).

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.


See above.

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.
>

It might be really interesting to see if possible oracles were indeed the
reason. Because right now, we don't actually know what the reason was --
maybe there was none and it's just a wording issue in the JavaDoc ...

If that was indeed the reason, then there are two options:
1. Re-throw a generic "new IOException()" -- thereby swallowing the actual
reason, but still considering AE
2. Follow Bouncy Castle's lead: Create a new constructor for AEAD ciphers
and consciously treat them differently

Just my two cents.
Sorry for the long mail. I tried to keep it short. But I find this very
important.

Best regards,
Philipp
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20140304/b8c4b705/attachment.htm>