Code Review Requests for 8012900: CICO ignores AAD in GCM mode

[Valerie (Yu-Ching) Peng]

The purpose of the buffering is for security or conforming to the spec 
as far as I am concerned.
We can't do the buffering in CICO classes as JCE Cipher class can be 
used directly.
It seems best to handle this in the internal GCM implementation classes 
since CipherCore is somewhat general and I'd like to avoid the 
potentially-extensive (isGCM) check if we were to add the handling at 
the CipherCore level.

As for the tag being appended to the cipher text, I think that's 
somewhat an industry convention for GCM impl, at least Solaris native 
Ucrypto library also assumes that the tag is appended after cipher text.

It's true that some existing code have problem handing GCM due to its 
different nature, i.e. AEAD cipher which has AAD and generate/verify 
tag. We can only do our best to fit GCM into the current model. When 
people start using GCM and encounter problems, they'd have to fix 
problems in their code accordingly. Nothing we can do on our end.

Once we agree on the buffering is necessary, then we can see how to 
minimize the performance impact, e.g. perhaps place an upper limit on 
input size, etc.
Thanks,
Valerie

On 07/04/13 06:20, Xuelei Fan wrote:
> Hi Valerie,
>
> CC Brad.
>
> We start to run into the pain to get authentication tag appended in the
> tail of cipher text in our design.
>
> I looked back the discussion when we designed the APIs for GCM cipher in
> JDK 7.  I found that when we run into GCM mode cipher operations, we may
> need to update the source code here or there because GCM cipher
> operation is a lot different than the old cipher modes.  For example, in
> JSSE, we have to update the cipher operations to accept GCM mode ciphers.
>
> I two major concerns about buffering all cipher text in the underlying
> crypto library. One is about the performance impact, another one is
> about compatibility issues.
>
> When the size of cipher text/data is huge or the application is running
> in a heavy loaded system, there is a big impact on memory load and may
> impact the performance a lot.  Even if we have to buffer the data in
> some special context, it would be better to do it in the particular
> context, but not in the crypto library level.
>
> There is two issues about compatibility.  One is about how to use the
> Cipher.getOutputSize().  Yes, it is supposed to call this method before
> doing any update operation.  But in practice, I think there is a lot
> applications that does not call this method. For example, in JSSE, it is
> supposed that the output size is not too much longer than the current
> input data length.  In this fix, the application would have to heavily
> depends on the Cipher.getOutputSize() because the cipher text is
> buffered.  And the return value of Cipher.getOutputSize() may be huge,
> which will prevent the reuse of memory (for example, ByteBuffer and
> array).  Here is another performance impact.
>
> Another issue about compatibility is about the return value of
> Cipher.update().  In the past, it may be always can return a positive
> value if the input data is big enough (bigger than the block size).  For
> example, in JSSE provider, we have code:
>
>      int newLen = cipher.update(buf, offset, len, buf, offset);
>      if (newLen != len) {
>          throw new RuntimeException("Cipher buffering error " +
>              "in JCE provider " + cipher.getProvider().getName());
>      }
>
>      int newLen = cipher.update(dup, bb);
>      if (newLen != len) {
>          // catch BouncyCastle buffering error
>          throw new RuntimeException("Cipher buffering error " +
>              "in JCE provider " + cipher.getProvider().getName());
>      }
>
> If I'm correct, the above code will run into problems with this fix,
> because the return value the Cipehr.update() for GCM will return 0.  I
> believe it is common in practice.
>
> Backing to this bug, if you want to buffering, I would suggest to doing
> the buffering in CI/CO classes rather than in GCM cipher implementation
> (performance issue is still a concern of mine, but the scope of the
> impact is limited).  I think it is accept to me that we only checking
> the authentication tag at the end (CI/CO.close(), with IOException to
> catch authentication tag checking failure).  The data may have been
> write to the I/O, therefor, applications who want to use GCM and CI/CO
> may need to take additional actions to handle the IOException.
>
> Xuelei
>
> On 6/12/2013 7:16 AM, Valerie (Yu-Ching) Peng wrote:
>> Xuelei,
>>
>> Here is another GCM and CipherInputStream/CipherOutputStream related
>> fix, i.e. for
>> 8012900: CICO ignores AAD in GCM mode
>>
>> The key changes are in CipherCore.java, GalorisCounterMode.java, the
>> rest files only have minor changes.
>> Essentially, when using AES/GCM cipher in decryption mode, the data will
>> be buffered and processed AFTER the tag has been verified. Otherwise,
>> most of the recovered text would be returned even if tag verification
>> failed later.
>> Given that CipherCore is shared by most modes, this particular buffering
>> is done inside the GCM impl. But then some more methods have to be
>> added/modified slightly so CipherCore will include this additional
>> buffering from the underneath level in its output size calculations, etc.
>>
>> The webrev is at: http://cr.openjdk.java.net/~valeriep/8012900/webrev.00/
>>
>> Thanks,
>> Valerie
>>
>>