[14] RFR JDK-8229243 "SunPKCS11-Solaris provider tests failing on Solaris 11.4"

[Xuelei Fan]

I looked at the latest webrev:
   http://cr.openjdk.java.net/~valeriep/8229243/webrev.01/

Which is good to me.

Thanks,
Xuelei

On 9/19/2019 5:46 PM, Valerie Peng wrote:
> 
> I am not on the PKCS#11 committee and not sure about the plan.
> As for which one is right, I am more inclined to the "spec is right" side which is also what NSS picked.
> Comparing between spec and header, shouldn't the former get more eyeballs in terms of review?
> The header file also has a deprecated structure CK_AES_GCM_PARAMS which contains both ulIvLen and ulIvBits fields.
> As ulIvBits and ulIvLen are essentially same in terms of meaning except bytes vs bits. Having both just creates confusion and potential inconsistency and makes not much sense to me.
> 
> Valerie
> 
> ----- Original Message -----
> From: xuelei.fan at oracle.com
> To: valerie.peng at oracle.com, security-dev at openjdk.java.net
> Sent: Thursday, September 19, 2019 7:47:43 AM GMT -08:00 US/Canada Pacific
> Subject: Re: [14] RFR JDK-8229243 "SunPKCS11-Solaris provider tests failing on Solaris 11.4"
> 
> Will the inconsistency structure be continue?  I was just wondering if
> OpenHSM2/Solaris/NSS will fix the bug and use one structure in the
> future, then we may not need to workaround the issue in the calling side.
> 
> I had a quick look the PKCS#11 3.0 draft, there is no update of the
> structure yet.
> 
> Xuelei
> 
> On 9/18/2019 6:01 PM, Valerie Peng wrote:
>> Ping?
>>
>> Can someone help take a look?
>>
>> Thanks,
>>
>> Valerie
>>
>> On 8/15/2019 4:49 PM, Valerie Peng wrote:
>>>
>>> Anyone has time to help review this fix? PKCS#11 v2.40 has
>>> inconsistent definition for CK_GCM_PARAMS struct. The mechanism spec
>>> (sec 2..12.3) has:
>>>
>>>      typedef struct CK_GCM_PARAMS {
>>>          CK_BYTE_PTR       pIv;
>>>          CK_ULONG          ulIvLen;
>>>          CK_BYTE_PTR       pAAD;
>>>          CK_ULONG          ulAADLen;
>>>          CK_ULONG          ulTagBits;
>>>      } CK_GCM_PARAMS;
>>>
>>> However, the header file pkcs11t.h has an extra "ulIvBits" field:
>>>
>>>      typedef struct CK_GCM_PARAMS {
>>>          CK_BYTE_PTR       pIv;
>>>          CK_ULONG          ulIvLen;
>>>      *    CK_ULONG          ulIvBits;*
>>>          CK_BYTE_PTR       pAAD;
>>>          CK_ULONG          ulAADLen;
>>>          CK_ULONG          ulTagBits;
>>>      } CK_GCM_PARAMS;
>>>
>>> Some vendors such OpenHSM2 and Solaris go with the header file while
>>> some vendor such as NSS go with the mech spec. Current SunPKCS11
>>> provider impl works with NSS but not with other vendors whose
>>> CK_GCM_PARAMS struct contains ulIvBits field. Based on testing
>>> results, OpenHSM2 and Solaris error out at init time when the
>>> parameter length does not have their expected value. Thus, one way to
>>> workaround this inconsistency is to re-try with a different structure
>>> for AES GCM when the init failed. In addition, given the parameters
>>> contains Iv and AAD which some vendor may use during subsequent
>>> enc/dec operations, I changed to use the same model as RSASSA-PSS to
>>> defer the freeing after the enc/dec operation is finished. Verified
>>> the changes on Solaris 11.4 against existing GCM regression tests.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8229243
>>>
>>> Webrev: http://cr.openjdk.java.net/~valeriep/8229243/webrev.00/
>>>
>>> Thanks,
>>> Valerie