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

[Valerie Peng]

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