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

[Zoltán Majó]

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