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

[Xuelei Fan]

I think the implementation of GCM cipher should respect the
specification of GCM mode.  Before authentication tag get checked, no
plaintext should be released to application. See section 7.0 of GCM spec
(NIST SP-800-38D).

Xuelei

On 3/5/2014 5:56 AM, Philipp Heckel wrote:
> 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
>