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

[Xuelei Fan]

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