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

[Matthew Hall]

I think the RFC might require that the auth tag gets checked first before any data from the block gets released to the application, to prevent performing any processing on any data that turns out to be insecure.

Matthew.
-- 
Sent from my mobile device.

Xuelei Fan <xuelei.fan at oracle.com> 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
>> 
>>