[9] RFR(S): 8067648: JVM crashes reproducable with GCM cipher suites in GCTR doFinal

[Zoltán Majó]

Hi Tony,


> On 13 Apr 2015, at 22:09, Anthony Scarpino <anthony.scarpino at oracle.com> wrote:
> 
> Hi,
> 
> Could you forward the whole message, with the patch, to the security list.  I have only received  John's response, but not the webrev. 

please find the original RFR below. I’ve sent it to security-dev at openjdk.java.net at the same time as I did send it to hotspot-compiler-dev at openjdk.java.net. But as security-dev seems to be moderated for non-members, the original message is most likely awaiting moderator approval.

Thank you and best regards,


Zoltan

================

Hi,


please review the following patch.

Bug: https://bugs.openjdk.java.net/browse/JDK-8067648


Problem: On architectures with hardware support for AES operations, the Java version (the version in the JDK sources) of the com.sun.crypto.provides.AESCrypt::encryptBlock(byte[], int, byte[], int) method is replaced with an intrinsic that uses the CPU's AES instructions.

The Java version of encryptBlock operates on arrays of size AES_BLOCK_SIZE=16 and it consequently performs a number of "implicit" checks (e.g., null checks and range checks) as required by the Java VM specification. The intrinsified version of encryptBlock, however, does not perform any of these checks.

Omitting checks results in a VM crash if invalid parameters (e.g., a null pointer, as reported in the current case) are passed to the method.


Solution: The failure reported in the current issue appears in the com.sun.crypto.provider.GCTR class that calls the intrinsified version of encryptBlock. None of the methods of the class are accessible from packages other than com.sun.crypto.provider. So, after private a discussion with John Rose, Vladimir Kozlov, and Roland Westrelin, I propose to solve this problem on the Java-level.

The GCTR::counter field is supposed to be initialized with an array of size AES_BLOCK_SIZE so that it is safe to call encryptBlock. The 'counter' field is never supposed to become NULL during the lifetime of a GCTR object (so that encryptBlock can be always called safely).

The GCTR class supports saving and restoring the value of the 'counter' field (via the save() and restore() methods). For saving/restoring, the class uses the 'counterSave' field as temporary storage.

It is also possible to reset the a GCTR object to its initial state by calling reset(). Reset sets both the 'counter' and 'counterSave' fields to their initial values.

If a call to the method reset() is followed by a call to restore(), the field 'counter' is not restored to its original value, but it becomes NULL. This is an invalid state, because a GCTR object should always contain a valid 'counter' array. This problem has been also described (in part) by Chris Ellis.

https://intrbiz.com/post/blog/development/java_8_aes_gcm_nullpointerexception

This patch proposes to restore the contents of 'counter' from 'counterSave' only if some data has been saved into 'counterSave' before (i.e., counterSave is not NULL). The patch also adds a check to the constructor of GCTR to verify if the length of 'counter' is AES_BLOCK_SIZE. (I checked and JDK code uses this class only with arrays of size AES_BLOCK_SIZE, but it is good if the required size is documented and enforced by GCTR.)

The array to store the output of the encryptBlock method (the third parameter) should be also of length AES_BLOCK_SIZE. That is ensured by the GCTR class (both in the doFinal and update methods). The input and output offsets (the second and fourth parameters) are 0, as required by encryptBlock.


Webrev: http://cr.openjdk.java.net/~zmajo/8067648/webrev.00/


Testing:

- JPRT (both with 9 and 8u), all tests in the testsets hotspot pass;
- JTREG tests in jdk_security[1-4] executed locally with the sources built with --enable-openjdk-only; all tests that pass without the patch pass with the patch as well;
- failure reported in 8067648 can be reproduced with 8u, failure is not triggered with patch applied.


Thank you and best regards,


Zoltan

> Thanks
> 
> Tony
> 
> 
> 
> On Apr 13, 2015, at 12:50 PM, John Rose <john.r.rose at oracle.com> wrote:
> 
>> On Apr 13, 2015, at 4:51 AM, Zoltán Majó <zoltan.majo at oracle.com> wrote:
>>> 
>>> please review the following patch.
>> 
>> Good.  This line has a typo ("encrypBlock" = gang member induction party foul?):
>> +  * AESCrypt.encrypBlock method can be intrinsified on the HotSpot VM
>> 
>> — John